CharShift: Configuration plugin #1069

algernon · 2021-06-17T00:38:39Z

This is a plugin on top of CharShift, similar to AutoShiftConfig and inspired by DynamicTapDance. Apart from a very quick compile, it is completely untested.

The core idea is that we have two function pointers in CharShift that allows anyone to override the numKeyPairs() and readKeyPair() functions, but without relying on weak symbols, and the CharShiftConfig uses this to extend the charshift map. The map is simply stored as an array of key pairs in EEPROM, allocation is to be done in setup(), as CharShiftConfig.setup(0, 4) (to allocate 4 keypairs, starting at index 0, thus, a complete override in this case).

There's at least one thing missing: we need to be able to query the dynamic offset via Focus, so Chrysalis can present the editor in the future correctly.

…bles Instead of using overridable functions, use function pointers (+ setters) to allow changing lookups and related things. This allows a bit more flexibility, and makes the user sketch simpler, because there is no override necessary there. Signed-off-by: Gergely Nagy <algernon@keyboard.io>

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

gedankenexperimenter · 2021-06-17T02:15:13Z

plugins/Kaleidoscope-CharShift/src/kaleidoscope/plugin/CharShift.cpp

+CharShift::GetNumKeyPairsFunction CharShift::numKeyPairs_ =
+    CharShift::numProgmemKeyPairs;


I feel like this is unnecessary, but I'm not sure I'm right. Couldn't the same num_keypairs_ variable be used, regardless of where the data is stored? As long as we're reading values from either PROGMEM or EEPROM, but not both (perhaps by using an index offset), there's no need to use three bytes to store a separate count, plus the function pointer, right? Or are you thinking the user might want to switch back and forth between the two without power-cycling the keyboard?

I'm thinking that a user might want to use both at the same time, store some pairs in PROGMEM, some in EEPROM. Not sure if that makes sense for CharShift - it does for Macros and TapDance, where I copied the basic architecture from.

gedankenexperimenter · 2021-06-17T02:19:27Z

plugins/Kaleidoscope-CharShift/src/kaleidoscope/plugin/CharShift.cpp

