-
Notifications
You must be signed in to change notification settings - Fork 369
RMT: Make PulseCode a newtype rather than an extension trait #3884
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
Conversation
15537c9 to
91c0f14
Compare
esp-hal/src/rmt.rs
Outdated
| /// This corresponds to the all-zero `PulseCode`, i.e. an end marker with | ||
| /// level `Level::Low`. | ||
| #[inline] | ||
| pub const fn empty() -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? Won't this cause confusion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I had simply renamed this to end_marker; then I returned to it and was unsure whether it was worth it in the face of backwards-compatibility, and because end_marker does not necessarily correspond to user intent (e.g. if initializing an rx buffer).
I think there are two uses for 0-valued pulse codes:
- terminate a tx buffer:
tx_data[tx_data.len() - 1] = PulseCode::end_marker() - initializing an rx buffer:
let mut rx_data: [_; 42] = [PulseCode::empty(); 42]
and 3 ways to generate them
PulseCode::end_marker()PulseCode::empty()PulseCode::default()
which, as you say, seems likely to generate confusion.
I'd argue that it might be best to keep 1) and 3), allowing to express both uses in a meaningful way. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed an updated branch with .empty removed and an addition to the migration guide concerning these methods.
91c0f14 to
8f8027e
Compare
|
Updated with changes addressing the review and some further enhancements (see the commit message for a full listing of changes compared to the previous version); rebased on main. |
8f8027e to
59808d2
Compare
This has several advantages:
- the meaning of `u32` used as pulse code becomes more explicit
- it allows using `PulseCode` methods in `const` context (which is otherwise
not possible because Rust does not presently support associated const
fn in traits).
- it allows providing custom `defmt::Format` and `core::fmt::Debug` impls
for `PulseCode`, greatly helping with debugging
I have taken the liberty to implement `core::fmt::Debug` in a slightly
non-standard way: The most natural implementation would probably use a
struct-style output like
PulseCode { length1: 42, level1: Level::High, length2: 24, level2: Level::Low }
However, that is very lengthy and not really human-readable anymore when
dealing with an array of `PulseCode`s. Thus, this uses the more compact format
PulseCode(H 42, L 24)
This provides `u32: From<PulseCode>` and `PulseCode: From<u32>` impls and
converts rx and tx methods to accept `impl Into<PulseCode>` and
`impl From<PulseCode>`, respectively.
This should help to reduce how much user code needs to change (but replacing
`u32` type annotations with `PulseCode` will be required).
By applying `#[repr(transparent)]` to `struct PulseCode`, it is
guaranteed to match the layout of `u32` such that accessing the hardware
buffer via `*mut PulseCode` pointers is valid.
…ce a bit - introduce Level::const_from and Level::const_into - pre-compute a few more shifts and masks (this is unlikely to affect generated code, since the compiler would have const propagated them anyway, but it helps to keep the PulseCode impl more readable) - improve docstrings - remove PulseCode::empty in favor of PulseCode::default, keep PulseCode::end_marker for now (but it's not completely clear that this interface is optimal; see also the FIXME note on adding a variant of this methods that supports settings levels and length1) - make PulseCode::new_unchecked pub and shuffle it around in the code so that it doesn't show up first in the docs - factor out PulseCode::symbolX methods for internal use in debug formatting - sprinkle a few more #[inline] to be totally sure that this really adds no overhead over having plain u32 - convert methods receivers from &self to self given that PulseCode is Copy (which probably doesn't matter much since all these methods should always be inlined) - remove PulseCode::as_u32() and make the tuple field pub: There's no safety implication of marking this pub, and field access still provides a const-compatible way to obtain the wrapped value
59808d2 to
635ff32
Compare
bugadani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I forgot about this. Let's see how this turns out :)
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 is on top of #3716, only the last commit is new.This PR proposes to convert the RMT
PulseCode, which is currently an extension trait implemented foru32, to a newtypestruct PulseCode(u32).This has several advantages, as outlined in the commit message:
Testing
As part of #3509 in CI and locally for esp32c3.