IdleLEDs: Optional support for EEPROM-Storage & Focus #708
Conversation
algernon
added
enhancement
|
FWIW, I'm open to changing the Focus command, but since Dygma's Raise will ship with the current command, we will need a way to remain compatible. Either in firmware, or - as is more likely - in Chrysalis. |
|
When I read the initial description of this PR it reminded me of a plugin that I wrote a while ago which also helps to make things runtime configurable: https://github.com/CapeLeidokos/Kaleidoscope-RemoteControl The plugin follows a slightly different path as it removes the burden of requiring special versions of plugins that are meant to be runtime configurable. Its main idea is to allow the user to identify member data or accessor methods/functions from within the sketch that are supposed to be runtime configurable. Values can be set and retrieved via the Focus protocol. Currently settings get lost once the keyboard is unplugged. But the plugin is still very helpful to play with specific settings without the burden of running compile/flash/test cycles. I felt it worth being mentioned as the plugin could be slightly modified to work with EEPROM to for the user selected settings to be persistent. The plugin could then load them at firmware startup. By this means it would be possible to gain general runtime configurability with persistence of settings and without requiring to touch any other plugins or parts of the firmware. Not sure if this could somehow be made working together with Chrysalis. |
|
FWIW, for you usecase the setup in the sketch would read #define REMOTE_CONTROL(OP) \
DIRECT_ACCESS(OP, 0, ::IdleLEDs::idle_time_limit)
REMOTE_CONTROL_INIT(REMOTE_CONTROL)To set an idle timeout of 1000 ms you would do the following. |
|
@noseglasses - that does seem like a very useful tool for firmware that's
insufficiently configurable. As a -goal-, such a plugin shouldn't be
needed. I'd rather make everything sensible run-time configurable.
best,
jesse
ᐧ
…On Mon, Nov 4, 2019 at 8:57 AM noseglasses ***@***.***> wrote:
FWIW, for you usecase the setup in the sketch would read
#define REMOTE_CONTROL(OP) \
DIRECT_ACCESS(OP, 0, ::IdleLEDs::idle_time_limit)
REMOTE_CONTROL_INIT(REMOTE_CONTROL)
To set an idle timeout of 1000 ms you would do the following.
focus-test remote_control 0 1000
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#708?email_source=notifications&email_token=AAALC2G3JRHZFVEJSGSBN6LQSBH67A5CNFSM4JIWAYUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC76QXA#issuecomment-549447772>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALC2BDWG3MOA34CMZISZ3QSBH67ANCNFSM4JIWAYUA>
.
|
|
I think I'd rather just add features to IdleLeds, rather than add a subclass that adds a feature. I don't think I'm thrilled that it's IdleLeds.idle_time_out. I think we could drop the second idle_. In terms of compatibility - At least eventually, presumably we'd be pushing new firmware to a raise anyway.. |
|
@obra, the problem with runtime compatibility is that if you add it to all sorts of plugins the way @algernon does with this PR, it will easily consume a lot of PROGMEM without being always wanted by the user (if a stock sketch would use the runtime configurable plugin versions instead of the original ones). That's probably the main advantage of my approach. Only a minimum amount of PROGMEM is consumed and also only where the user really wants to invest it. Even if there were runtime configurable versions of most stock plugins they probably could not simply replace the non-configurable versions in the stock firmware because of PROGMEM restrictions. Which depends of course on the degree of configurability. So it looks as if this problem can only be solved by using a custom sketch (with only a selection of configurable plugins) that is then configured through Chrysalis. But once a custom sketch gets involved, a more general approach that makes literally everything - plugins and the firmware core - runtime configurable is IMHO not such a bad choice. Mind you, with my plugin, the host side can be completely agnostic of what is configured. It just queries the keyboard for what configuration options it supports based on the user's configuration in the sketch and what data types are involved (I/O). That's something a configuration tool like Chrysalis could easily support. That way even arbitrary user plugins and any other settings of the firmware core could be made runtime configurable. However, I surely understand @obra, that your interest in the most user-friendly solution is paramount. My intention was mainly to point out the resource restriction bottleneck and the high implementation and maintenance expense that will result from adding custom runtime configuration code to existing plugins. This is really a strategic decision that needs to be well considered. |
|
@noseglasses, I hear you.
On Mon, Nov 4, 2019 at 10:11 AM noseglasses ***@***.***> wrote:
@obra <https://github.com/obra>, the problem with runtime compatibility
is that if you add it to all sorts of plugins the way @algernon
<https://github.com/algernon> does with this PR, it will easily consume *a
lot* of PROGMEM without being always wanted by the user (if a stock
sketch would use the runtime configurable plugin versions instead of the
original ones).
That's probably the main advantage of my approach. Only a minimum amount
of PROGMEM is consumed and also only where the user really wants to invest
it.
The big problems with that for me are:
1. it makes it harder to generally support the various bits of
configurability on the other side, since we have much less of an
understanding of what the compiled firmware supports.
2. It doesn't let us put as clean a facade in front of the implementation,
if we want to change how the backend code works while still keeping the
same wire protocol.
Even if there were runtime configurable versions of most stock plugins they
probably could not simply replace the non-configurable versions in the
stock firmware because of PROGMEM restrictions. Which depends of course on
the degree of configurability.
Over time, this will become less of an issue. The AVR inside the Model 01
is dramatically more resource restricted than most other microcontrollers
folks are moving to. Also, I'm betting that we can do better on the
resource usage front than we are today.
My intention was mainly to point out the resource restriction bottleneck
and the high implementation and maintenance expense that will result from
adding custom runtime configuration code to existing plugins. This is
really a strategic decision that needs to be well considered.
Understood and agreed.
I...wonder if there's a middle ground around a simple
get-prop/set-prop/store-prop focus command. that takes a plugin and a
property. (set-prop would set in memory. store-prop would write to non
volatile storage)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#708?email_source=notifications&email_token=AAALC2FVNCBZKCKHYVSZ2RDQSBQVRA5CNFSM4JIWAYUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDAGC5I#issuecomment-549478773>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAALC2COIHA7WPEAXUMQNBLQSBQVRANCNFSM4JIWAYUA>
.
ᐧ
|
|
The reason I put it into a separate object is that going from If I merge As for the focus command, how about |
That's true but there are going to be a lot of AVR based keyboards around for quite a while. And it would be really cool to support them all as long as possible. I, for example, got quite addicted to the Model01's thumb arc and therefore will likely stick with it for a while 😄
I am not sure if I understand. What I propose is the exact opposite of messing with internals. instead of adding any runtime configuration and Focus code to any of the plugins (like it already happened with some of them - see this PR), let a completely independent subsystem use the plugins' accessor methods to get/set parameters via serial communication and use auto-allocated EEPROM-storage to make those settings persistent. The interface between the proposed subsystem and the plugins/the firmware core is configured through some very simple macros from the user's sketch. In an optimal case (i.e. when using only accessor methods and keeping all member data private) there's no interaction with any backend of any plugin or the firmware at all. I can't imagine any cleaner separation. In contrast, adding Focus related code to individual plugins and letting it access member data adds tons of boilerplate code and unwanted direct interaction with the plugins' internals (e.g. Just look at this part of the settings_base_ = ::EEPROMSettings.requestSlice(sizeof(uint16_t));
// If idleTime is max, assume that EEPROM is uninitialized, and store the
// defaults.
uint16_t idle_time;
KeyboardHardware.storage().get(settings_base_, idle_time);
if (idle_time == 0xffff) {
KeyboardHardware.storage().put(settings_base_, idle_time);
}
setIdleTimeoutSeconds(idle_time);
...And that's only part of the actual code necessary. @algernon, no offense intended when citing you code. The implementation is nice, clean and easy to read. It only adds huge amounts of boilerplate code whose maintenance does not scale very well.
That's pretty close to what the remote control plugin already does. Though it's still just a proof of concept and not very elaborated when it comes to the focus interface. As I wanted to keep the memory footprint low, I did not use command strings for the focus commands (set-prop/get-prop) but that would be a very simple change. So it already does the get-prop/set-prop part (it even supports get&set in a joint operation). store-prop is not yet supported as the plugin does not use EEPROM to store any settings (it was originally only meant to be used for realtime device/host interaction like incoming message notification, etc.). But EEPROM support would also be really simple to add in a generic fashion. Nevertheless, I would leave it for the user to decide for each individual setting whether to allocate EEPROM or not. When someone just wants to set the color of an LED for e.g. an incoming message notification, this is the type of information meant to be stored in EEPROM. Let me summarize the main advantages of the strategy I propose:
I do not want to force my implementation upon you, rather the idea 😏. AddendumIf we had no hardware resource limitations, we could even go without modifications to the sketch. Then all plugins could register their accessors with the configuration subsystem (e.g. like boost-serialization does it). Every plugin could feature a standardized method. template<typename _Archive>
void SomePlugin::serialize(_Archive &archive) {
archive.memberFunction("some_setting", &SomePlugin::setSomeSetting, &SomePlugin::getSomeSetting);
...
}For the template parameter But given the AVR dominated keyboard world I do not see this happen too soon. |
|
As boilerplate code is one of my main concerns with the way Focus/EEPROM support is added to some plugins, I submitted an example PR that demonstrates who Focus/EEPROM can be added to plugin class members with only little boilerplate code. Mind you, the example compiles but I had no time for testing it thoroughly. There might be some minor issues. |
Having to recompile and re-flash firmware to set the idle timeout of the plugin isn't a fun or desired experience. It's fine when one already figured out the timeout they want, and have no desire to change it. For everyone else, being able to configure it at run-time via Focus, and have it persist to EEPROM is a much nicer experience. This change adds an alternative version of the plugin, `PersistentIdleLEDs`, a subclass of the original one. This one provides the focus command and persistence. It's a child class, because that results in a smaller footprint than a separate plugin that calls the `IdleLEDs` object. The code borrows from - but is not wire-compatible with - Dygma's implementation by @mattvenn. Signed-off-by: Gergely Nagy <[email protected]>
algernon
force-pushed
the
idleleds/focus-support
branch
from
268963f to
311a676
Compare
|
Rebased on top of current master, to make Travis happy. Also fixed the uninitialized settings case, the Focus command isn't stored twice anymore, and has been changed to I also added the focus command to the docs. While I agree that we should come up with a better way of adding Focus & Storage support to plugins, that's going to be a separate issue, because it's a tad more complicated than what I feel is appropriate here. For the time being, I'd settle with something like this, to allow us to move forward. |
|
What's the actual use case / business need for this? |
|
The use case is that people want Most customers of Dygma aren't really the tech-type people, the expectation is that very, very few will ever recompile their firmware. Flash a new one, sure, no problem. But change and recompile is unlikely. Thus, being able to change a desirable setting on-the-fly, and let it persist is useful for their end users. That's the use & business case for this. We could make it non-optional, but that presents other problems: increased PROGMEM use, and possible EEPROM layout issues, when a plugin that didn't use storage suddenly starts to, offsetting anything that was inited after it. For example, if we added storage unconditionally, the Model01 Firmware would break its EEPROM layout, because Having a separate object, they have to make a conscious decision to change to it, and may move |
|
(As an additional info: HostPowerManagement doesn't seem to reliably function on SAMD yet, so even if the host goes to sleep, the keyboard doesn't react reliably, thus, IdleLEDs is even more useful there.) |
|
I'm willing to take this, but it feels like a bit of a canary for things starting to get messy with settings and custom commands. We also really need to rethink how we do EEPROM storage ordering across firmware flashes. The simplest might just be to always dump eeprom over focus before flash and then restore eeprom afterward. |
Having to recompile and re-flash firmware to set the idle timeout of the plugin isn't a fun or desired experience. It's fine when one already figured out the timeout they want, and have no desire to change it. For everyone else, being able to configure it at run-time via Focus, and have it persist to EEPROM is a much nicer experience.
This change adds an alternative version of the plugin,
PersistentIdleLEDs, a subclass of the original one. This one provides the focus command and persistence. It's a child class, because that results in a smaller footprint than a separate plugin that calls theIdleLEDsobject.The code borrows from - and is compatible with - Dygma's implementation by @mattvenn. This compatibility is why we're storing only
uint16_t(ie, seconds), and why the focus command isidleLeds.