search

tiny-pilot / tinypilot

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

Support multiple keycodes per HID keystroke #1707

Closed jdeanwallace closed 7 months ago

jdeanwallace commented 7 months ago

Related https://github.com/tiny-pilot/tinypilot/issues/1167

This PR adds support for writing multiple keycode combinations to the HID by refactoring the HID Keystroke object to expect a list of keycodes instead of only a single keycode.

This PR is a non-functional change and currently we still only write a single keycode to the HID, but at least we can support writing more.

This is how the code was refactored:

  1. Find and replace HID Keystroke.keycode: int with Keystroke.keycodes: List[int]
    1. Find: r'keycode=([\w\.\\_]+)'
    2. Replace: r'keycodes=[$1]'
  2. Find and replace HID Keystroke.keycodes=[KEYCODE_NONE] with Keystroke.keycodes=[]
  3. Map a JS keystroke to a list of keycodes
    1. _map_keycode(keystroke): int -> _map_keycodes(keystroke): List[int]
  4. Actually write multiple keycodes to the HID: https://github.com/tiny-pilot/tinypilot/blob/d12e94e02c9d019307387eae163c0704d7c5cc9d/app/hid/keyboard.py#L7-L8 Review on CodeApprove
jdeanwallace commented 7 months ago

@mtlynch - What do you think of this refactoring? I know it still creates a lot of code churn, but most of it was bulk find + replace. I think this is the simplest way to support keycode combinations under the hood.

In order to prevent very risky code changes with how we send/receive/parse JS keystrokes, I think it's possible to "hack" on an extra "sysRq" field without fundamentally changing our system and tests. Should I explore this idea further in a stacked PR?

mtlynch commented 7 months ago

Let's shelve this for now. We've punted on #1167, so I don't want to keep investing in it.

I think #1704 is a worthwhile refactoring to capture because it simplifies the code and brings us closer to a solution to SysRq. This PR brings us closer to a solution to SysRq but it adds complexity that doesn't provide value right now.

One of the rules I try to follow is "don't design into the future." We should make sure the codebase serves our current need as simply as possible. When we're ready to extend the behavior, then we should revisit adding support for multiple keys in a single HID message.