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

Add RMT Output support #53

Merged
merged 1 commit into from
May 17, 2022
Merged

Add RMT Output support #53

merged 1 commit into from
May 17, 2022

Conversation

ducktec
Copy link
Contributor

@ducktec ducktec commented Apr 25, 2022

This PR adds support for the Output Channel functionality of the RMT peripheral.

Details

  • The (macro-heavy) implementation is generic and not solely focused on driving pulse-driven RGB LEDs.
  • The RMT implementation is located in the pulse_control module.
  • Due to the substantially different register structures of the variants, numerous macros had to be used to achieve a shared implementation.
    • The ESP32-C3 and ESP32-S3 seem to share a common RMT architecture with a channel either being output or input channel.
    • In contrast, the ESP32 and ESP32-S2 have a different architecture where any channel can be configured to be either output or input channel.
    • This difference results in numerous fields being assigned to different registers in different variants.
  • A SmartLedsAdapter implementation is provided as part of this PR that allows for the RMT peripheral to be used with the functionality provided by the external smart-leds crate.
  • An example hello_rgb is provided for all variants.

Limitations

  • The implementation requires the entire pulse code sequence to be present in memory at the time that the RMT send function is called and as such is pretty memory-demanding. This could be improved in the future by generating the pulse code data on the fly from more a more compact representation (e.g., with closures). However, this needs to be pretty performant so that it can keep up with the RMT fifo buffer refilling and the short LED cycle periods (1,2us per LED).
  • The SmartLedsAdapter needs to be initialized using a macro smartLedAdapter!. This is because generic const expressions are not yet supported and we need the buffer to be of sizex*24+1 with x being the numbers of LEDs. If anybody has a more elegant solution for this, please let me know.
  • There are three new TODO in the code, all related to the clock configuration and time types that are subject of Time types #24 and We need a way to configure clock rates #44.

Roadmap:

  • Add support for ESP32-C3 variant
  • Add support for ESP32-S2 variant
  • Add support for ESP32 variant
  • Add support for ESP32-S3 variant
  • Add RMT device driver for Smart-leds crate.
  • Cleanup

@jessebraham
Copy link
Member

Thank you for this effort, very excited to have support for this peripheral!

I just wanted to let you know that I have released new SVDs which include your RMT patches, and have updated the esp-rs/esp-pacs's with_source branch accordingly. You should be able to revert the PACs' git dependencies whenever.

@jessebraham
Copy link
Member

I've pushed some updates which made some small changes to a number of files you've touched, sorry about that. I hope the rebase isn't too bad, but if it's painful please let me know and I will suffer in your place 😁

@jessebraham
Copy link
Member

I felt kind of guilty for making unnecessary work for you, so I've rebased your branch on main myself; you can find the branch here:
https://github.com/jessebraham/esp-hal/tree/feature/rmt

CI seems happy, so hopefully I haven't broken anything in the process 😅

(Also, sorry for all the comments 😁 )

- Add RMT output channel support for ESP32, ESP32-S2, ESP32-S3, ESP32-C3
- Add add RMT adapter for `smart-leds` crate
- Add example `hello_rgb` for ESP32-S2, ESP32-S3 and ESP32-C3 that either
  drives one LED at the pin where a LED is located on the official devkits
- Add example `hello_rgb` for ESP32 that is driving a 12-element RGB ring.
@ducktec
Copy link
Contributor Author

ducktec commented May 15, 2022

Sorry that it took so long again for some progress on this. Thank you for the rebasing. I used it as basis for the final changes and then did a git squash to clean up the history.

This PR is now ready for review and merge.

@ducktec ducktec marked this pull request as ready for review May 15, 2022 17:32
@bjoernQ
Copy link
Contributor

bjoernQ commented May 16, 2022

Awesome! I just tried the examples w/o really looking into the code.
Works fine on the S2/S3 dev-boards. My C3 dev-boards don't have an addressable LED so I used a 16xWS2812 led-ring - also works

On ESP32 I used the same 16xWS2812 ring but there it lights up all the LEDs ... maybe just my setup however.
Will have a look with a logic analyzer

Update: Seems like it's sending a lot of pulses on ESP32
image

@jessebraham
Copy link
Member

jessebraham commented May 16, 2022

Thank you for getting this wrapped up, overall I think it looks really good! I took a cursory look at your changes but still need to do a bit more of a thorough read through.

