-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(behaviors): Allow mod-morph to swallow mods #1114
Conversation
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
2372be3
to
2cac694
Compare
Successful manual test by @alex-popov-tech on a "jorne nrfmicro_13": #683 (comment) |
Have just tested this PR on a Sweep (Cradio shield) using nice!nano v2 boards. The tested over-ride was for Can confirm that it works as expected. |
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) |
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. 😓 |
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! |
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:
|
I also used this I particularly liked that I could have multiple mod-morphs per key, for example:
|
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. |
Great idea. Once I find the time and get the tests passing, I'll add some real world examples in the docs. |
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. |
hey guys, any update on this? |
No real updates. The task list right now is:
I have not been able to make any progress, cause life priorities. |
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 the2
just becauseMOD_LSFT
was in themasked_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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 the2
just becauseMOD_LSFT
was in themasked_mods
.
@slashfoo, for this particular mod-morph, as a workaround, would it work to remove MOD_LSFT
from the masked mods?
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 |
@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.
|
@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:
here is my config - https://github.com/alex-popov-tech/zmk-config/tree/master/jian/config |
@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 |
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. |
I just merged the latest 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? |
Nope, the order shouldn't matter here. There is a minor formatting issue in |
Sorry for the inconvenience, but I’ve noticed that key-repeat doesn’t work with the swallowed key. Specifically, |
@gaoDean I am not sure I follow. Can you break this down to "keydown"/"keyup" events for me? |
edit: got some things wrong x = key (mod morph) ( the shift I am using below in the keydown/keyup is merely the key shift as bound to, not the actual modifier being pressed ) say I have mod morph with normal being dot, morph being slash. |
A few quick comments on the PR.
|
I am on vacation at the moment and can't test this on a physical device until late next week.
This sounds like a breaking change, no?
Can you point me to the
From what I know, test coverage is the only piece missing. See #1114 (comment). |
Technically, yes, it does change the behavior of
Sorry, I meant |
Co-authored by: Kostas Karachalios <[email protected]> Co-authored by: urob <[email protected]>
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:
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 |
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: Specifically, the test sequence is Here's the diff:
The discrepancy is due to the PR's refactoring of how explicit and implicit modifiers interact in @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 |
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 @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 <[email protected]> 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 <[email protected]> Date: Sun Jul 24 11:08:39 2022 -0400 Explain how to fully disable masked_mods commit 00a0235 Author: urob <[email protected]> Date: Sun Jul 24 11:05:44 2022 -0400 Add mod-morph tests commit bfba42f Author: urob <[email protected]> Date: Sun Jul 24 02:17:34 2022 -0400 Fix doc formatting commit 67412ed Author: urob <[email protected]> Date: Sun Jul 24 01:50:54 2022 -0400 Fix clang-format commit ebc127d Author: urob <[email protected]> Date: Sun Jul 24 01:04:13 2022 -0400 Update docs for mod-morph commit 44297de Author: urob <[email protected]> Date: Sun Jul 24 00:24:18 2022 -0400 Set masked-mods to mods if unspecified commit 7c647b0 Author: urob <[email protected]> Date: Mon Jul 18 20:31:24 2022 -0400 Trigger-mods are unused commit 4cf66a4 Author: urob <[email protected]> Date: Mon Jul 18 19:57:46 2022 -0400 Don't mask implicit mods commit 89dac4c Author: Kostas Karachalios <[email protected]> Date: Tue Jun 28 09:43:02 2022 +0200 Add some whitespace for clarity commit e96f516 Merge: 2cac694 ef3eb33 Author: Kostas Karachalios <[email protected]> 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 <[email protected]> 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 <[email protected]> Co-authored by: urob <[email protected]>
Co-authored by: Kostas Karachalios <[email protected]> Co-authored by: urob <[email protected]>
Co-authored by: Kostas Karachalios <[email protected]> Co-authored by: urob <[email protected]>
Squashed commit of the following: commit f18eeab Author: urob <[email protected]> Date: Thu Jul 28 15:59:55 2022 -0400 Clean up expected test output commit f05b251 Author: urob <[email protected]> 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 <[email protected]> Date: Wed Jul 27 22:40:37 2022 -0400 Document masked_mods limitation with hold-taps commit 386b5d1 Author: urob <[email protected]> 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 <[email protected]> Date: Sun Jul 24 11:08:39 2022 -0400 Explain how to fully disable masked_mods commit 00a0235 Author: urob <[email protected]> Date: Sun Jul 24 11:05:44 2022 -0400 Add mod-morph tests commit bfba42f Author: urob <[email protected]> Date: Sun Jul 24 02:17:34 2022 -0400 Fix doc formatting commit 67412ed Author: urob <[email protected]> Date: Sun Jul 24 01:50:54 2022 -0400 Fix clang-format commit ebc127d Author: urob <[email protected]> Date: Sun Jul 24 01:04:13 2022 -0400 Update docs for mod-morph commit 44297de Author: urob <[email protected]> Date: Sun Jul 24 00:24:18 2022 -0400 Set masked-mods to mods if unspecified commit 7c647b0 Author: urob <[email protected]> Date: Mon Jul 18 20:31:24 2022 -0400 Trigger-mods are unused commit 4cf66a4 Author: urob <[email protected]> Date: Mon Jul 18 19:57:46 2022 -0400 Don't mask implicit mods commit 89dac4c Author: Kostas Karachalios <[email protected]> Date: Tue Jun 28 09:43:02 2022 +0200 Add some whitespace for clarity commit e96f516 Merge: 2cac694 ef3eb33 Author: Kostas Karachalios <[email protected]> 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 <[email protected]> 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 <[email protected]> Co-authored by: urob <[email protected]>
Squashed commit of the following: commit 386b5d1 Author: urob <[email protected]> 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 <[email protected]> Date: Sun Jul 24 11:08:39 2022 -0400 Explain how to fully disable masked_mods commit 00a0235 Author: urob <[email protected]> Date: Sun Jul 24 11:05:44 2022 -0400 Add mod-morph tests commit bfba42f Author: urob <[email protected]> Date: Sun Jul 24 02:17:34 2022 -0400 Fix doc formatting commit 67412ed Author: urob <[email protected]> Date: Sun Jul 24 01:50:54 2022 -0400 Fix clang-format commit ebc127d Author: urob <[email protected]> Date: Sun Jul 24 01:04:13 2022 -0400 Update docs for mod-morph commit 44297de Author: urob <[email protected]> Date: Sun Jul 24 00:24:18 2022 -0400 Set masked-mods to mods if unspecified commit 7c647b0 Author: urob <[email protected]> Date: Mon Jul 18 20:31:24 2022 -0400 Trigger-mods are unused commit 4cf66a4 Author: urob <[email protected]> Date: Mon Jul 18 19:57:46 2022 -0400 Don't mask implicit mods commit 89dac4c Author: Kostas Karachalios <[email protected]> Date: Tue Jun 28 09:43:02 2022 +0200 Add some whitespace for clarity commit e96f516 Merge: 2cac694 ef3eb33 Author: Kostas Karachalios <[email protected]> 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 <[email protected]> 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
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 themods
option, and allows the user to configure which of the mods will be swallowed when the morph triggers.Behavior without
masked_mods
&mmlt
results in(
L-shift + &mmlt
results inL-shift + ,
, which is interpreted as<
R-shift + &mmlt
results inR-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)>; }; };
&mmlt
results in(
L-shift + &mmlt
results in,
(theL-shift
is swallowed)R-shift + &mmlt
results inR-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:
masked_mods
"Testers that are willing to help are more than welcome.
TODO