support for ws2814 led strips, white channel first #88
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Light preset enum & handling src/MoonBase/Nodes.h, src/MoonBase/Nodes.cpp |
Added lightPreset_WRGB to LightPresetsEnum and implemented handling in DriverNode::onUpdate to set channelsPerLight = 4 with offsets white=0, red=1, green=2, blue=3 when WRGB is selected. |
Virtual layer RGB/White writes src/MoonLight/Layers/VirtualLayer.h |
Replaced white-position check with channelsPerLight == 4 for RGBW behavior: setRGB and setWhite now treat 4-channel lights consistently (white at offset 3 for internal buffer or configured offset for non-RGBW). |
Parallel LED driver integration src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h |
Extended logging and updated calls to initled(...) and updateDriver(...) to pass the additional offsetWhite parameter so driver init/update use the white-channel offset. |
Sequence Diagram(s)
sequenceDiagram
autonumber
participant UI as DriverNode (config)
participant VL as VirtualLayer
participant D as ParallelLEDDriver
note right of UI `#DDEEFF`: onUpdate for WRGB preset
UI->>VL: set channelsPerLight=4, offsets {W=0,R=1,G=2,B=3}
VL-->>D: initled(channelsPerLight, offsetR, offsetG, offsetB, offsetWhite)
D-->>VL: driver initialized
note right of UI `#E8F5E9`: runtime updates
UI->>VL: write RGB/White values
VL->>D: updateDriver(buffer, offsets..., offsetWhite)
D-->>VL: DMA/LED buffer updated
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- Board presets p4 #74 — Modifies the same lighting preset handling and related VirtualLayer/Nodes code paths (enum, preset mapping, RGBW handling).
Poem
🐰 Four tiny channels hop in a line,
White first, then red, green, blue align,
A WRGB tune hums through driver and layer,
I mapped their offsets with careful-care-er! 🥕✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: adding support for WS2814 LED strips with white channel first (WRGB), which is implemented via the new lightPreset_WRGB across multiple files. |
✨ Finishing touches
- 📝 Generate docstrings
📜 Recent review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MoonLight/Layers/VirtualLayer.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Layers/VirtualLayer.h
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/MoonLight/Layers/VirtualLayer.h (1)
114-117: The special-case logic works for current presets but may be fragile for future ones.The condition
offsetWhite == 0 && channelsPerLight == 4specifically handles WRGB, but a hypothetical future 4-channel preset withoffsetWhiteat position 1 or 2 would incorrectly write to that physical offset rather than the internal white position (3).Consider generalizing to always write to position 3 for all 4-channel configurations, since the internal format is RGBW regardless of physical ordering:
🔎 Suggested alternative (optional)
void setWhite(const uint16_t indexV, const uint8_t value) { - if (layerP->lights.header.offsetWhite == 0 && layerP->lights.header.channelsPerLight == 4) setLight(indexV, &value, 3, sizeof(value)); - else if (layerP->lights.header.offsetWhite != UINT8_MAX) setLight(indexV, &value, layerP->lights.header.offsetWhite, sizeof(value)); + if (layerP->lights.header.channelsPerLight == 4 && layerP->lights.header.offsetWhite != UINT8_MAX) { + setLight(indexV, &value, 3, sizeof(value)); // Internal RGBW format: white always at position 3 + } else if (layerP->lights.header.offsetWhite != UINT8_MAX) { + setLight(indexV, &value, layerP->lights.header.offsetWhite, sizeof(value)); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/MoonBase/Nodes.cppsrc/MoonBase/Nodes.hsrc/MoonLight/Layers/VirtualLayer.hsrc/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h
🧰 Additional context used
🧬 Code graph analysis (1)
src/MoonLight/Layers/VirtualLayer.h (1)
src/MoonLight/Layers/VirtualLayer.cpp (2)
setLight(126-162)setLight(126-126)
🔇 Additional comments (6)
src/MoonBase/Nodes.cpp (2)
356-356: LGTM!The WRGB control value is added in the correct position matching the enum order in
Nodes.h.
462-468: LGTM!The WRGB preset correctly configures the 4-channel layout with white at position 0, matching WS2814 LED strip specifications.
src/MoonBase/Nodes.h (1)
30-30: LGTM!The
lightPreset_WRGBenum value is correctly positioned afterlightPreset_GRBW, maintaining backward compatibility with existing preset indices.src/MoonLight/Layers/VirtualLayer.h (1)
100-111: LGTM!The removal of the
offsetWhite == offsetRGB + 3constraint is correct. This allows WRGB (where white is at offset 0) to properly trigger the 4-channel RGBW path. The internalrgbwarray maintains standard RGBW order, with reordering to physical layout handled downstream byreOrderAndDimRGBW.src/MoonLight/Nodes/Drivers/D_ParallelLEDDriver.h (2)
133-134: LGTM!The update path mirrors the initialization path, consistently passing
offsetWhitetoupdateDriver()for runtime preset changes.
120-121: LGTM, pending external dependency verification.The initialization and update paths consistently log and pass
offsetWhiteto bothinitled()andupdateDriver()methods, enabling white channel offset mapping. However, the method signatures forI2SClocklessLedDriver::initled()andupdateDriver()cannot be verified as this is an external dependency not present in the repository. Confirm with the I2SClocklessLedDriver library that both methods accept theoffsetWhiteparameter in their current signatures.
src/MoonLight/Layers/VirtualLayer.h
Outdated
| void setWhite(const uint16_t indexV, const uint8_t value) { | ||
| if (layerP->lights.header.offsetWhite != UINT8_MAX) setLight(indexV, &value, layerP->lights.header.offsetWhite, sizeof(value)); | ||
| if (layerP->lights.header.offsetWhite == 0 && layerP->lights.header.channelsPerLight == 4) setLight(indexV, &value, 3, sizeof(value)); | ||
| else if (layerP->lights.header.offsetWhite != UINT8_MAX) setLight(indexV, &value, layerP->lights.header.offsetWhite, sizeof(value)); |
There was a problem hiding this comment.
I think this can be combined into :
If offsetWhite != UINT8_MAX {
If (channelsPerLight is 4) assign to position 3; // for rgbw lights
Else assign to offsetWhite position; // for moving heads
}
src/MoonLight/Layers/VirtualLayer.h
Outdated
|
|
||
| void setRGB(const uint16_t indexV, CRGB color) { | ||
| if (layerP->lights.header.offsetWhite != UINT8_MAX && layerP->lights.header.offsetWhite == layerP->lights.header.offsetRGB + 3) { // W set and after RGB | ||
| if (layerP->lights.header.offsetWhite != UINT8_MAX) { // W set |
There was a problem hiding this comment.
I think we should add && channelsPerLight == 4 here to avoid problems with moving heads
|
the complicating matter here is that the channels array always stores RGB(W) in the order of RGB(W), not GRB(W) etc, even if the lights are GRB(W) etc. In the reOrderAndDimRGBW() this is set right for the specific LED type. There is a 'revolutionary' idea to store the color order already in the channels array during setRGB. This simplifies a number of things, like the checks I proposed above (if channels per light == 4) are not needed. But for now this is the best compromise |
|
@ewowi PR updated :) |
2 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.