feat(radio): support faster RGB led timings on compatible hardware#7144
Conversation
|
Looks OK to me, but is it really required to rename everything? Most drivers I've seen still use that terminology (ws2812), since this is the part that started it all. |
|
I was not planning to, but pfeerick had me rename:D |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR migrates the STM32 RGB LED driver from the ws2812-specific API to a generalized rgbleds API. The core driver is rewritten with new timing/DMA constants and initialization logic. All board implementations, build configurations, and the simulator are updated to use the new API naming (macros, functions, and driver sources). ChangesRGBLEDS Driver Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/boards/generic_stm32/rgb_leds.cpp`:
- Around line 81-85: The seqlock window is left open: snapshotting _commit_seq
once then memcpy(_back_colors) can copy mid-write; change to a read-verify loop
using _commit_seq as the seqlock: repeatedly read seq = _commit_seq, skip if
odd, memcpy(_back_colors -> _led_colors), then verify that _commit_seq hasn’t
changed (seq == _commit_seq) before setting _consumed_seq = seq; only update
_consumed_seq after the successful verify so you never consume a torn frame.
Ensure you reference and update the same _commit_seq and _consumed_seq variables
and memcpy _back_colors into _led_colors inside that loop.
In `@radio/src/targets/common/arm/stm32/stm32_rgbleds.cpp`:
- Around line 245-250: rgbleds_get_color_in_buf assumes GRB order and always
reads pixel[1], pixel[0], pixel[2]; change it to use the configured channel
offsets (_r_offset, _g_offset, _b_offset) when indexing the per-LED pixel buffer
so it mirrors rgbleds_set_color_in_buf behavior: compute const uint8_t* pixel =
&buf[led * RGBLEDS_BYTES_PER_LED] then read pixel[_r_offset], pixel[_g_offset],
pixel[_b_offset] (combining into the same 24-bit order the function returns) and
keep the early bounds check on led against _led_strip_len.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cb9f969c-8f8a-4de2-a562-241c6e7c7484
📒 Files selected for processing (16)
radio/src/boards/generic_stm32/rgb_leds.cppradio/src/boards/rm-h750/bsp_io.cppradio/src/edgetx.cppradio/src/targets/common/arm/stm32/stm32_rgbleds.cppradio/src/targets/common/arm/stm32/stm32_rgbleds.hradio/src/targets/horus/CMakeLists.txtradio/src/targets/pa01/CMakeLists.txtradio/src/targets/pa01/board.cppradio/src/targets/pl18/CMakeLists.txtradio/src/targets/simu/led_driver.cppradio/src/targets/st16/CMakeLists.txtradio/src/targets/st16/board.cppradio/src/targets/t15pro/CMakeLists.txtradio/src/targets/taranis/CMakeLists.txtradio/src/targets/tx15/CMakeLists.txtradio/src/targets/tx16smk3/CMakeLists.txt
|
@3djc Did you ever test this on the GX12 or T15Pro? Out of T15Pro, TX16SMK3 and GX12, this seems to make the GX12 customisable switches flicker and carry on when they should be static, and the T15Pro gimbal rings and customisable switch leds go crazy... TX16SMK3 is the only one behaving properly. On 2.12.2 the T15Pro and GX12 are both fine. |
|
Yes, they worked fine at the time. Likey tested the 2.12 version, but it should not matter. Will retry |
|
Not too sure what I was under the day I tested, should have taken a note, because that must have been good stuff !! Sorry for that, tested tx16smk3, tx15, t15pro and gx12 ok |
|
t22 added and checked |
Aw... I wanna know also! 🤣 Thanks JC :) |
Several manufacturers are using RGB leds that, while supporting 'standard' ws2812 timings, are designed for faster timings. This introduces support for those faster native timings.
Summary by CodeRabbit