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

Allow passing bare modifier keys (control, alt, shift) when triggered alone #507

Merged
merged 1 commit into from
Feb 15, 2021

Conversation

WarheadsSE
Copy link
Contributor

@WarheadsSE WarheadsSE commented Feb 15, 2021

Allow the Keyboard hit to pass the bare modifier keys, such that hardware KVM key sequences can be used.

In order for hardware KVM switches to recognize patterns such as right-control, right-control, #, enter, we must recognize and pass the correct sequence onward. In testing, this means passing only the control_keys and not the HID keycode itself.

This adds js_to_hid._map_keycode, called from js_to_hid.convert. hid.keycodes.KEYCODE_NONE which is 0 for "no event indicated" per https://www.usb.org/sites/default/files/documents/hut1_12v2.pdf (page 53)

Updated js_to_hid_test to account for the expected changes within asserts for ctrl only tests

Relates to #78

Reported types from #78, and other personal experiences:

  • scroll lock, scroll lock, number
  • right-ctl, right-ctl, number, enter
  • left-ctrl, left-ctl, number
  • right-ctl, right-ctl, left arrow || right arrow

Willing to adjust/update for others, but built on in-place installation in rack.

@WarheadsSE
Copy link
Contributor Author

I can see that it appears something is disliked by the build_python ... but I can't see what without logging into Circle CI.

@mtlynch
Copy link
Contributor

mtlynch commented Feb 15, 2021

Is it possible for you to log in to CircleCI? You should be able to log in with your Github account for free.

@WarheadsSE
Copy link
Contributor Author

Is it possible for you to log in to CircleCI? You should be able to log in with your Github account for free.

One more account ... 😒

Looks like a formatting complaint? Addressed.

@WarheadsSE
Copy link
Contributor Author

I need to update the description / commit comment. The "math" is not based on left/right ctrl directly, but on the offset. I'm remember this now as the original change was made a month ago and I forgot 😅

@mtlynch
Copy link
Contributor

mtlynch commented Feb 15, 2021

Just as an FYI, you can run the build scripts locally. See CONTRIBUTING.md for details.

@WarheadsSE WarheadsSE force-pushed the kb-bare-modifier branch 2 times, most recently from 4cbbeb8 to 16f34af Compare February 15, 2021 17:28
@WarheadsSE
Copy link
Contributor Author

Just as an FYI, you can run the build scripts locally. See CONTRIBUTING.md for details.

You're right. I've been running around overly busy, my apologies for the noise.

@mtlynch
Copy link
Contributor

mtlynch commented Feb 15, 2021

Thinking about this a bit more, I think the better place to apply this logic would be in js_to_hid.

The keyboard HID module is pretty low-level, so we want to keep it as simple as possible.

js_to_hid is easy to unit test, so we should update the test case for left-ctrl alone and add a test case for left-ctrl + other modifier.

@WarheadsSE
Copy link
Contributor Author

I am not as familiar with the code as you are (obviously), but we'd want to keep concern for more than just left-ctl. I've updated the PR description to clarify, as previously mentioned.

I'm looking at the simplicity of convert, and all logic is really handed off to _map_modifier_keys. I believe I looked at this prior, as I remember looking this function. I will spend some time looking through the callers, and follow the path backwards.

We need to specifically pass 0x00 as the hid_keycode for send_keystroke, or this will not function properly.

@mtlynch mtlynch marked this pull request as draft February 15, 2021 18:39
@mtlynch
Copy link
Contributor

mtlynch commented Feb 15, 2021

We need to specifically pass 0x00 as the hid_keycode for send_keystroke, or this will not function properly.

Yep, that should be fine. keystroke.py is just responsible for sending a HID, whereas js_to_hid is the module responsible for generating the values of the control keys and keycodes.

@WarheadsSE
Copy link
Contributor Author

@mtlynch If you want this logic moved into js_to_hid.convert and such that we return 0x00 for hid_keycode, we'll also have to look at the impact of socket_api.socket_keystroke which calls both js_to_hid.convert and eventually fake_keyboard.send_keystroke directly. The only "don't whoopsy" I see there is the direct check of if hid_keycode is None. To that end, we'd need to either explicitly return 0x00, or add hid.KEYCODE_EMPTY = 0 for sanity.

I believe we'll need to update convert to call & store appropriately for handling the modifier keys & codes, as we can't "just return X from dict" as currently in place. I'll attempt to sort out the cleanest method, but am leaning to a _map_keycode(keystroke).

Please bear in mind, my Python is quite rusty, as it is not something I have use extensively for several years.

Allow the Keyboard hit to pass the bare modifier keys, such that
hardware KVM key sequences can be used.

In order for hardware KVM switches to recognize patterns such as
`right-control, right-control, #, enter`, we must recognize and pass the
correct sequence onward. In testing, this means passing only the
`control_keys` and not the HID keycode itself.

This adds `js_to_hid._map_keycode`, called from `js_to_hid.convert`.
`hid.keycodes.KEYCODE_NONE` which is `0` for "no event indicated" per
https://www.usb.org/sites/default/files/documents/hut1_12v2.pdf (page 53)

Updated `js_to_hid_test` to account for the expected changes within
asserts for `ctrl` only tests.
@WarheadsSE WarheadsSE marked this pull request as ready for review February 15, 2021 21:29
@WarheadsSE WarheadsSE changed the title hid/keyboard: allow passing bare modifier keys Allow passing bare modifier keys (control, alt, shift) when triggered alone Feb 15, 2021
@mtlynch
Copy link
Contributor

mtlynch commented Feb 15, 2021

LGTM, thanks!

@mtlynch mtlynch merged commit 1d941a1 into tiny-pilot:master Feb 15, 2021
@WarheadsSE WarheadsSE deleted the kb-bare-modifier branch February 16, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants