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 DOUBLE_REPORT to fix RDP modifier key reliability #19449

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Taudris
Copy link

@Taudris Taudris commented Dec 31, 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, but I've experienced as high as 50% for some keys.)

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

In my testing so far, this almost completely resolves the issue for MS RDP and Hyper-V. Out of several hundred keypresses, one might still fail to have the correct modifiers applied. This error rate is below typical human error when typing, so it is essentially transparent in real use.

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.

This PR replaces #19436.

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
Copy link

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Feb 24, 2023
@waffle87 waffle87 added awaiting review and removed stale Issues or pull requests that have become inactive without resolution. labels Feb 24, 2023
@sambaum
Copy link
Contributor

sambaum commented Mar 6, 2023

I have tested this for a few weeks now and had no issues with it. I would say this approach is better than in #19405
@drashna, in #19405 you have stated some concerns about solving the RDP issue by introducing a delay. So what do you think about this approach?

@drashna
Copy link
Member

drashna commented Mar 6, 2023

I've been using it for a while now, and it does work well. However, I'm not as knowledgeable about the USB stuff, so I'd rather defer to others that are much more knowledgeable.

Though, the name ... Could be better. HOST_SEND_DOUBLE_REPORT or some such. Makes it much clearer what the intent is.

@ntzm
Copy link

ntzm commented Mar 24, 2023

This solves the issue I was having with Wayland Ubuntu 22.04 terminal not recognising auto shifted keys, thank you

@Taudris
Copy link
Author

Taudris commented Mar 25, 2023

Other name ideas that include QMK function names in the name:

  • HOST_KEYBOARD_SEND_TWICE (I think I prefer this one at the moment)
  • DOUBLE_HOST_KEYBOARD_SEND
  • SEND_KEYBOARD_REPORT_TWICE
  • DOUBLE_SEND_KEYBOARD_REPORT

@IsaacElenbaas
Copy link
Contributor

This solves the issue I was having with Wayland Ubuntu 22.04 terminal not recognising auto shifted keys, thank you

@ntzm Auto Shift doesn't currently apply TAP_CODE_DELAY between modifier changes and key presses/releases, just between the key presses/releases. There are a couple of other issues open related to that. A PR to do that would also probably fix that for you if this ends up having any drawbacks or side effects.

@Roming22
Copy link

Roming22 commented Apr 2, 2023

It does not solve the issue if the press/release of the key occurs in a very short time frame.

Not sure if that's relevant, but in my case the key is part of a layer activated by an LT key.

@drashna drashna requested a review from a team April 4, 2023 21:30
alsatierf pushed a commit to alsatierf/qmk that referenced this pull request Jul 25, 2023
- Use qmk/qmk_firmware#19449 to fix Alt as OSM
  over RDP.
- SPC + LSFT on base layer
- OSM for CGAS + arrows on layer 1
- OSM activated by combos in any layer
- Use autoshift to deal with Ç and KC_GRV on layer 1
@cromulus80
Copy link

Is there an update on this?

@sambaum
Copy link
Contributor

sambaum commented Oct 6, 2023

It does not solve the issue if the press/release of the key occurs in a very short time frame.
Not sure if that's relevant, but in my case the key is part of a layer activated by an LT key.

I had the same issue, so I went back to #19405, and I have never had any issues again. Unfortunately this PR is also stale/awaiting review.

Riskipa

This comment was marked as spam.

@arman639
Copy link

arman639 commented Dec 19, 2023

I was using the REPORT_MOD_SEPARATELY from #19436 (by @Taudris as well) for the entire year already and was satisfied with that fix but lately I was noticing inconsistencies like before. Not sure if it was due to RDP itself or network related issue. After trying the solution on this new PR, the error rate becomes significantly lower.

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.

10 participants