Qmk_firmware: Rolling quickly from KC_EXLM to KC_EQL types `!+` rather than `!=`

[callum-oakley]

I can reproduce this with any shifted key followed by a non shifted key. I guess the emulated shift isn't cancelled before the = keycode is sent?

I'm on a rev_5 planck, this keymap. Though I'm certainly a few commits behind master, so if nobody can reproduce I'll double check that the problem persists in master.

(I'm also not 100% sure that this couldn't be a hardware issue? Haven't looked in to how KC_EXLM is implemented.)

[callum-oakley]

Ahah, related to https://github.com/qmk/qmk_firmware/issues/1708 ? Will take a closer look later and close this if so.

[drashna]

And fred has a PR that may fix this too.
If you download "https://github.com/qmk/qmk_firmware/pull/2710.diff", and use git apply 2710.diff, you can test out the fix and see if it works for you.

[callum-oakley]

Great stuff, will check the diff out when I get home. Thanks :)

[callum-oakley]

Hmm unfortunately I can still reproduce after applying the diff. (built this commit: https://github.com/callum-oakley/qmk_firmware/commit/b344f99c7fa030e8bd5f125aa488b114355da4dd, on my fork, but only a couple of unrelated commits away from master here.)

Anything else I can do to help debug?

[drashna]

do you have NKRO enabled?

If not, it may be worth enabling and see if that helps.

To enable it, you'd need NKRO_ENABLE=yes in your rules.mk and #define FORCE_NKRO in your config.h.

Note, that if you're using a VUSB device, it does not support NKRO.

[callum-oakley]

Doesn't help I'm afraid :/

Assuming I did it right: I already had NKRO_ENABLE = yes in my rules.mk, but I didn't have a config.h, would this do the trick?

// keyboards/planck/keymaps/callum/config.h
#ifndef CONFIG_USER_H
#define CONFIG_USER_H

#include "config_common.h"

#define FORCE_NKRO

#endif

(tested with this commit https://github.com/callum-oakley/qmk_firmware/commit/fc009782a0d1424ebe92040c0d968106ad84b94c)

[callum-oakley]

So after thinking about this some more and having a brief look at the code I think I know that's going on. It happens when you press = before releasing !. Since ! hasn't been released yet, the synthetic shift _also_ hasn't been released yet, so you get shift = or +. From that perspective the behaviour makes perfect sense. It's just like you've held shift and 1, and then hit = -- you would expect it to type +.

buuuuuut having auto shifted keys lulls you in to thinking they will behave just like regular keys, and you can type ab perfectly fine by depressing a, then depressing b, then releasing a, then releasing b. Conceptually, I suppose I want the auto shifted key to fire and then immediately release (before I _actually_ release the key), though that does mean you can't hold an auto shifted key (does anything require that?).

As far as I can tell, I don't ever want or need to hold an auto shifted key, so a possible workaround is to catch all those keycodes in my process_record_user and use SEND_STRING instead.

case KC_EXLM:
  if (record->event.pressed) {
      SEND_STRING("!");
  }
  return false;
  break;

which gives me the behaviour I want, but feels like a bit of an abuse of SEND_STRING.

[drashna]

Sorry, yes the above would work.

As for send string, if you think you're abusing it.... :D
But that's fine, actually.

[callum-oakley]

Cool cool, well I'm happy with the behaviour of the SEND_STRING version (see https://github.com/qmk/qmk_firmware/pull/4223).

That being said I do think this is still an issue with the existing shifted key implementation, as rolling from key to key is something that happens naturally while typing fast. I suppose the only problem with the SEND_STRING method is that we can't hold the keys down any more. Could something like the following work?

What happens already:

• ! is pressed, send shift down and 1 down
• ! is released, send 1 up and shift up

The problem if you hit another key before releasing the shifted one:

• ! is pressed, send shift down and 1 down
• = is pressed, send = down (shift is still down, so we get +)
• ! is released, send 1 up and shift up
• = is released, send = up

A potential solution? If we keep track of if we're currently handling an auto shifted key, we can do:

• ! is pressed, send shift down and 1 down
• = is pressed
  
  
  
  • recognise that an auto shifted character is still being pressed, send shift up
  
  
  • send = down (shift is no longer down so we actually get what we want)
  
  
• ! is released, send 1 up and shift up (shift up is a NOOP)
• = is released, send = up

[IsaacElenbaas]

Also occurs with Tapdance shifted keys and AutoShift, see #6214. Clearing weak mods before sending the second key fixes it for both cases on my end. Not sure why the second key would go through before the first has finished, as the modifier does get released eventually. I don't see a great way to determine if a modifier is still there because of the previous key, and we obviously can't clear weak mods before every key. Just read callum-oakley's explanation, makes sense. I don't see a great qmk-wide solution, this is probably going to be something to fix on a per-keymap basis.

[enryco]

@callum-oakley still no fix with existing shifted key implementation :(

who holds down and repeat characters anyway; seems like a more reasonable tradeoff than the existing problem :)

[enryco]

@callum-oakley i endet up implementing the SEND_STRING solution, thanks :) although i had to rewrite the code, as SEND_STRING only accepts "" (or replace SEND_STRING with send_string_P in your function)

[callum-oakley]

Glad it was helpful :)

