Act gracefully when more than 6 keys are reported at once

[jepler]

When making a keyboard, it's easy for the user to mash plenty of keys at once. While a keyboard firmware could trap the ValueError and continue gracefully, I think it would be nice if instead the oldest keycode is dropped from the report to make room for the newest one.

This does slightly complicate the code but it makes all my keyboard projects more robust.

The pylint disables are in order to avoid new memory allocations, which enumerate() and tuple construction both do.

[deshipu]

IIRC there was a specific thing the keyboard is supposed to send when more than 6 keys are pressed.

From https://wiki.osdev.org/USB_Human_Interface_Devices

There is also a "phantom condition" which you can think of as an overflow. A USB keyboard packet can indicate up to 6 keypresses in one transfer, but let's imagine someone dropped their keyboard and more than 6 keys were pressed in one time. The keyboard will enter the phantom condition, in which all reported keys will be of the invalid scancode 0x01. Modifier keys will still be reported however. Imagine that 8 keys (or any random number more than 6) are being pressed, and the right shift key is also being pressed. The packet sent would look like:

20 00 01 01 01 01 01 01

Notice that the modifier keys are still being indicated, but the actual scancodes all return the phantom condition. There are other special scancodes besides the phantom condition: 0x00 indicates there is no scancode and no key being pressed, 0x01 indicates the phantom condition we just explained, 0x02 indicates the keyboard's self-test failed and 0x03 indicates an undefined error occured. Starting from 0x04 the scancodes are valid and correspond to "real" keys. IMHO, a device driver should ignore the phantom condition if it happens.

[dhalbert]

It would be good to test some bog-standard keyboards like a Dell or Logitech and see what reports are produced when you press say 8 keys at once.

[jepler]

I was not familiar with the "phantom state". Another reference to it: https://www.usb.org/sites/default/files/hid1_11.pdf page 62 (PDF page 72) also has language about the "phantom state".

While this was my original implementation idea, just now I checked what my WASD v2 keyboard with Holtek controller does. What I implemented here matches USB captures from my WASD v2 keyboard with a Holtek controller. It does not implement the "phantom state".

QMK also does not implement the "phantom state". It does optionally implement a ring buffer style storage similar to what Dan suggests.

I don't think the ring buffer actually improves efficiency, at least in a big-O sense. Before this change, both add and remove already have to iterate over all the slots in report_keys. They may do a few more operations now but that's all. I re-worked the code slightly so that in the "press" case to unify two loops, reducing overhead a bit.

I chose the "move reports down" technique because it doesn't require any additional storage, just a bit more data-shuffling. qmk/tmk_core optionally implement something similar to what Dan suggests, basically a ring buffer where the oldest data gets overwritten. However, as keys can be released in any order it still has to implement a 'shift down' routine to compact the array sometimes. In tmk_core this "shift down" step can be done less frequently: only when a key is added to a full ring buffer.

I gathered performance data and optimized the code.The new code is actually more performant whenever 4 or fewer non-modifier keys are specified in a report. (this is possible mainly because I now assume that the report is always compact and unique, while the original code did not. But when the report is managed by the internal routines, this invariant is respected)

Even when it's slower it's just by a few microseconds, compared to the 8ms report interval of our default keyboard descriptor (.2%) or default KeyMatrix polling time (.09%).

[jepler]

the .mpy file size change seems to be +30 bytes

[dhalbert]

I don't think the ring buffer actually improves efficiency, at least in a big-O sense.

I was really thinking strictly about code size.

[jepler]

By changing the type hints a bit I reduced the mpy-compiled size to less than the baseline.

Original: 1191 Before shrink commit: 1234 (+43) Final: 1186 (-5)

[jepler]

Reduced size another 11 bytes & sped it up so it's faster than the original in all tested cases.

[dhalbert]

On my Kinesis Gaming split keyboard, and on a very standard Dell keyboard, if I press 8 keys at once I get five or six keypresses sent. If I press a dozen or more at once (with the palm of my hand), no keypresses are sent.

[dhalbert]

"Too many keys pressed" might be more likely handled by the key monitoring part of the program rather than this. Given that this improvement is smaller than the previous less-functional code, happy to approve.