Address wake from sleep instability #11450

spidey3 · 2021-01-05T22:43:45Z

Description

My work-from-home setup is a complicated mess of KVM switches, USB hubs, etc. Sleep / wake works fine when the keyboard is plugged directly into the computer, but through the switches / hubs it becomes unreliable, with the keyboard hanging sometimes on wake, or lighting being wrong after wakeup. Other people have reported similar experiences, especially when using KVM switches or when using pass-through USB on a USB-C or HDMI connected monitor.

Debugging showed that when a key is tapped to wake the board / host, if it goes through the switch, instead of USB device state going smoothly from Suspended to Configured, it goes back and forth a few times before settling on Configured. When that happens, sometimes, the keyboard hangs. Other times the host just doesn't stay awake.

After spending some time looking into this, I've applied a few targeted changes:

Removed a duplicate call to suspend_power_down_kb() (it was called twice for each suspend cycle).
Added a short (200ms) delay after sending the RemoteWakeup packet to allow USB Device State to "settle" before continuing the loop.
~~At present, the suspend code is called repeatedly in a tight loop during suspend. I've made a change so that it is only called once on the transition to Suspended state.~~ [It turns out that the repeated calls are required, as that is where the low-power sleep cycle is implemented, so this has been restored.]

At the moment this is a draft, as I've only been able to test it on a few boards, and only for a few days. Help testing this would be greatly appreciated.

It's been tested thoroughly on atmega32u4, and on at90usb1286
It should be good on AVR in general, but I'd appreciate folks testing on other AVR MCUs.
~~Code has been written for ARM / chibiOS, but is has not been tested at all yet.~~
It has been tested on ARM / chibiOS with STM32F103 (bluepill) and STM32F303 (proton c).

Note that I am targeting the develop branch, as this seems a bit too deep and critical to go directly to master...

Types of Changes

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-01-06T03:11:02Z

This should also target the chibiOS suspend, too

spidey3 · 2021-01-06T04:19:28Z

This should also target the chibiOS suspend, too

I just added some speculative code for chibiOS, but I don't have an ARM-based board on which to test it (yet - bluepill on the way). Help with testing would be appreciated!

tmk_core/common/avr/suspend.c

drashna · 2021-01-07T17:37:08Z

Unfortunately, testing this, this breaks sleep for me, outright. Computer IMMEDIATELY wakes up after trying to put it to sleep. And this is on macOS Catalina.

Windows PC works fine.

spidey3 · 2021-01-07T17:44:37Z

Unfortunately, testing this, this breaks sleep for me, outright. Computer IMMEDIATELY wakes up after trying to put it to sleep. And this is on macOS Catalina.

Is that on AVR or on ARM?

drashna · 2021-01-07T17:59:15Z

AVR.

spidey3 · 2021-01-07T18:08:14Z

AVR.

Hmm... I am trying to imagine how this code could cause that, since it doesn't send anything to the host that would wake it up unless suspend_wakeup_condition() returns true. I'll need to do some more A/B testing with and without the new code on my wife's Mac [once she is done working for the day].

spidey3 · 2021-01-07T18:20:23Z

Unfortunately, testing this, this breaks sleep for me, outright. Computer IMMEDIATELY wakes up after trying to put it to sleep. And this is on macOS Catalina.

One more question: How soon is "IMMEDIATELY"? Less that a second? Less than 10 seconds? I just did a quick test on the Mac, and didn't see it wake up right away.

Also, what method did you use to put the Mac to sleep?

drashna · 2021-01-07T18:27:36Z

Oh, boy, this is fun.
With an atmega32u4, no issues.
But with the at90usb1286, within a second of being put to sleep, it wakes the system back up. This is with both teensy++2.0's, and the planck light

And I use the sleep option in the apple menu.

spidey3 · 2021-01-07T18:56:22Z

Oh, boy, this is fun.

Yeah!

With an atmega32u4, no issues.
But with the at90usb1286, within a second of being put to sleep, it wakes the system back up. This is with both teensy++2.0's, and the planck light

Sigh. Time to buy yet another flavor of MCU so that I can test stuff... :)
I presume you verified that sleep was working with the at90usb1286 without the PR...

