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.

Sign up for GitHub

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

Jump to bottom

Improvements to handling of disconnected split keyboards. #14033

Merged
merged 3 commits into from
Sep 18, 2021
Merged

Improvements to handling of disconnected split keyboards. #14033

merged 3 commits into from
Sep 18, 2021

Conversation

firetech
Copy link
Contributor

@firetech firetech commented Aug 16, 2021 •
edited
Loading

Description

Continuing from #13523.

First, a slight cleanup of how the slave matrix is copied to the master matrix. Now using memcmp, memcpy and memset, as suggested by @KarlK90 in comments in the above mentioned PR.

Secondly, fixes an issue with matrix_post_scan signalling a matrix change on every scan cycle while disconnected, causing issues with (at least) OLED timeouts.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

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 Aug 16, 2021
@drashna
Copy link
Member

drashna commented Aug 17, 2021

Testing this out, there is about 5 scans dropped according to debug scan rate reporting. But that's 624 --> 619 for the averages. And about 12 bytes more used (but 6 of those are due to "-dirty" tag for the version). So I'd say that this the AVR impact is minimal?

(tested on my tractyl manuform using a teensy 2++)

@firetech
Copy link
Contributor Author

Huh, I'm quite sure it made the firmware smaller on ARM (Ergodox Infinity at least). 🤔

@drashna
Copy link
Member

drashna commented Aug 17, 2021

Huh, I'm quite sure it made the firmware smaller on ARM (Ergodox Infinity at least). 🤔

Nobody said that AVR was efficient. :D

@drashna
Copy link
Member

drashna commented Aug 17, 2021

Running into some issues on AVR with this, and locking up on startup/initialization.

@firetech
Copy link
Contributor Author

firetech commented Aug 17, 2021 •
edited
Loading

Running into some issues on AVR with this, and locking up on startup/initialization.

Not sure why that would happen... 😨

One could try with another way of specifying the length of matrix and raw_matrix to the memset calls (i.e. MATRIX_ROWS * sizeof(matrix_row_t) instead of sizeof(matrix)), but I have no way of testing if that helps. 😕

EDIT: Noticed one issue of unitialization that might be an issue, but probably not.

@firetech
Copy link
Contributor Author

firetech commented Aug 17, 2021 •
edited
Loading

I tried undoing a minor change I made earlier to keep firmware size down. This, however, made my Ergodox Infinity firmware 8 bytes larger (instead of 8 bytes smaller), and undid the improved scan rate for some reason (back to how it is without this PR now). I tried a couple of different methods and getting that +100 scan rate is very delicate. Probably some weird compiler optimization in play here.

Please check if this resolves issues on AVR, without serious effects to firmware size or scan rate.

During my testing of different implementations, I did get some initialization failures on my Ergodox Infinity as well. Not sure if it ever happened for the implementation I finally committed, but I only ever experienced it immediately after flashing. Just re-plugging the keyboard afterwards solved it every time, so it at least wasn't obviously broken init code. 🤷‍♂️

@drashna
Copy link
Member

drashna commented Aug 17, 2021

Yeah, it does seem to resolve the issue

firetech and others added 3 commits August 22, 2021 09:47
@firetech @KarlK90
Use memcmp and memcpy to compare and copy slave matrix.
2b55f6b 
...and memset to initialize `matrix` and `raw_matrix`.

