Return USB HID GET_REPORT requests #14814

jackhumbert · 2021-10-13T22:53:24Z

Description

This adds the responses to GET_REPORT that the USB HID spec requires. Some of this got removed in #3951, but it's necessary to return the GET_REPORT requests for the keyboard to work in a timely fashion - otherwise it can take up to 20s to start working on Linux (specifically Debian on arm). Other features that have their own interfaces may require the same sort of treatment. My understanding of this is the host queries these interfaces as soon as it gets the USB configuration and descriptors, so if it's in there, it needs to be handled.

SET_REPORT needs some similar handling for NKRO, but it's optional there, whereas GET_REPORT is required.

Wireshark may be useful in figuring out which systems query which interfaces/reports - that's how I could tell they weren't being responded to.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project: C, Python
I have read the PR Checklist document and have made the appropriate changes.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

drashna · 2021-10-14T00:08:46Z

case KEYBOARD_INTERFACE: has to be hidden behind preprocessors, as it doesn't exist if there is a shared endpoint

sigprof · 2021-10-14T05:54:21Z

I wonder whether this could be done in a more generic way somehow — we have some more optional HID interfaces (RAW_INTERFACE, JOYSTICK_INTERFACE, DIGITIZER_INTERFACE) which are not handled by the code in the PR, but technically the HID protocol spec says that all of them must support GET_REPORT.

tmk_core/protocol/chibios/usb_main.c

sigprof · 2021-10-14T07:14:02Z

tmk_core/protocol/chibios/usb_main.c

                            default:
                                usbSetupTransfer(usbp, NULL, 0, NULL);
                                return TRUE;


I'm not sure whether replying with zero-length data is a proper way to handle an HID_GET_REPORT with some unexpected parameters. Looks like the underlying ChibiOS code might not be handling this case properly: https://github.com/qmk/ChibiOS/blob/413e39c5681d181720440f2a8b7391f581788d7b/os/hal/src/hal_usb.c#L828-L849 (it will go directly to USB_EP0_OUT_WAITING_STS and wait for the OUT packet from the host, but actually the host will send IN packets, expecting the requested data). So a better solution might be return FALSE, which will stall the endpoint and fail the control transfer. This needs some actual testing on various host OS versions though.

I think this is the core of the problem here - returning false like this does address the issue as well, so that is an option. Do you know if usbSetupTransfer(usbp, NULL, 0, NULL); is appropriate in the other contexts it's used in here?

Co-authored-by: Sergey Vlasov <sigprof@gmail.com>

stale · 2022-01-03T21:45:33Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

sigprof

Replying with at least an empty report will probably solve the timeout issues that some people see (although a 100% correct solution would probably be to actually store the last sent report of each type, I'm not sure whether any real HID host actually uses the GET_REPORT request outside of the device initialization). The only remaining issue is handling of some definitely wrong requests (someone may claim that it's a security issue, because technically it could allow someone to read a limited amount of RAM contents that happened to be located after universal_report_blank).

sigprof · 2022-02-04T19:00:25Z

tmk_core/protocol/chibios/usb_main.c

                            default:
-                                usbSetupTransfer(usbp, NULL, 0, NULL);
+                                universal_report_blank.report_id = usbp->setup[2];
+                                usbSetupTransfer(usbp, (uint8_t *)&universal_report_blank, usbp->setup[6], NULL);


Should we guard against an attempt to read the MCU RAM beyond sizeof(universal_report_blank) here? Either clamp the size, or just return FALSE on a bad value (probably should not care much about the particular kind of error if the host definitely tries to do something wrong).

nblyumberg · 2022-02-11T16:29:16Z

I had flashed my Preonic v3 with a test build that @tzarc provided and that solved the problem of going into the Preonic becoming non responsive after using layers. I am using the latest available version of Mint Linux (20.3 "Una") where when using a layer key will almost always make the keyboard non-responsive until it's unplugged and reconnected.

stale · 2022-04-16T07:33:30Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

tzarc

Let's get this in and iron out the details over the rest of the develop cycle.

Co-authored-by: Sergey Vlasov <sigprof@gmail.com> Co-authored-by: Nick Brassel <nick@tzarc.org>

The keyboard can still detect NKRO but the HID report limits that to 6KRO. Causes slow start up on Linux. Probably be related to: - qmk/qmk_firmware#8733 - qmk/qmk_firmware#14814 - qmk/qmk_firmware#19372 Signed-off-by: Daniel Schaefer <dhs@frame.work>

