extended CCT blending: exclusive blend, bugfix in CCT calculation (fixes color jump) #5382
Open
extended CCT blending: exclusive blend, bugfix in CCT calculation (fixes color jump) #5382
Conversation
- CCT from RGB has to be calculated from original color to be accurate - negative blend values create "exclusive" zones where only one channel is on, blending happens in the center, total is always 255 (non additive)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCCT blending moved from unsigned 0–100 to signed −100..+100 and now drives multi-branch WW/CW computation. Bus::autoWhiteCalc was changed to output per-pixel WW and CW via reference params; bus internals, accessors, call sites, and the LED settings UI were updated to propagate the new WW/CW and blending semantics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wled00/bus_manager.cpp (3)
706-707: Uninitialized dummy variables. Since_hasCCTis alwaysfalsefor network buses,ww/cware never written or read. Consider zero-initializing for consistency withBusDigital::setPixelColor(line 265).Proposed fix
- uint8_t ww, cw; // dummy, unused + uint8_t ww = 0, cw = 0; // dummy, unused (network buses have no CCT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 706 - 707, The dummy variables ww and cw used as arguments to autoWhiteCalc are not initialized when _hasCCT is false (network buses), causing undefined behavior; initialize ww and cw to 0 before calling autoWhiteCalc (similar to BusDigital::setPixelColor) or otherwise ensure they are assigned a defined value when passed, and keep the existing conditional on _hasWhite so autoWhiteCalc receives valid inputs.
463-464: InitializeCCTww/CCTcwto avoid relying on implicit initialization path.These variables are uninitialized but are read in the
TYPE_ANALOG_2CHandTYPE_ANALOG_5CHswitch cases. WhilecalculateCCTwill set them for those bus types (since theyhasCCT()), zero-initializing matches the pattern used inBusDigital::setPixelColor(line 265) and avoids compiler warnings or accidental UB if bus type logic changes.Proposed fix
- uint8_t CCTww, CCTcw; + uint8_t CCTww = 0, CCTcw = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 463 - 464, The variables CCTww and CCTcw are used uninitialized for some bus types; initialize them to zero where they are declared before calling autoWhiteCalc in setPixelColor: change the declaration of uint8_t CCTww, CCTcw to explicitly zero-initialize (e.g., uint8_t CCTww = 0, CCTcw = 0) so that TYPE_ANALOG_2CH and TYPE_ANALOG_5CH cases that read them (and calculateCCT when hasCCT() is true) do not rely on implicit initialization—follow the same pattern as BusDigital::setPixelColor and keep calling autoWhiteCalc(c, CCTww, CCTcw) for non-TYPE_ANALOG_3CH.
81-93: New CCT blending logic looks correct for the expected_cctBlendrange (−127 to +127).One minor robustness concern: line 85 computes
rangeasuint16_t. If_cctBlendwere ever −128 (the minimumint8_t), then2 * (uint8_t)(128) = 256and255 − 256underflows to 65535 asuint16_t. WhilesetCCTBlendclamps to ±127,_cctBlendis aprotectedstatic so derived classes or future changes could theoretically set it directly. Consider astatic_assertor a defensive clamp.Optional: defensive clamp on line 85
- uint16_t range = 255 - 2 * (uint8_t)(-_cctBlend); + int blendAbs = _cctBlend > -128 ? -_cctBlend : 127; // guard against int8_t minimum + uint16_t range = (255 > 2 * blendAbs) ? 255 - 2 * blendAbs : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 81 - 93, The computation of range uses uint16_t and can underflow if _cctBlend ever equals -128; modify the CCT blending code around _cctBlend and ww_val (and the local variable range) to defensively ensure _cctBlend is within [-127,127] before using it (either by clamping the value into a local int8_t/int or by changing range to a signed type and guarding the -128 case), or add a static_assert that _cctBlend cannot be -128 plus a runtime clamp in setCCTBlend and/or before the range calculation to prevent the 255 - 2*(uint8_t)(-_cctBlend) underflow. Ensure you update the logic that computes ww_val and cw_val to use the clamped/local-safe _cctBlend so behavior remains identical for valid inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.h`:
- Line 204: Typo in the inline comment for getCCTBlend: change "returns -100 t
+100" to "returns -100 to +100" on the line with static inline int8_t
getCCTBlend(), leaving the rest of the explanatory text ("/+/-100% = +/-127.
+/-64 for rounding") intact; update the comment string near _cctBlend to read
"returns -100 to +100" to clarify the range.
---
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Around line 706-707: The dummy variables ww and cw used as arguments to
autoWhiteCalc are not initialized when _hasCCT is false (network buses), causing
undefined behavior; initialize ww and cw to 0 before calling autoWhiteCalc
(similar to BusDigital::setPixelColor) or otherwise ensure they are assigned a
defined value when passed, and keep the existing conditional on _hasWhite so
autoWhiteCalc receives valid inputs.
- Around line 463-464: The variables CCTww and CCTcw are used uninitialized for
some bus types; initialize them to zero where they are declared before calling
autoWhiteCalc in setPixelColor: change the declaration of uint8_t CCTww, CCTcw
to explicitly zero-initialize (e.g., uint8_t CCTww = 0, CCTcw = 0) so that
TYPE_ANALOG_2CH and TYPE_ANALOG_5CH cases that read them (and calculateCCT when
hasCCT() is true) do not rely on implicit initialization—follow the same pattern
as BusDigital::setPixelColor and keep calling autoWhiteCalc(c, CCTww, CCTcw) for
non-TYPE_ANALOG_3CH.
- Around line 81-93: The computation of range uses uint16_t and can underflow if
_cctBlend ever equals -128; modify the CCT blending code around _cctBlend and
ww_val (and the local variable range) to defensively ensure _cctBlend is within
[-127,127] before using it (either by clamping the value into a local int8_t/int
or by changing range to a signed type and guarding the -128 case), or add a
static_assert that _cctBlend cannot be -128 plus a runtime clamp in setCCTBlend
and/or before the range calculation to prevent the 255 - 2*(uint8_t)(-_cctBlend)
underflow. Ensure you update the logic that computes ww_val and cw_val to use
the clamped/local-safe _cctBlend so behavior remains identical for valid inputs.
Member
|
@babadam2002 please test, your feedback is welcome 😃 |
Collaborator
Author
|
I found a bug: now global brightness is no longer applied to CCT. easy fix though. |
|
I test it. When the cct bending is at -100 and if I adjust it minimally towards warm or cold, it suddenly jumps to the other white LED with max brightness. There is no nice transition between them. For example, when I turn the color to completely white, only the cold white LED lights up, which is good, but when I turn it very slightly towards warmer, the cold white LED does not fade out nicely, but suddenly turns off and the warm white LED suddenly turns on at max. |
Collaborator
Author
|
that is what -100% means. no transition just jump. |
|
I just need the cold white without jumping. |
|
With your modified code and my code with "0 cct blending" I was able to do the white led treatment, with this the two white leds have a nice transition. but this is only optimized for rgb+cct. The program may not be very accurate, but you can improve it. void Bus::calculateCCT(uint32_t c, uint8_t &ww, uint8_t &cw) {
unsigned cct = 0;
unsigned w = W(c);
// Source definition (normalized to 0-255 range)
if (_cct > -1) {
if (_cct >= 1900) cct = (_cct - 1900) >> 5;
else if (_cct < 256) cct = _cct;
} else {
cct = (approximateKelvinFromRGB(c) - 1900) >> 5;
}
// My CUSTOM ZONE LOGIC (No jumps, just smooth transitions)
if (cct <= 127) {
// ZONE 1: Warm to Pure White (0 - 127)
float rel_cct = cct / 127.0f;
// WW: Quadratic decay (this makes it silky and doesn't jump)
float ww_factor = (1.0f - rel_cct) * (1.0f - rel_cct);
ww = (uint8_t)(ww_factor * 255.0f);
// CW: Linear ramp up
cw = (uint8_t)(rel_cct * 255.0f);
}
else {
// ZONE 2: From Pure White to Extra Cold (128 - 255)
float extra_cold_factor = (cct - 127) / 128.0f;
ww = 0; // Warm white is dead here
// We move the CW LED back a little at the end of the blue zone
cw = (uint8_t)(255 - (extra_cold_factor * 55));
}
// Brightness scaling (based on the calculated white channel)
ww = (w * ww) / 255;
cw = (w * cw) / 255;
}
uint32_t Bus::autoWhiteCalc(uint32_t c, uint8_t &ww, uint8_t &cw) const {
unsigned aWM = _autoWhiteMode;
if (_gAWM < AW_GLOBAL_DISABLED) aWM = _gAWM;
CRGBW cIn = c; // SAVE THE ORIGINAL COLOR (This is the essence of the bugfix!)
unsigned w = W(c);
if (aWM != RGBW_MODE_MANUAL_ONLY) {
unsigned r = R(c);
unsigned g = G(c);
unsigned b = B(c);
if (aWM == RGBW_MODE_DUAL && w > 0) {
// In DUAL mode, if there is white, we do not touch it
} else if (aWM == RGBW_MODE_MAX) {
w = r > g ? (r > b ? r : b) : (g > b ? g : b);
} else {
w = r < g ? (r < b ? r : b) : (g < b ? g : b);
// ACCURATE mode: Subtract white from RGB
if (aWM == RGBW_MODE_AUTO_ACCURATE) { r -= w; g -= w; b -= w; }
}
c = RGBW32(r, g, b, w);
}
if (_hasCCT) {
// Here's the trick: we use the original RGB (cIn) for the calculation,
// but we pass the white brightness (w) to it as fresh.
cIn.w = w;
calculateCCT(cIn, ww, cw);
}
return c;
} |
Member
This is a different (asymmetric) blending logic, as it does not sum up to 100% for every CCT value. I'm not against it, but its a special case that should be "opt-in" with a user setting. |
Collaborator
Author
it does not, it buttery smooth if you set it right. |
Member
@babadam2002 I'm not sure that you only "need the cold white". Your code example says something different - it modifies the WW, and CW. It means you'll also need the warm white - otherwise why would we do all this experiments, if you could simply buy an SK6812 "RGBW" strip with a single cold white channel 🤔. Please forgive us simple engineers - a few extra words would really help us to understand what is necessary 😉 |
|
Sorry, my English is not very good. |
Collaborator
Author
I already did. your math is actually incorrect or at least not at all based on color theory / black body radiation. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 475-476: The local variables cctWW and cctCW are declared but not
initialized before use; initialize them to 0 where declared and update the TODO
comment typo to "use cw and ww". Specifically, in the scope declaring uint8_t
cctWW, cctCW (used around the call to autoWhiteCalc and the switch handling
TYPE_ANALOG_1CH/3CH/4CH), change the declaration to initialize both to 0 and
correct the inline TODO from "use cw ad ww" to "use cw and ww" so intent is
explicit and compiler warnings are avoided.
- Around line 233-239: The CW value is being truncated because cctCW is declared
as uint8_t but the expression "((cctCW + 1) * _bri) & 0xFF00" produces an
upper-byte 16-bit value; fix by performing the brightness scaling into a 16-bit
temporary (mirror setPixelColor pattern) so the CW upper byte is preserved and
then combine into wwcw (e.g., keep cctWW as uint8_t, cast/compute cctCW scaled
into uint16_t before ORing into wwcw after Bus::calculateCCT), and also correct
the comment typo "brightess" → "brightness".
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wled00/bus_manager.cpp (2)
715-716: Dummy WW/CW variables — consider initializing for safety.
wwandcware passed by reference toautoWhiteCalcbut only written when_hasCCTis true (which isfalseforBusNetwork). They're correctly documented as dummy/unused, but initializing to0would silence potential compiler warnings and prevent any future regression if_hasCCTlogic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 715 - 716, Initialize the dummy variables ww and cw to zero before passing them to autoWhiteCalc to avoid compiler warnings and future regressions: change the declaration of ww and cw (used when _hasWhite is true and passed by reference to autoWhiteCalc) from uninitialized uint8_t ww, cw to initialized values (e.g., uint8_t ww = 0, cw = 0) so BusNetwork cases where _hasCCT is false remain safe; locate the declaration near the call site in bus_manager.cpp where autoWhiteCalc(c, ww, cw) is invoked.
566-566: PWM dead-time condition should also cover negative_cctBlendvalues.Now that
_cctBlendisint8_t, negative values (exclusive mode) also guarantee non-overlapping WW/CW signals. For both_cctBlend == 0(standard crossfade) and_cctBlend < 0(exclusive), the relationshipcw_val = 255 - ww_valensures the sum never exceeds 255. Dead-time optimization applies equally to both cases but currently only== 0triggers it.♻️ Proposed fix
- if (_type == TYPE_ANALOG_2CH && Bus::_cctBlend == 0) { + if (_type == TYPE_ANALOG_2CH && Bus::_cctBlend <= 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` at line 566, The PWM dead-time optimization currently only activates when Bus::_cctBlend == 0, but since Bus::_cctBlend is int8_t negative values (exclusive mode) also guarantee non-overlapping WW/CW (cw_val = 255 - ww_val); change the condition in the branch that checks _type == TYPE_ANALOG_2CH so it tests Bus::_cctBlend <= 0 instead of == 0, ensuring both standard crossfade (0) and exclusive (<0) use the dead-time path; update any adjacent comment to reflect the new <= 0 logic and verify behavior where cw_val and ww_val are computed remains correct for negative blend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 233-241: The CCT channels are being scaled with the old _bri
(double-applying ABL) — in the hasCCT() block where Bus::calculateCCT(c, cctWW,
cctCW) is called and wwcw is computed, replace uses of _bri with newBri so the
computed CCT upper/lower bytes are scaled by the same final brightness as the
RGB path (matching the subsequent color_fade(c, newBri, true) and
PolyBus::setPixelColor call); update the expressions that build wwcw (which
currently multiply by _bri) to multiply by newBri instead.
- Around line 84-87: _cctBlend can be -128 (int8_t) and causes uint16_t range
underflow in the block computing range and ww_val/cw_val; fix by clamping
_cctBlend into [-127,127] at its setter (the function that assigns _cctBlend)
or, if you prefer a local guard, compute range using a signed intermediate and
ensure range is >= 0 before casting (e.g. compute int r = 255 - 2 *
(int8_t)(-_cctBlend); if (r < 0) r = 0; then use r for ww_val/cw_val), and
update references to ww_val and cw_val in this calculation accordingly.
---
Duplicate comments:
In `@wled00/bus_manager.cpp`:
- Around line 474-475: The variables cctWW and cctCW declared before the
conditional are left uninitialized and the TODO has a typo; initialize both to 0
when declared (uint8_t cctWW = 0, cctCW = 0) and then call autoWhiteCalc(c,
cctWW, cctCW) only when _type != TYPE_ANALOG_3CH as written; also update the
TODO text from "use cw ad ww" to "use cw and ww". Ensure the change is made in
the same scope where the current declaration and call to autoWhiteCalc occur.
---
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Around line 715-716: Initialize the dummy variables ww and cw to zero before
passing them to autoWhiteCalc to avoid compiler warnings and future regressions:
change the declaration of ww and cw (used when _hasWhite is true and passed by
reference to autoWhiteCalc) from uninitialized uint8_t ww, cw to initialized
values (e.g., uint8_t ww = 0, cw = 0) so BusNetwork cases where _hasCCT is false
remain safe; locate the declaration near the call site in bus_manager.cpp where
autoWhiteCalc(c, ww, cw) is invoked.
- Line 566: The PWM dead-time optimization currently only activates when
Bus::_cctBlend == 0, but since Bus::_cctBlend is int8_t negative values
(exclusive mode) also guarantee non-overlapping WW/CW (cw_val = 255 - ww_val);
change the condition in the branch that checks _type == TYPE_ANALOG_2CH so it
tests Bus::_cctBlend <= 0 instead of == 0, ensuring both standard crossfade (0)
and exclusive (<0) use the dead-time path; update any adjacent comment to
reflect the new <= 0 logic and verify behavior where cw_val and ww_val are
computed remains correct for negative blend.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/bus_manager.cpp`:
- Around line 476-477: The variables cctWW and cctCW are declared but not
initialized before being passed to autoWhiteCalc; initialize them to 0 at
declaration (uint8_t cctWW = 0, cctCW = 0) so the compiler warning is silenced
and intent is explicit—leave the conditional call to autoWhiteCalc(c, cctWW,
cctCW) unchanged and keep the TYPE_ANALOG_3CH branch logic as-is.
- Around line 235-242: Fix the typo "brightess" → "brightness" in the two inline
comments: update the comment next to the CCT brightness application where wwcw
is computed (in the hasCCT() block that calls Bus::calculateCCT and uses
newBri/wwcw) and also correct the same typo in the comment associated with
PolyBus::setPixelColor (the comment referenced around the pixel
repaint/setPixelColor usage). Keep the wording consistent (use "brightness") and
do not change code logic.
- Around line 85-89: The calculation of range uses unsigned arithmetic so when
_cctBlend == -128 it underflows and the hard-switch branch is never hit; change
range to a signed type and compute using signed negation so negative results are
preserved, then clamp into 0..255 to allow the ternary fallback to run for range
== 0. Specifically, update the declaration and computation of range (currently
"uint16_t range = 255 - 2 * (uint8_t)(-_cctBlend);") to use a signed integer
(e.g., int16_t) and compute with (int16_t)(-_cctBlend), then add explicit clamps
like "if (range < 0) range = 0; if (range > 255) range = 255;" so that the
ww_val/cw_val branch (the ternary using cct) executes for maximum exclusivity.
Member
|
@DedeHai is the new CCT blending working already for analog leds? If yes, I can surely did out a few "classic" LEDs and resistors to help with testing. |
Collaborator
Author
|
@softhack007 yes, the changes are already there for PWM, it was pretty straight forward. if you have hardware to test that it would be great. I only have digital RGBCCT. edit: if you check, please confirm WW and CW are not flipped. hm. could actually just ask the rabbit to confirm that :) |
|
Another problem is that when I change the color selector from any color to cold white, the brightness of the cold LED increases nicely, when it reaches the maximum transition, the brightness of the LED jumps a little higher. This was present before. |
Collaborator
Author
I have seen this before but was not able to reproduce it reliably. can you please share on how exactly to make this happen? what led settings, what brightness, what starting and what ending color |
I've seen this with the pwm led mode before. The settings are almost the same as the error above. Maybe I should tell you. When "auto calculate cct from rgb" is turned on and it's set to "None" mode, there's no ugly last jump in brightness, but in "accurate" mode, when the transition from color to white has already happened, it jumps a little brighter at the very end. Maybe the problem could be the brightness code of the white LEDs from the RGB color (if there is one) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
717-718: Initialize dummyww/cwto 0 for consistency.These are correctly annotated as unused, but initializing them matches the pattern in
BusDigital::setPixelColor(line 273) and silences potential compiler warnings.Suggested fix
- uint8_t ww, cw; // dummy, unused + uint8_t ww = 0, cw = 0; // dummy, unused🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 717 - 718, The dummy variables ww and cw in the BusManager code path where autoWhiteCalc is called should be explicitly initialized to 0 to match the pattern used in BusDigital::setPixelColor and avoid compiler warnings; locate the declaration "uint8_t ww, cw;" near the call to autoWhiteCalc(c, ww, cw) and change it so ww and cw are initialized (e.g., uint8_t ww = 0, cw = 0) before calling autoWhiteCalc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/bus_manager.cpp`:
- Around line 235-242: The ABL path recalculates CCT using modified/stored pixel
RGB which can produce Kelvin drift when _cct < 0 and auto-white is ACCURATE;
update the ABL branch in Bus::calculateCCT / the block guarded by hasCCT() so it
does not call approximateKelvinFromRGB on white-subtracted RGB (options: skip
recalculation when _cct < 0 and ACCURATE, or use stored wwcw if available from
setPixelColor, or add a fallback that treats near-zero RGB as preserving
previous wwcw), and correct the comment typos from "brightess" to "brightness"
near color_fade and related comments; ensure adjustments reference hasCCT(),
Bus::calculateCCT(), color_fade(), PolyBus::setPixelColor(), and the _cct flag
so reviewers can find the change.
---
Duplicate comments:
In `@wled00/bus_manager.cpp`:
- Around line 476-477: The variables cctWW and cctCW are declared but not
initialized before being passed to autoWhiteCalc; initialize them (e.g., uint8_t
cctWW = 0, cctCW = 0) or compute/assign proper values before the call, or move
their declaration/assignment inside the branch that calls autoWhiteCalc so they
are always initialized when passed to autoWhiteCalc(c, cctWW, cctCW); update the
line using TYPE_ANALOG_3CH accordingly to ensure no uninitialized use.
- Around line 85-88: The exclusive CCT blending still misbehaves for _cctBlend
== -128 because clamping range to 255 makes the math behave like 0; fix by
enforcing that _cctBlend is clamped to the range [-127, 127] at the setter
(where _cctBlend is assigned) so the computation using range and ww_val never
sees -128; update the setter that assigns _cctBlend (and any public API that
writes it) to coerce values outside that range into [-127,127] and add a brief
comment explaining why -128 is invalid for the exclusive blend path.
---
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Around line 717-718: The dummy variables ww and cw in the BusManager code path
where autoWhiteCalc is called should be explicitly initialized to 0 to match the
pattern used in BusDigital::setPixelColor and avoid compiler warnings; locate
the declaration "uint8_t ww, cw;" near the call to autoWhiteCalc(c, ww, cw) and
change it so ww and cw are initialized (e.g., uint8_t ww = 0, cw = 0) before
calling autoWhiteCalc.
Collaborator
Author
please we need EXACT settings, there are thousands of possibilities. |
Collaborator
Author
|
@coderabbitai please confirm cw and ww channels in all led types are unchanged and there are no unwanted side effects with this PR |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12939
✅ WW and CW channels are correctly maintained across all LED typesChannel ordering verification:Digital buses (RGBCCT/WWA):
PWM/Analog buses:
All call sites:
✅ Core bugfix properly implemented:The PR correctly addresses the known issue (from learning ce8d9c5b) where CRGBW cIn = c; // save original color for CCT calculation
// ... white calculation modifies c ...
if (_hasCCT) {
cIn.w = w; // use original RGB with updated white brightness
calculateCCT(cIn, ww, cw);
}This ensures CCT is calculated from the original RGB before Intentional side effects (improvements):
No unwanted side effects or channel flipping detected. All changes maintain backward compatibility while fixing the CCT calculation bug and adding the new exclusive blending feature. 🐰 🧠 Learnings used |
Messenger_creation_D31A794C-E0F8-4D39-A268-5886E13FBA6D.mp4I have attached a video. In which you can see that I set the lamp to all points of the original Philips HUE rgb+cct led strip + color wheel. In the video you can see the cold white led on the left, the rgb led in the middle, and the warm white led on the right. You can see how they work together. My request is, how can you reproduce this in wled "accurate" mode? |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Understanding Philips Hue RGB+CCT BehaviorFrom your previous comments and the web research, Philips Hue RGB+CCT strips use:
Reproducing This in WLED with This PRBased on the discussion in this PR, here's how to configure WLED to achieve Hue-like behavior: Settings to Try:
What the PR Fixes:This PR addresses two key issues that improve Hue-like behavior:
Could You Help Me Understand:Since I cannot view the video, could you describe:
This will help me provide more specific guidance on the exact settings to use! Testing Recommendation:Try these steps in WLED with this PR:
The behavior should now be much closer to Philips Hue, with smooth transitions and no abrupt jumps at the center. Let me know how this works for you! 🐰 🧠 Learnings used |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: Philips Hue “White and color ambiance” lightstrips (e.g., Lightstrip Plus / Gradient families) are effectively 5‑channel strips: R, G, B + warm‑white + cool‑white LEDs. They’re not “RGB only” and not “RGBW with a single fixed-white”; they have separate warm and cool white emitters. [1], [2] What that means for white/CCT blending
Important exception: Hue “Essential” lightstripHue’s lower‑cost Essential strip is reported to lack dedicated white LEDs (RGB only), so any “white/CCT” it offers is produced by RGB mixing, not WW/CW blending. [3] Sources: Citations:
What I see in your screenshotThe Comment image shows a video recording interface with three LEDs visible at the top:
At the bottom is the Philips Hue app interface with a circular color wheel selector positioned in the cyan region, brightness at 25%, and a video timeline (4:12/4:30) showing multiple color keyframes demonstrating the RGB+CCT transitions across different colors. How Philips Hue handles RGB+CCT blendingBased on technical documentation, Philips Hue "White and color ambiance" strips are 5-channel devices (R, G, B, warm-white, cool-white) that can drive all five channels simultaneously in color mode to achieve the requested color more efficiently. This means even when selecting a saturated color like cyan, Hue may add some white channel contribution to boost brightness and efficiency—exactly what you see in your video with the cold white LED slightly active. How to reproduce this in WLED "accurate" modeWith this PR's improvements, you can achieve similar behavior: Required settings:
Expected behavior:
Try starting with CCT blending = +20 and Auto White = Accurate mode, then fine-tune the blending value to match your preference! 🎨 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/bus_manager.cpp (1)
717-718: Initialize dummy variables for consistency.While the "dummy, unused" comment is clear and the uninitialized state doesn't cause issues, initializing
wwandcwto 0 would be consistent with the pattern inBusDigital::setPixelColor(line 273) and prevent any potential compiler warnings.♻️ Minor cleanup suggestion
- uint8_t ww, cw; // dummy, unused + uint8_t ww = 0, cw = 0; // dummy, unused🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/bus_manager.cpp` around lines 717 - 718, The local variables ww and cw are declared uninitialized before being passed into autoWhiteCalc; initialize them to zero for consistency and to avoid compiler warnings by changing the declaration of uint8_t ww, cw in the block that calls autoWhiteCalc (the same pattern used in BusDigital::setPixelColor) so they are set to 0 before use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/bus_manager.cpp`:
- Around line 476-477: Initialize the local variables cctWW and cctCW to 0 when
declaring them to avoid warnings and make intent explicit: change the
declaration of uint8_t cctWW, cctCW; to initialize both (e.g., uint8_t cctWW =
0, cctCW = 0;) before the conditional that calls autoWhiteCalc; keep the
existing logic involving _type, TYPE_ANALOG_3CH, autoWhiteCalc, _hasCCT and
calculateCCT unchanged.
---
Nitpick comments:
In `@wled00/bus_manager.cpp`:
- Around line 717-718: The local variables ww and cw are declared uninitialized
before being passed into autoWhiteCalc; initialize them to zero for consistency
and to avoid compiler warnings by changing the declaration of uint8_t ww, cw in
the block that calls autoWhiteCalc (the same pattern used in
BusDigital::setPixelColor) so they are set to 0 before use.
Collaborator
Author
|
@coderabbitai I have noticed another peculiar behaviour that is not directly related to this PR. When using the value/brightness slider from the color UI (not global brightness) and dimming the color at the lowest end (most visible for slider values < 10) the color transitions are not smooth but fluctuate in brightness and even different hues. It is independend of auto white calculation mode and also not related to gamma correction. please explain how that slider progresses through the color calculation pipeline and check for potential issues that could explain the described behaviour. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 50676 🏁 Script executed: Length of output: 46185 🏁 Script executed: Length of output: 34135 🏁 Script executed: Length of output: 3898
Root Cause AnalysisThe ProblemThe color picker flow is: Value slider → iro.js HSV→RGB conversion → At low HSV values (<10%), the iro.js color picker suffers from severe quantization errors:
Why It FluctuatesEvery slider adjustment triggers a new HSV→RGB→HSV round-trip:
Example: At v=10%, s=100%, changing hue from 0° to 2°: The 1-bit change in R causes the backend to perceive a different brightness. Why This Occurs
Solutions (for future consideration)This is a pre-existing UI issue, not introduced by your CCT blending PR:
The issue affects all WLED installations using the current iro.js color picker and becomes most apparent when users dim colors to very low brightness levels using the value slider. Your CCT blending changes do not interact with this code path. 🐰 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai no, i am using the wled internal transition and only click the picker once. Say start point is 0f0a0d, transition to 0 using that slider and a transition time of 5s. the last few transition steps even show greenish tint. Am I correct to assume the UI only sends one single command and the transition happens in WLED code? If so please trace the called functions as requested and re-trace the issue I discribed. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 13460 🏁 Script executed: Length of output: 61320 🏁 Script executed: Length of output: 6995
🏁 Script executed: Length of output: 5442 🏁 Script executed: Length of output: 3568 🏁 Script executed: Length of output: 99 Let me use Python instead to analyze the color_blend precision issue: 🏁 Script executed: Length of output: 1441 🏁 Script executed: Length of output: 1175
Let me trace where the blended colors go and what additional processing happens: 🏁 Script executed: Length of output: 1141 🏁 Script executed: Length of output: 6175 🏁 Script executed: Length of output: 5118
Let me trace the exact issue: 🏁 Script executed: Length of output: 1425 🏁 Script executed: Length of output: 534
The greenish tint (and other color shifts) during transitions is caused by The ProblemHere's what happens in if (hasWhite()) c = autoWhiteCalc(c, cctWW, cctCW);Every pixel color - including intermediate transition colors - goes through
Example with your test case (0f0a0d → 000000):Why This HappensAt low brightness, the channel with the minimum value gets completely zeroed by the white subtraction, while other channels retain fractional values. This destroys the color ratio that the transition tried to maintain, causing visible color shifts throughout the fade. The FixThe solution is to preserve the original color before This is a pre-existing bug in WLED that affects all transitions when using "accurate" white mode, not introduced by your PR. Your PR actually improves things by preserving the original color for CCT calculation! Would you like me to help create a separate issue for tracking this autoWhiteCalc transition bug? 🐰 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai it also happens when using auto white calc = none so that can not be it. it must be something to do with low brightness color calculation. could it be the "hue preservation" feature I added and not working correctly or are there other sources for such error in color accuracy? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7850 🏁 Script executed: Length of output: 12231 🏁 Script executed: Length of output: 1893 🏁 Script executed: Length of output: 1487 🏁 Script executed: Length of output: 1401
🎯 ROOT CAUSE: Gamma Correction ThresholdThe gamma lookup table (with default gamma=2.8) has a critical behavior at low brightness:
Your test color Any color with all channels below 28 becomes completely BLACK after gamma correction! 🔍 Analysis of Transition BehaviorDuring your 5-second transition from
However, you report seeing a greenish tint rather than immediate black-out. This suggests:
❓ About Hue PreservationThe hue preservation feature you added is in the // Removes channels <13% of max to prevent hue distortion
addRemains = r && (r<<5) > maxc ? 0x00010000 : 0;But this is NOT used in transitions! Transitions use
🤔 Questions for ClarificationTo identify the exact issue:
The gamma threshold at 28 is a fundamental limitation - any fix for low-brightness color accuracy may need to address the gamma table itself (e.g., using a lower gamma value or implementing a minimum brightness floor). 🐰 🧠 Learnings used |
Collaborator
Author
|
@coderabbitai I already said its not related to gamma, gamma is disabled. so it must be the color blend function. can it be improved similar to color fade? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4874 🏁 Script executed: Length of output: 4082 🏁 Script executed: Length of output: 1141
🎯 Root CauseAt low brightness, The channels lose their relative proportions due to integer truncation, causing the greenish tint you observed. ✅ Solution: Apply Hue Preservation from
|
Collaborator
Author
|
I tested the rabbits solution on my new found "issue" and its not doing anything. Then also this behaviour is the same as it was since 0.14 so no regression. Probably just hitting the 8-bit math limit there and not much can be done without workarounds and slowing down the hot path in the process. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tested on digital strip only, transitions and color accuracy is much improved in "auto white calc: accurate" mode
Fixes #5377
Summary by CodeRabbit
New Features
Documentation / UI