Extra key reports (potentially) causing errors, and how to improve that

(Michael Richters) #1

This is mostly a theoretical problem; I don’t know if anyone has actually observed this in the field yet, but I’ve just recently released a plugin that might replace Kaleidoscope-DualUse at some point, and it’s much more likely than most plugins to trigger this potential bug. Here’s a summary:

When the event handler hooks are being called by handleKeyswitchEvent() from the main loop (via KaleidoscopeHardware.actOnMatrixScan()), a plugin might try to guess (it’s hard to know for sure) if – for example – a modifier key is active. To do this, it’s better to check the previous HID report using wasModifierActive() because the current report is incomplete. If that plugin makes a decision based on that information – and let’s suppose that information is correct – it could be invalidated by a different plugin acting on a later key which triggers an injected hid::sendKeyboardReport() before the main report is sent by Kaleidoscope at the end of the main loop.

Now, I’ve made a plugin that sends such reports, and it currently works by copying the previous report, and modifying it by calling handleKeyswitchEvent(), then sending the modified report, then restoring the state of the in-progress report before returning and letting the main loop proceed. To make matters worse, it is quite likely to change the modifier state of not just the next (in-progress) report, but also the state of the (new) previous report, since it’s whole raison d’être is to overload letter keys with modifiers. This could retroactively make prior calls to wasModifierActive() during the same scan cycle incorrect.

The only solution I’ve come up with is to get Kaleidoscope to re-start the event handler loop, stopping when it gets to the key currently being handled. This would give other plugins a chance to get the correct information about the previous keyboard report, but it has a couple of potential drawbacks:

  • Lots of extra calls to handleKeyswitchEvent(), the vast majority of which won’t produce any different result
  • A plugin could save state based on an aborted call to handleKeyswitchEvent() without realizing it, possibly resulting in very unpredictable behaviour (e.g. if it increments a counter, then acts on that counter during the regular pre-report loopHook pass, the counter would have the wrong value).
  • Memory limits could be exceeded by the recursive nature of calling handleKeyswitchEvent() from itself, possibly even triggering infinite (until we run out of memory) recursion.

I have a much deeper solution in mind, but it depends on some much bigger architectural changes in Kaleidoscope, beginning with @noseglasses’s changes to the plugin hook interface, but that’s a bit of a separate discussion.

I have a working version of Kaleidoscope that implements the changes necessary to make it possible to restart the event handler pass. It changes the hardware modules:

  • Kaleidoscope-Hardware
  • Kaleidoscope-Hardware-Model01
  • Kaleidoscope-Hardware-Shortcut

I added a function that’s basically a version of actOnMatrixScan(), but which takes row & column parameters, and stops after the given coordinates. This could be done with less code by modifying actOnMatrixScan() to take those parameters and test the conditionals for every key on every scan. I don’t know whether the code size matters more than the speed difference.

Two big questions, then:

  1. Is this a good idea?
  2. Even if it is an improvement, should we bother with this kind of change, or should we drop it because there are better ways to make it irrelevant (I have one, I think) once the plugin hook interface improvements land?

(Michael Richters) #2

Third question: Is this a real problem, or is this just excessive perfectionism on my part?

(Gergely Nagy) #3

I think this is a real problem, there are a number of plugins that could benefit from this, or something like this.

I’m not entirely convinced that what you proposed above is something that covers all the cases I have in mind, but I need to spend a few more brain cycles to properly understand both your proposal, and @noseglasses’s PR. (ETA: tomorrow)

(Jesse) #4

Given the two of these, I think it makes sense to hold off.

What would be useful is figuring out specific failure cases. Exact sequences of events that would cause the problem you’re worried about. These are the sorts of test cases that we should be thinking about adding once we have a decent test harness

(Do note that we have guards in the HID layer to prevent sending duplicate key reports.)