fix(combos)Fix bug with overlapping combos timeouts (#1945)

* Fix bug with overlapping combos timeouts

* Fix trailing whitespace

* Fix log format

app/src/combo.c

@@ -204,22 +204,34 @@ static inline bool candidate_is_completely_pressed(struct combo_cfg *candidate)
 static int cleanup();
 
 static int filter_timed_out_candidates(int64_t timestamp) {
-    int num_candidates = 0;
+    int remaining_candidates = 0;
     for (int i = 0; i < CONFIG_ZMK_COMBO_MAX_COMBOS_PER_KEY; i++) {
         struct combo_candidate *candidate = &candidates[i];
         if (candidate->combo == NULL) {
             break;
         }
         if (candidate->timeout_at > timestamp) {
-            // reorder candidates so they're contiguous
-            candidates[num_candidates].combo = candidate->combo;
-            candidates[num_candidates].timeout_at = candidate->timeout_at;
-            num_candidates++;
+            bool need_to_bubble_up = remaining_candidates != i;
+            if (need_to_bubble_up) {
+                // bubble up => reorder candidates so they're contiguous
+                candidates[remaining_candidates].combo = candidate->combo;
+                candidates[remaining_candidates].timeout_at = candidate->timeout_at;
+                // clear the previous location
+                candidates[i].combo = NULL;
+                candidates[i].timeout_at = 0;
+            }
+
+            remaining_candidates++;
         } else {
             candidate->combo = NULL;
         }
     }
-    return num_candidates;
+
+    LOG_DBG(
+        "after filtering out timed out combo candidates: remaining_candidates=%d timestamp=%lld",
+        remaining_candidates, timestamp);
+
+    return remaining_candidates;
 }
 
 static int clear_candidates() {
@@ -449,7 +461,7 @@ static void combo_timeout_handler(struct k_work *item) {
         // timer was cancelled or rescheduled.
         return;
     }
-    if (filter_timed_out_candidates(timeout_task_timeout_at) < 2) {
+    if (filter_timed_out_candidates(timeout_task_timeout_at) == 0) {
         cleanup();
     }
     update_timeout_task();

app/tests/combo/overlapping-combos-4-different-timeouts/events.patterns

@@ -0,0 +1 @@
+s/.*\(hid_listener_keycode_pressed\|filter_timed_out_candidates\): //p

app/tests/combo/overlapping-combos-4-different-timeouts/keycode_events.snapshot

@@ -0,0 +1,8 @@
+after filtering out timed out combo candidates: remaining_candidates=2 timestamp=71
+after filtering out timed out combo candidates: remaining_candidates=1 timestamp=81
+after filtering out timed out combo candidates: remaining_candidates=0 timestamp=91
+usage_page 0x07 keycode 0x04 implicit_mods 0x00 explicit_mods 0x00
+after filtering out timed out combo candidates: remaining_candidates=2 timestamp=143
+after filtering out timed out combo candidates: remaining_candidates=1 timestamp=153
+after filtering out timed out combo candidates: remaining_candidates=1 timestamp=159
+usage_page 0x07 keycode 0x1D implicit_mods 0x00 explicit_mods 0x00

app/tests/combo/overlapping-combos-4-different-timeouts/native_posix_64.keymap

@@ -0,0 +1,98 @@
+#include <dt-bindings/zmk/keys.h>
+#include <behaviors.dtsi>
+#include <dt-bindings/zmk/kscan_mock.h>
+
+#define kA 0
+#define kB 1
+#define kC 2
+#define kD 3
+
+/ {
+    combos {
+        compatible = "zmk,combos";
+
+        // Intentionally out of order in the config, to make sure 'combo.c' handles it properly
+        combo_40 {
+            timeout-ms = <40>;
+            key-positions = <kA kD>;
+            bindings = <&kp Z>;
+        };
+        combo_20 {
+            timeout-ms = <20>;
+            key-positions = <kA kB>;
+            bindings = <&kp X>;
+        };
+        combo_30 {
+            timeout-ms = <30>;
+            key-positions = <kA kC>;
+            bindings = <&kp Y>;
+        };
+
+    };
+
+    keymap {
+        compatible = "zmk,keymap";
+        label ="Default keymap";
+
+        default_layer {
+            bindings = <
+                &kp A &kp B
+                &kp C &kp D
+            >;
+        };
+    };
+};
+
+#define press_A_and_wait(delay_next) \
+    ZMK_MOCK_PRESS(0,0,delay_next)
+#define press_B_and_wait(delay_next) \
+    ZMK_MOCK_PRESS(0,1,delay_next)
+#define press_C_and_wait(delay_next) \
+    ZMK_MOCK_PRESS(1,0,delay_next)
+#define press_D_and_wait(delay_next) \
+    ZMK_MOCK_PRESS(1,1,delay_next)
+
+#define release_A_and_wait(delay_next) \
+    ZMK_MOCK_RELEASE(0,0,delay_next)
+#define release_D_and_wait(delay_next) \
+    ZMK_MOCK_RELEASE(1,1,delay_next)
+
+&kscan {
+    events = <
+        /* Note: This starts at T+50 because the ZMK_MOCK_PRESS seems to launch the first event at T+(first wait duration). So in our case T+50 */
+
+
+
+        /*** First Phase: All 3 combos expire ***/
+
+        /* T+50+0=    T+50: Press A and wait 50ms */
+        press_A_and_wait(50)
+
+        /* T+50+20=   T+70: 'combo_20' should expire */
+        /* T+50+30=   T+80: 'combo_30' should expire */
+        /* T+50+40=   T+90: 'combo_40' should expire, and we should send the keycode 'A' */
+
+        /* T+50+50=  T+100: We release A and wait 20ms */
+        release_A_and_wait(20)
+
+
+
+        /*** Second Phase: 2 combo expire, 1 combo triggers ***/
+
+        /* T+120+0=  T+120: Press A and wait 35ms */
+        press_A_and_wait(35)
+
+        /* T+120+20= T+140: 'combo_20' should expire */
+        /* T+120+30= T+150: 'combo_30' should expire */
+
+        /* T+120+35= T+155: We press 'D', this should trigger 'combo_40' and send the keycode 'Z'. We wait 15ms */
+        press_D_and_wait(15)
+
+
+
+        /*** Cleanup ***/
+        /* T+120+50= T+170: We release both keys */
+        release_A_and_wait(20)
+        release_D_and_wait(0)
+    >;
+};