Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ChibiOS: Use 'usbTransmit' instead of custom function for 'send_report' #22386

Closed
wants to merge 1 commit into from
Closed

ChibiOS: Use 'usbTransmit' instead of custom function for 'send_report' #22386

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 1 addition & 18 deletions tmk_core/protocol/chibios/usb_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,24 +827,7 @@ uint8_t keyboard_leds(void) {
}

void send_report(uint8_t endpoint, void *report, size_t size) {
osalSysLock();
if (usbGetDriverStateI(&USB_DRIVER) != USB_ACTIVE) {
osalSysUnlock();
return;
}

if (usbGetTransmitStatusI(&USB_DRIVER, endpoint)) {
/* Need to either suspend, or loop and call unlock/lock during
* every iteration - otherwise the system will remain locked,
* no interrupts served, so USB not going through as well.
* Note: for suspend, need USB_USE_WAIT == TRUE in halconf.h */
if (osalThreadSuspendTimeoutS(&(&USB_DRIVER)->epc[endpoint]->in_state->thread, TIME_MS2I(10)) == MSG_TIMEOUT) {
osalSysUnlock();
return;
}
}
usbStartTransmitI(&USB_DRIVER, endpoint, report, size);
osalSysUnlock();
usbTransmit(&USB_DRIVER, endpoint, report, size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change effectively makes send_report() use infinite timeout — if the host does not poll the corresponding input endpoint, usbTransmit() will block forever when called the second time.

One way to reproduce this is using a slightly modified joystick keymap for onekey:

--- a/keyboards/handwired/onekey/keymaps/joystick/keymap.c
+++ b/keyboards/handwired/onekey/keymaps/joystick/keymap.c
@@ -5,7 +5,7 @@
 #endif

 const uint16_t PROGMEM keymaps[][MATRIX_ROWS][MATRIX_COLS] = {
-    LAYOUT_ortho_1x1(JS_0)
+    LAYOUT_ortho_1x1(KC_A)
 };

 void matrix_scan_user(void) {

and then using

qmk flash -kb handwired/onekey/rp2040 -km joystick -e MOUSEKEY_ENABLE=no -e EXTRAKEY_ENABLE=no

(for some reason JOYSTICK_SHARED_EP=yes and DIGITIZER_SHARED_EP=yes are effectively forced if the shared endpoint is ever used, so everything except the joystick is disabled instead).

With this configuration at least in Linux the key sending KC_A does not work unless some app actually reads the joystick events (e.g., jstest-gtk, and the properties window for the joystick needs to be open).

Maybe something similar could happen when trying to use a more usual keyboard configuration in BIOS, if any event gets sent over the shared endpoint for some reason (the host might not be polling the shared endpoint in such limited environment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's ... fun.

Though, it seems that this block is what seems to be causing issues:

    if (usbGetTransmitStatusI(&USB_DRIVER, endpoint)) {
        /* Need to either suspend, or loop and call unlock/lock during
         * every iteration - otherwise the system will remain locked,
         * no interrupts served, so USB not going through as well.
         * Note: for suspend, need USB_USE_WAIT == TRUE in halconf.h */
        if (osalThreadSuspendTimeoutS(&(&USB_DRIVER)->epc[endpoint]->in_state->thread, TIME_MS2I(10)) == MSG_TIMEOUT) {
            osalSysUnlock();
            return;
        }
    }

And I'm not really sure the best way to fix that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal for a fix is #21656 😉

}

/* prepare and start sending a report IN
Expand Down