keyboardio / avr_keyscanner Goto Github PK

This is not exactly an issue (except maybe of documentation in a comment), but more of a possible answer to a question that @obra indirectly posed in wire-protocol.c:

                // Nothing in the ring buffer is the same thing as all keys released
                // Really, we _should_ be able to return a single byte here, but
                // Jesse is too clueless to figure out how to get I2C to signal
                // a 'short' response

I think the short answer is that you're not clueless: a target transmitter doesn't have a conforming way to shorten its responses beyond what the controller expects.

A target can maybe prolong a response by violating the I2C protocol and continuing to transmit after the controller NACKs a byte, if it can prevent the controller from asserting a STOP or repeated START condition. (The AVR TWI hardware module doesn't seem to support doing this.) If the target is honoring the I2C protocol, the controller determines the length of the response. (If the target stops transmitting early, the controller will receive 0xFF bytes until it gives up and NACKs and then sends a STOP or repeated START. The controller has no way to distinguish this from the target deliberately transmitting 0xFF bytes.)

I guess a target could also cause a bus error by violating the protocol in some other way (maybe by asserting a STOP or START before the bus is free?) and force a short read that way. Again, I think the AVR TWI hardware module doesn't support doing that.

Does this explanation seem to fit the observations? (I might be able to do some testing to confirm the finer details, if you have additional questions.)

It's probably a non-issue for a lot of people, but important enough to me that I think it's something to think about before a debouncer from f/configurable-scanners-and-debouncers goes mainstream.

If the debounce release delay is higher than the press delay then it makes rapid successive key strokes overlap, which is most critical for key sequences using modifiers.

It took a long time to discover this: at first I found key sequences like Ctrl-X O would often fail, not often enough to make it a clear issue instantly, but enough to feel like it happened way more often than "since I flashed this or that debouncer".
Then I discovered I was actually because it was typing Ctrl-X Ctrl-O instead of Ctrl-X O.
Then thinking about it, I found it could be explained by the debouncer having a release delay higher than press: it could make the Ctrl press overlap the O press even if physically not when the real time between release Ctrl and press O was shorter that release delay - press delay.

As an emacs user, those kinds of key sequences happens a lot. Non-emacs users might never see this issue. But to test this, one could type in a browser Ctrl-F L very fast, which should search l, but if the url bar is focused then it means Ctrl-F Ctrl-L happened.

I find key sequences which uses both hands are really sensible to this delay difference. And testing from 0 to 10 milliseconds difference between press and release delay, you can feel the overlap happening way more easily than, for example, feel an additional 10 ms latency on both release and press delay.
But I'm pretty sure in every-day typing it's unconscious, and those kind of delays are only hit because of muscle memory, which makes this issue more pronounced as you get used to the keyboard.

What the reason was for using (a snapshot of?) the twi.c internals from the Wire library instead of Wire? I'm suspecting some sort of incompatibility, but I'm not finding many clues in the history. Will it work on the GD32-based (?) Model 100 without porting to Wire? The Kaleidoscope hardware support for the Model 100 seems to use Wire to talk to the ATtinys, from what I can tell.

This could actually be serious and be interfering with normal operation, to some extent.

The ATtiny TWI implementation doesn't seem to handle repeated starts in receive mode. If it gets one, it seems to loop long enough to miss the next start condition, so effectively it will NAK the next transaction, if it's addressed to it. Register reads seem likely to be particularly problematic. (Write register address, issue repeated start, then read.) It also seems to be the problem that this code

https://github.com/keyboardio/Kaleidoscope/blob/6f11d89152ea274357afad9f621854d1f38bbf00/plugins/Kaleidoscope-Hardware-Keyboardio-Model01/src/kaleidoscope/device/keyboardio/Model01.cpp#L116

might be working around by alternating hands. (The ignored start condition is addressed to the other hand.)

Much of the code in the Model 01 driver seems to use repeated starts. I think some of it is working by accident, because the read operations all send stops, even though (almost?) none of the write operations seems to do so. (This being a polled keyboard, as I recall, read operations probably happen frequently.)

I was trying to add some LED prompting to the flasher sketch, and ran into a problem where I couldn't send multiple LED commands back-to-back using repeated starts without some of them getting ignored. (twi_writeTo(..., true, false) on all except the last command) If I always used sendStop=true, they would all execute with no problem.

It seems to be a problem with twi-slave.c mishandling repeated starts in receive mode by looping until a stop condition is detected. (I think another start condition would break out of the loop, but not before missing the ack of the address.) Sending four LED commands in a row, with sendStop=false on the first three, resulted in the first one being read and executed, the second one being NAKed in the address, the third one being read and executed, and the fourth one being NAKed in the address.

The problem is likely here:

because TWI_Stop() loops until TWSTO is clear, which is not actually documented in the datasheet, and is probably wrong for this situation. Clearing the interrupt flag and not looping, as in

https://github.com/keyboardio/Kaleidoscope/blob/6f11d89152ea274357afad9f621854d1f38bbf00/plugins/Kaleidoscope-Hardware-Keyboardio-Model01/src/kaleidoscope/device/keyboardio/twi.c#L508

seems to allow repeated starts to work correctly.

I think this problem would only be obvious when the TWI bus is relatively quiescent (such as in the ATtiny flashing sketch), and cause subtle (but possibly serious) misbehavior in normal operation.

Maybe use the intelhex library instead of custom code to manipulate/edit .hex files? Or do you prefer to keep Python 2 compatibility in the tools? (the stable intelhex library requires Python 3)

I'm willing to work on a PR for this.