Replace uint8_t color arrays with CRGBW (DO NOT MERGE, its broken still)#5700
Replace uint8_t color arrays with CRGBW (DO NOT MERGE, its broken still)#5700DedeHai wants to merge 5 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughGlobal color state variables ChangesCRGBW Type Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
DedeHai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/colors.h (1)
165-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix HSV-to-CRGBW channel ordering.
CRGBW::rawis B,G,R,W, buthsv2rgb_rainbow()now writes RGB into offsets 0..2. These constructors/operators therefore swap red and blue for every HSV-derivedCRGBWvalue.🐛 Proposed fix: stage through RGB-order bytes before assigning CRGBW fields
- inline CRGBW(CHSV32 hsv) __attribute__((always_inline)) { hsv2rgb_rainbow(hsv.h, hsv.s, hsv.v, raw); w = 0; } + inline CRGBW(CHSV32 hsv) __attribute__((always_inline)) { + uint8_t rgb[3]; + hsv2rgb_rainbow(hsv.h, hsv.s, hsv.v, rgb); + r = rgb[0]; g = rgb[1]; b = rgb[2]; w = 0; + } // Constructor from CHSV - inline CRGBW(CHSV hsv) __attribute__((always_inline)) { hsv2rgb_rainbow(hsv.h<<8, hsv.s, hsv.v, raw); w = 0;} + inline CRGBW(CHSV hsv) __attribute__((always_inline)) { + uint8_t rgb[3]; + hsv2rgb_rainbow(hsv.h<<8, hsv.s, hsv.v, rgb); + r = rgb[0]; g = rgb[1]; b = rgb[2]; w = 0; + } - inline CRGBW& operator=(CHSV32 hsv) __attribute__((always_inline)) { hsv2rgb_rainbow(hsv.h, hsv.s, hsv.v, raw); w = 0; return *this; } + inline CRGBW& operator=(CHSV32 hsv) __attribute__((always_inline)) { + uint8_t rgb[3]; + hsv2rgb_rainbow(hsv.h, hsv.s, hsv.v, rgb); + r = rgb[0]; g = rgb[1]; b = rgb[2]; w = 0; + return *this; + } // Assignment from CHSV - inline CRGBW& operator=(CHSV hsv) __attribute__((always_inline)) { hsv2rgb_rainbow(hsv.h<<8, hsv.s, hsv.v, raw); w = 0; return *this; } + inline CRGBW& operator=(CHSV hsv) __attribute__((always_inline)) { + uint8_t rgb[3]; + hsv2rgb_rainbow(hsv.h<<8, hsv.s, hsv.v, rgb); + r = rgb[0]; g = rgb[1]; b = rgb[2]; w = 0; + return *this; + }🤖 Prompt for 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. In `@wled00/colors.h` around lines 165 - 180, The CRGBW constructors and assignment operators that accept HSV values (CHSV32 and CHSV) are passing the raw array directly to hsv2rgb_rainbow(), but the raw array is ordered as B,G,R,W while hsv2rgb_rainbow() writes RGB into offsets 0..2, causing red and blue channels to be swapped. To fix this, modify all four HSV-related methods (the two CRGBW constructors taking CHSV32 and CHSV, and the two operator= overloads for CHSV32 and CHSV) to create a temporary uint8_t array in RGB order, pass that to hsv2rgb_rainbow(), and then correctly map the RGB values to the raw array's B,G,R,W layout before setting w to 0.
🤖 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 `@wled00/colors.cpp`:
- Line 18: The blend formula documented in the comment at the top of
wled00/colors.cpp does not match the actual optimized implementation. Update the
comment that states "the calculation for each color is: result =
(C1*(256-blend)+C2*blend) / 256" to correctly reflect the code behavior by
changing C2*blend to C2*(blend+1), which ensures that when blend equals 255, the
result is color2.
In `@wled00/json.cpp`:
- Around line 243-250: The code is attempting to read color values from a JSON
array using object keys ("r", "g", "b", "w"), but JSON arrays use positional
indexing, not named keys. Replace the key-based access on the colX JsonArray
with positional indexing where index 0 is red, index 1 is green, index 2 is
blue, and index 3 is white. Make this change in both locations where this
pattern appears (the segment colors block and the per-LED colors block mentioned
in the comment).
In `@wled00/led.cpp`:
- Line 205: The comment at line 205 in the led.cpp file lacks a required space
after the line-comment marker. Update the comment from `//save current globals
in unused color channels` to `// save current globals in unused color channels`
by adding a space after the `//` to comply with the project's coding style
guidelines that require a space after the comment marker.
In `@wled00/mqtt.cpp`:
- Line 178: The sprintf_P call for publishing the MQTT color value on the `c`
topic is not masking out the white byte from colPri, which causes the format to
exceed the expected 6-digit RGB hex format when the upper byte is non-zero.
Apply a bitwise AND mask to colPri to extract only the lower 24 bits (the RGB
portion) before passing it to sprintf_P with the %06X format specifier. This
ensures the published color always remains in the `#RRGGBB` format regardless of
any white byte value in colPri.
---
Outside diff comments:
In `@wled00/colors.h`:
- Around line 165-180: The CRGBW constructors and assignment operators that
accept HSV values (CHSV32 and CHSV) are passing the raw array directly to
hsv2rgb_rainbow(), but the raw array is ordered as B,G,R,W while
hsv2rgb_rainbow() writes RGB into offsets 0..2, causing red and blue channels to
be swapped. To fix this, modify all four HSV-related methods (the two CRGBW
constructors taking CHSV32 and CHSV, and the two operator= overloads for CHSV32
and CHSV) to create a temporary uint8_t array in RGB order, pass that to
hsv2rgb_rainbow(), and then correctly map the RGB values to the raw array's
B,G,R,W layout before setting w to 0.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d4408a2-8ff7-423a-964a-12a5d71b4f64
📒 Files selected for processing (17)
usermods/usermod_rotary_brightness_color/usermod_rotary_brightness_color.cppusermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.cppwled00/alexa.cppwled00/colors.cppwled00/colors.hwled00/fcn_declare.hwled00/json.cppwled00/led.cppwled00/lx_parser.cppwled00/mqtt.cppwled00/overlay.cppwled00/set.cppwled00/src/dependencies/fastled_slim/fastled_slim.cppwled00/src/dependencies/fastled_slim/fastled_slim.hwled00/wled.cppwled00/wled.hwled00/xml.cpp
| /* | ||
| * color blend function | ||
| * the calculation for each color is: result = (C1*(256-blend)+C2+C2*blend) / 256 | ||
| * the calculation for each color is: result = (C1*(256-blend)+C2*blend) / 256 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented blend formula.
The optimized code adds one extra C2 term so blend == 255 reaches color2; the comment should say C2*(blend+1), not C2*blend. As per path instructions, comments should be verified against code behavior.
📝 Proposed comment fix
- * the calculation for each color is: result = (C1*(256-blend)+C2*blend) / 256
+ * the calculation for each color is: result = (C1*(256-blend)+C2*(blend+1)) / 256📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * the calculation for each color is: result = (C1*(256-blend)+C2*blend) / 256 | |
| * the calculation for each color is: result = (C1*(256-blend)+C2*(blend+1)) / 256 |
🤖 Prompt for 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.
In `@wled00/colors.cpp` at line 18, The blend formula documented in the comment at
the top of wled00/colors.cpp does not match the actual optimized implementation.
Update the comment that states "the calculation for each color is: result =
(C1*(256-blend)+C2*blend) / 256" to correctly reflect the code behavior by
changing C2*blend to C2*(blend+1), which ensures that when blend equals 255, the
result is color2.
Source: Path instructions
| } else { //Array of ints (RGB or RGBW color), e.g. [255,160,0] | ||
| byte sz = colX.size(); | ||
| if (sz == 0) continue; //do nothing on empty array | ||
| copyArray(colX, rgbw, 4); | ||
| rgbw.r = colX["r"] | 0; | ||
| rgbw.g = colX["g"] | 0; | ||
| rgbw.b = colX["b"] | 0; | ||
| rgbw.w = colX["w"] | 0; | ||
| colValid = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify JsonArray color reads in wled00/json.cpp are positional indexes.
rg -nP --type=cpp 'colX\["[rgbw]"\]|icol\["[rgbw]"\]|colX\[[0-3]\]|icol\[[0-3]\]' wled00/json.cpp -C2
# Expected after fix:
# - no matches for colX["r"/"g"/"b"/"w"] or icol["r"/"g"/"b"/"w"]
# - matches only for numeric indexing colX[0..3], icol[0..3]Repository: wled/WLED
Length of output: 749
🏁 Script executed:
# Check the full context around lines 243-250 to understand the format
sed -n '235,260p' wled00/json.cppRepository: wled/WLED
Length of output: 1162
🏁 Script executed:
# Check the full context around lines 341-360 to understand the format
sed -n '335,365p' wled00/json.cppRepository: wled/WLED
Length of output: 1107
🏁 Script executed:
# Look for other color parsing patterns in the same file to understand intended behavior
rg -nP 'colX\[|icol\[' wled00/json.cpp -B2 -A2 | head -80Repository: wled/WLED
Length of output: 749
Use positional indexing for JSON color arrays.
Both "array of ints" paths read JsonArray with object keys ("r", "g", "b", "w"). Since JsonArray is positional, these lookups fail and return null, causing colors from valid inputs like [255,160,0] to default to black.
💡 Proposed fix
- rgbw.r = colX["r"] | 0;
- rgbw.g = colX["g"] | 0;
- rgbw.b = colX["b"] | 0;
- rgbw.w = colX["w"] | 0;
+ rgbw.r = colX[0] | 0;
+ rgbw.g = colX[1] | 0;
+ rgbw.b = colX[2] | 0;
+ rgbw.w = colX[3] | 0;- rgbw.r = icol["r"] | 0;
- rgbw.g = icol["g"] | 0;
- rgbw.b = icol["b"] | 0;
- rgbw.w = icol["w"] | 0;
+ rgbw.r = icol[0] | 0;
+ rgbw.g = icol[1] | 0;
+ rgbw.b = icol[2] | 0;
+ rgbw.w = icol[3] | 0;Affects: lines 246–249 (segment colors) and 346–349 (per-LED colors)
🤖 Prompt for 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.
In `@wled00/json.cpp` around lines 243 - 250, The code is attempting to read color
values from a JSON array using object keys ("r", "g", "b", "w"), but JSON arrays
use positional indexing, not named keys. Replace the key-based access on the
colX JsonArray with positional indexing where index 0 is red, index 1 is green,
index 2 is blue, and index 3 is white. Make this change in both locations where
this pattern appears (the segment colors block and the per-LED colors block
mentioned in the comment).
| colNlT[0] = effectCurrent; | ||
| colNlT[1] = effectSpeed; | ||
| colNlT[2] = effectPalette; | ||
| //save current globals in unused color channels |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required space after the line-comment marker.
Line 205 uses //save; this should be // save to match the project comment style.
Proposed fix
- //save current globals in unused color channels
+ // save current globals in unused color channelsAs per coding guidelines, "Use // for inline comments (always space after)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //save current globals in unused color channels | |
| // save current globals in unused color channels |
🤖 Prompt for 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.
In `@wled00/led.cpp` at line 205, The comment at line 205 in the led.cpp file
lacks a required space after the line-comment marker. Update the comment from
`//save current globals in unused color channels` to `// save current globals in
unused color channels` by adding a space after the `//` to comply with the
project's coding style guidelines that require a space after the comment marker.
Source: Coding guidelines
| mqtt->publish(subuf, 0, retainMqttMsg, s); // optionally retain message (#2263) | ||
|
|
||
| sprintf_P(s, PSTR("#%06X"), (colPri[3] << 24) | (colPri[0] << 16) | (colPri[1] << 8) | (colPri[2])); | ||
| sprintf_P(s, PSTR("#%06X"), colPri); // 0xWWRRGGBB |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrate width behavior and verify current callsite.
rg -nP --type=cpp 'sprintf_P\s*\(\s*s\s*,\s*PSTR\("#%06X"\)\s*,\s*colPri' wled00/mqtt.cpp -C1
python - <<'PY'
vals = [0x00112233, 0xFF112233]
for v in vals:
print(hex(v), "->", f"#{v:06X}")
PY
# Expected:
# 0x00112233 -> `#112233`
# 0xff112233 -> `#FF112233` (shows why masking is needed)Repository: wled/WLED
Length of output: 351
🏁 Script executed:
# First, examine the context around line 178 in mqtt.cpp
sed -n '160,190p' wled00/mqtt.cpp
# Look for colPri variable declaration and its type
rg -nP 'colPri' wled00/mqtt.cpp | head -20
# Check the broader context of the publishColorTopic function
ast-grep outline wled00/mqtt.cpp | grep -i colorRepository: wled/WLED
Length of output: 1512
🏁 Script executed:
# Look for where colPri is assigned/set
rg -nP 'colPri\s*=' wled00/ -A2 -B2 | head -40
# Check MQTT topic documentation or comment
rg -nP 'MQTT.*color|mqtt.*c.*topic|`#RRGGBB`|0xWWRRGGBB' wled00/ -i | head -20Repository: wled/WLED
Length of output: 3195
🏁 Script executed:
# See if there are other places publishing color to understand the expected format
rg -nP 'sprintf.*#.*[0-9A-Fa-f]' wled00/mqtt.cpp -B3 -A1 | head -30Repository: wled/WLED
Length of output: 499
🏁 Script executed:
# Check the type of colPri
rg -nP 'WLED_GLOBAL.*colPri|extern.*colPri' wled00/wled.h -B2 -A2
# Check Segment colors array type
rg -nP 'struct Segment|class Segment' wled00/Segment.h -A40 | head -60
# Look for color_blend or colorFromDecOrHexString to understand color storage
rg -nP 'color_blend|colorFromDecOrHexString' wled00/ --type cpp -lRepository: wled/WLED
Length of output: 643
🏁 Script executed:
# Check what seg.colors[0] actually contains - is it 32-bit WRGB or 24-bit RGB?
rg -nP 'colors\[.*\].*=|colors.*uint' wled00/Segment.h -B3 -A3 | head -40
# Look at actual segment color initialization
rg -nP 'uint32_t.*colors|colors\s*\[' wled00/Segment.h | head -10Repository: wled/WLED
Length of output: 261
🏁 Script executed:
# Check if there's any documentation or tests showing expected MQTT color format
rg -nP '#[0-9A-Fa-f]{6}|#[0-9A-Fa-f]{8}|RRGGBB|WWRRGGBB' wled00/ -i | grep -i mqtt
# Check what colorFromDecOrHexString does with the value
sed -n '100,130p' wled00/mqtt.cppRepository: wled/WLED
Length of output: 1266
🏁 Script executed:
# Find CRGBW definition
rg -nP 'struct CRGBW|class CRGBW|typedef.*CRGBW' wled00/ -B3 -A15
# Check colors.h for CRGBW and color handling
head -100 wled00/colors.h | cat -n
# Look for uint32_t conversion in CRGBW
rg -nP 'operator.*uint32|operator.*uint' wled00/colors.h -B2 -A5Repository: wled/WLED
Length of output: 11012
🏁 Script executed:
# Check colorFromDecOrHexString implementation to see what it expects
rg -nP 'void colorFromDecOrHexString' wled00/ -A20Repository: wled/WLED
Length of output: 2527
🏁 Script executed:
# Verify if CRGBW is indeed a 32-bit structure and understand its layout
fd -t f colors wled00/ | head -5
# Let's check what type colPri really gets cast to when passed to sprintf
grep -n "sprintf_P.*colPri" wled00/mqtt.cpp -B5 -A5Repository: wled/WLED
Length of output: 853
🏁 Script executed:
# Check if there's documentation about MQTT topic formats
rg -i 'mqtt.*color|mqtt.*#|topic.*format' wled00/ --type cpp --type h | head -20
# Look for any comments about the "c" topic specifically
rg -nP '"/c"' wled00/ -B5 -A5
# Check the MQTT subscription handler for "c" topic
rg -nP 'strcmp.*"/col"|if.*col' wled00/mqtt.cpp -B3 -A10Repository: wled/WLED
Length of output: 2646
🏁 Script executed:
# Look for other color publishing or MQTT references
rg -nP 'mqtt->publish.*col|publish.*color' wled00/mqtt.cpp -B2 -A2
# Check what colorFromDecOrHexString expects - does it handle RGBW properly?
sed -n '537,560p' wled00/colors.cppRepository: wled/WLED
Length of output: 819
🏁 Script executed:
# Check if there's any test or documentation mentioning MQTT color format
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec rg -l "mqtt.*color|mqtt.*#" {} \; 2>/dev/null | head -10
# Look for any CI/test files mentioning MQTT or color format
find . -path ./node_modules -prune -o -type f \( -name "*test*" -o -name "*.yaml" \) -print | xargs rg "mqtt.*color|color.*mqtt" 2>/dev/null | head -10
# Check git history or issues that might mention this
git log --oneline --all | grep -i "mqtt.*color\|color.*mqtt" | head -5Repository: wled/WLED
Length of output: 350
🏁 Script executed:
# Verify the actual behavior: trace how colPri flows through the system
# Is there any place where white byte is intentionally preserved/used in MQTT?
rg -nP 'colPri.*mqtt|mqtt.*colPri' wled00/ -B3 -A3
# Check if there's any segment color handling that depends on white byte
rg -nP 'W\(colPri\)|colPri.*>>.*24|byte.*colPri.*>>.*24' wled00/ -B2 -A2Repository: wled/WLED
Length of output: 147
🏁 Script executed:
# Check the "Fixed MQTT color response" commit
git show 961d23e2 --stat | head -20
git show 961d23e2 | grep -A10 -B10 "color\|mqtt" | head -40Repository: wled/WLED
Length of output: 1296
🏁 Script executed:
# Look for the actual implementation of what "c" topic publishes and accepts
# Check if there's any explicit documentation about RGB vs RGBW in MQTT
rg -nP 'c.*RGB|RGB.*c.*topic|MQTT.*RGB' wled00/ -i -B2 -A2 | head -30
# Check if white byte is used anywhere in the published color
grep -n "W(" wled00/mqtt.cppRepository: wled/WLED
Length of output: 2222
🏁 Script executed:
# Verify the issue: does the current code actually publish wrong format?
# Simulate what happens when white byte is set
python3 <<'PY'
# Test the actual sprintf behavior with colPri values
test_values = [
0x00112233, # RGB only: `#112233`
0xFF112233, # WRGB with white: should publish what?
0x80112233, # Half white: should publish what?
]
for val in test_values:
# Current code: sprintf_P(s, PSTR("#%06X"), colPri)
formatted = f"#{val:06X}"
print(f"colPri=0x{val:08X} -> current={formatted} (len={len(formatted)})")
# Proposed fix: mask to RGB only
masked = val & 0x00FFFFFF
fixed = f"#{masked:06X}"
print(f" -> masked={fixed} (len={len(fixed)})")
print()
PYRepository: wled/WLED
Length of output: 400
Mask out the white byte before publishing MQTT c color.
The MQTT c topic publishes color in RGB hex format (#RRGGBB). The %06X format specifier defines a minimum width of 6 digits, not a maximum. When colPri has a non-zero white byte (upper 8 bits), sprintf_P produces 8 hex digits (#WWRRGGBB) instead of 6, breaking the established MQTT contract (see commit 961d23e, which specifically enforced the 6-digit RGB format in 2019).
Example: colPri=0xFF112233 publishes #FF112233 (9 chars) instead of #112233 (7 chars).
💡 Proposed fix
- sprintf_P(s, PSTR("#%06X"), colPri); // 0xWWRRGGBB
+ sprintf_P(s, PSTR("#%06X"), (uint32_t(colPri) & 0x00FFFFFFU)); // publish RGB only (`#RRGGBB`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sprintf_P(s, PSTR("#%06X"), colPri); // 0xWWRRGGBB | |
| sprintf_P(s, PSTR("#%06X"), (uint32_t(colPri) & 0x00FFFFFFU)); // publish RGB only (`#RRGGBB`) |
🤖 Prompt for 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.
In `@wled00/mqtt.cpp` at line 178, The sprintf_P call for publishing the MQTT
color value on the `c` topic is not masking out the white byte from colPri,
which causes the format to exceed the expected 6-digit RGB hex format when the
upper byte is non-zero. Apply a bitwise AND mask to colPri to extract only the
lower 24 bits (the RGB portion) before passing it to sprintf_P with the %06X
format specifier. This ensures the published color always remains in the `#RRGGBB`
format regardless of any white byte value in colPri.
1 participant
saves some flash and simplifies code.
Summary by CodeRabbit