use video scaling instead of NPB luminance & new ABL #4798
Conversation
DedeHai
added
enhancement
optimization
WalkthroughReworks bus/NeoPixelBus API usage, introduces a global Automatic Brightness Limiter (ABL) pipeline, restructures gamma/color utilities into a new colors.h, moves gamma application to the final send stage, and removes several per-bus brightness dispatch paths. Multiple FX and realtime paths updated to use raw brightness values. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 0
🧹 Nitpick comments (1)
wled00/bus_wrapper.h (1)
8-8: Consider thread safety and encapsulation for global brightness variableWhile using a global variable for brightness might be acceptable in this embedded context (ESP8266/ESP32), consider:
- Making it
staticwithin thePolyBusclass to improve encapsulation- Adding
volatileif accessed from interrupts- Documenting that this is shared across all bus instances
-uint8_t globalBrightness = 255; // global brightness for digital busses, set by setBrightness() +class PolyBus { + private: + static bool _useParallelI2S; + static volatile uint8_t _globalBrightness; // shared brightness for all digital busesAnd initialize it in the implementation file:
volatile uint8_t PolyBus::_globalBrightness = 255;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/bus_wrapper.h(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
wled00/bus_wrapper.h (4)
Learnt from: KrX3D
PR: #4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, bri is a global variable used for brightness control.
Learnt from: KrX3D
PR: #4664
File: wled00/syslog.cpp:50-51
Timestamp: 2025-04-26T12:51:04.993Z
Learning: In the WLED codebase, syslog configuration variables (syslogHost, syslogPort, syslogProtocol, syslogFacility, syslogSeverity) are defined as global variables in wled.h and are accessed directly in the SyslogPrinter implementation, rather than being class members.
Learnt from: blazoncek
PR: #4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Learnt from: willmmiles
PR: #4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
🔇 Additional comments (2)
wled00/bus_wrapper.h (2)
786-798: Well-implemented video scaling algorithmThe brightness scaling implementation correctly addresses the PR objective by ensuring non-zero color values remain visible at low brightness levels. The use of bit manipulation for processing two channels simultaneously is an efficient approach.
919-921: Confirm removal of hardware brightness controlI searched the entire codebase and found no remaining calls to SetLuminance/SetBrightness or other hardware‐driven brightness routines. Shifting all dimming into software video scaling matches the PR’s intent, but please manually verify:
- Test on every supported bus/LED type to ensure LEDs still initialize and dim correctly.
- Measure power consumption at very low brightness to catch any unexpected draw.
- Validate color accuracy and dynamic range across your target chipsets.
|
@blazoncek I am not sure this is the proper/best way to implement it, feel free to move stuff around. |
|
It is not. At least not with If you intend to replace luminance (and since 0.16 with segment blending you can) use base class The other way is to move gamma handling into |
|
I thought so :) |
|
Base NPB does not handle gamma, nor luminance. It is left for the consumer to do any brightness adjustments on RGB values sent to NPB. |
|
I am not going to mess with NPB, I want to focus on other things so I will leave that for someone else. I will update this PR with an improved solution that can be used in the meantime. |
|
@blazoncek I am a bit lost on how to properly implement this as I don't understand all the busses i.e. digitalbus vs polybus and need some guidance: where to set the brightness if not using luminance? |
I think the key question is, "do we get any benefits from letting different bus types use custom luminance or gamma calculations"? Ie. should this be the responsibility of the bus, or should we have a single formula we always use? IF we want to use a common formula for all buses, the calculation code belongs in IF instead we think there's some advantage to letting the bus decide how to apply the brightness calculation, then we should integrate the new calculation (and possibly move gamma correction) directly in to |
At first glance I'd push in favor of using a common formula. It might be that there are special cases (network outputs?) where passing a global brightness separately is preferable -- do we know of any? |
|
@willmmiles is right. IIRC at some point @Makuna considered abandoning NeoPixelBrightnessBus (or whatever it was called) but ultimately replaced it with NeoPixelBusLg. At the time it was impossible to use NeoPixelBus (non Lg or Brightness) as entire WLED logic relied on NeoPixelBus to do the brightness scaling and WLED also handled gamma in an inappropriate way. Both these issues are resolved and indeed brightness scaling and/or gamma correction can be done in show() (and not in a bus). We can stick with Lg however and let it do brightness/gamma for digital LEDs. For analog we will need to add gamma adjustment to bus as it already have brightness. For network/virtual bus we'd also need to do the same as it expects gamma to be applied. |
I did note that I'll also add that, using this branch and doing the brightness ourselves for digital busses, switching from To confirm, then, you think we should go the other way around, ie.push the gamma calculation down so each bus type does it? NeoPixelBusLg bakes its brightness algorithm in fairly deeply -- to adopt the new one here, we can't really use NeoPixelBusLg either way. One up side of doing the brightness and gamma in the bus object is that we could later support different gamma values for each bus -- could be helpful if you're tweaking mismatched hardware or something. |
That would be the correct way.
I think it does and IMO worth doing (as it will also simplify bus wrapper).
Not yet 100% sure but, to answer your next question, we don't need to use Lg, we can implement brightness and gamma in bus manager ( |
|
actually applying brightness before gamma would be the correct way:
this is my understanding of how it should be, not claiming this is the best way. |
|
I ran a test with applying brightness before gamma: it looks awful. edit: |
The issue IMO is linear versus logarithmic brightness slider.
If you haven't noticed, the gamma (functions) rewrite in WLED was with this step in mind. Hence |
|
I did see the comment in the code about |
- mproved gamma table calculation for low values - fixed mismatch in inverting gamma table calculation: inversion should now be as good as it gets
|
replacing NPB was easier than I thought, did that in the last commit. |
|
Ran some speed comparison: I deem this PR ready to undraft&merge if this is the route we want to go, which I highly recommend for quality reasons, see #4794 @netmindz what is your opinion on this?
|
|
As a user, I like the result much better than the current version in main. It even produces some mixed colors at brightness 1, which the old version didn't do. It doesn't appear to produce even brightness at least until around brightness 30, above it, I can't tell. It looks much more even to me without gamma correction. Even brightness is the point of gamma correction, isn't it? One minor problem though: Using the power button in the UI doesn't turn off any LEDs. Did you also want my opinion as a developer? I'd have to get into the gamma topic first. |
|
alright, I can prototype that.
not so sure about that, remapping pixel buffer with the correct pre-sorted mapping should be as fast if not faster, its basically a lokup table with minimal checks. Its just more complex to pre-compute that table and harder to hunt bugs. |
AFAIK I've already implemented as such, except points 1, 2 & 3 are swapped (3,1,2). |
It adds all the complexity, it doesn't save us a pass over the pixels, and the random access will be absolutely brutal on performance if any of those buffers end up in PSRAM. One of the nice things about the NPB buffers is that they're almost always in SRAM, since the hardware layer needs to read 'em in ISRs. Another issue is that if we try to re-use the global pixel buffer, there's no guarantee a bus-ordered version will fit -- multiple buses can output the same pixel. I think also no guarantee there's a viable copy order that ensures that we don't have to have an arbitrarily deep scratch pad somewhere else. (To be fair, my gut reaction "lookup tables slow" is definitely based on experience with big CPUs, where lookup tables are often almost as bad as linked lists in terms of cache management -- very hard to predict which memory needs to be accessed when, so performance tanks unless the table AND data fit in cache. Not so much an issue on these micros main SRAMs, though, but any time PSRAM comes in to play, all the same issues arise.)
Yes, that's correct - the current implementation is pretty close! All that's really needed is moving the global ABL to where it can share the estimate pass with per-bus ABL. We might get some benefit from inverting the bus mapping and sampling from the global pixel buffer instead of iterating over it -- particularly for sparse setups -- but I think this can be looked at later. Eg. |
IMO that makes no sense. Strip-level ABL operates on mapped pixels (pixels that may not exist and may be scattered across multiple buses in undetermined order). It will reduce overall brightness to be within global limit. Strip-level ABL already ignores non-existent pixels (i.e. pixels not belonging to a digital bus). Per-bus ABL, however, operates on physical pixels and retrieves (modified) pixel values from NPB. This may cause a single bus to be limited while the rest may operate at desired brightness (where no ABL is needed). EDIT: And ABL is either strip-level or per-bus, not both. |
the global ABL will limit too much on sparse mappings, it would be better to first map them to busses and then calculate the total brightness, no matter if global or per bus. This means passing colors to NPB then read them back (any loss here if passed unscaled?), calculate ABL if used, then scale each color in a final pass. |
|
RE: PSRAM-Speed for (size_t i = 0; i < count; i++) {
sum += mem[indices[i]];
}Write acces like this: for (size_t i = 0; i < count; i++) {
mem[indices[i]] = i;
}The first access to PSRAM in the test is always slower, most likely as that is when PSRAM buffer get chached. |
|
Key take-aways from my test:
Assuming reading colors back from the bus is fast enough (which it probably is) doing the mapping in
I will try to implement this and check if there are any unforseen complications. |
|
Thanks for working on improving the gamma handling. I think your plan in the last message makes sense. But regarding this earlier question:
If I understand correctly, 0.16 applies gamma before doing effects/color mixing, whereas 0.15 applied gamma at the end instead? The difference will certainly change how things wrap around or clip during effect calculations. It only makes sense to apply gamma at the end as the final step. That way, colors have more headroom during effect calculations, and you ensure that effects happen in the correct, linear space (not a scaled/shifted color space). Gamma is a destructive process that causes clipping and shifting of colors. So let's not modify the colors until the end, at the output stage. That is definitely more correct and will ensure more consistent effect calculations. So in the plan from the previous message, move gamma as the final stage before display on the output device. |
no, the other way around. 0.16 is doing it the more mathematically correct way to produce better hue accuracy |
|
Ah okay, yeah doing it last is much better. The quoted plan said "apply global gamma" first so I thought that's what it was doing. :) Well, anyway, regarding the user checkbox, I personally don't think it makes sense to keep the wrong behavior even as an option. Sure, maybe someone was used to a certain effect being slightly different due to clipping in the old code, but ehhhhhhhhhhhhhhhhhhhh, can't they just... get used to looking at the correct effect. ;) |
Improvements to ABL handling: - removed strip level handling, ist now all done on bus level - limiter now respects pixel mapping - proper handling of white channel - improved current estimation - current is now always correctly reported to UI - minimal FPS impact if the ABL is not limiting but slighly higher impact for global ABL limit due to double-scaling - moved brightness scaling to BusDigital - created new header file colors.h to be able to access color functions in bus-manager. - updated colo_fade() with better video scaling to preserve hue's at low brightness - added IRAM_ATTR to color_fade (negligible speed impact when compared to inline and benefits other functions) - added IRAM_ATTR to color_blend as it is used a lot throughout the code, did not test speed impact but adding it to color_fade made it almost on-par with an inlined function Additional changes: - fixes for properly handling `scaledBri()` (by @blazoncek) - also use bit-shift instead of division in blending for ESP8266 - improvements for faster "softlight" calculation in blending - changed some variables to uint8_t to maybe let the compiler optimize better, uint8_t can be faster if read, store and set are all done in uint8_t, which is the case in the ones I changed - various minor code formatting changes
|
Finally finished the re-work of the bus brightness handling and the ABL. Please review. Hope no changes got lost in manual merging of my WIP branch. I reworked the code several times over to arrive at this version which IMHO is the best compromise between speed and code consistency. Global ABL would be faster if done on strip level, only scaling once but it convolutes code and adds computational overhead even if the limit does not apply. @blazoncek took that approach in his dev branch (see table) I did some testing on the ESP32 C3 but not extensively. Here is a summary of my findings: Flash usage: only checked for C3 and ESP32
Test with 2x 256 LEDs (2 outputs) on C3, 32x16 matrix, palette effect
In summary:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/led.cpp (1)
67-73: Mandatory refactors required: remove legacy scaledBri() and briMultiplier usageThe grep output shows numerous remaining call sites and declarations of the old brightness-scaling helpers—these must be updated to use the new central briT model (or removed entirely) before dropping scaledBri() and briMultiplier. Please refactor or replace each of the following:
• wled00/led.cpp (lines 57–60):
- Declaration and implementation of scaledBri(byte) and comment referencing briMultiplier.
• wled00/wled.h (line 433):
- Global declaration of briMultiplier.
• wled00/xml.cpp (around line 378):
- printSetFormValue(..., PSTR("BF"), briMultiplier).
• wled00/wled.cpp (line 193):
- BusManager::setBrightness(scaledBri(bri));
• wled00/set.cpp (line 362):
- briMultiplier assignment in user-set logic.
• wled00/fcn_declare.h (line 170):
- Prototype for scaledBri(byte).
• wled00/cfg.cpp (around lines 518 & 1040):
- JSON serialization/deserialization of briMultiplier.
• wled00/wled_eeprom.cpp (line 134):
- EEPROM.read into briMultiplier.
• wled00/FX_fcn.cpp (lines 1199 & 1674):
- bus->setBrightness(scaledBri(...)).
Until all these are migrated to the new briT API (or removed if obsolete), you cannot safely delete scaledBri() or briMultiplier. Ensure each call site uses the updated brightness value so that legacy scaling is fully eradicated before drop.
wled00/colors.cpp (1)
94-104: Bug: bitwise OR used instead of logical ORThis returns early more often than intended when inputs are non-zero bit patterns. Use logical OR.
Apply fix:
- if (rgb == 0 | hueShift + lighten + brighten == 0) return rgb; // black or no change + if ((rgb == 0) || (hueShift + lighten + brighten == 0)) return rgb; // black or no changeAdd parentheses to avoid precedence surprises.
♻️ Duplicate comments (1)
wled00/colors.cpp (1)
587-598: Gamma table calc: start at 1, set [0] after loop — OKThis avoids powf(0, …) work and keeps gamma[0] == 0. The inverse table center-bias is acceptable given prior discussion.
🧹 Nitpick comments (10)
wled00/led.cpp (1)
58-63: Cast width is fine; consider explicit fixed-width type for readabilitySwitching to an unsigned wider intermediate avoids 8-bit overflow. To make intent obvious across platforms, prefer a fixed-width type (e.g., uint32_t) instead of plain unsigned.
Apply this small clarification:
-byte scaledBri(byte in) +byte scaledBri(byte in) { - unsigned val = ((unsigned)in*briMultiplier)/100; + uint32_t val = (uint32_t(in) * briMultiplier) / 100u; if (val > 255) val = 255; return (byte)val; }wled00/colors.h (3)
41-48: Index operator returns B,G,R,W order — consider documenting to avoid misuseoperator[] maps 0->B, 1->G, 2->R, 3->W. That’s easy to misuse when people expect RGBW. Consider a short comment or named accessors to reduce footguns.
Example minimal doc:
- // Access as an array + // Access as an array (index order: 0=B, 1=G, 2=R, 3=W) inline const uint8_t& operator[] (uint8_t x) const __attribute__((always_inline)) { return raw[x]; }
126-126: Magic number in getPaletteCount()The base count “13” is a magic number. Define it as a named constant to make intent explicit.
- inline size_t getPaletteCount() { return 13 + GRADIENT_PALETTE_COUNT + customPalettes.size(); } + constexpr size_t BASE_PALETTE_COUNT = 13; + inline size_t getPaletteCount() { return BASE_PALETTE_COUNT + GRADIENT_PALETTE_COUNT + customPalettes.size(); }
134-135: Prototypes present even when HueSync is disabledDefinitions are under #ifndef WLED_DISABLE_HUESYNC in colors.cpp, but these declarations are unconditional. Guard them to avoid ODR/link errors if someone calls them when HueSync is disabled.
-void colorXYtoRGB(float x, float y, byte* rgb); // only defined if huesync disabled TODO -void colorRGBtoXY(const byte* rgb, float* xy); // only defined if huesync disabled TODO +#ifndef WLED_DISABLE_HUESYNC +void colorXYtoRGB(float x, float y, byte* rgb); +void colorRGBtoXY(const byte* rgb, float* xy); +#endifwled00/colors.cpp (1)
67-87: Video-scaling fade looks solid; preserves dim huesThe “quarter of max” threshold and +1 carry to keep channels alive achieves the PR goal of avoiding black crush at very low brightness. Nice.
Consider making the threshold a named constexpr for easier tuning and experimentation:
- uint8_t quarterMax = maxc >> 2; // note: using half of max results in color artefacts + constexpr uint8_t kVideoHoldShift = 2; // 2 => 1/4 max; tweak if artifacts emerge + uint8_t quarterMax = maxc >> kVideoHoldShift;wled00/e131.cpp (1)
191-197: Avoided double-scaling in DMX MODE_PRESET; confirm transition intentSwitching to strip.setBrightness(bri, false) removes the previous scaledBri duplication — good. One behavioral point: DMX_MODE_SINGLE_DRGB (Line 169) uses direct=true, whereas PRESET now uses direct=false. If the intent is immediate DMX response without fades, consider making both direct=true for consistency. If the fade is intentional only for PRESET, please document the rationale.
Apply if immediate DMX response is desired:- strip.setBrightness(bri, false); + strip.setBrightness(bri, true);Please confirm which behavior users expect for the PRESET mode brightness channel: stepped (direct=true) or transition-based (direct=false). A brief comment in code would prevent future flip-flops.
wled00/wled.cpp (1)
193-193: Brightness Usage Audit Passed; Optional Gamma-Preserving Refactor RemainsAudit results confirm that all brightness adjustments follow the intended pattern:
strip.setBrightness(...)is only ever called with raw brightness values (no use ofscaledBri), and
BusManager::setBrightness(...)is only called withscaledBri(...), at two locations:• wled00/wled.cpp:193
• wled00/FX_fcn.cpp:1674Since the original suggestion is an optional refactor—to route the reinit brightness call through
strip.setBrightness(...)and thus invokegammaCorrectBriimmediately—you may apply it if uniform gamma handling is preferred. If you do, please update both instances:- BusManager::setBrightness(scaledBri(bri)); // fix re-initialised bus' brightness #4005 and #4824 + // Use the standard path so gammaCorrectBri and other side-effects remain consistent. + strip.setBrightness(bri, true); // fix re-initialised bus' brightness #4005 and #4824And similarly in FX_fcn.cpp at the
BusManager::setBrightness(scaledBri(b))call.wled00/FX.cpp (1)
7531-7533: Aligning with centralized gamma: looks good; consider adding a clarifying commentSwitching from gamma8(cos8_t(...)) to just cos8_t(...) removes per-pixel gamma in the effect and is consistent with the PR’s intent to do gamma/brightness at the final output stage. This should also reduce per-frame cycles slightly. To prevent future regressions, add a short comment noting that gamma is intentionally omitted here because it’s handled later in the post-processing/show() pipeline.
byte valueR = rdistort + ((a- ( ((xoffs - cx) * (xoffs - cx) + (yoffs - cy) * (yoffs - cy))>>7 ))<<1); byte valueG = gdistort + ((a2-( ((xoffs - cx1) * (xoffs - cx1) + (yoffs - cy1) * (yoffs - cy1))>>7 ))<<1); byte valueB = bdistort + ((a3-( ((xoffs - cx2) * (xoffs - cx2) + (yoffs - cy2) * (yoffs - cy2))>>7 ))<<1); - - valueR = cos8_t(valueR); - valueG = cos8_t(valueG); - valueB = cos8_t(valueB); + // Intentionally no per-pixel gamma here; gamma/brightness is applied in the final output stage. + valueR = cos8_t(valueR); + valueG = cos8_t(valueG); + valueB = cos8_t(valueB);wled00/bus_manager.h (1)
417-418: Use consistent terminology for ABL milliAmps calculation.The comment was updated to reflect "milliamps used by ESP (for power estimation)", but the macro name still uses
MA_FOR_ESP. Consider using consistent terminology throughout the codebase.-// milliamps used by ESP (for power estimation) -// you can set it to 0 if the ESP is powered by USB and the LEDs by external +// milliamps used by ESP for power estimation +// set to 0 if ESP is powered via USB and LEDs via external supplywled00/bus_manager.cpp (1)
928-929: Consider ABL timing optimization.Calling
applyABL()on everyshow()might be unnecessary if pixel colors haven't changed. Consider adding a dirty flag to optimize this.Consider tracking whether any pixel colors have changed since the last ABL application to avoid unnecessary brightness recalculations:
void BusManager::show() { - applyABL(); // apply brightness limit, updates _gMilliAmpsUsed + if (_needsABL) { + applyABL(); // apply brightness limit, updates _gMilliAmpsUsed + _needsABL = false; + } for (auto &bus : busses) { bus->show(); } }Then set
_needsABL = trueinsetPixelColor()when ABL is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
wled00/FX.cpp(1 hunks)wled00/FX_fcn.cpp(5 hunks)wled00/bus_manager.cpp(10 hunks)wled00/bus_manager.h(5 hunks)wled00/bus_wrapper.h(3 hunks)wled00/colors.cpp(6 hunks)wled00/colors.h(1 hunks)wled00/e131.cpp(1 hunks)wled00/fcn_declare.h(0 hunks)wled00/image_loader.cpp(1 hunks)wled00/json.cpp(1 hunks)wled00/led.cpp(2 hunks)wled00/udp.cpp(2 hunks)wled00/wled.cpp(1 hunks)wled00/wled.h(1 hunks)
💤 Files with no reviewable changes (1)
- wled00/fcn_declare.h
✅ Files skipped from review due to trivial changes (1)
- wled00/wled.h
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-04-28T20:51:29.773Z
Learnt from: KrX3D
PR: wled/WLED#4585
File: usermods/seven_segment_display_reloaded_v2/seven_segment_display_reloaded_v2.cpp:430-435
Timestamp: 2025-04-28T20:51:29.773Z
Learning: In WLED, `bri` is a global variable used for brightness control.
Applied to files:
wled00/e131.cppwled00/json.cppwled00/led.cppwled00/udp.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX_fcn.cpp
🧬 Code graph analysis (7)
wled00/FX.cpp (1)
wled00/wled_math.cpp (2)
cos8_t(88-90)cos8_t(88-88)
wled00/image_loader.cpp (1)
wled00/data/pixart/getPixelValues.js (1)
red(237-237)
wled00/wled.cpp (2)
wled00/FX_fcn.cpp (2)
setBrightness(1667-1679)setBrightness(1667-1667)wled00/led.cpp (2)
scaledBri(58-63)scaledBri(58-58)
wled00/FX_fcn.cpp (2)
wled00/led.cpp (2)
scaledBri(58-63)scaledBri(58-58)wled00/bus_manager.cpp (12)
initializeABL(963-997)initializeABL(963-963)setPixelColor(260-297)setPixelColor(260-260)setPixelColor(455-486)setPixelColor(455-455)setPixelColor(633-638)setPixelColor(633-633)setPixelColor(691-700)setPixelColor(691-691)setPixelColor(934-939)setPixelColor(934-934)
wled00/colors.h (1)
wled00/colors.cpp (40)
color_blend(11-21)color_blend(11-11)color_add(28-61)color_add(28-28)adjust_color(94-104)adjust_color(94-94)ColorFromPaletteWLED(107-134)ColorFromPaletteWLED(107-107)generateHarmonicRandomPalette(146-241)generateHarmonicRandomPalette(146-146)generateRandomPalette(243-249)generateRandomPalette(243-243)loadCustomPalettes(251-297)loadCustomPalettes(251-251)hsv2rgb(299-328)hsv2rgb(299-299)colorHStoRGB(351-357)colorHStoRGB(351-351)rgb2hsv(330-349)rgb2hsv(330-330)colorKtoRGB(360-382)colorKtoRGB(360-360)colorCTtoRGB(384-404)colorCTtoRGB(384-384)colorXYtoRGB(407-461)colorXYtoRGB(407-407)colorRGBtoXY(463-470)colorRGBtoXY(463-463)colorFromDecOrHexString(474-492)colorFromDecOrHexString(474-474)colorFromHexString(495-513)colorFromHexString(495-495)colorBalanceFromKelvin(529-542)colorBalanceFromKelvin(529-529)approximateKelvinFromRGB(551-581)approximateKelvinFromRGB(551-551)setRandomColor(136-140)setRandomColor(136-136)color_fade(67-87)color_fade(67-67)
wled00/bus_manager.h (1)
wled00/bus_manager.cpp (8)
estimateCurrent(195-205)estimateCurrent(195-195)applyBriLimit(207-239)applyBriLimit(207-207)initializeABL(963-997)initializeABL(963-963)applyABL(999-1040)applyABL(999-999)
wled00/bus_manager.cpp (4)
wled00/bus_manager.h (5)
hasWhite(165-171)hasCCT(172-177)show(238-244)show(297-298)show(324-325)wled00/FX_fcn.cpp (8)
getPixelColor(896-951)getPixelColor(896-896)setPixelColor(683-859)setPixelColor(683-683)setPixelColor(863-893)setPixelColor(863-863)show(1559-1615)show(1559-1559)wled00/FX.h (4)
getPixelColor(961-961)setPixelColor(693-693)setPixelColor(903-903)setPixelColor(907-907)wled00/colors.cpp (2)
color_fade(67-87)color_fade(67-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (20)
wled00/bus_wrapper.h (2)
289-291: TM1914 feature differs between ESP8266 and ESP32 variantsESP8266 uses NeoRgbTm1914Feature while ESP32 uses NeoGrbTm1914Feature. Unless the protocol/feature mapping intentionally differs per method, this will produce mismatched channel orders across platforms.
Consider unifying to the same feature:
-#define B_32_RN_TM1914_3 NeoPixelBus<NeoGrbTm1914Feature, NeoEsp32RmtNTm1914Method> -#define B_32_I2_TM1914_3 NeoPixelBus<NeoGrbTm1914Feature, X1Tm1914Method> -#define B_32_IP_TM1914_3 NeoPixelBus<NeoGrbTm1914Feature, X8Tm1914Method> // parallel I2S +#define B_32_RN_TM1914_3 NeoPixelBus<NeoRgbTm1914Feature, NeoEsp32RmtNTm1914Method> +#define B_32_I2_TM1914_3 NeoPixelBus<NeoRgbTm1914Feature, X1Tm1914Method> +#define B_32_IP_TM1914_3 NeoPixelBus<NeoRgbTm1914Feature, X8Tm1914Method> // parallel I2SIf different boards truly require different feature layouts, please add a comment explaining why.
293-295: SM16825 white-channel order mismatch between ESP8266 and ESP32ESP8266 uses NeoRgbwcSm16825eFeature, ESP32 uses NeoRgbcwSm16825eFeature. The order of WW/CW appears swapped.
If unintentional, align to the same feature:
-#define B_32_RN_SM16825_5 NeoPixelBus<NeoRgbcwSm16825eFeature, NeoEsp32RmtNWs2812xMethod> -#define B_32_I2_SM16825_5 NeoPixelBus<NeoRgbcwSm16825eFeature, X1Ws2812xMethod> -#define B_32_IP_SM16825_5 NeoPixelBus<NeoRgbcwSm16825eFeature, X8Ws2812xMethod> // parallel I2S +#define B_32_RN_SM16825_5 NeoPixelBus<NeoRgbwcSm16825eFeature, NeoEsp32RmtNWs2812xMethod> +#define B_32_I2_SM16825_5 NeoPixelBus<NeoRgbwcSm16825eFeature, X1Ws2812xMethod> +#define B_32_IP_SM16825_5 NeoPixelBus<NeoRgbwcSm16825eFeature, X8Ws2812xMethod> // parallel I2SPlease confirm against real strips or NPB docs.
wled00/colors.cpp (2)
11-21: IRAM’ing color_blend is goodTagging color_blend IRAM_ATTR for tight timing paths is appropriate here.
106-134: Palette sampling math cleanup is fine; avoids slower helpersThe inlined blend and brightness scaling are clear and performant. Matches the “video” scaling approach elsewhere.
wled00/image_loader.cpp (1)
54-63: Verified: No premature gamma application detectedA search across the codebase shows that:
- drawPixelCallback now feeds raw r/g/b values directly into setPixelColorXY, with no inline gamma calls in the GIF path.
- All occurrences of gamma8(…) are confined to brightness adjustments (e.g. WS2812FX setBrightness, flasher ramps), TV-simulator color expansion, and particle systems—none overlap the GIF–loader’s drawPixelCallback.
No other code applies per-channel gamma before show(), so there’s no risk of double correction.
wled00/json.cpp (1)
315-315: Good fix: pass raw bri to avoid double-scalingUsing strip.setBrightness(bri, true) here is correct since WS2812FX::setBrightness will apply scaledBri(b) internally. This prevents applying the briMultiplier twice in the immediate i-array path. Looks consistent with the PR goal of centralizing brightness handling.
Quick check: with brightness set to 1–5, send a JSON "i" update containing a dim non-zero color and verify no pixels go black spuriously. This specifically guards against the low-brightness regression from #4794.
wled00/udp.cpp (1)
427-427: LGTM!The direct use of brightness values instead of
scaledBri()aligns with the PR's goal of moving gamma correction to the final output stage, ensuring consistent brightness handling across the codebase.Also applies to: 437-437, 444-444
wled00/bus_manager.h (2)
251-253: LGTM! Well-designed ABL API.The addition of per-bus current limiting methods provides a clean interface for the new ABL implementation. The separation of concerns between current estimation and brightness limiting is well thought out.
449-450: LGTM! Proper ABL lifecycle management.The ABL initialization and application methods are well-placed in the BusManager namespace, providing centralized control over automatic brightness limiting.
wled00/FX_fcn.cpp (4)
1199-1201: LGTM! Proper ABL initialization sequence.The ABL initialization is correctly placed after all buses are created and configured, ensuring proper current limit calculation. The use of
scaledBri()for initial brightness maintains consistency with the brightness multiplier system.
1303-1307: LGTM! Architecture-specific optimizations.The expanded conditional compilation to include ESP8266 alongside ESP32C3 for fixed-point arithmetic paths is appropriate. These platforms benefit from the optimized integer-only calculations due to their lack of hardware floating-point units.
Also applies to: 1314-1318
1595-1598: LGTM! Clean gamma correction implementation.Moving gamma correction to the final pixel output stage (with proper guards for realtime mode) is the correct approach. This ensures gamma is applied after all color manipulations and brightness adjustments, preventing color distortion.
1674-1674: Brightness scaling withscaledBri(b)is correct and does not conflict with ABL
The call to
BusManager::setBrightness(scaledBri(b));applies only the user-configured brightness multiplier (briMultiplier) before handing off to the bus API; the implementation ofscaledBriinwled00/led.cppconfirms this is just a linear scale:// scales the brightness with the briMultiplier factor byte scaledBri(byte in) { unsigned val = ((unsigned)in * briMultiplier) / 100; return (byte)val; }Auto Brightness Limiter (ABL) is applied later in the pixel-show path within
bus_manager.cpp, where current draw is estimated and per-bus limits are enforced duringshow(). This logic does not override or bypass the initial brightness setting but further constrains output if the current limit is reached. Key comments in that file include:// if using ABL, sum all color channels to estimate current and limit brightness in show() // ... busd.applyBriLimit(0); // apply per bus ABL limit, updates _milliAmpsTotal if limit reachedBecause scaledBri remains the first step in brightness handling and ABL runs independently at show-time, there is no conflict between the two systems—no change required.
wled00/bus_manager.cpp (7)
188-194: Excellent documentation of ABL implementation!The comment clearly explains the ABL workflow and the special case for buses with LED current set to 0. This will help future maintainers understand the design decisions.
195-205: LGTM! Clean current estimation logic.The
estimateCurrent()implementation properly handles both normal LEDs and the special WS2815 power model. The color sum calculation is efficient and the milliamps conversion is mathematically correct.
207-239: Robust brightness limiting implementation!The
applyBriLimit()method handles edge cases well:
- Minimum brightness of 1 ensures LEDs remain visible even at very low current limits
- Video scaling preserves color ratios
- Proper CCT handling when applicable
- Efficient pixel repainting with color order correction
The use of video scaling when brightness is limited is particularly clever as it maintains color fidelity.
260-297: LGTM! Proper ABL integration in setPixelColor.The color sum accumulation for ABL is correctly placed after brightness scaling, and the special handling for WS2815 (using max of RGB channels) properly implements the workaround for issue #549.
300-321: Good color restoration logic.The
getPixelColor()implementation correctly restores colors with proper handling of WS2812_1CH_X3 mapping. The addition of explicituint8_ttype for color order improves type safety.
963-997: Well-designed ABL initialization!The
initializeABL()implementation intelligently handles both global and per-bus current limits:
- Proper ESP current budget sharing across ABL-enabled buses
- Minimum 1mA per LED ensures functionality even with very low limits
- Buses with LED current = 0 are properly excluded from ABL
The logic for distributing the ESP's current budget proportionally across buses is particularly well thought out.
999-1040: Excellent ABL application logic!The
applyABL()method properly orchestrates the brightness limiting process:
- Per-bus current estimation
- Per-bus limiting (when global limit is 0)
- Global limiting with proper minimum handling
- Efficient brightness distribution
The two-pass approach (per-bus then global) ensures fair brightness distribution while respecting both local and global constraints.
| #define B_8266_U0_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266Uart0Apa106Method> //3 chan, esp8266, gpio1 | ||
| #define B_8266_U1_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266Uart1Apa106Method> //3 chan, esp8266, gpio2 | ||
| #define B_8266_DM_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266DmaApa106Method> //3 chan, esp8266, gpio3 | ||
| #define B_8266_BB_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266BitBangApa106Method> //3 chan, esp8266, bb (any pin but 16) | ||
| //FW1906 GRBCW |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
APA106 color order inconsistent across methods (UART vs DMA/BB)
U0/U1 use NeoRbgFeature, but DMA/BitBang use NeoGrbFeature. This likely flips colors for APA106 on those methods.
Suggest aligning all APA106 variants to the same color order:
-#define B_8266_DM_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266DmaApa106Method> //3 chan, esp8266, gpio3
-#define B_8266_BB_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266BitBangApa106Method> //3 chan, esp8266, bb (any pin but 16)
+#define B_8266_DM_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266DmaApa106Method> //3 chan, esp8266, gpio3
+#define B_8266_BB_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266BitBangApa106Method> //3 chan, esp8266, bb (any pin but 16)Please validate on hardware; if APA106 is truly GRB for some driver paths, set all four consistently to that feature instead.
Align APA106 definitions to the correct RGB feature (NeoRgbFeature)
The APA106 LED’s native channel order is Red–Green–Blue, so all four method variants should use NeoRgbFeature. The current mix of NeoRbgFeature (R–B–G) and NeoGrbFeature (G–R–B) will produce incorrect colors.
Please update in wled00/bus_wrapper.h:
- #define B_8266_U0_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266Uart0Apa106Method>
- #define B_8266_U1_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266Uart1Apa106Method>
- #define B_8266_DM_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266DmaApa106Method>
- #define B_8266_BB_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266BitBangApa106Method>
+ #define B_8266_U0_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266Uart0Apa106Method>
+ #define B_8266_U1_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266Uart1Apa106Method>
+ #define B_8266_DM_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266DmaApa106Method>
+ #define B_8266_BB_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266BitBangApa106Method>Validate on hardware; this ensures all APA106 paths use the correct RGB channel mapping.
📝 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.
| #define B_8266_U0_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266Uart0Apa106Method> //3 chan, esp8266, gpio1 | |
| #define B_8266_U1_APA106_3 NeoPixelBus<NeoRbgFeature, NeoEsp8266Uart1Apa106Method> //3 chan, esp8266, gpio2 | |
| #define B_8266_DM_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266DmaApa106Method> //3 chan, esp8266, gpio3 | |
| #define B_8266_BB_APA106_3 NeoPixelBus<NeoGrbFeature, NeoEsp8266BitBangApa106Method> //3 chan, esp8266, bb (any pin but 16) | |
| //FW1906 GRBCW | |
| #define B_8266_U0_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266Uart0Apa106Method> //3 chan, esp8266, gpio1 | |
| #define B_8266_U1_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266Uart1Apa106Method> //3 chan, esp8266, gpio2 | |
| #define B_8266_DM_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266DmaApa106Method> //3 chan, esp8266, gpio3 | |
| #define B_8266_BB_APA106_3 NeoPixelBus<NeoRgbFeature, NeoEsp8266BitBangApa106Method> //3 chan, esp8266, bb (any pin but 16) | |
| //FW1906 GRBCW |
🤖 Prompt for AI Agents
In wled00/bus_wrapper.h around lines 179-183, the APA106 macro definitions use
incorrect feature types (NeoRbgFeature and NeoGrbFeature); update all four
defines so they use NeoRgbFeature instead (i.e., replace NeoRbgFeature and
NeoGrbFeature with NeoRgbFeature for B_8266_U0_APA106_3, B_8266_U1_APA106_3,
B_8266_DM_APA106_3, and B_8266_BB_APA106_3) and rebuild/validate on hardware to
confirm correct color mapping.
There was a problem hiding this comment.
@blazoncek is the rabbit right? do you know anything about this?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Regardless if rabbit is correct or not, users have color order override so it really doesn't matter.
DedeHai
changed the title
|
I do like the idea of BusManager::initializeABL() and the swap of NPB, however I do not like the pre-calculation of sum of all pixels on bus level. In current implementation it assumes a pixel is only ever modified once, which may not be true in the future. There are a few other erros regarding reverse scaling of PolyBus::getPixelColor() too. |
|
Thanks for reviewing.
I don't understand what you mean by that. If a pixel is sent out to the bus, it should have its final value. What manipulation at bus level are you thinking of?
very well possible. any hints? |
Multiple calls to BusDigital::setPixelColor() for the same pixel. Actually 2 strips may (partially) overlap if you choose so in settings (not relevant in this case, but still). However, I've dug deeper now and do see the benefit of pre-calculating sum of all channels in the setPixelColor() phase.
Check restoreColorLossy() occurrences. |
it makes it a tad faster :)
In the case I use to read back from NPB there is no need to restore the brightness, the ABL calculates on top of the applied brightness as that is applied before summing. |
|
any objections to merging this @willmmiles? |
I haven't done a thorough test, but the code looks good to me. Go for it. |
|
thanks for reviewing. I did a lot of tests on the actual color scaling, not so many on the new ABL since I do not have all the hardware but the output power values for WS2812 were withing a reasnable tolerance. If you did not see any flaws in logic, this is good to go live. |
NPB luminance setting dims dark colors to black, this gives really ugly results at low brightness:
Examples:
The proposed method applies video scaling: any color that is non zero will stay non zero
NPB luminance is not used anymore, so changing to non "Lg" version of NPB, saving flash.
With brightness no longer being handled by NPB, a new approach to brightness limiter was implemented.
fixes #4794
Summary by CodeRabbit
New Features
Improvements
Bug Fixes