I was able to test this for the ESP32-C3, ESP32-S2, and ESP32-S3; unfortunately I do not have an ESP32 board with an addressable LED on it, and I can't seem to find any external parts laying around either. I'll be sure to pick some up in my next order just so I'm able to test this moving forward.

@bjoernQ I'd like to hear what your thoughts are on the issues with the ESP32, mainly if we should consider them blockers at this point in time. Being pre-0.1.0 I'm not super picky about including "mostly working" things in the repo, personally, as long as the behaviour is noted somewhere.

@ducktec
Copy link
Contributor Author

ducktec commented May 16, 2022

@bjoernQ The example for the ESP32 is different in that it is supposed to address 12 RGB LEDs instead of just one as with the other examples. Also, for this example to work it's important to flash the release version as the debug version is too slow to replace the entries in the RAM. Please let me know if on your RGB LED ring more than 12 LEDs light up when flashing the release version and I'll further investigate this on my side.

Number of expected pulses

As for the number of pulses that you observed, that's how it's supposed to be. 😆 With 12 RGB LEDs being addressed, the RMT peripheral has to send 288 pulses (= 12 LEDs * 3 channels (R,G,B) * 8 bit channel resolution) with a period of 1.2 us every 20 ms.

Why is the release version required?

The RMT RAM section for e.g. the ESP32 variant has 64 elements and we have to replace always half of it. So, after half of the buffer has been sent, we can observe an interrupt set being set and have then as at most as much time as the sending of the remaining 32 elements takes to replace the first 32 entries. This is only 32 * 1.2 us = 38.4 us to detect the state and update the elements. The debug version does not seem to be fast enough to achieve that. Or my implementation is just not optimized enough. 😐

@bjoernQ
Copy link
Contributor

bjoernQ commented May 16, 2022

Ah I should have looked into the example a bit deeper. Still slightly surprising 😄 but with that information the amount of pulses make sense

I will recheck this tomorrow morning and also look at the code, then

Anyway a great contribution

Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

After looking at this again (and having read your most recent comment) I think it's probably ready to merge. You have addressed your choices and any limitations more than adequately, and the documentation is fantastic, so thank you for all of that!

Some thoughts (these aren't criticisms!), just doing a bit of a brain dump:

  • Regarding the smartLedAdapter! macro, @MabezDev suggested that by enabling the unstable generic_const_exprs feature we may be able to eliminate this. I see no issue with doing this if you feel it's an adequate solution, however I'm also fine keeping the current solution for now.
  • I'm not a huge fan of having a utils module, as this is generally a bit of a code-smell for me. However I don't really have a better suggestion of where that stuff should go right now (and I think it's probably too early to make that decision) so I really have no business even bringing it up 😉 Maybe we'll accumulate a few of these types of modules eventually and there will be a more clear home for them (eg. some compat layer, a separate package, or something else entirely).
  • The macro situation is a bit unfortunate, but I understand the necessity. This is a common problem in a number of our peripheral drivers presently. I hope in the future we can find ways to reduce the number of macros required.
  • I generally would prefer for peripheral drivers to match the name of the peripheral, but despite that I feel you've made the correct choice in naming. RMT is a bit mysterious to begin with, and given that we do not have input channel capabilities it's even a bit of a misnomer at this point in time. I'm not really sure what we should call this when/if we do implement the input channel stuff, but that is future-Jesse's problem.

So with all that, thank you again for all the work you've put into this PR, I'm really excited about this! I'd still like to wait for @bjoernQ's stamp of approval, but once he's had a chance to take a look we'll get this merged!

@jessebraham jessebraham requested a review from bjoernQ May 16, 2022 19:50
@bjoernQ
Copy link
Contributor

bjoernQ commented May 17, 2022

So - I had another look and everything looks really good. After reading the code-comments of the ESP32 example (and actually soldering the connections to the LED-Ring ...) the ESP32 example works fine for me with up to 12 LEDs

  • Macro usage: We have the idea/desire to reduce macro usage in general where possible. That's not easy and definitely not in scope of this PR IMHO. Personally, I'm fine with that
  • that Utils .... it's really convenient to have that in the HAL but also a bit more than what a HAL usually does. I could imagine to have that in a separate crate like esp-smartled-adapter or something. But it's already feature-gated and can be done later (if we really decide to go that way ... it's just my very personal preference)
  • Thanks for the really detailed inline-documentation!

Really great work and an awesome contribution. Lots of useful things can be done with it

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

@jessebraham jessebraham merged commit a55c9d7 into esp-rs:main May 17, 2022
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