Demo of boot protocol working #50

gedankenexperimenter · 2019-03-14T22:29:55Z

I'm submitting this as a PR, rather than an issue, because I want to discuss the code very specifically, and this is a much simpler way to present it, and make it easy for others to test it on systems that I don't have access to. I'm creating it as a draft PR, and do not intend for it to be merged, as it contains debugging code for demonstration purposes. Please forgive me if I get some of the terminology wrong; I'm still not clear on much of it.

Background

I've been working on my own version of the keyboard HID module, based on KeyboardioHID, and had NKRO "report" protocol working just fine. I wanted to add 6KRO "boot" protocol support to it, and ran into some trouble getting it to work. It seemed like the problem was that my primary machine (an iMac running Mojave) was rejecting attempts to send boot protocol reports when the keyboard was advertising a report protocol HID endpoint to the host. So I started testing out Kaleidoscope itself, and making very small changes to KeyboardioHID, I stumbled on something that seems to work.

The "Solution"?

The change that I made seems like it shouldn't work. I had configured a Macro key to switch the keyboard from report protocol to boot protocol by using the USBQuirks plugin, but that didn't seem to work on either my iMac or my (rather old) Linux (Ubuntu) machine. I would switch modes, and it either wouldn't work at all (no output on the host, with quarter-second delays every time a 6KRO report was attempted), or it would switch itself back into NKRO mode. Frustrated by the latter problem, I added a new variable (BootKeyboard.boot_protocol_), and used that as the test condition instead of protocol, and suddenly everything worked.

The reason it seems like it shouldn't work is that I didn't remove the protocol variable, so when PluggableUSB (presumably) calls BootKeyboard.setup(), it seems like the host should still expect the keyboard to be using NKRO reports. I'm stumped as to how, but it works, and I can easily verify that both protocols work fine on the two machines I have access to.

So, I may have stumbled on a way to improve the functioning of Kaleidoscope in this area, but I don't know if these changes break something else…

Possible (Likely?) Problems

This code might not work on all operating systems, of course. I don't have the means to test it on ChromeOS, Windows, or any BSD variant, and the Linux machine I've got is very out-of-date. I'm not at all an expert on USB HID, and I think I've reached the limit of what I can figure out on my own.

gedankenexperimenter · 2019-03-14T22:33:33Z

@algernon – I figured this is a better place for the discussion that we were having than Discord. @obra – I've marked this as a draft PR, and I don't really intend for it to be merged at all. Nevertheless, something useful might come out of it, so I want to call your attention to it regardless.

gedankenexperimenter · 2019-03-14T22:34:58Z

Note: I submitted the changes to intentionally fail the DCO test, just as an extra precaution to stop the changes being merged.

gedankenexperimenter · 2019-03-14T22:45:59Z

src/MultiReport/Keyboard.cpp

+    Serial.print(F(" "));
+    Serial.print(keyReport.allkeys[i], HEX);
+  }
+  Serial.println();


This is all just debug output, making it clear when the keyboard is in report protocol mode.

gedankenexperimenter · 2019-03-14T22:46:48Z

src/BootKeyboard/BootKeyboard.cpp

+      Serial.print(F(" "));
+      Serial.print(_keyReport.bytes[i], HEX);
+    }
+    Serial.println();


Serial debug output showing the details of any report sent in boot protocol mode.

gedankenexperimenter · 2019-03-14T22:47:44Z

src/BootKeyboard/BootKeyboard.cpp

+    boot_protocol_ = true;
+  } else {
+    boot_protocol_ = false;
+  }


Instead of setting the protocol variable here, I leave it alone and set the new boolean boot_protocol_. The protocol variable stays set to HID_REPORT_PROTOCOL, set in the constructor:

BootKeyboard_::BootKeyboard_(void) : PluggableUSBModule(1, 1, epType), protocol(HID_REPORT_PROTOCOL), idle(1), leds(0) {

gedankenexperimenter · 2019-03-14T22:49:01Z

src/BootKeyboard/BootKeyboard.cpp

+    return HID_BOOT_PROTOCOL;
+  } else {
+    return HID_REPORT_PROTOCOL;
+  }


This reports the protocol based on the new boot_protocol_ variable instead of protocol. This is ultimately what controls which type of report is sent, in the HID adaptor code. See this line in Kaleidoscope-HIDAdaptor-KeyboardioHID for details.

gedankenexperimenter · 2019-03-14T22:57:05Z

src/BootKeyboard/BootKeyboard.h

@@ -78,6 +78,7 @@ class BootKeyboard_ : public PluggableUSBModule {

  uint8_t epType[1];
  uint8_t protocol;
+  bool boot_protocol_{false};


true => boot protocol
false => report (NKRO) protocol

This adds a state variable whose only purpose is to track changes requested by Kaleidoscope, and to feed them back to Kaleidoscope via getProtocol. I'm not sure it belongs here. On the other hand, setProtocol is of dubious value as well, because (before these changes) it can force the interface to answer GET_PROTOCOL with a different protocol than the host previously selected, which might cause the host to decide that the interface is misbehaving.

gedankenexperimenter · 2020-11-11T15:37:23Z

I still want to know what the heck is going on with this, but there's no sense keeping this PR open.

algernon · 2022-05-17T00:42:06Z

Even though it's been almost two years, I'd like to reopen this. It fell under my radar a while ago, and then dropped off, but now that it's been mentioned in keyboardio/Kaleidoscope#1168, I finally had a look.

I have a suspicion why this seems to work, and our current hacks don't: the 6KRO and NKRO parts are presented as two different endpoints by Kaleidoscope.

Why does that matter? Because both Linux and macOS try to set the keyboard (the NKRO endpoint) into report mode, and if it doesn't do that, they get upset. It's an NKRO keyboard, it should act as such. Meanwhile, since we're using protocol, BootKeyboard will say it's report protocol, too, while it isn't. So everyone gets mightily confused.

With your change, however, we simply let each end point handle their own protocol, so BootKeyboard will be 6KRO, the other will be NKRO. And you simply choose which end-point to send the report on.

This makes a whole lot of sense in hindsight, but I do need to take it for a test-drive on a modern Linux, Windows, and the BSDs. I'll try to get around to do that in the next couple of days.

obra · 2022-05-17T00:56:58Z

As a note, we may be maxxed out on endpoints already on GD32.

gedankenexperimenter · 2022-05-17T03:40:27Z

Despite having provided this code in the first place, I'm still far from fully understanding the problem and the solution. @algernon — please feel free to take over and either edit this PR (I think that's something you can do on GitHub) or just create a new one to fix the problem properly.

This changes the variable used to decide which type of report to send to the host, without changing the `getDescriptor()` and `setup()` methods, which still do whatever they do with the `protocol` variable, and `UEDATX`. I don't understand why this works, but it seems to allow proper switching between boot protocol and report protocol on macOS & Linux (the two machines I have access to).

This writes to the serial port, demonstrating when the keyboard is actually sending reports using either NKRO or Boot protocol.

I don't understand why what I've done works. This comment is meant to simply highlight the line in the code that seems to be the cause of boot protocol failing without the earlier changes I made (i.e. `boot_protocol_`).

gedankenexperimenter · 2022-05-21T18:45:02Z

I rebased this PR onto master. It's still not something we want merged, but it is hopefully functional (I still haven't had a change to test it).

tlyu

Thank you for your work and experimentation here. I think that we ultimately want to try to fix this at a higher level, in Kaleidoscope. I'll open a new issue there with some of my thoughts and open design questions about how to make the boot keyboard work properly.

@gedankenexperimenter I would be interested to hear more details about what changes you tried previous to this PR, and the failure modes you experienced with them.

tlyu · 2022-11-06T23:23:58Z

src/BootKeyboard/BootKeyboard.cpp

+      // know what. With the change that I've made to use `boot_protocol_` instead, this
+      // would always be set to `HID_REPORT_PROTOCOL`, even when sending boot protocol
+      // reports (successfully). It doesn't seem correct, but it works on macOS and Linux
+      // (or at least, my old Ubuntu machine).
      UEDATX = protocol;


This is an optimization for AVR to queue a single byte for transmission on the control pipe. In this particular case, it is the single byte response to the HID_GET_PROTOCOL request.

tlyu · 2022-11-06T23:27:08Z

src/BootKeyboard/BootKeyboard.h

@@ -78,6 +78,7 @@ class BootKeyboard_ : public PluggableUSBModule {

  uint8_t epType[1];
  uint8_t protocol;
+  bool boot_protocol_{false};


This adds a state variable whose only purpose is to track changes requested by Kaleidoscope, and to feed them back to Kaleidoscope via getProtocol. I'm not sure it belongs here. On the other hand, setProtocol is of dubious value as well, because (before these changes) it can force the interface to answer GET_PROTOCOL with a different protocol than the host previously selected, which might cause the host to decide that the interface is misbehaving.

gedankenexperimenter · 2022-11-07T01:06:21Z

I would be interested to hear more details about what changes you tried previous to this PR, and the failure modes you experienced with them.

Unfortunately, those experiments were conducted so long ago that I can't recall any of the details, other than that I put in debug code to get output over serial to see what reports were actually being sent. I tried changing a bunch of stuff in the code until I stumbled on something semi-random that caused it to actually switch protocols when requested.

I was doing this work so that I could write a version of KeyboardioHID that was simpler, more efficient, and (most important) easier for me to understand while I was writing my own alternative to Kaleidoscope. This was before Kaleidoscope became event-driven, so it was nigh impossible for me to write certain plugins that became almost trivial with the event-driven design. If you want, I can point you at that code, but I suspect that you've got a deep enough understanding of the problem that it wouldn't be of any help.

gedankenexperimenter commented Mar 14, 2019

View reviewed changes

algernon self-assigned this Apr 28, 2020

gedankenexperimenter closed this Nov 11, 2020

gedankenexperimenter mentioned this pull request Feb 7, 2022

Atreus keyboard does't work in UEFI/BIOS keyboardio/Chrysalis#733

Closed

gedankenexperimenter mentioned this pull request May 14, 2022

Non-functioning under coreboot + seabios, model01 keyboardio/Kaleidoscope#1168

Open

algernon reopened this May 17, 2022

gedankenexperimenter added 3 commits May 20, 2022 23:34

Add debugging output for keyboard reports

3606015

This writes to the serial port, demonstrating when the keyboard is actually sending reports using either NKRO or Boot protocol.

Comment the relevant line where it seems things should fail

b8001cd

I don't understand why what I've done works. This comment is meant to simply highlight the line in the code that seems to be the cause of boot protocol failing without the earlier changes I made (i.e. `boot_protocol_`).

gedankenexperimenter force-pushed the boot-report-demo branch from 5cea4fa to b8001cd Compare May 21, 2022 18:42

tlyu suggested changes Nov 6, 2022

View reviewed changes

tlyu mentioned this pull request Nov 7, 2022

high-level design changes for Boot/NKRO switching keyboardio/Kaleidoscope#1305

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Demo of boot protocol working #50

Demo of boot protocol working #50

gedankenexperimenter commented Mar 14, 2019 •

edited

Loading

gedankenexperimenter commented Mar 14, 2019

gedankenexperimenter commented Mar 14, 2019

gedankenexperimenter Mar 14, 2019

gedankenexperimenter Mar 14, 2019

gedankenexperimenter Mar 14, 2019

gedankenexperimenter Mar 14, 2019

gedankenexperimenter Mar 14, 2019 •

edited

Loading

tlyu Nov 6, 2022

gedankenexperimenter commented Nov 11, 2020

algernon commented May 17, 2022

obra commented May 17, 2022

gedankenexperimenter commented May 17, 2022

gedankenexperimenter commented May 21, 2022

tlyu left a comment

tlyu Nov 6, 2022

tlyu Nov 6, 2022

gedankenexperimenter commented Nov 7, 2022

Demo of boot protocol working #50

Are you sure you want to change the base?

Demo of boot protocol working #50

Conversation

gedankenexperimenter commented Mar 14, 2019 • edited Loading

Background

The "Solution"?

Possible (Likely?) Problems

gedankenexperimenter commented Mar 14, 2019

gedankenexperimenter commented Mar 14, 2019

gedankenexperimenter Mar 14, 2019

Choose a reason for hiding this comment

gedankenexperimenter Mar 14, 2019

Choose a reason for hiding this comment

gedankenexperimenter Mar 14, 2019

Choose a reason for hiding this comment

gedankenexperimenter Mar 14, 2019

Choose a reason for hiding this comment

gedankenexperimenter Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

tlyu Nov 6, 2022

Choose a reason for hiding this comment

gedankenexperimenter commented Nov 11, 2020

algernon commented May 17, 2022

obra commented May 17, 2022

gedankenexperimenter commented May 17, 2022

gedankenexperimenter commented May 21, 2022

tlyu left a comment

Choose a reason for hiding this comment

tlyu Nov 6, 2022

Choose a reason for hiding this comment

tlyu Nov 6, 2022

Choose a reason for hiding this comment

gedankenexperimenter commented Nov 7, 2022

gedankenexperimenter commented Mar 14, 2019 •

edited

Loading

gedankenexperimenter Mar 14, 2019 •

edited

Loading