dynamic_tapping_term: prevent DT_DOWN underflowing the tapping term#26337
Draft
arcaartem wants to merge 4 commits into
Draft
dynamic_tapping_term: prevent DT_DOWN underflowing the tapping term#26337arcaartem wants to merge 4 commits into
arcaartem wants to merge 4 commits into
Conversation
arcaartem
force-pushed
the
fix-dynamic-tapping-term-underflow
branch
from
July 16, 2026 12:04
8bd779b to
7502a31
Compare
Author
|
Rebased to stack on #26338 (new tests/dynamic_tapping_term suite) and added a regression test for the underflow: 100 DT_DOWN presses from the default term, asserting the term never exceeds its starting value and floors at the increment. Verified the test fails against the unguarded code (wraps to 65531) and passes with the guard. |
Add a test suite under tests/dynamic_tapping_term covering the DYNAMIC_TAPPING_TERM_ENABLE feature: the initial value of g_tapping_term matches the configured TAPPING_TERM, DT_UP raises it by DYNAMIC_TAPPING_TERM_INCREMENT per press, and DT_DOWN lowers it by the same amount per press. No prior test coverage existed for this feature. Give the fixture a destructor that restores g_tapping_term to TAPPING_TERM after every test, so InitialTappingTermMatchesConfiguredDefault does not depend on running before the other tests mutate the global tapping term. Verified order-independence by running the suite with --gtest_shuffle across many seeds.
arcaartem
force-pushed
the
fix-dynamic-tapping-term-underflow
branch
from
July 16, 2026 17:38
7502a31 to
e3817a1
Compare
Author
|
Rebased on the updated #26338. The regression test now derives its starting term as an exact multiple of DYNAMIC_TAPPING_TERM_INCREMENT so the floor assertions hold for any increment configuration, and the clamp is now documented in docs/keycodes.md and docs/tap_hold.md (DT_DOWN never lowers the tapping term below the increment). |
Co-authored-by: Joel Challis <git@zvecr.com>
…c term Exercise the feature end to end: the same hold duration resolves as a tap under the default tapping term, then as a hold after DT_DOWN lowers the term below it. This pins the GET_TAPPING_TERM -> g_tapping_term wiring that the arithmetic-only tests do not cover.
g_tapping_term is uint16_t. QK_DYNAMIC_TAPPING_TERM_DOWN unconditionally subtracted DYNAMIC_TAPPING_TERM_INCREMENT from it, so repeated presses once the term dropped below the increment would wrap it past 0 to ~65531, making tap-hold effectively unusable. Guard the subtraction so it only applies while it would not underflow. Adds a regression test that taps DT_DOWN 100 times from a derived starting term (an exact multiple of DYNAMIC_TAPPING_TERM_INCREMENT) and asserts g_tapping_term never exceeds that starting value and settles at the increment floor instead of wrapping, regardless of how the increment is configured. Also document the clamp: DT_DOWN never lowers the tapping term below DYNAMIC_TAPPING_TERM_INCREMENT, noted in docs/keycodes.md and docs/tap_hold.md next to the existing DT_DOWN description.
arcaartem
force-pushed
the
fix-dynamic-tapping-term-underflow
branch
from
July 17, 2026 09:41
e3817a1 to
db37d92
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
g_tapping_termis auint16_t.QK_DYNAMIC_TAPPING_TERM_DOWNunconditionally subtractedDYNAMIC_TAPPING_TERM_INCREMENTfrom it, so once the term dropped below the increment the next press would wrap it past zero to roughly 65531 ms. At that point every tap-hold key resolves as a tap and the keyboard is effectively unusable until the term is raised back step by step or the board is reset.Guard the subtraction so it only applies while it would not underflow.
DT_UPbehavior is unchanged.Includes a regression test in the
tests/dynamic_tapping_termsuite: tappingDT_DOWN100 times from a starting term derived as an exact multiple ofDYNAMIC_TAPPING_TERM_INCREMENTassertsg_tapping_termnever exceeds its starting value and settles at the increment floor instead of wrapping, valid for any increment configuration. The test was verified to fail against the unguarded code (term reaches 0, then wraps to 65531) and passes with the guard.Also documents the clamp: the
DT_DOWNentries indocs/keycodes.mdanddocs/tap_hold.mdnow note that the tapping term is never lowered below the increment.Stacked on #26338, which introduces the test suite; the first commit here belongs to that PR and will drop out of this diff once it merges.
Types of Changes
Checklist
qmk format-capplied, diff is noise-free.developbranch.make test:dynamic_tapping_term, 4/4 passing; regression test fails without the fix).crkbd/rev1:defaultwithDYNAMIC_TAPPING_TERM_ENABLE=yes, confirmingprocess_dynamic_tapping_term.cis built and linked.