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

[Core] Add Chordal Hold, an "opposite hands rule" tap-hold option similar to Achordion, Bilateral Combinations. #24560

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

getreuer
Copy link
Contributor

@getreuer getreuer commented Nov 3, 2024

This PR adds "Chordal Hold," a tap-hold option implementing the "opposite hands" rule similar to Achordion and Bilateral Combinations. Chordal Hold may be used out of the box without any configuration, or if desired, Achordion-like per-chord configuration is supported.

Description

Suppose tap-hold key is pressed and then, before the tapping term, another key is pressed. With Chordal Hold, the tap-hold key is settled as tapped if the two keys are on the same hand. This behavior may be useful to avoid accidental modifier activation with mod-taps, particularly in rolled keypresses when using home row mods.

In the case that the keys are on opposite hands, Chordal Hold alone does not yet settle the tap-hold key. Chordal Hold may be used in combination with Hold On Other Key Press or Permissive Hold to determine the behavior. With Hold On Other Key Press, an opposite hands chord is settled immediately as held. Or with Permissive Hold, an opposite hands chord is settled as held provided the other key is pressed and released (nested press) before releasing the tap-hold key.

Further notes:

  • Chordal Hold has no effect after the tapping term.

  • Chordal Hold has no effect when the other key is also a tap-hold key or when combos are involved.

History

Previously, "opposite hands" rule behavior like this has been implemented in

This is a highly desired feature. Judging from Reddit, Achordion is my most popular QMK library. Still so far, "opposite hands" behavior like this is not yet in QMK core. I've been reluctant to touch action_tapping.c, it is an intimidating piece of code, but I think it's important to figure this out and make it happen!

Comparison to Achordion and Bilateral Combinations

  • Achordion and Bilateral Combinations operate after action_tapping, by manipulating hold events after QMK core has settled them. This means there are two stages of event buffering, first in core, then second in Achordion/Bilateral Combinations.

  • Chordal Hold operates within action_tapping itself. This is advantageous since one stage of event buffering rather than two can in some cases complete more quickly for reduced input lag, and it is (arguably) conceptually simpler.

How to use Chordal Hold

Chordal Hold is intended to be used together with either Permissive Hold or Hold On Other Keypress. Enable one of them, and enable Chordal Hold by adding in config.h:

#define CHORDAL_HOLD

For an Achordion-like experience, I suggest enabling Permissive Hold and setting the tapping term rather high, say, 250 ms. With Chordal Hold + Permissive Hold, keys usually settle before the tapping term.

"Handedness"

Determining whether keys are on the same or opposite hands involves defining the "handedness" of each key position. By default, if nothing is specified handedness is guessed based on keyboard matrix dimensions. If this is inaccurate, handedness may be specified in several ways. The easiest way to specify handedness is by chordal_hold_layout. Define in config.h:

#define CHORDAL_HOLD_LAYOUT

Then in keymap.c, define chordal_hold_layout in the following form:

const char chordal_hold_layout[MATRIX_ROWS][MATRIX_COLS] PROGMEM =
    LAYOUT(
        'L', 'L', 'L', 'L', 'L', 'L',  'R', 'R', 'R', 'R', 'R', 'R', 
        'L', 'L', 'L', 'L', 'L', 'L',  'R', 'R', 'R', 'R', 'R', 'R', 
        'L', 'L', 'L', 'L', 'L', 'L',  'R', 'R', 'R', 'R', 'R', 'R', 
                       'L', 'L', 'L',  'R', 'R', 'R'
    );

Use the same LAYOUT macro as used to define your keymap layers. Each entry is a character, either 'L' for left, 'R' for right, or '*' to exempt keys from the "opposite hands rule." When a key has '*' handedness, pressing it with either hand results in a hold. This could be used perhaps on thumb keys or other places where you want to allow same-hand chords.

chordal_hold_layout is inspired by the convenient "BILATERAL_COMBINATIONS_HANDS_LAYER" in Bilateral Combinations. But with the differences that chordal_hold_layout is defined as a separate array outside the keymap and in char datatype to halve the flash space cost.

Advanced configuration

Per-chord tuning of the behavior is possible with the get_chordal_hold() callback. This parallels Achordion's achordion_chord() callback. Returning true from this callback indicates that the chord may be settled as held, while returning false immediately settles the chord as tapped. Example:

bool get_chordal_hold(uint16_t tap_hold_keycode, keyrecord_t* tap_hold_record,
                      uint16_t other_keycode, keyrecord_t* other_record) {
    // Exceptionally allow some one-handed chords for hotkeys.
    switch (tap_hold_keycode) {
        case LCTL_T(KC_A):
           if (other_keycode == KC_C || other_keycode == KC_V) {
               return true;
           }
           break;
        case RCTL_T(KC_SCLN):
           if (other_keycode == KC_N) {
               return true;
           }
           break;
    }
    // Otherwise defer to the opposite hands rule.
    return get_chordal_hold_default(tap_hold_record, other_record);
}

Please see the addition to docs/tap_hold.md for further documentation.

Implementation

  • The meat of Chordal Hold's implementation is in action_tapping.c. Please review this first.
  • I include unit tests that check that Chordal Hold operates as intended when used together with either Permissive Hold or Hold On Other Key Press.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Then in keymap.c, define `chordal_hold_layout` in the following form:

```c
const char chordal_hold_layout[MATRIX_ROWS][MATRIX_COLS] PROGMEM =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something that could probably be generated automatically based on the matrix config, for most of the dual controller splits. And doing so may be a good idea. It can be set to be weak so users can override it, if need be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent idea! Handedness could be inferred from the .x, .y fields in the layouts array in keyboard.json, something like that? I also like that idea to make such a default chordal_hold_layout as a weak symbol for easy overriding.

It would be fantastic if handedness could be correctly defined in more cases out of the box. As it is, if no configuration is made, handedness is assumed according to this fallback heuristic based on matrix dimensions:

#if defined(SPLIT_KEYBOARD) || ((MATRIX_ROWS) > (MATRIX_COLS))
// If the keyboard is split or if MATRIX_ROWS > MATRIX_COLS, assume that the
// first half of the rows are left and the latter half are right.
return (key.row < (MATRIX_ROWS) / 2) ? /*left*/ 'L' : /*right*/ 'R';
#else
// Otherwise, assume the first half of the cols are left, others are right.
return (key.col < (MATRIX_COLS) / 2) ? /*left*/ 'L' : /*right*/ 'R';
#endif

This simple definition works as expected for my split keyboards. But surely this is not enough in all cases, since there are keyboards with weird matrix circuits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm partway on implementing this idea. Here's an update on what I've fleshed out so far...

I've found that the LAYOUT macro is generated from keyboard.json by lib/python/qmk/cli/generate/keyboard_h.py. With some simple logic, I can test for the easy case that the keyboard is horizontally symmetric and generate the handedness by checking whether each key's .x field lies left or right of the midline. That handles many split keyboards (but not all, e.g. not Charybdis) and some Planck-like keyboards.

In order to make this chordal_hold_layout as a weak symbol variable, I think I need to prepend __attribute__((weak)) to the beginning of the line, and I believe it's necessary to put the definition in its own .c file (or can a weak variable be overridden by a strong definition in the same translation unit?). I'm planning to do parallel to what you did for Autocorrect, making a new cli tool to generate the code and adding a build rule to invoke it.

An example output for the Moonlander:

const char chordal_hold_layout[MATRIX_ROWS][MATRIX_COLS] PROGMEM = LAYOUT(
    'L', 'L', 'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'R', 'R', 'R'
);

This output is as intended, but the formatting is ugly. If by hand, I would make it like this:

const char chordal_hold_layout[MATRIX_ROWS][MATRIX_COLS] PROGMEM = LAYOUT(
    'L', 'L', 'L', 'L', 'L', 'L', 'L',   'R', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L', 'L',   'R', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L', 'L',   'R', 'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L', 'L',             'R', 'R', 'R', 'R', 'R', 'R',
    'L', 'L', 'L', 'L', 'L',      'L',   'R',      'R', 'R', 'R', 'R', 'R',
                        'L', 'L', 'L',   'R', 'R', 'R'
);

I suppose formatting doesn't matter, if this generated code is only an intermediate of the build system and not seen by humans. Or would pretty formatting be worthwhile? Is there an easy way to do it? Thanks again for the idea.

docs/tap_hold.md Outdated

* Chordal Hold has no effect after the tapping term.

* Chordal Hold has no effect when the other key is also a tap-hold key. This is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chordal Hold has no effect when the other key is also a tap-hold key.

Is this a limitation of the implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Great comment. I was originally thinking multiple simultaneous tap-hold keys is out of scope of what Chordal Hold needs to consider, since in an input sequence like "A↓, B↓" where A and B are tap-hold keys, A and B can't be settled yet.

But rethinking it, these keys will eventually settle, of course, depending on subsequent events. I've revised so that Chordal Hold considers input sequences involving multiple tap-hold keys and I'm happy how this is working out.

Details:

  1. Consider an input sequence "A↓, B↓, C↓, ..." of all presses of tap-hold keys. If held until the tapping-term, then regardless of handedness, these keys should be settled as held as usual. It is important to preserve this behavior for home row mods so that it is possible to chord multiple mods, e.g. Ctrl + Shift, in one hand.

  2. Another case: consider if within the tapping term a key is released: "A↓, B↓, C↓, C↑". Provided either Permissive Hold or Hold on Other Key Press is enabled, the tap-hold keys A, B preceding C would then be settled as held. With Chordal Hold, perhaps depending on handedness some or all of the keys should be tapped instead.

    I've implemented a general heuristic (waiting_buffer_find_chordal_hold_tap() in the code) to decide this, based on evaluating get_chordal_hold() between successive pairs of keys. Some specific practical cases described for 3-key sequences, though the rule works for any number of keys:

    • If A, B, C are all on the same hand (get_chordal_hold() evaluates false between AB and BC), they are all considered tapped. This is effectively "typing streak" suppression of the hold function. This is cool!

    • If A and B are on one hand and C on the other, then both A and B are held.

    • If A is on one hand and B and C on the other, then just A is held.

  3. Suppose an input sequence of tap-hold presses followed by a press of a regular key Z, like "A↓, B↓, C↓, ..., Z↓". Then, all the same logic as in case 2 applies.

The implementation is more invasive than where this PR started, which makes me nervous. The "state machine" is not easy to reason about or modify, though I'm starting to get a feel for it.

I think you understand this area of the code a lot better than I do. Please let me know whether what I describe here needs further explanation, or seems like a flawed design and/or could be implemented better. I've made an effort to make it clean, been testing Chordal Hold in daily use (typing with it as we speak), and significantly beefed up the unit tests. But surely there's room for improvement. Anyway, I'm excited how this is progressing! 🔥

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading and changing the action_tapping.c file after a long hiatus is non-trivial (to say the least) and I'm trying to get up to speed again to evaluate this PR.

My user space implementation is similar to ZMK's "positional hold-tap" when the following are pressed sequentially within tapping term on the right side of a QWERTY layout:

  1. Press RALT_T(KC_L)
  2. Press RGUI_T(KC_K)
  3. Continue holding both keys down

The host received the following:

KEY-DOWN - QMK: KC_L    Event key: l           Code: KeyL          KeyCode: 76
KEY-DOWN - QMK: KC_RGUI Event key: Meta        Code: MetaRight     KeyCode: 93

But Chordal implementation does not override the first key and will transmit both modifiers:

KEY-DOWN - QMK: KC_RALT Event key: Alt         Code: AltRight      KeyCode: 18
KEY-DOWN - QMK: KC_RGUI Event key: Meta        Code: MetaRight     KeyCode: 93

Will review the changes in more detail to figure this out.

I haven't considered the scenario of more than 2 simultaneous key presses, and it seems that specific use cases will have nuances. This PR might benefit from more end user tests to ensure robustness because support of this feature will land on the Discord server when merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comparison! Yes, that is intended that both mods be held. I just added a unit test to verify this case. This behavior is useful so that one hand can hold Alt + GUI, or some other set of mods, while the other hand types a hotkey or uses the mouse.

OTOH, I can imagine there are other ways to handle those use cases. When using behavior as in your first output, what is a good solution for sending hotkeys like Ctrl+Shift+V? Do you find such multi-mod hotkeys are rare enough in practice that it's not a big deal? or maybe switch over to a Callum-style mod scheme for such things, or something else?

Copy link
Contributor

@filterpaper filterpaper Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your solution includes the following user function:

bool get_chordal_hold(uint16_t tap_hold_keycode, keyrecord_t* tap_hold_record,
                      uint16_t other_keycode, keyrecord_t* other_record)

In the event of "Ctrl+Shift+V" or any other (frequent) same hand modifier combination, users can use that function to enable specific same hand modifier activation. Layouts such as Colemak that places a lot of frequently used letters on home row and may benefit from a default same-hand tap as default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants