Retro Tapping Re-Write; Key Roll Fix

[JohnRigoni]

Description

The purpose of this PR is to fix the condition found with RETRO_TAPPING enabled when key rolling. The issue is that the dual function keys's retro tap keycode is not fired when a different key has been released between the dual function keys's down and up events. I am not sure if this is intended behavior or not, but does not seem to be given that retro tapping is supposed to apply if another key is not pressed. Also, none of the existing tests failed after the re-write.

I had to rewrite the logic, though the original idea is still intact. When considering key rolling from a dual function key to a dual function key, it was necessary to capture the previous modifier state and temporarily apply it to the tap code. There is a unit test for this.

This is still an early draft. I am brand new to QMK and am running the ZSA fork so I do not have the experience/ability to see this through without some assistance. I would be happy to hand this off to someone more qualified if appropriate.

⚠️ This change in current form does not consider RETRO_SHIFT nor AUTO_SHIFT_ENABLE. The ZSA fork does not seem to support RETRO_SHIFT so I am not able to test this. I also have not tested RETRO_TAPPING_PER_KEY, though that looks like it will function with no issue. It is not configured in Oryx and I have sparsely looked at keymap.c.

I ensured these changes work on my ZSA Voyager and that I could make all defaults. Thanks.

Types of Changes

• [x] Core
• [x] Bugfix
• [ ] New feature
• [x] Enhancement/optimization
• [ ] Keyboard (addition or update)
• [ ] Keymap/layout/userspace (addition or update)
• [ ] Documentation

Issues Fixed or Closed by This PR

#22916

Checklist

• [x] My code follows the code style of this project: C, Python
• [x] 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.
• [x] I have read the CONTRIBUTING document.
• [x] I have added tests to cover my changes.
• [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Media

Before Key Roll Fix: Single to Dual Function

After Key Roll Fix: Single to Dual Function

Before Key Roll Fix: Dual to Dual Function

After Key Roll Fix: Dual to Dual Function

[JohnRigoni]

Should I have based this branch on develop? I did not see any conflicts or other changes so I did not think anything of it.

[drashna]

Should I have based this branch on develop? I did not see any conflicts or other changes so I did not think anything of it.

Yes. New features, changes to core, etc should all target develop. And this definitely qualifies for that. Generally, only keyboard PRs should target master... as well as important bugfixes (which this doesn't exactly quality as).

⚠️ This change in current form does not consider RETRO_SHIFT nor AUTO_SHIFT_ENABLE. The ZSA fork does not seem to support RETRO_SHIFT so I am not able to test this. I also have not tested RETRO_TAPPING_PER_KEY, though that looks like it will function with no issue. It is not configured in Oryx and I have sparsely looked at keymap.c.

I believe that firmware 23 (eg that named branch) should include those features, and is what is compiling on oryx.

[JohnRigoni]

I moreso meant that my branch was initially based off master. I did not see any differences in the PR besides my code changes so I did not create a new branch based on develop.

I have tried enabling both RETRO_TAPPING_PER_KEY and RETRO_SHIFT in the config.h that Oryx generates but have not found success. Even when my LSP is showing me the code is defined.

Also, since I intend to add one more feature, Ill set up the CI locally to catch these simple errors I keep making.

[JohnRigoni]

It appears using set_mod() was leaving modifiers still active, hence last commit's test that ended in an unrelated keypress. It appears the only solution is to use oneshot modifiers, though it requires more lines of code.

Based on the Retro Shift tests at https://github.com/qmk/qmk_firmware/blob/8075003e6068d6f8fa5396972c2a17d39d14c584/tests/auto_shift/retro_shift/tap_hold_configurations/default_mod_tap/test_retro_shift.cpp#L136 and surrounding it, Retro Shift never has an instance where it is activated via keyroll. Thus, it has no need for the retro_tap mods. This also explains why there was no need to change any existing tests.

[github-actions[bot]]

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[IsaacElenbaas]

First off, great idea for a first PR. I think having roll cases working well for people is much more effective than them doing too many tapping term shenanigans.

Stale-bot brought me back to this and I took a closer look, two things I see to address:

1. You are saving and reapplying mods. TAPPING_TERM should still apply to rolls with Retro Tapping, and I am not sure if this respects that. If it does, that should be tested given the current implementation method. IGNORE_MOD_TAP_INTERRUPT is dead (long live HOLD_ON_OTHER_KEYPRESS) so this is out of date, but there may be a similar doc somewhere.
2. Also with saving and reapplying mods, I believe TAP_CODE_DELAY should be in there? The docs state that it is only for before the release of a programmatically-tapped key, but we were doing it between every modifier and key action in auto shift even before I touched it. (People are also having issues with that not working properly which I still need to address. . . no working keyboards right now though.) @drashna thoughts?

Re: RETRO_SHIFT, as long as its test cases are passing don't worry about it. I looked back over them recently and I think there was one combo of key types I realized I missed, but for the most part every behavior is tested there. It is also completely separate from retro tapping aside from using the same defines and them existing around each other in action / action_tapping.

[IsaacElenbaas]

I have tried enabling both RETRO_TAPPING_PER_KEY and RETRO_SHIFT in the config.h that Oryx generates but have not found success. Even when my LSP is showing me the code is defined.

Again, don't worry about RETRO_SHIFT for the most part, but if you want to poke at it and see if both features will combine flawlessly this is probably because the keys aren't enabled on Auto Shift's side. MT(MOD_LCTL, KC_A) is different from KC_A and so will need to be added to Auto Shift. By default everything is retro shifted without RETRO_TAPPING_PER_KEY defined (if it is auto shifted).

[JohnRigoni]

This should sort out both the concerns you raised.

1: This change does respect TAPPING_TERM and I have added tests for every permutation of key roll (finally got the hang of the tests).

2: I removed the use of one-shot-modifiers and registered/unregistered the mods manually. After the mods are registered and before they are unregistered, wait_ms(TAP_CODE_DELAY) is run. The code is sent via tap_code(), which has TAP_CODE_DELAY built in. That should respect the delay, and CAPSLOCK is not a modifier so it shouldn't be necessary to consider when waiting.

In regards to RETROSHIFT, since there is never an instance where it is activated by a key roll and the tests are passing, I am confident it is unaffected.

I am going to add one more test that will cover chaining 2 keyrolls.

[github-actions[bot]]

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[NikGovorov]

Hi @JohnRigoni,

Thank you for the fix! Would it be safe to apply your RETRO_TAPPING_TIMEOUT modifications on top of these changes?

diff --git a/quantum/action.c b/quantum/action.c
index a39631ba3e..8cada5ce40 100644
--- a/quantum/action.c
+++ b/quantum/action.c
@@ -47,7 +47,16 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 int tp_buttons;

 #if defined(RETRO_TAPPING) || defined(RETRO_TAPPING_PER_KEY) || (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
-int retro_tapping_counter = 0;
+bool     retro_tap_primed   = false;
+uint16_t retro_tap_curr_key = 0;
+#    if !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+uint8_t retro_tap_curr_mods = 0;
+uint8_t retro_tap_next_mods = 0;
+#    endif
+#    if defined(RETRO_TAPPING_TIMEOUT) && !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+uint16_t retro_last_press_time   = 0;
+uint16_t retro_last_release_time = 0;
+#    endif
 #endif

 #if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT) && !defined(NO_ACTION_TAPPING)
@@ -77,7 +86,19 @@ void action_exec(keyevent_t event) {
         debug_event(event);
         ac_dprintf("\n");
 #if defined(RETRO_TAPPING) || defined(RETRO_TAPPING_PER_KEY) || (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
-        retro_tapping_counter++;
+        uint16_t event_keycode = get_event_keycode(event, false);
+        if (event.pressed) {
+            retro_tap_primed   = false;
+            retro_tap_curr_key = event_keycode;
+#    if defined(RETRO_TAPPING_TIMEOUT) && !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+            retro_last_press_time = timer_read();
+#    endif
+        } else if (retro_tap_curr_key == event_keycode) {
+            retro_tap_primed = true;
+#    if defined(RETRO_TAPPING_TIMEOUT) && !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+            retro_last_release_time = timer_read();
+#    endif
+        }
 #endif
     }

@@ -531,7 +552,8 @@ void process_action(keyrecord_t *record, action_t action) {
 #    if defined(RETRO_TAPPING) && defined(DUMMY_MOD_NEUTRALIZER_KEYCODE)
                             // Send a dummy keycode to neutralize flashing modifiers
                             // if the key was held and then released with no interruptions.
-                            if (retro_tapping_counter == 2) {
+                            uint16_t ev_kc = get_event_keycode(event, false);
+                            if (retro_tap_primed && retro_tap_curr_key == ev_kc) {
                                 neutralize_flashing_modifiers(get_mods());
                             }
 #    endif
@@ -825,30 +847,47 @@ void process_action(keyrecord_t *record, action_t action) {

 #ifndef NO_ACTION_TAPPING
 #    if defined(RETRO_TAPPING) || defined(RETRO_TAPPING_PER_KEY) || (defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
-    if (!is_tap_action(action)) {
-        retro_tapping_counter = 0;
-    } else {
+    if (is_tap_action(action)) {
         if (event.pressed) {
             if (tap_count > 0) {
-                retro_tapping_counter = 0;
+                retro_tap_primed = false;
+            } else {
+#        if !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+                retro_tap_curr_mods = retro_tap_next_mods;
+                retro_tap_next_mods = get_mods();
+#        endif
             }
         } else {
+            uint16_t event_keycode = get_event_keycode(event, false);
+#        if !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+            uint8_t curr_mods = get_mods();
+#        endif
             if (tap_count > 0) {
-                retro_tapping_counter = 0;
-            } else {
+                retro_tap_primed = false;
+            } else if (retro_tap_curr_key == event_keycode) {
                 if (
 #        ifdef RETRO_TAPPING_PER_KEY
-                    get_retro_tapping(get_event_keycode(record->event, false), record) &&
+                    get_retro_tapping(event_keycode, record) &&
+#        endif
+#        if defined(RETRO_TAPPING_TIMEOUT) && !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+                    retro_last_release_time - retro_last_press_time < RETRO_TAPPING_TIMEOUT &&
 #        endif
-                    retro_tapping_counter == 2) {
+                    retro_tap_primed) {
 #        if defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT)
                     process_auto_shift(action.layer_tap.code, record);
 #        else
+                    register_mods(retro_tap_curr_mods);
+                    wait_ms(TAP_CODE_DELAY);
                     tap_code(action.layer_tap.code);
+                    wait_ms(TAP_CODE_DELAY);
+                    unregister_mods(retro_tap_curr_mods);
 #        endif
                 }
-                retro_tapping_counter = 0;
+                retro_tap_primed = false;
             }
+#        if !(defined(AUTO_SHIFT_ENABLE) && defined(RETRO_SHIFT))
+            retro_tap_next_mods = curr_mods;
+#        endif
         }
     }
 #    endif

It works for me, but I'm curious about any potential issues.

[JohnRigoni]

Hi @NikGovorov, glad you are finding the changes useful. There should be no issue running both together, I have been doing so since I made this PR! I plan to submit the RETRO_TAPPING_TIMEOUT feature once this bug fix is accepted, hopefully soon.

[NikGovorov]

Thanks! Hope the PR and the RETRO_TAPPING_TIMEOUT feature will be accepted by the team and merged to master soon.