initial implementation for modifiers #153

okke-formsma · 2020-09-02T20:31:19Z

Implements #86.

&sft NUM_1 presses shift, then 1, then releases shift, then releases 1.

BrainWart

Thanks, Okke. I saw a couple of things and was wondering what MOD_LAST_MODIFIER was for. I haven't had a chance to test it yet personally.

app/dts/behaviors/modifiers.dtsi

app/include/dt-bindings/zmk/keys.h

app/src/hid.c

CrossR · 2020-09-09T18:31:43Z

Just to duplicate the comment I made on discord, I'm seeing some stuck modifiers using this PR merged with main. I see either my LCTL or LSFT stick after pressing them, and I don't see this behaviour on main.

My config is in a branch here, and I don't believe I'm doing anything especially odd, just defining the keys for my layout.

https://github.com/CrossR/zmk_config/compare/CrossR/ShiftedKeyCodes

I'm seeing the stuck modifiers when using the normal LCTL and LSFT on my default layer, which are unchanged from my normal keymap.

okke-formsma · 2020-09-10T13:37:11Z

I've fixed the bug CrossR was running into, and updated the test so it won't happen again.

CrossR

No stuck mods now! Though I am not seeing any mod applied keypresses, though I believe I see why, assuming I'm following the code correctly.

With the change I've mentioned, "it works on my machine" TM at least.

app/dts/behaviors/modifiers.dtsi

app/src/hid.c

okke-formsma · 2020-10-27T21:04:49Z

app/include/zmk/events/keycode-state-changed.h

-    ev->keycode = keycode;
+    // this is ugly and after merging https://github.com/zmkfirmware/zmk/pull/300
+    // should move into the keycode_state_changed_from_encoded method
+    // discuss: should we keep LCTL and MOD_LCTL even though it causes this ugly code?


I don't like this code here, but I'm not sure it's better to in hid.c.

I think one good reason to remove this from here is to help keep clearly distinct the modifiers that are being held/applied because they are being asked for in addition to a different keycode, and a modifier that truly is just a modifier being pressed/released.

That context I think is important to preserve, because we'll need to when we work to solve the more nuanced handling of modifiers and when we release them because of other key presses, etc.

Thoughts?

Agreed, that's a neat way to separate the implicit/explicit modifiers.

zmkfirmware#86

innovaker · 2020-10-31T21:19:37Z

app/include/dt-bindings/zmk/keys.h

I'd suggest moving the modifiers codes and macros into their own header - perhaps modifiers.h?

For now, modifiers.h can be included into keys.h until the single entry point enhancement for keymaps has been implemented. The includes to keys.h elsewhere in the system (such as in keycode-state-changed.h) can then be replaced by modifiers.h which is a tighter design.

Fully agreed. I'm going to merge this from @okke-formsma , and then follow up with that small refactor.

innovaker · 2020-10-31T21:20:33Z

Thanks @okke-formsma! Just a quick suggestion for now (above), will look closer tomorrow with fresh eyes.

innovaker · 2020-11-01T20:44:47Z

app/include/zmk/hid.h

 int zmk_hid_register_mod(zmk_mod modifier);
 int zmk_hid_unregister_mod(zmk_mod modifier);
-int zmk_hid_register_mods(zmk_mod_flags modifiers);
-int zmk_hid_unregister_mods(zmk_mod_flags modifiers);
+int zmk_hid_implicit_modifiers_press(zmk_mod_flags implicit_modifiers);
+int zmk_hid_implicit_modifiers_release();


I'm not sure if the modifier implicit/explicit/counting state and related functions should live within hid.h/c.

Rationale:

it reduces the cohesion of the hid system/module.

there's weak coupling to the rest of the hid module.

it feels like a conceptual layer above hid.

the modifiers system will likely be part of or linked to the internal state subsystem(s) that have been discussed recently.

hid.c itself can probably be refactored into hid_keyboard.c and hid_consumer.c

Perhaps it's better suited to a dedicated module, say modifiers.h/c or hid_modifiers.h/c?

I think this is fine for now, and we can refactor if/when we have a plan/need for the state changes.

innovaker · 2020-11-01T20:51:44Z

app/src/hid.c

+int zmk_hid_register_mod(zmk_mod modifier) {
+    explicit_modifier_counts[modifier]++;
+    LOG_DBG("Modifier %d count %d", modifier, explicit_modifier_counts[modifier]);
+    WRITE_BIT(explicit_modifiers, modifier, true);
+    SET_MODIFIERS(explicit_modifiers);


If we're going for explicit and implicit, let's be consistent throughout?

petejohanson

Thanks for this! I've been playing with it the past couple hours, and it seems really solid. Definitely good enough we should get this in, and pound on it a bit, get the HID refactor from @innovaker in with the nice defines for common shifted codes, and call it a day. Thanks again!

petejohanson · 2020-11-03T05:18:02Z

app/include/dt-bindings/zmk/keys.h

Fully agreed. I'm going to merge this from @okke-formsma , and then follow up with that small refactor.

BrainWart reviewed Sep 3, 2020

View reviewed changes

app/dts/behaviors/modifiers.dtsi Outdated Show resolved Hide resolved

app/include/dt-bindings/zmk/keys.h Outdated Show resolved Hide resolved

app/src/hid.c Outdated Show resolved Hide resolved

okke-formsma force-pushed the modifiers branch from c7dfbce to 29d66e3 Compare September 4, 2020 18:08

This was referenced Sep 4, 2020

Feature: layer-tap behavior #49

Closed

Feature: "Shifted Keycodes", e.g. assigning ! to a key press or mod-tap behavior. #86

Closed

CrossR reviewed Sep 10, 2020

View reviewed changes

app/dts/behaviors/modifiers.dtsi Outdated Show resolved Hide resolved

app/dts/behaviors/modifiers.dtsi Outdated Show resolved Hide resolved

app/src/hid.c Outdated Show resolved Hide resolved

app/src/hid.c Outdated Show resolved Hide resolved

okke-formsma force-pushed the modifiers branch from ead705a to 63e18ce Compare September 15, 2020 09:30

This was referenced Oct 2, 2020

Feature: Unicode #232

Open

feat: HID Usages + Keys Standardisation #236

Merged

okke-formsma force-pushed the modifiers branch from 63e18ce to 7f0409b Compare October 16, 2020 12:29

krikun98 mentioned this pull request Oct 20, 2020

CONFIG_ZMK_USB interferes with CONFIG_ZMK_SLEEP #287

Closed

okke-formsma force-pushed the modifiers branch 2 times, most recently from 423f342 to ef9ada3 Compare October 27, 2020 21:02

okke-formsma commented Oct 27, 2020

View reviewed changes

okke-formsma force-pushed the modifiers branch from ef9ada3 to d4b6ee0 Compare October 31, 2020 13:40

initial implementation for modifiers

9c2b21d

zmkfirmware#86

okke-formsma force-pushed the modifiers branch from d4b6ee0 to 9c2b21d Compare October 31, 2020 14:08

innovaker self-requested a review October 31, 2020 16:27

clang-format

6868263

innovaker reviewed Oct 31, 2020

View reviewed changes

innovaker reviewed Nov 1, 2020

View reviewed changes

petejohanson approved these changes Nov 3, 2020

View reviewed changes

petejohanson merged commit 55b5936 into zmkfirmware:main Nov 3, 2020

innovaker mentioned this pull request Nov 3, 2020

Unify HID usage page approach to avoid consumer page crazy. #300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial implementation for modifiers #153

initial implementation for modifiers #153

okke-formsma commented Sep 2, 2020

BrainWart left a comment

CrossR commented Sep 9, 2020

okke-formsma commented Sep 10, 2020

CrossR left a comment

okke-formsma Oct 27, 2020

petejohanson Oct 27, 2020

okke-formsma Oct 28, 2020

innovaker Oct 31, 2020

petejohanson Nov 3, 2020

innovaker commented Oct 31, 2020

innovaker Nov 1, 2020 •

edited

Loading

petejohanson Nov 1, 2020

innovaker Nov 1, 2020

petejohanson left a comment

petejohanson Nov 3, 2020

initial implementation for modifiers #153

initial implementation for modifiers #153

Conversation

okke-formsma commented Sep 2, 2020

BrainWart left a comment

Choose a reason for hiding this comment

CrossR commented Sep 9, 2020

okke-formsma commented Sep 10, 2020

CrossR 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

innovaker commented Oct 31, 2020

innovaker Nov 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petejohanson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

innovaker Nov 1, 2020 •

edited

Loading