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

Windows Remote Desktop Cannot Recognize Shifted Keys like KC_LPRN 100% of the time #1037

Closed
IBNobody opened this issue Jan 26, 2017 · 16 comments

Comments

@IBNobody
Copy link
Contributor

When attempting to send a "(" key via KC_LPRN to a remote desktop session, the LShift operation does not always register correctly. Instead of a "(", I get a "9".

This is a showstopper for me, as I do most of my work-work through remote desktop sessions.

I used Switch Hitter to get a log of what was happening. In the log below, I hold down a key assigned to KC_LPRN for 6 seconds. You can see immediately that LShift gets sensed as UP.

40:36.0967 LShift (0x10, BIOS 0x2A) DOWN
40:36.0969 LShift (0x10, BIOS 0x2A) UP -> 3ms
40:36.0971 9 (0x39, BIOS 0x0A) DOWN
40:43.0485 9 (0x39, BIOS 0x0A) UP -> 6514ms
40:43.0488 LShift (0x10, BIOS 0x2A) UP -> 6517ms

This does not happen 100% of the time either. Sometimes it works flawlessly, repeatedly. Other times, all I get are 9's.

Here is an example of a success.

50:05.0943 LShift (0x10, BIOS 0x2A) DOWN
50:05.0945 9 (0x39, BIOS 0x0A) DOWN
50:07.0061 9 (0x39, BIOS 0x0A) UP -> 1116ms
50:07.0063 LShift (0x10, BIOS 0x2A) UP -> 1120ms

I have no problems in my normal Win10 environment.

My debounce was set to 5. I switched it to 10 without effect.

@priyadi
Copy link
Contributor

priyadi commented Jan 26, 2017

This probably has nothing to do with QMK. You might want to read this or this.

@IBNobody
Copy link
Contributor Author

IBNobody commented Jan 26, 2017 via email

@fredizzimo
Copy link
Contributor

fredizzimo commented Jan 27, 2017

One interesting observation is that there are two UPs, and only one DOWN. I wonder how that's even possible, since the keyboard report only reports the states of the keys.

Edit Maybe it would be good to run Switch Hitter on both the host and the virtual machine at the same time, if possible, to see if there's any difference. If that's not possible, then maybe just running it on the host could show some other pattern.

@IBNobody
Copy link
Contributor Author

Host Machine

35:30.0905 LShift (0x10, BIOS 0x2A) DOWN
35:30.0907 0 (0x30, BIOS 0x0B) DOWN
35:31.0881 0 (0x30, BIOS 0x0B) UP -> 973ms
35:31.0883 LShift (0x10, BIOS 0x2A) UP -> 976ms

I cannot run Switch Hitter and have it collect data from both at the same time because it only grabs keystrokes when it is the active window. However, I CAN use AutoHotkey.

Here is what I see when I snoop using AHK.

NOTE: To disable the key history shown below, add the line "#KeyHistory 0" anywhere in the script.  The same method can be used to change the size of the history buffer.  For example: #KeyHistory 100  (Default is 40, Max is 500)

The oldest are listed first.  VK=Virtual Key, SC=Scan Code, Elapsed=Seconds since the previous event.  Types: h=Hook Hotkey, s=Suppressed (blocked), i=Ignored because it was generated by an AHK script, a=Artificial, #=Disabled via #IfWinActive/Exist, U=Unicode character (SendInput).

VK  SC	Type	Up/Dn	Elapsed	Key		Window
-------------------------------------------------------------------------------------------------------------

... This is what I see on the local machine.

A0  02A	 	d	4.28	LShift         	
30  00B	 	d	0.00	0              	
30  00B	 	u	0.17	0              	
A0  02A	 	u	0.00	LShift    

... This is what I see when using RDP. Success and failure looked the same.

FF  000	a	d	5.13	not found      	xxxx - Remote Desktop Connection
FF  000	a	u	0.00	not found      	
FF  000	a	d	0.01	not found      	
FF  000	a	u	0.00	not found      	
A0  02A	a	d	4.49	LShift         	
A0  02A	a	u	0.11	LShift       

