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

Seebs/debouncez #2675

Closed
wants to merge 2 commits into from
Closed

Seebs/debouncez #2675

wants to merge 2 commits into from

Conversation

seebs
Copy link
Contributor

@seebs seebs commented Apr 4, 2018

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.

seebs added 2 commits April 4, 2018 12:07
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]>
@drashna
Copy link
Member

drashna commented Oct 22, 2018

Any reason this can't be merged as is?

@ezuk @jackhumbert ?

@seebs
Copy link
Contributor Author

seebs commented Oct 22, 2018

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.

@drashna
Copy link
Member

drashna commented Oct 22, 2018

Okay.

did you want to close this? Or was this still a good addition to the board?

@seebs
Copy link
Contributor Author

seebs commented Oct 22, 2018

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.

@fredizzimo
Copy link
Contributor

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.

@alex-ong
Copy link
Contributor

alex-ong commented Nov 5, 2018

@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.

@drashna
Copy link
Member

drashna commented Dec 6, 2018

Looks like there are some merge conflicts here, as well.

@seebs
Copy link
Contributor Author

seebs commented Jan 5, 2019

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.

@drashna
Copy link
Member

drashna commented Jan 25, 2019

The other debounce PR is #3720, for reference, and to make it easier to find.

@drashna
Copy link
Member

drashna commented Mar 27, 2019

@seebs #3720 has been merged, and it may be better to just incorporate that with the Ergodox EZ's matrix.

Would you mind updating the PR to reflect that, or is it okay for me to close this?

@seebs
Copy link
Contributor Author

seebs commented Mar 27, 2019

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.

@alex-ong alex-ong mentioned this pull request Mar 27, 2019
13 tasks
@seebs
Copy link
Contributor Author

seebs commented Apr 6, 2019

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.

@seebs seebs closed this Apr 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants