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

Avoid 8-bit timer overflows in debounce algorithms #12240

Merged
merged 6 commits into from
Jun 9, 2021

Conversation

nomis
Copy link

@nomis nomis commented Mar 14, 2021

Description

Add fast_timer_t that is 16-bit or 32-bit based on architecture

A 16-bit timer will overflow sooner but be faster to compare on AVR.

Avoid 8-bit timer overflows in debounce algorithms

Count down remaining elapsed time instead of trying to do 8-bit timer comparisons.
Add a "none" implementation that is automatically used if DEBOUNCE is 0 otherwise it will break the _pk/_pr count down.

Avoid unnecessary polling of the entire matrix in sym_eager_pk

The matrix only needs to be updated when a debounce timer expires.

Avoid unnecessary polling of the entire matrix in sym_eager_pr

The matrix only needs to be updated when a debounce timer expires.

The use of the "needed_update" variable is trying to do what "matrix_need_update" was added to fix but didn't work because it only applied when all keys finished debouncing.

Fix sym_defer_g timing inconsistency compared to other debounce algorithms

DEBOUNCE=5 should process the key after 5ms, not 6ms

Add debounce tests

Unit tests for all the debounce algorithms. I've only done some basic tests but more advanced scenarios could be added.

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 Mar 14, 2021
@nomis nomis force-pushed the debounce_timers branch 2 times, most recently from 5e1c296 to feb7cac Compare March 14, 2021 21:05
@nomis
Copy link
Author

nomis commented Mar 14, 2021

I also have https://github.com/nomis/qmk_firmware/tree/debounce_asym_eager_defer_pk to go on top of this.

The three *_pk implementations could be merged (*_pr is a bit too different) but it'd make them all less readable.

@tzarc tzarc requested a review from a team March 14, 2021 23:11
@nomis
Copy link
Author

nomis commented Mar 15, 2021

I've added unit tests for the debounce algorithms and modified the timing of sym_defer_g slightly to be consistent.

build_test.mk Show resolved Hide resolved
Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

LGTM, and awesome on the added tests!!

tmk_core/common/timer.h Outdated Show resolved Hide resolved
@zvecr
Copy link
Member

zvecr commented Mar 17, 2021

Also not a fan of the following paradigm,

#ifdef UNIT_TEST
    free(debounce_counters);
#endif

Test specific code in core. While i dont know the area in the greatest detail, i see no real why it couldnt be changed to free if already allocated.

@nomis
Copy link
Author

nomis commented Mar 17, 2021

Test specific code in core. While i dont know the area in the greatest detail, i see no real why it couldnt be changed to free if already allocated.

If I remove the #ifdef I'll be increasing the code size for something that has no purpose outside the test case.

I'll add a debounce_free() function to the API that only gets called from the tests.

tmk_core/common/chibios/timer.h Outdated Show resolved Hide resolved
A 16-bit timer will overflow sooner but be faster to compare on AVR.
Count down remaining elapsed time instead of trying to do 8-bit timer
comparisons.

Add a "none" implementation that is automatically used if DEBOUNCE is
0 otherwise it will break the _pk/_pr count down.
The matrix only needs to be updated when a debounce timer expires.
The matrix only needs to be updated when a debounce timer expires.

The use of the "needed_update" variable is trying to do what
"matrix_need_update" was added to fix but didn't work because it only
applied when all keys finished debouncing.
…ithms

DEBOUNCE=5 should process the key after 5ms, not 6ms
@nomis
Copy link
Author

nomis commented Mar 17, 2021

I've squashed my extra changes back into the original commits

@nomis nomis requested a review from zvecr April 25, 2021 09:32
@nomis nomis mentioned this pull request Apr 25, 2021
14 tasks
@drashna drashna requested a review from a team April 26, 2021 02:01
@tzarc tzarc merged commit b829a1d into qmk:develop Jun 9, 2021
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jul 30, 2021
* Add fast_timer_t that is 16-bit or 32-bit based on architecture

A 16-bit timer will overflow sooner but be faster to compare on AVR.

* Avoid 8-bit timer overflows in debounce algorithms

Count down remaining elapsed time instead of trying to do 8-bit timer
comparisons.

Add a "none" implementation that is automatically used if DEBOUNCE is
0 otherwise it will break the _pk/_pr count down.

* Avoid unnecessary polling of the entire matrix in sym_eager_pk

The matrix only needs to be updated when a debounce timer expires.

* Avoid unnecessary polling of the entire matrix in sym_eager_pr

The matrix only needs to be updated when a debounce timer expires.

The use of the "needed_update" variable is trying to do what
"matrix_need_update" was added to fix but didn't work because it only
applied when all keys finished debouncing.

* Fix sym_defer_g timing inconsistency compared to other debounce algorithms

DEBOUNCE=5 should process the key after 5ms, not 6ms

* Add debounce tests
mechlovin pushed a commit to mechlovin/qmk_firmware that referenced this pull request Jul 30, 2021
* Add fast_timer_t that is 16-bit or 32-bit based on architecture

A 16-bit timer will overflow sooner but be faster to compare on AVR.

* Avoid 8-bit timer overflows in debounce algorithms

Count down remaining elapsed time instead of trying to do 8-bit timer
comparisons.

Add a "none" implementation that is automatically used if DEBOUNCE is
0 otherwise it will break the _pk/_pr count down.

* Avoid unnecessary polling of the entire matrix in sym_eager_pk

The matrix only needs to be updated when a debounce timer expires.

* Avoid unnecessary polling of the entire matrix in sym_eager_pr

The matrix only needs to be updated when a debounce timer expires.

The use of the "needed_update" variable is trying to do what
"matrix_need_update" was added to fix but didn't work because it only
applied when all keys finished debouncing.

* Fix sym_defer_g timing inconsistency compared to other debounce algorithms

DEBOUNCE=5 should process the key after 5ms, not 6ms

* Add debounce tests
@shelaf shelaf mentioned this pull request Aug 19, 2021
14 tasks
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
* Add fast_timer_t that is 16-bit or 32-bit based on architecture

A 16-bit timer will overflow sooner but be faster to compare on AVR.

* Avoid 8-bit timer overflows in debounce algorithms

Count down remaining elapsed time instead of trying to do 8-bit timer
comparisons.

Add a "none" implementation that is automatically used if DEBOUNCE is
0 otherwise it will break the _pk/_pr count down.

* Avoid unnecessary polling of the entire matrix in sym_eager_pk

The matrix only needs to be updated when a debounce timer expires.

* Avoid unnecessary polling of the entire matrix in sym_eager_pr

The matrix only needs to be updated when a debounce timer expires.

The use of the "needed_update" variable is trying to do what
"matrix_need_update" was added to fix but didn't work because it only
applied when all keys finished debouncing.

* Fix sym_defer_g timing inconsistency compared to other debounce algorithms

DEBOUNCE=5 should process the key after 5ms, not 6ms

* Add debounce tests
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Add fast_timer_t that is 16-bit or 32-bit based on architecture

A 16-bit timer will overflow sooner but be faster to compare on AVR.

* Avoid 8-bit timer overflows in debounce algorithms

Count down remaining elapsed time instead of trying to do 8-bit timer
comparisons.

Add a "none" implementation that is automatically used if DEBOUNCE is
0 otherwise it will break the _pk/_pr count down.

* Avoid unnecessary polling of the entire matrix in sym_eager_pk

The matrix only needs to be updated when a debounce timer expires.

* Avoid unnecessary polling of the entire matrix in sym_eager_pr

The matrix only needs to be updated when a debounce timer expires.

The use of the "needed_update" variable is trying to do what
"matrix_need_update" was added to fix but didn't work because it only
applied when all keys finished debouncing.

* Fix sym_defer_g timing inconsistency compared to other debounce algorithms

DEBOUNCE=5 should process the key after 5ms, not 6ms

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

Successfully merging this pull request may close these issues.

4 participants