tud_hid_set_protocol_cb() is not called when SET_PROTOCOL is sent

[dhalbert]

Operating System

Others

Board

Adafruit MacroPad RP2040

Firmware

circuitpython (post 7.0.0, PR under development) using TinyUSB commit 43aac70

I'm testing this in the BIOS screen on a Dell Latitude E4310, which requires a boot keyboard,, and sends a SET_PROTOCOL. Interestingly, some later Dell BIOS'es no longer require a boot keyboard. I also see this problem on an older MSI motherboard.

What happened ?

I'm implementing boot protocol support in CircuitPython. I set up an HID interface descriptor specifying boot protocol, using the keyboard (device 1), and a keyboard HID report descriptor with no report ID (for failover).

CircuitPython never sees a call to its tud_hid_set_protocol_cb(), to confirm that the host wants to switch to boot protocol. I instrumented the calling routine.

I also instrumented hidd_control_xfer_cb(), which is supposed to call tud_hid_set_protocol_cb(). This routine is never entered, which seems odd. I do see hidd_control_xfer_cb() entered when the same setup is just plugged into Linux, when that the host doesn't ask for boot protocol. So I think my instrumentation is OK.

(The instrumentation consists of setting a float global to various values so I can see which pieces of code were entered. print the value out at various places in my CircuitPython test program, and can see the printouts on the MacroPad display. I. I am hijacking microcontroller.cpu.temperature to get the value: it returns the global instead of the temperature.)

How to reproduce ?

The setup of this is complicated, but I can help you get it set up if necessary. I think this should be testable with your examples/hid_boot_interface. Right now that example does nothing in the callback, but it could be changed to do something (or you could set a breakpoint there).

The trick is to find an old-enough machine whose BIOS wants a boot keyboard.

Debug Log

Here is a Beagle .tdc trace of the USB traffic. I wait five seconds at the beginning of the program to allow plenty of
time for enumeration:
macropad1.tdc.zip

For comparison, here is a conventional Dell keyboard responding to the SET_PROTOCOL request:
dell1.tdc.zip

Screenshots

Macropad: I think the "Set Configuration" is the start of the SET_PROTOCOL transaction, and the 21 0B is the actual SET_PROTOCOL message. Note the repeated Control Transfer stalls after it.

In comparison, here's the Dell example, which seems to work smoothly:

[hathach]

thanks @dhalbert for a detail issue, I will do my own test. meanwhile can you provide following detail, to see if I could spot any isue in the configuration.

1. the branch/code for cpy you are working on along with code.py for reproducing purpose.
2. your configuration descriptor via command lsusb -v -d 239a:
3. hid descriptor via command sudo usbhid-dump -d 239a | grep -v : | xxd -r -p | hidrd-convert -o spec

[dhalbert]

Here's a patch file of the instrumentation of src/class/hid/hid_device.c. I just add an extern float indicator, and then set it to various values at various points in the code:
instrumentation.hid_device.c.patch.txt

CircuitPython changes are here:
https://github.com/dhalbert/circuitpython/tree/hid-boot-protocol. There are two commits on this branch.

boot.py and code.py (renamed for uploading):
boot.py.txt
code.py.txt
code.py implements just two keys: up arrow and down arrow, which is sufficient for moving around in the BIOS screen.

lsusb -v -d 239a:
lsusb.txt

hidrd-convert is not easily available for Ubuntu 20.04 (it was in 18.04), but here is the descriptor:

001:019:003:DESCRIPTOR         1633610483.029616
 05 01 09 06 A1 01 05 07 19 E0 29 E7 15 00 25 01
 75 01 95 08 81 02 95 01 75 08 81 01 95 03 75 01
 05 08 19 01 29 05 91 02 95 01 75 05 91 01 95 06
 75 08 15 00 26 FF 00 05 07 19 00 2A FF 00 81 00
 C0

passed through http://eleccelerator.com/usbdescreqparser/. This should be the same as what is in boot.py.

0x05, 0x01,        // Usage Page (Generic Desktop Ctrls)
0x09, 0x06,        // Usage (Keyboard)
0xA1, 0x01,        // Collection (Application)
0x05, 0x07,        //   Usage Page (Kbrd/Keypad)
0x19, 0xE0,        //   Usage Minimum (0xE0)
0x29, 0xE7,        //   Usage Maximum (0xE7)
0x15, 0x00,        //   Logical Minimum (0)
0x25, 0x01,        //   Logical Maximum (1)
0x75, 0x01,        //   Report Size (1)
0x95, 0x08,        //   Report Count (8)
0x81, 0x02,        //   Input (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position)
0x95, 0x01,        //   Report Count (1)
0x75, 0x08,        //   Report Size (8)
0x81, 0x01,        //   Input (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
0x95, 0x03,        //   Report Count (3)
0x75, 0x01,        //   Report Size (1)
0x05, 0x08,        //   Usage Page (LEDs)
0x19, 0x01,        //   Usage Minimum (Num Lock)
0x29, 0x05,        //   Usage Maximum (Kana)
0x91, 0x02,        //   Output (Data,Var,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
0x95, 0x01,        //   Report Count (1)
0x75, 0x05,        //   Report Size (5)
0x91, 0x01,        //   Output (Const,Array,Abs,No Wrap,Linear,Preferred State,No Null Position,Non-volatile)
0x95, 0x06,        //   Report Count (6)
0x75, 0x08,        //   Report Size (8)
0x15, 0x00,        //   Logical Minimum (0)
0x26, 0xFF, 0x00,  //   Logical Maximum (255)
0x05, 0x07,        //   Usage Page (Kbrd/Keypad)
0x19, 0x00,        //   Usage Minimum (0x00)
0x2A, 0xFF, 0x00,  //   Usage Maximum (0xFF)
0x81, 0x00,        //   Input (Data,Array,Abs,No Wrap,Linear,Preferred State,No Null Position)
0xC0,              // End Collection

[dhalbert]

What I'm seeing with the hid_device.c instrumentation:

On a regular host which does not try to enable boot protocol, I see 77.0 in the output prints. This indicates it did get into hidd_control_xfer_cb().

On the BIOS host, I only see 0.1. This is the static initial value of indicator, and indicates that it never got into hidd_control_xfer_cb().

[dhalbert]

To confirm this is not port-specific, I tried the same test (with a slightly different code.py) on a SAMD51 PyGamer, and saw the same issue. I see 0.1 printed as the only indicator value, showing that hidd_control_xfer_cb() was not entered.

Note that because the keyboard report descriptor in use has no report id and is very standard, it is possible to send keyboard reports, and the BIOS will respond. But the callback is not happening.

[hathach]

@dhalbert I have just tested with one my simple hid_boot_interface here with 1 boot keyboard + 1 boot mouse. Everything works just fine. The issue with your case is the host driver does not follow the specs, the HID is at bInterfaceNumber=3 however, wIndex in its SET_PROTOCOL does not correct set this value. TinyUSB interpreter this as invalid request toward CDC and stall in response.

You could try to test the protocol set/get by using the USBCV.

Apparently this is a host driver bug, It probably assume that it will only work with simple/single interface keyboard. though we could try to add some hack/walkaround for this if this is popular/common enough in bios world. If it is only a few machine, then we could safely ignore this, since it is its fault after all.

[dhalbert]

Thanks, I was afraid of something like that. I think I could make sure that the HID interface was always interface 0. I will check into that. Older BIOS's are probably all too simple. I saw this with two different manufacturers' motherboards.

[hathach]

@dhalbert that could cause issue with windows machine that already mounted our board before. I am not sure if windows change their way, but last time I tested: Windows stored binding drivers for interfaces using vid_pid (and probably bcd version), to skip re-analyze the configuration afterwards.

E.g once mounted, windows take notes that this metro board has interface 0 is cdc, next time, it will auto load the cdc driver (usbser) for interface0 without re-analyze the configuration descriptor. Depending on the incompatibility of mismatched drivers, in my last experience, it cause my machine instant crash (bluescreen) whenever the device is plugged in.

PS: of course, none of above may or may not an issue anymore (I tested with windows 7 previously). but you should check it out in case you want to do it.

[hathach]

@dhalbert Changing the bcd device number when changing driver combo is a good way to keep the same vid/pid. Though you still need to do a further research. Since my info could be obsolete.

Anyway, I think you should just go ahead with it, just to try to see we could quickly get the boot interface working first. Then we could decide how we walkaround this later on.

[dhalbert]

Currently, with dynamic USB, the interface numbers are assigned sequentially, depending on which devices are enabled (MIDI on/off, MSC on off/ ,etc.). So they already vary. I have never seen a blue screen! But sometimes I have to use Uwe Seiber's device cleanup tool to prevent Windows from getting confused or reporting an issue with the device.

[hathach]

Apparently, Microsoft fixed their hardfault with better error handling. Then I think it is safe to go ahead to re-arrange the interface :)

[dhalbert]

RIght now our Windows 7 and 8.x drivers require that CDC is at interface 0 (adafruit/Adafruit_Windows_Drivers#5). They have entries like

"%CIRCUIT_PLAYGROUND_EXPRESS% UF2 Bootloader (0018:00)"=DriverInstall, USB\VID_239A&PID_0018&MI_00

Note the MI_00. However, I think I might be able to imitate what Windows 10 does to select a class driver for usbser.sys: https://docs.microsoft.com/en-us/windows-hardware/drivers/usbcon/usb-driver-installation-based-on-compatible-ids#windows10, which uses

USB\Class_02&SubClass_02

If I can do this in Windows 7, then that will actually considerably simplify the Windows 7/8.x drivers .inf file, and I will stop needing to update it for each new board.

[hathach]

• If you want to load Usbser.sys automatically, set the class code to 02 and subclass code to 02 in the Device Descriptor. For more information, see USB communications device class. With this approach, you are not required to distribute INF files for your device because the system uses Usbser.inf.

@dhalbert it requires the CDC class & subclass code to be set in the Device Descriptor, just make sure you test with configuration without CDC (the device descriptor should unset the class code). However, this does not seem to work with win8 and prior

[dhalbert]

@hathach What I will test is to install a usbser .inf file that uses class and subclass to specify when to load the driver, instead of the current .inf which identifies the device by VID, PID, and interface. So you'll still need to install the driver package, but I hope to stop having to list all the boards in it indvidually. I also should be able to stop having to specify the interface (MI). adafruit/Adafruit_Windows_Drivers#34. I got the usbser.inf from Windows 10 and will merge it with our own .inf.

[hathach]

@dhalbert yeah, totally understand your point, according to MS doc above

• We need to set the class and subclass in device descriptor to CDC for this to work
• it won't work with win8 and win7

[dhalbert]

I tried a device id like:

"USB Serial"=DriverInstall, USB\Class_02&SubClass_02

This does not work because a third-party driver cannot be a class driver: it cannot recognize a device solely by its class (its "Compatible ID"). The .cat file creator complains and throws an error on the above. Instead, a device must be recognized by its complete hardware ID, e.g. USB\VID_0239a&PID_1234. It's not possible to wildcard the PID either.

So I am going to give up on this for now. We cannot get old BIOSes to work without an Interface number of 0, and we are already using Interface 0 to designate the CircuitPython CDC in the Windows driver package. We cannot change that easily, for legacy support reasons.

If we stopped supporting Windows 7 and 8.1 completely, we could start messing around with changing the Interface numbers.

It appears that at least some new BIOSes don't need boot keyboards anyway, so it's not terrible to give up on this.

[hathach]

@dhalbert I could add an extra callback for unsupported requests for cdc, which circupython can use for this scenario. However, it does add a bit of confusion in normal (non-bios) usage. Let me know if you think that worth the hassle to support it