I'm curious to see your send_string_P solution because I don't entirely follow you, but as long as you got something working that's the main thing 😅

[enryco]

If I try to compile your keymap it throws:

Compiling: keyboards/planck/keymaps/callum/keymap.c                                                In file included from quantum/quantum.h:19,
                 from keyboards/planck/planck.h:4,
                 from keyboards/planck/keymaps/callum/keymap.c:1:
keyboards/planck/keymaps/callum/keymap.c: In function 'send_string_if_keydown':
quantum/quantum.h:219:43: error: invalid initializer
  219 | #define SEND_STRING(string) send_string_P(PSTR(string))
      |                                           ^~~~
keyboards/planck/keymaps/callum/keymap.c:208:17: note: in expansion of macro 'SEND_STRING'
  208 |                 SEND_STRING(shifted);
      |                 ^~~~~~~~~~~
quantum/quantum.h:219:43: error: invalid initializer
  219 | #define SEND_STRING(string) send_string_P(PSTR(string))
      |                                           ^~~~
keyboards/planck/keymaps/callum/keymap.c:211:17: note: in expansion of macro 'SEND_STRING'
  211 |                 SEND_STRING(unshifted);
      |                 ^~~~~~~~~~~
quantum/quantum.h:219:43: error: invalid initializer
  219 | #define SEND_STRING(string) send_string_P(PSTR(string))
      |                                           ^~~~
keyboards/planck/keymaps/callum/keymap.c:214:13: note: in expansion of macro 'SEND_STRING'
  214 |             SEND_STRING(unshifted);
      |             ^~~~~~~~~~~
 [ERRORS]
 |
 |
 |
make[1]: *** [.build/obj_planck_rev3_callum/keyboards/planck/keymaps/callum/keymap.o] Error 1
make: *** [planck/rev3:callum] Error 1
Make finished with errors

That is because SEND_STRING is implemented as #define SEND_STRING(string) send_string_P(PSTR(string)) and in your function, you already have a string pointer. Using send_string_P(shifted) instead of SEND_STRING(unshifted) fixes that for me.

But I'm wondering why it was working for you before. Maybe the implementation of PSTR changed? No idea 🤣

(i used the qmk master branch v0.9.43)

[callum-oakley]

Ah interesting, I actually get the same if I try and compile for my rev 5 planck. But it works okay for my rev 6. 🤔 Not sure what's going on there, but changing to send_string (lowercase) seems to do the trick for me on the rev 5. I'm not sure what the difference is between send_string_P and send_string... 😬

[IsaacElenbaas]

I don't see why this is happening, actually. Supposedly weak_mods are cleared every key down to fix a nearly identical issue.
EDIT: Trying to rely on that for some Auto Shift work, it definitely isn't cleared.

[callum-oakley]

Interesting. Is the shift from auto shifted keys not counted as a "weak_mod" or is this broken (again) for the case described in the original issue as well?

[IsaacElenbaas]

Not certain if you're referencing Auto Shift "auto shifted keys" or stuff like KC_EXLM. This is not an issue with Auto Shift currently, but in a pull I'm working on it was if I didn't clear them myself on key down (just if (event->pressed) del_weak_mods(MOD_BIT(KC_LSFT)), don't see how they could have been cleared before and that fix it). The original issue seems to be referencing the exact same thing as this one, but they gave no specific example. I'm not certain where KC_EXLM and the like are defined, but them not using weak_mods would not make sense as that is their main purpose.

I'll check this out later today if you don't beat me to it by testing clearing them on any key down in process_record_user to make sure it's not me getting lost in my own Auto Shift code.

One thing I ran into problems with was that clearing them doesn't send a keyboard report, but pressing a key after they were cleared still had the correct behavior (the OS shift state isn't updated until you press a key), so I don't think that's the issue here.

[IsaacElenbaas]

Actually, I can't reproduce without AutoShift enabled (but clearing in process_auto_shift on key down fixes it). #6214 it happens with tapdance, too. I assume both TD and AS get the key event before process_action and don't clear them, so we either need to make it happen earlier or clear in them. If I understand the purpose of weak_mods right, it really should just be done as soon as the key is pressed.

[enryco]

TL;DR: yes talking about KC_EXLM but SEND_STRING does what the desired behavior is (it sends: shift⬇️, key⬇️, key⬆️, shift⬆️ ) which allows key rolling and works as fast as normal keys

yes, referring to shifted keys (not auto shift per qmk definitio) meaning KC_EXLM. they are defined in quantum/quantum_keycodes.h as

#define LSFT(kc) (QK_LSFT | (kc))
...
#define KC_EXLM LSFT(KC_1)  // !

however i cant find at first glance to force the key being released. SEND_STRING, however, works for just fine for me: SEND_STRING("!") does exactly what the preferred behavior is, on a single key down (not releasing)

KEY-DOWN - QMK: KC_LSFT Event key: Shift       Code: ShiftLeft     KeyCode: 16
KEY-DOWN - QMK: KC_1    Event key: !           Code: Digit1        KeyCode: 49
KEY-UP   - QMK: KC_1    Event key: !           Code: Digit1        KeyCode: 49 in 9.000ms
KEY-UP   - QMK: KC_LSFT Event key: Shift       Code: ShiftLeft     KeyCode: 16 in 37.000ms

whereas KC_EXLM pressing (without releasing) gives

KEY-DOWN - QMK: KC_LSFT Event key: Shift       Code: ShiftLeft     KeyCode: 16
KEY-DOWN - QMK: KC_1    Event key: !           Code: Digit1        KeyCode: 49

... until released (say one sec after) and every rolled key will spawn here

KEY-UP   - QMK: KC_1    Event key: !           Code: Digit1        KeyCode: 49 in 109.000ms
KEY-UP   - QMK: KC_LSFT Event key: Shift       Code: ShiftLeft     KeyCode: 16 in 137.000ms

which is great if you want autorepeat this character but is problematic if you roll bc the shift is not released yet.

[IsaacElenbaas]

does what the desired behavior is. . . which allows key rolling and works as fast as normal keys

It's a fine workaround, but one that's not needed and doesn't work if you want keyrepeat for both keys (you could also SEND_STRING what you roll to, eg. / in my */ issue if I want * repeatable but don't care about /).

which is great if you want autorepeat this character but is problematic if you roll bc the shift is not released yet.

That's why weak_mods are cleared on every key down. QMK seems to try to keep the shift state 'belonging' to the last pressed key. This approach works, the issue here is that AS and TD get the keycode first and return false. Clearing weak_mods in them on key down works with keyrepeat and rolling, but it should happen earlier.
EDIT: Needs to be moved to action_exec.

[callum-oakley]

I just tried to reproduce this on master in order to then test #9941 and couldn't! @enryco can you still reproduce this? If I get a chance I might try running a git bisect from when I know it was broken to track down what fixed it. But for now I'm just happy I can have my rolls and key repeat and that my keymap is a little cleaner 😄

!=!=!=!=!=!=!=!=!============================== 🎉

[callum-oakley]

I'll close this now as well since I opened it originally and can no longer reproduce. If anyone is still seeing this they should of course reopen.

[IsaacElenbaas]

I was able to reproduce on a pretty barebones keymap with just AS enabled when testing #9941 myself. != did not cause the issue for me, but ! then digits did. Either way weak_mods aren't cleared properly for TD/AS keys so that should still be merged or they should be cleared in TD/AS (would give more control, I suppose).

[callum-oakley]

Interesting, I can't even reproduce with ! then digit now. But absolutely, this is no reason not to merge your PR. :)

[callum-oakley]

I just discovered that this was Karabiner all along. 🤦

I started seeing this behaviour again, and after much debugging, confirmed that the keyboard was sending the correct report, but something was going wrong at a software level. Karabiner was the culprit. I don't know what it does that is incompatible with QMKs weak mods handling, but it seems to produce some delay in the keyup or something? Same underlying bug as #6571 I guess.

Leaving this here for future travellers stumbling upon the same issue.