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/usb_main: re-check USB status in send_keyboard after sleeping the thread #7784

Merged
merged 2 commits into from
Jan 13, 2020

Conversation

xyzz
Copy link
Contributor

@xyzz xyzz commented Jan 4, 2020

Fixes #5585 (attempt 2)

Description

USB status might change from USB_ACTIVE while the thread is sleeping in osalThreadSuspendS. If the status is not USB_ACTIVE, we don't have any endpoints and attempting to send on them crashes. Discard these sends. Also fix a race condition in the same function.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • 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).

@yanfali
Copy link
Contributor

yanfali commented Jan 4, 2020

Rather than returning I would prefer to see a go-to to the unlock code so we don't have to think about 3 unlock sites. What do you think?

@xyzz
Copy link
Contributor Author

xyzz commented Jan 4, 2020

Sure.

@xyzz xyzz marked this pull request as ready for review January 5, 2020 16:03
@goshdarnharris
Copy link
Contributor

Looks good on onekey f072 here.

Copy link
Contributor

@yanfali yanfali left a comment

Choose a reason for hiding this comment

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

Tested on a repeatable use case with NK65, board recovered gracefully after patch. Before that hard lock.

  • powered USB 2 hub
  • connected to macbook through dongle
  • unplug dongle, plug dongle back in
  • hadron v3 (arm) locked hard
  • previously nk65 locked, after patch it survived

@yanfali
Copy link
Contributor

yanfali commented Jan 6, 2020

Patched hadron v3, now it survives the same test. Approved++

@nicolai86
Copy link

👍, this fixes Ganymede locking up after sleeps of my MacBookPro, connected through a startech usb-c hub.

@drashna drashna requested a review from a team January 8, 2020 04:58
@yanfali
Copy link
Contributor

yanfali commented Jan 8, 2020

I've been running both keyboards continuously with sleep enabled on a macbook. Aside from losing initial keypresses, which is to be expected, keyboard is now well behaved and able to resume correctly. Let's get this merged.

Copy link
Contributor

@mechmerlin mechmerlin left a comment

Choose a reason for hiding this comment

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

My motivation to testing this is because I run a dual boot hackintosh and Windows 10 system. After watching the Witcher on Netflix, I was suddenly inspired to replay Witcher 3. However, to my chagrin, the board I was currently using, the NK65 could not allow me to access my boot menu on restart. Thanks to this, I can now play Witcher 3 without swapping boards lol.

This has been tested using the following methodology

  1. Shutdown computer
  2. Turn on computer
  3. Press F12, or Fn + = key when options are available.

Prior to this change, the methodology listed above never allowed me to successfully enter the boot menu.

This has been tested working on the following Arm boards in my collection

  1. NK65
  2. Clueboard 66 hotswap gen 1
  3. KBD67mkiirgb v1
  4. DZ60rgb-ansi v1

@mechmerlin mechmerlin merged commit 83be1ae into qmk:master Jan 13, 2020
@zvecr zvecr mentioned this pull request Jan 19, 2020
13 tasks
@brurion123
Copy link

Hello, Thank you for the fix. sorry to be a bother but how do I apply this to my DZ60 rgb? I appreciate any help. thanks

@noroadsleft
Copy link
Member

noroadsleft commented Jan 25, 2020

Hello, Thank you for the fix. sorry to be a bother but how do I apply this to my DZ60 rgb? I appreciate any help. thanks

@brurion123,

Depends on how you do your firmware. If you use QMK Configurator to create your firmware, it will already be applied there.

If you're compiling locally, you should clone the QMK Firmware repository (instructions – back up your keymap file somewhere safe before you do; a copy-paste to a different folder is fine) – then when you have the repo cloned, move your keymap back and compile it. The firmware you get should have this fix included.

drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 12, 2020
… the thread (qmk#7784)

* chibios/usb_main: re-check USB status in send_keyboard after sleeping the thread

* change send_keyboard to only have 1 exit point
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 14, 2020
… the thread (qmk#7784)

* chibios/usb_main: re-check USB status in send_keyboard after sleeping the thread

* change send_keyboard to only have 1 exit point
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
… the thread (qmk#7784)

* chibios/usb_main: re-check USB status in send_keyboard after sleeping the thread

* change send_keyboard to only have 1 exit point
fdidron pushed a commit to zsa/qmk_firmware that referenced this pull request Feb 26, 2020
… the thread (qmk#7784)

* chibios/usb_main: re-check USB status in send_keyboard after sleeping the thread

* change send_keyboard to only have 1 exit point
@xster
Copy link
Contributor

xster commented Mar 10, 2020

Thanks so much for fixing this. Solved such a huge headache I had where I had to turn unplug and replug my keyboard each time the mac host went to sleep.

kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
… the thread (qmk#7784)

* chibios/usb_main: re-check USB status in send_keyboard after sleeping the thread

* change send_keyboard to only have 1 exit point
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
… the thread (qmk#7784)

* chibios/usb_main: re-check USB status in send_keyboard after sleeping the thread

* change send_keyboard to only have 1 exit point
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HS60/v2 keyboard unresponsive after PC wakes from sleep
9 participants