From 80d5c5e62ed238bfd93426d5202071c7e44d02fa Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Mon, 4 Jan 2021 20:13:16 -0500 Subject: [PATCH 01/10] resolve race condition between suspend and wake in LUFA --- tmk_core/protocol/lufa/lufa.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index 623aa33ff5e4..413a9754cbad 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -435,7 +435,9 @@ void EVENT_USB_Device_Suspend() { */ void EVENT_USB_Device_WakeUp() { print("[W]"); +#if defined(NO_USB_STARTUP_CHECK) suspend_wakeup_init(); +#endif #ifdef SLEEP_LED_ENABLE sleep_led_disable(); @@ -1079,6 +1081,10 @@ int main(void) { if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) { USB_Device_SendRemoteWakeup(); } + if (USB_DeviceState != DEVICE_STATE_Suspended) { + printf("USB_DeviceState = %u\n", USB_DeviceState); + suspend_wakeup_init(); + } } #endif From 5e4be121f9392f7920293fe85d1c5884fb08bdb1 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 00:53:38 -0500 Subject: [PATCH 02/10] avoid multiple calls to suspend_power_down() / suspend_wakeup_init() --- tmk_core/protocol/lufa/lufa.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index 413a9754cbad..f559c90dbb57 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -1073,18 +1073,24 @@ int main(void) { #endif print("Keyboard start.\n"); + bool was_suspended = false; + while (1) { #if !defined(NO_USB_STARTUP_CHECK) while (USB_DeviceState == DEVICE_STATE_Suspended) { - print("[s]"); - suspend_power_down(); + if (!was_suspended) { + print("[s]"); + suspend_power_down(); + was_suspended = true; + } if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) { USB_Device_SendRemoteWakeup(); } - if (USB_DeviceState != DEVICE_STATE_Suspended) { - printf("USB_DeviceState = %u\n", USB_DeviceState); - suspend_wakeup_init(); - } + } + + if (was_suspended) { + suspend_wakeup_init(); + was_suspended = false; } #endif From 755b7760b33b6d9f62cc23f04b5bd5157fe88e94 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 13:48:48 -0500 Subject: [PATCH 03/10] Remove duplicate suspend_power_down_kb() call --- tmk_core/common/avr/suspend.c | 1 - 1 file changed, 1 deletion(-) diff --git a/tmk_core/common/avr/suspend.c b/tmk_core/common/avr/suspend.c index 9db0e0064aba..b784a0835d1e 100644 --- a/tmk_core/common/avr/suspend.c +++ b/tmk_core/common/avr/suspend.c @@ -102,7 +102,6 @@ static void power_down(uint8_t wdto) { # if defined(RGBLIGHT_SLEEP) && defined(RGBLIGHT_ENABLE) rgblight_suspend(); # endif - suspend_power_down_kb(); // TODO: more power saving // See PicoPower application note From 85641467f433404831f0927466a84465626b6214 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 13:49:57 -0500 Subject: [PATCH 04/10] pause on wakeup to wait for USB state to settle --- tmk_core/protocol/lufa/lufa.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index f559c90dbb57..977c195c13d0 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -1085,6 +1085,15 @@ int main(void) { } if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) { USB_Device_SendRemoteWakeup(); + clear_keyboard(); + + // Some hubs, kvm switches, and monitors do + // weird things, with USB device state bouncing + // around wildly on wakeup, yielding race + // conditions that can corrupt the keyboard state. + // + // Pause for a while to let things settle... + wait_ms(200); } } From 28b1d83cd35e458a14f78d4ed981c9827915fde4 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 22:35:08 -0500 Subject: [PATCH 05/10] need the repeated suspend_power_down() (that's where the sleep is) --- tmk_core/protocol/lufa/lufa.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index 977c195c13d0..240b301d3097 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -1073,16 +1073,13 @@ int main(void) { #endif print("Keyboard start.\n"); - bool was_suspended = false; - while (1) { #if !defined(NO_USB_STARTUP_CHECK) + bool was_suspended = false; while (USB_DeviceState == DEVICE_STATE_Suspended) { - if (!was_suspended) { - print("[s]"); - suspend_power_down(); - was_suspended = true; - } + print("[s]"); + suspend_power_down(); + was_suspended = true; if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) { USB_Device_SendRemoteWakeup(); clear_keyboard(); From 7460ec8a34c1d7d9760ef1b8b66090d44ee2f1f7 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 22:44:24 -0500 Subject: [PATCH 06/10] more efficient implementation --- tmk_core/protocol/lufa/lufa.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index 240b301d3097..051e5c041408 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -1075,28 +1075,24 @@ int main(void) { print("Keyboard start.\n"); while (1) { #if !defined(NO_USB_STARTUP_CHECK) - bool was_suspended = false; - while (USB_DeviceState == DEVICE_STATE_Suspended) { + if (USB_DeviceState == DEVICE_STATE_Suspended) { print("[s]"); - suspend_power_down(); - was_suspended = true; - if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) { - USB_Device_SendRemoteWakeup(); - clear_keyboard(); - - // Some hubs, kvm switches, and monitors do - // weird things, with USB device state bouncing - // around wildly on wakeup, yielding race - // conditions that can corrupt the keyboard state. - // - // Pause for a while to let things settle... - wait_ms(200); - } - } + while (USB_DeviceState == DEVICE_STATE_Suspended) { + suspend_power_down(); + if (USB_Device_RemoteWakeupEnabled && suspend_wakeup_condition()) { + USB_Device_SendRemoteWakeup(); + clear_keyboard(); - if (was_suspended) { + // Some hubs, kvm switches, and monitors do + // weird things, with USB device state bouncing + // around wildly on wakeup, yielding race + // conditions that can corrupt the keyboard state. + // + // Pause for a while to let things settle... + wait_ms(200); + } + } suspend_wakeup_init(); - was_suspended = false; } #endif From 1aae2cdbb23ac094ed24c53bc67f7f97bc2c1178 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 23:04:59 -0500 Subject: [PATCH 07/10] fine tune the pause after sending wakeup --- tmk_core/protocol/lufa/lufa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index 051e5c041408..c34b128d9fc3 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -1089,7 +1089,7 @@ int main(void) { // conditions that can corrupt the keyboard state. // // Pause for a while to let things settle... - wait_ms(200); + wait_ms(100); } } suspend_wakeup_init(); From f32e0c8d9928823d3ba42529081fadedee1b5c2c Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Tue, 5 Jan 2021 23:09:40 -0500 Subject: [PATCH 08/10] speculative chibios version of pause-after-wake --- tmk_core/protocol/chibios/main.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tmk_core/protocol/chibios/main.c b/tmk_core/protocol/chibios/main.c index 63e4c99d21ac..512867f96b4c 100644 --- a/tmk_core/protocol/chibios/main.c +++ b/tmk_core/protocol/chibios/main.c @@ -239,6 +239,15 @@ int main(void) { /* Remote wakeup */ if (suspend_wakeup_condition()) { usbWakeupHost(&USB_DRIVER); + + // Some hubs, kvm switches, and monitors do + // weird things, with USB device state bouncing + // around wildly on wakeup, yielding race + // conditions that can corrupt the keyboard state. + // + // Pause for a while to let things settle... + wait_ms(100); + restart_usb_driver(&USB_DRIVER); } } From f1fb3868a43c3213cff5dcb9bd119be74d984667 Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Thu, 7 Jan 2021 10:06:29 -0500 Subject: [PATCH 09/10] make wakeup delay configurable, and adjust value --- docs/config_options.md | 2 ++ tmk_core/common/suspend.h | 4 ++++ tmk_core/protocol/chibios/main.c | 4 +++- tmk_core/protocol/lufa/lufa.c | 4 +++- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/config_options.md b/docs/config_options.md index a3262b418b0f..9a64b9b3d209 100644 --- a/docs/config_options.md +++ b/docs/config_options.md @@ -97,6 +97,8 @@ This is a C header file that is one of the first things included, and will persi * sets the maximum power (in mA) over USB for the device (default: 500) * `#define USB_POLLING_INTERVAL_MS 10` * sets the USB polling rate in milliseconds for the keyboard, mouse, and shared (NKRO/media keys) interfaces +* `#define USB_SUSPEND_WAKEUP_DELAY 200` + * set the number of milliseconde to pause after sending a wakeup packet * `#define F_SCL 100000L` * sets the I2C clock rate speed for keyboards using I2C. The default is `400000L`, except for keyboards using `split_common`, where the default is `100000L`. diff --git a/tmk_core/common/suspend.h b/tmk_core/common/suspend.h index 766df95aa1bc..9d17d984eda5 100644 --- a/tmk_core/common/suspend.h +++ b/tmk_core/common/suspend.h @@ -12,3 +12,7 @@ void suspend_wakeup_init_user(void); void suspend_wakeup_init_kb(void); void suspend_power_down_user(void); void suspend_power_down_kb(void); + +#ifndef USB_SUSPEND_WAKEUP_DELAY +# define USB_SUSPEND_WAKEUP_DELAY 200 +#endif diff --git a/tmk_core/protocol/chibios/main.c b/tmk_core/protocol/chibios/main.c index 512867f96b4c..d7259023338c 100644 --- a/tmk_core/protocol/chibios/main.c +++ b/tmk_core/protocol/chibios/main.c @@ -240,13 +240,15 @@ int main(void) { if (suspend_wakeup_condition()) { usbWakeupHost(&USB_DRIVER); +# if USB_SUSPEND_WAKEUP_DELAY > 0 // Some hubs, kvm switches, and monitors do // weird things, with USB device state bouncing // around wildly on wakeup, yielding race // conditions that can corrupt the keyboard state. // // Pause for a while to let things settle... - wait_ms(100); + wait_ms(USB_SUSPEND_WAKEUP_DELAY); +# endif restart_usb_driver(&USB_DRIVER); } diff --git a/tmk_core/protocol/lufa/lufa.c b/tmk_core/protocol/lufa/lufa.c index c34b128d9fc3..74e48222d0cf 100644 --- a/tmk_core/protocol/lufa/lufa.c +++ b/tmk_core/protocol/lufa/lufa.c @@ -1083,13 +1083,15 @@ int main(void) { USB_Device_SendRemoteWakeup(); clear_keyboard(); +# if USB_SUSPEND_WAKEUP_DELAY > 0 // Some hubs, kvm switches, and monitors do // weird things, with USB device state bouncing // around wildly on wakeup, yielding race // conditions that can corrupt the keyboard state. // // Pause for a while to let things settle... - wait_ms(100); + wait_ms(USB_SUSPEND_WAKEUP_DELAY); +# endif } } suspend_wakeup_init(); From cf51106cc61000b026c06e6162e5e671435d1bab Mon Sep 17 00:00:00 2001 From: Joshua Diamond Date: Mon, 1 Feb 2021 11:12:56 -0500 Subject: [PATCH 10/10] better location for wakeup delay --- tmk_core/protocol/chibios/main.c | 11 ----------- tmk_core/protocol/chibios/usb_main.c | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tmk_core/protocol/chibios/main.c b/tmk_core/protocol/chibios/main.c index d7259023338c..63e4c99d21ac 100644 --- a/tmk_core/protocol/chibios/main.c +++ b/tmk_core/protocol/chibios/main.c @@ -239,17 +239,6 @@ int main(void) { /* Remote wakeup */ if (suspend_wakeup_condition()) { usbWakeupHost(&USB_DRIVER); - -# if USB_SUSPEND_WAKEUP_DELAY > 0 - // Some hubs, kvm switches, and monitors do - // weird things, with USB device state bouncing - // around wildly on wakeup, yielding race - // conditions that can corrupt the keyboard state. - // - // Pause for a while to let things settle... - wait_ms(USB_SUSPEND_WAKEUP_DELAY); -# endif - restart_usb_driver(&USB_DRIVER); } } diff --git a/tmk_core/protocol/chibios/usb_main.c b/tmk_core/protocol/chibios/usb_main.c index cb7aeb23b238..13b1e34d2825 100644 --- a/tmk_core/protocol/chibios/usb_main.c +++ b/tmk_core/protocol/chibios/usb_main.c @@ -708,6 +708,17 @@ void init_usb_driver(USBDriver *usbp) { void restart_usb_driver(USBDriver *usbp) { usbStop(usbp); usbDisconnectBus(usbp); + +#if USB_SUSPEND_WAKEUP_DELAY > 0 + // Some hubs, kvm switches, and monitors do + // weird things, with USB device state bouncing + // around wildly on wakeup, yielding race + // conditions that can corrupt the keyboard state. + // + // Pause for a while to let things settle... + wait_ms(USB_SUSPEND_WAKEUP_DELAY); +#endif + usbStart(usbp, &usbcfg); usbConnectBus(usbp); }