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

Debounce with EZ causes lots of duplicate presses #6269

Closed
kthibodeaux opened this issue Jul 6, 2019 · 50 comments
Closed

Debounce with EZ causes lots of duplicate presses #6269

kthibodeaux opened this issue Jul 6, 2019 · 50 comments

Comments

@kthibodeaux
Copy link

kthibodeaux commented Jul 6, 2019

Disclaimer: I have no idea what debounce even means.

I have had an old clone of the QMK firmware I have been using that was checked out about 8 months ago. I recently switched to a different OS so I now have the latest QMK.

When I made a change to my EZ layout and flashed my keyboard I noticed I get a lot of duplicate key presses. I change debounce to 10 and then 12, that helped a little, but I am still seeing the issue. I switched back to the old sha for now (cd544e10a) and everything seems to be working perfectly (without me needing to change the debounce).

My layout can be found here: https://github.com/kthibodeaux/ergodox-layout

Is there a quick fix that I'm missing to allow my keyboard to work on newer versions on QMK?

Edit: It seems switching it to 15 has totally resolved the problem. With the new default being 5, is there something else I could do to fix this? I assume 15 is no longer the default for a reason, but it is the only way my keyboard will work.

Edit 2: It is still happening, although much less often. I would say without changing the settings it happens every few words, and with the debounce at 15 it happens every few paragraphs. I'm going to switch to that old sha after all. What a roller coaster!

kthibodeaux added a commit to kthibodeaux/ergodox-layout that referenced this issue Jul 6, 2019
kthibodeaux added a commit to kthibodeaux/ergodox-layout that referenced this issue Jul 6, 2019
@drashna
Copy link
Member

drashna commented Jul 9, 2019

That's because there are two PRs that need to land to fix these issues.

The main one is: #6081
This one fixes a bug in the Eager debouncing algorithms. The Ergodox (OG and EZ) use the Eager Per Row algo, which is part of the problem.

Additionally, I suspect that the i2c_master code has an issue when optimization is enabled for it. This PR should fix it:
#5617

If you apply both of these PRs to your branch, it may resolve the issues that you're seeing outright. Or at least, help drastically!

@kthibodeaux
Copy link
Author

I did a lot of typing and it seems to have helped a lot.

In this comment I'm making I have had to correct 5+ double types.

Thank you for the fast response!

@danielo515
Copy link
Contributor

I also faced this problem. It happens specially on keys that are harder to reach or that you hit with a weaker finger like the pinkie.
I'm on the exact same situation as you. I tried also tunning debounce and changing the settings of repeat keys on my OS, but none of those are a satisfactory solution.

@danielo515
Copy link
Contributor

I just merged the latest master into my fork and seems that the problems with debounce has been fixed. The only problem is that firmware size has increased (again).
I'll setup my keyboard repeat back to normal and see how good it goes.

@kthibodeaux
Copy link
Author

kthibodeaux commented Jul 31, 2019

I just tried the latest and I am still see the issue, and quite often (3 times in this comment)) - make that 4 😆

Edit: I am not noticing a pattern to the keys affected. Letters, numbers, keys on layers, tap dancing thumb keys, they are all double typing.

@danielo515
Copy link
Contributor

The patter I notice is when my finger is "tired" or it is weak (like the pinky), then I usually get double taps

@drashna
Copy link
Member

drashna commented Aug 1, 2019

