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

Allow multiple process_record() calls per scan #2022

Merged
merged 1 commit into from
Nov 21, 2017

Conversation

seebs
Copy link
Contributor

@seebs seebs commented Nov 18, 2017

This is particularly relevant for, e.g., the ergodox EZ and
other keyboards with slow scan rates. Without changing the API or
behavior of individual process_record() calls, we allow a
configuration flag to make multiple calls in a single scan.

This will probably have miniscule effects on non-steno users,
and it's not enabled by default for any keyboards except
the one where it was causing me trouble.

(Relevant to issue #1476)

Signed-off-by: seebs [email protected]

@seebs
Copy link
Contributor Author

seebs commented Nov 18, 2017

okay, this one has a humorous error too, note the "static" on keys_processed, that shouldn't be there. will resend.

@seebs seebs closed this Nov 18, 2017
@jackhumbert jackhumbert reopened this Nov 18, 2017
@@ -114,5 +114,6 @@ along with this program. If not, see <http://www.gnu.org/licenses/>.
//#define NO_ACTION_MACRO
//#define NO_ACTION_FUNCTION
//#define DEBUG_MATRIX_SCAN_RATE
#define QMK_KEYS_PER_SCAN 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we put this in a specific keymap and not in the overriding config.h? I am concerned of potential effects on our many users.

@ezuk
Copy link
Contributor

ezuk commented Nov 20, 2017

Just asked for it to be excluded from the default config.h and made available as an option.

@@ -123,6 +123,9 @@ If you define these options you will enable the associated feature, which may in
* how many taps before oneshot toggle is triggered
* `#define IGNORE_MOD_TAP_INTERRUPT`
* makes it possible to do rolling combos (zx) with keys that convert to other keys on hold
* `#define QMK_KEYS_PER_SCAN`
* Allows sending more than one key per scan. (By default, only one key event gets
sent via `process_record()` per scan.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate on the effects of this in actual use here? Like, what's it good for, specifically, from the standpoint of a layperson mashing keys?

@seebs
Copy link
Contributor Author

seebs commented Nov 20, 2017

If you're mashing a LOT of keys... not much. If you're mashing a ton of keys, and you have an LCD hooked up and being updated by the firmware on scan, and it does work only when there's changes, and that work takes time, it goes from "you can watch the key states change" to "basically instant".

But if you're a really fast typist, this could start showing up in cases where you type stupidly fast. Moreso without the related performance hacks, because the ergodox's scan rate is unusually slow. Basically, right now, the absolute theoretical maximum key-processing rate is about 150 key state changes per second. And you'd think that wouldn't matter much, but like, there's a guy on typeracer who has been filmed typing, and he really is able to type "kno" fast enough that the software does not detect even one millisecond between those key activations. on an ergodox, the theoretical minimum would be about 4-6ms, because only one key record can happen at a time, and the next one would wait for the next scan.

Impact is probably pretty minor for most use cases.

@jackhumbert
Copy link
Member

@seebs I think @ezuk meant to elaborate in the documentation :) if you wanna add some of your explanation there to that page, and move #define QMK_KEYS_PER_SCAN 8 your just personal keymap, we should be able to get this merged :)

@seebs
Copy link
Contributor Author

seebs commented Nov 20, 2017

That makes sense. The "need" for KEYS_PER_SCAN became less severe anyway with the other performance changes. And yeah, I can see some TINY advantages to the doc change being more complete.

This is particularly relevant for, e.g., the ergodox EZ and
other keyboards with slow scan rates. Without changing the API or
behavior of individual process_record() calls, we allow a
configuration flag to make multiple calls in a single scan.

This will probably have miniscule effects on non-steno users,
and it's not enabled by default for any keyboards. Added note
about it to ergodox README.

Signed-off-by: seebs <[email protected]>
@seebs
Copy link
Contributor Author

seebs commented Nov 20, 2017

Revised commit, repushed branch.

@jackhumbert
Copy link
Member

Perfect! Thanks :)

@jackhumbert jackhumbert merged commit 39d3d92 into qmk:master Nov 21, 2017
@drashna
Copy link
Member

drashna commented Mar 25, 2018

Should help improve issue in #910

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants