Add state integration based debouncing to the keypad module

[xs5871]

Concerns / resolves #8777

Slight difference to the implementation mentioned in #8777: I "upgraded" to an integration based algorithm because the simple scanning timeout still gave me issues on some anoyingly bouncy keys and for better filtering of potential noise/EMI (which I did not encounter, but hey, it's basically free).
Summary:

• keypad state transitions happen after their cumulative average crosses an adjustable threshold,
• keypad.currently_pressed has been converted hold the integration counter,
• keypad.previously_pressed has been removed because it is unecessary,
• the debouncing routine has been factored out for code de-duplication.

[dhalbert]

You will need to bring your local branch up to date. Some help on that is here: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github/staying-up-to-date

[dhalbert]

So for your excessively bouncy switches, how is making the interval be (old)interval * debounce_threshold less satisfactory? Does it introduce too much latency? But I would think the integration algorithm also introduces similar latency with the threshold set to to some multiple of the interval.

[xs5871]

You will need to bring your local branch up to date. Some help on that is here: https://learn.adafruit.com/contribute-to-circuitpython-with-git-and-github/staying-up-to-date

I based it on the 8.2.x branch because I want to actively use it. Once we're satisfied with the code and doc conceptually I'll port it to main, no problem.

[dhalbert]

I based it on the 8.2.x branch because I want to actively use it. Once we're satisfied with the code and doc conceptually I'll port it to main, no problem.

Ok, you need to make the PR against 8.2.x, then. Go up to the top and hit "Edit" to the right of the title and you can change the base branch you are PR'ing against from main to8.2.x.

[xs5871]

So for your excessively bouncy switches, how is making the interval be (old)interval * debounce_threshold less satisfactory? Does it introduce too much latency? But I would think the integration algorithm also introduces similar latency with the threshold set to to some multiple of the interval.

Basically, yes. The integration algorithm has a similar latency (strictly at least equal, sometimes slightly longer) -- the thing that improved is that this algorithm has a seesaw-like characteristic, similar to a schmitt-trigger for lack of comparison.
Both algorithms set to the same interval/threshold, the previous version would occasionally emit multiple events, whereas the latter is about as fast, but more resilient to noise, if that makes sense.

[dhalbert]

Thanks for the comments. So my remaining question is

So is 1 the same as the previous algorithm? I would think that 0 would mean no debouncing then.

I am a little confused by your saying that 1 is no debouncing, instead of the (disallowed) 0.

[xs5871]

Thanks for the comments. So my remaining question is

So is 1 the same as the previous algorithm? I would think that 0 would mean no debouncing then.

I am a little confused by your saying that 1 is no debouncing, instead of the (disallowed) 0.

I can take another look at that. I think the constant offset of at least 1 is necessary to encode and distinguish pressed/released states. Offsetting the argument by one is a possibility, although that would introduce an awkward upper bound of 126.
My thoughts were also: You have to measure pressed/released for at least threshold (>=1) intervals for an event. Measuring for 0 intervals didn't make sense in that regard.

[dhalbert]

My thoughts were also: You have to measure pressed/released for at least threshold (>=1) intervals for an event. Measuring for 0 intervals didn't make sense in that regard.

I understand that 0 doesn't make sense, but I was confused that your doc string said 1 means NO debouncing. I thought it waits for one interval, so it is at least doing one interval of debouncing.

[xs5871]

Good point, I think I see what you mean. 1 does mean "no" debouncing, or current behavior. Do you have a suggestion how to rephrase the doc, or what would be a good resolution to avoid this confusion?

[dhalbert]

I would not describe the current behavior (before your code) as "no debouncing". It debounces fine for bounces that resolve in less than interval, because it collects state-transition info across two (consecutive) interval times.

I tested the current code a bunch with keyboards, etc. We have not gotten complaints of bouncing when the current keypad is used in keyboard projects.

I have seen some tactile switches that have really long bounce settle times, like 50 msecs.

