Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support RMT Rx for data with length greater than RMT channel RAM #3052

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

alexstephanus
Copy link

@alexstephanus alexstephanus commented Jan 28, 2025

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

I haven't added examples or updated the migration guide since there are some design concerns that I'd like to address before merge anyways.

Description

Allows use of the RMT peripheral to read out more data than fits into RMT device memory. It waits for the device to fill up each half of the RMT memory buffer before writing that data out to the passed-in memory. This is similar to how SingleShotTxTransaction works.

This change is not implemented for async Rx, because it's timing-sensitive enough that it needs to operate synchronously.

Concerns

This breaks the previous "fire and forget" behavior that RMT Rx had, where you could tell the RMT receiver to listen for enough data to fill up the buffer, do something else for a while, and then check back and read the data back out. It can still be fire and forget if you're only reading enough data to fill up half the memory buffer or less, but there are scenarios where someone could be using with that previous understanding and this messes them up. This is the current state of the Tx behavior, so I've kept the Rx behavior in line with that.

Generally, I'm not sure that this (and the analogous function on the Tx side) should operate using "start an Rx transaction" and "wait for the Rx transaction to complete" as separate steps. Given that it needs timing-sensitive CPU access to accurately read in data, it seems like giving callers the opportunity to split out the starting and supervising of a transaction is asking for trouble. In my opinion, the "start/wait" thing makes sense if you're receiving (or transmitting) a small enough amount of data that it fully fits in the RMT RAM buffer -- since in that scenario you can have the peripheral run autonomously and simply check back in when you need the data.

It might make sense to have something like RxChannel.receive_fire_and_forget(&mut rx_data) which creates an RxTransaction that you can call .wait() on later to retrieve the data and get the channel back, and RxChannel.receive_synchronously(&mut rx_data) which runs the entire transaction synchronously once it's called. In this scenario, receive_fire_and_forget would have the existing limitation of rx_data.len < constants::RMT_CHANNEL_RAM_SIZE, and receive_synchronously would allow buffers of arbitrary length. These names are off the top of my head, but hopefully they get the point across.

Testing

Local testing: I've tested it locally on an esp32s3 using the same code from the test. Additionally, I've tested that early termination (via the receiver going idle) works by using a modified version of TxChannel::transmit_continuously_with_loopcount that terminates after x loops rather than simply raising the interrupt.

HIL test: I've added a HIL test that sends and receives a larger amount of data than fits into the buffer, using TxChannel::transmit_continuously_with_loopcount. I can't add a HIL test for early termination without modifying transmit_continuously_with_loopcount, which introduces some additional questions about how to manage different functionality on chips.

esp-hal/src/rmt.rs Outdated Show resolved Hide resolved
*entry = unsafe { ptr.add(idx).read_volatile() };
// Stops us from writing garbage data if the transaction
// finishes early due to the receiver going idle
unsafe { ptr.add(idx).write_volatile(0); }
Copy link
Author

@alexstephanus alexstephanus Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reticent to add this extra instruction to a pretty tight loop, but if we don't have it then the buffer can receive some garbage data after the final zero-length entry (everything from the final entry to the next multiple of RMT_CHANNEL_RAM_SIZE / 2). That seems like it could be fine behavior, since the caller probably needs to check for the zero-length entry anyways. Just want to call it out

@alexstephanus alexstephanus marked this pull request as draft January 28, 2025 20:50
@alexstephanus alexstephanus marked this pull request as ready for review January 29, 2025 23:28
esp-hal/src/rmt.rs Outdated Show resolved Hide resolved
hil-test/tests/rmt.rs Outdated Show resolved Hide resolved
<C as RxChannelInternal>::reset_threshold_set();

// load new data into memory buffer
let ram_index = (((index - constants::RMT_CHANNEL_RAM_SIZE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this calculation, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants