Fix the layer-dependent modifiers handling

[vifon]

Closes #181.

I'm aware the function naming could be better. I'm open to suggestions.

[DidierLoiseau]

This fix quite deeply modifies the implementation of key releases, I wonder if it is really safe. It does not only affect the modifiers but actually all the keys are affected.

In fact, I think the current behaviour is really intentional as (default_)layer_state_set() already calls clear_keyboard_but_mods() such that modifiers aren't affected when switching layers.

Finally, action_t is 2 bytes so the declaration of this static array would consume 168 bytes (14×6×2) on an ErgoDox EZ. If I'm not mistaken this makes about 6.5% of the RAM of the Teensy 2 (2560 bytes).

I think this behaviour should thus at least be optional, independently of NO_ACTION_LAYER.

The documentation should also specify more clearly that (without this fix) the destination layer should contain the mods at the same place between layers – that is, if the user intends to switch layers while those mods are depressed.

[mecanogrh]

It's really an interesting patch that opens new possibilities, yes give it it proper symbol definition and not relate it to NO_ACTION_LAYER.

[vifon]

This fix quite deeply modifies the implementation of key releases, I wonder if it is really safe. It does not only affect the modifiers but actually all the keys are affected.

That's true and I'm eager to hear about the potential issues. I was using it for a few days and so far so good but we all know how these things tend to be.

In fact, I think the current behaviour is really intentional as (default_)layer_state_set() already calls clear_keyboard_but_mods() such that modifiers aren't affected when switching layers.

Kind of. Calling clear_keyboard_but_mods() prevents most keys from being stuck while still allowing to combine the modifiers and keys from different layers. I think this specific behavior is a bug.

Finally, action_t is 2 bytes so the declaration of this static array would consume 240 bytes (14×6×2) on an ErgoDox EZ. If I'm not mistaken this makes almost 10% of the RAM of the Teensy 2 (2560 bytes).

I'm aware of it. I considered storing a top-most layer number (uint8_t) for each key (taking the transparent keys into account of course) but I wanted to keep it simple for now. If the patch makes sense, I'll try to optimize it for memory.

I think this behaviour should thus at least be optional, independently of NO_ACTION_LAYER.

The documentation should also specify more clearly that (without this fix) the destination layer should contain the mods at the same place between layers – that is, if the user intends to switch layers while those mods are depressed.

Makes sense. Should I do these things or leave it for someone knowing the project's conventions?

[vifon]

I've just noticed another benefit of my patch. Until now, when I used a DF() key placed on a layer to change the default layer, I needed to enable a layer it was on, press it, release it and then disable the layer. I could not release the layer before releasing the DF() key. Now the order of releasing the keys doesn't matter.

[DidierLoiseau]

Indeed it would simplify the definition of layers as one wouldn't need to care about having the same key on the destination layer. Issues like #161 wouldn't occur any more.

I think the need for this feature is really keymap-specific, but it would require #90 or #151 to enable it per keymap.

[ezuk]

As for updating the docs: Please just go ahead and update the main readme.md at the root directory as part of this PR. I may suggest tweaks but obviously since this is your code you know exactly the intent behind it.

Reading this from the standpoint of a user, I'll try to summarize the rationale, please tell me if I got it. This patch addresses the following scenario:

1. Layer 0 has a key defined as Shift.
2. The very same key on layer 1 is defined, as, say, the letter R or something.
3. I press this key while on Layer 0.
4. While keeping it pressed, I switch to layer 1 for some reason.
5. I am now screwed. :)

Is this correct? With this patch, it would "remember" that I started out with Shift in that key, and treat the keyup event as "un-shift" in this case?

If so, I am not sure I understand the side effects we should watch out for. @DidierLoiseau, any chance you could explain a bit more? What's the danger in this change, where would it be surprising?

[vifon]

@ezuk Almost correct. This scenario works correctly if you release the layer modifier before releasing your Shift/R. If you release Shift/R before returning to Layer 0, well, then it becomes problematic (until you press and release Shift again).

I've just modified readme. I've also added an optional macro enabling this feature.

@Eric-L-T It depends on the user. For example it really bothered me and using such constructs for each key is quite syntax-heavy. Is it ok as an optional feature?

[vifon]

@Eric-L-T I read a bit about the weak mods and tried your solution. Unfortunately it does not achieve the same result. My solution allows to hold a modifier, switch layer and press some key affected by the said modifier. Weak mods are cleared on layer switch.

I've also encountered my first problem with my patch. I have a layer activated by holding two modifiers (link) and two macros allowing me to release either one and end up in one of the other layers (link). It no longer works because I am now releasing the original key. Any ideas for improvement and/or workaround?

[DidierLoiseau]

@vifon this is indeed the kind of issue I was thinking of. I intend to implement a similar behavior in my keymap but I wasn't sure exactly how this patch would affect it.

The "workaround" I see would be to implement this kind of layer switching with macros only, and test in the macros which layers are active. But I find it a bit cumbersome…

[vifon]

@DidierLoiseau I managed to solve it like this: vifon@3c04721

[DidierLoiseau]

@vifon I see. What's a bit hidden now is that when you are in _DL, the macro called by releasing one of the 2 keys you are holding (to return to either _LW or _RS) depends on the first layer you enabled (i.e. the first of the 2 keys you pressed).

BTW in which case are you switching layouts while holding modifiers?

If I had to cast a vote, I think this PR should be merged now that 8d55a12 has made it optional :-)

[vifon]

@DidierLoiseau Yes, the macro on release depends on the order of enabled layers but both release actions are essentially the same. The state is consistent.

