Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wip] A new key grouping based approach to flexible layer selection #594

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noseglasses
Copy link
Collaborator

@noseglasses noseglasses commented Mar 5, 2019

This is a draft PR. It is not meant to be merged in its original version.

Although everything works, there are two plugins where i am not 100% sure how the proposed changes will affect the behavior.

I left TODO comments where I need help/suggestions from the main developers about if there are issues to be expected and how to address these.

A New Key Grouping Based Approach to Flexible Layer Selection

Abstract

This PR enables logically grouping keys and apply layer switch and toggle operation to selected key groups only. Examples are provide that demonstrate grouping keys by keyboard hand sides, via condition expressions and with keymap-style color arrays.

Introduction

The basic idea that led to this PR was the question: How to switch one half of the keyboard to another layer while the other half should stay at the base layer. My intention was to switch via the thumb key of the Model01 one half of the keyboard to a modifier layer where all modifiers are supposed to reside on the home row. The other half of the keyboard should stay at the base layer. By means of that I intent to make typing modifier shortcuts with combined modifiers and keys more convenient.

Of coures, this could already be done with the existing Kaleidoscope, e.g. by defining two individual additional layers whose keys for one half match those of the base layer and the other half the modifier layer. The redundancy is obvious.

That's how the idea emerged to modity the layer class in a way that it stores not only one pair of layer_state_ and top_active_layer_ but several, each pair of those variables for a separate region of the keyboard. This makes it possible to make separate regions of the keyboard behave as individual keyboards in terms of their layer state. Lacking a better name, I called those keys assigned to such a region a "key group".

Keys can be arbitrarily assigned to key groups. Layer switches/toggles can be defined to affect one or more key groups (see examples).

By default only two key groups are defined. One for the left and one for the right hand side of the keyboard.

Changes

  • added header src/Kaleidoscope-KeyGroups.h
  • hardwares implement static constexpr bool isOnLeftHalf(uint8_t row, uint8_t col) convenience methods to test for the hand a key belongs to (one-handed keybords could just unconditionally return true)
  • added header key_groups.h
  • layers class tracks state of up to six key groups now
  • all public methods that affect the layer states now can be passed a key group flags to name the affected layers
  • method extern uint8_t groupOfKey(uint8_t row, uint8_t col) introduced that can be overridden from within the sketch
  • fixed Colormap plugin to work with multiple key groups
  • fixed plugin LED-ActiveLayerColor to work with multiple key groups
  • fixed plugin LED-ActiveModColor to work with multiple key groups
  • added TODO-comments in contexts (plugins EEPROM-KeymapOProgrmmer and NumPad) where layer-methods are used but no layer group information is available
  • added TODO-comments about the implementation of method isOnLeftHalf in some keyboards
  • examples were added to illustrate different use cases

Compatibility

The PR will not break any user code or plugins. Only if keygroups are assigned to layer switch operations, the firmware behavior will differ from what it does currently.

Performance

Changes will probably not noticeabley affect runtime performance.

Resource usage

Stock firmware: Groth of program size from 25192 to 26992 bytes. Data usage grows from 1414 to 1459 bytes.

TODO before this can be merged

Carefully check commit line changes containing TODOs and fix those. Especially concerning plugins EEPROM-KeymapOProgrmmer and NumPad.

Conclusion

This PR comes with quite some changes and both program size and Data usage grow significantly. Nonetheless, this feature might be worth it.

If Arduino eventually would introduce a global per-sketch configuration header (there's an ongoing discussion about this in the Arduino community) we could support several alternative implementations of the layer system, let the user select one and select a default one otherwise.

As this is currently not possible, I chose to "upgrade" the existing layer class.

@noseglasses noseglasses force-pushed the ng_key_grouping_flexible_layer_selection branch from 1c399ed to 2c33904 Compare March 5, 2019 10:42
@algernon
Copy link
Contributor

algernon commented Mar 5, 2019

If Arduino eventually would introduce a global per-sketch configuration header (there's an ongoing discussion about this in the Arduino community) we could support several alternative implementations of the layer system, let the user select one and select a default one otherwise.

We can make the layer system pluggable. It's something I've done before as an experiment, but it wasn't ready at the time, and I couldn't figure out a nice API. I'm pretty sure we can do better now, and come up with a system that lets one have alternate layer implementations. (In the long run, I'd love if the layer system would be Just Another Plugin, and with the eventhandler APIs, this should be a whole lot easier than before)

I didn't look at the PR yet, but the general idea is interesting. I wouldn't replace the current layer system, but once we can have alternate implementations - this is definitely something we should have.

One thing I'm unsure about: how to present this to Chrysalis?

@noseglasses
Copy link
Collaborator Author

noseglasses commented Mar 5, 2019

One thing I'm unsure about: how to present this to Chrysalis?

i didn't find time to checkout Chrysalis, yet. That's why I cannot really provide an reliable answer. But the following explanations might help you to get an dea about the integrateability in Chrysalis.

Key groups come into play by adding group tags to the layer toggle/shift keycodes when they are
defined in the keymap, e.g.

    KEYMAP_STACKED(
        Key_A, ...., KeyGroup(KEY_GROUP_1 | KEY_GROUP_3, ShiftToLayer(Function)), ...
    )

In this example only keys of groups 1 and 3 are supposed to shift to the specified layer. This would require Chrysalis to add an additional property to a keycode (a flag array with six flags - each of which represents a group - all flags 0 or all 1 means all groups affected).

The user can (optionally) define a callback function that decides on a key's group membership. If no such function is specified, a weak symbol, the default is used that groups keys by keyboard hand sides. Those callbacks are probably similar to macro callbacks in the sketch.

But there's another way to define key group membership that's very well suited for being used with Chrysalis. It uses an array similar to that defined by KEYMAP_STACKED (actually I rely on KEYMAP_STACKED wrapped up in a macro). Every entry in this array is the uint8_t id of the key's group. Chrysalis could easily generate this array just as it generates the keymap. A synthesized key group membership callback would then operate on this id array.

There's an example coming with this PR that demonstrates the latter approach.

@obra obra changed the title A new key grouping based approach to flexible layer selection [wip] A new key grouping based approach to flexible layer selection Apr 4, 2019
@noseglasses noseglasses force-pushed the ng_key_grouping_flexible_layer_selection branch from 2c33904 to f5e6962 Compare April 30, 2019 14:51
@noseglasses
Copy link
Collaborator Author

Rebased and force-pushed to consider changes in current master.

@noseglasses noseglasses force-pushed the ng_key_grouping_flexible_layer_selection branch from f5e6962 to 1ac7503 Compare April 30, 2019 15:48
@noseglasses noseglasses force-pushed the ng_key_grouping_flexible_layer_selection branch from 1ac7503 to f573fa7 Compare May 16, 2019 07:40
@noseglasses
Copy link
Collaborator Author

Resolved conflicts with current master. Rebased and force-pushed.

…ggle operation to selected key groups only. Examples are provide that demonstrate grouping keys by keyboard hand sides, via condition expressions and with keymap-style color arrays.

The basic idea that led to this PR was the question: How to switch one half of the keyboard to another layer while the other half should stay at the base layer. My intention was to switch via the thumb key of the Model01 one half of the keyboard to a modifier layer where all modifiers are supposed to reside on the home row. The other half of the keyboard should stay at the base layer. By means of that I intent to make typing modifier shortcuts with combined modifiers and keys more convenient.

Of coures, this could already be done with the existing Kaleidoscope, e.g. by defining two individual additional layers whose keys for one half match those of the base layer and the other half the modifier layer. The redundancy is obvious.

That's how the idea emerged to modity the layer class in a way that it stores not only one pair of `layer_state_` and `top_active_layer_` but several, each pair of those variables for a separate region of the keyboard. This makes it possible to make separate regions of the keyboard behave as individual keyboards in terms of their layer state. Lacking a better name, I called those keys assigned to such a region a "key group".

Keys can be arbitrarily assigned to key groups. Layer switches/toggles can be defined to affect one or more key groups (see examples).

By default only two key groups are defined. One for the left and one for the right hand side of the keyboard.

* added header `src/Kaleidoscope-KeyGroups.h`
* hardwares implement static constexpr bool isOnLeftHalf(uint8_t row, uint8_t col) convenience methods to test for the hand a key belongs to (one-handed keybords could just unconditionally return true)
* added header `key_groups.h`
* layers class tracks state of up to six key groups now
* all public methods that affect the layer states now can be passed a key group flags to name the affected layers
* method `extern uint8_t groupOfKey(uint8_t row, uint8_t col)` introduced that can be overridden from within the sketch
* fixed Colormap plugin to work with multiple key groups
* fixed plugin LED-ActiveLayerColor to work with multiple key groups
* fixed plugin LED-ActiveModColor to work with multiple key groups
* added TODO-comments in contexts (plugins EEPROM-KeymapOProgrmmer and NumPad) where layer-methods are used but no layer group information is available
* added TODO-comments about the implementation of method `isOnLeftHalf` in some keyboards
* examples were added to illustrate different use cases

The PR will not break any user code or plugins. Only if keygroups are assigned to layer switch operations, the firmware behavior will differ from what it does currently.

Changes will probably not noticeabley affect runtime performance.

Stock firmware: Groth of program size from 25192 to 26992 bytes. Data usage grows from 1414 to 1459 bytes.

Carefully check commit line changes containing TODOs and fix those. Especially concerning plugins EEPROM-KeymapOProgrmmer and NumPad.

This PR comes with quite some changes and both program size and Data usage grow significantly. Nonetheless, this feature might be worth it.

If Arduino eventually would introduce a global per-sketch configuration header (there's an ongoing discussion about this in the Arduino community) we could support several alternative implementations of the layer system, let the user select one and select a default one otherwise.

As this is currently not possible, I chose to "upgrade" the existing layer class.

Signed-off-by: Florian Fleissner <[email protected]>
@noseglasses noseglasses force-pushed the ng_key_grouping_flexible_layer_selection branch from f573fa7 to dedc9e0 Compare December 5, 2019 10:01
@obra obra marked this pull request as draft December 21, 2020 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants