Why toggleLedsOnSuspendResume needs to be included as part of the sketch and can’t be included as part of Kaleidoscope?
Why there is another function hostPowerManagementEventHandler that merely redirects to toggleLedsOnSuspendResume?
Why with latest Kaleidoscope and the above functions included as-is in (the end of) my current sketch don’t work for me? Before the move to the unified Kaleidoscope+plugins repo it was working, that’s all I know. I’m using Kubuntu Bionic with latest updates.
It could be included in Kaleidoscope, now that it’s a monorepo. It was not included with the HostPowerManagement plugin because we didn’t want to depend on LEDControl.
Because toggling LEDs on and off is just one thing you can do in response to the host going to sleep. You aren’t limited to turning off the LEDs. Not to mention that some of the boards Kaleidoscope supports now don’t even have LEDs.
I might have broken something - will try to test sometime in the next couple of days. Feel free to file an issue about this! (I can keep track of issues more easily than forum posts)
So how do you want to handle this? From the lines: https://github.com/keyboardio/Kaleidoscope/blob/master/src/kaleidoscope/plugin/HostPowerManagement.cpp#L60
I see that there is a weakly linked empty hostPowerManagementEventHandler within the plugin. Do you envisage that the user would like to include other actions to take place on suspend as part of the sketch? In which case only the definition toggleLedsOnSuspendResume can be pushed to the library and the sketch would still need to include the actual hostPowerManagementEventHandler calling toggleLedsOnSuspendResume again. This doesn’t seem all that useful vis-a-vis reducing code clutter in the sketch.
Another (better?) idea is: hostPowerManagementEventHandler within the plugin can be converted to strongly linked, and it should call toggleLedsOnSuspendResumeand another new helper function which can then be weakly linked and customized in the sketch, just like macroActions, tapDanceActions etc.
This fact doesn’t seem so relevant here? I mean because Kaleidoscope still includes LEDControl etc as default and they merely don’t do anything on such keyboards?
I think that the current setup is the correct one. I wouldn’t want to move toggleLedsOnSuspendResume into hostPowerManagementEventHandler, for two reasons:
I’d like to give the end-user the option to not turn LEDs off on suspend. They might want to just dim it. Or turn turn it off slowly with an animation (since the keyboard stays powered even during the host’s suspend, it is possible to fade out the leds).
Toggling LEDs off introduces a dependency on LEDControl, which I’d like to avoid, because Kaleidoscope supports boards without LEDs, and the dependency there would be wrong. We could likely figure out a way to let the compiler optimize out LEDControl in this case, but it is a lot easier if the default hostPowerManagementEventHandler is free of dependencies.
Case in point, I have a KBD4x that I use with a laptop from time to time. It has no per-key LEDs, but has an underglow. As such, it does not use LEDControl.
When the host suspends, I made it do a little Knight Rider animation with the underglow, and it does a small, dim red pulsing animation (which lasts about 10 seconds) every ten minutes to remind me that the laptop is still on, just suspended, and perhaps I should turn it off instead. When the laptop resumes, the keyboard does a different kind of animation (random color splash on all leds for a few seconds).
OK fine but at least can’t the existing toggleLedsOnSuspendResume be moved within Kaleidoscope? For instance you fixed the paused = true bug recently but every sketch of every user has now to be updated else they will face the problem. Likewise any future change can affect the users.
I suggest that only the hostPowerManagementEventHandler can be part of the sketch and in the default sketch for M01 it would call toggleLedsOnSuspendResume and users would be free to use it or not.