beta-3 Auto Brightness Limiter issue

[amikell94]

What happened?

Upon update to the new B3 firmware on an ESP32, I noticed a rhythmic bright-dim blink while my lights (sk6812 rgbw with accurate setting for white) were set to solid white. Upon further inspection, it seemed they were trying to pull maximum power even with Limiter set at 2000 ma. It was evident by the dimming of the ESP32 red led when it would blink. My USB power block also heated up quickly.

To Reproduce Bug

Run B3 on ESP32. Set brightness Limiter to 2000 ma. Using 5v sk6812 rgbw 144leds/m and a 2.4 amp USB block, run a 1 meter strip on white using accurate white calculation in settings and then turn the brightness to max on color page.

Expected Behavior

Not for it to try to burn up my power supply with the brightness limiter set to 400 ma below it's capacity(Yes, I know I should have it fused and need a way bigger power supply for the full potential of these lights. I just have to get back around to this particular project and get the stuff ordered.)

Install Method

Binary from WLED.me

What version of WLED?

WLED 0.14.0-b3

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

No response

Anything else?

Just a note, I rolled back to b1 and all is working as it should again. If you need more info, I can try to gather it at your request. Thanks for all you do for this project because WLED is pretty cool!!

Code of Conduct

• I agree to follow this project's Code of Conduct

[blazoncek]

Please reduce the limiter from 2000mA to 1000mA and report if the brightness varies again.

[KingSupernerd]

I have a similar issue - maybe the same, I'm not sure if it is appropriate to add here or start a new issue.
This is 0.14.0-b1 issue not present https://cdn.discordapp.com/attachments/473449548279185408/1121636747479089274/0.14.0.b1.mp4
This is 0.14.0.b3 issue present
https://cdn.discordapp.com/attachments/473449548279185408/1121637129005576242/0.14.0-b3.mp4
I've tried 2 boards, 2 led strips (one RGB, one RGBW), 3 power supplies (one a benchtop very precise power supply) and all new wiring.
I am currently at 5v/5a input, I had limiter at 3000, I changed it to 1000, and it started flashing immediately, powered off and on, then went to android and back to solid, and started flashing again.
I did more testing, at brightness <140 - it does not noticeably flicker. At brightness => 145, it appears to flicker, and is discernable on voltage up and down of .01 amp spread.
At full brightness, it is more noticeable, and amp spread is over 1 amp from high to low.
I'm willing to do more testing or start another issue thread if needed.

[softhack007]

@blazoncek the behaviour could be related to our change from NeoPixelBrightnessBus to NeoPixelBusLg, as there is a subtle difference in behaviour of setBrightness()

Old

void SetBrightness(uint8_t brightness)
This will change the brightness of the NeoPixelBus. All current pixels will be modified to the new brightness; and any subsequent calls to SetPixelColor() will also be modified.

New

void SetLuminance(uint8_t luminance)
This will set the luminance level for all new values being set. Changing this does not affect all previous pixel values.

Our brightness limiter will run just before busses.show(), and it calls SetLuminance() to modify brightness before sending out pixels to the LEDs. With the old object "All current pixels will be modified", but with LG it "does not affect all previous pixel values".

I understand that SetLuminance() will not touch pixels already stored with SetPixelColor() but not shown yet. The new luminance will only affect future SetPixelColor() operations.

@Makuna is this correct?

As consequence, our brightness limiter would always be "one frame behind". So the first frame above brightness limit would show at full brightness (brownout, magic smoke). Then the next frame will use the modified brightness intended for the previous frame, etc (rhythmic dim-bright).

[softhack007]

Related code:
https://github.com/Aircoookie/WLED/blob/ef3c72a24fb5584552475764c99e1f585acfb290/wled00/FX_fcn.cpp#L1249-L1254

https://github.com/Aircoookie/WLED/blob/ef3c72a24fb5584552475764c99e1f585acfb290/wled00/FX_fcn.cpp#L1227-L1238

https://github.com/Aircoookie/WLED/blob/ef3c72a24fb5584552475764c99e1f585acfb290/wled00/bus_wrapper.h#L809-L814

[softhack007]

Maybe this method of NeoPixelBusLg will help, but looks like luma and gamma could be applied twice which is not good either:shrug: ...

void ApplyPostAdjustments()
This will apply luminance and gamma adjustments to all existing stored pixel data; even if previously applied by calling SetPixelColor(). It is primarily used when the Pixels() method is used to directly access and set all pixel data into the memory buffer that will then need to be corrected.

[blazoncek]

As consequence, our brightness limiter would always be "one frame behind". So the first frame above brightness limit would show at full brightness (brownout, magic smoke). Then the next frame will use the modified brightness intended for the previous frame, etc (rhythmic dim-bright).

Exactly the behaviour I am seeing.

[blazoncek]

@softhack007 this was a good catch! ❤️

Solving this is going to be a tough call as setBrightness() is used in many places..

Another option I was thinking about yesterday (solving #3265) is to dump global brightness altogether and rely on segment opacity only. That was also on @Aircoookie 's mind some time ago but it will be a huge change for endusers. Possibly breaking everything if not implemented properly.

[Aircoookie]

Agreed!

Just using segment opacity will also still not solve the issue that we need to have set all LEDs in a given frame in order to determine the max. allowed brightness for that frame.
One solution may be always setting colors from effects to the LEDs with maximum brightness, then before every show() adjusting luminance and calling ApplyPostAdjustments().
What I would be most comfortable with is switching to plain NeoPixelBus and replicating the functionality of the deprecated NPBrightnessBus as it worked really well for the project IMO.

[blazoncek]

One solution may be always setting colors from effects to the LEDs with maximum brightness, then before every show() adjusting luminance and calling ApplyPostAdjustments().

I checked the code and UDP/realtime is setting bus brightness directly as is "i" from JSON. Other instances of setBrightness() seem to be cool and in line with the possibility to ApplyPostAdjustment() just in estimateCurrentAndLimitBri().

So, perhaps it is ok to just add ApplyPostAdjustment() to PolyBus::setBrightness()?

[softhack007]

So, perhaps it is ok to just add ApplyPostAdjustment() to PolyBus::setBrightness()?

Sound good to me 👍. The docs from makuna say

void ApplyPostAdjustments(): This will apply luminance and gamma adjustments to all existing stored pixel data

So unless we use NPB gamma correction, the function will adjust brightness as we want it.

.... and as a bonus, if WLED does not set brightness before "painting" but only after drawing, we could use max possible color resolution on strips that support more than 8 bit per color.

[Makuna]

ApplyPostAdjustments is meant for the case where you call Pixels() and manipulate the buffer directly. It is not meant to be called if you use SetPixelColor. All it does is get each pixel (by calling GetPixelColor which does not revert any previous brightness nor gamma correction already applied unlike the old brightness bus) and then apply the luminance and gamma correction before setting the color back. The key here is, SetPixelColor will apply Luminance and Gamma, then calling ApplyPostAdjustements will apply them again to those already modified values. Not what you want.

The "correct" way is to set Luminance, then draw all pixels, then call show in that order.

The reason is math. The old brightness bus, when you call change brightness, it would need to take the current value that is already fully rendered (brightness and gamma applied), try to reverse these with all the lost resolution and thus isn't the original value set, then reapply the new brightness and gamma. It is a flawed model that was never meant to be used the way brightness bus was being used. This is why I deprecated it.

If you need to call GetPixelColor, modify, and then SetPixelColor for a visual effect (not gamma or luminance but specific effect you do), then you should be using a DIB (device independent buffer class from the library with similiar API to a NPB) and do that to it, then Render that into the NeoPixelBus using the luminance/gamma shader exposed as strip.Shader member.

I want to add one technical detail that gets missed easily and glossed over.
The buffer of the NeoPixelBus (or NeoPixelBusLg) is in the byte format that is sent out. Not the format the hardware uses to send it, but the actual bits that are present read on the bus. So, its specific to the LEDs type targeted (device dependent). The bytes conform only to the device, so color order, pixel locations, and even bit encodings are device specific. This may lead to even more loss for those LEDs that are only 16 bits per pixel using a 555 encoding (yes, it's a thing that my library supports). So, when you call GetPixelColor, they have to be decoded back into the device independent RgbColor/RgbwColor. If you call GetPixelColor/SetPixelColor for a single pixel multiple times, it can take more CPU than is apparent.
Using a DIB, its buffer uses the device independent formatted bytes (RgbColor/RgbwColor/Rgb48Color/Rgbw64Color). So, calling GetPixelColor is fast and lossless.

[softhack007]

@Makuna thanks for explaining, makes perfect sense :-)

I think our way forward could be

A) hotfix for beta-3, based on the proposal from @blazoncek and @Aircoookie to use ApplyPostadjustments(). We are only using NeoGammaNullMethod, so the behaviour might be similar to the old BrightnessBus - maybe with reduced color quality

B) investigate how to integrate the DIB object into our code, as it seems this is the proper way of doing it.

@Makuna would the DIB increase memory needs, compared to NeoPixelBusLg?

[Aircoookie]

As I understand, DIB is a secondary buffer to enable lossless getPixelColor() and brightness adjustments. It would need LED count * 4 bytes extra memory in case of RgbwColor, which is no issue on ESP32 at all, but would force us to reduce maximum possible LEDs on ESP8266.
It may be worthwhile though, as it would also allow us to remove the current per-segment optional double buffering some effects do and thus simplify code and reduce the amount of dynamic allocations.

[Makuna]

Aircookie is correct, RgbColor DIB is Count * 3, RgbwColor DIB is Count * 4. Of course the Rgb48Color and Rgbw64Color are double those if you even want to use them.

Another feature of the DIB double buffer model, is that when you render into a 16 bit per element Bus, it still has benefits even if you only use an 8-bit per element DIB source as the render will up translate before doing the math for the final 16 bit per element bus.

[blazoncek]

We already have global LED buffer, which is (ATM) CRGB[ledcount] and, if enabled, serves for strip.getPixelColor() instead of calling busses.getPixelColor().
It is missing W channel but that is hardly used in any effect.

[amikell94]

KingSuperNerds>

This is exactly the problem that occurs on full white as well!! And if you look at your red board light, it's probably doing it to!

[amikell94]

Lowering Brightness to around 75% alleviated the issue. But due to safety implications with my power draw without the limiter, I reverted back until I can get an actual psu and wiring to handle the full load.

[KingSupernerd]

Is there a difference between the three temp fixes above? i tried the first one last night with success, didn't know if I should revert to the latest ones.

[blazoncek]

There's only one fix.
The other two mentions are from another fork.

[softhack007]

There's only one fix. The other two mentions are from another fork.

Confirmed :-) the "other two" are the same fix, just merge into the MoonModules fork (needed to do it twice). Github is a bit stupid about this, whenever some does cherry-picking from upstream, it gets listed as "pushed a commit".

So there is only one hotfix, and two copies of it in the other fork.

[sagitt]

Hi,
this is my config:

and the result, with b3, is this:
https://github.com/Aircoookie/WLED/assets/169086/e8578ef4-fda4-4f27-b58b-95d9d30deff3

(Sorry for bad video conversion)
but with the old b1, no problem.
it's related to this issue?

[KingSupernerd]

It looks like what I was experiencing. the temporary code fix worked for me, but they have converted it to a pull request that is in review processes. I would say you are seeing the same thing though.

[blazoncek]

@Makuna a bit of help needed.

I changed our code (in alt-buffer branch) to do the following (pseudo):

leds.SetBrightness(bri);
if (buffered) {
  for (i=0; i<leds.length(); i++) leds.SetPixelColor(i, c);
} else {
  leds.ApplyPostAdjustments();
}
leds.show();

If I added leds.SetBrightness(255); after the leds.show(); I would get wrong pixel colors (when reading with leds.GetPixelColor(i);) every other leds.show(). Which is causing pulsing like described above.

Do you have any idea why would such thing happen?

My initial thought was:

• keep bus brightness always at 255 (or full brightness)
• paint pixels un-scaled with SetPixelColor() if using un-buffered access
• when attempting to show() apply intended brightness with SetBrightness()
• paint pixels if buffered or apply ApplyPostAdjustments() if un-buffered
• do actual show()
• return bus brightness to 255 so that following SetPixelColor() will put un-scaled pixel values into buffer

[Makuna]

@blazoncek
Are you only working on ESP32 to demonstrate the issue you are hitting? RMT method only? Note that for RMT, due to the "translate" on the fly from low level API I use, it has two internal buffers, one to edit and one for active sending. So after a show, the buffers use swap. This means, the pointer from the Pixels() call will be different. It also means the GetPixelColor will be from the other buffers data; but internally it copies the data to maintain consistency (slower). To maintain consistency, then Show must be called with maintainBufferConsistency argument set to true, which is the default if you don't supply it. To improve speed but loose consistency, call it with false. Check your Show and see if you actually supply an argument. If its false, alternating shows will have old data when you call GetPixelColor right after a show.

How are the colors set in your psuedo code example for !buffered?

With NeoPixelBusLg, GetPixelColor will return what is the internal buffer, usually fully modified with luminance and gamma; not what you called SetPixelColor with. In the case you directly modify it using the pointer returned from Pixels(), then until you call ApplyPostAdjustments it will be whatever you set it too, and after you call ApplyPostAdjustments it will be the fully modified with luminance and gamma. After a show, it should not be affected except as outlined above with RMT and calling Show(false).

[blazoncek]

Thank you for explanation @Makuna
Yes, I am testing with RMT (ESP32-S2 ATM, but regular ESP32 shows the same behaviour). We use Show() without parameter so it is the default.
For color we store uint8_t[4] internally if using buffer (8-bit RGBW) and retrieve it as c = buf[i+3]<<24 | buf[i]<<16 | buf[i+1]<<8 | buf[i+2] (0xWWRRGGBB). For un-buffered we call SetPixelColor() and GetPixelColor() (we do channel swap if needed, but not the case in my testing) then convert it similarly to uint32_t.
While I browsed NeoPixelBusLg code I got the impression you are describing so my above pseudo code should return almost original pixel value (as we try to undo brightness in our getPixelColor()). Yet it behaves as if every other show would apply brightness and the other would not.
I will try with Show(false) and see if there is any difference.
Thanks again for your prompt reply.

[Makuna]

When you write into the buffer, are you also calling strip.Dirty() to let it know you modified the buffer? If not, the next ApplyPostAdjustments() and Show() would be skipped.
See Wiki for Pixels() method.

Calling SetPixelColor() internally marks it as dirty.
I also noticed that the NeoPixelBusLg when you call SetLuminance() doesn't check for a change and internally mark it as dirty; which it really should.

[blazoncek]

No, we never write directly into the NeoPixelBus buffer. We always use SetPixelColor() which does set dirty bit AFAIK.
We do one of the following:

• store pixels in our buffer, set luminance, paint pixels from our buffer using SetPixelColor(), do Show()
• paint pixels using SetPixelColor(), set luminance, apply post adjustments, do Show()

[Makuna]

so, with this

paint pixels using SetPixelColor(), set luminance, apply post adjustments, do Show()

You really should first set luminance to 255, then paint pixels using SetPixelColor(), set luminance to real value it should be, apply post adjustments, do Show().

An advanced way to do this that bypasses the built int shader is...

