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.

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(core): Adding pre-release for keys that were already pressed. #1828

Merged
merged 3 commits into from
Oct 2, 2023

Conversation

andrewjrae
Copy link
Contributor

There are a couple of cases where a user may end up rolling two physical keys that happen to map to the same keycode. When this happens, we register a second key press, but this does nothing at the OS level, meaning only one key press will be registered by the computer.

The two cases I'm aware of are:

  1. Rolling the key-repeat behavior (Key repeat cannot be rolled #1207)
  2. Rolling symbols that map to the same keycode (Feature Request: Better handling of keys with the same keycode but different modifiers #1076)

This PR solves both of these issues by simply releasing the keycode before it is pressed again.

I've also added some test suites to cover this behavior, though they rely on the new print out when pre-releasing keys in order to be effective.

This requires further testing, currently I'm running this without issue, but more users testing on different setups is needed. I'm curious if we're going to need larger delay between the releases and presses for some setups (remote desktops typically are sensitive to this kind of thing).

@andrewjrae andrewjrae requested a review from a team as a code owner June 7, 2023 00:53
@caksoylar
Copy link
Contributor

I have been testing these changes this week and everything is working as expected. It seems to fix #1076 but I haven't tried #1207. It works well with remote desktop too (at least with clients that didn't have the issues in #759).

@andrewjrae heads up that one test tests/modifiers/explicit/kp-lctl-dn-lctl-dn-lctl-up-lctl-up is failing which I reproduced locally too, so it might need a look.

@caksoylar caksoylar added enhancement New feature or request core Core functionality/behavior of ZMK labels Jun 10, 2023
@dlip
Copy link

dlip commented Jul 13, 2023

I just tested this and it works well for the case the key is a regular &kp.
This may be a separate issue, but I had issues with combos, so if you have a combo for the same letter on both halves and you rolled or pressed them together, it would only output 1 letter.
To give you some more context as to what I'm trying to achieve, I'm implementing the Taipo layout which has 2 mirrored halves that you are meant to alternate between for each keypress. Here is my repo. The only work around I've found so far is to pair both halves via bluetooth separately, but then layers/sticky keys don't work correctly.

@dlip
Copy link

dlip commented Aug 11, 2023

I opened #1899 as a separate issue for the combo problem

@petejohanson
Copy link
Contributor

@andrewjrae Did you see the comment from @caksoylar regarding the test failures?

@andrewjrae
Copy link
Contributor Author

andrewjrae commented Sep 8, 2023

I did @petejohanson, I haven't dug into it though, I was going to cover that when I did the renaming. I can try to get to this week after next, but if you're interested in pushing this through feel free to take the reigns.

Edit: I just realized this is a different PR than I initially thought lol, the dangers of answering on your phone I guess. Anyways, the point remains that I'll try and take a look later this week.

@vvv-ca
Copy link

vvv-ca commented Sep 16, 2023

@andrewjrae it looks like this pre-releases modifiers. Is this intended?
I believe the kp-lctl-dn-lctl-dn-lctl-up-lctl-up test fails because it tests rolling modifiers and errors out with " zmk: Tried to unregister modifier 0 too often".

Added cases for the two use cases I know of:
    1. Rolling with key-repeat behavior
    2. Rolling symbols that have the same base key, eg `+=`
@andrewjrae andrewjrae force-pushed the keycode-pre-releasing branch 2 times, most recently from e38ff44 to d4933e3 Compare September 16, 2023 21:29
@andrewjrae
Copy link
Contributor Author

andrewjrae commented Sep 16, 2023

Thanks for looking @vvv-ca.
It was initially intended to pre-release any key which had been hit more than once, but looking at the code this will mess with the mod tracking thats done in hid.c and we reach this issue.

Likely mods should be excluded from the pre-release code. I added another commit to prevent this from happening, and that test is now passing. It would be prettied up by a IS_MODIFIER_KEYCODE macro though, any thoughts on adding that somewhere @petejohanson, and if so where?

@petejohanson petejohanson self-assigned this Sep 24, 2023
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment on the code itself, thanks for the test on this!

app/src/hid_listener.c Outdated Show resolved Hide resolved
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@petejohanson petejohanson merged commit 2f05ad5 into zmkfirmware:main Oct 2, 2023
41 checks passed
@andrewjrae andrewjrae deleted the keycode-pre-releasing branch October 3, 2023 20:10
aaronkollasch added a commit to aaronkollasch/zmk-chocofi that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants