KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.39k stars 474 forks source link

[BUG] Combo timeout is not enforced for small values #748

Open Roming22 opened 1 year ago

Roming22 commented 1 year ago

Describe the bug I've set my combos to trigger if key presses occur within 45ms. The combo gets triggered on longer delays.

To Reproduce Steps to reproduce the behavior:

Expected behavior The combo should not activate, and individual key presses should be output.

Debug output

136262 kmk.kmk_keyboard: MatrixChange(ic=5, pressed=True)
136267 kmk.kmk_keyboard: KeyResolution(key=Key(code=1032, has_modifiers=None))
136288 kmk.kmk_keyboard: coordkeys_pressed={5: None}
136291 kmk.kmk_keyboard: keys_pressed=set()
136321 kmk.kmk_keyboard: processing timeouts
136347 kmk.kmk_keyboard: MatrixChange(ic=7, pressed=True)
136352 kmk.kmk_keyboard: KeyResolution(key=Key(code=1036, has_modifiers=None))
136358 kmk.kmk_keyboard: processing timeouts
136368 kmk.modules.combos: activate Chord([1032, 1036])
136381 kmk.kmk_keyboard: coordkeys_pressed={5: None, 7: None}
136384 kmk.kmk_keyboard: keys_pressed={Key(code=19, has_modifiers=None)}
136392 kmk.kmk_keyboard: MatrixChange(ic=5, pressed=False)
136397 kmk.kmk_keyboard: KeyResolution(key=Key(code=1032, has_modifiers=None))
136399 kmk.modules.combos: deactivate Chord([1032, 1036])

Additional context The keys of the combos are MT keys. I would expect the combo and MT delays to be in parallel.

In my example the keys pressed are 't' and 'e', and output p instead of te. The delay between the key presses is 85ms, almost twice as long as the 45ms timeout configured, and yet the chord gets activated.

My layout: xs18.txt

xs5871 commented 1 year ago

That's because python on an MCU is slow (and KMK isn't super optimized). Can't be easily "fixed" as such. Out of curiosity: what MCU is this on?

Roming22 commented 1 year ago

A sea-picro, which is an RP2040 based MCU using the pi micro footprint.

regicidalplutophage commented 1 year ago

I have a suspicion that the slowness can be sidestepped somehow. Likely a tough issue, but keypad library is interrupt driven, so it shouldn't be impossible. There's already ticks_ms attached to matrix changes, so if we could say "don't activate chord if matrix changes are this far apart" it would be significantly more precise.

xs5871 commented 1 year ago

That's not wrong, and in the long run I intended to look into basing timeouts on the KeyEvent timestamps, rather than now(). That could result in some funky behaviour though, especially for short timeouts and slow modules. Another thing I've been looking into is a better data structure for timeouts, like an actual priority queue.

Roming22 commented 1 year ago

I do not know about the internals of KMK. But if a timer was to be started on the first key press of a potential combo, and was to register an event on timeout, then that could be used to discriminate between a roll and a combo.