[Bug] Overlapping combos with different timeouts

[HelloThisIsFlo]

Hi,

I'm having an issue with overlapping combo with different timeouts. What I want to achieve is to have different timeouts for combos that are using keys on one hand only vs combos that requires two hands to activate. That way I can have 2-hands combos that are easier to trigger, while still keeping a short timeout for 1-hand combos so fast typing doesn't activate the combo.

Unfortunately, I think there is a bug in how the combo timeout are processed (see Context & The Problem). I did try to put a lot of logging try figure out what is going on, but I am not a C/C++ developer, and after spending 4hours trying to figure it out I didn't get very far. I really struggled with logging struct, or arrays. So the progress was excruciatingly slow 😬

That being said, I'd be more than happy to run local experiments if you need more info to isolate the issue

The Problem

Context

• I set up 2 combos, they have 1 key in common 20
  • Combo 1 OneHandLeft is: 20 21 with a timeout of 2000 (large for debug purposes)
  • Combo 2 TwoHands is: 20 29 with a timeout of 4000 (large for debug purposes)
  • Here are the key positions

    ╭────────────────────╮ ╭────────────────────╮
    │ __  __  __  __  __ │ │ __  __  __  __  __ │
    │ __  __  __  __  __ │ │ __  __  __  __  __ |
    | 20  21  __  23  24 │ │ __  __  __  28  29 |
    ╰───────╮ __  __  __ | | __  __  __ ╭───────╯
            ╰────────────╯ ╰────────────╯
    
    

The Issue

Expected

• When pressing 20, I would expect the key to wait for all associated combo timeouts to expire before deciding that no combo is a match.
• In other words I would expect it to trigger the regular keypress only after 4000ms have passed

Actual Behavior

• The behavior associated with the key is triggered as soon as the combo with the shortest timeout times out
• It triggers 2000ms after the key was pressed.
• This is consistent with other situations (see Real World Example below), the key will trigger its behavior as soon a one combo expires. It does not wait for all other combos with longer timeouts.

---

Real World Example

I'm using this example keymap on this setup:

• Actual Keyboard: Chocofi
• Board: Nice!Nano V2
• Shield: Corne (w/ five_column_transform)
• Build command:

  west build -d build/left -b nice_nano_v2 -- -DSHIELD="corne_left nice_view_adapter nice_view" -DZMK_CONFIG="PATH TO MY ZMK CONFIG"`
  

corne.keymap

#include <behaviors.dtsi>
#include <dt-bindings/zmk/keys.h>

/ {
    chosen {
        zmk,matrix_transform = &five_column_transform;
    };

/*

KEY POSITION 
╭────────────────────╮ ╭────────────────────╮
│ __  __  __  __  __ │ │ __  __  __  __  __ │
│ __  __  __  __  __ │ │ __  __  __  __  __ |
| 20  21  __  23  24 │ │ __  __  __  28  29 |
╰───────╮ __  __  __ | | __  __  __ ╭───────╯
        ╰────────────╯ ╰────────────╯

Expectations: 
- Holding 20 should not trigger anything for at least 4000ms
    - The OneHandLeft combo would timeout after 2000ms
    - But the TwoHands combo should not timeout until 4000ms

What happens:
- Holding 20 triggers the &kp Z binding after 2000ms

*/
    combos {
        compatible = "zmk,combos";

        OneHandRight {
            bindings = <&kp R>;
            key-positions = <29 28>;
            timeout-ms = <1000>;
        };

        OneHandLeft {
            bindings = <&kp L>;
            key-positions = <20 21>;
            timeout-ms = <2000>;
        };

        OneHandLeftDifferentKeys {
            bindings = <&kp D>;
            key-positions = <24 23>;
            timeout-ms = <3000>;
        };

        TwoHands {
            bindings = <&kp T>;
            key-positions = <20 29>;
            timeout-ms = <4000>;
        };



    };

    keymap {
        compatible = "zmk,keymap";

        QWERTY {
            label = "QWERTY";
            bindings = <
  &kp Q  &kp W  &kp E    &kp R    &kp T        &kp Y      &kp U     &kp I       &kp O    &kp P
  &kp A  &kp S  &kp D    &kp F    &kp G        &kp H      &kp J     &kp K       &kp L    &kp SEMI
  &kp Z  &kp X  &kp C    &kp V    &kp B        &kp N      &kp M     &kp COMMA   &kp DOT  &kp FSLH
                &kp TAB  &kp RET  &kp SPACE    &kp SPACE  &kp BSPC  &kp ESCAPE
            >;
        };
    };
};

[HelloThisIsFlo]

Update: I could confirm the behavior is indeed a bug with this test case I just wrote

https://github.com/HelloThisIsFlo/zmk/blob/c9ebd8938a80c5e2deaf59aa2eaf39ef5c02c518/app/tests/combo/overlapping-combos-4-different-timeouts/native_posix_64.keymap

I'll see if I can find a fix, but again, it's the first time in 15 years that I see C code, so if anyone else sees this and notices it's a quick fix, I'd appreciate the help 🙂 If it's a complex fix, no worries, I don't think it's a super high priority bug.

Also, I noticed the tests, at least in the combo folder, don't test the timings at all. I tried to modify the sed command to capture the timestamp, but it doesn't work.

This sed: s/\(hid_listener_keycode\)/\1/p would return (for example):

zmk: hid_listener_keycode_pressed: usage_page 0x07 keycode 0x04 implicit_mods 0x00 explicit_mods 0x00

The timestamp info is still missing. Any idea how to capture the timestamp so I include it in the snapshot?

[HelloThisIsFlo]

Edit: This example is invalid, see below: #1944 (comment)

---

Update

I found a way to test the timeouts without having access to the timestamps. I've reworked the test with a working assertion snapshot to illustrate the bug

https://github.com/HelloThisIsFlo/zmk/blob/50b89f676693e582a6d0b712174c5cf7a7e5609b/app/tests/combo/overlapping-combos-4-different-timeouts/native_posix_64.keymap

Everything is explained in the test itself, but to make it easier to follow, I'll copy paste the description here

Expected output:
----------------
- T+50: Key 0 (A) is pressed
    - Nothing to see
- T+80: 'combo_short' expires (press + 30ms) => Nothing happens because we're still waiting to see if 'combo_long' will be matched
    - Nothing to see
- T+100: Key 4 (D) is pressed and released
    - We should see: 'position 4 keycode 0x70007'
- T+150: 'combo_long' expires (press + 100ms) => Both combos expired, now we're sending the KEY_PRESS binding
    - We should see: 'position 0 keycode 0x70004' 
- T+200: Key 0 (A) is released
    - Nothing to see
- T+220: Key 4 (D) is pressed and released
    - We should see: 'position 4 keycode 0x70007' 


We don't have access to the timings in the snapshot, so this order allows to test timeouts based on the sequence on keycode sent.
If everything works as expected, we should send: D, A, D.

Current problem: ❌
----------------
At the moment there's a bug where whenever the shortest combo expires, we stop waiting for other candidates. With that buggy behavior
zmk will send instead: A, D, D. Because the expiry of 'combo_short' will output A before the first D.

[HelloThisIsFlo]

Update

I found and fixed the bug. I'm verifying just one last assumption, running all the tests, and I'll create PR (might be tomorrow)

[vvv-ca]

I think there is a somewhat important consideration to be made here: is the physical key press order something that should be preserved or not?
I believe the current code does attempt to preserve the order - A is pressed before D, thus A is output before D (no matter what combos exist). If the answer to the above is yes, then your use case is technically mistype, not a bug.

I've been working on rolling combos support for the past few weeks and I came across the same issue. My initial implementation did not preserve the physical press order and thus one of the current tests is failing. I'm making another attempt, but this time my goal is to make sure that physical key press order is preserved (combo is placed at first pressed key position).

Another aspect I see here is what I would call "combo preferred" behavior that should probably be optional. I do not know how waiting for the longest combo will affect other behaviors that might overlap with the the combos. In your use case, what happens if there is another behavior that will activate on the first A press and now you move it to after the D?

[HelloThisIsFlo]

@vvv-ca That is a very good point! And it is the reason my "verifying just one last assumption" is taking so long 😅

I agree with you that my test shouldn't pass as I described it in the message above. The behavior my test is expecting would indeed be a bug. Good catch!

Thanks to your comment, and some more investigation, this is the behavior I would expect in the scenario described above:

1. A is held
2. D is pressed, it is not included in any pending combos.
   • Because D isn't in any combo, all pending combos are cleared
   • A press event is sent
   • D press event is sent

That means I can't use my "extra key hack" to go around the fact we don't have access to the "log timestamp" in the golden master snapshot. For now, I'll focus on finding a fix to the bug, I'll think about how to test it properly later.

However, my use-case doesn't involve using this extra key. It was only for testing purposes. My use-case is simply to allow different timeout for different combos, and when they share the same key, the longest timeout should be used, not the shortest. And yes, any interrupt in un-related key should 100% interrupt said timeout (my example overlooked that part).

The fix I found would indeed break the intended behavior, so I'm very glad you brought that up. Now however, I'm all set up with debug environment, quick feedback loop, etc... So my workflow is much much better than when I first discovered this issue. So, while I didn't find a proper fix as I originally thought, I'll still spend some time to see if I can find a better solution. Probably tomorrow though. I'll share my progress if I have anything.

I'll check your code later for inspiration, thanks for sharing.

[HelloThisIsFlo]

Update

• Found the bug
• Confirmed it didn't have unintended consequences (the point brought up by @vvv-ca was valid, and I made sure the order of key press was maintained).
• Opened a PR: Fix bug with overlapping combos timeouts #1945