Harden LED configuration and color packing - #94
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the LED driver’s handling of LED strip color order/encoding by validating configuration earlier and making color packing/shifting behavior well-defined for 32-bit output values.
Changes:
- Reject invalid
LedColorOrdervalues duringled_driver_setup()before calculating/allocating the DMA bitbuffer, with an error that identifies the offending strip index. - Make color channel bit packing explicitly use
uint32_twidening before shifts to avoid undefined behavior on 32-bit targets. - Reject unexpected
color_orderencodings in the paint path (added adefaultcase in the switch).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8ae1c33 to
00062b3
Compare
| if (!leds->rear_strip.color_conv) { | ||
| log_error("Invalid rear LED color order: %u.", hw_cfg->rear.color_order); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This duplication is not good. It's about architecturally putting the code to the correct place. Looks like the whole color_order handling could be moved into the led_strip module, storing the bit number and the converter in the LedStrip struct. They are not really LedStrip related by nature, they should probably technically go into their own module (e.g. led_color_order.{c,h} or just make a directory for the led code at this stage). But I'd accept it if the functions were cleanly placed into led_strip.c too. More structural work and I don't really wanna spend the time iterating on this (so to be blunt if you can do it right please do, if not, let's not spend the time 😅 ).
A straightforward fix of the uninitialized pointer dereference would be to initialize it and early-return if it's NULL.
There was a problem hiding this comment.
sorry I should not have pushed until I had a chance to re-review the larger context. I'll put this back to draft and get back to you when it's really right.
a bit of a struggle since there's so much to keep track of, but I'll get back to you when it feels right.
00062b3 to
8dd3311
Compare
Move color-order resolution into led_color_order and cache the bit width and converter on each LedStrip during setup. This keeps the paint path generic and leaves one color-order mapping. Bind leds->cfg before the new fallible strip validation. leds_setup() returns locally for an invalid active strip, but startup still configures LCM in external modes. Invalid internal setup still stops before LED data or DMA allocation.
8dd3311 to
1d708fe
Compare
Validate LED strip color order before allocating DMA storage and report which strip is invalid.
The paint path also rejects unexpected encodings, and channel values are widened before bit packing. Valid GRB, GRBW, RGB, and WRGB output is unchanged.