Sticky keys behavior (one-shot mods and one-shot layers) #284

okke-formsma · 2020-10-16T08:02:06Z

implements sticky keys for #160

okke-formsma · 2020-11-07T21:05:10Z

@petejohanson do you know of a way to test the sensors in our test suite? It's not even compiling on a board with sensors like kyria even though the normal test suite succeed.

okke-formsma · 2020-11-11T16:09:19Z

rebased onto latest main, please review.

app/dts/behaviors/sticky_key.dtsi

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

innovaker · 2020-11-11T23:00:12Z

Can we please not bundle vaguely related commits (the mass license headers) into PRs? A decent git tool will let you fire off a second PR with ease.

petejohanson · 2020-11-12T04:35:10Z

I'd second @innovaker's request. Please do the header add (which is welcome!) as a fresh PR. Thanks! In the meantime, I'll review the code from the other two commits.

okke-formsma · 2020-11-12T08:13:03Z

Can we focus on the code instead of bikeshedding which commits are in this PR?

Frankly I'm a bit bummed out about this thing being open for six weeks and only getting comments about license headers and trivial commits that could be split out to another PR. I don't think that's the thing we should focus on, especially considering the limited time each of us has available.

I'll split the header commit out to another PR after the reviews are done, so I can batch all changes in one go instead of working on this N times.

innovaker · 2020-11-12T10:16:42Z

@okke-formsma, your contributions are always most appreciated and we're grateful for your patience. As someone who rebases daily, I can appreciate how frustrating it can be when PRs linger. With greatest respect though, please keep in mind that as maintainers and reviewers we also have competing priorities and other concerns that might not be immediately obvious. Also remember that doing a thorough review of an entirely new feature requires a significant block of time. Tidying up a PR takes far less time in comparison, yet makes it significantly easier (faster) for reviewers, which increases the chances of it being reviewed sooner. So far you've had cursory feedback - as in, we've done a quick scan and commented on any quick wins, as we do with any other PR - so that we can find an appropriate block of time for studying it in detail.

innovaker

I've reviewed the first commit, will do the second later.

6348afb:

Looks fine, only minor suggestions.
Commit title: feat(events): add timestamp to keycode_state_changed and sensor_event
- Please be specific. I had to dig around a little to gather it was only two of the event types.
Commit message: dueto > due to
Can confirm this commit is formatted and passes existing tests.

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

innovaker

Did a quick pass of eca8a2c, I'll let @petejohanson do the deep dive.

app/dts/bindings/behaviors/zmk,behavior-sticky-key.yaml

app/src/behaviors/behavior_sticky_key.c

These timestamps are necessary to correctly deal with delayed events due to hold-tap shenanigans.

okke-formsma · 2020-11-15T12:24:46Z

Thanks for the review @innovaker, I've made most fixes you proposed except for the rename of the constants.

petejohanson

A few review thoughts/questions.

app/src/behaviors/behavior_sticky_key.c

okke-formsma · 2020-11-18T20:03:26Z

Repushed with updated type signatures. Also renamed the array with active sticky keys to active_sticky_keys from active_sticky_key

okke-formsma · 2020-11-24T19:39:20Z

@petejohanson I've addressed all cancerns, can you have a look some time?

petejohanson

Few more minor follow up.s

app/src/behaviors/behavior_sticky_key.c

petejohanson · 2020-11-25T05:28:16Z

app/src/behaviors/behavior_sticky_key.c

+            continue;
+        }
+
+        if (strcmp(sticky_key->config->behavior.behavior_dev, "KEY_PRESS") == 0 &&


Hmm... I don't love keying off this label, but this might be the pragmatic way to do this. Any other options you'd considered?

Previously I had this keyed on key_position, but that requires keycode_state_changed to gain a position field too.

while implementing this, I ran into the issue with

static int on_sensor_binding_triggered(struct zmk_behavior_binding *binding, struct device *sensor, s64_t timestamp) {

a sensor does not have a "position", so the keycode_state_changed event can't have a position value.

instead we could also compare the timestamp, that's probably unique and matching too. May be a bit obscure though ...

Let's leave it then, and consider circling back on this later.

petejohanson · 2020-11-25T05:33:05Z

app/src/behaviors/behavior_sticky_key.c

+struct behavior_sticky_key_data {};
+static struct behavior_sticky_key_data behavior_sticky_key_data;
+
+#define _TRANSFORM_ENTRY(idx, node)                                                                \


Do we need to extra this somewhere? Seems like this is duplicated from the keymap code.

yeah it's copied in hold taps too. Mind if we commit this as-is and refactor the three common usages out in a separate PR?

Fine by me.

petejohanson

Just one minor comment, not worth holding this up.

petejohanson · 2020-11-28T05:53:50Z

app/src/behaviors/behavior_sticky_key.c

+
+LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);
+
+#if DT_NODE_EXISTS(DT_DRV_INST(0))


This should really be:

#if DT_HAS_COMPAT_STATUS_OKAY(DT_DRV_COMPAT)

petejohanson · 2020-11-28T05:57:45Z

app/src/behaviors/behavior_sticky_key.c

+            continue;
+        }
+
+        if (strcmp(sticky_key->config->behavior.behavior_dev, "KEY_PRESS") == 0 &&


Let's leave it then, and consider circling back on this later.

innovaker added behaviors enhancement New feature or request labels Oct 16, 2020

okke-formsma force-pushed the sticky-keys branch from a157f59 to bd36023 Compare October 16, 2020 17:51

okke-formsma changed the title ~~Sticky keys behavior~~ Sticky keys behavior (one-shot mods and one-shot layers) Oct 27, 2020

petejohanson added the hacktoberfest-accepted label Oct 30, 2020

okke-formsma force-pushed the sticky-keys branch from bd36023 to 252ff14 Compare November 6, 2020 20:05

okke-formsma force-pushed the sticky-keys branch 3 times, most recently from 64bf16a to 897320e Compare November 11, 2020 16:09

okke-formsma force-pushed the sticky-keys branch from 897320e to ddcba37 Compare November 11, 2020 18:28

petejohanson reviewed Nov 11, 2020

View reviewed changes

app/dts/behaviors/sticky_key.dtsi Show resolved Hide resolved

app/include/zmk/events/keycode-state-changed.h Show resolved Hide resolved

okke-formsma force-pushed the sticky-keys branch from ddcba37 to 310925b Compare November 11, 2020 21:10

innovaker reviewed Nov 12, 2020

View reviewed changes

app/include/zmk/events/keycode-state-changed.h Outdated Show resolved Hide resolved

innovaker self-requested a review November 12, 2020 20:20

innovaker reviewed Nov 13, 2020

View reviewed changes

okke-formsma mentioned this pull request Nov 15, 2020

add license headers to behavior dtsi files #388

Merged

okke-formsma force-pushed the sticky-keys branch 2 times, most recently from eca8a2c to 6fbe84c Compare November 15, 2020 12:10

feat(events): add timestamp to keycode_state_changed and sensor_event

a27b4e8

These timestamps are necessary to correctly deal with delayed events due to hold-tap shenanigans.

okke-formsma force-pushed the sticky-keys branch from 6fbe84c to 462804c Compare November 15, 2020 12:24

petejohanson reviewed Nov 17, 2020

View reviewed changes

feat(sticky keys): initial implementation

cd29f36

okke-formsma force-pushed the sticky-keys branch from 462804c to cd29f36 Compare November 18, 2020 20:02

petejohanson reviewed Nov 25, 2020

View reviewed changes

petejohanson approved these changes Nov 28, 2020

View reviewed changes

petejohanson merged commit 76a6d7b into zmkfirmware:main Nov 28, 2020

This was referenced Nov 28, 2020

Feature: sticky keys (one shot keys) #160

Closed

clean up sticky keys loose ends #447

Closed

okke-formsma mentioned this pull request Dec 12, 2020

Refactor _TRANSFORM_ENTRY #506

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sticky keys behavior (one-shot mods and one-shot layers) #284

Sticky keys behavior (one-shot mods and one-shot layers) #284

okke-formsma commented Oct 16, 2020

okke-formsma commented Nov 7, 2020

okke-formsma commented Nov 11, 2020

innovaker commented Nov 11, 2020

petejohanson commented Nov 12, 2020

okke-formsma commented Nov 12, 2020

innovaker commented Nov 12, 2020 •

edited

innovaker left a comment

innovaker left a comment

okke-formsma commented Nov 15, 2020

petejohanson left a comment

okke-formsma commented Nov 18, 2020

okke-formsma commented Nov 24, 2020

petejohanson left a comment

petejohanson Nov 25, 2020

okke-formsma Nov 26, 2020

okke-formsma Nov 26, 2020 •

edited

petejohanson Nov 28, 2020

petejohanson Nov 25, 2020

okke-formsma Nov 27, 2020

petejohanson Nov 27, 2020

petejohanson left a comment

petejohanson Nov 28, 2020

petejohanson Nov 28, 2020


		LOG_MODULE_DECLARE(zmk, CONFIG_ZMK_LOG_LEVEL);

		#if DT_NODE_EXISTS(DT_DRV_INST(0))

Sticky keys behavior (one-shot mods and one-shot layers) #284

Sticky keys behavior (one-shot mods and one-shot layers) #284

Conversation

okke-formsma commented Oct 16, 2020

okke-formsma commented Nov 7, 2020

okke-formsma commented Nov 11, 2020

innovaker commented Nov 11, 2020

petejohanson commented Nov 12, 2020

okke-formsma commented Nov 12, 2020

innovaker commented Nov 12, 2020 • edited

innovaker left a comment

Choose a reason for hiding this comment

innovaker left a comment

Choose a reason for hiding this comment

okke-formsma commented Nov 15, 2020

petejohanson left a comment

Choose a reason for hiding this comment

okke-formsma commented Nov 18, 2020

okke-formsma commented Nov 24, 2020

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

okke-formsma Nov 26, 2020 • edited

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

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

innovaker commented Nov 12, 2020 •

edited

okke-formsma Nov 26, 2020 •

edited