Make USBHIDKeyboard work at boot #6964

RefactorFactory · 2022-07-10T23:00:33Z

Add a boot_protocol parameter to the USBHIDKeyboard constructor. When true:

The USB interface descriptor will have an interface subclass of boot and
and interface protocol of keyboard. This will cause some PC BIOS to send a
a SET PROTOCOL BOOT request to the device.
The USB report descriptor will not have a report ID because boot protocol
does not use report IDs.
When the host sends reports to the device, USBHIDKeyboard will accept a
report ID of 0 instead of checking for HID_REPORT_ID_KEYBOARD(1).
When the device sends reports to the host, it will not send a report ID
because boot protocol does not use report IDs.
Use endpoint address of 1 for input and output because some PC BIOS require
it.

By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes

Checklist

Please provide specific title of the PR describing the change, including the component name (eg. „Update of Documentation link on Readme.md“)
Please provide related links (eg. Issue which will be closed by this Pull Request)
Please update relevant Documentation if applicable
Please check Contributing guide

This entire section above can be deleted if all items are checked.

Description of Change

USBHIDKeyboard didn't work at boot on my devices, so I've prototyped changes that make it work. I'm looking for feedback.

This adds a parameter to the USBHIDKeyboard constructor and when it is true, behavior is changed. I considered removing the parameter and changing the behavior all the time, but it seemed too risky.

This also adds a new parameter to the USBHID constructor, but it defaults to the old behavior.

I considered only using the new behavior if the host sends the SET PROTOCOL BOOT request, but it turns out that some BIOS do not send that request.

It is strange that some PC BIOS require specific endpoint addresses of 1 for input and output. I tried to do the reservation of the endpoint addresses similar to how USBCDC does it.

Some of the code seems ugly, so I'm looking for feedback.

Tests scenarios

I tested this with Arduino-esp32 core v2.0.4 with an ESP32 S2 board, with one old laptop and one new desktop, both from major PC manufacturers.

Related links

CLAassistant · 2022-07-10T23:00:59Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ SuGlider
❌ RefactorFactory
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

RefactorFactory · 2022-07-12T22:00:11Z

cores/esp32/esp32-hal-tinyusb.c

@@ -661,6 +665,13 @@ esp_err_t tinyusb_enable_interface(tinyusb_interface_t interface, uint16_t descr
        log_e("Interface %s invalid or already enabled", (interface >= USB_INTERFACE_MAX)?"":tinyusb_interface_names[interface]);
        return ESP_FAIL;
    }