And I use the sleep option in the apple menu.

That's what I was doing as well.

drashna · 2021-01-07T19:22:55Z

I presume you verified that sleep was working with the at90usb1286 without the PR...

Yup, sleeps and wakes just fine without.

daskygit · 2021-01-08T05:14:37Z

I did some quick testing of this on a split stm32f401, computer suspends and wakes correctly (Windows 10).

RGB behavior is a little odd with RGBLIGHT_SLEEP defined but that also seems to true with master branch, I'll do some more testing when I get a chance.

spidey3 · 2021-01-08T16:46:57Z

RGB behavior is a little odd with RGBLIGHT_SLEEP defined but that also seems to true with master branch, I'll do some more testing when I get a chance.

Can you be specific about exactly what you mean by "a little odd"? I've also seen some oddities, but most are explained by:

Hosts / hubs that bounce the device status around oddly
The issue addressed in Lighting Layers should be disabled when suspended #11442

daskygit · 2021-01-09T01:50:01Z

Can you be specific about exactly what you mean by "a little odd"?

I'm using the following hardware 2x STM32F401 using split common with no eeprom.

The rgblight is enabled using the code below and I do not use any rgblight layers.

void keyboard_post_init_user(void) {
  rgblight_enable_noeeprom();
  rgblight_sethsv_noeeprom(255, 255, 255);
}

Master, PR#11450 and PR#11442 all show the same behaviour with rgblight enabled and rgblight_sleep defined. I thought I noticed different behaviour but it seems I was mistaken. The computer sleeps without issue and wakes via key press fine, also I've had no incidents of self waking.

This is what's happening with the rgb lighting but I guess it might be more relevant to the other pull request, I've bracketed anything I did manually.

(Plug in keyboard), Lights come on, (Toggle on breathing mode), (Suspend comp), Lights go off, (Wake comp), Lights stay off, (Toggle lights on), (Suspend comp), Lights stay on and animation freezes, (Wake comp), Lights still on and animation still frozen.
(Turn on comp), Lights come on then turn off within a second and stay off while booting.

spidey3 · 2021-01-09T03:21:43Z

(Plug in keyboard), Lights come on, (Toggle on breathing mode), (Suspend comp), Lights go off, (Wake comp), Lights stay off, (Toggle lights on), (Suspend comp), Lights stay on and animation freezes, (Wake comp), Lights still on and animation still frozen.

(Turn on comp), Lights come on then turn off within a second and stay off while booting.

I think it is relevant to this PR - would like to understand it. Seems like rgblight is not being re-enabled on wakeup, which would imply that one of the following two cases is occurring:

rgblight_config.enable == false at the time of suspend
rgblight is not being enabled on wakeup

Do you have anything in suspend_power_down_user() or suspend_wakeup_init_user()?

daskygit · 2021-01-11T00:31:18Z

I did a little more playing, it doesn't seem like USB_EVENT_WAKEUP ever happens on my stm32 keyboard. I've got a bodge working but it's probably not a real fix. Just to be clear without my bodge suspend is never called on master either.

I've logged the behaviour for 2 different split keyboards, one avr (atmega32u4) and the other arm (stm32f401).

Below is this PR and my bodge for arm.

Plugged in:
AVR:S:W
ARM: //neither are called (which is fine)

Computer suspended and woke by keyboard:
AVR:S:W:S:W
ARM:S:W:S:W

Computer suspended and woke by power button:
AVR:S:W
ARM:S:W

Below is master and my bodge for arm.

Plugged in:
AVR:S:W:W:W
ARM:

Computer suspended and woke by keyboard:
AVR:S:W:S:W:W:W
ARM:S:W:S:W:S:W

Computer suspended and woke by power button:
AVR:S:W:W:W
ARM:S:W

Repeated calls to suspend are ignored, my crude buffer for logging wasn't big enough on avr so I disabled on both but there's definitely an improvement.

spidey3 · 2021-01-11T01:34:26Z

I did a little more playing, it doesn't seem like USB_EVENT_WAKEUP ever happens on my stm32 keyboard. I've got a bodge working but it's probably not a real fix. Just to be clear without my bodge suspend is never called on master either.

I just had a look at the bodge, and the funny thing is that it's almost identical to a change that I am planning to add to this PR. I need to do a bit more testing on the bluepill onekey that I just built yesterday, though, to verify that it works consistently.

That being said, like you, I cannot believe that suspend_wakeup_init() isn't being called for anyone on ARM, or that USB_EVENT_WAKEUP isn't happening for them. Do you have any odd setup? Any USB switches, hubs, or pass-through devices between your keeb and the host?

XBowsTech · 2021-01-11T01:37:18Z

(Plug in keyboard), Lights come on, (Toggle on breathing mode), (Suspend comp), Lights go off, (Wake comp), Lights stay off, (Toggle lights on), (Suspend comp), Lights stay on and animation freezes, (Wake comp), Lights still on and animation still frozen.

(Turn on comp), Lights come on then turn off within a second and stay off while booting.

I think it is relevant to this PR - would like to understand it. Seems like rgblight is not being re-enabled on wakeup, which would imply that one of the following two cases is occurring:

rgblight_config.enable == false at the time of suspend

rgblight is not being enabled on wakeup

Do you have anything in suspend_power_down_user() or suspend_wakeup_init_user()?

I think you're getting close to the truth.
I've tried two ways to solve this problem.

Increase the capacitance;
Set SLEEP_LED_ENABLE = yes
And here are my Settings for suspend_power_down_user() and suspend_wakeup_init_user()

void suspend_power_down_kb(void) {
rgb_matrix_set_suspend_state(true);
suspend_power_down_user();
}

void suspend_wakeup_init_kb(void) {
rgb_matrix_set_suspend_state(false);
suspend_wakeup_init_user();
}

daskygit · 2021-01-11T01:58:29Z

@XBowsTech which keyboard and mcu is this? Also, do you have #define RGB_DISABLE_WHEN_USB_SUSPENDED true in your config.h?

XBowsTech · 2021-01-11T02:03:02Z

@XBowsTech which keyboard and mcu is this? Also, do you have in your config.h?#define RGB_DISABLE_WHEN_USB_SUSPENDED true

xbows/nature
https://github.com/qmk/qmk_firmware/tree/master/keyboards/xbows/nature

spidey3 · 2021-01-11T02:03:42Z

Increase the capacitance;

Set SLEEP_LED_ENABLE = yes
And here are my Settings for suspend_power_down_user() and suspend_wakeup_init_user()

@XBowsTech did either of those help?

I'm running with SLEEP_LED_ENABLE = no.
I don't think a solution that required hardware changes is viable.

XBowsTech · 2021-01-11T02:08:08Z

Increase the capacitance;

Set SLEEP_LED_ENABLE = yes
And here are my Settings for suspend_power_down_user() and suspend_wakeup_init_user()

@XBowsTech did either of those help?

I'm running with SLEEP_LED_ENABLE = no.
I don't think a solution that required hardware changes is viable.

I've tested it myself. It works.

Are you using IS31FL3731?

spidey3 · 2021-01-11T02:56:31Z

@XBowsTech did either of those help?
I'm running with SLEEP_LED_ENABLE = no.
I don't think a solution that required hardware changes is viable.

I've tested it myself. It works.

Glad to hear that - but I need to get this working without SLEEP_LED_ENABLE; need to get the kb into lowest power state, and can't do that with the breathing on.

Are you using IS31FL3731?

No; my boards all use WS2812. But I don't think that's really the issue. This is all about suspend states and host interaction. I think the LEDs are just a symptom...

spidey3 · 2021-01-11T03:05:55Z

@XBowsTech which keyboard and mcu is this? Also, do you have #define RGB_DISABLE_WHEN_USB_SUSPENDED true in your config.h?

I am testing on kbd75 (AVR, atmega32u4), and on a bluepill (STM32F103). at90usb1286 is on it's way here so I can test that.

RGB_DISABLE_WHEN_USB_SUSPENDED is not relevant; it only applies to RGB Matrix, and I don't have that installed or enabled on any of these devices.

XBowsTech · 2021-01-11T03:06:41Z

You can try it. In fact, LEDs don't light up when the computer is asleep.

I'm not sure if this approach will work for WS2812.
But for my keyboard it works, and the reason it works is probably because of the charge-discharge properties of the capacitors.

spidey3 · 2021-01-11T03:09:47Z

You can try it. In fact, LEDs don't light up when the computer is asleep.

Like I said - that works for RGB Matrix, but NOT for RGB Lighting (underglow). The RGB_DISABLE_WHEN_USB_SUSPENDED macro is defined and used only in the context of quantum/rgb_matrix.c.

I'm not sure if this approach will work for WS2812.
But for my keyboard it works, and the reason it works is probably because of the charge-discharge properties of the capacitors.

Again, we need to come up with a solution that does not require non-hardware hacking folks to physically modify their keyboards.

And as I said, all of this mess with the LEDs is just a symptom. The real problem is that the methods that enable and disable them are not being called at the right times, and that's all because the suspend / wake state of the keyboard is incorrect.

spidey3 · 2021-01-13T03:01:34Z

Oh, boy, this is fun.
With an atmega32u4, no issues.
But with the at90usb1286, within a second of being put to sleep, it wakes the system back up. This is with both teensy++2.0's, and the planck light

@drashna — I just tried with an at90usb1286 (Teensy++ 2.0) on a Mac with Catalina and I am not seeing the problem that you mention. When I click on Sleep the screen goes dark, and then within 30 to 60 seconds the keyboard is suspended and the LEDs are shut off.

I am running with SLEEP_LED_ENABLE = no and #define RGBLIGHT_SLEEP.

That being said, it is surprising that you are seeing that odd behavior on two different Teensy boards and also on your Planck. I wonder — do you have anything unusual in suspend_power_down_user()? Is there some code that I could look at?

spidey3 · 2021-01-14T18:56:08Z

Per this discord discussion the problem that @drashna saw is no longer reproducible, so I am taking this PR out of draft and submitting it for review.

Note: Notes that are playing are not stopped on suspend, but that is a known issue, not related to the topic being addressed in this PR.

tmk_core/protocol/chibios/main.c

tzarc · 2021-02-02T00:12:52Z

Thanks!

spidey3 · 2021-02-02T00:16:39Z

Thanks!

You're welcome!

* resolve race condition between suspend and wake in LUFA * avoid multiple calls to suspend_power_down() / suspend_wakeup_init() * Remove duplicate suspend_power_down_kb() call * pause on wakeup to wait for USB state to settle * need the repeated suspend_power_down() (that's where the sleep is) * more efficient implementation * fine tune the pause after sending wakeup * speculative chibios version of pause-after-wake * make wakeup delay configurable, and adjust value * better location for wakeup delay

github-actions bot added the core label Jan 5, 2021

spidey3 changed the title ~~Address wake from sleep instability in AVR / lufa based boards.~~ Address wake from sleep instability Jan 6, 2021

drashna reviewed Jan 7, 2021

View reviewed changes

tmk_core/common/avr/suspend.c Show resolved Hide resolved

spidey3 mentioned this pull request Jan 7, 2021

Planck EZ freezes when any key is pressed while suspended #6369

Open

github-actions bot added the documentation label Jan 7, 2021

spidey3 marked this pull request as ready for review January 14, 2021 18:56

spidey3 requested a review from drashna January 14, 2021 20:31

drashna requested a review from a team January 20, 2021 02:46

drashna approved these changes Jan 29, 2021

View reviewed changes

drashna requested a review from a team January 29, 2021 17:24

nooges approved these changes Jan 31, 2021

View reviewed changes

tzarc requested changes Jan 31, 2021

View reviewed changes

tmk_core/protocol/chibios/main.c Outdated Show resolved Hide resolved

spidey3 added 10 commits February 1, 2021 11:31

resolve race condition between suspend and wake in LUFA

80d5c5e

avoid multiple calls to suspend_power_down() / suspend_wakeup_init()

5e4be12

Remove duplicate suspend_power_down_kb() call

755b776

pause on wakeup to wait for USB state to settle

8564146

need the repeated suspend_power_down() (that's where the sleep is)

28b1d83

more efficient implementation

7460ec8

fine tune the pause after sending wakeup

1aae2cd

speculative chibios version of pause-after-wake

f32e0c8

make wakeup delay configurable, and adjust value

f1fb386

better location for wakeup delay

cf51106

spidey3 force-pushed the startup_race_condition branch from 88d6f54 to cf51106 Compare February 1, 2021 16:33

spidey3 requested a review from tzarc February 1, 2021 16:38

tzarc approved these changes Feb 2, 2021

View reviewed changes

tzarc merged commit 9a4618b into qmk:develop Feb 2, 2021

firetech mentioned this pull request May 8, 2021

Trigger a wakeup after USB Reset on ChibiOS #12831

Merged

14 tasks

sigprof mentioned this pull request Oct 30, 2021

Fix remote wakeup issue #14988

Closed

14 tasks

tmk mentioned this pull request Feb 26, 2023

NeXT-USB: Wakeup problem on Mac and Remote Wakeup tmk/tmk_keyboard#753

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address wake from sleep instability #11450

Address wake from sleep instability #11450

spidey3 commented Jan 5, 2021 •

edited

Loading

drashna commented Jan 6, 2021

spidey3 commented Jan 6, 2021 •

edited

Loading

drashna commented Jan 7, 2021

spidey3 commented Jan 7, 2021 •

edited

Loading

drashna commented Jan 7, 2021

spidey3 commented Jan 7, 2021

spidey3 commented Jan 7, 2021

drashna commented Jan 7, 2021

spidey3 commented Jan 7, 2021

drashna commented Jan 7, 2021

daskygit commented Jan 8, 2021

spidey3 commented Jan 8, 2021

daskygit commented Jan 9, 2021

spidey3 commented Jan 9, 2021

daskygit commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 11, 2021

XBowsTech commented Jan 11, 2021 •

edited

Loading

daskygit commented Jan 11, 2021

XBowsTech commented Jan 11, 2021

spidey3 commented Jan 11, 2021

XBowsTech commented Jan 11, 2021

spidey3 commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 11, 2021

XBowsTech commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 13, 2021

spidey3 commented Jan 14, 2021 •

edited

Loading

tzarc commented Feb 2, 2021

spidey3 commented Feb 2, 2021

Address wake from sleep instability #11450

Address wake from sleep instability #11450

Conversation

spidey3 commented Jan 5, 2021 • edited Loading

Description

Types of Changes

Checklist

drashna commented Jan 6, 2021

spidey3 commented Jan 6, 2021 • edited Loading

drashna commented Jan 7, 2021

spidey3 commented Jan 7, 2021 • edited Loading

drashna commented Jan 7, 2021

spidey3 commented Jan 7, 2021

spidey3 commented Jan 7, 2021

drashna commented Jan 7, 2021

spidey3 commented Jan 7, 2021

drashna commented Jan 7, 2021

daskygit commented Jan 8, 2021

spidey3 commented Jan 8, 2021

daskygit commented Jan 9, 2021

spidey3 commented Jan 9, 2021

daskygit commented Jan 11, 2021 • edited Loading

spidey3 commented Jan 11, 2021

XBowsTech commented Jan 11, 2021 • edited Loading

daskygit commented Jan 11, 2021

XBowsTech commented Jan 11, 2021

spidey3 commented Jan 11, 2021

XBowsTech commented Jan 11, 2021

spidey3 commented Jan 11, 2021 • edited Loading

spidey3 commented Jan 11, 2021

XBowsTech commented Jan 11, 2021 • edited Loading

spidey3 commented Jan 11, 2021 • edited Loading

spidey3 commented Jan 13, 2021

spidey3 commented Jan 14, 2021 • edited Loading

tzarc commented Feb 2, 2021

spidey3 commented Feb 2, 2021

spidey3 commented Jan 5, 2021 •

edited

Loading

spidey3 commented Jan 6, 2021 •

edited

Loading

spidey3 commented Jan 7, 2021 •

edited

Loading

daskygit commented Jan 11, 2021 •

edited

Loading

XBowsTech commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 11, 2021 •

edited

Loading

XBowsTech commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 11, 2021 •

edited

Loading

spidey3 commented Jan 14, 2021 •

edited

Loading