search

cb22 / macbook12-spi-driver

WIP input driver for the SPI touchpad / keyboard found in the 12" MacBook (MacBook8,1 + MacBook9,1)
GNU General Public License v2.0
300 stars 106 forks source link

Typing "zu" sometimes becomes "zuzu" #53

Closed Dunedan closed 6 years ago

Dunedan commented 6 years ago

I've a very strange bug, I'm encountering pretty irregularly and I can't actively reproduce it, but it happens from time to time that when I type the German word "zu" instead "zuzu" appears on the screen. I haven't encountered this problem with any other combination of characters so far.

roadrunner2 commented 6 years ago

This does sound strange. My first thought is that this is some form of double-key-event generation that I've encountered on that keyboard: specifically if I pressed a certain key (and in my case it only happened on one key) on the side instead of in the middle, then it would generate an event both when pressed and when released - try playing around with pressing the z and u keys on the various sides and see if you can get a double event. Now, I would normally think that you would then get things like zzu, zuu, or even zzuu, not zuzu, but the latter is possible if z is pressed, u is pressed, then z released, and finally u released.

Also, try setting the debug parameter to 0x100:

  echo 0x100 | sudo tee /sys/module/applespi/parameters/debug

and then show what events are reported when this happens.

Dunedan commented 6 years ago

So, finally I managed to find a way to reproduce it. It's related to hitting the allowed maximum number of keys pressed in parallel. That limit seems to be dynamic, depending on which keys are pressed. E.g. for pressing qwertyu it's 6, while it's only 2 for pressing <space>gh (I realized that zu is a bad example as I've a German keyboard, where z and y are switched). If you press more keys than this limit in parallel the keyboard emits an unusual event (containing 01 in all fields where usually the key codes would be). If you then lift the fingers up in a any order you don't get the desired output.

Easiest way to reproduce:

  1. disable key repeating (just to make it easier to see)
  2. press <space> and keep it pressed
  3. press g and keep it pressed
  4. press h and keep it pressed
  5. lift the finger from <space>
  6. lift the finger from g
  7. lift the finger from h

Expected output: <space>gh Actual output: <space>g<space>gh

Attached is a debug log of pressing "<space>gh" which causes "<space>g<space>gh" to appear: key-press-bug.txt

roadrunner2 commented 6 years ago

Excellent debugging work! Thanks. Easy to reproduce now.

So it looks like the solution is just to ignore these rollover-overflow keyboard messages. I just pushed an update to my fork - if you can try that out and confirm it fixes things, I'll send the pull request.

Btw., while on the topic of key rollover, @cb22 you have a note in the README saying that key rollover does not work properly - can you elaborate? I'm not aware of any issues or missing functionality (especially with this issue here fixed).

Dunedan commented 6 years ago

Thanks for the quick fix. :+1:

It's already a significant improvement, but a small annoyance remains: With enabled key repeating its output isn't as expected. Pressing and holding <space>gh in order I'd ideally expect multiple spaces, followed by multiple g's, followed by multiple h's or (as that's not possible with the max rollover apparently) multiple spaces, multiple g's, where the g's stop to get printed as soon as I press h. Instead it continues to print g's even after pressing h and stops printing g's only once I remove one of the fingers (e.g. the one on space).

roadrunner2 commented 6 years ago

Yeah, I was thinking about this too after my last update. The problem is this: disabling of autorepeat only happens on another key-event, and there is no clean way to signal this condition. The best I've been able to come up with is to send a dummy key down/up event for the KEY_UNKNOWN key. This appears to work - see my latest repo update. However, I'm not sure how kosher this is and whether it'll survive scrutiny when we try to upstream this driver.

Dunedan commented 6 years ago

I did a check what Apple does in this situation under macOS, because maybe they have some smart handling for that. Turns out: no, their behavior is exactly the same as with your patch. Additionally I checked what Linux does with the hid_apple driver on a MacBook Air. Again: Same behavior as with your patch. So I'd say, opening a PR with your existing patch is the most pragmatic solution for this issue.