-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Brightness limiter utilizing global LED buffer and improved color restauration #3272
Conversation
On a first glance I must say that I do not like the complexity of the solution. Especially I do not like mixing of The other thing I do not like is the removal of global buffer from I will take a deeper look later. BTW I would also prefer this to be a feature of WLED 0.15. |
Thank you for the feedback! Color restauration does need to be done in BusDigital::getPixelColor(), we would need to add that regardless of the global buffer changes as
Agreed that global buffer should be preferred. Local is also suboptimal as it may cause memory fragmentation due to frequent reallocations if segment setup is modified. Best of both worlds would be making a |
Let me briefly comment, before I take a second look at the changes. 😃 Why do you think color restoration need to happen in My thought was also about TL:DR; Having global buffer can simplify color recovery and speed up the process. At the expense of RAM. And here we stumble on the dreaded ESP8266 support. 😅 |
It doesn't! :) In fact, As for why I added it to
It also does this! Only show() will write to busses now if double buffering is enabled, this even slightly increases performance (less for-loops and function calls). Just segment opacity needs further thought, completely agree on that. Color restoration from global buffer would be good enough for now IMO, but definitely not optimal and I'll think whether there there is a better way, but I'm not sure on one right now that wouldn't involve code complexity going 📈 |
The real concern is not really just the luminance effects (even though low original values still cause more error than higher values), it's the effects of reversing gamma correction and then add luminance on top of that. |
Also note, there is nothing stopping you from copying NeoPixelBrightnessBus into your project and modify it to exactly what you want. It was meant to be used as compatibility layer between Adafruits and my library, not as an advanced feature. |
The main problem here is legacy. Users are accustomed to have global brightness even though we have segment opacity (which behaves like local brightness). If there is only one segment, then one of them is redundant. My (radical) view would be to dump global brightness in favour of using only segment opacity. But I cannot afford that. 😁 I know you are unlikely to change NeoPixelBusLg @Makuna but what if you'd only apply gamma and luminosity in the ISR while sending out buffer data? Is that too much CPU intensive for ISR? Or are you not using ISRs and/or the buffer needs to be prepared in advance? |
@Aircoookie what if we implement double buffering on the bus level (the way you like it) in the same way as we did it for That would require no change on the upper layers (and we could go without What do you think? |
The issue here is that while that can work with Methods that do a translation into a hardware feature to send actual bits, not all methods have this extra step and only send what is in the buffer untranslated. Remember, NeoPixelBus supports other platforms than ESP, so this architecture is designed to be supportable across all. I have a future design Issue logged to review buffering at all layers listed above and abstract them so that methods like the ESP32 hardware could merge layers in way like you are thinking. Right now, this would be Luminance and Gamma correction merged with the Hardware requirement. And this is why NeoPixelBusLg was created, to start users on a supportable path for the future where those two features (luminance/gamma) are moved lower in the stack and not accessible for retrieval as they will in future would require (un) translation from a hardware sending specific format rather than just the LED chip format that is done today. |
I was afraid so. 😄 The problem we are currently facing is that NPBLg doesn't apply brightness/luminance to pixels that were already SetPixelColor-ed (prior to SetLuminance) which we need for brightness/current limitation. And this seem it is totally on us to solve by using double buffering (DIB as you call it). And we are at the dreaded RAM limitations of ESP8266. |
Yeah, right now this would be an issue for ESP8266. The I2s (DMA) method today has two buffers, one for the async send in the DMA format, and one that is DDB format mentioned above. In the future, The DDB layer could be done away with and either replaced with a DIB (internally managed) or my current thoughts with an external DIB that you pass to a Show()/Render(). The detail on this last method, due to async nature of DMA, this Show/Render would have to wait for the previous one to complete sending from hardware before being translated into the single DDB. Not often an issue as you have the external DIB to work with while it sends. This is analogue to double buffer rendering for video games BTW. And then could allow for those who want advanced drawing triple buffering. Maybe this conversation should be moved somewhere else: But the detail I am hearing from your concern is how to limit overall power usage, where the measurement would happen at lower layer (after Luminance/Gamma), but how you react would like to be done at a higher layer (before Luminance/Gamma). Today, you have to apply luminance/gamma, check the over all power usage, then mitigate overdraw how? Starting over with re-apply a lower luminance? Capping peek brightness pixels only? |
The process is rather simple overall.
|
While I was working on implementation of double buffering in a lower level than this PR (within our bus) and was checking NPBLg, I noticed that So, the simplest* solution is to decouple In such case simplest* will add complexity to On the other hand, the implementation of double buffer on |
Is this PR still active, or fully included in |
Replaced by it. But leave it open for now. |
Fixes #3264
leds
buffer, per-segment buffers are only allocated if the global buffer is disabled (per-segment buffer could still be useful with global buffer enabled in some cases as it is lossless even if less than maximum segment opacity is set, so we may consider changing the buffer option to a dropdown with "Global", "None", "Per-segment", "Global + per-segment")BusDigital::getPixelColor()
. Such functionality was part of NeoPixelBrightnessBus, but not NeoPixelBusLg, still it is absolutely essential if no double buffer is used. The restauration equation is improved to be lossless when setting repeatedly, e.g. callingsetPixelColor(i, getPixelColor(i))
will not gradually degrade the color to black.makePostAdjustments()
hotfix replaced by reading global buffer if enabled, else using restored colors from bus (lower performance)uint32_t
for ease of implementation, using RgbColor or CRGB types would also work and is up for consideration so that enabling global buffer would use 1 byte less per LED (perhaps at least if the setup contains no white channel LEDs at all?). Would cause headaches with how to get the white channel for ABL though and might impact performance slightly by having to convert from/to uint32_t.@Makuna if you are interested in testing the improved lossy restauration even though NPBrightnessBus is already deprecated, just replacing
*ptr++ = (value << 8) / scale;
(L78) with*ptr++ = ((value << 8) + _brightness) / scale;
should do the trick. While there is still significant loss, the recovered color is frequently one value closer to the one originally set, but most importantly repeatedly getting and setting again should no longer degrade the color any further.