Double tapping an OSM mod can cause another OSM mod to get stuck

[sypl]

Setup

I have two OSM modifiers:

#define OSMLSFT OSM(MOD_LSFT)
#define OSMLALT OSM(MOD_LALT)

Actions

I perform the following sequence, quickly:

• Tap OSMLSFT
• Tap OSMLALT
• Tap OSMLALT

Result

OSMLSFT is now stuck, all taps to alphanumeric part of keyboard output capital letters.

To clear I have to hold down OSMLSFT and hit another key.

Cause

Unclear. Double tapping a one shot mod should clear all mods, as this part of the code presumably does: https://github.com/qmk/qmk_firmware/blob/master/tmk_core/common/action.c#L290-L293

Debug

This is debug output of the actions mentioned above:

 > 
    ---- action_exec: start -----
    EVENT: 0301d(6277)
    Tapping: Start(Press tap key).
    TAPPING_KEY=0301d(6277):0 
    processed: 0301d(6277):0 

  > 
    ---- action_exec: start -----
    EVENT: 0301u(6373)
    Tapping: First tap(0->1).
    TAPPING_KEY=0301d(6277):1 
    ACTION: ACT_LMODS_TAP[2:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
    MODS_TAP: Oneshot: start
    waiting_buffer_enq: { [0]=0301u(6373):1  }
    ---- action_exec: process waiting_buffer -----
    Tapping: Tap release(1)
    ACTION: ACT_LMODS_TAP[2:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
    TAPPING_KEY=0301u(6373):1 
    processed: waiting_buffer[0] = 0301u(6373):1 

  > 
    ---- action_exec: start -----
    EVENT: 0302d(6453)
    Tapping: Start with interfering other tap.
    TAPPING_KEY=0302d(6453):0 
    processed: 0302d(6453):0 

  > 
    ---- action_exec: start -----
    EVENT: 0302u(6529)
    Tapping: First tap(0->1).
    TAPPING_KEY=0302d(6453):1 
    ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
    MODS_TAP: Oneshot: start
    waiting_buffer_enq: { [1]=0302u(6529):1  }
    ---- action_exec: process waiting_buffer -----
    Tapping: Tap release(1)
    ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
    TAPPING_KEY=0302u(6529):1 
    processed: waiting_buffer[1] = 0302u(6529):1 

  > 
    ---- action_exec: start -----
    EVENT: 0302d(6605)
    Tapping: Tap press(2)
    ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
  > TAPPING_KEY=0302d(6605):2 
    processed: 0302d(6605):2 

  > 
    ---- action_exec: start -----
    EVENT: 0302u(6699)
    Tapping: Tap release(2)
    ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
    TAPPING_KEY=0302u(6699):2 
    processed: 0302u(6699):2 

  > Tapping: End(Timeout after releasing last tap): FFFFu(6899)
    TAPPING_KEY=0000u(0):0 

[drashna]

It looks like it's triggering the tap toggle code, prematurely.

With a key tester, it becomes apparent, since it's holding "shift" down, which is what it does when tap toggle is enabled. Also, this only happens if the tapping is very fast (all within the tapping term).

(also, you should only need to tap the key once to release it, no need to hold it.

[mtourne]

I think I'm experiencing the same thing

OSMLCTL and then holding down OSMLSFT causes OSMLCTL to get stuck

whereas tapping OSMLCTL + holding OSMLCTL doesn't even it cause it to stick, so the behavior is weird

my ONESHOT_TAP_TOGGLE is set to 2, so OSMLCTL + OSMLCTL rapidly does get it to stick (expected)

[mcp292]

My shift locks with the following two sequences. The only way to disable is to hold shift past the tapping term.

Tap Shift Hold Numpad Layer Hold OS (Linux Meta) Tap Numpad Number Now shift should be locked for all numbers and even letters when you exit the layer. Hold shift to deactivate.

Tap Shift Tap Ctrl Hold OS Locked.

Linux Planck EZ Layout: https://configure.ergodox-ez.com/planck-ez/layouts/Oaj3d/latest/0

[josephemorgan]

I've been having this issue for the past year, and this is the first time I've found someone else describing it. I use i3wm, and the key combination that I have to toggle a window from floataing to tiled is: GUI+Shift+Space

Every single time I do so, GUI gets stuck on until I tap GUI rapidly a few times, or call clear_keyboard().

Anyone have any solutions to this?

[mcp292]

Found another sequence that triggers the lock.

Tap Ctrl Hold OS

Ctrl is now locked.

To unlock hit Ctrl with an alphanumeric (non-mod) key: Ctrl-c.

Update

This no longer triggers it as of pulling in the latest commits. Now at 5dc7951dc0. Before this I was at 361ac2f32. While skimming through I saw something about crkbd mod keys (027570a21; may be unrelated).

If you lock when switching layers, and you don't need the mod in the active layer, consider this:

layer_state_t layer_state_set_user(layer_state_t state)
{
     switch (get_highest_layer(state))
     {
     case BASE:
     case MOUSE:
         clear_oneshot_mods();
         break;
     }

   return state;
}

Second Update

This sequence stills locks Ctrl: Tap Ctrl Hold OS

Another way to unlock is to hold Ctrl.

Not sure why I wasn't experiencing this after pulling changes. Actually, since then I have switched from a manually maintained repo, to the AUR package.

After checking for updates and reflashing I can confirm I still experience this issue [6.12.21].

My current workaround is to run clear_mods() after ESC presses:

bool process_record_user(uint16_t keycode, keyrecord_t *record)
{
    switch (keycode)
    {
    case KC_ESC:
        if (!record->event.pressed)
        {
            /* on key up */
            clear_mods();
        }

        return true;            /* process keycode as usual */
    }

    return true;
}

[daliusd]

Yet another workaround is to use callum-mod user space one-shot keys implementation: https://github.com/callum-oakley/qmk_firmware/tree/master/users/callum

IMHO, it even works better than current QMK one.

[precondition]

As someone who uses a one-shot left shift and a one-shot right shift, here's a possible workaround you can do in process_record_user :

    case OSM(MOD_LSFT):
      if (record->event.pressed && record->tap.count == 0 && (get_oneshot_mods() & MOD_BIT(KC_RSFT))) {
        del_oneshot_mods(MOD_BIT(KC_RSFT));
      }
      return true;

    case OSM(MOD_RSFT):
      if (record->event.pressed && record->tap.count == 0 && (get_oneshot_mods() & MOD_BIT(KC_LSFT))) {
        del_oneshot_mods(MOD_BIT(KC_LSFT));
      }
      return true;

This can easily be adapted to other one-shot modifiers.

[geaz]

At least the issue mentioned by @mcp292 is related to line 348 of the action.c

register_mods(mods | get_oneshot_mods());

The problem is, that the first tapped mod is added here as a real mod and is not cleared after releasing the holded modifier. Changing the line to register_mods(mods); fixes the stuck modifier, but removes the ability to trigger the chained modifiers multpile times. Maybe someone more into the code knows a better way to fix it.

[mcp292]

@geaz Nice digging!

[iandunn]

@drashna , do you mind doing a brain dump on how you'd approach fixing this? I can try to submit a PR, but I'm new to QMK so it'd help to be pointed in the right direction.

Which key tester do you recommend?

I use OSMs a lot because they really help my RSI, but I run into this bug a dozen times a day and it's really frustrating.

[precondition]

@iandunn I've heard that Callum's one-shot mods implementation (see qmk_firmware/users/callum) may be free of this bug, have you tried giving them a shot?

[iandunn]

I haven't yet, but have been thinking about it. It'd be awesome to fix the issue for everyone, but that might be a more practical approach given my unfamiliarity w/ QMK 👍🏻

[daliusd]

Callum has one little bug (not sure if it is fixed already) - still much better than QMK implementation. You might want to check this for better/improved Callum one-shots https://blog.ffff.lt/posts/callum-layers/ or even better this https://github.com/qmk/qmk_firmware/pull/16174

[iandunn]

Thanks!

[mcp292]

@daliusd Thanks for sharing! Interesting layout!

[iandunn]

I've run into a few quirks with Callum's implementation, but overall it's been working well for me 👍🏻

My keymap.c, in case that's a helpful reference for anyone else looking to use Callum's implementation.

[iandunn]

After using Callum's OSM implementation for awhile, I found too many quirks that I didn't like, and switched back to the native one. I tried out @geaz's workaround and it's been working well for me so far. I had to modify both of the lines, though:

diff --git a/quantum/action.c b/quantum/action.c
index 5e81efb671..8cb5e6e6c5 100644
--- a/quantum/action.c
+++ b/quantum/action.c
@@ -345,7 +345,7 @@ void process_action(keyrecord_t *record, action_t action) {
                         if (event.pressed) {
                             if (tap_count == 0) {
                                 dprint("MODS_TAP: Oneshot: 0\n");
-                                register_mods(mods | get_oneshot_mods());
+                                register_mods(mods);
                             } else if (tap_count == 1) {
                                 dprint("MODS_TAP: Oneshot: start\n");
                                 set_oneshot_mods(mods | get_oneshot_mods());
@@ -357,7 +357,7 @@ void process_action(keyrecord_t *record, action_t action) {
                                 register_mods(mods);
 #        endif
                             } else {
-                                register_mods(mods | get_oneshot_mods());
+                                register_mods(mods);
                             }
                         } else {
                             if (tap_count == 0) {

[jerviscui]

Is it possible to solve this by the oneshot_mods_changed_user() method?

[kasimir-k]

Here is the reason for this bug, but the fix depends on the intention here, so I'll leave any PRs for now.

This code from /quantum/action.c line 433.

I'm leaving out the ONESHOT_TAP_TOGGLE things as they are not relevant for this discussion, and adding comments

if (event.pressed) {
    if (tap_count == 0) {
        // Key pressed, so not a tap, so not setting oneshot mod here
        dprint("MODS_TAP: Oneshot: 0\n");
        // On this line previous oneshot mods become real mods - but why?
        register_mods(mods | get_oneshot_mods());
    } else if (tap_count == 1) {
        dprint("MODS_TAP: Oneshot: start\n");
        set_oneshot_mods(mods | get_oneshot_mods());
    } else {
        // Key tapped more than once - but why do we have separate treatment
        // for multiple taps (apart from tap toggle)?
        // Here too oneshot mods become real mods
        register_mods(mods | get_oneshot_mods());
    }
} else {
    if (tap_count == 0) {
        // Key raised after press
        // Clear oneshot mods - but they are no longer oneshots, but real,
        // and are not cleared
        clear_oneshot_mods();
        // Unregister the raised key's mods - but those oneshots that were
        // made real are not cleared
        unregister_mods(mods);
    } else if (tap_count == 1) {
        // Retain Oneshot mods
    } else {
        // Same thing after multiple taps, the current mod is cleared, but 
        // oneshots turned real are not cleared
        unregister_mods(mods);
        clear_oneshot_mods();
    }
}

My personal choice for this was to leave oneshots intact on press - so just register_mods(mods); - so they get cleared when the key is raised, and to remove handling of multiple taps (the else block).

Another possibility would be to change

        unregister_mods(mods);
        clear_oneshot_mods();

to

        clear_mods();

so that those mods that were oneshots would be cleared.

[mcp292]

+1, any progress on this front is greatly appreciated!!

I haven't dug into the source, but at face value, what about diving into the multiple-tap else only if Tap Dance, or relevant features are enabled.

[kasimir-k]

Free time is not as abundant as I'd wish, but I'm progressing with this nevertheless :-)

To summarise the desired behaviour for oneshot modifieres:

• one tap
  • on pressed, set oneshot mod
  • on released, if ONESHOT_TAP_TOGGLE and the tapped mod is locked, clear it
• two or more taps
  • have effect only with ONESHOT_TAP_TOGGLE > 1
  • when tapped ONESHOT_TAP_TOGGLE times, on pressed, lock the tapped mod (other mods are left intact)
  • on released, no effect
  • more taps in succession have no effect
• keypress (longer than tap)
  • on pressed, register the pressed mod
  • on released, unregister the released mod, and if the mod was set as oneshot, del_oneshot_mods it
  • no effect on other oneshot mods

I'll start working on a PR for this some day soonish if there's no objections to the logic above.

[sigprof]

mods | get_oneshot_mods() was apparently introduced in #2799; that PR claims that before the change chaining multiple OSM() keys did not work. Does that chaining still work now if you remove the part of that change just for the register_mods() case?

I suppose that the case with taps for all OSM keys would still work, because that case uses only set_oneshot_mods(), so all mods would all be applied when the next normal key is pressed. However, if you tap one OSM key, and then hold another OSM key, the modifiers for the previously tapped OSMs would remain oneshots, and so they would be unregistered for any subsequent normal keys after the first one — only the modifier for the OSM which remains held would be retained for the whole hold duration. If retaining all oneshot mods for the whole hold duration is desired in this case, it would need some extra code and state.

[sigprof]

The multi-tap case may really be a “N taps + hold” case (the tap detection code can detect a hold only for the first action, and just passes all subsequent press and release events through with increasing record->tap.count values until the tapping term finally expires); its handling is probably intended to make the OSM hold work as a normal modifier in that case. So the else case should remain, but the register_mods() there should probably lose the | get_oneshot_mods() part unless there is a way to undo that properly.

[kasimir-k]

mods | get_oneshot_mods() was apparently introduced in #2799;

Good catch - I tried also to find when and why that did happen, but missed this. And it's easy to see how the bug appeared. The one set_oneshot_mods(mods | get_oneshot_mods()); was what was needed - it adds currently tapped mod to existing OSMs thus allowing chaining.

Those two register_mods(mods | get_oneshot_mods()); are the bug, they do not chain OSMs, they just add previous OSMs to regular mods but without way to unregister those.

If retaining all oneshot mods for the whole hold duration is desired

Do you mean something like

1. tap ctrl
2. press shift
3. hold shift and tap letter - now we have ctrl-shift-letter
4. continue to hold shift and tap letter - do we now have ctrl-shif-letter or shift-letter?

IMO we should have only shift-letter, OSM should always be One Shot Mod (unless toggled of course).

The multi-tap case may really be a “N taps + hold” case

That might make sense in tap-dance context, but this code is only for OSMs. In that context I don't see any meaningful use case for N taps + hold. And if you look at the elses for tap pressed and released you'll see that the tapped mod is first registered and then unregistered, and as this is a tap, it becomes a NOP (apart from the unwanted effect of turning the other OSMs to regular but unregistrable mods).

Switching the perspective from code and possible but unknown intentions of coders to user perspective and looking what behaviour the user might want:

• the user does not have ONESHOT_TAP_TOGGLE set
  • after tapping the tapped mod should be on as an OSM, regardeless if it was single, double or n-tap
  • press-hold-and-release should have the held mod active as long as it is held, and when it is released it should not be active in any form, not as an OSM, not as a locked OSM
• the user has ONESHOT_TAP_TOGGLE set
  • press-hold-and-release should behave as above, after release that mod should not be active in any form
  • with tapping when tap_count < ONESHOT_TAP_TOGGLE it should behave as above: the tapped mod should be on as an OSM
  • with tapping when tap_count == ONESHOT_TAP_TOGGLE it should toggle on locking of the mod - until the mod is tapped again or held and released, then it should be unlocked and unregistered
  • with tapping when tap_count > ONESHOT_TAP_TOGGLE there should not be any effect beyond the locking that took place at ONESHOT_TAP_TOGGLE taps. If I have ONESHOT_TAP_TOGGLE set to 3 but tap 4 times within the TAPPING_TERM, the locking should stay in effect.

I hope some code coming later today or tomorrow will this more clear :-)

[kasimir-k]

I still want to test this more before I make a PR, but you can already have a look at it on my fork: https://github.com/qmk/qmk_firmware/compare/master...kasimir-k:qmk_firmware:fix/OSM-get-stuck

[pacien]

I'm no longer experiencing this issue after updating to master, which includes @kasimir-k's fix (PR #20034).