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

Fix keyboard matrix scan rate with F072 #10226

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Fix keyboard matrix scan rate with F072 #10226

merged 3 commits into from
Nov 2, 2020

Conversation

Xelus22
Copy link
Contributor

@Xelus22 Xelus22 commented Sep 3, 2020

Description

Change matrix io delay to use nops. They have been tested working.

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

  • Fix 500 scan rate

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).

@Xelus22 Xelus22 changed the title Fix matrix scan rate with F072 Fix keyboard matrix scan rate with F072 Sep 3, 2020
@zvecr
Copy link
Member

zvecr commented Sep 5, 2020

This is "tested it still types" not "validated the gpio is in expected state with an oscilloscope", right?

@Xelus22
Copy link
Contributor Author

Xelus22 commented Sep 6, 2020

This is "tested it types" and I have given it to multiple users and have not had any complaints.

@drashna drashna requested review from drashna and a team September 7, 2020 03:13
@zvecr
Copy link
Member

zvecr commented Sep 23, 2020

I guess what I'm getting at is, I'm not a massive fan of the change and how it might propagate through the repo. And if changing the base config files for system timer config gives some improvement (but not quite the same) to scan rate as well as general microsecond accuracy, would rather see that. An issue of longer term maintainability.

That aside, the change itself looks fine.

@zvecr zvecr requested a review from tzarc September 23, 2020 02:04
@zvecr zvecr mentioned this pull request Oct 2, 2020
Merged
14 tasks
@tzarc
Copy link
Member

tzarc commented Oct 31, 2020

Maybe throw a comment in matrix_io_delay() saying something like

// Tested and verified working on the Xxxx keyboard

...just so when the inevitable copypasta occurs we can challenge subsequent PRs as far as validity is concerned. I know for a fact that this is too fast on the Djinn as it runs at 160MHz and electrically things don't "settle" after 3 nops -- others will likely be in the same boat in the future.

The DMA remap is a prime example of @zvecr's reluctance... people put it in without understanding the implications... the I/O delay will cause the same confusion without an explanation as to why it exists.

@Xelus22 Xelus22 mentioned this pull request Nov 1, 2020
Merged
14 tasks
@drashna drashna merged commit c148162 into qmk:master Nov 2, 2020
oscarcarlsson pushed a commit to oscarcarlsson/qmk_firmware that referenced this pull request Nov 2, 2020
@Xelus22
[Keyboard] Fix keyboard matrix scan rate with F072 (qmk#10226)
98664ef 
* fix matrix scan rate

* Update trinityxttkl.c

* Update rev2.c
ringmaster pushed a commit to ringmaster/qmk_firmware that referenced this pull request Nov 5, 2020
@Xelus22
[Keyboard] Fix keyboard matrix scan rate with F072 (qmk#10226)
1ff8428 
* fix matrix scan rate

* Update trinityxttkl.c

* Update rev2.c
barrettclark pushed a commit to barrettclark/qmk_firmware that referenced this pull request Nov 5, 2020
Merge remote-tracking branch 'upstream/master'
727ebc8 
* upstream/master: (153 commits)
  [Keymap] add brandonschlack userspace and keymaps (qmk#10411)
  [Keymap] add ai03/polaris:mekberg (qmk#10508)
  CLI: Add `qmk clean` (qmk#10785)
  Adds support for XD84 Pro (qmk#9750)
  Freyr refactor (qmk#10833)
  KC60 refactor (qmk#10834)
  [Keyboard] Fixes for PloopyCo mouse and readmes (qmk#10841)
  Enable extrakeys, mousekeys for all VIA keymaps. (qmk#10740)
  Add OLED support for Riblee F411 (qmk#10778)
  NK65 eeprom compatibility with 128KB and 256KB (qmk#10804)
  Add support for Noxary Vulcan (qmk#10822)
  Enable media keys support for Canoe VIA keymap (qmk#10829)
  Phantom refactor (qmk#10805)
  `qmk info`: Add `--ascii` flag (qmk#10793)
  [Keymap] Corrected the dvorak layout for kinesis advantage (qmk#10808)
  [Keyboard] Fix keyboard matrix scan rate with F072 (qmk#10226)
  [Keyboard] nullbitsco/nibble Configurator rework (qmk#10814)
  [Keyboard] VIA Support: Exent 65% (qmk#10797)
  [Keyboard] Add keebsforall/freebird60 (qmk#10774)
  add 65_ansi_split_bs to default community layouts (qmk#10770)
  ...
@Xelus22 Xelus22 deleted the scanrate_fix_f072 branch November 13, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drashna drashna drashna approved these changes

@tzarc tzarc tzarc approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@Xelus22 @zvecr @tzarc @drashna