Qmk_firmware: With eager_pk, fast key stroke will give stucked key event

[elfmimi]

Describe the Bug

When you select eager_pk as your debouncing filter,
if you press and release a key before the DEBOUNCE period ends,
that key will get stucked and keep appears to be held down to the OS and the user.

How to Reproduce

rules.mk:
DEBOUNCE_TYPE=eager_pk
config.h:
#define DEBOUNCE 100
and hit any key real quick.

Additional Context

The bug is introduced with this commit.
https://github.com/qmk/qmk_firmware/commit/d0fb7005d51be7c876e63e87778d080c8a733a13 https://github.com/qmk/qmk_firmware/pull/5621

Here is an example how to fix this.
https://github.com/elfmimi/qmk_firmware/commit/c69f74025dd1a3e16e2d93f2dbfcb1bcb6ec6978

Push notice: @alex-ong

[drashna]

not just eager per key. I've seen this with eager per row, and I think that ZSA (Ergodox EZ) has a number of reports of this too

[alex-ong]

Thanks - yeah as a result of using changed to do cycle optimization this bug was introduced.
The fix looks solid, and would apply to eager per row too, since the algorithm is (gasp) the same

[alex-ong]

@elfmimi could you test to ensure your fix is symmetric?
Feel free to add a pull request if you haven't already. While you're there, could you fix eager_per_row too?

e.g. set define DEBOUNCE 100
Press key for > 100ms
release key, and press again in <100ms
output should be key is pressed, then detects release, then detects press (timing may not line up due to 100ms debounce, but final state should be key is held down.

[drashna]

glad to hear that!

And yeah, @elfmimi could you open a PR with this change?
If you don't want to, let us know and one of us can handle that

[drashna]

Well, worst case, I'm testing out drashna@d9525c0db as that looks to include the changes to both, and I'll report back of that fixes the issues that I've been having.

[elfmimi]

I'd like to leave it to you two.

@alex-ong I checked and it behaved just as you mentioned. the final state is key down.

[drashna]

@alex-ong could you double check my commit?

I'm pretty sure that's correct, but .... I would like a double check.

@elfmimi awesome. And I've just created a PR to address this, for both PK and PR.

[alex-ong]

@drashna code looks right. If it seems like it works (set debounce to say 100 ms and then test it) then it.. probably works :)

[alex-ong]

Was this eventually fixed? Can this be closed?

[drashna]

There is a PR for it, which is waiting on being merged. But yeah, I think it's fixed