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] Process all changed keys in one scan loop, deprecate QMK_KEYS_PER_SCAN #15292

Merged
merged 2 commits into from
Aug 6, 2022

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Nov 24, 2021

For Reviewers: The changeset is huge as many keyboards where adjusted in the process.
The important changes to core functionality are in separate and small commits. I have taken great care to not break any existing keyboard, hopefully this was successful.

Description

Short version

  • Move matrix processing from keyboard_task() into seperate matrix_task()
  • Process all key changes in one scan loop if there are changes in the key matrix
    • Deprecate QMK_KEYS_PER_SCAN (Allow multiple process_record() calls per scan #2022) which previously allowed to process more than one key in a single scan
    • Make debounce() algorithms signal changes in the debounced "cooked" keyboard matrix via a return value (will be a separate PR)
  • Add LEGACY_MATRIX_SCAN bodge and info message for custom matrix scanning implementations that do not signal matrix changes (will be a separate PR)

Long version

This PR moves the core matrix processing from keyboard_task() into a separate matrix_task(). Processing all key changes of the matrix is also done in one scan loop if and only if there are key changes to be processed. This lazy evaluation of key changes is done by making the debounce() algorithms signal changes in the debounced "cooked" keyboard matrix via a return value. Previously the processing of more than one key change per scan loop could be enabled by setting QMK_KEYS_PER_SCAN to a number like 4 or 5 which was introduced in #2022 and I guess as a precaution not enabled for other keyboards. As a temporary bodge custom keyboard matrix implementations get a config flag LEGACY_MATRIX_SCAN which activates a info message like this:

Size impacts

ARM: +220 Bytes

❯ ./util/size_regression.sh -d maintenance/keyboard-scan-task planck/rev6:default
Size:    53368, delta:    +32 -- 9f6e1703bc Process the complete matrix in one scan loop
Size:    53336, delta:   +188 -- 7d5d9c3472 Remove QMK_KEYS_PER_SCAN option
Size:    53148, delta:     +0 -- 168a631720 Add support for MacroCat Keyboard (#17480)

AVR: -154 Bytes

❯ ./util/size_regression.sh -d maintenance/keyboard-scan-task crkbd:default
Size:    26254, delta:    +60 -- 9f6e1703bc Process the complete matrix in one scan loop
Size:    26194, delta:   -214 -- 7d5d9c3472 Remove QMK_KEYS_PER_SCAN option
Size:    26408, delta:     +0 -- 168a631720 Add support for MacroCat Keyboard (#17480)

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

  • (Hopefully) Reduce code complexity by refactoring the matrix loop into a separate task
  • Make QMK_KEYS_PER_SCAN the default behavior as this benefits some users and won't hurt others
  • Clean-up some matrix_scan() API and use its potential for lazy evaluation of the matrix
  • Remove dead code

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 core documentation keyboard keymap translation via Adds via keymap and/or updates keyboard for via support labels Nov 24, 2021
@KarlK90 KarlK90 changed the title [Core] Refactor keyboard scanning task Draft: [Core] Refactor keyboard scanning task Nov 25, 2021
@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch 6 times, most recently from 2425087 to d2e4d49 Compare November 26, 2021 09:47
@KarlK90 KarlK90 changed the title Draft: [Core] Refactor keyboard scanning task Draft: [Core] Refactor keyboard matrix processing to process all keys in one scan Nov 26, 2021
@KarlK90 KarlK90 changed the title Draft: [Core] Refactor keyboard matrix processing to process all keys in one scan Draft: [Core] Refactor matrix processing to process all keys in one scan Nov 26, 2021
@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch 3 times, most recently from d641f87 to 47eaa12 Compare November 26, 2021 11:28
@KarlK90 KarlK90 marked this pull request as ready for review November 26, 2021 11:29
@KarlK90 KarlK90 changed the title Draft: [Core] Refactor matrix processing to process all keys in one scan [Core] Refactor matrix processing to process all keys in one scan Nov 26, 2021
@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch from 47eaa12 to 41c8ba5 Compare November 26, 2021 19:41
@tzarc tzarc requested a review from a team November 26, 2021 21:43
@zvecr
Copy link
Member

zvecr commented Nov 28, 2021

Remove deprecated matrix_is_modified() dead code

This should really be tackled in its own PR.

@drashna
Copy link
Member

drashna commented Nov 28, 2021

Doesn't appear to break anything, at the very least. been running it for a full day now, and appears to work fine.

@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch from 41c8ba5 to 0bfe966 Compare November 29, 2021 12:16
@KarlK90
Copy link
Member Author

KarlK90 commented Nov 29, 2021

@zvecr I have dropped the commit and openend #15349 that removes yet another unused method.

@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch from 0bfe966 to 2ce68ed Compare November 29, 2021 20:06
@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch from 2ce68ed to a60e4a8 Compare December 7, 2021 09:56
@stale
Copy link

stale bot commented Apr 16, 2022

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 awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@tzarc
Copy link
Member

tzarc commented May 15, 2022

Deferred to Q3 cycle due to conflicts.

@ecerulm
Copy link

ecerulm commented May 23, 2022

@KarlK90 , is this PR still alive?

@KarlK90
Copy link
Member Author

KarlK90 commented May 23, 2022

@ecerulm currently on hold until the rp2040 support is finished

@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch 6 times, most recently from 70373ec to 9f6e170 Compare July 3, 2022 20:24
@KarlK90 KarlK90 changed the title [Core] Refactor matrix processing to process all keys in one scan [Core] Process all changed keys in one scan loop, deprecate QMK_KEYS_PER_SCAN Jul 3, 2022
@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch 2 times, most recently from 2c9c44d to 5792aee Compare August 6, 2022 09:58
...as this will become the default behaviour for matrix scanning and
processing code.
Which is mostly what QMK_KEYS_PER_SCAN did without a limit on how many keys
shall be processed.
@KarlK90 KarlK90 force-pushed the maintenance/keyboard-scan-task branch from 5792aee to 480d502 Compare August 6, 2022 10:01
@alex-ong
Copy link
Contributor

alex-ong commented Oct 3, 2022

Thanks for the merge; does QMK now send multiple keys in one report? or is that waiting for #12686?

@KarlK90
Copy link
Member Author

KarlK90 commented Oct 3, 2022

No QMK still only sends one key change per report.

nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core documentation in progress keyboard keymap translation via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants