Added a 10ms delay to work around a problem with Windows Remote Desktop

[gedankenexperimenter]

This change addresses a problem that has been observed with Windows Remote Desktop. It's basically the same issue as the ChromeOS bug, but a bit worse, because two HID reports that are sent to the local host from the Keyboard with a very small interval seem to be collapsed into a single report sent to the remote host, and the modifier keycodes appear to be processed last, so they don't get applied until after the unmodified keycode(s) in the report.

This report introduces a delay of 10ms if a report will be sent that changes both the modifiers byte and some other keycode in the keyboard HID report. If so, it will send the modifier in one report, delay for 10ms, then send the full report. Several different delay intervals were tested by a user on the community forum, who reported that a delay of 10ms was sufficient, but a delay of 5ms would occasionally result in an unintended unmodified character.

I don't like using delay() much, but I've done what I can to keep it from happening spuriously, and this is the best solution to the problem that I've come up with. On the other hand, users of Windows Remote Desktop are probably not a high percentage of total users, and there are other ways for them to get around this problem, though right now, those other techniques are likely to introduce other problems for them (scan order bugs with repeated characters).

[algernon]

Seeing as @obra does not like the 10ms delay there (understandably), would it make sense to either tie this to a compiler flag, or an in-code flag, to toggle the delay on?

I was thinking KEYBOARDIOHID_ENABLE_WINDOWS_REMOTE_DESKTOP_WORKAROUND (definable via the Arduino IDE, or LOCAL_CFLAGS) if using a compiler flag, or bool KeyboardioHID.WRDWorkaround if using a member variable. Either of these would result in the delay being off by default, but still available for the few users who need it.

Myself, I'd prefer the first, because then we also save a comparison each cycle...

[michabu]

Would it be a reasonable alternative to change the architecture to queue keyboard-reports and normally send them out as fast as possible but give the option to mark one report to be delayed?

I imagine that it would be quite a change to the architecture. But it would allow the cycle to continue and not stop all operations for a certain amount of time but would still allow to deal with problems like the windows remote desktop problem.

I could imagine that there might be other uses like investigating the reports to be send and altering them or being able to build quite a complex macro which creates a few reports and then allows the keyboard to continue the scan loop without having to wait till all the reports are actually sent.

[gedankenexperimenter]

I'm in favor of a compiler flag to turn the delay on or off, unless someone can think of a use case where this delay actually causes a problem. That could make it necessary for a user to switch the delay on and off depending on need.

[obra]

A compiler flag would be ok with me. It'd be sort of nice if we had arbitrary plugin hook points and could make this functionality just a plugin. I don't need us to wait for that possible future before getting an option in there for folks who need it. It -would- be nice of we could get the user to test some shorter delays (say 1, 3 and 5 ms) before we commit to 10.

…

On Thu, Mar 8, 2018 at 11:55 AM, Michael Richters ***@***.***> wrote: I'm in favor of a compiler flag to turn the delay on or off, unless someone can think of a use case where this delay actually causes a problem. That could make it necessary for a user to switch the delay on and off depending on need. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACxaAJM3Pm4rUhwk0h2XGL1f4e2BJ6Cks5tcYzGgaJpZM4SiMA3> .

[gedankenexperimenter]

He tested 5ms, and reported occasional extra characters. He reported that 10ms was enough. I don't think he tried intervals between 5 and 10.

[obra]

*nod* This would be so much "easier" in a multi-threaded world.

…

On Thu, Mar 8, 2018 at 4:47 PM, Michael Richters ***@***.***> wrote: He tested 5ms, and reported occasional extra characters. He reported that 10ms was enough. I don't think he tried intervals between 5 and 10. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACxaGxngLKzQ_l7dPdTzc1cSfRk2kfzks5tcdE4gaJpZM4SiMA3> .

[gedankenexperimenter]

shudder

Yeah… "Easier". Uh-huh. =)

[gedankenexperimenter]

I added the suggested compiler flag. I don't know if #ifdef is preferable to #if defined().

[michabu]

It -would- be nice of we could get the user to test some shorter delays (say 1, 3 and 5 ms) before we commit to 10.

Since I am that user allow me to chime in: I did test with various delays. With delays 5ms and shorter the problem did occur frequently. With 10ms I did see it occur once since I did apply the patch. I didn't test with intervals between 5ms and 10ms.

I did carry out all tests over the same internet connection which is a good on (at least for German standards). I could imagine that a slower internet connection could have an effect on how the generated scancodes are being transmitted to the server but I didn't have an opportunity to test that yet.

[gedankenexperimenter]

How about making the delay configurable in the user's sketch? That should be fairly straightforward.

[obra]

I'd be ok with having a zero delay default and code that would use a configurable delay from the user's sketch. I'll probably want to lawyer the naming to death before we merge, but that's nothing new. Thanks for pushing on this.

…

On Fri, Mar 9, 2018 at 5:07 AM, Michael Richters ***@***.***> wrote: How about making the delay configurable in the user's sketch? That should be fairly straightforward. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACxaOlnNLQOIOL1RraBDxIHRAkL6eWbks5tcn6UgaJpZM4SiMA3> .

[gedankenexperimenter]

How about making it just a single preprocessor macro, optionally defined in the sketch file, containing the number of milliseconds to delay? Call it whatever you like — I'll suggest KEYBOARDIOHID_MODIFIER_FLAG_DELAY.

[algernon]

I like that idea.

[gedankenexperimenter]

I started to edit the code, and realized that my idea won't work, because nothing in the sketch directory gets included in the compilation of Keyboard.cpp. I have two alternate ideas:

1. Use a compiler flag by editing boards.txt and adding KEYBOARDIOHID_MODIFIER_FLAG_DELAY to model01.build.extra_flags.

2. Define a weak-symbol function in Keyboard.cpp that's empty, and let the user define that same function with the delay in the sketch file.

I'm ambivalent about both of these options. The first is probably better, especially if there's someplace else that the compiler flag can be added in (I still haven't really figured out the build system).

[gedankenexperimenter]

If I added this line to ~/.kaleidoscope-builder.conf, would that work?

KEYBOARDIOHID_MODIFIER_FLAG_DELAY=10

[obra]

The only problem is that this won’t work when building with the GUI

…

On Mar 10, 2018, at 7:02 PM, Michael Richters ***@***.***> wrote: If I added this line to ~/.kaleidoscope-builder.conf, would that work? KEYBOARDIOHID_MODIFIER_FLAG_DELAY=10 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[gedankenexperimenter]

Shouldn't this be settable by the same methods as the build options in Kaleidoscope-HIDAdaptor-KeyboardioHID?

There are a number of features one can disable via a compiler flag (set either via the Arduino IDE, or by using the LOCAL_CFLAGS setting of kaleidoscope-builder), to reduce the size of the final firmware, if certain features aren't needed.

I don't actually know the proper way to do either of those things, so maybe it's only possible to use those compiler options as a boolean, whereas here we want to set an integer value.

@algernon, how does one set those compiler flags in the Arduino IDE? And how does one use LOCAL_CFLAGS? For that matter, maybe the README for Kaleidoscope-HIDAdaptor-KeyboardioHID should explain how, or provide a link to other documentation, rather than just saying that it's possible, and leaving the reader to his own devices to figure it out.

[algernon]

how does one set those compiler flags in the Arduino IDE?

Ugh. This is embarassing. It turns out I was using an unreleased version of Arduino that did let me set custom CFLAGS (via preferences). But that feature is not in 1.8.5, nor 1.9.0-beta. :/

Not sure what to do now... :(

And how does one use LOCAL_CFLAGS?

Add it to .kaleidoscope-builder.conf:

LOCAL_CFLAGS="-DFOO_BAR_BAZ"

[gedankenexperimenter]

Well, it wouldn't really be any different than the compile options in Kaleidoscope-HIDAdaptor-KeyboardioHID, would it? Maybe it would be a good idea to have Kaleidoscope.h include a file that would be found in the user's sketch directory (e.g. kaleidoscope-build-options.h), which would work for both IDE and CLI builds.

[algernon]

Maybe it would be a good idea to have Kaleidoscope.h include a file that would be found in the user's sketch directory (e.g. kaleidoscope-build-options.h), which would work for both IDE and CLI builds.

This would help, if the firmware would compile as a single unit, but each plugin is compiled as its own. So stuff defined in the user's sketch do not have any effect on the plugins, because when they are compiled, the user sketch is not consulted.

The only way we could accomplish this is to mandate such a file... not sure if that's a good idea.

On the other hand, some low-level things being tweakable only outside of the IDE is perhaps not terribly evil... as long as the defaults work for the vast majority of cases.

[gedankenexperimenter]

If the order of #include directories is consistent, and has the sketch first, there could be an empty kaleidoscope-build-options.h file in each module that needs it, allowing customizations in the sketch to be incorporated into the other modules, but not breaking the build if it's absent from the sketch directory.

[algernon]

That doesn't sound wrong. I quite like it, too. Will experiment with it, thank you!

[gedankenexperimenter]

I can't claim credit for the idea; I came across a version of it when searching StackOverflow for "preprocessor if file exists" (or something of the kind). Upon further consideration, it probably makes more sense to have the empty header just in Kaleidoscope, rather than multiple places, as long as it's possible to guarantee that the sketch dir's copy always gets found first.

[algernon]

Yeah, I'm a bit worried about that. The sketch's copy may not be on the include path when a plugin is compiled, rendering the trick useless :/

OTOH, we may be able to add the sketch dir to the include path via boards.txt, so that may save us.

[obra]

As I'm triaging open PRs, I've come back to this. Is there any extant public documentation on what Windows Remote Desktop is doing or what its real requirements are?

[gedankenexperimenter]

Alas, I have nothing but speculation, no documentation.

[obra]

Understood. I'd love for us to have a nice way to let people inject workarounds fo these kinds of quirks. ᐧ

…

On Sat, Aug 11, 2018 at 6:33 PM Michael Richters ***@***.***> wrote: Alas, I have nothing but speculation, no documentation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACxaPxFjnn-v3I2_oPnRKJkrHYJftkCks5uP4X2gaJpZM4SiMA3> .

[gedankenexperimenter]

Instead of a fixed delay, this should probably use a timeout interval, so the next report only gets delayed if the last one was less than x milliseconds ago. It might be better to do this check in Kaleidoscope instead. I'll probably work on this in a few weeks.

[DavidVentura]

So I tried this change today (because of keyboardio/Model01-Firmware#87) and it kind-of solves the problem - so long as the set latency is greater than the RDP latency!
By applying this with 200, 10 and 0 ms in delay I got varying amounts of wrong keys

+++ b/src/MultiReport/Keyboard.cpp
@@ -132,16 +132,17 @@ int Keyboard_::sendReport(void) {
         // will sometimes result in a spurious '3' rather than '#', especially when the keys
         // had been held for a while
         else if (( (lastKeyReport.modifiers ^ keyReport.modifiers) & lastKeyReport.modifiers)
                  && (memcmp(lastKeyReport.keys,keyReport.keys, sizeof(keyReport.keys)))) {
             uint8_t mods = keyReport.modifiers;
             keyReport.modifiers = lastKeyReport.modifiers;
             int returnCode = HID().SendReport(HID_REPORTID_NKRO_KEYBOARD, &keyReport, sizeof(lastKeyReport));
             keyReport.modifiers = mods;
+            delay(200);
         }

First row is 200ms, second is 10ms and third is 0ms.
The amount of mis-typed keys are 3, 19 and 25 respectively.

This is in a purposefully bad setup -- RDP that has ~70ms latency. I could not reproduce the issue on a <1ms setting.

[gedankenexperimenter]

On a connection with that much latency, can you reproduce the problem with an ordinary keyboard, by nearly simultaneous pressing of shift and a printable key? If not, maybe there's something better we can do.

[DavidVentura]

If I type at normal speeds something like a | b | c | d | e | f apparently the time I hold down shift is enought for it to not send a backslash. The normal time hovers around ~150-250ms.

$ xev | ts "%.s" | grep --line-buffered -iB 2  shift  |  grep  --line-buffered Key | awk '{ if (NR % 2 == 1) {val=$1} else {print $1-val} }'
0.223535
0.151941
0.23246
0.248429
0.192018

I tried
keydown shift backslash -- keyup shift backslash (both at the same time) and got lots of back-slashes

Measuring these keypresses, the shift key is pressed between 50 and 80 ms.

Then tried keydown shift keydown backslash keyup backslash keyup shift (hold shift, tap backslash, release shift) as fast as possible and got fewer back-slashes

Measuring these keypresses, the shift key is pressed between 75 and 100 ms.

Later I tried to reproduce with xdotool but I could never get it to make a backslash..

[DavidVentura]

Note: Measuring the delay when pressing | or {} in keyboardio I get 50-108ms (I've applied this patch with 10ms setting). I will remove the patch in a bit and try it out

[DavidVentura]

I'm back at work -- where I use the keyboard via RDP (sad). With a 10ms delay it works perfectly!

[gedankenexperimenter]

Since this whole thing is clearly a flaw in Windows Remote Desktop, and nothing has happened for more than a year, is it time to close this?

[obra]

I'm happy to close out this PR. The related issue should likely stay open, as it has an explanation of the in-app mitigation and the fact that the next big win10 release should have a proper fix. (As confirmed to me by the MS Engineer who committed the fix.)

…

On Wed, Sep 2, 2020 at 5:20 PM Michael Richters ***@***.***> wrote: Since this whole thing is clearly a flaw in Windows Remote Desktop, and nothing has happened for more than a year, is it time to close this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#30 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALC2CPM4OHPX5SF3G2U6TSD3OLVANCNFSM4EUIYA3Q> .