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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cac694
feat(behaviors): Allow mod-morph to swallow mods
vrinek Feb 3, 2022
e96f516
Merge remote-tracking branch 'origin/main' into masked-mod-morphs-unt…
vrinek Jun 27, 2022
89dac4c
Add some whitespace for clarity
vrinek Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/dts/bindings/behaviors/zmk,behavior-mod-morph.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,6 @@ properties:
mods:
type: int
required: true
masked_mods:
type: int
required: false
2 changes: 2 additions & 0 deletions app/include/zmk/hid.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,8 @@ int zmk_hid_register_mods(zmk_mod_flags_t explicit_modifiers);
int zmk_hid_unregister_mods(zmk_mod_flags_t explicit_modifiers);
int zmk_hid_implicit_modifiers_press(zmk_mod_flags_t implicit_modifiers);
int zmk_hid_implicit_modifiers_release();
int zmk_hid_masked_modifiers_set(zmk_mod_flags_t masked_modifiers);
int zmk_hid_masked_modifiers_clear();
int zmk_hid_keyboard_press(zmk_key_t key);
int zmk_hid_keyboard_release(zmk_key_t key);
void zmk_hid_keyboard_clear();
Expand Down
6 changes: 6 additions & 0 deletions app/src/behaviors/behavior_mod_morph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ struct behavior_mod_morph_config {
struct zmk_behavior_binding normal_binding;
struct zmk_behavior_binding morph_binding;
zmk_mod_flags_t mods;
zmk_mod_flags_t masked_mods;
};

struct behavior_mod_morph_data {
Expand All @@ -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;
Copy link

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.

Copy link
Author

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?

Copy link
Contributor

@urob urob Jun 22, 2022 •
edited
Loading

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 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

Copy link

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.

Copy link
Author

@vrinek vrinek Jun 27, 2022 •
edited
Loading

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 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?

zmk_hid_masked_modifiers_set(cfg->masked_mods);
data->pressed_binding = (struct zmk_behavior_binding *)&cfg->morph_binding;
} else {
data->pressed_binding = (struct zmk_behavior_binding *)&cfg->normal_binding;
Expand All @@ -64,6 +67,7 @@ static int on_mod_morph_binding_released(struct zmk_behavior_binding *binding,

struct zmk_behavior_binding *pressed_binding = data->pressed_binding;
data->pressed_binding = NULL;
zmk_hid_masked_modifiers_clear();
return behavior_keymap_binding_released(pressed_binding, event);
}

Expand All @@ -88,6 +92,8 @@ static int behavior_mod_morph_init(const struct device *dev) { return 0; }
.normal_binding = _TRANSFORM_ENTRY(0, n), \
.morph_binding = _TRANSFORM_ENTRY(1, n), \
.mods = DT_INST_PROP(n, mods), \
.masked_mods = COND_CODE_0(DT_INST_NODE_HAS_PROP(n, masked_mods), (0), \
(DT_INST_PROP(n, masked_mods))), \
}; \
static struct behavior_mod_morph_data behavior_mod_morph_data_##n = {}; \
DEVICE_DT_INST_DEFINE(n, behavior_mod_morph_init, device_pm_control_nop, \
Expand Down
24 changes: 21 additions & 3 deletions app/src/hid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ static struct zmk_hid_consumer_report consumer_report = {.report_id = 2, .body =
// Only release the modifier if the count is 0.
static int explicit_modifier_counts[8] = {0, 0, 0, 0, 0, 0, 0, 0};
static zmk_mod_flags_t explicit_modifiers = 0;
static zmk_mod_flags_t implicit_modifiers = 0;
static zmk_mod_flags_t masked_modifiers = 0;

#define SET_MODIFIERS(mods) \
{ \
keyboard_report.body.modifiers = mods; \
keyboard_report.body.modifiers = (mods | implicit_modifiers) & ~masked_modifiers; \
LOG_DBG("Modifiers set to 0x%02X", keyboard_report.body.modifiers); \
}

Expand Down Expand Up @@ -136,13 +138,29 @@ static inline int deselect_keyboard_usage(zmk_key_t usage) {
} \
}

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

int zmk_hid_implicit_modifiers_release() {
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) {
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() {
masked_modifiers = 0;
zmk_mod_flags_t current = GET_MODIFIERS;
SET_MODIFIERS(explicit_modifiers);
return current == GET_MODIFIERS ? 0 : 1;
Expand Down
5 changes: 5 additions & 0 deletions docs/docs/behaviors/mod-morph.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ The Mod-Morph behavior acts as one of two keycodes, depending on if the required

When the modifier is being held it is sent along with the morphed keycode. This can cause problems when the morphed keycode and modifier have an existing relationship (such as `shift-delete` or `ctrl-v` on many operating systems).

As a remedy, you can add the optional attribute `masked_mods`, containing
the bitwise OR of the modifiers that should be disabled while the key is held,
so that they are not included in the report sent to the host.

### Configuration

An example of how to implement the mod-morph "Grave Escape":
Expand All @@ -29,6 +33,7 @@ An example of how to implement the mod-morph "Grave Escape":
#binding-cells = <0>;
bindings = <&kp ESC>, <&kp GRAVE>;
mods = <(MOD_LGUI|MOD_LSFT|MOD_RGUI|MOD_RSFT)>;
masked_mods = <(MOD_LGUI|MOD_LSFT)>; // don't send left modifiers
};
};

Expand Down