Esp-idf: [TW#12112] RMT TX memory buffer support assumes direct copying only

[Makuna]

Due to the verbose format of the RMT data (32 bits for a single pulse); this leads to extremely large buffers even though there is a copy stage from the memory buffer to the RMT channel buffer.

The interface should expose a way for the application to insert a translator so that native memory format of the data can be translated on the fly to fill the RMT send buffer. This translator would be called from the ISR as it does today; with a default translator that just copies.

My specific application is for NeoPixels where the memory is often VERY large if kept the RMT format. But it is important for the RMT to be shared for other support like IR receiver so overriding the ISR for just one purpose would be problematic.

[devsaurus]

My specific application is for NeoPixels

I came across the very same challenge and would second the enhancement to have something like a user hook function being called form the standard ISR. Consumes data from the existing tx buffer and converts it into required rmt items.

My solution so far was to craft a shared ISR which manages the channels which run on the "proprietary" format. It eclipses the driver's ISR for the involved THR_EVENT interrupts while all other IRQs fall through to the driver.

[igrr]

What kind of an API would you like to see for this? Reading the issue description, I assume something like an extra pair of arguments in the rmt_tx_config_t structure?

size_t bits_per_sample;   //!< Number of bits per sample in the provided memory block
rmt_item32_t (*sample_conv_cb)(const uint32* buf, size_t offset_bits);    //!< Function which converts bits_per_sample bits at buf + offset_bits into the rmt_item32_t sample

[Makuna]

I will get back to you, I was actually working through how best to implement it. All due to "thinking" about providing the rmt_ methods as an object for Arduino that provided both this and issue 514

[Makuna]

I think translation should not limit the type of work that needs to be done. So its not just about a simple bit to RMT format translation. It could be application specific translation as long as its kept simple. The reasoning I use for this is the following example. For NeoPixels, the actual data sent varies by LED driver chip, sometimes it GBR (Green Byte, Blue Byte, Red Byte), sometimes its RBG or even other byte orders. But it can be very handy for the consumer to always work in the same standard format like RGB and then have it translated as it is sent out.

So here is a first pass, but by no means fully threshed out or correct syntactically named.

So extending the rmt_config_t to include translate call back and a generic translate argument for identity.

typedef void(*TranslateCallback)(void* arg, TranslateContext* context);

inside rmt_config_t we add something like

void* translateArg; // supplied by the consumer that points to what ever they want to be passed to the translate callback; I will use this to store a this pointer to a instance class 
TranslateCallback translateCallback; // the translator provided by the consumer

Then we expose a new method for writing these custom buffers that must be translated rather than copied

esp_err_t rmt_write(rmt_channel_t channel, const uint8_t* buffer, size_t bufferSize, bool wait_tx_done);

as the ISR needs the rmt write buffer filled, it calls the translate function with a context of the translation

struct TranslateContext
{
  const rmt_item32_t* dest; // the rmt formatted buffer to translate into
  const int destItemCount; // the number of items that should be placed into it (note, a terminating item has zero period length by rmt standard
  const uint8_t* source; // source buffer, as passed to rmt_write
  const size_t sourceSize; // size of source, as passed to rmt_write
};

The translator routine will do its work, updating what ever progress it has made through the source buffer by using the argument passed into the rmt_config_t and further supplied by the arg passed through to the translate callback.
NOTE: Progress maybe sub byte; there is no guarantee that a full byte of native data can be fully expressed into the rmt formatted data.

[igrr]

Calling arbitrary user code from ISR context is something we try to avoid in ESP-IDF drivers, when possible.

What we can do however is create a queue (or a ring buffer) which the application will use to feed rmt_item32_t type data into the transmit routine. ISR will get values from the queue and write them into TX buffer. Application can create a task which will do blocking writes to the queue; once ISR receives some data, a task will be unblocked and will have a chance to generate more data.

This way the task can do any required processing of data, without affecting high priority task latency and without risking stack overflow in ISR context.

Ringbuffer can offer smaller storage overhead compared to a queue. It is already used in the RX path, so using it in the TX path will not introduce new concepts for API users.

[devsaurus]

This reads like a clever approach, especially since you could still encapsulate feeding the TX queue/ringbuffer inside rmt_write_items().
In any case, it would allow full control over the data conversion while users need to size the queue/rb big enough to avoid starvation. Guess that's feasible from an application's point of view.
👍

[Makuna]

A bonus with the intermediate queue is that then the count of items to write can favor the number of bits the consumer needs to write to simplify tracking of its progress; so it can keep to a byte boundary (even if its translation this turns into 11 bits to output). Then with this, you can add another value to the translate context for progress like...

   uint8_t* sourceProgress; // pointer into source buffer that tracks progress, writeable by the translate callback and initialized to the same as source when first called.

Further, why not just have the task abstracted into the driver also, so the callback while not called directly from the ISR has the same feel? Further have the task have an affinity to being run on the same core as the one the write was called on.

[Makuna]

Any updates on this?

[Makuna]

Ping?

The importance of this has increased, as the bitbang for NeoPixels doesn't work (under the Arduino projects) since the main task is still interrupted causing timing problems. Using Rtos documented task methods to block interruptions don't work either; meaning the only real solution is RMT.

[ajvpot]

Is this issue still being tracked? Seems like the RMT can't be shared at all without this change.

[mdvorak]

I'm also very interested in this, at least a statement, whether it will be implemented and when, would be nice.
Thanks

[HorstBox84]

@igrr Any comment on this?

[sticilface]

bump. pls fix.....

[chrisella]

bump again, this issue is holding back various other libraries.

[penfold42]

+1 from me too

[Redferne]

+1

[Jorgen-VikingGod]

+1 pls fix this if possible

[piamay]

+1

[Tobias-Ki]

+1, please fix this as many other projects need this fix

[merlinschumacher]

+1

[niffumau]

+1

[michi1g]

+1

[jwd83]

+1

[tbreuer]

+1

[veitk]

+1

[31337Ghost]

+1

[niffumau]

cat /dev/request > /dev/null; rm -rf /

[mcer12]

Any statement at all or is this issue abandoned?

[sticilface]

It would be nice to have some comment from someone at espressif ?@igrr. Were approaching a year old issue...

[igrr]

Sorry for the lack of follow-up everyone! This task seems to be still assigned to a developer who is no longer working for Espressif. Reassigning and increasing the priority.

(Keep in mind that Chinese new year holiday is approaching, so the resolution will not be immediate)

[costaud]

fixing

[penfold42]

Thanks !

[jwd83]

Glad to finally see some traction here!

[31337Ghost]

@igrr @costaud does there are some progress?

[igrr]

Yes, there is a change to RMT driver in review at the moment, but it needs some improvement.

[Makuna]

Its going to be complex to get right; glad to see its being worked on.

[vtjballeng]

Really looking forward to a resolution on this! Glad to hear it has been moved up in priority order.

[Buffalchill]

+1

[nednil]

+1

[steminabox]

+1

[igrr]

@Makuna @devsaurus Could you please have a look if https://github.com/espressif/esp-idf/commit/35256c375b76a058846b3f5d839959f0a8735ae8 satisfies your use case?

[Makuna]

I will be reviewing it over the next few days.

[devsaurus]

Thanks a lot for the update on this @igrr! I gave https://github.com/espressif/esp-idf/commit/35256c375b76a058846b3f5d839959f0a8735ae8 a try and reworked my ws2812 code to comply with the new translator logic. It worked right away, but I face restrictions regarding real-time requirements of my use case.

First, I observe buffer underruns when the translated rmt items span more than a single memory block. There is an inherent race condition between sending the RMT buffer content and refilling tx_buf. I suspect that the time to send tx_sub_len items is shorter than the sum of handover from ISR to the application task via tx_thr_sem and translation of the next rmt items.
The duration for an rmt item in my use case varies between 1.1 ms and 1.4 ms, which sums up to 35.2 ms to 44.8 ms. The latency of the translation mechanism seems to be higher than this (CPU @ 160 MHz, RTOS tick rate 100 Hz).

The suspected race condition above is in fact more fundamental: L626 is missing

                            if(HPTaskAwoken == pdTRUE) {
                              portYIELD_FROM_ISR();
                            }

Second, the translator logic allows only for a single channel to be served per task as far as I understand: rmt_write_items() loops until almost all data is converted. I guess that parallel operation of multiple channels would require sample_to_rmt_t fn to be called directly from the ISR. This might also solve the race condition described above.

PS: L826 should be an ESP_LOGD :wink:

[Makuna]

@igrr Anyway for you to provide the binaries for these changes so I can try them? The local installed board support does not contain the c files so I can't just copy over.

Q: Is there a way to query for an unused RMT channel or even check if a channel is already being used? So that a library could obfuscate which channel from the user and just "pick" an available one?
Q: I am confused to how to use translation and not wait (I think this is what @devsaurus is getting at also). I believe I should be calling rmt_write_sample when I want translation, but if I call this with wait_tx_done false, then inside the ISR it will be checking for the translator flag which is clear before rmt_write_sample returns.

[Makuna]

More Feedback:
How does a library writer use this if their object can have multiple instances? The translate function has to be a static; and further there is no PVOID arg for a context passed through it. Currently, it would require the library to have ONE static function, and use a memory MAP of the channels to instances; which can be done. BUT, each library (object type) would have to do this, so each would require a map of all channels. A better solution is that the RMT include a single PVOID context in it (so its not duplicated for each library) and then pass this into the translate function or all callback functions.

[stealeks]

Any update here?

[jwd83]

Hey there, just checking in to see how the neopixel rmt support was coming along. :)

[stealeks]

@igrr, please notice us :)

[MichMich]

Any update? This would be great! Thanks!

[koobest]

Hi, all.
Thank you for your feedback and I'm sorry for the later reply because I have been occupied by other things recently. we are dealing with this and will update the RMT driver in this week.

@Makuna
Q: Is there a way to query for an unused RMT channel or even check if a channel is already being used?
Is "unused channel" means a channel that has not been initialized or an initialized but idle channel? I think it is really necessary to provide this API.

thanks !!

[koobest]

Hi, we have the rmt.c updated. please take a look. any suggestion, we will be greatly appreciated.

https://github.com/espressif/esp-idf/blob/feature/rmt_translator/components/driver/rmt.c

thanks!!.

[r1dd1ck]

@koobest The latest commit starting to look good already :-)

A couple of suggestions though..

• channel auto-select (@rmt_config)
        _auto-assign free/unused channel when_ config.channel == NULL

• custom bit values for translator/sample_to_rmt (@rmt_translator_init)
        rmt_obj_t → (rmt_item32_t) bit0, bit1

• TX initiator/terminator for translator/rmt_write_sample (@rmt_translator_init)
        rmt_obj_t → (rmt_item32_t) initiator, terminator

plus corresponding setter/getter functions, like:

esp_err_t rmt_get_channel_free(rmt_channel_t* channel);
esp_err_t rmt_get_channel_idle(rmt_channel_t* channel);
esp_err_t rmt_set_bit_pair(rmt_channel_t channel, rmt_item32_t bit0, rmt_item32_t bit1);
esp_err_t rmt_set_initiator(rmt_channel_t channel, rmt_item32_t initiator);
esp_err_t rmt_set_terminator(rmt_channel_t channel, rmt_item32_t terminator);

I believe I have a fairly good reasoning for all these to be implemented. Can explain in more detail, if needed.

Thank you for all your efforts and keep up the good work! :-)

[koobest]

Hi, @r1dd1ck
Thank you for your suggestions。
Can you provide more details on why you need these four APIs? : -)

esp_err_t rmt_set_bit_pair(rmt_channel_t channel, rmt_item32_t bit0, rmt_item32_t bit1);
esp_err_t rmt_set_initiator(rmt_channel_t channel, rmt_item32_t initiator);
esp_err_t rmt_set_terminator(rmt_channel_t channel, rmt_item32_t terminator);

thanks !!

[r1dd1ck]

@koobest

• initiator / terminator
  For use cases where a certain pulse (or pulse sequence) is required to be prepended/appended to the stream to initiate/terminate the transmission, and the pulses have different timing characteristics than those used for the translate function, which means there is no way to replicate them by simple inclusion to the src stream.
   
  You can argue that one could utilize rmt_tx_end_callback and simply do a rmt_write_items before/after rmt_write_sample to get those required sequences included, but this approach - apart from being rather cumbersome - also introduces signal discontinuity which skews the timing and could be rather detrimental to transmissions with tight timing requirements.
   
  Therefor it would be very practical if pointers to rmt_item32_t arrays could be added to each channels rmt_obj_t objects, which -if set- would be automatically incorporated into the stream for every rmt_write_* transmission.

[r1dd1ck]

@koobest

• exclusive lock on channel
  Example: Library(LED) gets initialized, picks rmt_channel_0 and begins transmission. Library(IR) intializes, and as there is no API provided to check whether a channel has already been taken (initialized or being used), it also picks rmt_channel_0 and calls rmt_config on it - essentially resetting all the channel's registers while in middle of a transmission (initiated by the LED library). None of the functions/methods provided actually check for this. Wonderful.
   
  Currently there is no simple way to avoid these collisions, so an option to acquire an exclusive lock on a channel would be a much appreciated feature.

[koobest]

Hi, @r1dd1ck,
The details you provide is very useful for us, thank you very much. we will add an API to get the status of eight channels. But for initiator / terminator, can this be implemented in the user callback(sample_to_rmt function )?

   sample_to_rmt()
   {
    ....;
   }

    rmt_translator_init(sample_to_rmt);
    rmt_write_sample(...);

because the sample_to_rmt callback is implemented by our users, so we can package the data.

thanks !!

[r1dd1ck]

@koobest

we will add API to get the status of channels

I hope you don't mean just the status register, because there is already an API for that: rmt_get_status() which apart from being entirely undocumented, also seems to be of only very limited use to the issue at hand, as it appears to hold only BUSY and PROGRESS states.

But for initiator / terminator, can this be implemented in the user callback(sample_to_rmt function)?
because the sample_to_rmt callback is implemented by our users, so we can package the data.

If there was an easy/simple way, I would not have to write all these requests. I think the question should not be whether some approach is possible, but rather whether it is optimal/feasible.

At the moment I'm not aware of any way for sample_to_rmt to know that the current call is the first (beginning) of a transfer. There is a possibility that pulling data from the (undocumented) channels status register (progress bits) could be used for this, but I still have to evaluate. Handling cases where the initiator/terminator sequence needed to be longer than tx_sub_len number of items could also prove to be tricky.

Either way, this would still be considered a "hacky" solution, and I don't think that finding "hacky solutions" is (or should be) the aim of these discussions. I believe I don't have to explain how much easier it would be if one could just set the comms speciffics at channel/translator init and be done with it, instead of inventing sub-optimal workarounds to make it work.

Please don't forget to also consider the exclusive lock request as it can provide a very simple way to solve alot of collision cases.

When I get some more time on my hands I will write up an explanation regarding inclusion of the translator bit/pulse pairs into rmt_obj_t. Although that is more of an ease-of-use/optimization case, I think it is still worth a consideration.

Have a nice day :-)

[FayeY]

Please have a try with the latest IDF. The feature has been added. Thanks for your interest in ESP32!