add back some get_report code for responsiveness

d6cc57f

github-actions bot added the core label Oct 13, 2021

jackhumbert mentioned this pull request Oct 13, 2021

[Bug] Faulty inputs with recent windows 10 update #13892

Closed

3 tasks

drashna requested review from fauxpark and a team October 14, 2021 00:03

add more cases for KEYBOARD_SHARED_EP

41ccca9

drashna approved these changes Oct 14, 2021

View reviewed changes

drashna requested a review from a team October 14, 2021 00:27

lint ppd

eb4793b

This was referenced Oct 14, 2021

Continuous Linux kernel message "Cannot enable. Maybe the USB cable is bad?" #6713

Closed

[Bug] Infinity60/Whitefox slow startup time in Linux #8733

Open

add mouse to shared interface response

9917211

sigprof requested changes Oct 14, 2021

View reviewed changes

jackhumbert and others added 3 commits October 14, 2021 09:41

use the console's ep size ref

e52eabe

Co-authored-by: Sergey Vlasov <sigprof@gmail.com>

use keyboard report size ref

56ec285

Co-authored-by: Sergey Vlasov <sigprof@gmail.com>

unify blank reports into one case

4a62ff5

tzarc requested a review from sigprof November 1, 2021 22:36

tzarc added breaking_change_2021q4 bug and removed breaking_change_2021q4 labels Nov 1, 2021

tzarc requested a review from a team November 1, 2021 22:36

stale bot added the awaiting changes label Jan 3, 2022

drashna approved these changes Jan 7, 2022

View reviewed changes

stale bot removed the awaiting changes label Jan 7, 2022

tzarc changed the base branch from master to develop February 4, 2022 18:39

sigprof reviewed Feb 4, 2022

View reviewed changes

stale bot added the awaiting changes label Apr 16, 2022

drashna requested a review from a team April 19, 2022 02:14

stale bot removed the awaiting changes label Apr 19, 2022

Merge branch 'develop' into fix/get_report

3f13da6

tzarc approved these changes Dec 8, 2022

View reviewed changes

tzarc merged commit a23333e into develop Dec 8, 2022

tzarc deleted the fix/get_report branch December 8, 2022 17:08

sigprof mentioned this pull request Dec 18, 2022

Enable NKRO for Aurora Corne #19372

Closed

14 tasks

sigprof mentioned this pull request Jan 15, 2023

Fix joystick build for ChibiOS #19602

Merged

14 tasks

omikronik pushed a commit to omikronik/qmk_firmware that referenced this pull request Jan 22, 2023

Return USB HID GET_REPORT requests (qmk#14814)

b826f2a

Co-authored-by: Sergey Vlasov <sigprof@gmail.com> Co-authored-by: Nick Brassel <nick@tzarc.org>

JohnAZoidberg mentioned this pull request Mar 23, 2023

Sometimes boots slowly FrameworkComputer/qmk_firmware#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return USB HID GET_REPORT requests #14814

Return USB HID GET_REPORT requests #14814

jackhumbert commented Oct 13, 2021 •

edited

Loading

drashna commented Oct 14, 2021

sigprof commented Oct 14, 2021

sigprof Oct 14, 2021

jackhumbert Oct 14, 2021

stale bot commented Jan 3, 2022

sigprof left a comment

sigprof Feb 4, 2022

nblyumberg commented Feb 11, 2022 •

edited

Loading

stale bot commented Apr 16, 2022

tzarc left a comment

Return USB HID GET_REPORT requests #14814

Return USB HID GET_REPORT requests #14814

Conversation

jackhumbert commented Oct 13, 2021 • edited Loading

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

drashna commented Oct 14, 2021

sigprof commented Oct 14, 2021

sigprof Oct 14, 2021

Choose a reason for hiding this comment

jackhumbert Oct 14, 2021

Choose a reason for hiding this comment

stale bot commented Jan 3, 2022

sigprof left a comment

Choose a reason for hiding this comment

sigprof Feb 4, 2022

Choose a reason for hiding this comment

nblyumberg commented Feb 11, 2022 • edited Loading

stale bot commented Apr 16, 2022

tzarc left a comment

Choose a reason for hiding this comment

jackhumbert commented Oct 13, 2021 •

edited

Loading

nblyumberg commented Feb 11, 2022 •

edited

Loading