Fix RGBW not turning off when saturation is set to `0`. #24254

bryan065 · 2024-08-07T23:15:57Z

(Temporary?) Fix for RGBW not turning off when saturation is set to 0.

Description

Using RGB_MATRIX and RGBW LED's, when any LED's are set to 0 saturation, they won't turn off when toggled/timeout/sleep.

Warning: My white LED shines much brighter now when set to 0 saturation and true white (instead of RGB array white), this resulted in my keyboard pulling about 30% more power when the full keyboard LEDs are set to white. Had to turn down maximum brightness accordingly (which would also limit your RGB LED brightness). Future PR for separate RGB maximum brightness and W maximum brightness perhaps?

Code credited to @drashna .

Types of Changes

Issues Fixed or Closed by This PR

n/a

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

(Temporary) Fix for RGBW not turning off when saturation is set to '0'.

quantum/color.c

Co-authored-by: Ryan <fauxpark@gmail.com>

KarlK90 · 2024-08-09T18:40:31Z

@bryan065 the existing rgb to rgbw conversion is very basic. I do not have any rgbw leds so I can't test any different algorithm for the conversion but maybe give these a shot and see if they work better?

https://www.reddit.com/r/FastLED/comments/135tgrk/comment/jin52gy/?utm_source=share&utm_medium=web3x&utm_name=web3xcss&utm_term=1&utm_content=share_button
https://github.com/BertanT/Arduino-RGBWConverter/blob/main/src/RGBWConverter.cpp#L21-L60 (uses floating point so would have to be converted to integer math)
https://www.shadertoy.com/view/NsjBzt (the INT_MATH implementation)

sigprof · 2024-08-09T19:15:09Z

quantum/color.c

-    led->w = MIN(led->r, MIN(led->g, led->b));
-    led->r -= led->w;
-    led->g -= led->w;
-    led->b -= led->w;
+    if (led->r == led->g && led->r == led->b) {
+        led->w = led->r;
+        led->r = 0;
+        led->g = 0;
+        led->b = 0;
+    } else {
+        led->w = MIN(led->r, MIN(led->g, led->b));
+        led->r -= led->w;
+        led->g -= led->w;
+        led->b -= led->w;
+    }


Does this actually change the behavior? In the general case, if all RGB color components are equal, MIN(led->r, MIN(led->g, led->b)) should give led->r, and then subtracting that value from the RGB components should give 0; so the special case should not be needed.

That's why I've mentioned that this is a temporary workaround to the issue. Without led->w = led->r;, any LED's currently set to saturation 0 will not toggle off with RGB_TOG or sleep/timeout.

This code doesn't really care about RGB_TOG or sleep, it simply converts the RGB value to RGBW. So the issue is likely not in how it handles the saturation value 0 since it does not even operate on HSV values... perhaps it should actually be checking for the case where all three components are 0, rather than the same value?

Update: I just tested this on a Proton C and an RGBW Neopixel stick - UG_TOGG turns the LEDs on and off just fine at any saturation level. I don't think there's anything wrong with the conversion code as it currently exists in the repo. The issue you're having must lie elsewhere, and this PR should be closed.

I'll pull out some AVR controllers and my proton-c to continue testing I guess.

I'm only using SK6812's from BTF-Lighting right now so it might be a timing issue then..

I'll grab a RGBW neopixel to test too. Do you have any SK6812's you can test?

Thanks.

As far as I'm aware, the RGBW Neopixel stick uses SK6812RGBWs, and I'm pretty sure they are the only 5050 WS2812-likes that have white LEDs.

Thanks, I'll keep testing on my end. I also found this change improves the brightness of the white LED when saturation is set to 0 so there's that as well.

I realized I had the elite-c, not proton-c so all my testing would be on AVR if that would make any difference.

I'll close this PR if I can't reproduce the issue with any other controller+RGBW+rgb_matrix.

I previously tested it with a DZ60 (added rgb_matrix SK6812) and kbd67mkii/soldered (added rgb_matrix sk6812).

Ah, I was testing with RGBLight (Underglow). I'll try RGB Matrix in the morning, it may be something to do with that.

