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

[getreuer]

This fixes Flow Tap bugs reported by @amarz45, @mwpardue, and @NikGovorov in the discussion on #25125.

Description

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 in #25125 that 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.

Debugging notes

In case it helps any further troubleshooting, here is how to enable verbose debug logging about the core tap-hold logic:

• Enable console debug logging.

• In your config.h, add: #define ACTION_DEBUG

• Optionally, you may find it useful to add KEYCODE_STRING_ENABLE = yes to rules.mk and call my dlog_record() logging function from process_record_user(). This way, you also get logging of the events coming out of the core tap-hold logic.

An example log (rapidly pressing a regular key O followed by two tap-hold keys S, H):

---- action_exec: start -----
EVENT: 0703d(59572)
L0  ( 7, 3)      press   KC_O
ACTION: ACT_LMODS[0:12] layer_state: 00000000(0) default_layer_state: 00000001(0)
processed: 0703d(59572):0


---- action_exec: start -----
EVENT: 0202d(59599)
0202d(59599) within flow tap term (28 < 125) considered a tap
registered_taps = { 0202 }
L0  ( 2, 2) tap  press   LALT_T(KC_S)
layer_state: 00000000(0) to 00000000(0)
ACTION: ACT_LMODS_TAP[4:16] layer_state: 00000000(0) default_layer_state: 00000001(0)
MODS_TAP: Tap: register_code
processed: 0202d(59599):1


---- action_exec: start -----
EVENT: 0901d(59692)
0901d(59692) within flow tap term (92 < 100) considered a tap
registered_taps = { 0202 0901 }
L0  ( 9, 1) tap  press   RCTL_T(KC_H)
ACTION: ACT_RMODS_TAP[1:0B] layer_state: 00000000(0) default_layer_state: 00000001(0)
MODS_TAP: Tap: register_code
processed: 0901d(59692):1

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout (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).

[NikGovorov]

Unfortunately, the issue I reported earlier is still reproducible after applying the changes. I've enabled logging, but since QMK Toolbox's HID Console doesn't allow copying the output, I've attached images of the logs instead 🙂

[NikGovorov]

Here is some extra logging I've added to https://github.com/getreuer/qmk_firmware/blob/2bd3f6cfde1ae6ea59960a3f208c9ceabb8e6ca5/quantum/action_tapping.c#L801 and https://github.com/getreuer/qmk_firmware/blob/2bd3f6cfde1ae6ea59960a3f208c9ceabb8e6ca5/quantum/action_tapping.c#L867

As you can see, we update the timer on the release of KC_RIGHT, but we set RALT_T(KC_I) as the prev_keycode, which makes it a valid flow tap previous key.

[tzarc]

If you're building locally, qmk console should allow you to attach to the output without needing toolbox (and is faster too). You should be able to copy/paste from the terminal just fine.

[NikGovorov]

If you're building locally, qmk console should allow you to attach to the output without needing toolbox (and is faster too). You should be able to copy/paste from the terminal just fine.

Thanks! Unfortunately, the keyboard I'm currently testing is connected to a Windows machine(with no msys installed). I've submitted a pull request to fix the HID console functionality on Windows.

[getreuer]

Thank you @NikGovorov for your continued testing and description! I successfully repro'd this bug now.

In this sequence:

1. Q down;
2. Q up;
3. LT(NAV, KC_SPC) down;
4. KC_RIGHT down;
5. KC_RIGHT up;
6. LT(NAV, KC_SPC) up;
7. MT(MOD_LCTL, KC_T) down;
8. U down;
9. U up;
10. MT(MOD_LCTL, KC_T) up;

The occurrence of the bug depends on that KC_RIGHT is tapped before LT(NAV, KC_SPC) is settled. Furthermore, on layer 0, the key RALT_T(KC_I) in the same position as KC_RIGHT is one where Flow Tap is enabled. It's a particular set of conditions, this is how I missed it before! I appreciate that you continued to explain until I got it.

What's happening is that in steps 4 and 5, LT(NAV, KC_SPC) hasn't been settled yet, so when conceptually KC_RIGHT is tapped, it is received in action_tapping as a tap on RALT_T(KC_I). The keycode stored in prev_keycode is RALT_T(KC_I). With Permissive Hold enabled, LT(NAV, KC_SPC) is settled as KC_RIGHT is released, and shortly after prev_keycode is set (d'oh!).

I'll see what can be done to fix this. Hopefully moving the prev_keycode to occur slightly later does it. The challenge with layer-taps is that when a key is pressed following a yet-undecided layer-tap key, we don't yet know which layer that key press is on since that depends on how the layer-tap is settled. It puts the "quantum" into "Quantum Mechanical Keyboard" :-P

[mwpardue]

For my part the bug I reported appears to have been corrected. I see the flow_tap_term reported in debugging on back-to-back hold-tap keys, now, so I'm pretty sure it's going to work fine. I've been typing random text or running through typing tests on monkeytype.com for half an hour now and haven't seen the bug again.

I also can't seem to reproduce the issue that NikGovorov is having. I do have a default case in get_flow_tap_term() of 0 so only my home-row keys get any sort of FLOW_TAP_TERM at all; I'm not sure if that would make any difference.

Thank you, @getreuer, for tackling these bugs and for writing this feature to begin with. It's very much appreciated.

[NikGovorov]

@getreuer Thanks for continuing to look into the issue!

With Permissive Hold enabled, LT(NAV, KC_SPC) is settled as KC_RIGHT is released

I’m fairly certain the same behavior occurs when Permissive Hold is disabled, as long as this happens within the TAPPING_TERM.

[getreuer]

I've been typing random text or running through typing tests on monkeytype.com for half an hour now and haven't seen the bug again.

@mwpardue that sounds like progress! =) Thank you so much for your testing and feedback.

[getreuer]

@NikGovorov, I just pushed commit 0e4c30a with a unit test repro'ing the scenario you describe, which would fail before, and revision to Flow Tap to fix this.

Summary of the revision:

• 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 your scenario as "layer_tap_ignored_with_disabled_key_complex."

[NikGovorov]

@getreuer Just tested 0e4c30a, and it works perfectly on my end. Really appreciate your help—thanks a lot!

[getreuer]

@NikGovorov fantastic, that's wonderful to hear! And thank you for that lightning-fast follow up!

[amarz45]

I also tested this patch and I can confirm that the issue I reported is now fixed. Thanks @getreuer!

[getreuer]

@amarz45 thank you!

@drashna, @KarlK90, this fix is good to go! @amarz45, @mwpardue, @NikGovorov have tested and confirmed that the problems reported previously are fixed.

[KarlK90]

It puts the "quantum" into "Quantum Mechanical Keyboard" :-P

I had a good laugh. Thanks again for wading through the action_taping swamp and the swift fixes.

[getreuer]

@KarlK90 this is definitely a "no, thank you" moment =) Let me tell you, with how delicate and complicated action_tapping is, unit tests are absolutely vital to making changes to this area of QMK successfully. Chordal Hold and Flow Tap could not have happened without your effort to develop QMK's testing framework. So thank you!

[amarz45]

Hey @getreuer and others, thanks so much for your work on this. Shortly after this has been merged, I noticed that I’m oftentimes accidentally triggering keyboard shortcuts. I don’t know if it’s a bug or intentional behaviour, and it happens if I have PERMISSIVE_HOLD enabled as well as flow tap. Let’s say I have D and F as tap-hold keys. For the following key sequence:

1. D down
2. F down
3. D up (resolved as tap)

I would have expected F to resolve as a tap key immediately after D is released, but instead it’s still a tap-hold key. I believe this is what has caused me to accidentally trigger a few keyboard shortcuts, even though I type consistently at around 150 WPM and I’ve set my FLOW_TAP_TERM to 200. One keyboard shortcut that I keep accidentally triggering is Control + H. I’ve set my Space key to Meta and my F key to Control, and on Colemak the F key is used to type T. So an example of a key sequence that would cause me to accidentally trigger Control + H is a space followed by “th”:

1. Space down
2. F down
3. Space up
4. H down
5. H up
6. F up

The expected behaviour is to type th. The actual behaviour is Space gets typed, then Control + H is triggered. I’m not 100 % sure this is what’s causing me to accidentally trigger keyboard shortcuts, so I will investigate further.

[getreuer]

@amarz45 thanks for the report, and good question.

Let’s say I have D and F as tap-hold keys. For the following key sequence:

1. D down
2. F down
3. D up (resolved as tap)

I would have expected F to resolve as a tap key immediately after D is released, but instead it’s still a tap-hold key.

This is a "working as intended" case where Flow Tap does what logic I meant to implement. Though, it's totally fair to say an elaboration of that logic would better fulfill the intended idea.

As it is, Flow Tap potentially intervenes only on the key down event. In step 2, since D has not yet been settled at that moment, so Flow Tap does not act on F. The consideration is that the intention might be to hold both D and F for a Ctrl+Alt+key hotkey.

It's a great point that Flow Tap could act later than the key down event to address such a roll event. I'll take a shot at it.

Thinking about how to implement this, when the current tap-hold key is settled as tapped (on key up, or perhaps by Chordal Hold), it seems plausible to scan the waiting buffer for potentially a following yet-unsettled tap-hold key, then re-evaluate whether Flow Tap ought to act on it.

1. Space down
2. F down
3. Space up
4. H down
5. H up
6. F up

Space in this example is MT(MOD_LGUI, KC_SPC), correct? If so, the reason is the same as above: Space is not yet settled at the time that F is pressed, so Flow Tap does not act on it. Permissive Hold then settles it as held when the H key is released.

[amarz45]

Space in this example is MT(MOD_LGUI, KC_SPC), correct? If so, the reason is the same as above: Space is not yet settled at the time that F is pressed, so Flow Tap does not act on it. Permissive Hold then settles it as held when the H key is released.

Correct. For me, it makes the most sense if the behaviour is to immediately resolve a tap-hold key as tapped whenever the previous key is tapped within the FLOW_TAP_TERM, even in the case of a roll. Perhaps this behaviour should be customizable though. Because in this situation, if a tap-hold key that wasn’t pressed for the TAPPING_TERM was released, instead of that modifier being cancelled it will result in all the keys being tapped instead. However, I find that to rarely be an issue because when I’m chaining modifiers I will have usually held them for longer than the TAPPING_TERM anyway.

[NikGovorov]

Correct. For me, it makes the most sense if the behaviour is to immediately resolve a tap-hold key as tapped whenever the previous key is tapped within the FLOW_TAP_TERM, even in the case of a roll. Perhaps this behaviour should be customizable though. Because in this situation, if a tap-hold key that wasn’t pressed for the TAPPING_TERM was released, instead of that modifier being cancelled it will result in all the keys being tapped instead. However, I find that to rarely be an issue because when I’m chaining modifiers I will have usually held them for longer than the TAPPING_TERM anyway.

In your case, you might find that the original implementation by @filterpaper suits your workflow better.

You might also consider adding an option to toggle a layer without HRM, ideally with a simple keybinding or a timer to switch between them, for example:

[amarz45]

In your case, you might find that the original implementation by @filterpaper suits your workflow better.

I think it’s better as part of QMK’s core functionality rather than in user space.

You might also consider adding an option to toggle a layer without HRM, ideally with a simple keybinding to switch between them, for example [...]

I like the idea, but I still prefer home-row mods.

[filterpaper]

I think it’s better as part of QMK’s core functionality rather than in user space.

The Flow Tap feature will typically require some customisation with callback functions in the keymap.c file (user space). The pre_process_record_user is also a callback function exposed for customisation in the keymap.c file. Is the preference of having the final "solution" that works in "core" over "user space" important because of specific reasons?

[KarlK90]

@KarlK90 this is definitely a "no, thank you" moment =) Let me tell you, with how delicate and complicated action_tapping is, unit tests are absolutely vital to making changes to this area of QMK successfully. Chordal Hold and Flow Tap could not have happened without your effort to develop QMK's testing framework. So thank you!

Thanks and great to hear that :-). The effort to overhaul the unit tests initially came from my desire to untangle the spaghetti that action_tapping is and replace it with a better system - which never happened :D.

[amarz45]

The Flow Tap feature will typically require some customisation with callback functions in the keymap.c file (user space). The pre_process_record_user is also a callback function exposed for customisation in the keymap.c file. Is the preference of having the final "solution" that works in "core" over "user space" important because of specific reasons?

I used the user space solution before #25125 and this pull request was merged. Now that it is part of QMK’s core functionality, I think it makes sense to change the behaviour to the way I described for ease of use. However, if that’s too complex then I understand and am also willing to use the user space solution.

[getreuer]

Hi all, I have a draft PR for a Flow Tap enhancement in #25200. This addresses @amarz45's case, while yet preserving the ability to chord multiple mods without having to wait out the Flow Tap term.

If you have time over the next few days, please check out #25200 and share your feedback—good or bad. The window for new PRs in this breaking changes cycle closes soon, this Sunday on 2025-04-27. So quick feedback helps especially for a chance of inclusion in this cycle. All the feedback here has been immensely helpful. Thank you!