New Feature: required keys for tap-hold behaviors #835

jmding8 · 2021-06-12T16:25:29Z

Adds the ability to for users to configure hold-tap behaviors so that the only resolve to hold behaviors if specified keys are pressed. Let's say that we set up a keymap this way:

keymap {
        compatible = "zmk,keymap";
        base_layer {
            bindings = <&cmt LSFT Q    &kp W    &kp E>
...

Where cmt is defined:

behaviors {
        cmt: conditional_mod_tap {
			compatible = "zmk,behavior-hold-tap";
			label = "TAP_PREFERRED_MOD_TAP";
			#binding-cells = <2>;
			flavor = "tap-preferred";
			tapping_term_ms = <1>;
			bindings = <&kp>, <&kp>;
			required_keys_for_hold = <1>;
		};
    };

These are the expected behaviors:

key_0_down, key_1_down, key_1_up, key_0_up => "W"
Because the sequence is executed in longer than 1 ms, and key_1 is pressed when the hold/tap behavior is decided, the hold/tap behavior is resolved into a hold. This is the same behavior as would occur with the current logic as well.

0_down, 2_down, 2_up, 0_up => "qe"
Because the sequence is executed in longer than 1 ms, but key_1 is NOT pressed when the hold/tap behavior is decided, the hold/tap behavior is resolved into a tap. This differs from the behavior that would occur with the current logic, which would not have a requirement that key_1 be pressed to resolve into a hold behavior, and would therefore emit "E".

This is extremely useful for my situation:

I have replaced Caps Lock on my keymap with &lt NUM CLCK, such that the sequence <caps_down, j_down, j_up, caps_up> produces "4"
Due to the position of the Caps Lock, it is difficult to press and hold LSFT while also holding Caps Lock down, so it becomes difficult to produce "$"
To make it easier to apply a LSFT modifier while also holding Caps Lock down, I have replaced the A key on my keymap with &mt LSFT A
However, mod-tap behaviors can be difficult to habituate due to the tight timing constraints
So instead of using a raw mod-tap, I condition the mod-tap A so that it can only produce a HOLD behavior if and only if the Caps Lock key is depressed at the time that the hold-tap is decided.

okke-formsma · 2021-06-12T18:46:06Z

I like the general idea, but maybe we should change the meaning of the list from "required_keys_for_hold" to "hold-keys". Any combinations of a hold-tap + a key (position) from this list can trigger a hold, but any keys not in the list won't trigger the hold

That would enable us to do "left-side hold taps" and "right side hold taps", that would only trigger for keys on the opposite side of the board for example.

The current implementation requires all keys on the list to be pressed, for which I can't really find a usecase.

okke-formsma · 2021-06-12T18:46:52Z

app/dts/bindings/behaviors/zmk,behavior-hold-tap.yaml

@@ -30,3 +30,7 @@ properties:
      - "tap-preferred"
  retro-tap:
    type: boolean
+  required_keys_for_hold:


required-keys-for-hold

okke-formsma · 2021-06-12T18:50:32Z

app/src/behaviors/behavior_hold_tap.c

@@ -90,6 +92,8 @@ struct last_tapped {

 struct last_tapped last_tapped;

+bool is_key_held[ZMK_KEYMAP_LEN] = {0};


maybe rename to key_position_pressed?

how about is_key_position_pressed? I do like the "is_" prefix, IMO it helps imply bool

jmding8 · 2021-06-12T22:15:12Z

I played around with this more, and you're right, AND on all the keys in the list isn't as useful as OR. As for naming, hold-enabler-keys?

okke-formsma · 2021-06-13T07:55:38Z

Great!

I think there's a bug in the current implementation. If one of the 'hold-enabled keys' is pressed before the hold-tap, the hold will be triggered even though the actual key being pressed is not a hold-enabled key.

Let's say 1_down, 0_down, 2_down, 2_up, 0_up => "wE" instead of "wqe".

As a possible fix, we could move the is_key_held array to the active_hold_tap struct (or rather, this should be the first_key_pressed_position int now instead of an array because we're only interested in the first key that was pressed after the hold-tap was enabled).

Please write some test to cover the scenarios you described above, and this one.

jmding8 · 2021-06-13T15:08:32Z

The original intent was indeed to produce "wE", and not "wqe". It was intended to only consider the pressed state, and be agnostic to the order of pressing. BTW, this is why the variable was named hold_enableR_keys, rather than hold_enableD_keys

I think it might be better to be order agnostic when using conditional hold/tap for situations like home-row mods. For example

/ {
	behaviors {
		cht: conditional_hold_tap {
			compatible = "zmk,behavior-hold-tap";
			label = "cond_hold_tap";
			#binding-cells = <2>;
			flavor = "hold-preferred";
			tapping-term-ms = <10>;
			bindings = <&kp>, <&kp>;
			required_keys_for_hold = <0 1>;
		};
	};

	keymap {
		compatible = "zmk,keymap";
		label ="Default keymap";

		default_layer {
			bindings = <
				&cht LEFT_CONTROL Q    &cht LEFT_SHIFT W    & kp E
			>;
		};
	};
};

Then the sequence
0_down, 1_down, 2_down, 2_up, 0_up, 1_up => "Ctrl + Shift + e"
The 1_down resolves the 0 key into it's hold, Ctrl behavior. And because it is order agnostic, the 0_down also conditions the 1 key such that it is allowed to resolve into it's hold, Shift behavior.

Let me know what you think? These are tricky for me to reason about.

okke-formsma · 2021-09-09T18:24:49Z

A concern I have with the proposal as it stands now, is that there are two distinct scenarios that are covered. I'd like to keep the set of behaviors and options we have as orthogonal as possible and combinable (e.g., there should be one way to do something, and many 'simple' behaviors build up to a more complex whole)

Scenario A: conditional key first

a key (X) is pressed
a conditional hold-tap is pressed
the conditional hold-tap is influenced by key X

Scenario B: hold-tap first

a conditional hold-tap is pressed
a key (X) is pressed
the conditional hold-tap is influenced by key X

Scenario A could be covered by making X a layer key (if it needs to have other behaviors too that could be arranged with a macro-like behavior).

Scenario B, where it depends which key is pressed to decide on the hold-tap, would be something that we can not do yet. It's also a often requested feature, for hold-taps that only hold for the other side of a split board for example.

jmding8 · 2021-09-12T04:33:15Z

Updated such that keypresses are order dependent.
Now, the hold-behavior is only enabled when the first other-key to be pressed is one of the configured, enabler keys. If the enabler key is pressed before the hold-tap key, it "does not count" and the hold behavior is not enabled.

okke-formsma

in general, this looks good. I think we can simplify somewhat.

Haven't looked into the tests and documentation yet.

app/src/behaviors/behavior_hold_tap.c

jmding8 · 2021-09-12T23:03:13Z

thanks! Based on your feedback, I was able to find an implementation that doesn't require us to store anything additional in the active_hold_tap struct. Instead, it simply sets active_hold_tap.status to STATUS_TAP under the appropriate conditions.

jmding8 · 2021-09-13T00:38:27Z

Tests pass:

…the prev commit

okke-formsma · 2021-09-13T19:52:58Z

app/src/behaviors/behavior_hold_tap.c

-        decide_tap_preferred(hold_tap, decision_moment);
+    // For conditional hold-tap behaviors that have failed to meet the required prerequisites
+    // for a hold decision, force a tap decision.
+    if (decision_moment == HT_OTHER_KEY_DOWN && hold_tap->config->hold_enabler_keys_len > 0) {


I don't think this if statement is correct for all 'flavors'. For example, the balanced never decides on HT_OTHER_KEY_DOWN; it decides on HT_OTHER_KEY_UP. I think we should put this code into a function is_hold_enabled_key(struct active_hold_tap *hold_tap, int32 other_key_decision), and call it from each of the decide_* methods.

Somewhat tricky would be the behavior of tap-preferred. I think it should act like this:

if the timer has not run out (e.g., we have not seen HT_TIMER_EVENT, always trigger tap. (doesn't matter which key is pressed)

if the timer did run out (the HT_TIMER_EVENT was seen, maybe setting STATUS_UNDECIDED_TIMER_SEEN), trigger HOLD if the key is a hold_enabler_key, otherwise trigger TAP

For balanced, the decision_moment to check for hold_enabler_keys would be on HT_OTHER_KEY_UP.

you're right, I'll fix. I'll also make the testing more robust to catch this.

I think we'll still need to store position_of_first_other_key_pressed in the active_hold_tap struct, because in tap-preferred situation 4a, I don't see any other way of verifying whether the first other key that was pressed was an enabler key, especially if a second key is pressed before tapping term expires

jmding8 · 2021-09-14T05:51:57Z

rewrote the feature again. Did a couple things:

renamed it "positional hold-tap", to be more specific as to what conditions enable/disable
copy-pasted (most of) the tests for balanced twice, once positionally enabled, and once positionally disabled. If this approach looks good to you, I'll go ahead and do the same for the hold-preferred and tap-preferred tests as well.
rewrote the feature. Now it only overwrites hold behaviors at the time of decision (and prior to execution)

To do:

incorporate any feedback
add tests for hold and tap-preferred flavors
lint

Conditional -> positional in docs

petejohanson

Looks good! One minor grammar nitpick.

docs/docs/behaviors/hold-tap.md

app/src/behaviors/behavior_hold_tap.c

okke-formsma · 2021-10-29T14:54:14Z

docs/docs/behaviors/hold-tap.md

+
+* Positional hold-taps can be useful with home-row modifiers. By using a positional hold-tap behavior for home-row modifiers on the left hand, and setting `hold-trigger-key-positions` to the keys under the right hand, positional hold-tap will only allow hold behaviors to occur with cross-hand keypresses.
+
+* Note that for regular hold-tap behaviors a shorter `tapping-term` encourages hold decisions. However the opposite is true for positional hold-tap behaviors, where a shorter `tapping-term` actually encourages tap decisions. This is because when the `tapping-term` expires, this triggers the behavior to decide as either a tap or a hold. But if the user has not yet had time to press one of the `hold-trigger-key-positions`, then with positional hold-tap the behavior will decide as a tap.


Maybe, we should instead only check the postiion-hold-keys when the current status is STATUS_HOLD_INTERRUPT
which keeps te behavior more in line with the normal behavior of the different flavors

I think that makes a lot of sense!

hmm, actually, in situation 4b of tap-preferred, the hold/tap is decided into STATUS_HOLD_TIMER by HT_TIMER_EVENT. Let's say "J" is one of the hold-trigger-keys. In that situation, I think the most intuitive behavior would be to decide into a hold and not a tap no?

Yes.

Sooo, where does that lead us. it means we need to work on both STATUS_HOLD_TIMER and STATUS_HOLD_INTERRUPT, but we only want to trigger the TAP change if a key was pressed.

app/dts/bindings/behaviors/zmk,behavior-hold-tap.yaml

…rred check

…into conditional_mod_tap

okke-formsma · 2021-10-31T15:40:11Z

LGTM

@petejohanson ready from merge as far as I'm concerned.

jmding8 · 2021-10-31T22:07:57Z

~~not sure why integration_test are failing, they succeed on my machine?~~ disregard, fixed

petejohanson

Thanks, and thanks for the test coverage!

petejohanson requested a review from okke-formsma June 12, 2021 16:40

okke-formsma reviewed Jun 12, 2021

View reviewed changes

jmding8 force-pushed the conditional_mod_tap branch 3 times, most recently from d951396 to 538cf3c Compare June 13, 2021 01:23

Add required keys for tap-hold behaviors

acabc7a

jmding8 force-pushed the conditional_mod_tap branch from 538cf3c to acabc7a Compare June 13, 2021 01:25

Merge remote-tracking branch 'upstream/main' into conditional_mod_tap

c63cde8

refactor to be keypress order aware

537feac

jmding8 added 2 commits September 12, 2021 05:40

run clang lint

e7b0f36

add explanation to hold-tap documentation

4283538

okke-formsma suggested changes Sep 12, 2021

View reviewed changes

refactor to eliminate additional field in active_hold_tap struct

9a5d5d7

minor cleanup

6edeefa

jmding8 added 2 commits September 13, 2021 02:20

fix NULL typecheck and add documentation note about tapping-term

67db38e

add ntoe about tapping term to docs (that was mistakenly left out of …

3d9a72e

…the prev commit

okke-formsma reviewed Sep 13, 2021

View reviewed changes

jmding8 added 2 commits September 13, 2021 20:25

add more robust testing, balanced only for now

b491cb6

rename feature 'positional hold-tap', and rewrite (again)

b182e42

Update hold-tap.md

110285f

Conditional -> positional in docs

petejohanson requested changes Oct 29, 2021

View reviewed changes

docs/docs/behaviors/hold-tap.md Outdated Show resolved Hide resolved

okke-formsma reviewed Oct 29, 2021

View reviewed changes

app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved

okke-formsma reviewed Oct 29, 2021

View reviewed changes

app/src/behaviors/behavior_hold_tap.c Outdated Show resolved Hide resolved

okke-formsma reviewed Oct 29, 2021

View reviewed changes

jmding8 and others added 8 commits October 29, 2021 15:48

fix typoed etcetera

3099736

comments, no logic change

dbb31cb

fix docs

82504a6

refactor, tap-positionally-preferred

66531d9

prettier docs/behaviors/hold-tap.md

4c94647

tests

a26e001

re-order function definitions

45721af

clang

14f2b6e

jmding8 commented Oct 30, 2021

View reviewed changes

app/dts/bindings/behaviors/zmk,behavior-hold-tap.yaml Show resolved Hide resolved

jdingRoblox and others added 9 commits October 30, 2021 00:25

remove unnecessary if statement

63776e3

add flavor enum

01518d4

add compile-time hold_trigger_key_positions vs tap-positionally-prefe…

db71a8e

…rred check

Merge branch 'conditional_mod_tap' of https://github.com/jmding8/zmk …

bd8b98a

…into conditional_mod_tap

rename other_key_down_position in decide_tap_positionally_preferred)

26a6fbf

build failures because enum starts at 1?

caad69a

Merge branch 'upstream_main' into conditional_mod_tap

4729e92

revert refactor, simplify tests

f3da13f

remove unnecesary if statement, add new line at bottom

411f91d

jmding8 mentioned this pull request Oct 31, 2021

New feature: hold/tap flavor tap-unless-interrupted #1005

Closed

fix missing last lines

99724cb

petejohanson approved these changes Nov 1, 2021

View reviewed changes

petejohanson merged commit 19ee784 into zmkfirmware:main Nov 1, 2021

jmding8 mentioned this pull request Nov 6, 2021

New feature: hold/tap flavor tap-unless-interrupted (squashed) #1018

Merged

jmding8 deleted the conditional_mod_tap branch January 9, 2022 20:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Feature: required keys for tap-hold behaviors #835

New Feature: required keys for tap-hold behaviors #835

jmding8 commented Jun 12, 2021

okke-formsma commented Jun 12, 2021 •

edited

Loading

okke-formsma Jun 12, 2021

okke-formsma Jun 12, 2021

jmding8 Jun 12, 2021

jmding8 commented Jun 12, 2021 •

edited

Loading

okke-formsma commented Jun 13, 2021 •

edited

Loading

jmding8 commented Jun 13, 2021

okke-formsma commented Sep 9, 2021 •

edited

Loading

jmding8 commented Sep 12, 2021

okke-formsma left a comment

jmding8 commented Sep 12, 2021 •

edited

Loading

jmding8 commented Sep 13, 2021 •

edited

Loading

okke-formsma Sep 13, 2021 •

edited

Loading

jmding8 Sep 13, 2021

jmding8 Sep 13, 2021

jmding8 commented Sep 14, 2021 •

edited

Loading

petejohanson left a comment

okke-formsma Oct 29, 2021

jmding8 Oct 29, 2021

jmding8 Oct 29, 2021

okke-formsma Oct 29, 2021 •

edited

Loading

okke-formsma commented Oct 31, 2021

jmding8 commented Oct 31, 2021 •

edited

Loading

petejohanson left a comment

		@@ -90,6 +92,8 @@ struct last_tapped {

		struct last_tapped last_tapped;

		bool is_key_held[ZMK_KEYMAP_LEN] = {0};


		* Positional hold-taps can be useful with home-row modifiers. By using a positional hold-tap behavior for home-row modifiers on the left hand, and setting `hold-trigger-key-positions` to the keys under the right hand, positional hold-tap will only allow hold behaviors to occur with cross-hand keypresses.

		* Note that for regular hold-tap behaviors a shorter `tapping-term` encourages hold decisions. However the opposite is true for positional hold-tap behaviors, where a shorter `tapping-term` actually encourages tap decisions. This is because when the `tapping-term` expires, this triggers the behavior to decide as either a tap or a hold. But if the user has not yet had time to press one of the `hold-trigger-key-positions`, then with positional hold-tap the behavior will decide as a tap.

New Feature: required keys for tap-hold behaviors #835

New Feature: required keys for tap-hold behaviors #835

Conversation

jmding8 commented Jun 12, 2021

okke-formsma commented Jun 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmding8 commented Jun 12, 2021 • edited Loading

okke-formsma commented Jun 13, 2021 • edited Loading

jmding8 commented Jun 13, 2021

okke-formsma commented Sep 9, 2021 • edited Loading

jmding8 commented Sep 12, 2021

okke-formsma left a comment

Choose a reason for hiding this comment

jmding8 commented Sep 12, 2021 • edited Loading

jmding8 commented Sep 13, 2021 • edited Loading

okke-formsma Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmding8 commented Sep 14, 2021 • edited Loading

petejohanson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okke-formsma Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

okke-formsma commented Oct 31, 2021

jmding8 commented Oct 31, 2021 • edited Loading

petejohanson left a comment

Choose a reason for hiding this comment

okke-formsma commented Jun 12, 2021 •

edited

Loading

jmding8 commented Jun 12, 2021 •

edited

Loading

okke-formsma commented Jun 13, 2021 •

edited

Loading

okke-formsma commented Sep 9, 2021 •

edited

Loading

jmding8 commented Sep 12, 2021 •

edited

Loading

jmding8 commented Sep 13, 2021 •

edited

Loading

okke-formsma Sep 13, 2021 •

edited

Loading

jmding8 commented Sep 14, 2021 •

edited

Loading

okke-formsma Oct 29, 2021 •

edited

Loading

jmding8 commented Oct 31, 2021 •

edited

Loading