... This is another case, but this was me holding down the key.
FF  000	a	d	4.11	not found      	pvc02 - Remote Desktop Connection
FF  000	a	u	0.00	not found      	
FF  000	a	d	0.02	not found      	
FF  000	a	u	0.00	not found      	
A0  02A	a	d	4.61	LShift         	
A0  02A	a	u	3.08	LShift         	
FF  000	a	d	21.33	not found      	
FF  000	a	u	0.00	not found   

What I need to try is to create a macro that adds in delays between shifting and pressing.

@fredizzimo
Copy link
Contributor

Those reports doesn't make sense, even the generated keycodes are messed up.

But did you try this workaround, which many people reported working?

Open Remote Desktop Connection and go to:
Options -> Local Resources -> Keyboard -> Apply Windows key combinations -> On the local computer

Even if that fixes the situation, it's still possible that there's something in QMK that breaks it, as someone reported it was broken by a Windows update for "IntelliType"

@IBNobody
Copy link
Contributor Author

I did try what they recommended, and while it worked, it had a side effect of blocking remote desktop shortcut keys like Alt-Tab. So, showstopper.

I did find a solution, though.

A delay needs to exist between registering the shift and pressing the key.


// When triggered, this case either outputs a 0 or a ) in a remote desktop session.
    case MACRO_HELP_1:
      if (record->event.pressed)
      {
            register_code(KC_LSFT);
            register_code(KC_0);
            unregister_code(KC_0);
            unregister_code(KC_LSFT);
      }
      break;

// When triggered, this case always outputs a ) in a remote desktop session.

    case MACRO_HELP_2:
      if (record->event.pressed)
      {
            register_code(KC_LSFT);
            { uint8_t ms = 10; while (ms--) wait_ms(1); } // unelegant 10ms delay code used in ACTION_MACRO
            register_code(KC_0);
            unregister_code(KC_0);
            unregister_code(KC_LSFT);
      }
      break;

What would the best way to address this?

My thought would be to add in a delay constant (default = 0) between registering the modifier and registering the key. Maybe mod do_code16?

  if (code & QK_LSFT) {
    f(KC_LSFT);
#if (MOD_DELAY_WAIT > 0)
    wait_ms(MOD_DELAY_WAIT);
#endif
}

@fredizzimo
Copy link
Contributor

I would do it at a lower level in send_keyboard_report inaction_util.c. This way it could handle all the different way of enabling modifiers real_mods, weak_mods and macro_mods, so it would work properly with all QMK features.

If any modifiers have been changed since the last report, then you could just do a wait after sending the report. I think it's the safest to check for all changes rather than just key down, since there's probably the same problem with releasing shift as well. Other modifiers might have the same problem too.

A slightly more advanced version of the algorithm would be to not wait immediately after sending the report, but only when trying to send another conflicting report too soon afterwards. But this would require a timer and saving of the previous report to detect changes. The saving might not be strictly necessary if you hook into the functions that modifies it, like add_key, del_key and so on. This algorithm would have the advantage that it doesn't introduce extra lag, unless it has to.

@IBNobody
Copy link
Contributor Author

I would do it at a lower level in send_keyboard_report inaction_util.c. This way it could handle all the different way of enabling modifiers real_mods, weak_mods and macro_mods, so it would work properly with all QMK features.

What about adding an if-then to the above code to detect changes to keyboard_report->mods?

void send_keyboard_report(void) {
    static uint_8 old_mods;
    keyboard_report->mods  = real_mods;
    keyboard_report->mods |= weak_mods;
    keyboard_report->mods |= macro_mods;
#ifndef NO_ACTION_ONESHOT
    if (oneshot_mods) {
#if (defined(ONESHOT_TIMEOUT) && (ONESHOT_TIMEOUT > 0))
        if (has_oneshot_mods_timed_out()) {
            dprintf("Oneshot: timeout\n");
            clear_oneshot_mods();
        }
#endif
        keyboard_report->mods |= oneshot_mods;
        if (has_anykey()) {
            clear_oneshot_mods();
        }
    }

#endif
    host_keyboard_send(keyboard_report);
#if (KEYBOARD_MOD_PACKET_DELAY > 0)
    if (keyboard_report->mods != old_mods) {
        wait_ms(KEYBOARD_MOD_PACKET_DELAY);
    }
#endif
#if (KEYBOARD_GENERAL_PACKET_DELAY > 0)
    wait_ms(KEYBOARD_GENERAL_PACKET_DELAY);
#endif
}

This way only gives you a delay if your mods changed.

Another delay can be used to pad the entire packet in case we run into problems with key-ups and key-downs for non-modifiers.

Both delays would be optional and default to 0.

A slightly more advanced version of the algorithm would be to not wait immediately after sending the report, but only when trying to send another conflicting report too soon afterwards. But this would require a timer and saving of the previous report to detect changes. The saving might not be strictly necessary if you hook into the functions that modifies it, like add_key, del_key and so on. This algorithm would have the advantage that it doesn't introduce extra lag, unless it has to.

Hmm... I don't know... This may be overkill. Why does it matter that there is a delay after a keypress if there is no follow-up keypress?

@fredizzimo
Copy link
Contributor

Yes, that was essentially what I was thinking.

There's a bug in your code snippet though, old_mods is not initialized and updated. But I think that it might not be needed, since the mods in the keyboard_report variable is never cleared.

And yes you are right that the advanced solution most likely is an overkill. It would reduce or eliminate the lag when you press shift before the actual key. But it's probably so small that it's not noticeable in any case.

One thing that has to be remembered about delays like these, is that the normal matrix scan loop is not run during the wait, so it doesn't react to new keystrokes during that time. It also doesn't run the debouncing logic. So on most of our debouncing logic implementations, which just counts the number of scans, there would be additional delays. Delays could also disrupt visual or audio effects. So only enabling them through ifdefs is a good idea. If the negative effects are too big it could even be something that can be toggled by a macro key.

@IBNobody
Copy link
Contributor Author

Could we be more surgical than what I showed in my second pseudocode snippit?

The true issue wasn't that modifiers need a delay. It was that events that force one or more keys to be registered at once need to have a delay, and this is mainly caused by modifiers and keys. Humans firing modifiers by themselves are not the issue.

The true culprets are...

// Ability to use mods in layouts
#define LCTL(kc) (kc | QK_LCTL)
#define LSFT(kc) (kc | QK_LSFT)
#define LALT(kc) (kc | QK_LALT)
#define LGUI(kc) (kc | QK_LGUI)
#define RCTL(kc) (kc | QK_RCTL)
#define RSFT(kc) (kc | QK_RSFT)
#define RALT(kc) (kc | QK_RALT)
#define RGUI(kc) (kc | QK_RGUI)

That is what I was initially targeting with the first snippit, but I may not have chosen the right place.

Thoughts?

@IBNobody
Copy link
Contributor Author

I am currently in trials for a 10ms delay. I will report back to see if it solved the issue. My semifinal code is...

#include "wait.h"

.......

#ifndef KEYBOARD_MOD_PACKET_DELAY
#define KEYBOARD_MOD_PACKET_DELAY 0
#endif

#ifndef KEYBOARD_GENERAL_PACKET_DELAY
#define KEYBOARD_GENERAL_PACKET_DELAY 0
#endif

.......

void send_keyboard_report(void) {

#if ((KEYBOARD_MOD_PACKET_DELAY > 0) || (KEYBOARD_GENERAL_PACKET_DELAY > 0))

    // Keep track of the state of mods
    uint8_t old_mods = keyboard_report->mods;
#endif

    keyboard_report->mods  = real_mods;
    keyboard_report->mods |= weak_mods;
    keyboard_report->mods |= macro_mods;

#ifndef NO_ACTION_ONESHOT
    if (oneshot_mods) {
#if (defined(ONESHOT_TIMEOUT) && (ONESHOT_TIMEOUT > 0))
        if (has_oneshot_mods_timed_out()) {
            dprintf("Oneshot: timeout\n");
            clear_oneshot_mods();
        }
#endif
        keyboard_report->mods |= oneshot_mods;
        if (has_anykey()) {
            clear_oneshot_mods();
        }
    }

#endif

    host_keyboard_send(keyboard_report);

#if (KEYBOARD_MOD_PACKET_DELAY > 0)
    // If the mods are changing...
    if (keyboard_report->mods != old_mods) {
        // Wait for a fixed amount of time to allow the host to process the report
        wait_ms(KEYBOARD_MOD_PACKET_DELAY);
    }
#endif
#if (KEYBOARD_GENERAL_PACKET_DELAY > 0)
    // Wait for a fixed amount of time to allow the host to process the report
    wait_ms(KEYBOARD_GENERAL_PACKET_DELAY);
#endif
}

