tiny-pilot / tinypilot

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

Add Magic SysRq key #1703

Closed jdeanwallace closed 9 months ago

jdeanwallace commented 10 months ago

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

Review on CodeApprove

jdeanwallace commented 9 months ago

Progress update: 2023-12-13

@mtlynch - I have a POC working for pressing Alt + SysRq + H via the on-screen keyboard:

https://github.com/tiny-pilot/tinypilot/assets/6730025/064345f2-51e2-43ef-a5b2-822d79d76da2

However, I'd like the flag that the approach I'm taking is creating a lot of churn because

  1. The HID Keystroke object now supports multiple keycodes to allow support for pressing multiple key combinations: https://github.com/tiny-pilot/tinypilot/blob/f08a50657d576bafd77840a18c44cf3f51e1036d/app/hid/keycodes.py#L160-L163

  2. The JS to HID keycode conversion has been simplified by reducing the JS keystroke object to just a list of JS keycodes being pressed. For example:

    • Before:
      {
          "ctrlLeft": False,
          "ctrlRight": False,
          "shiftLeft": True,
          "shiftRight": False,
          "altLeft": False,
          "altRight": False,
          "metaLeft": False,
          "metaRight": False,
          "key": "H",
          "code": "KeyH"
      }
    • After:
      {
          "codes": ["ShiftLeft", "KeyH"]
      }

    I'm not too sure what the downsides of this approach might be 🤔

  3. I simplified the on-screen keyboard slightly to not encode the "is a modifier key" into the UI and instead rely on our isModifierCode helper function.

Known issues so far

  1. While typing normally, if two keys are pressed at the same time, they are both written to the HID. For example, when I type the word "clear", it would come out as "cleear" because I briefly pressed "e" and "a" at the same time so the following individual keystrokes are sent:

    1. ["KeyC"]
    2. ["KeyL"]
    3. ["KeyE"]
    4. ["KeyE", "KeyA"]
    5. ["KeyR"]

    I think this is a minor JS bug and can be fixed with some effort.

Proposed next steps

mtlynch commented 9 months ago

Yeah, this is looking like a pretty risky change for what we're trying to accomplish.

Can we do this:

  1. Pull out the refactorings in (2) and (3) to their own PRs
    • If they simplify the code and make it friendlier to future changes to allow this feature, then we might as well capture them now.
  2. Bump #1167 with a summary of what we know now and what the possible paths are for getting back the functionality

Also, I'm still confused. Was this a true regression or did this functionality never work? If this functionality worked at one point, how did we do that without the broader changes that we're using now?

jdeanwallace commented 9 months ago

@mtlynch

Can we do this:

Sure, I'll update the issue thread with what I know.

Was this a true regression or did this functionality never work?

Looking at the code, I can't see how SysRq combination keys ever worked. However, I do believe that the PrintScreen key on its own did work. The PR that resolved the original issue was only a frontend change and the backend never wrote more than a single keycode byte to the HID: https://github.com/tiny-pilot/tinypilot/blob/4587f989b6d479034a64b2411c1c9964cdad7261/app/hid.py#L44

Disclaimer: I did not try resurrect the historic build to a device.