search

FrameworkComputer / qmk_firmware

Fork of QMK for the Framework Laptop 16
GNU General Public License v2.0
84 stars 24 forks source link

[Feature] Refactor matrix scanning implementation #24

Closed KarlK90 closed 7 months ago

KarlK90 commented 1 year ago

Description

Hi @JohnAZoidberg and Framework :wave:,

QMK maintainer and author of the RP2040 port here. I've followed your announcement of the Framework 16 and was pretty much blown away, such a cool product/project. As I heard that the input modules would be based on QMK and the RP2040 I naturally had to take a look. Reading through the issues I stumbled upon https://github.com/FrameworkComputer/qmk_firmware/issues/8 which kinda scratched an itch :wink:. So here is an PR with a refactoring of the matrix scanning implementation.

I've tested it as best a possible on a RPi Pico as I don't have a Framework 16 (yet?)

Details

The former matrix scan implementation (for the ISO variant) caped at around ~500Hz which is around half of the USB full speed bandwidth. As QMK is one big busy loop the faster the matrix scanning is done, the more cycles are available for further QMK shenanigans. This refactored implementation is around 4x faster and sits at ~2.1kHz on a pico proto board which gives good headroom.

The following changes brought the biggest wins:

  1. Use the pico-sdk ADC reading implementation as it uses busy waiting which has much less overhead compared to the current ChibiOS implementation that has context switching overhead.
  2. Pre-compute all row and col to pin lookups.
  3. Use whole port based IO to set multiple pins at once, instead of setting each pin individually.

For correctness the scanning now dynamically waits for all IO pins/ports to reach their expected target level before attempting to read matrix changes.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

JohnAZoidberg commented 1 year ago

That is awesome! Thanks so much for the help!

I'll check it out in detail after the weekend. I just tested it on the full size keyboard where it can reach ~1250Hz now. Compared to 500Hz previously. On the numpad module it goes from ~2350Hz to ~5750Hz.

KarlK90 commented 1 year ago

That is awesome! Thanks so much for the help!

You're welcome.

I'll check it out in detail after the weekend. I just tested it on the full size keyboard where it can reach ~1250Hz now. Compared to 500Hz previously. On the numpad module it goes from ~2350Hz to ~5750Hz.

That is a nice gain, I suspect that the fall and rise times on real hardware are longer (because of the capacitance of the wires/matrix) than on my Pico protoboard and therefore the scan rate is lower as the pin/port waiting functions have to wait longer.

JohnAZoidberg commented 1 year ago

Hey, sorry, I will get back to this. It's a really great improvement that we definitely want in one way or another. But it's not working well in the current implementation. Some keys don't work and n-key rollover is worse.

KarlK90 commented 1 year ago

No problem, I don't have access to real hardware so verification and bug fixing wasn't possible on my side.

github-actions[bot] commented 1 year ago

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.

KarlK90 commented 1 year ago

Rebased and fixed merge conflicts.

nyabinary commented 1 year ago

Maybe that the FW16 has been opened for preorders this can be worked on again?

KarlK90 commented 12 months ago

@JohnAZoidberg could you remove the stale label, I still want to work on this.

github-actions[bot] commented 8 months ago

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[bot] commented 7 months ago

Thank you for your contribution! This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. // [stale-action-closed]