-  if (i < numKeyPairs()) {
-    return readKeyPair(i);
+  if (i < numKeyPairs_()) {
+    return readKeyPair_(i);


Have we been using trailing underscores for private class function names as well as variables?

[My brain doesn't like the underscore immediately followed by the parens for some reason, but I'd much rather overhaul all the code to follow a consistent style than satisfy trivial preferences like that.]

Honestly, I don't like it either. And no, we've been using trail-less private function names in the past. In this case, it's there because I first added the function pointers, then removed the original functions. It's safe and fine to rename them.

gedankenexperimenter

I don't feel that I'm expert enough to do more than pose some (hopefully not stupid) questions. You mentioned that it would be impractical to use a function override in the config plugin (not the user sketch, which is pretty clearly undesirable) but I don't think I fully grasp why. Couldn't the EEPROM version store its max_pairs_, and if that value is zero (or if the base EEPROM address is invalid), fall back on PROGMEM, thereby still functioning if nothing has been configured in EEPROM, without using function pointers?

Again, I'm not convinced that would be better, but it seems like it would be more economical in both PROGMEM and RAM, and perhaps there's some advantage to the function pointer approach that's not apparent to me.

algernon · 2021-06-17T11:15:56Z

I don't feel that I'm expert enough to do more than pose some (hopefully not stupid) questions. You mentioned that it would be impractical to use a function override in the config plugin (not the user sketch, which is pretty clearly undesirable) but I don't think I fully grasp why.

I think if we'd use overrides, and did the override in the plugin, that would be susceptible to link-order issues. It would also make it that much harder for the end-user to have their override, would they wish to do so.

Couldn't the EEPROM version store its max_pairs_, and if that value is zero (or if the base EEPROM address is invalid), fall back on PROGMEM, thereby still functioning if nothing has been configured in EEPROM, without using function pointers?

We already store max_pairs_, and can fall back to PROGMEM, yes. But for the reasons above, I still think function pointers are the more reliable way.

Again, I'm not convinced that would be better, but it seems like it would be more economical in both PROGMEM and RAM, and perhaps there's some advantage to the function pointer approach that's not apparent to me.

Not depending on linking order is one such advantage.

Another - rather different - approach would be to derive CharShiftConfig from CharShift. In this case, I'd drop the weak symbols alltogether, along with the support of having both PROGMEM and EEPROM pairs. Less memory, less progmem, clearer code, at the cost of slightly less flexibility. Might need a different name than CharShiftConfig in this case, perhaps EEPROMCharShift?

algernon · 2021-06-17T11:26:54Z

Oooh, I had another idea!

What if we had a single plugin, CharShift, but abstracted away the storage? So we'd have ::charshift::ProgMemStorage and ::charshift::EEPROMStorage? Then we'd store a pointer to an object of the chosen storage class only, and that class would be free to handle storage and retrieval as it sees fit.

Thinking further... we'd still need CharShiftConfig for the Focus parts, to make those optional... or we could have that in the storage plugin, and either require the user to add the storage plugin to KALEIDOSCOPE_INIT_PLUGINS(), or delegate onFocusEvent(). I'd probably go with the latter. The ::charshift::ProgmemStorage class simply wouldn't implement it, so it'd be optimized out. ::charshift::EEPROMStorage would, and voila.

I feel this would be lighter on PROGMEM and RAM usage than the current implementation, but would provide pretty much the same flexibility, and not be susceptible to link-order issues either.

gedankenexperimenter · 2021-06-17T13:36:26Z

I think if we'd use overrides, and did the override in the plugin, that would be susceptible to link-order issues. It would also make it that much harder for the end-user to have their override, would they wish to do so.

I can't imagine a use-case for a user code override of these functions. The only reason I used a weak definition in the first place was the idea that a CharShiftConfig plugin could override it. You think we'd still get link-order problems, even if only one of the functions has a weak definition? If so, how does it work to have a weak definition in a plugin, and an override in the sketch (possibly in a *.cpp file in the sketch's source tree, not just directly in the *.ino file itself)—wouldn't that have the same link-order problem, if there is one? Just trying to understand.

gedankenexperimenter · 2021-06-17T13:42:38Z

Another - rather different - approach would be to derive CharShiftConfig from CharShift. In this case, I'd drop the weak symbols alltogether, along with the support of having both PROGMEM and EEPROM pairs. Less memory, less progmem, clearer code, at the cost of slightly less flexibility. Might need a different name than CharShiftConfig in this case, perhaps EEPROMCharShift?

I'm having trouble picturing this. By "derive", are you referring to inheritance?

gedankenexperimenter · 2021-06-17T13:55:39Z

I feel this would be lighter on PROGMEM and RAM usage than the current implementation, but would provide pretty much the same flexibility, and not be susceptible to link-order issues either.

If I understand correctly, this would require only two extra bytes of RAM for the storage object pointer over the function override method that might suffer from link-order problems. I haven't been able to figure out how it will work, but I'd like to understand it.

algernon · 2021-06-17T15:53:02Z

I'm having trouble picturing this. By "derive", are you referring to inheritance?

Yes.

If I understand correctly, this would require only two extra bytes of RAM for the storage object pointer over the function override method that might suffer from link-order problems. I haven't been able to figure out how it will work, but I'd like to understand it.

Two bytes for the object pointer + whatever RAM the object itself takes. Which shouldn't be much more (if more at all) than what CharShiftConfig currently uses.

I'll push up a variant soon, to show what I had in mind. It seems like a cleaner architecture in this case anyway.

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

algernon · 2021-06-17T16:52:57Z

Ok, did a quick comparison! Essentially the same sketch (0 builtin charshifts, 4 pairs in EEPROM):

Function pointers: 11620 PROGMEM, 894 RAM
Storage class: 11406 PROGMEM, 890 RAM

As such, we're saving a lot of progmem, and some ram too.

On the other hand, looking at the size map, we have a problem: in my implementation, I didn't use virtual to save RAM and stuff, but then we end up calling the storage object methods as if they were of Storage type, rather than their real one, which makes the whole thing nonfunctional. So I need to use virtual, and then it will use more RAM. Or I need to resort to some other trickery, which will make the code a mess (and might still end up being costy).

If we go down this route, we'd need to do it in a different way, and use templates, but that'd require the end-user to create the CharShift object, which is something we rarely do. Only our solid LED color plugin does that. So I don't think this is a good route.

Pushed it up anyway to showcase it, but it doesn't work in its current form. Compiles, but will definitely not work.

algernon · 2021-06-17T16:54:52Z

I wonder if we could cheat a bit, and use the props + typedef with static methods trickery we use for hardware devices and drivers... but that also sounds rather messy, and inflexible.

algernon · 2021-06-17T16:56:34Z

So... that leaves us with another option: CharShift, and EEPROMCharShift, the latter a class inheriting from the former, and simply overriding key methods. We'd need to make those key methods virtual, thus, use more memory, but perhaps we'll end up with less wasted memory than with function pointers.

gedankenexperimenter · 2021-06-18T00:32:54Z

If we go down this route, we'd need to do it in a different way, and use templates, but that'd require the end-user to create the CharShift object, which is something we rarely do. Only our solid LED color plugin does that. So I don't think this is a good route.

I really wish this wasn't true, almost as much as I wish Arduino didn't hide main() from the sketch. But we've got to play the hand we're dealt, so I can't really disagree.

gedankenexperimenter · 2021-06-18T00:39:24Z

I wonder if we could cheat a bit, and use the props + typedef with static methods trickery we use for hardware devices and drivers... but that also sounds rather messy, and inflexible.

I'm definitely not a fan of that approach. I'm filled with dread every time I have reason to look at the hardware code—I invariably search in several wrong places before I find what I'm looking for, and it's nigh impossible to picture how the whole system fits together.

algernon · 2022-07-22T16:42:56Z

I'm going to revisit this soon, in a large part because I ended up playing with a layout that could really benefit from CharShift, and I'd love to have Chrysalis support it too.

There will be a few changes once I get around to update the plugin, based on lessons learned from DynamicMacros. First of all, I changed my stance on extend vs override. I originally said that I'd like to support in-flash & EEPROM pairs at the same time - I no longer think that makes sense. If we have EEPROM pairs, they should override the built-in ones. We could copy the built-in ones, though, assuming the slice is empty, maybe.

algernon added 2 commits June 17, 2021 02:06

CharShift: WIP CharShiftConfig

7d6ce8b

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

algernon added enhancement New feature or request plugin Issues related to otherwise unlisted plugins labels Jun 17, 2021

algernon requested a review from gedankenexperimenter June 17, 2021 00:38

gedankenexperimenter reviewed Jun 17, 2021

View reviewed changes

algernon added 2 commits June 17, 2021 18:42

CharShift: Migrate to a pluggable storage sub-class

ee36ed6

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

wip: focus fix

0cc060e

Signed-off-by: Gergely Nagy <algernon@keyboard.io>

algernon self-assigned this Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CharShift: Configuration plugin #1069

CharShift: Configuration plugin #1069

algernon commented Jun 17, 2021

gedankenexperimenter Jun 17, 2021

algernon Jun 17, 2021

gedankenexperimenter Jun 17, 2021

algernon Jun 17, 2021

gedankenexperimenter left a comment

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

gedankenexperimenter commented Jun 17, 2021

gedankenexperimenter commented Jun 17, 2021

gedankenexperimenter commented Jun 17, 2021

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

gedankenexperimenter commented Jun 18, 2021

gedankenexperimenter commented Jun 18, 2021 •

edited

Loading

algernon commented Jul 22, 2022

		CharShift::GetNumKeyPairsFunction CharShift::numKeyPairs_ =
		CharShift::numProgmemKeyPairs;

CharShift: Configuration plugin #1069

Are you sure you want to change the base?

CharShift: Configuration plugin #1069

Conversation

algernon commented Jun 17, 2021

gedankenexperimenter Jun 17, 2021

Choose a reason for hiding this comment

algernon Jun 17, 2021

Choose a reason for hiding this comment

gedankenexperimenter Jun 17, 2021

Choose a reason for hiding this comment

algernon Jun 17, 2021

Choose a reason for hiding this comment

gedankenexperimenter left a comment

Choose a reason for hiding this comment

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

gedankenexperimenter commented Jun 17, 2021

gedankenexperimenter commented Jun 17, 2021

gedankenexperimenter commented Jun 17, 2021

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

algernon commented Jun 17, 2021

gedankenexperimenter commented Jun 18, 2021

gedankenexperimenter commented Jun 18, 2021 • edited Loading

algernon commented Jul 22, 2022

gedankenexperimenter commented Jun 18, 2021 •

edited

Loading