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 REPORT_MODS_SEPARATELY to fix RDP modifier key reliability #19436

Closed
wants to merge 3 commits into from

Conversation

Taudris
Copy link

@Taudris Taudris commented Dec 28, 2022

Description

QMK has trouble sending modifier keys (eg LSFT(KC_TAB)) to remote computer GUIs via software such as Microsoft's Remote Desktop, and the local Hyper-V viewer. The modifier is frequently lost, causing the unmodified key to be pressed instead. (Exactly how often the modifiers are lost seems to be environment-specific.)

This PR adds the ability for QMK to report changes to modifiers separately from changes to keys. Users need only add #define REPORT_MODS_SEPARATELY to their config.h.

In my testing so far, this completely resolves the issue for MS RDP and Hyper-V.

The other proposed solutions I've seen so far (eg #19405, and code snippets in the issues referenced below) all involve adding a delay somewhere in the QMK code. I found in my own testing that this type of solution is not adequate; it can reduce the frequency of occurrence, but it does not seem to completely eliminate it, and may not work with all QMK features. Tuning the delay is also a hassle, and a given delay amount may not even work optimally for all environments.

This solution may also make #4198 obsolete.

Other notes:

  • It's possible that this behavior should be the default for QMK.
  • I have not tested key combinations which use multiple modifiers.
  • It's possible that it would be better to send a single key change at a time for modifiers, keys, or both.
  • Since host_keyboard_send() modifies the keyboard report for some configurations, I also swapped the order of host_keyboard_send() and memcpy(). This change could (should?) be a separate PR.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@github-actions github-actions bot added the core label Dec 28, 2022
@arman639
Copy link

Thanks, it looks like this fixed my issue. So far after trying it for several minutes, I can't replicate the previous issue anymore while connected to RDP whereas previously I can by just doing few keypresses. I have not tried yet testing other key combinations with modifiers though.

@Taudris
Copy link
Author

Taudris commented Dec 30, 2022

Unfortunately, this is not a correct fix.

From an end user perspective, I'm definitely on to something. After now a couple days of using it for programming via RDP, I can confirm that this fix works just absolutely amazingly. No other fix I've tried even comes close.

From reading QMK's key handling logic in more detail, it actually already separates modifier changes from key state changes. I confirmed this using debug output. So REPORT_MODS_SEPARATELY should have had no effect whatsoever in the first place.

I also confirmed that defining a key as KC_LPRN (which maps to LSFT(KC_9)) produces the exact same sequence of reports as manually pressing LShift+9, though obviously with very different timing.

The main difference caused by REPORT_MODS_SEPARATELY at the moment is that QMK is now sending extra key reports. That's happening because swapping host_keyboard_send() and memcpy() actually introduces a defect, and the original order is correct. So..yay for creating an accidental fix?

I tested simply doubling up every report, and that seems to be performing similarly. Out of several hundred presses of repeatedly pressing KC_LPRN (arm got tired lol), I got just one where the modifier was not handled correctly and I got a 9 instead. I'll update this pull request accordingly when I have more time.

I don't know if this new fix is good enough to make it a default behavior, but I do think it should be available for users to enable when needed.

@Taudris
Copy link
Author

Taudris commented Dec 31, 2022

See #19449 for a correct fix.

@Taudris Taudris closed this Dec 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants