IdleLEDs: Implement a .suspend() and a .resume() method #1289
Conversation
Sometimes we want to control LEDs outside of `IdleLEDs`, like through `HostPowerManagement`, and in these cases, we would like `IdleLEDs` to have an up-to-date idea about what's going on. That is, if the host enters sleep, `IdleLEDs` should be aware of that, and enter idle state. Once the host wakes up, either via the keyboard or otherwise, `IdleLEDs` should resume its tasks. With the new `.suspend()` and `.resume()` method, this becomes possible. This addresses a big part of #1287. Signed-off-by: Gergely Nagy <algernon@keyboard.io>
algernon
force-pushed
the
idleleds/suspend-resume
branch
from
bb57a1f
to
8d0a9ac
Compare
|
Posting this as a draft for now, because I haven't actually tested that this works as it should in all scenarios. I'd appreciate some help there, not sure when I'll have the cycles to test it myself. |
| @@ -44,26 +44,38 @@ void IdleLEDs::setIdleTimeoutSeconds(uint32_t new_limit) { | |||
| idle_time_limit = new_limit * 1000; | |||
| } | |||
|
|
|||
| void IdleLEDs::resume() { | |||
| // Enabling LEDs is fairly expensive, so we only do it if we have to. | |||
There was a problem hiding this comment.
In what way is it expensive to enable them? I guess running an animated effect is expensive, but there's a similar comment around disabling, so I'm not sure what this comment is referring to.
There was a problem hiding this comment.
It refers to LEDControl.enable() and .disable() doing this:
refreshAll(); // set_all_leds_to(CRGB(0, 0, 0)); in case of disable()
Runtime.device().syncLeds();
The .syncLeds() part is the most expensive, but we don't want to refreshAll() on every "resume" either, only when we change states.
There was a problem hiding this comment.
Ah. It might make sense to push that logic down into LEDControl, then. Maybe that should be a separate PR.
The problem with that is that the Model01/Model100 sketches are copies of chyrsalis-firmware-bundle's, and I'd rather not have them diverge too much. |
I think it's worth doing, to make the resume behavior a little less surprising. (both in Chrysalis-Firmware-Bundle and the examples) |
Sometimes we want to control LEDs outside of
IdleLEDs, like throughHostPowerManagement, and in these cases, we would likeIdleLEDsto have an up-to-date idea about what's going on. That is, if the host enters sleep,IdleLEDsshould be aware of that, and enter idle state. Once the host wakes up, either via the keyboard or otherwise,IdleLEDsshould resume its tasks.With the new
.suspend()and.resume()method, this becomes possible.This addresses a big part of #1287.