+    if(interface == USB_INTERFACE_HID && reserve_endpoints){
+        // Some simple PC BIOS requires specific endpoint addresses for keyboard at boot
+        if(!tinyusb_reserve_out_endpoint(1) ||!tinyusb_reserve_in_endpoint(1)){


Without this reservation of endpoints IN 1, OUT 1, USBCDC will reserve OUT 3, IN 4, IN 5 (line 665), then when USBHID.cpp:tusb_hid_load_descriptor() calls tinyusb_get_free_in_endpoint()/tinyusb_get_free_out_endpoint(), those functions will return IN 3, OUT 4 because those functions prefer returning endpoints whose opposite is already used.

Some alternatives to calling tinyusb_reserve_out_endpoint()/tinyusb_reserve_in_endpoint() here to reserve IN 1, OUT 1:

USBHID.cpp:tusb_hid_load_descriptor() could call tinyusb_get_free_duplex_endpoint() which would likely get endpoints IN 1, OUT 1 because USBHID.cpp:tusb_hid_load_descriptor() runs early enough.

Change tinyusb_get_free_in_endpoint()/tinyusb_get_free_out_endpoint() not to prefer returning endpoints whose opposite is already used. Or add a flag parameter.

RefactorFactory · 2022-07-12T22:12:47Z

Because USBHIDKeyboard is simple, these changes could be greatly simplified by doing some or all of the following:

Remove all the new flags and new parameters.
In the USB interface descriptor, always use an interface subclass of boot protocol.
Never use a report ID in the USB report descriptor or when receiving or sending reports.
Always use endpoints 1 IN, 1 OUT for USBHIDKeyboard.

I'm looking for feedback on whether some of these simplifications should be done.

RefactorFactory · 2022-07-31T21:23:02Z

Any feedback on what I can do to get these changes in? I'm open to suggestions. Thanks.

RefactorFactory · 2022-08-20T04:47:13Z

This has a problem on one of my PCs at boot where the PC does not send "SET PROTOCOL BOOT" to the device and it expects the device to send reports prefixed with a report ID. I'll investigate further.

me-no-dev · 2022-08-24T10:52:07Z

USB Keyboards actually provide two HID devices, one for boot and another for all other cases. The non-boot device can have extra functions, buttons, etc.
I know tinyusb can have two devices, so looking into implementing that instead, seems like the better option here :)

1. Like a real keyboard, the USB interface descriptor will have an interface subclass of boot and an interface protocol of keyboard. This will cause some PC BIOS to send a SET PROTOCOL BOOT request to the device. 2. When the device sends reports to the host, if the host requested boot protocol, don't send a report ID because boot protocol does not use report IDs. 3. To work with some simple PC BIOS: a. Use endpoint address of 1 for input and output. b. Use separate reports for the shift key. These extra reports can be disabled by calling USBHIDKeyboard::setShiftKeyReports(false).

RefactorFactory · 2022-08-30T01:07:07Z

Ok, I've reworked this with some improvements.

I investigated a real keyboard from a major PC manufacturer and found that it makes the main HID device a boot keyboard. The real keyboard that I investigated has another HID device, but it is for special keys. So, like a real keyboard, I made USBHIDKeyboard use a descriptor that makes it a boot device, but it doesn't speak boot protocol (namely, omitting report IDs from reports) unless the host requests it.

This major PC manufacturer of a 2021 PC (which happens to not request boot protocol) needed two other work-arounds:

Use the endpoints 1 IN and 1 OUT. Without this, the device would never become ready at boot. Maybe there is some other deeper problem going on, but this work-around worked.
For reports using the shift key, break out the shift key state changes into separate reports. Without this, shifted characters would not be recognized at all (not even as non-shifted). I found this hard to believe, but this work-around worked. Because this work-around may not be necessary for everyone, I added an API, USBHIDKeyboard::setShiftKeyReports(), that can be called to disable this behavior, but left the default behavior enabled so that users don't have to spend hours rediscovering this problem.

Let me know any feedback, thanks.

RefactorFactory · 2022-09-18T23:47:24Z

Any feedback on what I can do to get these changes in? I'm open to suggestions. Thanks.

RefactorFactory · 2022-10-10T04:10:13Z

Any feedback on what I can do to get these changes in? I'm open to suggestions. Thanks.

me-no-dev · 2022-10-10T06:50:06Z

sorry for the delay @RefactorFactory . We will look into this very soon and have it in what will become version 3.0.0 (based on IDF 5.x)

VojtechBartoska · 2023-11-29T12:24:24Z

Hello @RefactorFactory, please sign CLA. Thanks

P-R-O-C-H-Y

Looks good. Can you please sign the CLA as requested?
Thanks @RefactorFactory for the PR!

lucasssvaz

Works as expected. Tested on my ASRock motherboard (that didn't work without this PR).

RefactorFactory force-pushed the master branch from 0e3c3e4 to 490a075 Compare July 11, 2022 19:55

RefactorFactory commented Jul 12, 2022

View reviewed changes

VojtechBartoska requested review from SuGlider and P-R-O-C-H-Y August 3, 2022 11:43

VojtechBartoska removed the request for review from SuGlider August 10, 2022 12:57

RefactorFactory marked this pull request as draft August 20, 2022 04:48

RefactorFactory force-pushed the master branch from 490a075 to 1201a79 Compare August 30, 2022 00:52

RefactorFactory marked this pull request as ready for review August 30, 2022 01:10

RefactorFactory mentioned this pull request Aug 31, 2022

USBHIDKeyboard SendReport(): report 1 wait failed #7197

Closed

1 task

VojtechBartoska linked an issue Sep 1, 2022 that may be closed by this pull request

USBHIDKeyboard SendReport(): report 1 wait failed #7197

Closed

1 task

SuGlider and others added 2 commits September 5, 2022 18:25

Merge branch 'master' into master

849c62c

Merge branch 'master' into master

a10e950

VojtechBartoska added this to the 3.0.0 milestone Oct 10, 2022

VojtechBartoska added the Status: To be implemented Selected for Development label Oct 10, 2022

This was referenced Nov 7, 2022

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

Closed

investigate endpoint 1 requirement for boot keyboards keyboardio/Kaleidoscope#1306

Open

RefactorFactory added 4 commits November 17, 2022 13:58

Merge branch 'master' into master

0c12eec

Merge branch 'master' into master

b7ac500

Merge branch 'master' into master

6041393

Merge branch 'master' into master

94eebf3

RefactorFactory mentioned this pull request Dec 12, 2022

USBHIDKeyboard print function does not reliably output uppercase characters #7578

Closed

1 task

RefactorFactory added 14 commits December 30, 2022 19:29

Merge branch 'master' into master

5500ccd

Merge branch 'master' into master

d06aa27

Merge branch 'master' into master

2e67a32

Merge branch 'master' into master

9f0bcaf

Merge branch 'master' into master

a30d36c

Merge branch 'master' into master

c28a6a6

Merge branch 'espressif:master' into master

e0d2042

Merge branch 'master' into master

e94a8c9

Merge branch 'master' into master

1896f96

Merge branch 'master' into master

16caa73

Merge branch 'master' into master

ce9049c

Merge branch 'master' into master

e3e612a

Merge branch 'master' into master

d8a61e1

Merge branch 'master' into master

b2643de

VojtechBartoska requested a review from lucasssvaz November 28, 2023 09:22

VojtechBartoska added the Status: Review needed Issue or PR is awaiting review label Nov 28, 2023

VojtechBartoska modified the milestones: 3.0.0, 3.0.0-Aplha3 Nov 28, 2023

Merge branch 'master' into master

6ab100c

Merge branch 'master' into master

aee11ba

P-R-O-C-H-Y approved these changes Nov 30, 2023

View reviewed changes

lucasssvaz approved these changes Nov 30, 2023

View reviewed changes

P-R-O-C-H-Y added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Nov 30, 2023

me-no-dev merged commit 9a9ec09 into espressif:master Nov 30, 2023
44 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make USBHIDKeyboard work at boot #6964

Make USBHIDKeyboard work at boot #6964

RefactorFactory commented Jul 10, 2022

CLAassistant commented Jul 10, 2022 •

edited

RefactorFactory Jul 12, 2022

RefactorFactory commented Jul 12, 2022

RefactorFactory commented Jul 31, 2022

RefactorFactory commented Aug 20, 2022

me-no-dev commented Aug 24, 2022

RefactorFactory commented Aug 30, 2022

RefactorFactory commented Sep 18, 2022

RefactorFactory commented Oct 10, 2022

me-no-dev commented Oct 10, 2022

VojtechBartoska commented Nov 29, 2023

P-R-O-C-H-Y left a comment

lucasssvaz left a comment

Make USBHIDKeyboard work at boot #6964

Make USBHIDKeyboard work at boot #6964

Conversation

RefactorFactory commented Jul 10, 2022

Checklist

Description of Change

Tests scenarios

Related links

CLAassistant commented Jul 10, 2022 • edited

RefactorFactory Jul 12, 2022

Choose a reason for hiding this comment

RefactorFactory commented Jul 12, 2022

RefactorFactory commented Jul 31, 2022

RefactorFactory commented Aug 20, 2022

me-no-dev commented Aug 24, 2022

RefactorFactory commented Aug 30, 2022

RefactorFactory commented Sep 18, 2022

RefactorFactory commented Oct 10, 2022

me-no-dev commented Oct 10, 2022

VojtechBartoska commented Nov 29, 2023

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

lucasssvaz left a comment

Choose a reason for hiding this comment

CLAassistant commented Jul 10, 2022 •

edited