search

tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
3.05k stars 255 forks source link

Modifier keys don't work in combination with mouse clicks #620

Closed mtlynch closed 3 years ago

mtlynch commented 3 years ago

Description

TinyPilot doesn't forward input events correctly for an event like Shift + left click.

What are the steps to reproduce this behavior?

  1. Open TinyPilot in Chrome
  2. Open a text field on the remote screen
  3. Hold shift and click different spots in the text

What's the behavior that you expect?

The target computer recognizes a shift + click and highlights the text

What's happening instead?

No text is highlighted

Example

https://user-images.githubusercontent.com/7783288/112372270-f4dc9c80-8cb5-11eb-89f4-529efaa88997.mp4

mtlynch commented 3 years ago

A user reports that this was working in 1.3.0 but broke after that. I haven't tested a 1.3.0 build to reproduce the working behavior.

jdeanwallace commented 3 years ago

A user reports that this was working in 1.3.0 but broke after that. I haven't tested a 1.3.0 build to reproduce the working behavior.

I tested with tag 1.3.0 and I still can't reproduce the working behavior. Here's an underwhelming demo video.

mtlynch commented 3 years ago

I think we're forwarding the shift key event incorrectly.

If the user presses the left shift key on its own, TinyPilot currently sends:

0x02 0x00 0x00 0x00 0x00 0x00 0x00 0x00
^^^  [       All other keys zero'ed        ]
 |
 |
 |
 Left shift modifier key

What we probably want is:

0x02 0xe1 0x00 0x00 0x00 0x00 0x00 0x00
^^^^ ^^^^  [  All other keys zero'ed   ]
 |     |
 |   Left shift key code
 |
 Left shift modifier key

Key code from HID spec, but also here

I tried sending the raw HID code directly from the command line to simulate holding down right shift and TinyPilot had the desired shift+click behavior when I used the mouse:

https://user-images.githubusercontent.com/7783288/117991413-046a8000-b30c-11eb-8dca-0b734942a501.mp4

I haven't tested it yet, but I recommend connecting a real, physical keyboard to a test machine and running usbhid-dump, then see what HID data a real keyboard sends when the user hits a modifier key in isolation. The part I'm not sure about is whether the keyboard still sets modifier bits (the first byte of the HID message) when you type a modifier key by itself.

Then, we'll need to update the HID logic on TinyPilot to match the behavior of a real keyboard for shift and all other modifiers.

jdeanwallace commented 3 years ago

Thanks for the advice. I tested using a physical keyboard & mouse connected to the host machine. Here's a video of that.

Summary:

  1. A physical keyboard also doesn't set the key code (when shift is pressed). It only sets the control key. So TinyPilot is doing the right thing. The problem is that TinyPilot always releases the key after it's been pressed. The problem code here should also check if a control key has been pressed before releasing the key.
  2. A physical mouse only writes 4 bytes, but TinyPilot writes 7 bytes. When streaming the HID input it looks like the buffer is overflowing (it seems like it's expecting a maximum of 5 bytes, then wraps the extra 2 bytes). Do you have a reference to the protocol we're using?
  3. (not shown in the video) When fixing no.1, I'm able to press shift via the TinyPilot web UI and click via the host machine mouse. This results in an expected shift+click behaviour.
mtlynch commented 3 years ago

A physical keyboard also doesn't set the key code (when shift is pressed). It only sets the control key. So TinyPilot is doing the right thing. The problem is that TinyPilot always releases the key after it's been pressed. The problem code here should also check if a control key has been pressed before releasing the key.

Ah, interesting! Looks like that's the issue.

A physical mouse only writes 4 bytes, but TinyPilot writes 7 bytes. When streaming the HID input it looks like the buffer is overflowing (it seems like it's expecting a maximum of 5 bytes, then wraps the extra 2 bytes). Do you have a reference to the protocol we're using?

The report size varies by mouse, as different mice offer different features. TinyPilot's mouse is a little wacky because it's an absolute mouse (it reports absolute position on the screen, like a drawing tablet). Most physical mice are relative mice, meaning that when you move them left 1cm, they move the cursor left by however many pixels. But TinyPilot needs to specify an absolute position (when the mouse is over point x,y on the remote image, TinyPilot needs to move the remote cursor to position x', y' on the remote screen, scaling for differences in screen size).

We specify the HID descriptor here:

https://github.com/mtlynch/ansible-role-tinypilot/blob/ed29bacf132625c26b24898c7a08376ba3d22c61/files/init-usb-gadget#L81

We declare a report_length of 5, so I think you're right about writing too many bytes.

The best resources I've found about the HID descriptor are these posts:

It'd be good to confirm from reading the docs that 5 bytes is correct and then make a separate fix for that.