@IBNobody
Copy link
Contributor Author

IBNobody commented Feb 16, 2017

The packet delay seems to be working for me. I have not encountered the issue. I will work to merge the changes in later this week.

@priyadi
Copy link
Contributor

priyadi commented Feb 17, 2017

@IBNobody Have you ever tried it with higher latency connection? Like 300ms or more. I think I got the same problem with the IPMI remote management thingy that came with some server motherboards.

@IBNobody
Copy link
Contributor Author

@priyadi I can try it tomorrow by accessing a machine in Malaysia. No promises on how bad the latency will be. But half-a-world is a good enough field test for me..

@IBNobody
Copy link
Contributor Author

@priyadi Yes, it seems to work in a remote desktop session from the US to Malaysia. I see significant input lag, but the key combos work correctly.

drashna added a commit to drashna/qmk_firmware that referenced this issue Oct 21, 2018
And some formatting and grammar corrections.

Closes qmk#1037
jackhumbert pushed a commit that referenced this issue Oct 22, 2018
* Add caveats for shifted characters

And some formatting and grammar corrections.

Closes #1037

* Fix spelling of remote

* Fix spelling error

* Remote not Remove

* Remote not Remove
zer09 pushed a commit to zer09/qmk_firmware that referenced this issue Nov 3, 2018
* Add caveats for shifted characters

And some formatting and grammar corrections.

Closes qmk#1037

* Fix spelling of remote

* Fix spelling error

* Remote not Remove

* Remote not Remove
3n4rKy pushed a commit to 3n4rKy/qmk_firmware that referenced this issue Nov 4, 2018
* Add caveats for shifted characters

And some formatting and grammar corrections.

Closes qmk#1037

* Fix spelling of remote

* Fix spelling error

* Remote not Remove

* Remote not Remove
rseymour pushed a commit to rseymour/qmk_firmware that referenced this issue Mar 13, 2019
* Add caveats for shifted characters

And some formatting and grammar corrections.

Closes qmk#1037

* Fix spelling of remote

* Fix spelling error

* Remote not Remove

* Remote not Remove
yamad pushed a commit to yamad/qmk_firmware that referenced this issue Apr 10, 2019
* Add caveats for shifted characters

And some formatting and grammar corrections.

Closes qmk#1037

* Fix spelling of remote

* Fix spelling error

* Remote not Remove

* Remote not Remove
@sambaum
Copy link
Contributor

sambaum commented Oct 18, 2022

I am currently in trials for a 10ms delay. I will report back to see if it solved the issue. My semifinal code is...

Wow, this really helped me! Thank you so much! I'm often dealing with remote connections like Citrix or other access methods where I need to jump over multiple RDP connections. I have no choice but to use them. This issue really bothered me and I had to create macros for important things, which was really annoying. I've implemented this fix in my current QMK version and this works really well! I've tested it over a few weeks and haven't encountered any issues. I have removed all of my workaround-macros and replaced them with regular modifiers.
Has there ever been an attempt to create a pull request with this feature?

PS: My sweat spot was with KEYBOARD_MOD_PACKET_DELAY 20

@sambaum sambaum mentioned this issue Dec 22, 2022
14 tasks
mattpcaswell pushed a commit to mattpcaswell/qmk_firmware that referenced this issue Jun 7, 2023
Currently, when you enable the wizard, it brings up a single video. While this is useful, I think it might be useful for users to see that there's a playlist of other related tutorials as well, so I have edited the embed link to include the playlist.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants