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

APA102/SK9822 global brightness sometimes incorrect #656

Open
cmsavage opened this Issue Sep 26, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@cmsavage

cmsavage commented Sep 26, 2018

I am very happy that FastLED is now making use of the global brightness aspect of the APA102/SK9822 LEDs. Even with the brightness turned down very low (and using much less power), these LEDs are capable of providing a wide dynamic range of colors with this feature.

However, while trying out the global brightness setting for an SK9822 LED strip, I noticed some odd brightness behavior that I was able to track down the cause of. Specifically, in the following chipsets.h code:

#if FASTLED_USE_GLOBAL_BRIGHTNESS == 1
                const uint16_t maxBrightness = 0x1F;
                uint16_t brightness = (max(max(s0, s1), s2) * maxBrightness >> 8) + 1;
                s0 = ((uint16_t)s0 + 1) * maxBrightness / brightness - 1;
                s1 = ((uint16_t)s1 + 1) * maxBrightness / brightness - 1;
                s2 = ((uint16_t)s2 + 1) * maxBrightness / brightness - 1;
#else
                const uint8_t brightness = 0x1F;
#endif

the right side of the s0 calculation is occasionally greater than 255, in which case uint8_t s0 wraps around (262 → 6), resulting in a grossly incorrect brightness. This happens for several values of s.

Even if the overflow is caught and s is set to 255, the algorithm does not preserve scale*brightness, leading to significant deviations in the resulting LED brightness at lower scale factors. I suggest using the following, in which scale*brightness deviates by at most 0.4% (and typically much less):

#if FASTLED_USE_GLOBAL_BRIGHTNESS == 1
                const uint16_t maxBrightness = 0x1F;
                uint16_t brightness = ((((uint16_t)max(max(s0, s1), s2) + 1) * maxBrightness - 1) >> 8) + 1;
                s0 = (maxBrightness * s0 + (brightness >> 1)) / brightness;
                s1 = (maxBrightness * s1 + (brightness >> 1)) / brightness;
                s2 = (maxBrightness * s2 + (brightness >> 1)) / brightness;
#else
                const uint8_t brightness = 0x1F;
#endif

The above algorithm does not experience the uint8_t overflow.

@cmsavage

This comment has been minimized.

Show comment
Hide comment
@cmsavage

cmsavage Sep 26, 2018

Attaching spreadsheet for looking at brightness and scale calculations.

GlobalBrightness_rollover.xlsx

cmsavage commented Sep 26, 2018

Attaching spreadsheet for looking at brightness and scale calculations.

GlobalBrightness_rollover.xlsx

@kasperkamperman

This comment has been minimized.

Show comment
Hide comment
@kasperkamperman

kasperkamperman Sep 26, 2018

Just a quick chime in. I've compared global brightness on APA102 and SK9822 and they both really have a different behaviour. In my experience the SK9822 starts to shift Hue as well when changing the GBC parameter. I didn't test with the FastLED implementation, but with SmartMatrix. Some discussion: https://community.particle.io/t/smartmatrix-apa102-library-open-hardware-photon-apa102-shield/21562/67

kasperkamperman commented Sep 26, 2018

Just a quick chime in. I've compared global brightness on APA102 and SK9822 and they both really have a different behaviour. In my experience the SK9822 starts to shift Hue as well when changing the GBC parameter. I didn't test with the FastLED implementation, but with SmartMatrix. Some discussion: https://community.particle.io/t/smartmatrix-apa102-library-open-hardware-photon-apa102-shield/21562/67

@extesy

This comment has been minimized.

Show comment
Hide comment
@extesy

extesy Sep 26, 2018

Contributor

I didn't have SK9822 to test this on but I do have it now and will check the math soon. Thanks for the spreadsheet, @cmsavage!

Contributor

extesy commented Sep 26, 2018

I didn't have SK9822 to test this on but I do have it now and will check the math soon. Thanks for the spreadsheet, @cmsavage!

@extesy extesy referenced a pull request that will close this issue Sep 27, 2018

Open

Update global brightness math to fix overflows #657

@extesy

This comment has been minimized.

Show comment
Hide comment
@extesy

extesy Sep 27, 2018

Contributor

One thing I've noticed during the testing is that when setCorrection(TypicalLEDStrip) is used (like in provided FastLED examples) the hue shift is very apparent on low brightness settings. White color becomes some weird purple-ish tint.

