feat(behaviors): Allow mod-morph to swallow mods #1114

vrinek · 2022-02-03T20:24:30Z

This PR is based on aumuell@fa61c6a and most, if not all, of the credits should go to @aumuell. I have only contributed some git-fu to get their work rebased on top of main.

Related issues

Fixes #683.

Feature description

This PR adds a new option to mod-morph: masked_mods. This option complements the mods option, and allows the user to configure which of the mods will be swallowed when the morph triggers.

Behavior without `masked_mods`

behaviors {
    mmlt: grave_escape {
        compatible = "zmk,behavior-mod-morph";
        label = "LPAR_LT";
        #binding-cells = <0>;
        bindings = <&kp LPAR>, <&kp COMMA>;
        mods = <(MOD_LSFT|MOD_RSFT)>;
    };
};

Pressing &mmlt results in (
Pressing L-shift + &mmlt results in L-shift + ,, which is interpreted as <
Pressing R-shift + &mmlt results in R-shift + ,, which is interpreted as <

Behavior with `masked_mods`

 behaviors {
     mmlt: grave_escape {
         compatible = "zmk,behavior-mod-morph";
         label = "LPAR_LT";
         #binding-cells = <0>;
         bindings = <&kp LPAR>, <&kp COMMA>;
         mods = <(MOD_LSFT|MOD_RSFT)>;
+        masked_mods = <(MOD_LSFT)>;
     };
 };

Pressing &mmlt results in (
Pressing L-shift + &mmlt results in , (the L-shift is swallowed)
Pressing R-shift + &mmlt results in R-shift + ,, which is interpreted as <

Manual testing

Sadly, the only keyboard I have at hand that runs ZMK is a Corne-ish Zen which (for the time being) depends on a fork of this repo (https://github.com/LOWPROKB/zmk). As such I have performed no testing of this branch and would welcome any testers to lend a hand.

What I have tested is https://github.com/vrinek/zmk/tree/masked-mod-morphs-corne-ish-zen, which is the same changes but on top of the branch that works for my keyboard:

defined the behavior as described at "Behavior with masked_mods"
tried pressing the key I bound ✔️
with L-shift ✔️
with R-shift ✔️
with other mods ✔️

Testers that are willing to help are more than welcome.

TODO

write some tests

Revert "fix(hid): Implicit mods on non-key page events" This reverts commit 6ef1e70. masked mods Unrevert "fix(hid): Implicit mods on non-key page events" Fix docs Lint code with clang-format

vrinek · 2022-02-04T19:01:03Z

Successful manual test by @alex-popov-tech on a "jorne nrfmicro_13": #683 (comment)

trankillity · 2022-03-09T23:09:38Z

Have just tested this PR on a Sweep (Cradio shield) using nice!nano v2 boards. The tested over-ride was for <(MOD_LSFT|MOD_RSFT)> on bindings = <&kp COMMA>, <&kp SEMI>;.

Can confirm that it works as expected.

okke-formsma · 2022-03-13T21:01:50Z

Before we can merge this, one or more tests will have to be written to verify the behavior.

I don't know why the tests are not being run for this MR, but I think some of them would fail (e.g., app/tests/modifiers/implicit/kp-mod1-dn-mod2-dn-mod2-up-mod1-up)

vrinek · 2022-03-15T13:26:36Z

Before we can merge this, one or more tests will have to be written to verify the behavior.

I agree, and it's on my tasklist for this PR to fix the (I think only one) failing test as well as writting at least one more to validate behavior.

Life has gotten in the way though and I am making very slow progress. 😓

MatCyg · 2022-03-21T01:04:27Z

FYI: Tested this on top of macro branch, and it seems to be broken. I used the example provided in first post. Not sure if that should be fixed in this PR, but worth keeping that in mind.

PS. I am glad someone picked this up, I am looking forward to use it!

infused-kim · 2022-04-06T07:21:44Z

I have rebased it onto the main branch before the zephyr-3 update (but after the macro addition) and it's working perfectly for me.

Thank you for this PR and adding this amazing feature.

Here are a few of my use case ideas that might be useful for others:

    /*
     * Shifted Backspace deletes + Layer Tap
     *
     * Usage: &mm_bspc_del_layer
     * Tap: Backspace
     * Shift-Tap: Delete
     * Hold: Switch layer
     */
    mm_bspc_del_layer: bspc_del_layer {
        compatible = "zmk,behavior-mod-morph";
        label = "BACKSPACE_DELETE_LAYER";
        #binding-cells = <0>;
        bindings = <&lt SYM BSPC>, <&kp DEL>;
        mods = <(MOD_LSFT|MOD_RSFT)>;

        // Requires forked zmk that supports this feature
        // https://github.com/zmkfirmware/zmk/pull/1114
        //
        // Ensures that when pressing Shift-Delete only delete is sent
        // and not Shift-Delete
        masked_mods = <(MOD_LSFT|MOD_RSFT)>;
    };

    /*
     * Shifted Space underscores + Layer Tap
     *
     * Usage: &mm_spc_unds_layer
     * Tap: Space
     * LShift-Tap: -
     * RShift-Tap: _
     * Hold: Switch layer
     */
    mm_spc_unds_layer: mod_morph_underscore {
        compatible = "zmk,behavior-mod-morph";
        label = "MM_UNDERSCORE";
        #binding-cells = <0>;
        bindings = <&lt NAV SPACE>, <&kp MINUS>;
        mods = <(MOD_LSFT|MOD_RSFT)>;

        // Requires forked zmk that supports this feature
        // https://github.com/zmkfirmware/zmk/pull/1114
        masked_mods = <(MOD_LSFT)>;
    };

    /*
     * Grave Escape + GUI
     *
     * Usage: &grescm_gui
     * Tap: Escape
     * LShift-Tap: `
     * RShift-Tap: ~
     * Hold: GUI
     */
     grescm_gui: grave_escape_masked_gui {
         compatible = "zmk,behavior-mod-morph";
         label = "GRESC_GUI";
         #binding-cells = <0>;
         bindings = <&hm LGUI ESCAPE>, <&kp GRAVE>;
         mods = <(MOD_LSFT|MOD_RSFT)>;

        // Requires forked zmk that supports this feature
        // https://github.com/zmkfirmware/zmk/pull/1114
         masked_mods = <(MOD_LSFT)>;
     };

dxmh · 2022-04-06T09:00:41Z

I also used this masked_mods functionality for a while – it worked great to get macOS-like text navigation on Linux:

https://github.com/dxmh/zmk-config/blob/6561ae7d9485a9e373dc22aeff00c2cd3b3b0e7d/config/mod_morphs.dtsi

I particularly liked that I could have multiple mod-morphs per key, for example:

right = right
gui + right = end
alt + right = ctrl + right (word right)

trankillity · 2022-04-06T10:52:11Z

Some great example use cases in here. I especially love the shift + space for underscore! Definitely stealing that.

infused-kim · 2022-04-06T11:23:01Z

Some great example use cases in here. I especially love the shift + space for underscore! Definitely stealing that.

Thanks. Credit goes to Jon on discord. He is the one who came up with the idea. I just added the masking mode so that you can type minus too.

All of these work really well with home row mods. It’s a pleasure to press shift with the pointing finger of a hand and then get a underscore or dash.

Perhaps it would make sense to add some of these examples to the documentation so that new users have some inspiration for what’s possible.

vrinek · 2022-04-06T15:04:16Z

Perhaps it would make sense to add some of these examples to the documentation so that new users have some inspiration for what’s possible.

Great idea. Once I find the time and get the tests passing, I'll add some real world examples in the docs.

tap0119 · 2022-04-09T22:47:54Z

I was testing this and found that keycodes for symbols that normally use shift like DOLLAR, PERCENT and QUESTION won't send properly and will send the normal unshifted key if you ignore LSFT. DOLLAR sends 4, PERCENT sends 5, QUESTION sends / and so on. If you only ignore RSFT they will send properly.

trankillity · 2022-04-10T05:47:18Z

I was testing this and found that keycodes for symbols that normally use shift like DOLLAR, PERCENT and QUESTION won't send properly and will send the normal unshifted key if you ignore LSFT. DOLLAR sends 4, PERCENT sends 5, QUESTION sends / and so on. If you only ignore RSFT they will send properly.

Well... I mean... yeah. But just use normal mod-morph without the swallowing for this. That's what I did for ./:

tap0119 · 2022-04-10T06:59:53Z

I was testing this and found that keycodes for symbols that normally use shift like DOLLAR, PERCENT and QUESTION won't send properly and will send the normal unshifted key if you ignore LSFT. DOLLAR sends 4, PERCENT sends 5, QUESTION sends / and so on. If you only ignore RSFT they will send properly.

Well... I mean... yeah. But just use normal mod-morph without the swallowing for this. That's what I did for ./:

I was using shift plus a key to momentarily move to a layer and wanted to use those keys on that layer but not have shift held down for arrow keys or numbers on that layer. You can do that as long as you use right shift.

alex-popov-tech · 2022-05-05T11:51:43Z

hey guys, any update on this?

vrinek · 2022-05-06T08:18:42Z

hey guys, any update on this?

No real updates. The task list right now is:

update patch with latest main
fix broken tests
add new tests to cover new functionality

I have not been able to make any progress, cause life priorities.

slashfoo · 2022-06-21T21:53:12Z

app/src/behaviors/behavior_mod_morph.c

@@ -45,6 +46,8 @@ static int on_mod_morph_binding_pressed(struct zmk_behavior_binding *binding,
    }

    if (zmk_hid_get_explicit_mods() & cfg->mods) {
+        zmk_mod_flags_t trigger_mods = zmk_hid_get_explicit_mods() & cfg->mods;


I patched this, and get a warning that reads that trigger_mods is an unused variable. Is this supposed to be used in some sort of restore behavior later?

Also, when using the behavior, if I add shifts to my masked_mods, and my binding has a shift, it still strips them out. I think the masked_mods should mask the trigger_mods and not the resulting binding, i.e. if the result is specified to be LS(N2), it should send @, and not the 2 just because MOD_LSFT was in the masked_mods.

I do not recall such behavior, but I am using an old version of ZMK at the moment.

@urob have you experienced any similar behavior?

I patched this, and get a warning that reads that trigger_mods is an unused variable. Is this supposed to be used in some sort of restore behavior later?

~~This is weird, I just merged this into the latest ZMK and I had no issues.~~ Sorry, I was too fast. Yes, I get a similar warning:

[31/203] Building C object CMakeFiles/app.dir/src/behaviors/behavior_mod_morph.c.obj /__w/zmk-config/zmk-config/zmk/app/src/behaviors/behavior_mod_morph.c: In function 'on_mod_morph_binding_pressed': /__w/zmk-config/zmk-config/zmk/app/src/behaviors/behavior_mod_morph.c:49:25: warning: unused variable 'trigger_mods' [-Wunused-variable] 49 | zmk_mod_flags_t trigger_mods = zmk_hid_get_explicit_mods() & cfg->mods; | ^~~~~~~~~~~~

As far as I can tell, there are no issues caused by this but it probably makes sense to check in with @aumuell just to make sure there is nothing amiss here.

Also, when using the behavior, if I add shifts to my masked_mods, and my binding has a shift, it still strips them out. I think the masked_mods should mask the trigger_mods and not the resulting binding, i.e. if the result is specified to be LS(N2), it should send @, and not the 2 just because MOD_LSFT was in the masked_mods.

Unless I am misunderstanding, this is the intended behavior. Don't include MOD_LSFT in masked_mods if you don't want it to be masked. I think there was a discussion on discord about a related PR whether to always pass through mods that are part of a binding, even if they are in masked_mods. But I can't think of any scenario where one couldn't achieve the same by just omitting them from masked_mods

The problem with the modifier not showing up on the host is that, it gets removed from what I want to send, masked mods should work in the manner of "remove all of these mods from what the user pressed" and everything that's specified inside the bindings should get to the host along with the modifiers that weren't masked.

The idea behind masked_mods as it is right now, contradicts the intent of adding a binding that contains any of those modifiers. And the way I understand it, when a user adds stuff to bindings it's because they want that to arrive at the host when pressing the binding position modulated by mod-morph.

Currently the behavior is similar to:

Calculate which binding we want.

Add active modifiers to the results

Add binding selected to the results

Strip anything in masked_mods from the results

Send the result to the host.

I think a better approach that would be:

Calculate which of the bindings we want

Add active modifiers to the results

Strip anything in masked_mods from the results

Add binding selected to the results

Send the result to the host.

In my proposed flow, there's no "unexpected" deletion of the modifiers explicitly defined in the bindings.

Also, when using the behavior, if I add shifts to my masked_mods, and my binding has a shift, it still strips them out. I think the masked_mods should mask the trigger_mods and not the resulting binding, i.e. if the result is specified to be LS(N2), it should send @, and not the 2 just because MOD_LSFT was in the masked_mods.

@slashfoo, for this particular mod-morph, as a workaround, would it work to remove MOD_LSFT from the masked mods?

urob · 2022-06-21T22:54:55Z

A quick update on this. I cherry-picked 241f82e into the latest main and successfully build against it. So far, all my tests did go well and masked_mods seem to do exactly what they set out to do. In case someone else wants to test without messing with a (minor) merge conflict, this is the commit: e40b9a7.

alex-popov-tech · 2022-06-22T06:56:36Z

@urob i am willing to test, can you please advice on how i can do that?

urob · 2022-06-22T13:23:02Z

@urob i am willing to test, can you please advice on how i can do that?

If you don't have a local copy of ZMK, you can use the beta testing feature to select an arbitrary remote branch of ZMK to be used with Github Actions.

If you want masked mods with the latest ZMK, you can build against this branch. ~~I didn't create a dedicated branch as there is already this PR, but this branch will give you the latest ZMK (as of 06/21/2022) + masked mods.~~ EDIT: I made a dedicated branch so that it won't be affected by other things I change in my personal branch. You can use it with Github Actions by changing your west.yml to something like this:

manifest:
  remotes:
    - name: zmkfirmware
      url-base: https://github.com/zmkfirmware
    - name: urob
      url-base: https://github.com/urob
  projects:
    - name: zmk
      remote: urob
      revision: masked-mods
      import: app/west.yml
  self:
    path: config

alex-popov-tech · 2022-06-22T22:14:06Z

@urob i usually build things locally with devcontainers

so i've pulled your repo, checkout to your branch, reopened it in container, did all instructions, then tried to build and received following error:

est build -p -s ./app -d build/jian_vu_keys/left -b nrfmicro_13 -- -DSHIELD=jian_left -DZMK_CONFIG="/workspaces/zmk-config/jian/config"
-- west build: making build dir /workspaces/zmk_fork/build/jian_vu_keys/left pristine
-- west build: generating a build system
Including boilerplate (Zephyr base): /workspaces/zmk_fork/zephyr/cmake/app/boilerplate.cmake
-- Application: /workspaces/zmk_fork/app
-- Adding /workspaces/zmk_fork/app/boards/shields/jian
-- ZMK Config directory: /workspaces/zmk-config/jian/config
-- ZMK Config Kconfig: /workspaces/zmk-config/jian/config/jian.conf
-- Using keymap file: /workspaces/zmk-config/jian/config/jian.keymap
-- Using keymap file: /workspaces/zmk-config/jian/config/jian.keymap
-- Zephyr version: 3.0.0 (/workspaces/zmk_fork/zephyr)
-- Found Python3: /usr/bin/python3.8 (found suitable exact version "3.8.10") found components: Interpreter 
-- Found west (found suitable version "0.13.0", minimum required is "0.7.1")
-- Board: nrfmicro_13, Shield(s): jian_left
-- Cache files will be written to: /root/.cache/zephyr
-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
-- Found host-tools: zephyr 0.13.2 (/opt/zephyr-sdk-0.13.2)
-- Found dtc: /opt/zephyr-sdk-0.13.2/sysroots/x86_64-pokysdk-linux/usr/bin/dtc (found suitable version "1.6.0", minimum required is "1.4.6")
-- Found toolchain: zephyr 0.13.2 (/opt/zephyr-sdk-0.13.2)
-- Found BOARD.dts: /workspaces/zmk_fork/app/boards/arm/nrfmicro/nrfmicro_13.dts
-- Found devicetree overlay: /workspaces/zmk_fork/app/boards/shields/jian/jian_left.overlay
-- Found devicetree overlay: /workspaces/zmk-config/jian/config/jian.keymap
'debounce-period' is marked as deprecated in 'properties:' in /workspaces/zmk_fork/app/drivers/zephyr/dts/bindings/kscan/zmk,kscan-gpio-matrix.yaml for node /kscan.
-- Generated zephyr.dts: /workspaces/zmk_fork/build/jian_vu_keys/left/zephyr/zephyr.dts
-- Generated devicetree_unfixed.h: /workspaces/zmk_fork/build/jian_vu_keys/left/zephyr/include/generated/devicetree_unfixed.h
-- Generated device_extern.h: /workspaces/zmk_fork/build/jian_vu_keys/left/zephyr/include/generated/device_extern.h
-- Including generated dts.cmake file: /workspaces/zmk_fork/build/jian_vu_keys/left/zephyr/dts.cmake
Parsing /workspaces/zmk_fork/app/Kconfig
/workspaces/zmk_fork/zephyr/scripts/kconfig/kconfig.py: /workspaces/zmk_fork/app/zephyr/Kconfig:8: recursive 'source' of 'Kconfig.zephyr' detected. Check that environment variables are set correctly.
Include path:
/workspaces/zmk_fork/app/Kconfig:473
Kconfig.zephyr:33
modules/Kconfig:6
/workspaces/zmk_fork/build/jian_vu_keys/left/Kconfig/Kconfig.modules:2
/workspaces/zmk_fork/app/zephyr/Kconfig:8
CMake Error at /workspaces/zmk_fork/zephyr/cmake/kconfig.cmake:272 (message):
  command failed with return code: 1
Call Stack (most recent call first):
  /workspaces/zmk_fork/zephyr/cmake/app/boilerplate.cmake:543 (include)
  /workspaces/zmk_fork/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:24 (include)
  /workspaces/zmk_fork/zephyr/share/zephyr-package/cmake/ZephyrConfig.cmake:35 (include_boilerplate)
  CMakeLists.txt:15 (find_package)


-- Configuring incomplete, errors occurred!
FATAL ERROR: command exited with status 1: /usr/local/bin/cmake -DWEST_PYTHON=/usr/bin/python3 -B/workspaces/zmk_fork/build/jian_vu_keys/left -S/workspaces/zmk_fork/app -GNinja -DBOARD=nrfmicro_13 -DSHIELD=jian_left -DZMK_CONFIG=/workspaces/zmk-config/jian/config
make: *** [Makefile:11: jian_vu_keys] Error 1

here is my config - https://github.com/alex-popov-tech/zmk-config/tree/master/jian/config

urob · 2022-06-23T00:58:21Z

@alex-popov-tech Not sure what's causing this or whether this is related to this PR. The ZMK discord https://discord.com/channels/719497620560543766/719909884769992755 might be able to provide better feedback on this

alex-popov-tech · 2022-06-23T20:14:22Z

just tested your branch @urob , it all working perfectly fine, even tho i have macros, combos, modmorph plain, modmorph with masked mods, and alltogether it works perfectly fine! is it possible to apply patch to this pr maybe?

vrinek · 2022-06-27T14:20:58Z

just tested your branch @urob , it all working perfectly fine, even tho i have macros, combos, modmorph plain, modmorph with masked mods, and alltogether it works perfectly fine! is it possible to apply patch to this pr maybe?

I'll try to make the time to get this done today.

…ested

vrinek · 2022-06-27T19:19:03Z

I just merged the latest main into this PR's branch. @urob, this is the only diff I see between this branch and yours:

diff --git a/app/src/hid.c b/app/src/hid.c
index 798801cd..1c903ca8 100644
--- a/app/src/hid.c
+++ b/app/src/hid.c
@@ -161,29 +161,29 @@ static inline int check_keyboard_usage(zmk_key_t usage) {
     }

 int zmk_hid_implicit_modifiers_press(zmk_mod_flags_t new_implicit_modifiers) {
-    zmk_mod_flags_t current = GET_MODIFIERS;
     implicit_modifiers = new_implicit_modifiers;
+    zmk_mod_flags_t current = GET_MODIFIERS;
     SET_MODIFIERS(explicit_modifiers);
     return current == GET_MODIFIERS ? 0 : 1;
 }

 int zmk_hid_implicit_modifiers_release() {
-    zmk_mod_flags_t current = GET_MODIFIERS;
     implicit_modifiers = 0;
+    zmk_mod_flags_t current = GET_MODIFIERS;
     SET_MODIFIERS(explicit_modifiers);
     return current == GET_MODIFIERS ? 0 : 1;
 }

 int zmk_hid_masked_modifiers_set(zmk_mod_flags_t new_masked_modifiers) {
-    zmk_mod_flags_t current = GET_MODIFIERS;
     masked_modifiers = new_masked_modifiers;
+    zmk_mod_flags_t current = GET_MODIFIERS;
     SET_MODIFIERS(explicit_modifiers);
     return current == GET_MODIFIERS ? 0 : 1;
 }

 int zmk_hid_masked_modifiers_clear() {
-    zmk_mod_flags_t current = GET_MODIFIERS;
     masked_modifiers = 0;
+    zmk_mod_flags_t current = GET_MODIFIERS;
     SET_MODIFIERS(explicit_modifiers);
     return current == GET_MODIFIERS ? 0 : 1;
 }

I do not think the above affects anything, but I am not familiar with C or this codebase much. Do you see something I do not?

urob · 2022-06-27T19:44:43Z

I just merged the latest main into this PR's branch. @urob, this is the only diff I see between this branch and yours. [...] Do you see something I do not?

Nope, the order shouldn't matter here. There is a minor formatting issue in app/include/zmk/hid.h. We both did resolve the merge conflict in that file by deleting the blank line that was added by @okke-formsma (57fca34) in main since @aumuell has written the fix. Obviously, this is not of any substance, but it probably makes sense to keep it for clarity (after the two new modifier-related lines).

gaoDean · 2022-07-09T08:13:07Z

Sorry for the inconvenience, but I’ve noticed that key-repeat doesn’t work with the swallowed key. Specifically, shift+key works, but then keyrepeat produces the uppercase version.
Everything else works extremely well, thank you for this!

vrinek · 2022-07-11T12:56:53Z

Sorry for the inconvenience, but I’ve noticed that key-repeat doesn’t work with the swallowed key. Specifically, shift+key works, but then keyrepeat produces the uppercase version. Everything else works extremely well, thank you for this!

@gaoDean I am not sure I follow. Can you break this down to "keydown"/"keyup" events for me?

gaoDean · 2022-07-12T12:44:54Z

@vrinek

edit: got some things wrong
edit: I think I have my shift on sticky-key-quick, don’t remember if this effect happens when holding shift

x = key (mod morph)
^ = keyup
_ = key down

( the shift I am using below in the keydown/keyup is merely the key shift as bound to, not the actual modifier being pressed )
_shift, _x, ^x, ^shft -> key ( no shift )
skq version: _shift, ^shift, _x, ^x -> key ( no shift)
_keyrepeat, ^keyrepeat -> shift + key

say I have mod morph with normal being dot, morph being slash.
Shift (as the mod) + dot produces slash.
After that, I press key repeat and it outputs question mark

urob · 2022-07-19T00:48:07Z

@vrinek @aumuell

A few quick comments on the PR.

I submitted a small "PR to the PR" that excludes implicit modifiers from getting masked. It enables users to re-include masked out mods in the mod-morph binding, which is (1) convenient (see @slashfoo 's review comment) and (2) enables binding macros that at some point (but not the entire time) use the masked-mods. As far as I see, there is no real downside given that LS(&my_mod_morph) is not a thing
Does it make sense to default masked_mods to mods if unspecified? It seems this would be the expected behavior of most mod-morph use cases, and users can overwrite it if wanted (and with the proposed change to implicit mods, there would also no unexpected implications when binding mods to the mod-morph)
I did a bit more exploring of @slashfoo 's question about the unused mod-trigger definition and recompiled excluding it. So far everything seems fine, raising the question whether it can indeed be deleted?
This PR seems now be used & tested by various people. What's needed at this point to push it forward?

vrinek · 2022-07-22T15:41:04Z

@urob

I submitted a small "PR to the PR" that excludes implicit modifiers from getting masked. It enables users to re-include masked out mods in the mod-morph binding, which is (1) convenient (see @slashfoo 's review comment) and (2) enables binding macros that at some point (but not the entire time) use the masked-mods. As far as I see, there is no real downside given that LS(&my_mod_morph) is not a thing

I am on vacation at the moment and can't test this on a physical device until late next week.

Does it make sense to default masked_mods to mods if unspecified? It seems this would be the expected behavior of most mod-morph use cases, and users can overwrite it if wanted (and with the proposed change to implicit mods, there would also no unexpected implications when binding mods to the mod-morph)

This sounds like a breaking change, no?

I did a bit more exploring of @slashfoo 's question about the unused mod-trigger definition and recompiled excluding it. So far everything seems fine, raising the question whether it can indeed be deleted?

Can you point me to the mod-trigger definition? https://github.com/vrinek/zmk/search?q=mod-trigger&type=code brings up no results.

This PR seems now be used & tested by various people. What's needed at this point to push it forward?

From what I know, test coverage is the only piece missing. See #1114 (comment).

urob · 2022-07-22T16:02:34Z

This sounds like a breaking change, no?

Technically, yes, it does change the behavior of mod-morph. But I would argue that in the majority of cases what the uses wants is to indeed swallow the mods (see #683). Hence, I'd view it more a bug fix than a breaking change.

Can you point me to the mod-trigger definition? https://github.com/vrinek/zmk/search?q=mod-trigger&type=code brings up no results.

Sorry, I meant trigger_mods:
https://github.com/vrinek/zmk/blob/89dac4c54bf0f679d79f1da27c069d30cc102581/app/src/behaviors/behavior_mod_morph.c#L49

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

urob · 2022-07-24T05:27:12Z

Quick update: I added a few more commits to the "PR to the PR". It now includes the following changes relative to the original PR:

Implicit mods specified as part of the morphed binding take prevalence over masked_mods (see explanations above)
I deleted the unused trigger_mods definition
I am now setting masked_mods to mods if unspecified. Based on a discussion on discord, this seems to be the preferred behavior.
Docs for mod-morph to reflect the changes.
[edit:] Tests for mod-morph with different masked_mods settings

To keep things simple, I added all these changes to the open "PR to the PR" (vrinek#1). If someone wants to test them, this is the branch for the tweaked PR: https://github.com/urob/zmk/tree/fix-mod-morph

urob · 2022-07-24T07:03:54Z

I took at look at the failed test that @okke-formsma had pointed to. After its last rebase, the PR passes all but one test: kp-lctl-dn-mod-dn-lctl-up-mod-up. My reading is that the "failure" reflects that the PR actually fixes a bug in how implicit mods interact with explicit mods.

Specifically, the test sequence is press &kp LCTRL, press &kp LS(B), release &kp LCTRL, release &kp LS(B). The difference between the PR's behavior and the expected test result is about what happens upon the release of LCTRL. The test expects all modifiers to be cleared (0x00), whereas with the PR the implicitly specified LEFT_SHIFT which is not yet released remains active (0x02).

Here's the diff:

Running tests/modifiers/mixed/kp-lctl-dn-mod-dn-lctl-up-mod-up:
--- tests/modifiers/mixed/kp-lctl-dn-mod-dn-lctl-up-mod-up/keycode_events.snapshot      2022-06-23 13:21:37.471342000 -0400
+++ build/tests/modifiers/mixed/kp-lctl-dn-mod-dn-lctl-up-mod-up/keycode_events.log     2022-07-24 02:31:49.751496900 -0400
@@ -7,7 +7,7 @@
 released: usage_page 0x07 keycode 0xE0 implicit_mods 0x00 explicit_mods 0x00
 unreg: Modifier 0 count: 0
 unreg: Modifier 0 released
-unreg: Modifiers set to 0x00
+unreg: Modifiers set to 0x02
 mods: Modifiers set to 0x00
 released: usage_page 0x07 keycode 0x05 implicit_mods 0x02 explicit_mods 0x00
 mods: Modifiers set to 0x00
FAILED: tests/modifiers/mixed/kp-lctl-dn-mod-dn-lctl-up-mod-up

The discrepancy is due to the PR's refactoring of how explicit and implicit modifiers interact in app/src/hid.c, which fixes an (undocumented?) bug where implicit mods get cleared by explicit mod events that occurs while they are being held (see the changes to SET_MODIFIERS, zmk_hid_implicit_modifiers_press and zmk_hid_implicit_modifiers_release).

@okke-formsma do you agree with this? If yes, then my take is that the right course of action would be to modify the expected test output of tests/modifiers/mixed/kp-lctl-dn-mod-dn-lctl-up-mod-up/keycode_events.snapshot to reflect the improved behavior. (Like this: 386b5d1.) Do you agree?

urob · 2022-07-24T15:13:54Z

One more update: I added a battery of tests to the open "PR to the PR" branch (https://github.com/urob/zmk/tree/fix-mod-morph). Note that the tests are based on my modified PR branch with the changes outlined above.

If we are feeling comfortable with changing the expected test output of kp-lctl-dn-mod-dn-lctl-up-mod-up (see previous comment), this PR is ready to go (or at least ready to enter official review status).

@vrinek, let me know if you want to take it from here or if you want me to open a new PR with all the updates.

EDIT: I checked with Pete on discord, and we decided that it may be better to open a new PR with the changes: #1412

Explicit mods no longer clear implicit mods that are being held. See zmkfirmware#1114 (comment) for details

Squashed commit of the following: commit 386b5d1 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 25 22:07:13 2022 -0400 Update expected test output Explicit mods no longer clear implicit mods that are being held. See zmkfirmware#1114 (comment) for details commit b4dad62 Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 11:08:39 2022 -0400 Explain how to fully disable masked_mods commit 00a0235 Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 11:05:44 2022 -0400 Add mod-morph tests commit bfba42f Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 02:17:34 2022 -0400 Fix doc formatting commit 67412ed Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 01:50:54 2022 -0400 Fix clang-format commit ebc127d Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 01:04:13 2022 -0400 Update docs for mod-morph commit 44297de Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 00:24:18 2022 -0400 Set masked-mods to mods if unspecified commit 7c647b0 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 18 20:31:24 2022 -0400 Trigger-mods are unused commit 4cf66a4 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 18 19:57:46 2022 -0400 Don't mask implicit mods commit 89dac4c Author: Kostas Karachalios <vrinek@hey.com> Date: Tue Jun 28 09:43:02 2022 +0200 Add some whitespace for clarity commit e96f516 Merge: 2cac694 ef3eb33 Author: Kostas Karachalios <vrinek@hey.com> Date: Mon Jun 27 21:11:20 2022 +0200 Merge remote-tracking branch 'origin/main' into masked-mod-morphs-untested commit 2cac694 Author: Kostas Karachalios <vrinek@hey.com> Date: Thu Feb 3 19:00:03 2022 +0100 feat(behaviors): Allow mod-morph to swallow mods Revert "fix(hid): Implicit mods on non-key page events" This reverts commit 6ef1e70. masked mods Unrevert "fix(hid): Implicit mods on non-key page events" Fix docs Lint code with clang-format

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

Squashed commit of the following: commit f18eeab Author: urob <978080+urob@users.noreply.github.com> Date: Thu Jul 28 15:59:55 2022 -0400 Clean up expected test output commit f05b251 Author: urob <978080+urob@users.noreply.github.com> Date: Thu Jul 28 09:32:43 2022 -0400 Explicitly specify default masked_mods in test Just so that the test can be run on branches with different defaults commit 278dc67 Author: urob <978080+urob@users.noreply.github.com> Date: Wed Jul 27 22:40:37 2022 -0400 Document masked_mods limitation with hold-taps commit 386b5d1 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 25 22:07:13 2022 -0400 Update expected test output Explicit mods no longer clear implicit mods that are being held. See zmkfirmware#1114 (comment) for details commit b4dad62 Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 11:08:39 2022 -0400 Explain how to fully disable masked_mods commit 00a0235 Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 11:05:44 2022 -0400 Add mod-morph tests commit bfba42f Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 02:17:34 2022 -0400 Fix doc formatting commit 67412ed Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 01:50:54 2022 -0400 Fix clang-format commit ebc127d Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 01:04:13 2022 -0400 Update docs for mod-morph commit 44297de Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 00:24:18 2022 -0400 Set masked-mods to mods if unspecified commit 7c647b0 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 18 20:31:24 2022 -0400 Trigger-mods are unused commit 4cf66a4 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 18 19:57:46 2022 -0400 Don't mask implicit mods commit 89dac4c Author: Kostas Karachalios <vrinek@hey.com> Date: Tue Jun 28 09:43:02 2022 +0200 Add some whitespace for clarity commit e96f516 Merge: 2cac694 ef3eb33 Author: Kostas Karachalios <vrinek@hey.com> Date: Mon Jun 27 21:11:20 2022 +0200 Merge remote-tracking branch 'origin/main' into masked-mod-morphs-untested commit 2cac694 Author: Kostas Karachalios <vrinek@hey.com> Date: Thu Feb 3 19:00:03 2022 +0100 feat(behaviors): Allow mod-morph to swallow mods Revert "fix(hid): Implicit mods on non-key page events" This reverts commit 6ef1e70. masked mods Unrevert "fix(hid): Implicit mods on non-key page events" Fix docs Lint code with clang-format

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

Squashed commit of the following: commit 386b5d1 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 25 22:07:13 2022 -0400 Update expected test output Explicit mods no longer clear implicit mods that are being held. See zmkfirmware#1114 (comment) for details commit b4dad62 Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 11:08:39 2022 -0400 Explain how to fully disable masked_mods commit 00a0235 Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 11:05:44 2022 -0400 Add mod-morph tests commit bfba42f Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 02:17:34 2022 -0400 Fix doc formatting commit 67412ed Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 01:50:54 2022 -0400 Fix clang-format commit ebc127d Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 01:04:13 2022 -0400 Update docs for mod-morph commit 44297de Author: urob <978080+urob@users.noreply.github.com> Date: Sun Jul 24 00:24:18 2022 -0400 Set masked-mods to mods if unspecified commit 7c647b0 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 18 20:31:24 2022 -0400 Trigger-mods are unused commit 4cf66a4 Author: urob <978080+urob@users.noreply.github.com> Date: Mon Jul 18 19:57:46 2022 -0400 Don't mask implicit mods commit 89dac4c Author: Kostas Karachalios <vrinek@hey.com> Date: Tue Jun 28 09:43:02 2022 +0200 Add some whitespace for clarity commit e96f516 Merge: 2cac694 ef3eb33 Author: Kostas Karachalios <vrinek@hey.com> Date: Mon Jun 27 21:11:20 2022 +0200 Merge remote-tracking branch 'origin/main' into masked-mod-morphs-untested commit 2cac694 Author: Kostas Karachalios <vrinek@hey.com> Date: Thu Feb 3 19:00:03 2022 +0100 feat(behaviors): Allow mod-morph to swallow mods Revert "fix(hid): Implicit mods on non-key page events" This reverts commit 6ef1e70. masked mods Unrevert "fix(hid): Implicit mods on non-key page events" Fix docs Lint code with clang-format

feat(behaviors): Allow mod-morph to swallow mods

2cac694

Revert "fix(hid): Implicit mods on non-key page events" This reverts commit 6ef1e70. masked mods Unrevert "fix(hid): Implicit mods on non-key page events" Fix docs Lint code with clang-format

vrinek force-pushed the masked-mod-morphs-untested branch from 2372be3 to 2cac694 Compare February 3, 2022 20:26

vrinek marked this pull request as ready for review February 3, 2022 20:26

vrinek marked this pull request as draft February 3, 2022 20:43

vresch added a commit to vresch/zmk-config that referenced this pull request May 11, 2022

rmv unsupported masked moprh modifier zmkfirmware/zmk#1114

2990690

slashfoo reviewed Jun 21, 2022

View reviewed changes

urob mentioned this pull request Jun 23, 2022

Feature: Unicode #232

Open

urob mentioned this pull request Jun 26, 2022

when using mod-morph to make shift-backspace work as delete, the shift key is not 'swallowed' #683

Closed

Merge remote-tracking branch 'origin/main' into masked-mod-morphs-unt…

e96f516

…ested

Add some whitespace for clarity

89dac4c

caksoylar added a commit to caksoylar/zmk that referenced this pull request Jul 23, 2022

Apply masked mods PR zmkfirmware#1114 with urob's tweaks

1658f73

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

urob added a commit to urob/zmk that referenced this pull request Jul 26, 2022

Update expected test output

386b5d1

Explicit mods no longer clear implicit mods that are being held. See zmkfirmware#1114 (comment) for details

caksoylar added a commit to caksoylar/zmk that referenced this pull request Jul 27, 2022

Apply masked mods PR zmkfirmware#1114 with urob's tweaks

1d93b58

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

caksoylar added a commit to caksoylar/zmk that referenced this pull request Jul 30, 2022

Apply masked mods PR zmkfirmware#1114 with urob's tweaks

0bc7f5e

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

caksoylar added a commit to caksoylar/zmk that referenced this pull request Jul 31, 2022

Apply masked mods PR zmkfirmware#1114 with urob's tweaks

f52d1b4

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

urob mentioned this pull request Jul 31, 2022

Fix mod-morph #1412

Merged

caksoylar added a commit to caksoylar/zmk that referenced this pull request Aug 1, 2022

Apply masked mods PR zmkfirmware#1114 with urob's tweaks

5a8c274

Co-authored by: Kostas Karachalios <vrinek@hey.com> Co-authored by: urob <978080+urob@users.noreply.github.com>

petejohanson closed this Dec 13, 2022

yanshay mentioned this pull request Mar 3, 2023

Can't get SHIFT-SPACE to issue TAB (with karabiner elements) #1685

Open

feat(behaviors): Allow mod-morph to swallow mods #1114

feat(behaviors): Allow mod-morph to swallow mods #1114

Conversation

vrinek commented Feb 3, 2022 • edited Loading

Related issues

Feature description

Behavior without masked_mods

Behavior with masked_mods

Manual testing

TODO

vrinek commented Feb 4, 2022

trankillity commented Mar 9, 2022

okke-formsma commented Mar 13, 2022 • edited Loading

vrinek commented Mar 15, 2022

MatCyg commented Mar 21, 2022

infused-kim commented Apr 6, 2022

dxmh commented Apr 6, 2022 • edited Loading

trankillity commented Apr 6, 2022

infused-kim commented Apr 6, 2022

vrinek commented Apr 6, 2022

tap0119 commented Apr 9, 2022

trankillity commented Apr 10, 2022

tap0119 commented Apr 10, 2022

alex-popov-tech commented May 5, 2022

vrinek commented May 6, 2022

slashfoo Jun 21, 2022

Choose a reason for hiding this comment

vrinek Jun 22, 2022

Choose a reason for hiding this comment

urob Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

slashfoo Jun 26, 2022

Choose a reason for hiding this comment

vrinek Jun 27, 2022 • edited Loading

Choose a reason for hiding this comment

urob commented Jun 21, 2022

alex-popov-tech commented Jun 22, 2022

urob commented Jun 22, 2022 • edited Loading

alex-popov-tech commented Jun 22, 2022 • edited Loading

urob commented Jun 23, 2022

alex-popov-tech commented Jun 23, 2022

vrinek commented Jun 27, 2022

vrinek commented Jun 27, 2022

urob commented Jun 27, 2022

gaoDean commented Jul 9, 2022 • edited Loading

vrinek commented Jul 11, 2022

gaoDean commented Jul 12, 2022 • edited Loading

urob commented Jul 19, 2022 • edited Loading

vrinek commented Jul 22, 2022 • edited Loading

urob commented Jul 22, 2022 • edited Loading

urob commented Jul 24, 2022 • edited Loading

urob commented Jul 24, 2022 • edited Loading

urob commented Jul 24, 2022 • edited Loading

vrinek commented Feb 3, 2022 •

edited

Loading

Behavior without `masked_mods`

Behavior with `masked_mods`

okke-formsma commented Mar 13, 2022 •

edited

Loading

dxmh commented Apr 6, 2022 •

edited

Loading

urob Jun 22, 2022 •

edited

Loading

vrinek Jun 27, 2022 •

edited

Loading

urob commented Jun 22, 2022 •

edited

Loading

alex-popov-tech commented Jun 22, 2022 •

edited

Loading

gaoDean commented Jul 9, 2022 •

edited

Loading

gaoDean commented Jul 12, 2022 •

edited

Loading

urob commented Jul 19, 2022 •

edited

Loading

vrinek commented Jul 22, 2022 •

edited

Loading

urob commented Jul 22, 2022 •

edited

Loading

urob commented Jul 24, 2022 •

edited

Loading

urob commented Jul 24, 2022 •

edited

Loading

urob commented Jul 24, 2022 •

edited

Loading