search

keyboardio / KeyboardioHID

A HID library for Arduino
MIT License
34 stars 17 forks source link

Improve handling of simultaneous changes in a single NKRO report #67

Closed gedankenexperimenter closed 3 years ago

gedankenexperimenter commented 3 years ago

When sendReport() is called, it now first checks for changes in the modifiers byte, and breaks down the process of transitioning to the new report into three stages:

  1. First, any non-modifier keycodes that toggled off are removed from the old report, if any, and a report is (conditionally) sent. This ensures that a key with a modifier flag attached that toggles off won't have the host process the modifier first, potentially causing a erroneous repeat character. For example, if LSHIFT(Key_4) is held and released, we need to first send a report without the 4, or some hosts will process the release of shift first, and give us $$$4 in the output instead of $$$$.

  2. Second, the modifiers that toggled on and/or off are updated, and a report is sent. This handles the situation where a key with a modifier flag toggles on, and we need to ensure that the host will process the modifier first to prevent erroneous output on the first character (4$$$ vs $$$$).

  3. Last, if there are any non-modifier keycodes that toggled on, they are added to the old report, and the final report is sent.

To accomplish this, the old report is modified in each step, and sendReportUnchecked() sends the contents of that (modified) old report rather than the new one. We could avoid that by making a temporary copy of the old report, and updating and sending that instead, but there isn't much point to doing so because there isn't any reasonable way for us to recover from errors. Kaleidoscope doesn't even check the return value when calling sendReport(), and it's not clear what we should do if an error occurs, anyway.

This new way of handling simultaneous changes from one report to the next covers more cases than the old way, which would never send both extra reports and failed to cover cases where a modifier toggles off when a non-modifier keycode toggles on (something that happens with the TopsyTurvy plugin), or where a modifier toggles on in the same report as a non-modifier toggles off (also possible with TopsyTurvy).

gedankenexperimenter commented 3 years ago

In a second commit, I changed the names of the two stored reports to something that I think better describes the way they're used.

gedankenexperimenter commented 3 years ago

Note: This change makes the current version of Kaleidoscope fail the TopsyTurvy test suite. That's not because TopsyTurvy is broken by it, however; it's because the testcases are designed to pass with the reports that are generated by the current KeyboardioHID. I'll submit a PR with improved testcases for TopsyTurvy when I can. (I've written a whole bunch of them, but they're only on my KeyEvent branches so far, and might need a little tweaking to work with master.)

gedankenexperimenter commented 3 years ago

With keyboardio/Kaleidoscope#989, all existing Kaleidoscope tests now pass with this PR.

gedankenexperimenter commented 3 years ago

Since the functional part of this PR has already been merged, and the name change is very unlikely to be accepted, I'm closing the PR. (I came across it and spent half an hour incorrectly thinking that the functional change had not yet been merged; I'd rather not waste time on that again.)