(feat/enhance) Tie RGB Underglow into sleep statuses

[mcrosson]

This PR includes the following

• Ties the underglow on/off into the sleep status events so the underglow will blank when a device goes to sleep
• Mild refactor of rgb underglow toggle to be a dedicated set of on/off methods
• Slightly re-orders methods to be more clear / ordered

[Nicell]

Is there any chance you could separate the re-ordering of functions and the additions you've made into separate commits? It's hard to read the diff for me at the moment.

[mcrosson]

@Nicell force pushed an update that undoes the method order changes and should make the change log easier to follow

[Nicell]

I'm wondering if the optional save to state of the ext_power settings should be in a separate PR that also improves upon #417. I'm liking the direction you're going with optional save to state.

[Nicell]

I like the addition of the optional save to state, but is there any reason we can't then pull the previous state from settings instead of saving previous state on the state (the previous_on variable)? For example settings_load_subtree("rgb/underglow") will load the state in settings to the state variable. I think that could be used instead, and then we could get other previous state rather than just on state.

[mcrosson]

The problem here is the settings save timeout(s). If the settings save timeout trips we don't know what previous state was valid relative to what was saved. I added the previous_on to work around the settings being saved and breaking the actual pre-sleep + pre-save state.

[Nicell]

I'm a bit confused. You've added the optional save to state parameter, so that settings save doesn't happen when sleeping etc, but now you don't trust the settings store to be proper?

Let's say I do this:

• Turn off RGB
• RGB is turned off in settings
• Board goes to sleep and turns off RGB again, settings aren't saved due to the optional
• Board wakes up and then pulls from settings, RGB is off as it should

And then alternatively I do this:

• Turn on RGB
• RGB is turned on in settings
• Board goes to sleep and turns off RGB, settings aren't saved due to the optinal
• Board wakes up and then pulls from settings, RGB is on as it should

In both these cases, settings is the source of truth. It seems to me that previous_on is the opposite of state.on, which in the first exercise above would cause RGB to turn on when it shouldn't.

I should also note that when the device goes into sleep mode, the settings can't be saved because the board is going into deep sleep. Also currently RGB turns off automatically at sleep with #417. I think maybe this should be refactored to work more alongside ZMK_ACTIVITY_IDLE?

[mcrosson]

Couple notes that I caught while re-visiting this PR:

• Updated to use settings_load_subtree("rgb/underglow") (will be pushed shortly)
• Was already taking ZMK_ACTIVITY_IDLE into account so this will fire when the board sleeps or goes idle

[jrhrsmit]

I tested this out for a while, but I had a lot of trouble with the state being saved (basically what mcrosson described earlier).
Because I set the idle timeout to 5 seconds, the save timeout (CONFIG_ZMK_SETTINGS_SAVE_DEBOUNCE) had to be shorter than that (I set it to 3s) to save any changed settings before it goes to sleep. However, this configuration wasn't stable at all (I don't really know why, but it kept crashing. I guess NVM erases take too long or something).
My guess is that I'm the only one setting the idle timeout so low, and when the idle timeout is 10mins and the save debounce is 1min everything just works.
Although I do like the idea of the saved settings being the single source of truth, I do think saving so often to NVM is a bad thing in this case.
The easy solution would be to just add the previous_on again.

Besides all that, I would love to see a fade in/out when turning off and on. At some point I had implemented this as effects, but to transition back to the old effect it also requires a previous_effect and also the pixel values from when it went to sleep (although that could be implemented in a different way).

[mcrosson]

Were you able to try this when previous_on was present in the code?

[jrhrsmit]

No, but when I just add the previous_on and this segment:

    case ZMK_ACTIVITY_ACTIVE:
        // Don't auto on underglow unless it was on before the board went to sleep
        if (state.previous_on) {
            zmk_rgb_underglow_on(false);
        }
        break;
    case ZMK_ACTIVITY_IDLE:
    case ZMK_ACTIVITY_SLEEP:
        state.previous_on = state.on;
        zmk_rgb_underglow_off(false);
        break;

Then it works.
Also, I just got an error that I haven't seen before (without the above edit):
<err> settings: set-value failure. key: rgb/underglow/state error(-22)
this error comes from zephyr/subsys/settings/src/settings.c:220, which might be what's causing my problem, but I haven't found what the return code means yet...

[mcrosson]

That's about what I had for the previous_on code before.

The error could be related to the write not finishing or working due to sleep / idle related state.

This definitely needs additional work and I'm inclined to add previous_on back to the code as long as @Nicell agrees based on your feedback.

[mcrosson]

I'm wondering if the optional save to state of the ext_power settings should be in a separate PR that also improves upon #417. I'm liking the direction you're going with optional save to state.

I'd like to keep it trim for this PR so the underglow turning off during sleep can get in faster for users to enhance battery life.

Maybe a new ticket / feature request to do better about when to save settings would be better. I feel like being better about setting saves (ie. only if saving differences) would be better as a dedicated PR and unit of work.

[mcrosson]

@Nicell Updated the sources and rebased to include latest main branch