-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add per-bus white-LED color temperature for accurate auto-white #5654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 16 commits
d7e45b4
01fe430
113b3dc
cd00e56
cd6bf0e
38d19b0
5f61870
b42e4e1
5312bd0
15016d6
adefb08
7f7e78a
9965ca6
c26fc17
18bdf30
988d833
d9881b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,32 @@ void Bus::calculateCCT(uint32_t c, uint8_t &ww, uint8_t &cw) { | |
| cw = (w * cw) / 255; | ||
| } | ||
|
|
||
| // AI: below section was generated by an AI | ||
| // recompute cached W-LED RGB equivalent when the configured Kelvin changes; | ||
| // 0 means "treat the W LED as neutral white" which preserves legacy behavior | ||
| // where autoWhiteCalc subtracted the same value from R, G, B. | ||
| void Bus::setWhiteKelvin(uint16_t k) { | ||
| _whiteKelvin = k; | ||
| if (k == 0) { | ||
| _wR = _wG = _wB = 255; // legacy: treat W as neutral | ||
| } else { | ||
| byte rgb[4]; | ||
| colorKtoRGB(k, rgb); | ||
| _wR = rgb[0]; _wG = rgb[1]; _wB = rgb[2]; | ||
| } | ||
| // Precompute fixed-point reciprocals (Q15) so autoWhiteCalc's hot path can | ||
| // replace the per-pixel division (channel*255)/_wX with (channel*_rwX)>>15. | ||
| // floor() here under-estimates the reciprocal, so the derived w cap can only | ||
| // come out <= the true floor value — never larger — keeping the subtraction | ||
| // underflow-safe (verified exact across all channel/_wX combinations: max | ||
| // error 1, no over-estimate; max product 255*_rwX fits uint32). _rwX==0 is the | ||
| // "channel does not constrain w" sentinel, matching the _wX==0 guard below. | ||
| _rwR = _wR ? ((255U << 15) / _wR) : 0; | ||
| _rwG = _wG ? ((255U << 15) / _wG) : 0; | ||
| _rwB = _wB ? ((255U << 15) / _wB) : 0; | ||
| } | ||
| // AI: end | ||
|
|
||
| // calculates white channel and CCT values based on given settings | ||
| uint32_t Bus::autoWhiteCalc(uint32_t c, uint8_t &ww, uint8_t &cw) const { | ||
| unsigned aWM = _autoWhiteMode; | ||
|
|
@@ -112,9 +138,43 @@ uint32_t Bus::autoWhiteCalc(uint32_t c, uint8_t &ww, uint8_t &cw) const { | |
| //ignore auto-white calculation if w>0 and mode DUAL (DUAL behaves as BRIGHTER if w==0) | ||
| } else if (aWM == RGBW_MODE_MAX) { | ||
| w = r > g ? (r > b ? r : b) : (g > b ? g : b); // brightest RGB channel | ||
| } else if (_whiteKelvin == 0 || _hasCCT || !_hasRgb) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be done in the UI, avoid conditionals in the hot path if possible |
||
| // Fast path: per-bus W channel color temperature feature is off, OR the bus type can't | ||
| // use it — dual-white CCT buses have a variable white point set via | ||
| // the CCT control (not a single fixed Kelvin), and non-RGB buses have | ||
| // nothing to derive the correction from. Identical to the pre-feature | ||
| // behavior: pick darkest RGB channel as W and (for ACCURATE) subtract | ||
| // it equally. Also avoids three divisions per pixel in the common | ||
| // default case, since most strips never enable the feature. | ||
| w = r < g ? (r < b ? r : b) : (g < b ? g : b); | ||
| if (aWM == RGBW_MODE_AUTO_ACCURATE) { r -= w; g -= w; b -= w; } | ||
| } else { | ||
| w = r < g ? (r < b ? r : b) : (g < b ? g : b); // darkest RGB channel | ||
| if (aWM == RGBW_MODE_AUTO_ACCURATE) { r -= w; g -= w; b -= w; } //subtract w in ACCURATE mode | ||
| // AI: below section was generated by an AI | ||
| // Per-channel cap path (feature on): pick the largest w such that | ||
| // (w * _wX)/255 <= channel for every X in {R,G,B}, preventing | ||
| // underflow when subtracting the W LED's RGB contribution. Floor | ||
| // division composes back through the subtract — i.e. | ||
| // floor((r*255)/_wR) * _wR <= r*255 — so the subtraction is safe. | ||
| // Hot path: the (channel*255)/_wX divisions are replaced by the Q15 | ||
| // reciprocals precomputed in setWhiteKelvin (multiply + shift, no | ||
| // per-pixel divide). _rwX is floor-biased so wMax never over-estimates, | ||
| // keeping the cap underflow-safe. _rwX==0 means _wX==0 (channel doesn't | ||
| // constrain w — _wB is 0 at/below 1900 K, _wG only at extreme lows), | ||
| // matching the previous per-channel zero guards. The /255 in the | ||
| // subtraction stays: 255 is a compile-time constant the compiler already | ||
| // strength-reduces, so it isn't an actual division. | ||
| unsigned wMaxR = _rwR ? (r * _rwR) >> 15 : 255U; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove conditionals, _rwR is never zero, _rwG is also never zero and the multiplication + shift is usually faster than a branch as it lets the compiler optimize more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Last time I worked through the math, I thought there was a case where _rwR can be zero, but I also want to simply this. I will look into this some more |
||
| unsigned wMaxG = _rwG ? (g * _rwG) >> 15 : 255U; | ||
| unsigned wMaxB = _rwB ? (b * _rwB) >> 15 : 255U; | ||
| unsigned wCap = wMaxR < wMaxG ? (wMaxR < wMaxB ? wMaxR : wMaxB) : (wMaxG < wMaxB ? wMaxG : wMaxB); | ||
| if (wCap > 255U) wCap = 255U; | ||
| w = wCap; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this changes the MODE_MIN behaviour. intentional? if so why only min and not MODE_MAX?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was primarily intended for MODE_AUTO_ACCURATE and DUAL because they advertise themselves as an "accurate" translation of RGB into RGBW (and ACCURATE is the mode I use on my daily-driver WLED controllers). But it may be extended to include MODE_MAX if desired, although the Please let me know if I am misunderstanding what you are referring to as MODE_MIN and MODE_MAX |
||
| if (aWM == RGBW_MODE_AUTO_ACCURATE) { | ||
| r -= (w * _wR) / 255; // subtract W LED's R contribution | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. still uses division, why not right shift?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it divided by a constant, the compiler already converts that to a shift at compile time. But it can just as easily be explicitly written as a right shift |
||
| g -= (w * _wG) / 255; // subtract W LED's G contribution | ||
| b -= (w * _wB) / 255; // subtract W LED's B contribution | ||
| } | ||
| // AI: end | ||
| } | ||
| c = RGBW32(r, g, b, w); | ||
| } | ||
|
|
@@ -1226,6 +1286,7 @@ int BusManager::add(const BusConfig &bc, bool placeholder) { | |
| } else { | ||
| busses.push_back(make_unique<BusPwm>(bc)); | ||
| } | ||
| if (!busses.empty()) busses.back()->setWhiteKelvin(bc.whiteKelvin); | ||
| return busses.size(); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ make_unique(Args&&... args) | |
|
|
||
| //colors.cpp | ||
| uint16_t approximateKelvinFromRGB(uint32_t rgb); | ||
| void colorKtoRGB(uint16_t kelvin, byte* rgb); | ||
|
|
||
| #define GET_BIT(var,bit) (((var)>>(bit))&0x01) | ||
| #define SET_BIT(var,bit) ((var)|=(uint16_t)(0x0001<<(bit))) | ||
|
|
@@ -121,6 +122,10 @@ class Bus { | |
| , _reversed(reversed) | ||
| , _valid(false) | ||
| , _needsRefresh(refresh) | ||
| , _whiteKelvin(0) | ||
| , _wR(255) | ||
| , _wG(255) | ||
| , _wB(255) | ||
| { | ||
| _autoWhiteMode = Bus::hasWhite(type) ? aw : RGBW_MODE_MANUAL_ONLY; | ||
| }; | ||
|
|
@@ -162,6 +167,8 @@ class Bus { | |
| inline void setStart(uint16_t start) { _start = start; } | ||
| inline void setAutoWhiteMode(uint8_t m) { if (m < 5) _autoWhiteMode = m; } | ||
| inline uint8_t getAutoWhiteMode() const { return _autoWhiteMode; } | ||
| inline uint16_t getWhiteKelvin() const { return _whiteKelvin; } | ||
| void setWhiteKelvin(uint16_t k); | ||
| inline size_t getNumberOfChannels() const { return hasWhite() + 3*hasRGB() + hasCCT(); } | ||
| inline uint16_t getStart() const { return _start; } | ||
| inline uint8_t getType() const { return _type; } | ||
|
|
@@ -221,6 +228,13 @@ class Bus { | |
| uint8_t _autoWhiteMode; // global Auto White Calculation override | ||
| uint16_t _start; | ||
| uint16_t _len; | ||
| uint16_t _whiteKelvin; // physical W channel color temperature in Kelvin (0 = neutral/legacy behavior) | ||
| uint8_t _wR; // cached W LED RGB equivalent (255,255,255 when _whiteKelvin==0) | ||
| uint8_t _wG; | ||
| uint8_t _wB; | ||
| uint32_t _rwR; // Q15 reciprocal of _wR (floor((255<<15)/_wR), 0 if _wR==0) for autoWhiteCalc hot path | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uint16_t should be accurate enough I think, no need to waste RAM |
||
| uint32_t _rwG; | ||
| uint32_t _rwB; | ||
|
Comment on lines
+238
to
+240
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initialize reciprocal cache members in
Proposed fix , _whiteKelvin(0)
, _wR(255)
, _wG(255)
, _wB(255)
+ , _rwR(0)
+ , _rwG(0)
+ , _rwB(0)
{🤖 Prompt for AI Agents |
||
| //struct { //using bitfield struct adds abour 250 bytes to binary size | ||
| bool _reversed;// : 1; | ||
| bool _valid;// : 1; | ||
|
|
@@ -461,6 +475,7 @@ struct BusConfig { | |
| uint8_t skipAmount; | ||
| bool refreshReq; | ||
| uint8_t autoWhite; | ||
| uint16_t whiteKelvin; // physical W channel color temperature in Kelvin (0 = neutral/legacy behavior) | ||
| uint8_t pins[OUTPUT_MAX_PINS] = {255, 255, 255, 255, 255}; | ||
| uint16_t frequency; | ||
| uint8_t milliAmpsPerLed; | ||
|
|
@@ -469,13 +484,14 @@ struct BusConfig { | |
| uint8_t iType; // internal bus type (I_*) determined during memory estimation, used for bus creation | ||
| String text; | ||
|
|
||
| BusConfig(uint8_t busType, uint8_t* ppins, uint16_t pstart, uint16_t len = 1, uint8_t pcolorOrder = COL_ORDER_GRB, bool rev = false, uint8_t skip = 0, byte aw=RGBW_MODE_MANUAL_ONLY, uint16_t clock_kHz=0U, uint8_t maPerLed=LED_MILLIAMPS_DEFAULT, uint16_t maMax=ABL_MILLIAMPS_DEFAULT, uint8_t driver=0, String sometext = "") | ||
| BusConfig(uint8_t busType, uint8_t* ppins, uint16_t pstart, uint16_t len = 1, uint8_t pcolorOrder = COL_ORDER_GRB, bool rev = false, uint8_t skip = 0, byte aw=RGBW_MODE_MANUAL_ONLY, uint16_t clock_kHz=0U, uint8_t maPerLed=LED_MILLIAMPS_DEFAULT, uint16_t maMax=ABL_MILLIAMPS_DEFAULT, uint8_t driver=0, String sometext = "", uint16_t whiteK=0) | ||
| : count(std::max(len,(uint16_t)1)) | ||
| , start(pstart) | ||
| , colorOrder(pcolorOrder) | ||
| , reversed(rev) | ||
| , skipAmount(skip) | ||
| , autoWhite(aw) | ||
| , whiteKelvin(whiteK) | ||
| , frequency(clock_kHz) | ||
| , milliAmpsPerLed(maPerLed) | ||
| , milliAmpsMax(maMax) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -238,6 +238,7 @@ bool deserializeConfig(JsonObject doc, bool fromFS) { | |
| bool refresh = elm["ref"] | false; | ||
| uint16_t freqkHz = elm[F("freq")] | 0; // will be in kHz for DotStar and Hz for PWM | ||
| uint8_t AWmode = elm[F("rgbwm")] | RGBW_MODE_MANUAL_ONLY; | ||
| uint16_t whiteK = elm[F("wk")] | 0; // physical W channel color temperature in K (0 = neutral/legacy) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clamp
Proposed fix- uint16_t whiteK = elm[F("wk")] | 0; // physical W channel color temperature in K (0 = neutral/legacy)
+ uint16_t whiteK = elm[F("wk")] | 0; // physical W channel color temperature in K (0 = neutral/legacy)
+ if (whiteK != 0 && (whiteK < 1000 || whiteK > 10000)) whiteK = 0;As per coding guidelines, enforce input-validation at the first untrusted ingress point (HTTP/JSON request bodies and query parameters). 🤖 Prompt for AI Agents |
||
| uint8_t maPerLed = elm[F("ledma")] | LED_MILLIAMPS_DEFAULT; | ||
| uint16_t maMax = elm[F("maxpwr")] | (ablMilliampsMax * length) / total; // rough (incorrect?) per strip ABL calculation when no config exists | ||
| // To disable brightness limiter we either set output max current to 0 or single LED current to 0 (we choose output max current) | ||
|
|
@@ -249,7 +250,7 @@ bool deserializeConfig(JsonObject doc, bool fromFS) { | |
| uint8_t driverType = elm[F("drv")] | 0; // 0=RMT (default), 1=I2S note: polybus may override this if driver is not available | ||
|
|
||
| String host = elm[F("text")] | String(); | ||
| busConfigs.emplace_back(ledType, pins, start, length, colorOrder, reversed, skipFirst, AWmode, freqkHz, maPerLed, maMax, driverType, host); | ||
| busConfigs.emplace_back(ledType, pins, start, length, colorOrder, reversed, skipFirst, AWmode, freqkHz, maPerLed, maMax, driverType, host, whiteK); | ||
| doInitBusses = true; // finalization done in beginStrip() | ||
| if (!Bus::isVirtual(ledType)) s++; // have as many virtual buses as you want | ||
| } | ||
|
|
@@ -999,6 +1000,7 @@ void serializeConfig(JsonObject root) { | |
| ins["type"] = bus->getType() & 0x7F; | ||
| ins["ref"] = bus->isOffRefreshRequired(); | ||
| ins[F("rgbwm")] = bus->getAutoWhiteMode(); | ||
| ins[F("wk")] = bus->getWhiteKelvin(); | ||
| ins[F("freq")] = bus->getFrequency(); | ||
| ins[F("maxpwr")] = bus->getMaxCurrent(); | ||
| ins[F("ledma")] = bus->getLEDCurrent(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -205,6 +205,7 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage) | |
| char sl[4] = "SL"; sl[2] = offset+s; sl[3] = 0; //skip first N LEDs | ||
| char rf[4] = "RF"; rf[2] = offset+s; rf[3] = 0; //refresh required | ||
| char aw[4] = "AW"; aw[2] = offset+s; aw[3] = 0; //auto white mode | ||
| char wk[4] = "WK"; wk[2] = offset+s; wk[3] = 0; //W channel color temperature (Kelvin) | ||
| char wo[4] = "WO"; wo[2] = offset+s; wo[3] = 0; //channel swap | ||
| char sp[4] = "SP"; sp[2] = offset+s; sp[3] = 0; //bus clock speed (DotStar & PWM) | ||
| char la[4] = "LA"; la[2] = offset+s; la[3] = 0; //LED mA | ||
|
|
@@ -230,6 +231,9 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage) | |
| break; // no parameter | ||
| } | ||
| awmode = request->arg(aw).toInt(); | ||
| uint16_t whiteK = request->hasArg(wk) ? (uint16_t)request->arg(wk).toInt() : 0; | ||
| // Reject out-of-range or sub-1000K Kelvin values; 0 means "neutral/legacy" | ||
| if (whiteK != 0 && (whiteK < 1000 || whiteK > 10000)) whiteK = 0; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the first condition is redundant |
||
| uint16_t freq = request->arg(sp).toInt(); | ||
| if (Bus::isPWM(type)) { | ||
| switch (freq) { | ||
|
|
@@ -265,7 +269,7 @@ void handleSettingsSet(AsyncWebServerRequest *request, byte subPage) | |
| text = request->arg(hs).substring(0,31); | ||
| // actual finalization is done in WLED::loop() (removing old busses and adding new) | ||
| // this may happen even before this loop is finished so we do "doInitBusses" after the loop | ||
| busConfigs.emplace_back(type, pins, start, length, colorOrder | (channelSwap<<4), request->hasArg(cv), skip, awmode, freq, maPerLed, maMax, driverType, text); | ||
| busConfigs.emplace_back(type, pins, start, length, colorOrder | (channelSwap<<4), request->hasArg(cv), skip, awmode, freq, maPerLed, maMax, driverType, text, whiteK); | ||
| busesChanged = true; | ||
| } | ||
| //doInitBusses = busesChanged; // we will do that below to ensure all input data is processed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -371,6 +371,8 @@ void getSettingsJS(byte subPage, Print& settingsScript) | |
| char sl[4] = "SL"; sl[2] = offset+s; sl[3] = 0; //skip 1st LED | ||
| char rf[4] = "RF"; rf[2] = offset+s; rf[3] = 0; //off refresh | ||
| char aw[4] = "AW"; aw[2] = offset+s; aw[3] = 0; //auto white mode | ||
| char wke[5] = "WKE"; wke[3] = offset+s; wke[4] = 0; //W channel color temperature enabled (UI checkbox) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like the use of an enabled flag, could use 0 (or out of range) means disabled (the code in set.cpp already treats it that way)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I will change that |
||
| char wk[4] = "WK"; wk[2] = offset+s; wk[3] = 0; //W channel color temperature (Kelvin) | ||
| char wo[4] = "WO"; wo[2] = offset+s; wo[3] = 0; //swap channels | ||
| char sp[4] = "SP"; sp[2] = offset+s; sp[3] = 0; //bus clock speed | ||
| char la[4] = "LA"; la[2] = offset+s; la[3] = 0; //LED current | ||
|
|
@@ -392,6 +394,8 @@ void getSettingsJS(byte subPage, Print& settingsScript) | |
| printSetFormValue(settingsScript,sl,bus->skippedLeds()); | ||
| printSetFormCheckbox(settingsScript,rf,bus->isOffRefreshRequired()); | ||
| printSetFormValue(settingsScript,aw,bus->getAutoWhiteMode()); | ||
| printSetFormCheckbox(settingsScript,wke,bus->getWhiteKelvin() > 0); | ||
| printSetFormValue(settingsScript,wk,bus->getWhiteKelvin() > 0 ? bus->getWhiteKelvin() : 6500); | ||
| printSetFormValue(settingsScript,wo,bus->getColorOrder() >> 4); | ||
| unsigned speed = bus->getFrequency(); | ||
| if (bus->isPWM()) { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.