[Bug] Retro Shift not working on Mod Tap keys

[theblaggy]

Describe the Bug

Using Retro_Shift has no effect on my Mod Tap keys.
Auto Shift is enabled and works for normal alpha keys.
But when exceeding the AUTO_SHIFT_TIMEOUT on the Mod Tap keys without pressing an additional key, retro shift doesn't trigger.

System Information

• Keyboard: Planck
  • Revision (if applicable): rev6_drop
• Operating system: macOS 12.1
• AVR GCC version: 8.4.0
• ARM GCC version: 8.3.1
• QMK Firmware version: 0.15.6
• Any keyboard related software installed?
  • AutoHotKey
  • Karabiner
  • Other:

Additional Context

Retro Shift added in d9393b8

Relevant part of config.h:

// Tapping Term
#define TAPPING_TERM 300
#define TAPPING_TERM_PER_KEY

// Prevent normal rollover on alphas from accidentally triggering mods.
#define IGNORE_MOD_TAP_INTERRUPT

// Auto Shift
#define NO_AUTO_SHIFT_NUMERIC
#define NO_AUTO_SHIFT_SPECIAL
#define AUTO_SHIFT_REPEAT
#define AUTO_SHIFT_TIMEOUT TAPPING_TERM
#define AUTO_SHIFT_NO_SETUP
#define RETRO_SHIFT

[Ga68]

I've seen the same behavior without Karabiner installed, in case there's a worry that it's related to that. It's also on MacOS 11.6.1, with QMK 15.5.

[IsaacElenbaas]

You have to add each mod tap to Auto Shift, see here (there's also a typo there, "shold," and the default get_auto_shifted_key is out of date). Looking back, that's not at all clear in the docs. . . I'll open a PR to fix all that (and this which didn't make it in because the merge happened quickly) soonish if noone beats me to it.

[Ga68]

Thanks for taking a look Isaac. This is great.
Does your comment mean that you'd expect it to work right now, as is, if the mod-tap is added or only after you get a chance to make some changes?
I've just tried adding the mod-tap into my get_auto_shifted_key; however, it has some odd behavior still. If you would have expected it to work I can detail what I'm seeing, but in case your comment above meant "sit tight", I'll hold off unless you say otherwise.

[IsaacElenbaas]

It should work as-is if the mod-tap is added, I just need to update docs and a bit of code for per-interrupting-key tapping term which shouldn't be an issue here unless you're trying to use that (and certainly not for just getting a shifted key at all). You could also put if(IS_RETRO(keycode)) return true; at the top of it to add all mod and layer taps.

If I remember correctly I tested again on develop after my merge due to conflicts with early combos, but not after the entire breaking change cycle. I plan to test for the upcoming IGNORE_MOD_TAP_INTERRUPT removal PR and will check this again then, but yeah a small outline of what seems to be broken would be nice in case it works after reflashing my setup. There were issues with getting correct timestamps on some boards that showed up during the keyrepeat PR, and your issue could even be that workaround not working everywhere as I believe the same timestamps are used.

[IsaacElenbaas]

Most everything looks fine on my end on master, with both my config.h and yours.

I did run across one issue (?): With #define RETRO_SHIFT, no value set, it is as if it defaulted to TAPPING_TERM. That is, holds longer than TAPPING_TERM with no keys pressed cause the modifier to be pressed for use with the mouse. With TAPPING_TERM near or less than AUTO_SHIFT_TIMEOUT, getting shifted mod taps is near or completely impossible.
I'm not sure if this should be the case (if not it's probably an easy fix, making this and this be (RETRO_SHIFT + 0) == 0 || TIMER_DIFF_16(event.time, tapping_key.event.time) < RETRO_SHIFT instead to make holds of any length send the shifted value) but if so it needs to be documented as well. Thoughts?

[Ga68]

You were right. It is workIng for me okay with your comment regarding auto the mod tap keycodes into the auto shift functIon. The strange behavior I was having previously was related to a "hot fix" I had tried and forgot I still had buried away in quantum/actIon.c. So ... turns out if I hadn't messed with your work, and read the documentation a bit more closely, it would seem to be working very nicely 🤦‍♂️️🙂️

https://github.com/Ga68/qmk_firmware/compare/my_layout...Ga68:auto_shift?expand=1

I'm going to keep playing with it more now as I get used to the timing of keypresses to make sure I don't the acciDenTal CaPs too much, but it's looking GREAT!

[theblaggy]

I'm not sure if this should be the case (if not it's probably an easy fix, making this and this be (RETRO_SHIFT + 0) == 0 || TIMER_DIFF_16(event.time, tapping_key.event.time) < RETRO_SHIFT instead to make holds of any length send the shifted value) but if so it needs to be documented as well. Thoughts?

Yes, I think the default behavior of #define RETRO_SHIFT with no value specified should be to send the shifted value for holds of any length.