Trigger a wakeup after USB Reset on ChibiOS

[firetech]

Description

I have my keyboard (Ergodox Infinity) connected to a powered USB hub/switch that can be connected to two computers allowing me switch between the computers with a button. Currently, switching between computers (or unplugging the cable from the "active" computer) causes suspend_power_down() to be called, but suspend_wakeup_init() never gets called afterwards. I suspect the same would happen with keyboards connected to at least some externally powered USB hubs when the hubs get disconnected from the host.

Some investigation showed that the device got a reset event when the USB switch was triggered, but retained power. Discussing this on Discord notified me about the fact that the USB specification (section 7.1.7.3) says that "Reset must wake a device from the Suspend state.", so a Reset event (or maybe more fitting, a Configured event) should in some way also be an implicit Wakeup. This is only a problem on ChibiOS as it calls suspend_wakeup_init() purely based on events, while the corresponding code for LUFA calls it after exiting the loop that calls suspend_power_down() since #11450 (both implementations have this kind of loop).

This pull request adds a wakeup trigger to the end of handling USB_EVENT_CONFIGURED (if the device was suspended prior to the event) on ChibiOS. I must admit I'm in slightly deep water here regarding the specifics, but it does solve my issue. One could argue that the issue could be solved the same way as for LUFA instead (adding suspend_wakeup_init() after the suspend loop in tmk_core/protocol/chibios/main.c), but I'm not sure if there's a reason for why that's not done before.

Types of Changes

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

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

[sigprof]

To avoid a tiny race condition when the USB interrupt for a new state change comes while usb_event_queue_task() handles the previous state change, maybe it would be better to do usb_event_queue_enqueue(USB_EVENT_WAKEUP); unconditionally, and filter out duplicate wakeup events inside usb_event_queue_task()?

One could argue that the issue could be solved the same way as for LUFA instead (adding suspend_wakeup_init() after the suspend loop in tmk_core/protocol/chibios/main.c), but I'm not sure if there's a reason for why that's not done before.

This actually may be even better, because then it's obvious that suspend_wakeup_init() would always be called after suspend_power_down(), no matter what causes the keyboard to leave the USB suspend state. But this may change the user-visible behavior in some cases (e.g., what happens when the machine is powered off); also the LUFA USB states might not exactly match the states reported by ChibiOS in some cases.

[firetech]

To avoid a tiny race condition when the USB interrupt for a new state change comes while usb_event_queue_task() handles the previous state change, maybe it would be better to do usb_event_queue_enqueue(USB_EVENT_WAKEUP); unconditionally, and filter out duplicate wakeup events inside usb_event_queue_task()?

Hmm, true. I mainly added the if statement to avoid calling suspend_wakeup_init() on the first Configured event.

[firetech]

This actually may be even better, because then it's obvious that suspend_wakeup_init() would always be called after suspend_power_down(), no matter what causes the keyboard to leave the USB suspend state. But this may change the user-visible behavior in some cases (e.g., what happens when the machine is powered off); also the LUFA USB states might not exactly match the states reported by ChibiOS in some cases.

My testing suggests that this would cause e.g. backlight to turn on immediately when data is disconnected but power is still there. Probably not desirable...

[daskygit]

This seems to be behaving well on my split f401, I'm not using a usb switch or hub. The keyboard is directly plugged in at the computer.

After suspending the computer the keyboard would suspend correctly but on resume suspend_wakeup_init() was not called. I did a little debugging (out of my depth really) it seemed the wakeup interrupt never happened, so this check was never true. I did check GINTMSK to make sure it was enabled and it seemed okay.

I didn't manage to work out why, instead I resorted to using this bodge to fix my wakeup issue.

[drashna]

Have you tried using #define USB_SUSPEND_WAKEUP_DELAY 200?

[firetech]

Have you tried using #define USB_SUSPEND_WAKEUP_DELAY 200?

Well, it wouldn't have any effect, as I had to comment out the call to reset_usb_driver to get my keyboard to boot properly after suspend... The hotfix from #10088 has the opposite effect for me, it causes that behaviour to occur, wakeup works perfectly without the hotfix (as I mentioned earlier in that issue). Except for this issue, that is.

(And yes, I tried setting USB_SUSPEND_WAKEUP_DELAY to fix that issue, and it didn't help.)