uint8_t* data = strip.Pixels();
for every pixel from custom buffer
    NeoRgbFeature::applyPixelColor(data, indexPixel, color);  // use the static method applyPixelColor off the feature you use to define your NeoPixelBusLg.
strip.Dirty(); // needed due to modifying the buffer
strip.SetLuminance(newLuminance);
strip.ApplyPostAdjustments();
strip.Show();

[blazoncek]

You really should first set luminance to 255, then paint pixels using SetPixelColor(), set luminance to real value it should be, apply post adjustments, do Show().

That was exactly my idea, but to make it simpler to maintain, I moved SetLuminance(255) immediately after Show() so it is ready for next frame.

Yet somehow GetPixelColor() was returning "inappropriate" values every other Show(). 😄

Advanced method would be too complicated using our flexible bus infrastructure.

[Makuna]

Do you know which frames it is showing "inappropriate" values? Ones only when use one specific method of the two you supplied?
Having static data you send and apply/confirm would tell you. If you always send the same "image" for every frame then you can put in some test code to confirm after set and log information when it doesn't match (frame count is another simple debug thing to add that is easy to include in a log that provides interesting details.)

[blazoncek]

Do you know which frames it is showing "inappropriate" values? Ones only when use one specific method of the two you supplied?

When using our buffer (and retrieving color information from it) our brightness limiting routine works as expected. But when using:

• paint pixels using SetPixelColor(), set luminance, apply post adjustments, do Show()

It doesn't. It is "pulsing" every other frame. So using only NeoPixelBusLg, the every other frame we retrieve "inappropriate" values from GetPixelColor() if using SetLuminance(255) immediately after (every) Show() even though all pixels are painted with SetPixelColor() before we fetch them with GetPixelColor().

If I reiterate our workflow in a flawed case in a slightly different way:

1. SetLuminance(255)
2. for all pixels SetPixelColor()
3. summarize GetPixelColor() for every pixel*
4. if sum is above threshold, temporarily reduce our brightness value
5. SetLuminance(brightness)
6. ApplyPostAdjustments()
7. Show()
8. SetLuminance(255)
9. repeat 2.

NOTE * GetPixelValue() undergoes color restoration process which does (c*255+brightness)/(brightness+1)

I am testing with static full white color only (FPS is about 3-4, but I could reduce it to 1 for testing). I will add some code, to display actual GetPixelColor() values but it may take some time as I have some IRL stuff to do.

[blazoncek]

Thinking of it, there is no need to restore brightness of modified pixels we only need to do it for unmodified ones (if not every pixel was touched).
@Aircoookie perhaps this is the place of error. Dual use case for getPixelColor()?

[Aircoookie]

@blazoncek that is true, but would require us to keep track of which pixels were modified in the current frame, which could be complex and memory intensive. At that point, true double buffering or just repainting the entire bus to the new brightness seems preferable.

@Makuna thank you for your help, I highly appreciate it! One small clarification question, I always assumed it'd be unsafe to call SetPixelColor() before the async update has finished so as to not change the buffer while it is sending out.
Now I've read in the wiki that internally two buffers are used for async methods where Show() returns before the LED update is finished.
So my question is, is it always safe to call SetPixelColor() immediately after Show() (given maintainBufferConsistency = true) or do we have to wait until e.g. CanShow() returns true?

[blazoncek]

@Makuna thank you also from my side for help and clarifications. The problem was entirely on our side and has now been mitigated.

[Makuna]

@Makuna my question is, is it always safe to call SetPixelColor() immediately after Show() (given maintainBufferConsistency = true) or do we have to wait until e.g. CanShow() returns true?

It is safe to call SetPixelColor and even call Pixels() to get the pointer to buffer to modify it while CanShow() is still returning false. Assuming a single thread calls SetPixelColor and Show (or you have appropriate blocking in place).
Yes, there is internal double buffering happening when a method supports an Async Show, basically all methods other than bitbang.