Contributor

extesy commented Sep 27, 2018

One thing I've noticed during the testing is that when setCorrection(TypicalLEDStrip) is used (like in provided FastLED examples) the hue shift is very apparent on low brightness settings. White color becomes some weird purple-ish tint.

@christophepersoz

This comment has been minimized.

Show comment
Hide comment
@christophepersoz

christophepersoz Sep 27, 2018

christophepersoz commented Sep 27, 2018

@cmsavage

This comment has been minimized.

Show comment
Hide comment
@cmsavage

cmsavage Sep 27, 2018

I have been using setCorrection(UncorrectedColor) and did not notice a hue change through even the lowest brightness levels. Now that I try setCorrection(TypicalLEDStrip), I can see the hue changes at very low brightness changes. I can see one issue that causes hue changes: the 8-bit storage of the RGB scale factors in the controller.h computeAdjustment() routine. The scale factors s0, s1, s2 that are used in the APA102/SK9822 global illumination calculation in the parent post above are given by scale*(Correction[k]+1)*(Temp[k]+1)/65536 where scale is the overall scale passed into show(), and Correction and Temp are the RGB color correction and color temperature factors. Here are some results using the TypicalLEDStrip correction:

Scale Correction Adjusted (s0,s1,s2)
255 (255,176,240) (255,176,240)
128 (255,176,240) (128,88,120)
64 (255,176,240) (64,44,60)
32 (255,176,240) (32,22,30)
16 (255,176,240) (16,11,15)
8 (255,176,240) (8,5,7)
4 (255,176,240) (4,2,3)
3 (255,176,240) (3,2,2)
2 (255,176,240) (2,1,1)
1 (255,176,240) (1,0,0)

Because the final scale factors are restricted to integer values, they cannot be kept in the same ratios as the numbers get small. Calling show(4) results in a more purple hue [→(4,2,3)] than show(255) [→(255,176,240)], while show(3) is reddish [→(3,2,2)], show(2) is even more red [→(2,1,1)], and show(1) is just red [→(1,0,0)].

This scaling procedure works fine for 8-bit RGB outputs, but APA102/SK9822's essentially have a dynamic range of 31*256 (~13 bits) that this procedure was not meant to handle (in its current form).

@extesy, @christophepersoz: When you say low brightness levels, are you talking about scales below around 8, where the finite resolution effects of the adjusted scales start to be significant?

cmsavage commented Sep 27, 2018

I have been using setCorrection(UncorrectedColor) and did not notice a hue change through even the lowest brightness levels. Now that I try setCorrection(TypicalLEDStrip), I can see the hue changes at very low brightness changes. I can see one issue that causes hue changes: the 8-bit storage of the RGB scale factors in the controller.h computeAdjustment() routine. The scale factors s0, s1, s2 that are used in the APA102/SK9822 global illumination calculation in the parent post above are given by scale*(Correction[k]+1)*(Temp[k]+1)/65536 where scale is the overall scale passed into show(), and Correction and Temp are the RGB color correction and color temperature factors. Here are some results using the TypicalLEDStrip correction:

Scale Correction Adjusted (s0,s1,s2)
255 (255,176,240) (255,176,240)
128 (255,176,240) (128,88,120)
64 (255,176,240) (64,44,60)
32 (255,176,240) (32,22,30)
16 (255,176,240) (16,11,15)
8 (255,176,240) (8,5,7)
4 (255,176,240) (4,2,3)
3 (255,176,240) (3,2,2)
2 (255,176,240) (2,1,1)
1 (255,176,240) (1,0,0)

Because the final scale factors are restricted to integer values, they cannot be kept in the same ratios as the numbers get small. Calling show(4) results in a more purple hue [→(4,2,3)] than show(255) [→(255,176,240)], while show(3) is reddish [→(3,2,2)], show(2) is even more red [→(2,1,1)], and show(1) is just red [→(1,0,0)].

This scaling procedure works fine for 8-bit RGB outputs, but APA102/SK9822's essentially have a dynamic range of 31*256 (~13 bits) that this procedure was not meant to handle (in its current form).

@extesy, @christophepersoz: When you say low brightness levels, are you talking about scales below around 8, where the finite resolution effects of the adjusted scales start to be significant?

@christophepersoz

This comment has been minimized.

Show comment
Hide comment
@christophepersoz

christophepersoz Sep 27, 2018

christophepersoz commented Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment