-
-
Notifications
You must be signed in to change notification settings - Fork 40.4k
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
Seebs/debouncez #2675
Seebs/debouncez #2675
Conversation
This changes back to something closer to the old debouncing algorithm, which will ignore brief spikes and record only sustained states; you have to have the same columns for a given row for DEBOUNCE+1 scans before those changes are committed. Any additional changes in the row reset the counter. Doing this per row reduces the cost of rapid typists, as "rows" on the Ergodox are actually vertical columns, with one finger on each; you don't usually move very rapidly within a row, but adjacent keys can be struck rapidly without triggering this. This should probably be fine with significantly lower debounce numbers. It also relies on the other changes that sped up scan rates. This also cleans up the scan logic a bit, and shuffles things to scan the right hand side first; this appears to improve stability, I think. Signed-off-by: Seebs <[email protected]>
So, I looked at some scope traces of bouncing keys, and I noticed: There's only ever bouncing at the start and end of keypresses. This means that eager reporting of key *presses* is safe, as long as you're conservative about key *releases*. So I modify the debounce code slightly further. When this read doesn't match the previous read, we set the debounce timer, but instead of always setting it to the same thing, we set it to DEBOUNCE if at least one bit is set in the matrix but not in this scan (thus, "a key has been released"), and to 1 if it's the other way. So, if you have a very spiky initial state on press, you still get a key press as soon as two consecutive reads reported it, but you don't get a key release until you have at least DEBOUNCE+1 consecutive reads without that key. Testing: Typing with it right now. Less stuttery than the previous ergodox EZ debounce code, but responds faster than the previous debounce code. (Probably not *observably* faster.) Signed-off-by: Seebs <[email protected]>
Any reason this can't be merged as is? |
I'm not sure what the current state of my code is. I eventually concluded that it was safe to process keys becoming pressed on the very first state transition to pressed, I think; no matter how spiky the transition is, it shouldn't happen at all unless the switch has actually closed. |
Okay. did you want to close this? Or was this still a good addition to the board? |
I've honestly forgotten where I was with this, or what I was up to. I would definitely like to see improved debouncing, because the performance tweaks I did a while back made the old debounce code stutter noticably sometimes. I just don't remember which debouncing I'm on, and haven't had a chance to check because Everything Keeps Happening. |
There's another debounce pull request open #3720, so I think these two should consolidated into one. Taking the best parts of both. I will try to check out these two a bit more later today, to get a better overview of the differences. |
@fredizzimo #3720 is a more generic version. It lets you choose the debounce algorithm. In this case, implementing a new algorithm (debounce_eager_pr where pr == per_row) would give the same performance as this, with the upside that the algorithm can be used for other keyboards that are wired with each finger getting their own row. |
Looks like there are some merge conflicts here, as well. |
If there's another similar thing being considered, it's probably better, especially if it doesn't need to be ergodox-specific. I was overall very happy with "eagerly jump to a pressed state, but debounce before leaving it"; that appears to produce very few false positives, without the frustrating lag of debouncing in both directions. |
The other debounce PR is #3720, for reference, and to make it easier to find. |
I haven't had a chance to look at it, and I'm not sure whether it's practical -- the EZ's matrix behavior is weird. I do think that some kind of debouncing improvement would probably help a lot, though. |
on further study, it looks like alex's PR has the same effect, and does it in a cleaner way, so I think I'm gonna opt to close this one since it's now redundant. |
This results from some chat with freddizzimo about the debouncing algorithm. The ergodox EZ tends to need a long debounce period to avoid stutter, which makes it less responsive, and this has been aggravated by the improvements to scan rates. This change gets back some of the responsiveness but has basically entirely elimintated stutter for me.
Note that the underlying innovation (?) of doing debounce per-row, rather than per-keyboard, may also work elsewhere. Debouncing per-key might be better still, but requires a much larger pool of debounce information (note the MATRIX_ROWS*MATRIX_COLS allocation in the old version), which is enough storage to be a problem for some keyboards. The decision to store that per-row is influenced by the fact that, on the ergodox, "row" actually means "column" in that a single "row" is all keys hit by the same finger, meaning there's usually not a lot of very fast movement within a row; fast typists will be causing changes in multiple columns.
Also important: This dramatically reduces reordering. If you hit three adjacent keys with the standard keyboard-wide debounce code, and you hit each before the keyboard's done waiting out the previous one's debounce, you will always get them in matrix order. With this, each row reports as soon as it's done debouncing, and it's much less reordered.