Switch process_combo to using global register and timer (#2561)

Since combos keep local state about what keys have been previously pressed, when combos are layered, multiple keypresses will register for any key with multiple combos assigned to it. In order to fix this, I switched process_combo to use a global keycode / keyrecord register and timer. When a keypress is consumed by a combo, it gets stored in the register and the timer is updated; when the next keypress takes too long or a key is pressed that isn't part of any combo, the buffer is emitted and the timer reset. This has a few side effects. For instance, I couldn't _not_ fix combo keys printing out of order while also fixing this bug, so combo keys print in order correctly when a combo fails. since combos no longer have local timers, the logic around when combos time out has changed. now that there is a single timer pressing any combo key (including one in a different combo) will reset the timer for all combos, making combo entry a little more lenient. Since combos no longer have local keycode / keyrecord state, there is an edge case where incomplete combo keys can be consumed. if you have a combo for a+s = tab and a combo for b+n = space, if you press a+b+n, only a space will be emitted. This is because when b+n completes successfully, it drops the register.

quantum/process_keycode/process_combo.c

@@ -14,141 +14,164 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "process_combo.h"
 #include "print.h"
+#include "process_combo.h"
 
-
-__attribute__ ((weak))
-combo_t key_combos[COMBO_COUNT] = {
+__attribute__((weak)) combo_t key_combos[COMBO_COUNT] = {
 
 };
 
-__attribute__ ((weak))
-void process_combo_event(uint8_t combo_index, bool pressed) {
-
-}
+__attribute__((weak)) void process_combo_event(uint8_t combo_index,
+                                               bool pressed) {}
 
+static uint16_t timer = 0;
 static uint8_t current_combo_index = 0;
+static bool drop_buffer = false;
+static bool is_active = false;
 
-static inline void send_combo(uint16_t action, bool pressed)
-{
-    if (action) {
-        if (pressed) {
-            register_code16(action);
-        } else {
-            unregister_code16(action);
-        }
-    } else {
-        process_combo_event(current_combo_index, pressed);
-    }
-}
-
-#define ALL_COMBO_KEYS_ARE_DOWN     (((1<<count)-1) == combo->state)
-#define NO_COMBO_KEYS_ARE_DOWN      (0 == combo->state)
-#define KEY_STATE_DOWN(key)         do{ combo->state |= (1<<key); } while(0)
-#define KEY_STATE_UP(key)           do{ combo->state &= ~(1<<key); } while(0)
-static bool process_single_combo(combo_t *combo, uint16_t keycode, keyrecord_t *record) 
-{
-    uint8_t count = 0;
-    uint8_t index = -1;
-    /* Find index of keycode and number of combo keys */
-    for (const uint16_t *keys = combo->keys; ;++count) {
-        uint16_t key = pgm_read_word(&keys[count]);
-        if (keycode == key) index = count;
-        if (COMBO_END == key) break;
-    }
-
-    /* Return if not a combo key */
-    if (-1 == (int8_t)index) return false;
-
-    /* The combos timer is used to signal whether the combo is active */
-    bool is_combo_active = combo->is_active;
-
-    if (record->event.pressed) {
-        KEY_STATE_DOWN(index);
-
-        if (is_combo_active) {
-            if (ALL_COMBO_KEYS_ARE_DOWN) { /* Combo was pressed */
-                send_combo(combo->keycode, true);
-                combo->is_active = false;
-            } else { /* Combo key was pressed */
-                combo->timer = timer_read();
-                combo->is_active = true;
+static uint8_t buffer_size = 0;
 #ifdef COMBO_ALLOW_ACTION_KEYS
-                combo->prev_record = *record;
+static keyrecord_t key_buffer[MAX_COMBO_LENGTH];
 #else
-                combo->prev_key = keycode;
+static uint16_t key_buffer[MAX_COMBO_LENGTH];
 #endif
-            }
-        }
+
+static inline void send_combo(uint16_t action, bool pressed) {
+  if (action) {
+    if (pressed) {
+      register_code16(action);
     } else {
-        if (ALL_COMBO_KEYS_ARE_DOWN) { /* Combo was released */
-            send_combo(combo->keycode, false);
-        }
+      unregister_code16(action);
+    }
+  } else {
+    process_combo_event(current_combo_index, pressed);
+  }
+}
 
-        if (is_combo_active) { /* Combo key was tapped */
+static inline void dump_key_buffer(bool emit) {
+  if (buffer_size == 0) {
+    return;
+  }
+
+  if (emit) {
+    for (uint8_t i = 0; i < buffer_size; i++) {
 #ifdef COMBO_ALLOW_ACTION_KEYS
-            record->event.pressed = true;
-            process_action(record, store_or_get_action(record->event.pressed, record->event.key));
-            record->event.pressed = false;
-            process_action(record, store_or_get_action(record->event.pressed, record->event.key));
+      const action_t action = store_or_get_action(key_buffer[i].event.pressed,
+                                                  key_buffer[i].event.key);
+      process_action(&(key_buffer[i]), action);
 #else
-            register_code16(keycode);
-            send_keyboard_report();
-            unregister_code16(keycode);
+      register_code16(key_buffer[i]);
+      send_keyboard_report();
 #endif
-            combo->is_active = false;
-            combo->timer = 0;            
-        }
-
-        KEY_STATE_UP(index);        
     }
+  }
 
-    if (NO_COMBO_KEYS_ARE_DOWN) {
-        combo->is_active = true;
-        combo->timer = 0;
-    }
-
-    return is_combo_active;
+  buffer_size = 0;
 }
 
-bool process_combo(uint16_t keycode, keyrecord_t *record)
-{
-    bool is_combo_key = false;
+#define ALL_COMBO_KEYS_ARE_DOWN (((1 << count) - 1) == combo->state)
+#define KEY_STATE_DOWN(key)                                                    \
+  do {                                                                         \
+    combo->state |= (1 << key);                                                \
+  } while (0)
+#define KEY_STATE_UP(key)                                                      \
+  do {                                                                         \
+    combo->state &= ~(1 << key);                                               \
+  } while (0)
+
+static bool process_single_combo(combo_t *combo, uint16_t keycode,
+                                 keyrecord_t *record) {
+  uint8_t count = 0;
+  uint8_t index = -1;
+  /* Find index of keycode and number of combo keys */
+  for (const uint16_t *keys = combo->keys;; ++count) {
+    uint16_t key = pgm_read_word(&keys[count]);
+    if (keycode == key)
+      index = count;
+    if (COMBO_END == key)
+      break;
+  }
+
+  /* Continue processing if not a combo key */
+  if (-1 == (int8_t)index)
+    return false;
+
+  bool is_combo_active = is_active;
+
+  if (record->event.pressed) {
+    KEY_STATE_DOWN(index);
+
+    if (is_combo_active) {
+      if (ALL_COMBO_KEYS_ARE_DOWN) { /* Combo was pressed */
+        send_combo(combo->keycode, true);
+        drop_buffer = true;
+      }
+    }
+  } else {
+    if (ALL_COMBO_KEYS_ARE_DOWN) { /* Combo was released */
+      send_combo(combo->keycode, false);
+    } else {
+      /* continue processing without immediately returning */
+      is_combo_active = false;
+    }
 
-    for (current_combo_index = 0; current_combo_index < COMBO_COUNT; ++current_combo_index) {
-        combo_t *combo = &key_combos[current_combo_index];
-        is_combo_key |= process_single_combo(combo, keycode, record);
-    }    
+    KEY_STATE_UP(index);
+  }
 
-    return !is_combo_key;
+  return is_combo_active;
 }
 
-void matrix_scan_combo(void)
-{
-    for (int i = 0; i < COMBO_COUNT; ++i) {
-        // Do not treat the (weak) key_combos too strict.
-        #pragma GCC diagnostic push
-        #pragma GCC diagnostic ignored "-Warray-bounds"
-        combo_t *combo = &key_combos[i];
-        #pragma GCC diagnostic pop
-        if (combo->is_active &&
-            combo->timer &&
-            timer_elapsed(combo->timer) > COMBO_TERM) {
-
-            /* This disables the combo, meaning key events for this
-             * combo will be handled by the next processors in the chain 
-             */
-            combo->is_active = false;
+#define NO_COMBO_KEYS_ARE_DOWN (0 == combo->state)
+
+bool process_combo(uint16_t keycode, keyrecord_t *record) {
+  bool is_combo_key = false;
+  drop_buffer = false;
+  bool no_combo_keys_pressed = false;
+
+  for (current_combo_index = 0; current_combo_index < COMBO_COUNT;
+       ++current_combo_index) {
+    combo_t *combo = &key_combos[current_combo_index];
+    is_combo_key |= process_single_combo(combo, keycode, record);
+    no_combo_keys_pressed |= NO_COMBO_KEYS_ARE_DOWN;
+  }
+
+  if (drop_buffer) {
+    /* buffer is only dropped when we complete a combo, so we refresh the timer
+     * here */
+    timer = timer_read();
+    dump_key_buffer(false);
+  } else if (!is_combo_key) {
+    /* if no combos claim the key we need to emit the keybuffer */
+    dump_key_buffer(true);
+
+    // reset state if there are no combo keys pressed at all
+    if (no_combo_keys_pressed) {
+      timer = 0;
+      is_active = true;
+    }
+  } else if (record->event.pressed && is_active) {
+    /* otherwise the key is consumed and placed in the buffer */
+    timer = timer_read();
 
+    if (buffer_size < MAX_COMBO_LENGTH) {
 #ifdef COMBO_ALLOW_ACTION_KEYS
-            process_action(&combo->prev_record, 
-                store_or_get_action(combo->prev_record.event.pressed, 
-                                    combo->prev_record.event.key));
+      key_buffer[buffer_size++] = *record;
 #else
-            unregister_code16(combo->prev_key);
-            register_code16(combo->prev_key);
+      key_buffer[buffer_size++] = keycode;
 #endif
-        }
     }
+  }
+
+  return !is_combo_key;
+}
+
+void matrix_scan_combo(void) {
+  if (is_active && timer && timer_elapsed(timer) > COMBO_TERM) {
+
+    /* This disables the combo, meaning key events for this
+     * combo will be handled by the next processors in the chain
+     */
+    is_active = false;
+    dump_key_buffer(true);
+  }
 }

quantum/process_keycode/process_combo.h

@@ -17,33 +17,34 @@
 #ifndef PROCESS_COMBO_H
 #define PROCESS_COMBO_H
 
-#include <stdint.h>
 #include "progmem.h"
 #include "quantum.h"
+#include <stdint.h>
 
-typedef struct
-{
-    const uint16_t *keys;
-    uint16_t keycode;        
 #ifdef EXTRA_EXTRA_LONG_COMBOS
-    uint32_t state;
+#define MAX_COMBO_LENGTH 32
 #elif EXTRA_LONG_COMBOS
-    uint16_t state;
+#define MAX_COMBO_LENGTH 16
 #else
-    uint8_t state;
+#define MAX_COMBO_LENGTH 8
 #endif
-    uint16_t timer;
-    bool is_active;
-#ifdef COMBO_ALLOW_ACTION_KEYS
-    keyrecord_t prev_record;
+
+typedef struct {
+  const uint16_t *keys;
+  uint16_t keycode;
+#ifdef EXTRA_EXTRA_LONG_COMBOS
+  uint32_t state;
+#elif EXTRA_LONG_COMBOS
+  uint16_t state;
 #else
-    uint16_t prev_key;
+  uint8_t state;
 #endif
 } combo_t;
 
-
-#define COMBO(ck, ca)       {.keys = &(ck)[0], .keycode = (ca)}
-#define COMBO_ACTION(ck)    {.keys = &(ck)[0]}
+#define COMBO(ck, ca)                                                          \
+  { .keys = &(ck)[0], .keycode = (ca) }
+#define COMBO_ACTION(ck)                                                       \
+  { .keys = &(ck)[0] }
 
 #define COMBO_END 0
 #ifndef COMBO_COUNT