Migrate to CircuitPython 7 Keypad APIs

[klardotsh]

Our current matrix scanner is, well, what it is. It's pure Python, it bakes in quite a few assumptions about how your keyboard looks and works (eg #192), and it cuts some interesting corners in the name of performance. Thankfully, the lovely folks at Adafruit have taken it upon themselves to support various types of matrix and non-matrix key-based input mechanisms in CircuitPython itself, with efficient and ergonomic C-backed APIs. Let's use them.

We should decide whether to retain the old matrix scanner wholesale (@Gigahawk has alluded in Matrix that this is preferable for those using GPIO expanders, as some of them expose DigitalInOut-like objects that the C HAL can't talk to), use the existing MatrixScanner class as a stub/abstraction over the CircuitPython library (I'm not entirely sure why, other than for backwards compatibility, though the MatrixScanner isn't particularly publicly-used anyway so there should be no third-party consumers), or completely scrap the file.

This trivializes #188 to the point that I'm just going to absorb it into this issue: when we migrate to Keypad, we should expose the KeyMatrix's interval to KMK users in a reasonably ergonomic way.

[XScorpion2]

CircuitPython Keypad does not debounce the signal and the comments do not match what the code is actually doing. The interval parameter controls how frequently it gets the switches current state using the common_hal_digitalio_digitalinout_get_value function. That function returns the current raw state of the pin with no filtering or debouncing added to the return.

So with the default implementation, that code reads the pin values every 20ms, and queues up a key event if the current state does not match the previous state. Cherry MX switches per spec are rated for 5ms of debounce, so if you close that switch 1ms right before the 20ms scan rate, that switch will be 4ms away from being considered stable per spec. So the code could read any value at that point, closed or open. So if it reads that it is still open, and the key is released before the next scan rate, then the key will never be picked up that it changed state. If you shorten this interval to accommodate this short of a press, then you risk getting multiple press and release events.

So to reiterate, the Keypad does not implement any debounce logic, it just runs slower than debounce spec for common switches and thus hides the issue by not responding to fast keypress and release cycles.

[klardotsh]

I'd argue defined behavior of "fixed 20ms intervals" is probably a better situation than KMK's current, which is fully undefined behavior (indeed, I get occasional debounce issues on my NRF52840 boards if the processor cycles run a tick faster than normal), but noted - it might be worth trying to collaborate with @dhalbert and/or the rest of the CircuitPython crew to figure out what a "proper" debounce might look like

[dhalbert]

@XScorpion2 If you have suggestions for how to improve the debouncing, I'd be interested in hearing them. Perhaps you are thinking of making it interrupt driven. Probably best to open a new issue with suggestions in https://github.com/adafruit/circuitpython.

The current choice was partly made based on the overhead of doing the scans, given the other things that CircuitPython has to do in the background.

[ladyada]

fixed-rate scanning is equivalent to a low-pass filter debounce - if the code 'responds fast' to a keypress or release then its not debouncing. if you want a faster response rate, we'd just change the sample rate from 20ms to 5ms, but there's tradeoffs of course. 10ms or 20ms is considered 'standard'

[XScorpion2]

@XScorpion2 If you have suggestions for how to improve the debouncing, I'd be interested in hearing them. Perhaps you are thinking of making it interrupt driven. Probably best to open a new issue with suggestions in https://github.com/adafruit/circuitpython.

The current choice was partly made based on the overhead of doing the scans, given the other things that CircuitPython has to do in the background.

Right, moving it to C is most certainly a better approach and I do like the code. I've been slowly reading through the structure to see what debounce code might look like with it. Sorry I am fairly new to this code base, My first reply was mainly about ensuring that the need for faster switch response and debouncing logic was not lost when switching from the current implementation to a native one as the problem is still there, just hidden. Interrupt driven is an interesting idea I hadn't considered, but I have seen folks in QMK and ZMK use this for improved power/cycle savings while retaining fast response time.

fixed-rate scanning is equivalent to a low-pass filter debounce - if the code 'responds fast' to a keypress or release then its not debouncing. if you want a faster response rate, we'd just change the sample rate from 20ms to 5ms, but there's tradeoffs of course. 10ms or 20ms is considered 'standard'

I would argue that 10ms or 20ms is not standard when Cherry MX themselves list per their spec lists <1ms bounce time, 5ms is the default in QMK and ZMK, and this library is intended for PC Keyboard use.

[ladyada]

did you try changing the rate? https://learn.adafruit.com/key-pad-matrix-scanning-in-circuitpython/advanced-features#scanning-interval-and-debouncing-3099161-1

[dhalbert]

The 20 msecs was chosen as a reasonable default, based on experiences with keyboard switches vs tactile switches.

The current scheme is similar to various other scanning code I encountered. I think it would be interesting to see if a human keyboard user can actually generate a keypress/release that is shorter than 20msecs (or some somewhat smaller length). This would be easy to test with a logic analyzer or oscilloscope.

[Gigahawk]

Somewhat contrived, but I can eventually get 5ms using this scan rate estimator on my QMK board https://blog.seethis.link/scan-rate-estimator/

Personally I probably would never be able to achieve this in normal use but I don't think it's unreasonable that someone gaming or typing really fast might.

[XScorpion2]

The 20 msecs was chosen as a reasonable default, based on experiences with keyboard switches vs tactile switches.

The current scheme is similar to various other scanning code I encountered. I think it would be interesting to see if a human keyboard user can actually generate a keypress/release that is shorter than 20msecs (or some somewhat smaller length). This would be easy to test with a logic analyzer or oscilloscope.

Generally I agree, press and release under 20ms is pretty uncommon. However computers events respond to either a key down or key up action, and with this setup it's effectively adding 20ms delay on top of switch bounce, which is pretty rough, especially for gaming. Changing the scan rate to 5ms is certainly better, but as mentioned, there is the potential, if timing is off, of clipping the bounce causing additional undesired events. Personally I've experienced this more frequently on the upstroke of my my tactile switches.

[xs5871]

Didn't realise that this was already in discussion, but I may have implemented a proof-of-concept of the keypad class in KMK. It's not quite production ready and would introduce breaking changes (no more intify_coords for example), it would allow for multiple keypads though (for mixing direct and matrix keys maybe?).

[dhalbert]

@xs5871 if the hashes are identical, as you noted, please file a bug in https://github.com/adafruit/circuitpython. Thanks!

[eltariel]

I've also done some initial work to use the keypad library - in my case it's a self-contained hack that subclasses KMKKeyboard to allow a custom matrix scanner, and a barebones scanner that wraps keypad.Keys so I can do key-per-gpio (same device as #192 ).

My fork is here: https://github.com/eltariel/kmk_firmware

[kdb424]

Feel free to open a draft PR so it's easier to collaborate, and tag this issue in the PR

[klardotsh]

@xs5871 I'm willing to consider intify_coords a private API not worth keeping backwards compatibility with - I'm sure any impl of CPy7 Keypad I would have come up with would have similarly trashed that entity. In general the matrix scanner was never really meant to be tinkered with by runtime code anyway (and long predates this project even pretending to care about users that weren't @kdb424, a mutual friend of kdb and I's, or myself), so I don't think we need to sweat too much about backwards compatibility within it. We rolled out much more invasive breaking changes under yolo terms not too long ago anyway...

For anyone else jumping in this thread, the diffs for each aforementioned WIP branch (as permalinks) are:

• https://github.com/KMKfw/kmk_firmware/compare/master...xs5871:ec88653
• https://github.com/KMKfw/kmk_firmware/compare/master...eltariel:3be9dd1

I haven't done a super deep dive on both to see a full pros/cons analysis on them, but my gut punch reaction after skimming both is that I'd lean towards the approach of the latter that doesn't bother trying to retain (much) backwards compatibility with the existing MatrixScanner private or semi-public APIs, and instead only provides a scan_for_changes impl, provided that it retains split and bluetooth support.

I'll give these a more thorough reading, and try to figure out how/if we can get the whole band together to align on one "blessed" pattern for use of the new keypad APIs and perhaps ship one (or a combo of) these forks, some time soon (I hope tomorrow, since I have all Mondays off at this new gig, but :sparkles: life happens :sparkles: pretty often and I learned long ago to stop pretending to give ETAs on GitHub...)

[klardotsh]

forgot to tag @eltariel in ^ since I kinda responded to xs and then moved to a public service announcement with only a line break, but thank you as well for your work on this!

[eltariel]

Mentioned it in the PR, but it's worth repeating here to keep the discussion in one place:

The approach I took was to entirely bypass anything related to the MatrixScanner initialisation code in KMKKeyboard, instead having the scanner implementation passed into the keyboard's constructor. The scanner implementation is very little beyond a translation layer around CircuitPython's keypad.Keys, and adding one for keypad.KeyMatrix should be pretty trivial.

Using this approach it should also be possible to write an aggregating scanner that just passes events from any of a collection of scanners. At the most basic level this could let us do key-per-gpio and a regular matrix on a single board, but would also potentially be able to be extended to enable splits if we were to make e.g. a uart or i2c scanner.

[kdb424]

I believe this has been resolved. Closing.