I switch layers for example in the following scenario. On my keyboard layout (the software one in the OS) I input umlauts by pressing AltGr+[ and then a letter, for instance o. My AltGr is on the layer 0 but [ is on _RS layer. I need to press AltGr, switch layers, press [ and release the keys, hopefully in the correct order. There are many more combinations like that for example with my right Ctrl. I'm using GNU Emacs so there are many strange key combinations.

[vifon]

I've added a new process_action variant that allows to bypass the new pressed key caching mechanism. I'm not sure if it would be desirable to add it to this PR so it's on a separate branch for now: link. I found it useful in my keymacro recording macro (sic!): link

Without it the release events were never intercepted by my recording keymap and were not recorded properly.

[ezuk]

I'm not actually sure where this PR stands. @vifon, @jackhumbert, @DidierLoiseau - any thoughts? Are we going to merge this?

[vifon]

I still want to optimize the used memory, together with @Eric-L-T, but the functionality shouldn't change. I have some additional changes on my modifier-release-fix-2 branch. Should I merge them into this PR?

[ezuk]

I think that'd be good, yes. Let's consolidate all related changes so we can discuss and review.

As for the memory considerations: Assuming we're not hitting the Teensy's memory limit, what's the implication here? I mean, of course it's great to conserve memory, but:

1. If I don't use this feature, will it still eat up memory?

2. If I do use it (knowing that it eats up memory), will it limit functionality in any tangible way? Will I run into a wall because of this?

Asking dumb questions so I can really understand what's at stake here -- whether we're talking aesthetics (which are important) or functionality (which is crucial obviously).

[vifon]

I've added the rest of the commits.

ad. 1) No, it's enabled in compile-time so it will change nothing unless enabled.

ad. 2) If user's other features use considerable amount of RAM, they may have not enough after enabling this feature, that's all. For example my macro recording is quite memory-hungry so it was a concern for me. Anyway, the memory consumption is constant per keyboard (2 bytes per physical key).

[ezuk]

Given your answers to 1 and 2, I'd say from a pragmatic standpoint memory consumption is not a deal breaker here. We need to be sure to note in the docs that this is a memory-hungry feature, and perhaps optimize in time -- but it's nice that it simplifies layer management.

@DidierLoiseau what are your thoughts? And @jackhumbert?

[vifon]

@ezuk I think it's ready. @eltang managed to reduce the memory consumption down to about 30% of the original value -- about 5 bits per key. Seems to be working ok for me.

[ezuk]

@vifon nice! Can't merge due to conflicts though, could you fix? And then let's ask @jackhumbert for feedback, and maybe @DidierLoiseau too

[vifon]

I've resolved the conflicts.

@jackhumbert, @DidierLoiseau, could you take a look?

[DidierLoiseau]

I think this is a nice improvement and having it optional makes sure it won't impact anyone who doesn't use it.

However I am not really convinced by the last optimisation that reduces the memory consumption to 5 bits per key (instead of 1 byte). The code has become hard to understand and maintain and I think the previous optimisation to only store the layer was already sufficient (down to 3.3% of the memory for an Ergodox).

I think if we wanted to improve the memory consumption further, it would be better to switch to a simple array storing (row, col, layer) structs, which would fit on 2 or 3 bytes per key pressed.

p.s.: I just figured I had overestimated the memory consumption in my first comment: it was actually around 6.6% instead of 10% for an Ergodox.

[jackhumbert]

I agree with @DidierLoiseau, and this sounds cool! Let's get this merged :)

[vifon]

However I am not really convinced by the last optimisation that reduces the memory consumption to 5 bits per key (instead of 1 byte). The code has become hard to understand and maintain and I think the previous optimisation to only store the layer was already sufficient (down to 3.3% of the memory for an Ergodox).

Just to be clear: It still stores only the layer number. The layer number is <32, so we need only 5 bits to store it. Conceptually it still does the same thing, only the encoding differs.

In case you want to revert to the "1 byte per key" version, revert 8ef14d0, 4dce725 and 97cc44e. That should be enough.

I've got my doubts about the maintainability of this version too, though I cannot deny its efficiency.

I think if we wanted to improve the memory consumption further, it would be better to switch to a simple array storing (row, col, layer) structs, which would fit on 2 or 3 bytes per key pressed.

I don't get how it would be more memory efficient. Can you elaborate?

@jackhumbert Is there anything else I need to do before you merge it?

[vifon]

I've refactored the source layer cache code a bit and moved it to action_layer.c.

[DidierLoiseau]

I think if we wanted to improve the memory consumption further, it would be better to switch to a simple array storing (row, col, layer) structs, which would fit on 2 or 3 bytes per key pressed.

I don't get how it would be more memory efficient. Can you elaborate?

@vifon We could define a struct like

typedef struct {
  uint8_t row :5;
  uint8_t col :5;
  uint8_t layer :5;
  uint8_t active :1;
} pressed_key_t;

and store the pressed keys in an array defined as

pressed_key_t pressed_keys[KEYBOARD_REPORT_SIZE];

(this could have preprocessor conditions to choose the appropriate size of each field)

When a key is pressed, record the pressed key in the first entry that has active set to false. When releasing a key, find the entry with the matching row/col to extract the layer and set its active flag to false to allow reusing the entry.

I used KEYBOARD_REPORT_SIZE above as I think it is the maximum number of simultaneously pressed keys that the driver supports. With NKRO it is 16, otherwise 8. As pressed_key_t is 2 bytes, the whole array is thus 32 bytes (with NKRO) for any keyboard, and it works as long as the dimensions of the keyboard are less than 32×32 (that would already be a huge keyboard!).

NB.: I don't pretend to be a C expert so don't hesitate to correct me if I made mistakes.

[vifon]

@DidierLoiseau NKRO allows only 16 keys? I didn't know that... Well, in that case it should be possible.