feat(underglow): Add RGB auto off timeout on idle and on usb disconnect

[ReFil]

This is based off the excellent work by @bortoz but modified to work with the RGB underglow feature

if the config settings are enabled the lights will shut off when the board goes into idle and resumes when you push a key and wake the board.

Tested and working on my personal BT75 board, it's just a minor change

[petejohanson]

Thanks for the PR! A few items from my first review of this.

[berfarah]

This is super cool and something I'll definitely set up for myself.

I was wondering: Since we're seeing multiple instances of wanting to disable power-intensive behavior when not connected to USB, would it be worth centralizing this behavior under EXT_POWER (I'll do my best in cobbling together a PR)?

[petejohanson]

Sorry, one minor tweak left.

[berfarah]

Reporting in with testing on a split keyboard (corne) with brand new 110mah batteries on each side:
This doesn't appear to work. My config is:

CONFIG_ZMK_RGB_UNDERGLOW=y
CONFIG_WS2812_STRIP=y

CONFIG_ZMK_RGB_UNDERGLOW_EXT_POWER=y
CONFIG_ZMK_RGB_UNDERGLOW_AUTO_OFF_USB=y

When I unplug my keyboard from power, it immediately begins flickering. The only way I can get this behavior to kind of work is by leaving the USB connection half-way in - but the LEDs turn back on when I completely unplug for some reason.

Is there a difference in how USB events are triggered for split keyboards vs single-board keyboards?

[ReFil]

Reporting in with testing on a split keyboard (corne) with brand new 110mah batteries on each side: This doesn't appear to work. My config is:

CONFIG_ZMK_RGB_UNDERGLOW=y
CONFIG_WS2812_STRIP=y

CONFIG_ZMK_RGB_UNDERGLOW_EXT_POWER=y
CONFIG_ZMK_RGB_UNDERGLOW_AUTO_OFF_USB=y

When I unplug my keyboard from power, it immediately begins flickering. The only way I can get this behavior to kind of work is by leaving the USB connection half-way in - but the LEDs turn back on when I completely unplug for some reason.

Is there a difference in how USB events are triggered for split keyboards vs single-board keyboards?

That's strange, we've been testing this on a split without issue, what setup are you using?

[berfarah]

That's strange, we've been testing this on a split without issue, what setup are you using?

Sorry for the delay - had a baby and it took up some mental time!

In my case, I have a corne v3 with 5050 LEDs and a 110mAh battery using this config. It's possible the battery just can't supply the wattage required for the LEDs and this logic doesn't have a chance to run before the nice nano is severely underpowered.

[ReFil]

That's strange, we've been testing this on a split without issue, what setup are you using?

Sorry for the delay - had a baby and it took up some mental time!

In my case, I have a corne v3 with 5050 LEDs and a 110mAh battery using this config. It's possible the battery just can't supply the wattage required for the LEDs and this logic doesn't have a chance to run before the nice nano is severely underpowered.

Well congratulations on the baby firstly! 5050 LEDs are very thirsty, approx 50ma at full brightness but I can't imagine it'd kill it that quickly. I tried but couldn't replicate the specific issue you were describing

[ReFil]

Hi, just wondering if we can get this merged soon? a lot of our customers are interested in this feature getting merged

[petejohanson]

One code comment, and then a general item:

When testing this, the initial auto state isn't set for USB. If I've enabled the auto off USB setting, and I start the firmware from reset w/o USB connected, the RGB starts on, instead of off.

Need to update the auto stuff at the end of the init callback, I imagine.

[petejohanson]

Suggested change

	depends on USB
	depends on USB_DEVICE_STACK

Needed w/ the Zephyr 3.0 update.

[ReFil]

One code comment, and then a general item:

When testing this, the initial auto state isn't set for USB. If I've enabled the auto off USB setting, and I start the firmware from reset w/o USB connected, the RGB starts on, instead of off.

Need to update the auto stuff at the end of the init callback, I imagine.

Given itll always be active when the leds are initialised i think this is only relevant to usb, i'll add in that functionality

[ReFil]

Just checked and the backlighting auto off does the same thing, is it better to mirror the slightly worse behaviour so both sets of lights are in sync or fix the rgb now and then i can submit another pr to do the same for the backlighting?

[ReFil]

Rectified both issues

[petejohanson]

Minor style tweak. This should get fixed for the backlight logic as well, I'll create an issue for that.

[ReFil]

Once this is merged i can open a pr to sort that backlight thing as well

[petejohanson]

@ReFil Tested with the latest changes, and even though the animation isn't running, if I have a saved state it seems to still initialize the underglow w/ a fixed color at reset.

[ReFil]

@ReFil Tested with the latest changes, and even though the animation isn't running, if I have a saved state it seems to still initialize the underglow w/ a fixed color at reset.

by this do you mean it's overriding the animation state or it's not shutting off properly? on my little test board it was initialising with lights off properly

[petejohanson]

Actually, looks like it is fine, I think the way I had done a reset through the programmer hadn't done the same as a full reset.

Only lingering issue: This seems to still start the timer for the animation, even if we start off. That timer start shuld be conditional.

[ReFil]

Should we change it to call zmk_rgb_underglow_off and on then? that properly handles all the timer stopping and starting without intriducing superfluous code

[petejohanson]

Should we change it to call zmk_rgb_underglow_off and on then? that properly handles all the timer stopping and starting without intriducing superfluous code

I had considered that. The risk is that also persists the settings afterwards, which seems superfluous, and extra flash wear to be avoided.

Likely needs a small refactor to extract the "turn it off" from the "queue settings save" so we can invoke just the "turn it off" code.

[ReFil]

What if i just wrap the k_timer_start in the if enabled block with an else when it's not enabled?

[petejohanson]

What if i just wrap the k_timer_start in the if enabled block with an else when it's not enabled?

I'd totally support that, yeah.

[petejohanson]

Testing was looking good now. One minor style tweak needed for the init changes.

[petejohanson]

Thanks!