ahoneybun/qmk_firmware
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

[Bug][Core] Fix for Flow Tap: fix handling of distinct taps and timer updates. (#25175)

Browse source 
* Flow Tap bug fix.

As reported by @amarz45 and @mwpardue, there is a bug where if two
tap-hold keys are pressed in distinct taps back to back, then Flow Tap
is not applied on the second tap-hold key, but it should be.

In a related bug reported by @NikGovorov, if a tap-hold key is held
followed by a tap of a tap-hold key, then Flow Tap updates its timer on
the release of the held tap-hold key, but it should be ignored.

The problem common to both these bugs is that I incorrectly assumed
`tapping_key` is cleared to noevent once it is released, when actually
`tapping_key` is still maintained for `TAPPING_TERM` ms after release
(for Quick Tap). This commit fixes that. Thanks to @amarz45, @mwpardue,
and @NikGovorov for reporting!

Details:

* Logic for converting the current tap-hold event to a tap is extracted
  to `flow_tap_key_if_within_term()`, which is now invoked also in the
  post-release "interfered with other tap key" case. This fixes the
  distinct taps bug.

* The Flow Tap timer is now updated at the beginning of each call to
  `process_record()`, provided that there is no unsettled tap-hold key
  at that time and that the record is not for a mod or layer switch key.
  By moving this update logic to `process_record()`, it is conceptually
  simpler and more robust.

* Unit tests extended to cover the reported scenarios.

* Fix formatting.

* Revision to fix @NikGovorov's scenario.

The issue is that when another key is pressed while a layer-tap hasn't
been settled yet, the `prev_keycode` remembers the keycode from before
the layer switched. This can then enable Flow Tap for the following key
when it shouldn't, or vice versa.

Thanks to @NikGovorov for reporting!

This commit revises Flow Tap in the following ways:

* The previous key and timer are both updated from `process_record()`.
  This is slightly later in the sequence of processing than before, and
  by this point, a just-settled layer-tap should have taken effect so
  that the keycode from the correct layer is remembered.

* The Flow Tap previous key and timer are updated now also on key
  release events, except for releases of modifiers and held layer
  switches.

* The Flow Tap previous key and timer are now updated together, for
  simplicity. This makes the logic easier to think about.

* A few additional unit tests, including @NikGovorov's scenario as
  "layer_tap_ignored_with_disabled_key_complex."
This commit is contained in:
Pascal Getreuer 2025-04-22 00:59:49 -07:00 • committed by GitHub
parent b5f8f4d6a2
commit 73e2ef486a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 410 additions and 33 deletions
Download patch file Download diff file Expand all files Collapse all files

105
quantum/action_tapping.c
View file

@ -6,6 +6,7 @@
#include "action_tapping.h"
#include "action_util.h"
#include "keycode.h"
#include "quantum_keycodes.h"
#include "timer.h"
#ifndef NO_ACTION_TAPPING
@ -102,10 +103,10 @@ __attribute__((weak)) bool get_hold_on_other_key_press(uint16_t keycode, keyreco
# endif
# if defined(FLOW_TAP_TERM)
static uint32_t last_input = 0;
static uint16_t prev_keycode = KC_NO;
static uint32_t flow_tap_prev_time = 0;
static uint16_t flow_tap_prev_keycode = KC_NO;
uint16_t get_flow_tap_term(uint16_t keycode, keyrecord_t *record, uint16_t prev_keycode);
static bool flow_tap_key_if_within_term(keyrecord_t *record);
# endif // defined(FLOW_TAP_TERM)
static keyrecord_t tapping_key = {};
@ -157,19 +158,6 @@ void action_tapping_process(keyrecord_t record) {
}
}
if (IS_EVENT(record.event)) {
# if defined(FLOW_TAP_TERM)
const uint16_t keycode = get_record_keycode(&record, false);
// Track the previous key press.
if (record.event.pressed) {
prev_keycode = keycode;
}
// If there is no unsettled tap-hold key, update last input time. Ignore
// mod keys in this update to allow for chording multiple mods for
// hotkeys like "Ctrl+Shift+arrow".
if (IS_NOEVENT(tapping_key.event) && !IS_MODIFIER_KEYCODE(keycode)) {
last_input = timer_read32();
}
# endif // defined(FLOW_TAP_TERM)
ac_dprintf("\n");
}
}
@ -252,22 +240,8 @@ bool process_tapping(keyrecord_t *keyp) {
// into the "pressed" tapping key state
# if defined(FLOW_TAP_TERM)
const uint16_t keycode = get_record_keycode(keyp, false);
if (is_mt_or_lt(keycode)) {
const uint32_t idle_time = timer_elapsed32(last_input);
uint16_t term = get_flow_tap_term(keycode, keyp, prev_keycode);
if (term > 500) {
term = 500;
}
if (idle_time < 500 && idle_time < term) {
debug_event(keyp->event);
ac_dprintf(" within flow tap term (%u < %u) considered a tap\n", (int16_t)idle_time, term);
keyp->tap.count = 1;
registered_taps_add(keyp->event.key);
debug_registered_taps();
process_record(keyp);
return true;
}
if (flow_tap_key_if_within_term(keyp)) {
return true;
}
# endif // defined(FLOW_TAP_TERM)
@ -582,6 +556,13 @@ bool process_tapping(keyrecord_t *keyp) {
return true;
} else if (is_tap_record(keyp)) {
// Sequential tap can be interfered with other tap key.
# if defined(FLOW_TAP_TERM)
if (flow_tap_key_if_within_term(keyp)) {
tapping_key = (keyrecord_t){0};
debug_tapping_key();
return true;
}
# endif // defined(FLOW_TAP_TERM)
ac_dprintf("Tapping: Start with interfering other tap.\n");
tapping_key = *keyp;
waiting_buffer_scan_tap();
@ -809,6 +790,66 @@ static void waiting_buffer_process_regular(void) {
# endif // CHORDAL_HOLD
# ifdef FLOW_TAP_TERM
void flow_tap_update_last_event(keyrecord_t *record) {
// Don't update while a tap-hold key is unsettled.
if (waiting_buffer_tail != waiting_buffer_head || (tapping_key.event.pressed && tapping_key.tap.count == 0)) {
return;
}
const uint16_t keycode = get_record_keycode(record, false);
// Ignore releases of modifiers and held layer switches.
if (!record->event.pressed) {
switch (keycode) {
case MODIFIER_KEYCODE_RANGE:
case QK_MOMENTARY ... QK_MOMENTARY_MAX:
case QK_LAYER_TAP_TOGGLE ... QK_LAYER_TAP_TOGGLE_MAX:
# ifndef NO_ACTION_ONESHOT // Ignore one-shot keys.
case QK_ONE_SHOT_MOD ... QK_ONE_SHOT_MOD_MAX:
case QK_ONE_SHOT_LAYER ... QK_ONE_SHOT_LAYER_MAX:
# endif // NO_ACTION_ONESHOT
# ifdef TRI_LAYER_ENABLE // Ignore Tri Layer keys.
case QK_TRI_LAYER_LOWER:
case QK_TRI_LAYER_UPPER:
# endif // TRI_LAYER_ENABLE
return;
case QK_MODS ... QK_MODS_MAX:
if (QK_MODS_GET_BASIC_KEYCODE(keycode) == KC_NO) {
return;
}
break;
case QK_MOD_TAP ... QK_MOD_TAP_MAX:
case QK_LAYER_TAP ... QK_LAYER_TAP_MAX:
if (record->tap.count == 0) {
return;
}
break;
}
}
flow_tap_prev_keycode = keycode;
flow_tap_prev_time = timer_read32();
}
static bool flow_tap_key_if_within_term(keyrecord_t *record) {
const uint16_t keycode = get_record_keycode(record, false);
if (is_mt_or_lt(keycode)) {
const uint32_t idle_time = timer_elapsed32(flow_tap_prev_time);
uint16_t term = get_flow_tap_term(keycode, record, flow_tap_prev_keycode);
if (term > 500) {
term = 500;
}
if (idle_time < 500 && idle_time < term) {
debug_event(record->event);
ac_dprintf(" within flow tap term (%u < %u) considered a tap\n", (int16_t)idle_time, term);
record->tap.count = 1;
registered_taps_add(record->event.key);
debug_registered_taps();
process_record(record);
return true;
}
}
return false;
}
// By default, enable Flow Tap for the keys in the main alphas area and Space.
// This should work reasonably even if the layout is remapped on the host to an
// alt layout or international layout (e.g. Dvorak or AZERTY), where these same