Correct single RGB & DRGB modes to explicitly utilize white #3507
Conversation
|
Please re-base this PR to 0_15 branch. |
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
I do not use realtime data so cannot test this PR.
Still, as WLED allows realtime data to be displayed on main segment only, and main segment may not include white channel, it is mandatory to differentiate if the underlying canvas supports white properly.
Segment has light capabilities embedded (and recalculated on each change) and white channel presence can be determined by Segment::hasWhite() method.
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
Seems ok but please add parentheses. 😄
|
@blazoncek please check bdd667d I think it could be faster when only iterating over differentiated length as well, shall I add these checks again? |
|
IDK. I have very little knowledge about realtime apart from DDP implementation. |
|
I'll change back PR draft mode, when I've tested code changes with realtime DMX console e131 input. |
@mxklb did you make progress? This PR is in draft for 8 months now, are you still working on it?
PS: please don't force-push - it leads to many strange effects on github, including lost review comments. |
softhack007
added
the
stale
|
I forgot to test it. Implementaion looks still good, is an improvement. I'll try to test it the following week, and will change draft mode, when everything works as expected. |
softhack007
added
enhancement
Awaiting testing
and removed
stale
WalkthroughThe Changes
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Found some time to test, should be save to merge. With this change, sending white channel The number of DMX channels depends on the white color capability of the strip and the selected DMX mode:
Compared to pervious hidden |
![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 (2)
wled00/e131.cpp (2)
148-161: Excellent implementation of dynamic white channel handling.The changes correctly address the past review feedback by differentiating between main segment and full strip usage for white channel detection. The logic properly determines channel count and conditionally reads the white channel.
For improved readability, consider adding parentheses around the conditional expression:
- dmxChannelCount = (useMainSegmentOnly ? strip.getMainSegment().hasWhite() : strip.hasWhiteChannel()) ? 4 : 3; + dmxChannelCount = ((useMainSegmentOnly ? strip.getMainSegment().hasWhite() : strip.hasWhiteChannel()) ? 4 : 3);Also, consider adding parentheses in the for loop condition for clarity:
- for (unsigned i = 0; i < useMainSegmentOnly ? strip.getMainSegment().length() : totalLen; i++) + for (unsigned i = 0; i < (useMainSegmentOnly ? strip.getMainSegment().length() : totalLen); i++)
163-181: Consistent and correct implementation for DRGB mode.The changes mirror the RGB mode implementation correctly, accounting for the additional dimmer channel in DRGB mode. The logic properly handles 4-5 channels depending on white channel presence.
For consistency and improved readability, apply the same parentheses improvements as suggested for RGB mode:
- dmxChannelCount = (useMainSegmentOnly ? strip.getMainSegment().hasWhite() : strip.hasWhiteChannel()) ? 5 : 4; + dmxChannelCount = ((useMainSegmentOnly ? strip.getMainSegment().hasWhite() : strip.hasWhiteChannel()) ? 5 : 4);- for (unsigned i = 0; i < useMainSegmentOnly ? strip.getMainSegment().length() : totalLen; i++) + for (unsigned i = 0; i < (useMainSegmentOnly ? strip.getMainSegment().length() : totalLen); i++)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/e131.cpp(2 hunks)
🔇 Additional comments (4)
wled00/e131.cpp (4)
125-125: Good addition of dynamic channel counting.The introduction of
dmxChannelCountvariable enables dynamic determination of required DMX channels based on hardware configuration, which aligns perfectly with the PR objectives.
155-155: Improved bounds checking with dynamic channel count.The replacement of fixed channel count checks with dynamic
dmxChannelCountvalidation significantly improves safety and correctness. The code now properly validates that sufficient DMX data is available based on the actual hardware configuration.Also applies to: 170-170
157-157: Safe and correct conditional white channel handling.The conditional reading of the white channel value ensures that it's only accessed when the hardware configuration supports it and sufficient DMX data is available. The fallback to 0 when no white channel is present maintains backwards compatibility.
Also applies to: 177-177
123-181: Excellent implementation addressing the PR objectives.The changes successfully implement the dynamic white channel handling as described in the PR objectives. Key improvements include:
- Dynamic channel counting: Now based on actual hardware capabilities rather than
availDMXLen- Proper differentiation: Correctly uses
useMainSegmentOnlyflag as requested in past reviews- Improved safety: Better bounds checking with dynamic channel count validation
- Consistent implementation: Both RGB and DRGB modes handle white channels consistently
- Breaking change handled: The change to explicit white channel usage is properly implemented
The implementation makes white channel usage more explicit and precise, addressing the core issue where
availDMXLendidn't reflect actual LED strip capabilities.
|
P.S. To avoid the breaking change when only sending RGB instead of RGBW, implementation could be changed to auto set What do you think? |
4 participants
Using
availDMXLento choose to utilize white channel is messy because it has nothing to do with ability of the strip. It is the number of channels available in the E1.31/Artnet package. Additional this may lead to unexpected behavior if subsequent DMX start addresses for multiple WLED controllers shall be used.The approach here is clear, if no white channel exist:
And if white channel exists:
So number of DMX channels for both modes is +1 if LEDs have white, else number of channels is as documented.
Summary by CodeRabbit