Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sypl opened this issue Sep 23, 2018 · 26 comments
Open

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

sypl opened this issue Sep 23, 2018 · 26 comments

Comments

@sypl
Copy link
Contributor

sypl commented Sep 23, 2018

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
Copy link
Member

drashna commented Sep 23, 2018

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
Copy link

mtourne commented Mar 20, 2019

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
Copy link

mcp292 commented Feb 8, 2020

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
Copy link

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
Copy link

mcp292 commented Mar 28, 2021

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 5dc7951. Before this I was at 361ac2f. While skimming through I saw something about crkbd mod keys (027570a; 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
Copy link
Contributor

daliusd commented Aug 24, 2021

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
Copy link
Contributor

precondition commented Oct 11, 2021

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
Copy link

geaz commented Nov 29, 2021

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
Copy link

mcp292 commented Nov 29, 2021

@geaz Nice digging!

precondition added a commit to precondition/polilla-keymap that referenced this issue Jan 24, 2022
@iandunn
Copy link

iandunn commented Feb 17, 2022

@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
Copy link
Contributor

@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
Copy link

iandunn commented Feb 18, 2022

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
Copy link
Contributor

daliusd commented Feb 18, 2022

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 #16174

@iandunn
Copy link

iandunn commented Feb 18, 2022

Thanks!

@mcp292
Copy link

mcp292 commented Feb 20, 2022

@daliusd Thanks for sharing! Interesting layout!

@iandunn
Copy link

iandunn commented Feb 21, 2022

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
Copy link

iandunn commented Mar 21, 2022

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) {

iandunn added a commit to iandunn/dotfiles that referenced this issue Mar 21, 2022
It had unexpected interactions with mouse, so scrolling and file selecting locked on because the click doesn't cancel it. Would often accidentally
close windows, etc.

Native OSM also has the bug, but qmk/qmk_firmware#3963 (comment) works around it.
@jerviscui
Copy link

jerviscui commented Jan 13, 2023

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

@kasimir-k
Copy link
Contributor

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
Copy link

mcp292 commented Feb 27, 2023

+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
Copy link
Contributor

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
Copy link
Contributor

sigprof commented Mar 4, 2023

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
Copy link
Contributor

sigprof commented Mar 4, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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

@kasimir-k kasimir-k mentioned this issue Mar 6, 2023
14 tasks
@pacien
Copy link

pacien commented Oct 14, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests