bugfix: sticky keys held for longer than release-after-ms get stuck

[metaeaux]

Sticky keys work to modify the next keypress, but also have utility in being held to achieve certain behaviours such as making multiple selections with a mouse. The current implementation of sticky keys does not account for the case where a sticky key is held longer than release-after-ms. In this case, the sticky key gets stuck.

This PR proposes releasing the sticky key 1ms after the key is released if release-after-ms has lapsed during the hold, so that the sticky key behaves like a momentary key for long holds.

I have tested this for the past few days and have found it fixes my use cases.

Board/Shield Check-list

• This board/shield is tested working on real hardware
• Definitions follow the general style of other shields/boards upstream (Reference)
• .zmk.yml metadata file added
• Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
• General consistent formatting of DeviceTree files
• Keymaps do not use deprecated key defines (Check using the upgrader tool)
• &pro_micro used in favor of &pro_micro_d/a if applicable
• If split, no name added for the right/peripheral half
• Kconfig.defconfig file correctly wraps all configuration in conditional on the shield symbol
• .conf file has optional extra features commented out
• Keyboard/PCB is part of a shipped group buy or is generally available in stock to purchase (OSH/personal projects without general availability should create a zmk-config repo instead)

[petejohanson]

Thanks for the PR!

This should have an extra unit test added to verify this behavior change works as expected for the new functionality. Please add a new test to cover this. Thanks!

[nguyendown]

This feature (or fix) should be called release-after-hold. I am not sure whether it should be classified as a fix or a feature. Personally, I consider it a bug that needs fixing because I cannot think of any use case for persisting a key press after holding the key.

The fact that no one has ever complained about this for years is questionable. Perhaps some people rely on this behavior, or there may be some use cases that I am unaware of. Should we create a poll?

Here is the current sticky key behavior:

    sk press                   sk release
        ^                           ^
========|---------------------------|===========================
                                      release-after-ms
                                    <------------------>
========|---------------------------|------------------|========
        v                                              v
     kp press                                     kp release

The feature (or fix) starts the timer on sk press instead of sk release:

    sk press                   sk release
        ^                           ^
========|---------------------------|===========================
          release-after-hold-ms
        <----------------------->
========|---------------------------|===========================
        v                           v
     kp press                  kp release

You did not include this change in your pull request, or perhaps I misunderstood what you meant?

Here's a unit test for release-after-hold.
https://github.com/nguyendown/zmk/tree/behaviors/sticky-key-release-after-hold-metaeaux

[theol0403]

Either there was a breaking change in main or the code wasn't properly tested, because this patch doesn't seem to work for me. I think the problem is that stick_key->release_at is only calculated on key release, and the timer is only started then, so ms_left will ALWAYS be release_after_ms.

zmk/app/src/behaviors/behavior_sticky_key.c

Lines 165 to 172 in e7a6e40

	// No other key was pressed. Start the timer.
	sticky_key->timer_started = true;
	sticky_key->release_at = event.timestamp + sticky_key->config->release_after_ms;
	// adjust timer in case this behavior was queued by a hold-tap
	int32_t ms_left = sticky_key->release_at - k_uptime_get();
	if (ms_left > 0) {
	k_work_schedule(&sticky_key->release_timer, K_MSEC(ms_left));
	}

The code there actually confuses me. I am unsure how it is functionally different from just calling k_work_schedule(&sticky_key->release_timer, K_MSEC(sticky_key->config->release_after_ms));. There is the note about hold-taps, but I'm not sure how it applies. Is it that the event timestamp will be from when the hold-tap is released, and not when it decides to release the behaviour? I suppose that's the functionality we want then.

To properly implement the feature, I had to move the timer start code to behaviour pressed, and then I made it release the behavior directly on behaviour released instead of scheduling a 1ms delay:

theol0403@0673d1a

[theol0403]

I think I understand the code now. It was not actually designed to represent the "time left on the timer", instead it was to correct an edge case where you release the physical key but for some reason there is a delay until it processes the behavior release. Perhaps the original PR was actually just fixing a small bug where it won't release the behavior if the time remaining is negative for some reason, but not actually implementing the feature where it will start timing on keypress.

[nguyendown]

fixing a small bug where it won't release the behavior if the time remaining is negative for some reason

The implementation in #1788 will also fix this. Can you try it out?