[xs5871]

I agree, up to 50ms works for most mechanical keyboard switches. There are some buttons that only barely work consistently with that scan rate, but that's not really my incentive. I also didn't mean to call the current implementation "no debouncing", that's why I put the "no" in parentheses; I apologize if that came across the wrong way. The rate limiting does work in most cases, no arguing about that.
It's not the absolute delay that the scan rate limiting introduces that can be perceived as an issue, it's the consistency of the delay. For example, we recently had someone mentioning that rythm games are almost impossible, because the input delay spread is basically a random distribution from 0 to interval. A more sophisticated debounce algorithm can cut down on that spread, and only as a side effect may reduce latency.

[jepler]

Just as a data point, qmk uses 5ms as the default debounce time. They describe many other aspects of their algorithm in https://github.com/qmk/qmk_firmware/blob/master/docs/feature_debounce_type.md

[xs5871]

How's this:

//|         :param int debounce_threshold: Emit events for state changes only after a key has been
//|           in the respective state for ``debounce_threshold`` scans on average. Successive
//|           measurements are spaced apart by `interval` seconds.
//|           The default is 1, which resolve immediately. The maximum is 127.

I think replacing the original term of "scans" by "intervals" made it a little confusing in different way, by implying that at least 1 interval of delay is introduced = the key has to be x for at least 1 interval.
Asserting that "scans" directly corresponds elapsed time in intervals was incorrect of me. "Scans" meant the momentary events that happen between intervals.

[xs5871]

Sorry, got a bit sidetracked on other things.
I changed the documentation to my last suggestion, that should avoid some potential misunderstandings.
@dhalbert Is there anything else we need to discuss for the draft, or can I mark the PR ready for review?

[dhalbert]

Asked a question in the code.

Note also we just merged #9035, which is support for using a demultiplexer on one side of a matrix. You may want (or need) to rebase or merge and modify that implementation to match what you changed here.

[dhalbert]

I think this is clearer; it still takes a bit of head-scratching to understand the algorithm. A diagram would theoretically help, maybe, but let's not go there yet.

The current defaults are interval=0.020 and debounce_threshold=1. What values would you choose on a typical keyboard to make, say, the rhythm game you are talking about work better? And what values are you choosing for your own use now?

[xs5871]

Ok, rebased on main and integrated the demux keypad.

I also spend a couple of hours testing almost every switch and button I had lying around; that includes multiple keyboard switches, through-hole and face-plate-mounted buttons, and a rubber dome pad. Surprisingly, almost all read consistently with interval=0.001 and debounce_threshold=3. The noisiest switches required debounce_threshold=5.
From a typing perspective, I can't tell the difference between threshold values 3 and 5 (also the USB polling interval is 8ms, so no surprise there) -- The latter should be a sensible default.
(The rythm game was an example from another user, I can't verify an improvement there. Typing seems ever so slighly more responsive, although I couldn't do a double blind test.)

[dhalbert]

I appreciate your testing. Did you try with a longer interval, such as the 0.008 you suggested? I think I will leave the current defaults alone for now. The overhead of the interrupts is lower with 0.020, and for most people, the current defaults seem fine. I did some testing with tactile switches and keyswitches when I chose that value. Those who are using keypad for a performance keyboard can decrease the polling interval. It would be good to write this up somewhere once it's merged.

[xs5871]

I did test with longer intervals, works as expected, but then I'd have to set the integration threshold lower to get reasonable response times (obviously). I intentionally tested the extreme case of lowest latency -- increasing any of the two variables will always increase both reliability and latency.

If you'd like me to contribute to that write-up, maybe with a graph illustrating the algorithm as you mentioned, I'd be happy to help.

[dhalbert]

It would be a fine topic for https://adafruit-playground.com/, and we could point to it from the keypad Learn Guide.

[dhalbert]

Thanks for your perseverance on this. Let's merge it in for 9.1.0. A writeup in https://adafruit-playground.com/ would be welcome.