Improve debounce testing and fixes

* Add tests for none
* Add check for consistency between cooked_changed and actual result
* Add check for ensuring debounce never reads the timer more than once
* Fix sym_eager_pr incorrectly returning cooked_changed as false
* Fix sym_defer_g reading the timer twice when debouncing
* Save some useless memory copies on none when there are no changes
* Restructure sym_defer_g so it is smaller on AVR

platforms/test/timer.c

@@ -15,19 +15,69 @@
  */
 
 #include "timer.h"
+#include <stdatomic.h>
 
-static uint32_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 timer_init(void) { current_time = 0; }
+void simulate_async_tick(uint32_t t) {
+    async_tick_amount = t;
+}
 
-void timer_clear(void) { current_time = 0; }
+uint32_t timer_read_internal(void) {
+    return current_time;
+}
 
-uint16_t timer_read(void) { return current_time & 0xFFFF; }
-uint32_t timer_read32(void) { 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); }
+uint32_t current_access_counter(void) {
+    return access_counter;
+}
 
-void set_time(uint32_t t) { current_time = t; }
-void advance_time(uint32_t ms) { current_time += ms; }
+void reset_access_counter(void) {
+    access_counter = 0;
+}
 
-void wait_ms(uint32_t ms) { advance_time(ms); }
+void timer_init(void) {
+    current_time      = 0;
+    async_tick_amount = 0;
+    access_counter    = 0;
+}
+
+void timer_clear(void) {
+    current_time      = 0;
+    async_tick_amount = 0;
+    access_counter    = 0;
+}
+
+uint16_t timer_read(void) {
+    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;
+    access_counter = 0;
+}
+
+void advance_time(uint32_t ms) {
+    current_time += ms;
+    access_counter = 0;
+}
+
+void wait_ms(uint32_t ms) {
+    advance_time(ms);
+}

quantum/debounce/none.c

@@ -13,19 +13,25 @@
  * You should have received a copy of the GNU General Public License
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
+#include "debounce.h"
+#include <string.h>
 
-#include "matrix.h"
-#include "quantum.h"
-#include <stdlib.h>
 
 void debounce_init(uint8_t num_rows) {}
 
-void debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
-    for (int i = 0; i < num_rows; i++) {
-        cooked[i] = raw[i];
+bool debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
+    bool cooked_changed = false;
+
+    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;
 }
 
-bool debounce_active(void) { return false; }
 
 void debounce_free(void) {}

quantum/debounce/sym_defer_g.c

@@ -17,36 +17,52 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 Basic global debounce algorithm. Used in 99% of keyboards at time of implementation
 When no state changes have occured for DEBOUNCE milliseconds, we push the state.
 */
-#include "matrix.h"
+#include "debounce.h"
 #include "timer.h"
-#include "quantum.h"
+#include <string.h>
 #ifndef DEBOUNCE
 #    define DEBOUNCE 5
 #endif
 
+// Maximum debounce: 255ms
+#if DEBOUNCE > UINT8_MAX
+#    undef DEBOUNCE
+#    define DEBOUNCE UINT8_MAX
+#endif
+
+typedef uint8_t debounce_counter_t;
+
 #if DEBOUNCE > 0
-static bool         debouncing = false;
-static fast_timer_t debouncing_time;
+
+#    define DEBOUNCE_ELAPSED 0
+
+static debounce_counter_t debounce_counter = DEBOUNCE_ELAPSED;
+
+static fast_timer_t last_time;
 
 void debounce_init(uint8_t num_rows) {}
 
-void debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
-    if (changed) {
-        debouncing      = true;
-        debouncing_time = timer_read_fast();
-    }
+bool debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
+    bool cooked_changed = false;
 
-    if (debouncing && timer_elapsed_fast(debouncing_time) >= DEBOUNCE) {
-        for (int i = 0; i < num_rows; i++) {
-            cooked[i] = raw[i];
+    if (changed) {
+        debounce_counter = DEBOUNCE;
+        last_time        = timer_read_fast();
+    } else if (debounce_counter != DEBOUNCE_ELAPSED) {
+        if (debounce_counter <= timer_elapsed_fast(last_time)) {
+            debounce_counter   = DEBOUNCE_ELAPSED;
+            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;
     }
-}
 
-bool debounce_active(void) { return debouncing; }
+    return cooked_changed;
+}
 
 void debounce_free(void) {}
-#else  // no debouncing.
+#else // no debouncing.
 #    include "none.c"
 #endif

quantum/debounce/sym_eager_pr.c

@@ -19,9 +19,8 @@ After pressing a key, it immediately changes state, and sets a counter.
 No further inputs are accepted until DEBOUNCE milliseconds have occurred.
 */
 
-#include "matrix.h"
+#include "debounce.h"
 #include "timer.h"
-#include "quantum.h"
 #include <stdlib.h>
 
 #ifdef PROTOCOL_CHIBIOS
@@ -48,6 +47,7 @@ static bool matrix_need_update;
 static debounce_counter_t *debounce_counters;
 static fast_timer_t        last_time;
 static bool                counters_need_update;
+static bool                cooked_changed;
 
 #    define DEBOUNCE_ELAPSED 0
 
@@ -67,8 +67,9 @@ void debounce_free(void) {
     debounce_counters = NULL;
 }
 
-void debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
+bool debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool changed) {
     bool updated_last = false;
+    cooked_changed    = false;
 
     if (counters_need_update) {
         fast_timer_t now          = timer_read_fast();
@@ -92,6 +93,8 @@ void debounce(matrix_row_t raw[], matrix_row_t cooked[], uint8_t num_rows, bool
 
         transfer_matrix_values(raw, cooked, num_rows);
     }
+
+    return cooked_changed;
 }
 
 // If the current time is > debounce counter, set the counter to enable input.
@@ -124,7 +127,8 @@ static void transfer_matrix_values(matrix_row_t raw[], matrix_row_t cooked[], ui
         // determine new value basd on debounce pointer + raw value
         if (existing_row != raw_row) {
             if (*debounce_pointer == DEBOUNCE_ELAPSED) {
-                *debounce_pointer    = DEBOUNCE;
+                *debounce_pointer = DEBOUNCE;
+                cooked_changed |= cooked[row] ^ raw_row;
                 cooked[row]          = raw_row;
                 counters_need_update = true;
             }
@@ -133,7 +137,6 @@ static void transfer_matrix_values(matrix_row_t raw[], matrix_row_t cooked[], ui
     }
 }
 
-bool debounce_active(void) { return true; }
 #else
 #    include "none.c"
 #endif

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

@@ -23,15 +23,20 @@
 #include <sstream>
 
 extern "C" {
-#include "quantum.h"
-#include "timer.h"
 #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) { events_.insert(events_.end(), events.begin(), events.end()); }
+void DebounceTest::addEvents(std::initializer_list<DebounceTestEvent> events) {
+    events_.insert(events_.end(), events.begin(), events.end());
+}
 
 void DebounceTest::runEvents() {
     /* Run the test multiple times, from 1kHz to 10kHz scan rate */
@@ -57,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);
 
@@ -69,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);
@@ -123,11 +129,21 @@ 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_));
 
-    debounce(raw_matrix_, cooked_matrix_, MATRIX_ROWS, changed);
+    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() reported a wrong cooked matrix change result " << 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" << strTime() << "\ntimer: access_count=" << current_access_counter() << "\noutput_matrix: cooked_changed=" << cooked_changed << "\n" << strMatrix(output_matrix_) << "\ncooked_matrix:\n" << strMatrix(cooked_matrix_);
+    }
 }
 
 void DebounceTest::checkCookedMatrix(bool changed, const std::string &error_message) {
@@ -139,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);