[Enhancement] Improvements for debounce test coverage + bug fixes for sym_defer_g and sym_eager_pr (qmk#21667)

Co-authored-by: Nebuleon <[email protected]>

Author: 2 people

platforms/test/timer.c

@@ -17,34 +17,65 @@
 #include "timer.h"
 #include <stdatomic.h>
 
-static atomic_uint_least32_t current_time = 0;
+static atomic_uint_least32_t current_time      = 0;
+static atomic_uint_least32_t async_tick_amount = 0;
+static atomic_uint_least32_t access_counter    = 0;
+
+void simulate_async_tick(uint32_t t) {
+    async_tick_amount = t;
+}
+
+uint32_t timer_read_internal(void) {
+    return current_time;
+}
+
+uint32_t current_access_counter(void) {
+    return access_counter;
+}
+
+void reset_access_counter(void) {
+    access_counter = 0;
+}
 
 void timer_init(void) {
-    current_time = 0;
+    current_time      = 0;
+    async_tick_amount = 0;
+    access_counter    = 0;
 }
 
 void timer_clear(void) {
-    current_time = 0;
+    current_time      = 0;
+    async_tick_amount = 0;
+    access_counter    = 0;
 }
 
 uint16_t timer_read(void) {
-    return current_time & 0xFFFF;
+    return (uint16_t)timer_read32();
 }
+
 uint32_t timer_read32(void) {
+    if (access_counter++ > 0) {
+        current_time += async_tick_amount;
+    }
     return current_time;
 }
+
 uint16_t timer_elapsed(uint16_t last) {
     return TIMER_DIFF_16(timer_read(), last);
 }
+
 uint32_t timer_elapsed32(uint32_t last) {
     return TIMER_DIFF_32(timer_read32(), last);
 }
 
 void set_time(uint32_t t) {
-    current_time = t;
+    current_time   = t;
+    access_counter = 0;
 }
+
 void advance_time(uint32_t ms) {
     current_time += ms;
+    access_counter = 0;
 }
 
 void wait_ms(uint32_t ms) {

quantum/debounce/none.c

@@ -20,9 +20,15 @@
 void debounce_init(uint8_t num_rows) {}
 
 bool debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
-    bool cooked_changed = memcmp(raw, cooked, sizeof(matrix_row_t) * num_rows) != 0;
+    bool cooked_changed = false;
 
-    memcpy(cooked, raw, sizeof(matrix_row_t) * num_rows);
+    if (changed) {
+        size_t matrix_size = num_rows * sizeof(matrix_row_t);
+        if (memcmp(cooked, raw, matrix_size) != 0) {
+            memcpy(cooked, raw, matrix_size);
+            cooked_changed = true;
+        }
+    }
 
     return cooked_changed;
 }

quantum/debounce/sym_defer_g.c

@@ -24,6 +24,12 @@ When no state changes have occured for DEBOUNCE milliseconds, we push the state.
 #    define DEBOUNCE 5
 #endif
 
+// Maximum debounce: 255ms
+#if DEBOUNCE > UINT8_MAX
+#    undef DEBOUNCE
+#    define DEBOUNCE UINT8_MAX
+#endif
+
 #if DEBOUNCE > 0
 static bool         debouncing = false;
 static fast_timer_t debouncing_time;
@@ -36,11 +42,10 @@ bool debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool
     if (changed) {
         debouncing      = true;
         debouncing_time = timer_read_fast();
-    }
-
-    if (debouncing && timer_elapsed_fast(debouncing_time) >= DEBOUNCE) {
-        if (memcmp(cooked, raw, sizeof(matrix_row_t) * num_rows) != 0) {
-            memcpy(cooked, raw, sizeof(matrix_row_t) * num_rows);
+    } else if (debouncing && timer_elapsed_fast(debouncing_time) >= DEBOUNCE) {
+        size_t matrix_size = num_rows * sizeof(matrix_row_t);
+        if (memcmp(cooked, raw, matrix_size) != 0) {
+            memcpy(cooked, raw, matrix_size);
             cooked_changed = true;
         }
         debouncing = false;

quantum/debounce/sym_eager_pr.c

@@ -128,8 +128,8 @@ static void transfer_matrix_values(matrix_row_t raw[], matrix_row_t cooked[], ui
         if (existing_row != raw_row) {
             if (*debounce_pointer == DEBOUNCE_ELAPSED) {
                 *debounce_pointer = DEBOUNCE;
-                cooked[row]       = raw_row;
-                cooked_changed |= cooked[row] ^ raw[row];
+                cooked_changed |= cooked[row] ^ raw_row;
+                cooked[row]          = raw_row;
                 counters_need_update = true;
             }
         }

quantum/debounce/tests/asym_eager_defer_pk_tests.cpp

@@ -392,3 +392,32 @@ TEST_F(DebounceTest, OneKeyDelayedScan8) {
     time_jumps_ = true;
     runEvents();
 }
+
+TEST_F(DebounceTest, AsyncTickOneKeyShort1) {
+    addEvents({
+        /* Time, Inputs, Outputs */
+        {0, {{0, 1, DOWN}}, {{0, 1, DOWN}}},
+        /* Release key after 1ms delay */
+        {1, {{0, 1, UP}}, {}},
+
+        /*
+         * Until the eager timer on DOWN is observed to finish, the defer timer
+         * on UP can't start. There's no workaround for this because it's not
+         * possible to debounce an event that isn't being tracked.
+         *
+         * sym_defer_pk has the same problem but the test has to track that the
+         * key changed state so the DOWN timer is always allowed to finish
+         * before starting the UP timer.
+         */
+        {5, {}, {}},
+
+        {10, {}, {{0, 1, UP}}}, /* 5ms+5ms after DOWN at time 0 */
+        /* Press key again after 1ms delay */
+        {11, {{0, 1, DOWN}}, {{0, 1, DOWN}}},
+    });
+    /*
+     * Debounce implementations should never read the timer more than once per invocation
+     */
+    async_time_jumps_ = DEBOUNCE;
+    runEvents();
+}

quantum/debounce/tests/debounce_test_common.cpp

@@ -26,8 +26,12 @@ extern "C" {
 #include "debounce.h"
 #include "timer.h"
 
-void set_time(uint32_t t);
-void advance_time(uint32_t ms);
+void     simulate_async_tick(uint32_t t);
+void     reset_access_counter(void);
+uint32_t current_access_counter(void);
+uint32_t timer_read_internal(void);
+void     set_time(uint32_t t);
+void     advance_time(uint32_t ms);
 }
 
 void DebounceTest::addEvents(std::initializer_list<DebounceTestEvent> events) {
@@ -58,6 +62,7 @@ void DebounceTest::runEventsInternal() {
     /* Initialise keyboard with start time (offset to avoid testing at 0) and all keys UP */
     debounce_init(MATRIX_ROWS);
     set_time(time_offset_);
+    simulate_async_tick(async_time_jumps_);
     std::fill(std::begin(input_matrix_), std::end(input_matrix_), 0);
     std::fill(std::begin(output_matrix_), std::end(output_matrix_), 0);
 
@@ -70,9 +75,9 @@ void DebounceTest::runEventsInternal() {
             advance_time(1);
         } else {
             /* Fast forward to the time for this event, calling debounce() with no changes */
-            ASSERT_LT((time_offset_ + event.time_) - timer_read_fast(), 60000) << "Test tries to advance more than 1 minute of time";
+            ASSERT_LT((time_offset_ + event.time_) - timer_read_internal(), 60000) << "Test tries to advance more than 1 minute of time";
 
-            while (timer_read_fast() != time_offset_ + event.time_) {
+            while (timer_read_internal() != time_offset_ + event.time_) {
                 runDebounce(false);
                 checkCookedMatrix(false, "debounce() modified cooked matrix");
                 advance_time(1);
@@ -124,14 +129,20 @@ void DebounceTest::runDebounce(bool changed) {
     std::copy(std::begin(input_matrix_), std::end(input_matrix_), std::begin(raw_matrix_));
     std::copy(std::begin(output_matrix_), std::end(output_matrix_), std::begin(cooked_matrix_));
 
+    reset_access_counter();
+
     bool cooked_changed = debounce(raw_matrix_, cooked_matrix_, MATRIX_ROWS, changed);
 
     if (!std::equal(std::begin(input_matrix_), std::end(input_matrix_), std::begin(raw_matrix_))) {
         FAIL() << "Fatal error: debounce() modified raw matrix at " << strTime() << "\ninput_matrix: changed=" << changed << "\n" << strMatrix(input_matrix_) << "\nraw_matrix:\n" << strMatrix(raw_matrix_);
     }
 
-    if (std::equal(std::begin(output_matrix_), std::end(output_matrix_), std::begin(cooked_matrix_)) && cooked_changed) {
-        FAIL() << "Fatal error: debounce() did detect a wrong cooked matrix change at " << strTime() << "\noutput_matrix: cooked_changed=" << cooked_changed << "\n" << strMatrix(output_matrix_) << "\ncooked_matrix:\n" << strMatrix(cooked_matrix_);
+    if (std::equal(std::begin(output_matrix_), std::end(output_matrix_), std::begin(cooked_matrix_)) == cooked_changed) {
+        FAIL() << "Fatal error: debounce() reported a wrong cooked matrix change result at " << strTime() << "\noutput_matrix: cooked_changed=" << cooked_changed << "\n" << strMatrix(output_matrix_) << "\ncooked_matrix:\n" << strMatrix(cooked_matrix_);
+    }
+
+    if (current_access_counter() > 1) {
+        FAIL() << "Fatal error: debounce() read the timer multiple times, which is not allowed, at " << strTime() << "\ntimer: access_count=" << current_access_counter() << "\noutput_matrix: cooked_changed=" << cooked_changed << "\n" << strMatrix(output_matrix_) << "\ncooked_matrix:\n" << strMatrix(cooked_matrix_);
     }
 }
 
@@ -144,7 +155,7 @@ void DebounceTest::checkCookedMatrix(bool changed, const std::string &error_mess
 std::string DebounceTest::strTime() {
     std::stringstream text;
 
-    text << "time " << (timer_read_fast() - time_offset_) << " (extra_iterations=" << extra_iterations_ << ", auto_advance_time=" << auto_advance_time_ << ")";
+    text << "time " << (timer_read_internal() - time_offset_) << " (extra_iterations=" << extra_iterations_ << ", auto_advance_time=" << auto_advance_time_ << ")";
 
     return text.str();
 }

quantum/debounce/tests/debounce_test_common.h

@@ -54,8 +54,9 @@ class DebounceTest : public ::testing::Test {
     void addEvents(std::initializer_list<DebounceTestEvent> events);
     void runEvents();
 
-    fast_timer_t time_offset_ = 7777;
-    bool         time_jumps_  = false;
+    fast_timer_t time_offset_      = 7777;
+    bool         time_jumps_       = false;
+    fast_timer_t async_time_jumps_ = 0;
 
    private:
     static bool        directionValue(Direction direction);