Increased my scan rate (while connected) by ~100 (on Ergodox Infinity).
Effect on AVR is unknown.

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
@firetech
Fix matrix_post_scan signalling change on every scan while disconne…
7f8d2f0 
…cted.
@firetech
Undo removal of initialization of slave_matrix.
cf14b95 
This has the effect of increasing my Ergodox Infinity firmware size by 8
bytes instead of decreasing by 8 bytes, and lowers the scan rate while
connected back to the initial value before these changes, but _might_
solve some issues on AVR.
@firetech firetech force-pushed the split_disconnect_improvements branch from b564407 to cf14b95 Compare August 22, 2021 07:47
@firetech firetech marked this pull request as ready for review August 22, 2021 08:07
@drashna drashna requested a review from a team September 11, 2021 06:18
@tzarc tzarc merged commit 8130690 into qmk:develop Sep 18, 2021
cadusk pushed a commit to cadusk/qmk_firmware that referenced this pull request Sep 19, 2021
Merge remote-tracking branch 'qmk/develop' into develop
bb4a07d 
* qmk/develop: (24 commits)
  [Keyboard] Fix BT rules for dosa40rgb (qmk#14497)
  [Keyboard] Add Gentleman65 (qmk#12971)
  [Keyboard] Add new keyboard quick17 (qmk#13703)
  [Keyboard] Add superuser keyboard (qmk#13840)
  [Keyboard] Add dyz60 (qmk#13864)
  [Keymap] GMMK Pro keymap (qmk#14389)
  [Keyboard] Add support for Coarse60 (qmk#14416)
  [Keymap] Added Brazilian keymap for BM40RGB (qmk#14431)
  [Keyboard] Fix dosa40rgb compilation issues (qmk#14491)
  Improvements to handling of disconnected split keyboards. (qmk#14033)
  Add RGBW support to PWM and SPI drivers for ChibiOS (qmk#14327)
  [Keymap] Add 'j4ckofalltrades' keymap for sofle/rev1 (qmk#14446)
  [Keyboard] Add Dosa40RGB + dtisaac01 (qmk#14476)
  [Keymap] Major Updates to Personal Kyria Keymap (qmk#14485)
  fix link error for helix/rev3_5rows:five_rows (qmk#14466)
  Nix Studio OXALYS80 Refactor (qmk#14473)
  [Keyboard] Move Planck EZ off 'Proton C' board (qmk#14479)
  [Keyboard] Move Moonlander off 'Proton C' board (qmk#14478)
  Update rules.mk for xbows keyboard  (qmk#14477)
  [Keymap] Adding personal keymap (qmk#14326)
  ...
ptrxyz pushed a commit to ptrxyz/qmk_firmware that referenced this pull request Apr 9, 2022
@firetech @KarlK90 @ptrxyz
Improvements to handling of disconnected split keyboards. (qmk#14033)
de6de2f 
* Use memcmp and memcpy to compare and copy slave matrix.

...and memset to initialize `matrix` and `raw_matrix`.

Increased my scan rate (while connected) by ~100 (on Ergodox Infinity).
Effect on AVR is unknown.

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>

* Fix `matrix_post_scan` signalling change on every scan while disconnected.

* Undo removal of initialization of `slave_matrix`.

This has the effect of increasing my Ergodox Infinity firmware size by 8
bytes instead of decreasing by 8 bytes, and lowers the scan rate while
connected back to the initial value before these changes, but _might_
solve some issues on AVR.

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
@firetech firetech deleted the split_disconnect_improvements branch May 8, 2022 10:06
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@firetech @KarlK90
Improvements to handling of disconnected split keyboards. (qmk#14033)
8c5b7a9 
* Use memcmp and memcpy to compare and copy slave matrix.

...and memset to initialize `matrix` and `raw_matrix`.

Increased my scan rate (while connected) by ~100 (on Ergodox Infinity).
Effect on AVR is unknown.

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>

* Fix `matrix_post_scan` signalling change on every scan while disconnected.

* Undo removal of initialization of `slave_matrix`.

This has the effect of increasing my Ergodox Infinity firmware size by 8
bytes instead of decreasing by 8 bytes, and lowers the scan rate while
connected back to the initial value before these changes, but _might_
solve some issues on AVR.

Co-authored-by: Stefan Kerkmann <karlk90@pm.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tzarc tzarc tzarc approved these changes

@drashna drashna drashna approved these changes

Assignees
No one assigned
Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@firetech @drashna @tzarc