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.
00062b3 to
8dd3311
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.