Act gracefully when more than 6 keys are reported at once #103
Conversation
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.
There was a problem hiding this comment.
I believe the order of the keys in the report is not important (cf. https://github.com/jpbrucker/BLE_HID/blob/master/doc/HID.md: "... does not matter ..."), so I think you could do this more simply by just keeping an index of the oldest keypress, which should be less code.
|
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
|
|
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. |
this invariant is: the report keys are unique (any nonzero entry appears
at most once) and compact (all nonzero entries are first).
A benchmark loop measures how long add/remove report take depending
on the number of other keys pressed:
```
kbd = Keyboard(usb_hid.devices)
t0 = ticks_ms()
for _ in range(1000):
kbd._add_keycode_to_report(K.A)
kbd._remove_keycode_from_report(K.A)
t1 = ticks_ms()
print(f"Press release key time {ticks_diff(t1,t0)}us/call")
kbd = Keyboard(usb_hid.devices)
t0 = ticks_ms()
for _ in range(1000):
kbd._add_keycode_to_report(K.A)
kbd._remove_keycode_from_report(K.A)
t1 = ticks_ms()
print(f"Press release key time {ticks_diff(t1,t0)}us/call")
for k in range(K.ONE, K.SIX):
kbd._add_keycode_to_report(k)
t0 = ticks_ms()
for _ in range(1000):
kbd._add_keycode_to_report(K.A)
kbd._remove_keycode_from_report(K.A)
t1 = ticks_ms()
print(f"Press release key time {ticks_diff(t1,t0)}us/call")
kbd.release_all()
```
Timings were done on a KB2040 with 8.0.0-beta.
| Test | Before | After |
| Empty | 238 | 158 |
| 1 | 246 | 188 |
| 2 | 255 | 219 |
| 3 | 266 | 249 |
| 4 | 274 | 280 |
| 5 | 283 | 301 |
So in the usual case (fewer than 4 non-modifier keys pressed) this
slightly improves performance, but when the report is full (even if
it doesn't overflow) performance decreases, albeit by just 18us. Compared
to the 8ms time to send a report or the default 20ms between polling
a keypad.KeyMatrix this is minor.
|
I was not familiar with the "phantom state". Another reference to it: 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%). |
|
the .mpy file size change seems to be +30 bytes |
I was really thinking strictly about code size. |
* Use list[usb_hid.Device] allows getting rid of typing import. * can then make `import usb_hid` unconditional This decreases the mpy-cross size of the file to 1186 bytes, smaller than the baseline of 1191 bytes before this PR. (down from 1234 bytes)
|
By changing the type hints a bit I reduced the mpy-compiled size to less than the baseline. Original: 1191 |
.. improves performance and reduces mpy size (1175, -11). Now for 1 to 5 key slots filled it's always faster than baseline (268us worst case) too.
|
Reduced size another 11 bytes & sped it up so it's faster than the original in all tested cases. |
|
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. |
|
"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. |
| @@ -39,7 +35,7 @@ class Keyboard: | |||
|
|
|||
| # No more than _MAX_KEYPRESSES regular keys may be pressed at once. | |||
|
|
|||
| def __init__(self, devices: Sequence[usb_hid.Device]) -> None: | |||
| def __init__(self, devices: list[usb_hid.Device]) -> None: | |||
There was a problem hiding this comment.
@jepler Just a heads up that using built-ins as typing generics wasn't added until Python 3.9.
There was a problem hiding this comment.
what's the impact of this, unimportable on older python now? what's our baseline python requirement, given that current raspberry pi os is on 3.9 by now?
There was a problem hiding this comment.
Yeah, I believe it makes it unimportable. My understanding is that the baseline is 3.7. You could get around this by typing it simply as list if you want to avoid List[usb_hid.Device], though you lose the helpfulness of saying what it should contain using the type hint. To be fair, though, I think the context of what it should contain is pretty clear between what everything is named and the docstring.
There was a problem hiding this comment.
@makermelissa did I get that right?
Updating https://github.com/adafruit/Adafruit_CircuitPython_TFmini to 1.2.15 from 1.2.14: > Merge pull request adafruit/Adafruit_CircuitPython_TFmini#14 from tcfranks/main Updating https://github.com/adafruit/Adafruit_CircuitPython_TLV493D to 2.0.0 from 1.2.13: > Merge pull request adafruit/Adafruit_CircuitPython_TLV493D#16 from BrianPugh/patch-1 Updating https://github.com/adafruit/Adafruit_CircuitPython_asyncio to 0.5.16 from 0.5.15: > Merge pull request adafruit/Adafruit_CircuitPython_asyncio#28 from tekktrik/dev/fix-ci > Merge pull request adafruit/Adafruit_CircuitPython_asyncio#26 from dedukun/main Updating https://github.com/adafruit/Adafruit_CircuitPython_HID to 5.3.3 from 5.3.2: > Merge pull request adafruit/Adafruit_CircuitPython_HID#104 from adafruit/typing-improvements > Merge pull request adafruit/Adafruit_CircuitPython_HID#103 from adafruit/dont-crash-on-full > Merge pull request adafruit/Adafruit_CircuitPython_HID#101 from adafruit/fix-import-python3
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.