@danielo515 Yeah, feature creep. :(
And for the duplicate presses, that's an actual debounce issue. Setting that higher may help.

@kthibodeaux That's very odd. Try increasing your debounce setting? Or try adding DEBOUNCING_TYPE = sym_g to your rules.mk and see if that helps.

@danielo515
Copy link
Contributor

Today I noticed something... let's say weird.
This debounce problem seems to not be just tied to our potential problems we have as humans, but something more deep on the code. Today I got a debounce error with a sendstring macro. I got two accented characters like this: éé. The macro is quite complex because it uses the alt key to generate the international keycode. Not sure how that is even possible

@danielo515
Copy link
Contributor

I don't think increasing debounce could help.
Mine was already 11 and I was still getting issues. Making it higher makes my computer feel slow.
I'll try that other debouncing type to see how good it goes.

@ethanspitz
Copy link
Contributor

I'm having this issue with a handwired dactyl. Tried all the debounce algorithms, bringing the time up, etc. Certain things made it worse than others, and bringing up the debounce time helped slightly.

I did notice a pattern though, it's mostly (if not only) on the top row of the slave (right). I have a pro micro (elite-c actually) on each side communicating using I2C. I'm pretty close to main and I don't see any commits since I last grabbed main that could have helped.

I also noticed something odd. When I plug the cable into the right half, the top row doesn't work on that side anymore. Not sure if it's related or some sort of bug in the code.

My configurations are here: https://github.com/ethanspitz/qmk_firmware/tree/master/keyboards/handwired/dactyl_promicro

@danielo515
Copy link
Contributor

I am on the latest version of QMK and it is still being an issue.
This is getting worse every day and make me less productive with my keyboard rather than more.
Did any of the involved ones on this issue found a solution? In which firmware version worked for you? I'm willing to go to that one and just re-apply my changes there.

@kthibodeaux
Copy link
Author

@danielo515 I build my changes on top of sha cd544e10a and it works great for me

@kthibodeaux
Copy link
Author

kthibodeaux commented Aug 21, 2019

@drashna DEBOUNCING_TYPE = sym_g combined with a debounce of 15 (with latest master) has reduced the errors greatly, but it is still unusable

@drashna
Copy link
Member

drashna commented Aug 22, 2019

@alex-ong Sorry for tagging you again, but it looks like there are still some issues with the debounce code.

Unfortunately, I don't really see this on my board.... So my ability to help here is limited.

@alex-ong
Copy link
Contributor

alex-ong commented Aug 22, 2019

I don't have an ez either haha. There is a trade-off between responsiveness and debounce.

Using sym_g with 15 should be the same as before I ever touched it; which from memory works perfectly fine, though people complained about slow responsiveness. Theoretically sym_g should NEVER give you extra presses, though it should have a weird latency feel to it.

Using eager_pr/pk can give you ghost presses since it has no emi protection; you can get presses without pressing anything. I've never experienced this.

Ergodox is problematic because of the incredibly slow scan rate due to i2c bottleneck. Eager_pk with 500hz scan rate and 5-8ms debounce should always work, but eager_pk kills your scan rate to 133hz. This is why we switched to eager_pr, which uses a timer per physical column, and leaves scan rate at around 300hz.

Changes in other parts of qmk have in general made the codebase slower, reducing scanrate over time. Rgb is another thing that reduces scanrate.

I actually don't understand how eager_pr /pk could give ghost presses. I've never seen switches that need more than 5ms debounce. The code is simple ; when you press a key, send a press then block all changes for DEBOUNCE ms. It is literally impossible to get a double press unless your physical switch sent two presses greater than (debounce*2) Ms apart. If you get a double press with DEBOUNCE = 15… then debounce.c is receiving press - 15ms - release -15ms - press

Sym_g is even more un-double inputtable; it waits for completely stable input from the entire keyboard for DEBOUNCE ms. If a change occurs while it's in the frozen state, it resets it's timer and waits even more. To get a double press in sym_g, debounce.c is receiving a press, 15ms of same input, then release, 15ms of same input, then press, with another 15ms of input.

We should verify what the undebounced input looks like; if someone could log a time-stamped (Ms elapsed) / raw button state that could help.

Another thought (along the "debounce is perfect with no bugs, the input is the problem" theory) is that at some point the 5 cycle / 2 nop delay between selecting/reading pins got removed. This might give raw data as noise? It was removed (the nop thing) as talking to the i2c had an incurred 5-10 cycle wait already.

@kthibodeaux - what do you mean by unusable, (after using sym_g)? Latency or erroneous (extra) presses? Extra keypresses shouldn't be possible with sym_g

@kthibodeaux
Copy link
Author

kthibodeaux commented Aug 22, 2019

@alex-ong Extra keypresses (with sym_g and debounce of 15). More rare, but still happens

We should verify what the undebounced input looks like; if someone could log a time-stamped (Ms elapsed) / raw button state that could help.

Is there any documentation on how I could do that?

Could it be a combination of settings that I have? I am using permissive hold, tap dancing, and set my tap dance term to 100. All of this works great on sha cd544e10a (it could work in some later commits as well, this just happens to be one that I've been sticking with).

@danielo515
Copy link
Contributor

What about asking ergodox_ez company members to have a look? At the end it is their hardware and their clients are going to face this problem sooner or later.
I would love to help also with the error report, because obviously the theory is far from matching the practice, but I need instructions on how to do this.
Just to be clear, this was not happening before.
I'm also using tap_dance and leader key, nothing else.

@drashna
Copy link
Member

drashna commented Aug 22, 2019

@danielo515 Well, that's what makes this complicated. The Ergodox board design is open source, and that's what Ergodox EZ is using. This issue effects any of the boards that use that design.

The simplest fix here, is to pull the matrix for the old commit and ues that. (eg run git checkout cd544e10a keyboards/ergodox_ez/matrix.c, I believe).

However, as this affects more than just Ergodox EZ's boards, I would rather that we pursue a proper solution that works in all cases, rather than just reverting code. Especially as there are ~6 or so other boards that use the same hardware setup that the Ergodox does.

As for ZSA (the Ergodox EZ company) looking into the issue, well, that's why I'm monitoring this issue, and doing what I can to get a good resolution. Absolute worst case is that we revert the code. But I would rather avoid that if at all possible.

@alex-ong
Copy link
Contributor

@danielo515

Just to be clear, this was not happening before.

Do you know which commit does work? i'm more than happy to compare matrix.c's and debouncing

@danielo515
Copy link
Contributor

According to @kthibodeaux cd544e10a works fine

@kthibodeaux
Copy link
Author

Yep, cd544e10a works fine.

I am not saying that is the latest working commit; it just happens to be the random commit one of my computers had checked out because I rarely pull on it. I attempted to git bisect to try to find exactly when this started, but I had no luck with that.

@danielo515
Copy link
Contributor

If I try to bring stuff from that commit to my branch I get this kind of errors:

keyboards/ergodox_ez/matrix.c: In function 'select_row':
keyboards/ergodox_ez/matrix.c:358:13: error: too many arguments to function 'i2c_stop'
             i2c_stop(ERGODOX_EZ_I2C_TIMEOUT);

So probably not as simple as just making a checkout of some files

@alex-ong
Copy link
Contributor

alex-ong commented Aug 30, 2019

Ahh if you're running out of space thats a bit tough; but using CONSOLE_ENABLE is the right way.

I'll try and implement the debouncing code verbatim; if it works then that would be great. Then we could attempt to figure out what's wrong with eager_pr/pk. Unfortunately I don't have the keyboard so can't test it...

@alex-ong
Copy link
Contributor

alex-ong commented Aug 30, 2019

@kthibodeaux:

Try pulling this: https://github.com/alex-ong/qmk_firmware/tree/ergoez_debounce
It compiles but i havent tested it. I copy pasted the code almost verbatim. Let me know how it goes.
Here's the diff, it should be pretty easy to paste the new file and change a config 👍

master...alex-ong:ergoez_debounce

If it works then its time to figure out whats wrong with eager_pk/pr...

(changes are to rules.mk which just uses debounce.c instead of eager_pr. Then i put the old implementation into debounce.c).

@kthibodeaux
Copy link
Author

@alex-ong I cloned the repo and added my layout to it (I assume I didn't need to change my local rules.mk) and the issue still persists :(

@alex-ong
Copy link
Contributor

alex-ong commented Sep 3, 2019

Just confirming that the rules.mk you're compiling with has debounce.c (my rendition of the working branches debounce)?
@kthibodeaux

@kthibodeaux
Copy link
Author

kthibodeaux commented Sep 3, 2019

@alex-ong Thank you for confirming. I double checked and forgot to check out your branch. Thank you so much for your continued attention and patience!

I copied debounce.c to my layout's folder, and changed my layout's rules.mk to add SRC += debounce.c and DEBOUNCE_TYPE = custom.

I'm hesitant to say, but it looks like the issue is resolved!!!

@kthibodeaux
Copy link
Author

I've been using it for an hour or so now, and every now and then (rare) I still see a duplicate press. It might be every 100 words or so (compared to every 5 before this).

The current master has DEBOUNCE 5, where as the working commit has DEBOUNCE 15. I assume that will fix the issue 100% with the custom debounce method, but it might be worth changing that default for the EZ?

@alex-ong
Copy link
Contributor

alex-ong commented Sep 3, 2019

I'm glad that it's improved!

Try testing it on 15 and let me know how you go.

One thing to note is that since this implementation of debounce is scan_rate based, its real debounce is DEBOUNCE * time_per_scan_in_ms. Last time i checked this was roughly 3ms. Setting it to 15 means that it is using an effective debounce time of 45ms~, which is definitely not ideal; cherry keys bounce for 5ms MAXIMUM.

The fact that 5 itself isn't enough (remember 5 scan_cycles is roughly 15ms. waaay larger than the spec'ed rate) is very... disturbing. There is something definitely off about ergodox (ez?) or something else in QMK seeing as sym_g, eager_pk, eager_pr and even this algorithm all are giving ghost presses with 15ms / 5 cycle debounce, but only on this keyboard... I feel the that sym_g / eager_pk / this all failing due to all having independent bugs is probably not that likely.

The only difference between eager_pk and this algorithm afaict is this is cycle based and eager_pk is time in ms based. eager_pk + DEBOUNCE = 15 should be same as debounce.c + DEBOUNCE = 5, assuming scan_cycle time is 3ms. A possibility is maybe your scanrate is abnormally low (say 100hz); most of the debounce algorithms would totally break (in different excitng ways) at this point.

Definitely sit on DEBOUNCE=15 for a while and see how that goes; i'll try and get a branch up that does scanrate reporting.

@kthibodeaux
Copy link
Author

kthibodeaux commented Sep 3, 2019

I've been on DEBOUNCE=15 since making my last comment and I have not had a single issue

Edit: If it matters I'm using Kalih copper switches

@alex-ong
Copy link
Contributor

alex-ong commented Sep 6, 2019

Could you try mainline branch (eager_pr) and use DEBOUNCE=45? And let me know how that goes?

Otherwise i'm happy to add this debounce algo to debounce folder and hook up ergodox_ez to it.
@kthibodeaux

@kthibodeaux
Copy link
Author

@alex-ong using latest master and setting DEBOUNCE=45 works great for me

@danielo515
Copy link
Contributor

danielo515 commented Sep 6, 2019 via email

@drashna
Copy link
Member

drashna commented Sep 6, 2019

It is on the high side, IMO. but it's also debouncing the entire row, not the entire matrix or individual keys, so I think this may be okay.

@alex-ong
Copy link
Contributor

alex-ong commented Sep 6, 2019

Isn't devounce 45 ridiculously high?

Yes. There is still an inherit problem somewhere in the chain, but i was just checking to see that there wasnt anything wrong with debounce itself. I ported the old algorithm over and it was found that it needed 15 cycles to be stable in current QMK. This is equivalent to 45ms~. So the new debounce algorithm (eager_pr/pk) is no different in terms of performance.

The original kthibodeuax working commit cd544e1 is 5 cycles. We have currently no idea what the cycle rate is. The new debounce rate is 15 cycles. Since the rest of the qmk codebase gets faster/slower over time this 15 cycles could be exactly the same, or it could be completely different 🤷‍♀.

I'm guessing that

  • qmk has gotten slower over time
  • all the time based debounce algos (eager_pr/pk) completely crash at really low scan rates,
  • cycle based debounce algos also seem to crash at low scan rates (old impl still isn't good at DEBOUNCE=5)
  • 15 cycle scan rate / 45ms on time/cycle algorithms just masks it.

Or

  • qmk has gotten significantly faster over time (yeah, nah)
  • it was always using 45ms debounce rate, cycles were 9ms before and now are 3ms/cycle
  • there was always some weird underlying bug in i2c or hardware related that was masked by having a 45ms debounce rate

From a technical point of view eager_pk and the old algo are the extremely similar and should fail almost identically.

If someone can benchmark scanrate/double-tap for the old commit (the latest with debounce=45 would mean you can't have two key-downs faster than 90ms)

So far at least we have a fix (debounce=45. It's eager so you shouldn't get any latency on key-down, but you will get 45ms latency on key-up)

The problem seems to me like its not debounce algo related.

@drashna
Copy link
Member

drashna commented Sep 6, 2019

Well, that was about the same time that we set "Prevent Stuck Modifiers" as the default, too. So it's very possible that that's part of why it got slower.
#3107

So .... I wouldn't be surprised if that's part of the problem.

As for testing, I can do that later this week.

@alex-ong
Copy link
Contributor

Nice; do you remember how to print out polling rate drashna? Though from memory you can use master branch of ergodox_ez with no problems; right?

@danielo515
Copy link
Contributor

@alex-ong I'm trying to apply your changes in my branch and I'm getting the following error:

Linking: .build/ergodox_ez_danielo515.elf                                                           [ERRORS]
 | 
 | /tmp/cc8xcohF.ltrans5.ltrans.o: In function `keyboard_init':
 | /qmk_firmware/tmk_core/common/keyboard.c:207: undefined reference to `matrix_init'
 | /tmp/cc8xcohF.ltrans5.ltrans.o: In function `suspend_wakeup_condition':
 | /qmk_firmware/tmk_core/common/avr/suspend.c:172: undefined reference to `matrix_scan'
 | /qmk_firmware/tmk_core/common/avr/suspend.c:175: undefined reference to `matrix_get_row'
 | /tmp/cc8xcohF.ltrans5.ltrans.o: In function `keyboard_task':
 | /qmk_firmware/tmk_core/common/keyboard.c:274: undefined reference to `matrix_scan'
 | /qmk_firmware/tmk_core/common/keyboard.c:279: undefined reference to `matrix_get_row'
 | /qmk_firmware/tmk_core/common/keyboard.c:285: undefined reference to `matrix_print'
 | collect2: error: ld returned 1 exit status
 | 
tmk_core/rules.mk:298: recipe for target '.build/ergodox_ez_danielo515.elf' failed

Am I missing something?

@danielo515
Copy link
Contributor

Ok, your branch is on qmk 0.5.174 while I was on 0.6.414

@danielo515
Copy link
Contributor

I can confirm that building with your branch (adding my changes into it) and with a normal debounce of 11 I didn't experienced a single issue on the entire day, so I'm very glad the old mechanism still works.

@tyxieblub
Copy link

tyxieblub commented Oct 10, 2019

I can confirm that eager_pr on master and DEBOUNCE=45 works for me too.

Just wanted to point that the issue first occured without any direct update of the firmware (which was from May). The system (debian sid) has been upgraded several times during this time, though.

Edit: Well, on master before #6994… had this page open for too long.

@danielo515
Copy link
Contributor

Is this solved for everyone? I'm still getting lots of duplicated key presses on certain keys, the n is one of the most affected and it is very annoying because I use it a lot.

@kthibodeaux
Copy link
Author

I haven't had any issues since changing my debounce to 45.

@danielo515
Copy link
Contributor

Latest master commit and that debounce? I just flashed with the default ergodox debounce on master which is 30. If the problem persist I will raise it to 45.
Thanks

@igorrafael
Copy link

After some experimentation, I fixed mine with DEBOUNCE=15 and DEBOUNCE_TYPE = sym_g

@kthibodeaux
Copy link
Author

@danielo515 yes

@igorrafael I will have to try that!

@danielo515
Copy link
Contributor

Do you remember that I said the n key was one of the most repeated ones? Well, today it suddently started to do the opposite: despite I was hitting the key, it was not being registered properly, I had to hit it several times.
I just changed the switch (thanks god for ergodox_ez hot swap switches) and now it does not get repeated as much as it used to be. I would say it now behaves like most other keys. Sometimes I get an extra unwanted keystroke (even with the debounce set to 45) but nothing crazy like before.

I'll have to try what @igorrafael proposed. Did you tried @kthibodeaux ? Does it improve?

@kthibodeaux
Copy link
Author

@danielo515 I tried what @igorrafael used to fix his, but it does not totally eliminate the issue for me. It is much rarer than DEBOUNCE 15 without DEBOUNCE_TYPE = sym_g but still is an issue.

I went back to just DEBOUNCE 45 to solve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants