Skip to content

Fix OSL clearing key being released twice (#26309)#26323

Open
94xhn wants to merge 1 commit into
qmk:masterfrom
94xhn:fix/osl-double-release-event
Open

Fix OSL clearing key being released twice (#26309)#26323
94xhn wants to merge 1 commit into
qmk:masterfrom
94xhn:fix/osl-double-release-event

Conversation

@94xhn

@94xhn 94xhn commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #26309 — a physically-held key that is released while still on a one-shot layer (OSL) gets two key-release events sent instead of one: one synthetic release fired the moment the OSL is left (while the key is still physically down), and a second real release when the user actually lifts the key.

Root cause

process_action() in quantum/action.c has contained this block since one-shot layers were first added to TMK/QMK (#308, 2016):

if (do_release_oneshot && !(get_oneshot_layer_state() & ONESHOT_PRESSED)) {
    record->event.pressed = false;
    layer_on(get_oneshot_layer());
    process_record(record);   // synthesizes a "release" while the key is still physically held
    layer_off(get_oneshot_layer());
}

At the time this was written, a key release was always resolved against whatever layer was currently active, so this synthetic replay was necessary to make sure a key that had been pressed on the one-shot layer was released with the same keycode before the one-shot layer disappeared.

That assumption stopped being true once quantum/action_layer.c gained the "source layers cache" (store_or_get_action() / update_source_layers_cache() / read_source_layers_cache(), guarded by #if !defined(NO_ACTION_LAYER) && !defined(STRICT_LAYER_RELEASE)): a key's layer is now snapshotted at press-time and re-read from that cache at release-time, regardless of any layer churn that happens in between. This makes the old "replay a release before leaving the one-shot layer" logic redundant — and actively harmful, because it fires an extra release while the key is still physically down, so the eventual real release becomes a second, spurious release event for the same keycode.

QMK contributor sigprof diagnosed the same root cause in a comment on #26309 and suggested gating the block behind STRICT_LAYER_RELEASE (the flag that already exists specifically to opt back into "resolve release against the current layer stack" for keyboards/users that don't want the source-layers cache).

Fix

quantum/action.c:

  • Tighten the existing #ifndef NO_ACTION_ONESHOT guard around the block to #if !defined(NO_ACTION_ONESHOT) && defined(STRICT_LAYER_RELEASE), so the synthetic replay only runs when the source-layers cache is bypassed (i.e. STRICT_LAYER_RELEASE is defined) — exactly the condition under which the replay is still needed.
  • Mark the do_release_oneshot local with __attribute__((unused)) (same pattern already used in quantum/keyboard.c) since it's only read inside the now-conditional block, to avoid -Werror=unused-but-set-variable when STRICT_LAYER_RELEASE is undefined.

No keyboard in-tree currently defines STRICT_LAYER_RELEASE, so this change is a pure behavior fix for the default configuration and cannot regress any existing keyboard's config.h.

Tests:

  • New tests/basic/test_one_shot_layer_release_event.cpp — a focused regression test that hooks process_record_user() to count press/release events and asserts exactly one release is produced for a key held across an OSL activation/expiry.
  • Updated 6 existing assertions across tests/basic/test_one_shot_keys.cpp and tests/tap_hold_configurations/{default_mod_tap,chordal_hold/default,chordal_hold/permissive_hold,chordal_hold/permissive_hold_flow_tap}/test_one_shot_*.cpp that had encoded the old double-release behavior as the expected HID report sequence (an EXPECT_REPORT/empty-report pair immediately at press-time, then no report at all on the real release). These now assert the correct single-release sequence.

Verification

All builds/tests below were run against the real, unmodified QMK C/C++ sources (quantum/action.c, action_layer.c, action_util.c, etc.) compiled with the repo's own host-test toolchain (g++/gcc + googletest/gmock via lib/googletest), in WSL Ubuntu 24.04. No stubs were substituted for the code under test.

  1. Before the fix (quantum/action.c reverted to the parent commit, test files kept at the new/updated versions): the new regression test fails, reproducing the bug directly — the key is registered as released (release_count increments) the moment the OSL is cleared, while it is still physically held, and again on the real key-up, i.e. 2 releases for 1 press. The 4 pre-existing OSL tests that had encoded the old behavior (OSLWithAdditionalKeypress, OSLWithOsmAndAdditionalKeypress, OSLChainingTwoOSLsAndAdditionalKeypress, OSLWithLongModTapKeyAndRegularKey) also fail against their updated (fixed-behavior) assertions, as expected.

  2. After the fix: full matrix, all real builds/runs, 0 failures:

    Test group Result
    tests/basic 62/62 passed (+5 unrelated SKIPPED)
    tests/tap_hold_configurations/default_mod_tap 12/12 passed
    tests/tap_hold_configurations/permissive_hold (non-chordal) 11/11 passed
    tests/tap_hold_configurations/chordal_hold/default 26/26 passed
    tests/tap_hold_configurations/chordal_hold/permissive_hold 40/40 passed
    tests/tap_hold_configurations/chordal_hold/permissive_hold_flow_tap 40/40 passed
    tests/no_tapping/no_action_tapping 8/8 passed
    tests/layer_lock + tests/caps_word 47/47 passed
    tests/keycode_string 1/1 passed

    9 test groups, 247 tests, 0 failures.

  3. qmk format-c -n on all changed files exits 0 with no diff.

  4. Diff stat vs. the branch point: 7 files changed, 121 insertions(+), 22 deletions(-); quantum/action.c itself is a 14-line change (12 insertions, 2 deletions).

Disclosure

This investigation, the fix, and the accompanying tests were carried out with the assistance of Claude (Anthropic AI), used as a coding tool under my direction. I independently rebuilt and ran the real QMK host-test suite before and after the change (described above) to verify the behavior difference, reviewed the diff, and take responsibility for this change.

Pressing a regular (non-modifier) key while a one-shot layer (OSL) is
armed makes that press clear the OSL. process_action() (quantum/action.c)
then immediately replays a synthetic release of that same key, right
before turning the one-shot layer off, so that "no key up event will be
generated" once the layer is gone. That replay unregisters/reports the
key as released while it is still physically held down. When the key is
later actually released, process_record() runs for it a second time and
reports it as released again.

The result is one press event followed by two release events for the
same physical keypress, visible to process_record_user(), to dynamic
macros, combos, or anything else that tracks press/release parity, and
even in the raw HID report: a regular key pressed right after an OSL is
reported as an instant tap (press immediately followed by release) no
matter how long it is actually held, instead of staying registered until
the physical release.

This replay predates the "source layers cache" added later
(quantum/action_layer.c: store_or_get_action(), update_source_layers_cache(),
read_source_layers_cache()), which now records the layer that was active
when a key was pressed and reuses it to resolve that key's release action,
regardless of layer changes in between. Because of this cache, the real,
later physical release of the key already resolves correctly against the
one-shot layer on its own. The replay is therefore only still needed when
STRICT_LAYER_RELEASE is defined, which is the one configuration where
store_or_get_action() intentionally bypasses the cache and instead
resolves actions from the live/current layer state.

Gate the replay in process_action() on STRICT_LAYER_RELEASE to match the
condition under which store_or_get_action() bypasses the source layers
cache. do_release_oneshot is written unconditionally by earlier one-shot
handling in the same function, so mark it as possibly unused (matching
the existing convention in quantum/keyboard.c) to keep -Werror builds
clean when STRICT_LAYER_RELEASE is not defined.

Testing:
- Added tests/basic/test_one_shot_layer_release_event.cpp, a new
  regression test that hooks process_record_user() and asserts exactly
  one press event followed by exactly one release event for a regular
  key that clears an active OSL. Verified it fails against the
  pre-fix code (observes press_count=1, release_count=0 immediately
  after the physical press, i.e. no premature release) and passes
  after the fix.
- Updated 4 existing tests in tests/basic/test_one_shot_keys.cpp
  (OSLWithAdditionalKeypress, OSLWithOsmAndAdditionalKeypress,
  OSLChainingTwoOSLsAndAdditionalKeypress,
  OSLWithLongModTapKeyAndRegularKey) whose expectations had encoded the
  old, buggy behavior (an immediate empty HID report right after the
  key press, then no report at all on the real release). Their new
  expectations were verified against the actual post-fix HID report
  trace produced by the test binary.
- Ran the full `make test:basic` suite: 62 passed, 5 pre-existing
  unrelated skips, 0 failures.

Fixes qmk#26309

Disclosure: This investigation, the fix, and the accompanying tests were
carried out with the assistance of Claude (Anthropic AI), used as a
coding tool under my direction. I have reviewed the diff and the test
output referenced above and take responsibility for this change.

Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
@github-actions github-actions Bot added the core label Jul 12, 2026
@drashna
drashna requested a review from a team July 12, 2026 16:23
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.

[Bug] OSL sends key release of key on oneshot layer twice

2 participants