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

[Core] suspend: suppress wake up keypress #23389

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Apr 1, 2024

Description

Waking the host from suspend is done by pressing any key on the keyboard, the regular key codes assigned to the keys are not important and must not be sent - otherwise they usually end up in password prompts as ghost characters that have to be deleted again. This PR adds suppression for all keys pressed at the time of wake up. Once a key is released it functions as a regular key again.

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: 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 Apr 1, 2024
@KarlK90 KarlK90 requested a review from a team April 1, 2024 12:52
Comment on lines 63 to 80
void process_wakeup_matrix(uint8_t row, uint8_t col, bool pressed) {
if (!pressed) {
wakeup_matrix[row] &= ~((matrix_row_t)1 << col);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So the wakeup flag gets cleared only when the main loop had seen both the press and the release event for the wakeup key. However, if USB_SUSPEND_WAKEUP_DELAY is defined to a large enough value (and some boards have it defined to 5000), the key will probably be already released when the main loop gets a chance to run again after the wakeup delay; depending on the debouncing algorithm, the wakeup key may not be reported as pressed at that time, therefore the next real press and release events for that key would be eaten by the wakeup key filter.

Not sure what to do about that, apart from moving the filtering and clearing to the matrix_get_row() call in matrix_task() (but then it would run every time instead of only after detecting a change).

Copy link
Member Author

@KarlK90 KarlK90 Apr 1, 2024

Choose a reason for hiding this comment

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

You're right. I have added a update_wakeup_matrix function in 36f3da61f36252f3818114d8bd2e84e355400d97 which is called after the wake up delay and resets only the keys which have been released during the suspend. This should mitigate the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sigprof friendly ping 🙂

@KarlK90 KarlK90 requested review from sigprof and a team April 1, 2024 16:06
@KarlK90 KarlK90 force-pushed the feature/suppress-wake-up-keypress branch from 73cdf41 to 36f3da6 Compare April 1, 2024 16:10
@lokher
Copy link
Contributor

lokher commented Apr 5, 2024

the regular key codes assigned to the keys are not important and must not be sent

  1. Some Windows PCs do wake up on usb remote wakeup signal, but sleep again shortly if there's no valid input from keyboard and mouse HID usage page, this means all zero bytes keyboard/mouse report and valid consumer page report can't wake the PC up.

  2. The USB_SUSPEND_WAKEUP_DELAY is not flexible, IMO first SOF after remote wake up means PC USB controller is ready and it's the best time start to send the report, thus no report will be lost. The problem is a queue is needed to buffer the report and need to change the logic to restore the usb state from USB_SUSPENDED to USB_ACTIVE

Waking the host from suspend is done by pressing any key on the
keyboard, the regular key codes assigned to the keys are not important
and must not be sent - otherwise they usually end up in password prompts
as ghost characters that have to be deleted again. This commit adds
suppression for all keys pressed at the time of wake up. Once a key is
released it functions as a regular key again.

Signed-off-by: Stefan Kerkmann <karlk90@pm.me>
If USB_SUSPEND_WAKEUP_DELAY is set, the keyboard sleeps during wake up -
which can be up to multiple seconds. To not miss any key releases the
wake up matrix is updated with the current state of the matrix - only
resetting the keys that have been released.

Signed-off-by: Stefan Kerkmann <karlk90@pm.me>
@KarlK90 KarlK90 force-pushed the feature/suppress-wake-up-keypress branch from 36f3da6 to db8e91c Compare April 5, 2024 10:30
@KarlK90
Copy link
Member Author

KarlK90 commented Apr 9, 2024

the regular key codes assigned to the keys are not important and must not be sent

1. Some Windows PCs do wake up on usb remote wakeup signal, but sleep again shortly if there's no valid input from keyboard and mouse HID usage page, this means all zero bytes keyboard/mouse report and valid consumer page report can't wake the PC up.

Thanks for pointing this out. Is this behavior observed with this PR as well? The last time I captured the wake up sequence of multiple non-qmk keyboards (some Cherry) non of them sent a keyboard report after the remote wake-up, so these keyboards should show the same problem in theory.

2. The `USB_SUSPEND_WAKEUP_DELAY` is not flexible, IMO first SOF after remote wake up means PC USB controller is ready and it's the best time start to send the report, thus no report will be lost.

This is done by the USB peripheral automatically as long as there is data ready to be sent in the output FIFOs. But I agree the USB_SUSPEND_WAKEUP_DELAY is more of a bandaid, then a proper solution.

The problem is a queue is needed to buffer the report and need to change the logic to restore the usb state from USB_SUSPENDED to USB_ACTIVE

The buffering of the reports was implemented in #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 May 25, 2024
@KarlK90 KarlK90 added awaiting changes awaiting review in progress and removed awaiting changes stale Issues or pull requests that have become inactive without resolution. labels May 29, 2024
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.

None yet

3 participants