Fix OSM modifiers only send keydown when held after tap

[Jari27]

Description

(seems related to #3963)

Currently, one shot modifiers have weird behavior on windows (and maybe others). After a OSM(MOD_XXX) key is tapped and then held and released, there is a key down even registered for that key but no key up. Additionally, according to QMK the mod is not considered active. So for the host, the modifier is active but for QMK it's not. This seems like a bug.

This PR fixes the problem by only sending one shot modifiers to the host if there is in fact a relevant key pressed, i.e. no one shots. This allows the user to deactivate one shots by holding and releasing past COMBO_TERM.

Replicate:

1. Set up a OSM(MOD_GUI) key (or another OSM modifier)
2. Tap the the key -> one shot mod becomes active
3. Hold the key -> key down registered
4. Release the key -> key down still registered, one shot mod inactive

Example debug output with bug

# tap a normal key (this seems necessary to exhibit the behaviour)
> ---- action_exec: start -----
> EVENT: 0201d(6189)
> ACTION: ACT_LMODS[0:04] layer_state: 00000000(0) default_layer_state: 00000001(0)
> keyboard_report: 00 | 04 00 00 00 00 00 
> processed: 0201d(6189):0 
> 
> 
> ---- action_exec: start -----
> EVENT: 0201u(6269)
> ACTION: ACT_LMODS[0:04] layer_state: 00000000(0) default_layer_state: 00000001(0)
> keyboard_report: 00 | 00 00 00 00 00 00 
> processed: 0201u(6269):0 
> 
> 
# tap OSM(MOD_GUI)
> ---- action_exec: start -----
> EVENT: 0403d(16164)
> Tapping: Start(Press tap key).
> TAPPING_KEY=0403d(16164):0 
> processed: 0403d(16164):0 
> 
> 
> ---- action_exec: start -----
> EVENT: 0403u(16215)
> Tapping: First tap(0->1).
> TAPPING_KEY=0403d(16164):1 
> ACTION: ACT_LMODS_TAP[1:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> MODS_TAP: Oneshot: start
> waiting_buffer_enq: { [0]=0403u(16215):1  }
> ---- action_exec: process waiting_buffer -----
> Tapping: key event while last tap(>0).
> ACTION: ACT_LMODS_TAP[1:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> processed: waiting_buffer[0] =0403u(16215):1 
> 
> 
> 
# hold and release OSM(MOD_GUI)
# note that the keyboard report for keydown is emitted; but the keyup report is not
> ---- action_exec: start -----
> EVENT: 0403d(18176)
> Tapping: Start while last timeout tap(1).
> TAPPING_KEY=0403d(18176):0 
> processed: 0403d(18176):0 
> 
> Tapping: End. Timeout. Not tap(0): 0000u(18366)
> ACTION: ACT_LMODS_TAP[1:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> MODS_TAP: Oneshot: 0
> keyboard_report: 01 | 00 00 00 00 00 00 
> TAPPING_KEY=0000u(0):0 
> 
> ---- action_exec: start -----
> EVENT: 0403u(19454)
> ACTION: ACT_LMODS_TAP[1:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> processed: 0403u(19454):0 
> 
> 
# hold and release again, and now the report is correctly emitted for keyup
> ---- action_exec: start -----
> EVENT: 0403d(32197)
> Tapping: Start(Press tap key).
> TAPPING_KEY=0403d(32197):0 
> processed: 0403d(32197):0 
> 
> Tapping: End. Timeout. Not tap(0): 0000u(32387)
> ACTION: ACT_LMODS_TAP[1:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> MODS_TAP: Oneshot: 0
> TAPPING_KEY=0000u(0):0 
> 
> ---- action_exec: start -----
> EVENT: 0403u(32962)
> ACTION: ACT_LMODS_TAP[1:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> keyboard_report: 00 | 00 00 00 00 00 00 
> processed: 0403u(32962):0 
> 

Example debug output with this new PR

# tap a normal key
> ---- action_exec: start -----
> EVENT: 0201d(3953)
> ACTION: ACT_LMODS[0:04] layer_state: 00000000(0) default_layer_state: 00000001(0)
> keyboard_report: 00 | 04 00 00 00 00 00 
> processed: 0201d(3953):0 
> 
> 
> ---- action_exec: start -----
> EVENT: 0201u(4007)
> ACTION: ACT_LMODS[0:04] layer_state: 00000000(0) default_layer_state: 00000001(0)
> keyboard_report: 00 | 00 00 00 00 00 00 
> processed: 0201u(4007):0 
> 
> 
# tap OSM(MOD_GUI)
> ---- action_exec: start -----
> EVENT: 0402d(4919)
> Tapping: Start(Press tap key).
> TAPPING_KEY=0402d(4919):0 
> processed: 0402d(4919):0 
> 
> 
> ---- action_exec: start -----
> EVENT: 0402u(4976)
> Tapping: First tap(0->1).
> TAPPING_KEY=0402d(4919):1 
> ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> MODS_TAP: Oneshot: start
> waiting_buffer_enq: { [0]=0402u(4976):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=0402u(4976):1 
> processed: waiting_buffer[0] =0402u(4976):1 
> 
> 
> Tapping: End(Timeout after releasing last tap): 0000u(5166)
> TAPPING_KEY=0000u(0):0 
> 
# hold and release OSM(MOD_GUI) and now the keyup event is correctly emitted.
> ---- action_exec: start -----
> EVENT: 0402d(6775)
> Tapping: Start(Press tap key).
> TAPPING_KEY=0402d(6775):0 
> processed: 0402d(6775):0 
> 
> Tapping: End. Timeout. Not tap(0): 0000u(6965)
> ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> MODS_TAP: Oneshot: 0
> keyboard_report: 04 | 00 00 00 00 00 00 
> TAPPING_KEY=0000u(0):0 
> 
> ---- action_exec: start -----
> EVENT: 0402u(7529)
> ACTION: ACT_LMODS_TAP[4:00] layer_state: 00000000(0) default_layer_state: 00000001(0)
> keyboard_report: 00 | 00 00 00 00 00 00 
> processed: 0402u(7529):0 
> 

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[tzarc]

This would need to go to develop and given it's messing about with action handling would require associated unit tests to validate behaviour.

That said, there have been a few changes on develop in this area this cycle, might be wise confirming it's still a problem?

[Jari27]

Good point, I'll try and see if it's still a problem on develop first. Unfortunately, I have little experience with C and C unit tests and my IDE seems to not understand the macros used there, so it's a little hard to dig into.

I'll get back to you about develop asap.

w.r.t. messing with action handling, I've verified the following

• one shot modifiers can still be used together (in sequence) to trigger multiple mods for the next keydown
• one shot modifiers can be used with held modifiers (oneshot or normal)
• behaviour (to me) seems more intuitive as there is no leaking between "keyboard" features (i.e. one shot modifiers) and "hardware" features (i.e. keyboard_report)

I have not been able to get the unit tests working (I have little C experience) and my IDE doesn't seem to recognise the macros and defined used there, so it's hard to dig into. Any pointers here would be helpful.

[getreuer]

@Jari27 thank you for this contribution! Here are a few pointers on unit testing.

QMK uses the GoogleTest C++ unit testing framework (yes, C++, though QMK itself is C). The quickest way to get started is to look at how existing tests under the tests/ directory are written.

Some essential parts of GoogleTest are implemented as macros, and yes, this is confusing to IDEs (and programmers). I'm afraid the macros can't be avoided. If you are looking for definitions of the test harness ("EXPECT_REPORT()," etc.), many of them are defined under tests/test_common/test_driver.hpp and other headers in the test_common folder.

You can build and run test "<name>" (e.g. "caps_word") with the command

qmk test-c -t <name>

[sigprof]

Also unit tests don't work on Windows with MSYS2 due to some deficiencies in that toolchain (QMK uses weak symbols in lots of places, and apparently the gcc-mingw64 toolchain does not support weak symbols properly). You can either use Linux (including QMK WSL) or macOS to run tests, or, if you are stuck with MSYS2, try passing LTO_ENABLE=yes to make (-e LTO_ENABLE=yes if using qmk test-c); the latter is not guaranteed to work though.

[Jari27]

Got a bit further -- current approach is resetting oneshots when the same oneshot key is held (instead of playing around with the keyboard_record function). I am not able to write a test that exhibits the incorrect behaviour (test seems to completely ignore the held OSM and does not send any reports:

OneShot.OSMTapThenHoldNotLockingOSMs failed!
[ LEVEL    ] [TIME] [EVENT]
[ INFO     ] 0      tapping term is 200ms
[ TRACE    ] 0      pressed:  OSM(MOD_LSFT)
[ TRACE    ] 0      1 keyboard task loop
[ TRACE    ] 1      released: OSM(MOD_LSFT) was pressed for 1ms
[ TRACE    ] 1      1 keyboard task loop
[ TRACE    ] 2      pressed:  OSM(MOD_LSFT)
[ TRACE    ] 2      1 keyboard task loop
[ TRACE    ] 3      200 keyboard task loops
# expect a keydown report here
[ TRACE    ] 203    released: OSM(MOD_LSFT) was pressed for 201ms
[ TRACE    ] 203    1 keyboard task loop
# expect a keyup report here
[ TRACE    ] 204    pressed:  KC_A
[ TRACE    ] 204    1 keyboard task loop
[ TRACE    ] 204    report:   (KC_A) [KC_LEFT_SHIFT]
[ TRACE    ] 205    released: KC_A was pressed for 1ms
[ TRACE    ] 205    1 keyboard task loop
[ TRACE    ] 205    report:   empty
[ INFO     ] 206    test fixture clean-up start.
[ TRACE    ] 206    2000 keyboard task loops
[ TRACE    ] 2206   2000 keyboard task loops
[ INFO     ] 4206   test fixture clean-up end.

Will look a bit more tomorrow!