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.

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

Conversation

drashna
Copy link
Member

@drashna drashna commented Nov 1, 2023

Description

The current code is similar to usbTransmit with some key differences. These differences seem to make the difference with properly resuming from sleep and not, for me. For reference, the usbTransmit function from chibios:
https://github.com/qmk/ChibiOS/blob/11edb1610980f213b9f83161e1715a46fb7e4c51/os/hal/src/hal_usb.c#L586-L601

Tested on F303, F411, F405, RP2040.

Types of Changes

  • Core
  • Bugfix

Issues Fixed or Closed by This PR

  • Issues with resuming from sleep/suspend

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).

@github-actions github-actions bot added the core label Nov 1, 2023
}
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 😉

Copy link

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 bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Dec 18, 2023
@drashna drashna closed this Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants