-
Notifications
You must be signed in to change notification settings - Fork 266
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
Conversation
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 |
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 😁 |
I felt kind of guilty for making unnecessary work for you, so I've rebased your branch on 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.
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. |
Awesome! I just tried the examples w/o really looking into the code. On ESP32 I used the same 16xWS2812 ring but there it lights up all the LEDs ... maybe just my setup however. |
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- |
@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 pulsesAs 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 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 |
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 |
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.
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 unstablegeneric_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. somecompat
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!
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
Really great work and an awesome contribution. Lots of useful things can be done with it |
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.
LGTM
This PR adds support for the
Output Channel
functionality of the RMT peripheral.Details
pulse_control
module.SmartLedsAdapter
implementation is provided as part of this PR that allows for the RMT peripheral to be used with the functionality provided by the externalsmart-leds
crate.hello_rgb
is provided for all variants.Limitations
1,2us
per LED).SmartLedsAdapter
needs to be initialized using a macrosmartLedAdapter!
. This is because generic const expressions are not yet supported and we need the buffer to be of sizex*24+1
withx
being the numbers of LEDs. If anybody has a more elegant solution for this, please let me know.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:
ESP32-C3
variantESP32-S2
variantESP32
variantESP32-S3
variant