RMT: Use new Encoder trait for Tx data #4604
Draft
+999
−195
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
in anticipation of adding another, user-visible type which will be named RmtWriter This type is private, so no changelog or migration guide entry required.
which was probably of little value, anyway, and also in preparation for adding more sophisticated Encoder data types
…writer.state read-only
Previously, we stored &mut dyn Encoder and dynamically dispatched the Encoder::encode method. Now, we store &mut dyn EncoderExt and dynamically dispatch EncoderExt::write with the expectation that Encoder::encode should be inlined in EncoderExt::write (which is the only caller, and its Encoder implementations in esp-hal are also marked as #[inline(always)]). This might allow for small optimizations, since the RmtWriter type will typically not need to be constructed on the stack, but can be kept in registers.
to ensure that the Encoder-related refactoring didn't break anything
&mut dyn EncoderExt is a fat pointer to the data and the vtable (likely in flash), but we only need a single entry of the vtable. Thus, implement our own pointer type, which will avoids the indirection via the vtable.
Contributor
Author
|
Would you mind granting me HIL access here? Thanks! |
NonNull::from_mut is new in 1.89
bugadani
reviewed
Dec 2, 2025
Comment on lines
+438
to
+456
| #[derive(Clone, Debug)] | ||
| pub struct IterEncoder<D> | ||
| where | ||
| D: Iterator<Item = PulseCode>, | ||
| { | ||
| data: D, | ||
| } | ||
|
|
||
| // If the input data was not exhausted, update offset as | ||
| // | ||
| // | initial | offset | max_count | new offset | | ||
| // | ------- + ----------- + ----------- + ----------- | | ||
| // | true | 0 | memsize | 0 | | ||
| // | false | 0 | memsize / 2 | memsize / 2 | | ||
| // | false | memsize / 2 | memsize / 2 | 0 | | ||
| // | ||
| // Otherwise, the new position is invalid but the new slice is empty and we won't use the | ||
| // offset again. In either case, the unsigned subtraction will not underflow. | ||
| self.offset = memsize as u16 - max_count as u16 - self.offset; | ||
|
|
||
| // The panic can never trigger since count <= data.len()! | ||
| data.split_off(..count).unwrap(); | ||
| if data.is_empty() { | ||
| self.state = WriterState::Done; | ||
| impl<D> IterEncoder<D> | ||
| where | ||
| D: Iterator<Item = PulseCode>, | ||
| { | ||
| /// Create a new instance that transmits the provided `data`. | ||
| pub fn new(data: impl IntoIterator<IntoIter = D>) -> Self { | ||
| Self { | ||
| data: data.into_iter(), | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Do you actually need anything more than this? Copying from a slice, or converting from a bitstream can both be expressed as an iterator. Wouldn't it be better to not introduce a whole subsystem for something that could be formulated in user code with common enough Rust machinery?
wisp3rwind
force-pushed
the
rmt-encoder-v3
branch
from
b04afae to
36c5264
Compare
bugadani
added
the
trusted-author
[HIL trust list]Trusted users for this PR (click to expand) |
|
Author @wisp3rwind was trusted for this PR via the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
This changes the input data type for RMT Tx methods from
&[PulseCode]to&mut impl EncoderwhereEncoderis conceptually similar toIterator<Item = PulseCode>, but allows for more efficient code in many cases.... (TODO: add more details on how I got here)
Cf. also the IDF encoder type: https://docs.espressif.com/projects/esp-idf/en/latest/esp32c3/api-reference/peripherals/rmt.html#rmt-rmt-encoder
Testing
HIL tests, incl. new ones.
Closes #1768