Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Magic SysRq key #1703

Closed
wants to merge 7 commits into from
Closed

Add Magic SysRq key #1703

wants to merge 7 commits into from

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented Dec 8, 2023

Resolves #1167

Review on CodeApprove

@jdeanwallace
Copy link
Contributor Author

Progress update: 2023-12-13

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

Screen.Recording.2023-12-12.at.17.30.44.mov

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:

    @dataclasses.dataclass
    class Keystroke:
    keycodes: List[int]
    modifier: int = KEYCODE_NONE

  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

  • Option A
    1. Halt PR
    2. Continue in current code trajectory by creating a list of tasks needed to achieve support for Magic SysRq key
    3. Reach task list consensus
  • Option B
    1. Halt PR
    2. Find a simpler way to achieve support for Magic SysRq key without so much code churn that maintains the current status quo (in the form of another POC PR)

@mtlynch
Copy link
Contributor

mtlynch commented Dec 13, 2023

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 No longer possible to enter the Magic SysRq key combination #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
Copy link
Contributor Author

jdeanwallace commented Dec 14, 2023

@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:

buf[2] = hid_keycode

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

jdeanwallace added a commit that referenced this pull request Dec 16, 2023
Related #1167

This PR fixes a minor bug in our keyboard state of currently pressed
keys. I discovered this issue while exploring
#1703.

The issue was that when I press <kbd>Meta</kbd> + <kbd>L</kbd> in
chrome, the focus switches to the address bar and I release all keys,
but the keyboard state still thinks `KeyL` is pressed.

I used #1705 to investigate
the keyboard state.


<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1706"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No longer possible to enter the Magic SysRq key combination
2 participants