The real problem may be this piece of code in the set_color implementation for WS2812, which is from #21135 and tries to avoid a potentially expensive ws2812_setleds() call if there were no actual LED state changes:

qmk_firmware/quantum/rgb_matrix/rgb_matrix_drivers.c

Lines 180 to 182 in 5acc3b2

if (rgb_matrix_ws2812_array[i].r == r && rgb_matrix_ws2812_array[i].g == g && rgb_matrix_ws2812_array[i].b == b) {

return;

}

In the RGBW case rgb_matrix_ws2812_array contains RGBW data, and for the zero saturation the RGB components will be set to 0, so the above code will mistakenly ignore the request to set the RGB color to (0, 0, 0).

If we want to keep this optimization for RGBW LEDs, we would need to apply convert_rgb_to_rgbw() to a temporary buffer, and then compare all 4 components with rgb_matrix_ws2812_array[i] when detecting changes.

Yep, I'm able to repro with RGB Matrix and it is probably due to the above.

Untangling this will be a little more complicated since the WS2812 driver is the only one with this single set_leds() API, rather than the init/flush/set_color/set_color_all setup, which necessitates this ugly glue code. Additionally, RGBLight will require some refactoring to remove its own internal LED array, as that should really be the the driver's responsibility. Split support also needs to be retained, ideally moved into the RGBLight & RGB Matrix code so that it "just works" for any driver.

All of this is probably a blocker for the original issue here to be resolved.

bryan065 · 2024-08-10T02:54:13Z

@zvecr Indeed just led->w = led->r; is needed.

@KarlK90 I'm aware the current RGBW is extremely basic (to the point of just getting them working). Thanks for linking those so I might return to this in the future.

Right now, This fix is just for LED's not turning off when saturation is set to 0. Doesn't matter if it's the whole keyboard or just indicator LED's set to saturation 0.

Video of the issue. Going from 255 to 0 back to ~50 saturation. Tested with and without userspace, custom and default keymaps.

20240805_115339.1.1.mp4

github-actions · 2024-09-26T02:02:22Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

github-actions · 2024-10-27T02:06:39Z

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]

Fix RGBW not turning off

892a887

(Temporary) Fix for RGBW not turning off when saturation is set to '0'.

github-actions bot added the core label Aug 7, 2024

zvecr reviewed Aug 8, 2024

View reviewed changes

quantum/color.c Outdated Show resolved Hide resolved

fauxpark reviewed Aug 8, 2024

View reviewed changes

quantum/color.c Outdated Show resolved Hide resolved

Update quantum/color.c

9efaae1

Co-authored-by: Ryan <fauxpark@gmail.com>

sigprof reviewed Aug 9, 2024

View reviewed changes

Remove unnecessary led->r = 0;

936c537

github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Sep 26, 2024

github-actions bot closed this Oct 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RGBW not turning off when saturation is set to `0`. #24254

Fix RGBW not turning off when saturation is set to `0`. #24254

bryan065 commented Aug 7, 2024

KarlK90 commented Aug 9, 2024

sigprof Aug 9, 2024

bryan065 Aug 10, 2024

fauxpark Aug 10, 2024 •

edited

Loading

fauxpark Aug 10, 2024

bryan065 Aug 10, 2024

fauxpark Aug 10, 2024

bryan065 Aug 10, 2024

fauxpark Aug 10, 2024

sigprof Aug 10, 2024

fauxpark Aug 11, 2024

bryan065 commented Aug 10, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Oct 27, 2024

	if (rgb_matrix_ws2812_array[i].r == r && rgb_matrix_ws2812_array[i].g == g && rgb_matrix_ws2812_array[i].b == b) {
	return;
	}

Fix RGBW not turning off when saturation is set to 0. #24254

Fix RGBW not turning off when saturation is set to 0. #24254

Conversation

bryan065 commented Aug 7, 2024

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

KarlK90 commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fauxpark Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryan065 commented Aug 10, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Oct 27, 2024

Fix RGBW not turning off when saturation is set to `0`. #24254

Fix RGBW not turning off when saturation is set to `0`. #24254

fauxpark Aug 10, 2024 •

edited

Loading