tap-dance: Major rework, to make it more reliable

This reworks how the tap-dance feature works: instead of one global
state, we have a state for each tap-dance key, so we can cancel them
when another tap-dance key is in flight. This fixes #527.

Since we have a state for each key, we can avoid situation where a keyup
would mess with our global state. This fixes #563.

And while here, we also make sure to fire events only once, and this
fixes #574.

There is one breaking change, though: tap-dance debugging support was
removed, because dumping the whole state would increase the firmware
size too much. Any keymap that made use of this, will have to be
updated (but there's no such keymap in the repo).

Also, there's a nice trick used in this rework: we need to iterate
through tap_dance_actions in a few places, to check for timeouts, and so
on. For this, we'd need to know the size of the array. We can't discover
that at compile-time, because tap-dance gets compiled separately. We'd
like to avoid having to terminate the list with a sentinel value,
because that would require updates to all keymaps that use the feature.
So, we keep track of the highest tap-dance code seen so far, and iterate
until that index.

Signed-off-by: Gergely Nagy <algernon@madhouse-project.org>

keyboards/ergodox/keymaps/algernon/keymap.c

@@ -853,7 +853,7 @@ void ang_tap_dance_ta_reset (qk_tap_dance_state_t *state, void *user_data) {
   td_ta->finished_once = false;
 }
 
-const qk_tap_dance_action_t tap_dance_actions[] = {
+qk_tap_dance_action_t tap_dance_actions[] = {
    [CT_CLN] = ACTION_TAP_DANCE_FN_ADVANCED (NULL, ang_tap_dance_cln_finished, ang_tap_dance_cln_reset)
   ,[CT_MNS] = ACTION_TAP_DANCE_FN_ADVANCED (NULL, ang_tap_dance_mns_finished, ang_tap_dance_mns_reset)
   ,[CT_TA]  = {

keyboards/preonic/keymaps/kinesis/keymap.c

@@ -123,7 +123,7 @@ void music_scale_user(void)
 }
 
 
-const qk_tap_dance_action_t tap_dance_actions[] = {
+qk_tap_dance_action_t tap_dance_actions[] = {
   //Tap once for equal, twice for hyper + X (alfred lock)
   [TD_EQ_LOCK] = ACTION_TAP_DANCE_DOUBLE(KC_EQL,  HYPR(KC_X)),
   //Tap once for minus, twice for time.heals.nothing

quantum/process_keycode/process_tap_dance.c

@@ -1,19 +1,8 @@
 #include "quantum.h"
 #include "action_tapping.h"
 
-static qk_tap_dance_state_t qk_tap_dance_state;
+static uint16_t last_td;
-bool td_debug_enable = false;
+static int8_t highest_td = -1;
-
-#if CONSOLE_ENABLE
-#define td_debug(s) if (td_debug_enable) \
-    { \
-      xprintf ("D:tap_dance:%s:%s = { keycode = %d, count = %d, active = %d, pressed = %d }\n", __FUNCTION__, s, \
-               qk_tap_dance_state.keycode, qk_tap_dance_state.count, \
-               qk_tap_dance_state.active, qk_tap_dance_state.pressed);  \
-    }
-#else
-#define td_debug(s)
-#endif
 
 void qk_tap_dance_pair_finished (qk_tap_dance_state_t *state, void *user_data) {
   qk_tap_dance_pair_t *pair = (qk_tap_dance_pair_t *)user_data;
@@ -44,90 +33,102 @@ static inline void _process_tap_dance_action_fn (qk_tap_dance_state_t *state,
   }
 }
 
-static inline void process_tap_dance_action_on_each_tap (qk_tap_dance_action_t action)
+static inline void process_tap_dance_action_on_each_tap (qk_tap_dance_action_t *action)
 {
-  td_debug("trigger");
+  _process_tap_dance_action_fn (&action->state, action->user_data, action->fn.on_each_tap);
-  _process_tap_dance_action_fn (&qk_tap_dance_state, action.user_data, action.fn.on_each_tap);
 }
 
-static inline void process_tap_dance_action_on_dance_finished (qk_tap_dance_action_t action)
+static inline void process_tap_dance_action_on_dance_finished (qk_tap_dance_action_t *action)
 {
-  td_debug("trigger");
+  if (action->state.finished)
-  _process_tap_dance_action_fn (&qk_tap_dance_state, action.user_data, action.fn.on_dance_finished);
+    return;
+  action->state.finished = true;
+  _process_tap_dance_action_fn (&action->state, action->user_data, action->fn.on_dance_finished);
 }
 
-static inline void process_tap_dance_action_on_reset (qk_tap_dance_action_t action)
+static inline void process_tap_dance_action_on_reset (qk_tap_dance_action_t *action)
 {
-  td_debug("trigger")
+  _process_tap_dance_action_fn (&action->state, action->user_data, action->fn.on_reset);
-  _process_tap_dance_action_fn (&qk_tap_dance_state, action.user_data, action.fn.on_reset);
 }
 
 bool process_tap_dance(uint16_t keycode, keyrecord_t *record) {
-  bool r = true;
   uint16_t idx = keycode - QK_TAP_DANCE;
-  qk_tap_dance_action_t action;
+  qk_tap_dance_action_t *action;
+
+  if (last_td && last_td != keycode) {
+    (&tap_dance_actions[last_td - QK_TAP_DANCE])->state.interrupted = true;
+  }
 
   switch(keycode) {
   case QK_TAP_DANCE ... QK_TAP_DANCE_MAX:
-    action = tap_dance_actions[idx];
+    if ((int16_t)idx > highest_td)
+      highest_td = idx;
+    action = &tap_dance_actions[idx];
 
-    process_tap_dance_action_on_each_tap (action);
+    action->state.keycode = keycode;
-    if (qk_tap_dance_state.keycode && qk_tap_dance_state.keycode != keycode) {
+    action->state.pressed = record->event.pressed;
-      process_tap_dance_action_on_dance_finished (action);
-    } else if (qk_tap_dance_state.active && qk_tap_dance_state.pressed) {
-      reset_tap_dance (&qk_tap_dance_state);
-    } else {
-      r = false;
-    }
-
-    qk_tap_dance_state.active = true;
-    qk_tap_dance_state.pressed = record->event.pressed;
     if (record->event.pressed) {
-      qk_tap_dance_state.keycode = keycode;
+      action->state.count++;
-      qk_tap_dance_state.timer = timer_read ();
+      action->state.timer = timer_read();
-      qk_tap_dance_state.count++;
+
+      if (last_td && last_td != keycode) {
+        qk_tap_dance_action_t *paction = &tap_dance_actions[last_td - QK_TAP_DANCE];
+        paction->state.interrupted = true;
+        process_tap_dance_action_on_dance_finished (paction);
+        reset_tap_dance (&paction->state);
       }
+    }
+    last_td = keycode;
+
     break;
 
   default:
-    if (qk_tap_dance_state.keycode) {
+    if (!record->event.pressed)
-      // if we are here, the tap dance was interrupted by a different key
+      return true;
-      idx = qk_tap_dance_state.keycode - QK_TAP_DANCE;
-      action = tap_dance_actions[idx];
 
-      process_tap_dance_action_on_each_tap (action);
+    if (highest_td == -1)
+      return true;
+
+    for (int i = 0; i <= highest_td; i++) {
+      action = &tap_dance_actions[i];
+      if (action->state.count == 0)
+        continue;
+      action->state.interrupted = true;
       process_tap_dance_action_on_dance_finished (action);
-      reset_tap_dance (&qk_tap_dance_state);
+      reset_tap_dance (&action->state);
-      qk_tap_dance_state.active = false;
     }
     break;
   }
 
-  return r;
+  return true;
 }
 
 void matrix_scan_tap_dance () {
-  if (qk_tap_dance_state.active && timer_elapsed (qk_tap_dance_state.timer) > TAPPING_TERM) {
+  if (highest_td == -1)
-    // if we are here, the tap dance was timed out
+    return;
-    uint16_t idx = qk_tap_dance_state.keycode - QK_TAP_DANCE;
-    qk_tap_dance_action_t action = tap_dance_actions[idx];
 
+  for (int i = 0; i <= highest_td; i++) {
+    qk_tap_dance_action_t *action = &tap_dance_actions[i];
+
+    if (action->state.count && timer_elapsed (action->state.timer) > TAPPING_TERM) {
       process_tap_dance_action_on_dance_finished (action);
-    reset_tap_dance (&qk_tap_dance_state);
+      reset_tap_dance (&action->state);
+    }
   }
 }
 
 void reset_tap_dance (qk_tap_dance_state_t *state) {
-  uint16_t idx = state->keycode - QK_TAP_DANCE;
+  qk_tap_dance_action_t *action;
-  qk_tap_dance_action_t action;
 
   if (state->pressed)
     return;
 
-  action = tap_dance_actions[idx];
+  action = &tap_dance_actions[state->keycode - QK_TAP_DANCE];
+
   process_tap_dance_action_on_reset (action);
 
-  state->keycode = 0;
   state->count = 0;
-  state->active = false;
+  state->interrupted = false;
+  state->finished = false;
+  last_td = 0;
 }

quantum/process_keycode/process_tap_dance.h

@@ -11,8 +11,9 @@ typedef struct
   uint8_t count;
   uint16_t keycode;
   uint16_t timer;
-  bool active:1;
+  bool interrupted;
-  bool pressed:1;
+  bool pressed;
+  bool finished;
 } qk_tap_dance_state_t;
 
 #define TD(n) (QK_TAP_DANCE + n)
@@ -26,6 +27,7 @@ typedef struct
     qk_tap_dance_user_fn_t on_dance_finished;
     qk_tap_dance_user_fn_t on_reset;
   } fn;
+  qk_tap_dance_state_t state;
   void *user_data;
 } qk_tap_dance_action_t;
 
@@ -48,8 +50,7 @@ typedef struct
     .fn = { user_fn_on_each_tap, user_fn_on_dance_finished, user_fn_on_reset } \
   }
 
-extern const qk_tap_dance_action_t tap_dance_actions[];
+extern qk_tap_dance_action_t tap_dance_actions[];
-extern bool td_debug_enable;
 
 /* To be used internally */
 

readme.md

@@ -431,7 +431,7 @@ enum {
 };
 
 //Tap Dance Definitions
-const qk_tap_dance_action_t tap_dance_actions[] = {
+qk_tap_dance_action_t tap_dance_actions[] = {
   //Tap once for Esc, twice for Caps Lock
   [TD_ESC_CAPS]  = ACTION_TAP_DANCE_DOUBLE(KC_ESC, KC_CAPS)
 // Other declarations would go here, separated by commas, if you have them
@@ -517,7 +517,7 @@ void dance_flsh_reset(qk_tap_dance_state_t *state, void *user_data) {
   ergodox_right_led_3_off();
 }
 
-const qk_tap_dance_action_t tap_dance_actions[] = {
+qk_tap_dance_action_t tap_dance_actions[] = {
   [CT_SE]  = ACTION_TAP_DANCE_DOUBLE (KC_SPC, KC_ENT)
  ,[CT_CLN] = ACTION_TAP_DANCE_FN_ADVANCED (NULL, dance_cln_finished, dance_cln_reset)
  ,[CT_EGG] = ACTION_TAP_DANCE_FN (dance_egg)