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

[Bug] AUTO_SHIFT_MODIFIERS occasionally sends single key instead of with-modifier-variant #19671

Open
2 tasks
mplattner opened this issue Jan 23, 2023 · 9 comments

Comments

@mplattner
Copy link

mplattner commented Jan 23, 2023

Describe the Bug

Since I've enabled AUTO_SHIFT_MODIFIERS, I have the issue that ~5% of all uses/presses of e.g. CTRL+c (copy to clipboard) just send a c instead of CTRL+c.

I thought it might be that I unintentionally press CTRL+c for too long (longer than AUTO_SHIFT_TIMEOUT, which is 150ms in my case), and thus it sends CTRL+SHIFT+c (as expected, when you press it for too long), but this is not the case: it doesn't send CTRL+SHIFT+c, but actually only c.

Seems like a bug to me. Here is my keyboard code. Any ideas?

Keyboard Used

sculpter

Link to product page (if applicable)

No response

Operating System

Windows 10

qmk doctor Output

No response

Is AutoHotKey / Karabiner installed

  • AutoHotKey (Windows)
  • Karabiner (macOS)

Other keyboard-related software installed

No response

Additional Context

No response

@lesshonor
Copy link
Contributor

Have you tried setting TAP_CODE_DELAY to a relatively small value (10)?

If you're the tinkering type, you could also experiment with #19449.

@mplattner
Copy link
Author

Thanks @lesshonor. It feels like the issue occurs less often now, but it's not fixed. Do you suggest do lower or increase the value of TAP_CODE_DELAY further?

@lesshonor
Copy link
Contributor

Thanks @lesshonor. It feels like the issue occurs less often now, but it's not fixed. Do you suggest do lower or increase the value of TAP_CODE_DELAY further?

The problem is that the keypresses aren't being detected, so a lower TAP_CODE_DELAY can't help. Only a higher one.

That said, increasing TAP_CODE_DELAY is something of a brute force solution. Since it hasn't worked for you (I have given others this exact same advice with more positive results) I would suggest testing the code in #19449 to see if that solves your issue. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

If you do try it out, I'm sure the person who opened that pull request would appreciate your feedback.

@mplattner
Copy link
Author

Thanks again. Incorporating #19449 did not help. I did some more testing. It all boils down to whether AUTO_SHIFT_MODIFIERS is defined, i.e., enabled. If it is, the issue occurs; otherwise it does not.

The only two occurences of AUTO_SHIFT_MODIFIERS within qmk are

# ifndef AUTO_SHIFT_MODIFIERS
and
// Fixes modifiers not being applied to rolls with AUTO_SHIFT_MODIFIERS set.

so I guess the issue is somewhere there?

@IsaacElenbaas
Copy link
Contributor

TL;DR you could try adding the below at the bottom of autoshift_end:

#    if TAP_CODE_DELAY > 0
        send_keyboard_report();
        wait_ms(TAP_CODE_DELAY);
#    endif

autoshift_press might, depending on the held modifiers, exit early and remove that key press (and the coming release) from the Auto Shift logic entirely. With AUTO_SHIFT_MODIFIERS defined that is not the case and the key is registered once you release the key or the modifier in autoshift_end. When AUTO_SHIFT_MODIFIERS is not set and ctrl is held you are manually putting in delay because the key events are tied 1:1 to the physical events.

There is a wait_ms(TAP_CODE_DELAY) between pressing and releasing the key in autoshift_end, but not in the default autoshift_press/release_users when setting shift (see #13708), before autoshift_flush_shift, or before returning.
Those not being present could all cause similar issues - modifier presses or releases are sent immediately before/after the key event, even with TAP_CODE_DELAY set.

The reason for it only happening occasionally for you could in part be because the ctrl release would only happen immediately after the key release if you release it before the key - e.g. ctrl down, key down, ctrl up, key up. Though it could just be intermittent anyway.

@mplattner
Copy link
Author

mplattner commented May 14, 2023

Thanks @IsaacElenbaas. I tried to incorporate your suggestion in mplattner@4eec608. Did I do it the way you had in mind?
It did not resolve the issue, it felt like it got worse. Any other ideas?

@mplattner
Copy link
Author

I've just updated to the latest version in the master branch (dbd847d), but the problem persists.
Any help/ideas are appreciated 😊

@IsaacElenbaas
Copy link
Contributor

IsaacElenbaas commented Dec 5, 2023

The probably bad code shape I described above was changed for other reasons in the latest dev->master merge. Could you try once more? I would have guessed that the TAP_CODE_DELAY would have fixed this, but looking at the code now, AUTO_SHIFT_MODIFIERS should have no effect on how keys are handled.

I just looked at that commit from your comment in May again, it should already have those changes. autoshift_press at L160 in that commit is probably the better place to put it, actually. I will try to look at this soon myself though - just to clarify, this is a normal ctrl key, not a MT or tapdance or anything?

@mplattner
Copy link
Author

mplattner commented Dec 5, 2023

Thanks for getting back! Would be great if this can be fixed.

Good question - my CTRL is actually a OSM(MOD_LCTL), SHIFT is OSM(MOD_LSFT) - see https://github.com/mplattner/qmk_firmware/blob/4eec608c552a98ec79b8b60a8a92c2b45dee51fb/keyboards/sculpter/keymaps/mplattner/keymap.c#L47
I haven't tried how it behaves with the regular variant (just KC_LCTL).

Not sure if this is related or if it can be fixed together, but another problem that persists for me with OSM() is documented in #18488.

Thank you @IsaacElenbaas!

mplattner added a commit to mplattner/qmk_firmware that referenced this issue Apr 5, 2024
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

3 participants