Merge pull request #877 from keyboardio/debouncer-cleanup

ATmega debouncer cleanup

docs/api-reference/device-apis.md

@@ -223,15 +223,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/device/kbdfans/KBD4x.cpp

@@ -42,15 +42,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/device/keyboardio/Atreus2.cpp

@@ -43,15 +43,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/device/keyboardio/Imago.cpp

@@ -69,15 +69,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/device/olkb/Planck.cpp

@@ -41,15 +41,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/device/softhruf/Splitography.cpp

@@ -48,15 +48,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/device/technomancy/Atreus.cpp

@@ -51,15 +51,9 @@ const uint8_t KeyScannerProps::matrix_columns;
 constexpr uint8_t KeyScannerProps::matrix_row_pins[matrix_rows];
 constexpr uint8_t KeyScannerProps::matrix_col_pins[matrix_columns];
 
-// Resolving is a bit different in case of templates, however: the name of the
+// `KeyScanner` here refers to the alias set up above, just like in the
-// array is resolved within the scope of the namespace and the class, but the
+// `KeyScannerProps` case above.
-// array size is not - because it is a template. Therefore, we need a fully
+template<> KeyScanner::row_state_t KeyScanner::matrix_state_[KeyScannerProps::matrix_rows] = {};
-// qualified name there - or an alias in the global scope, which we set up just
-// above.
-template<> uint16_t KeyScanner::previousKeyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::keyState_[KeyScannerProps::matrix_rows] = {};
-template<> uint16_t KeyScanner::masks_[KeyScannerProps::matrix_rows] = {};
-template<> uint8_t KeyScanner::debounce_matrix_[KeyScannerProps::matrix_rows][KeyScannerProps::matrix_columns] = {};
 
 // We set up the TIMER1 interrupt vector here. Due to dependency reasons, this
 // cannot be in a header-only driver, and must be placed here.

src/kaleidoscope/driver/keyscanner/ATmega.h

@@ -1,6 +1,6 @@
 /* -*- mode: c++ -*-
  * kaleidoscope::driver::keyscanner::ATmega -- AVR ATmega-based keyscanner component
- * Copyright (C) 2018-2019  Keyboard.io, Inc
+ * Copyright (C) 2018-2020  Keyboard.io, Inc
  *
  * This program is free software: you can redistribute it and/or modify it under
  * the terms of the GNU General Public License as published by the Free Software
@@ -34,9 +34,8 @@ namespace driver {
 namespace keyscanner {
 
 struct ATmegaProps: kaleidoscope::driver::keyscanner::BaseProps {
-  static const uint8_t debounce = 3;
   static const uint16_t keyscan_interval = 1700;
-
+  typedef uint16_t RowState;
 
   /*
    * The following two lines declare an empty array. Both of these must be
@@ -91,20 +90,25 @@ class ATmega: public kaleidoscope::driver::keyscanner::Base<_KeyScannerProps> {
     TIMSK1 = _BV(TOIE1);
   }
 
-  void __attribute__((optimize(3))) readMatrix(void) {
+  __attribute__((optimize(3)))
-    for (uint8_t current_row = 0; current_row < _KeyScannerProps::matrix_rows; current_row++) {
+  void readMatrix(void) {
-      uint16_t mask, cols;
+    typename _KeyScannerProps::RowState any_debounced_changes = 0;
-
-      mask = debounceMaskForRow(current_row);
 
+    for (uint8_t current_row = 0; current_row < _KeyScannerProps::matrix_rows; current_row++) {
       OUTPUT_TOGGLE(_KeyScannerProps::matrix_row_pins[current_row]);
-      cols = (readCols() & mask) | (keyState_[current_row] & ~mask);
+      typename _KeyScannerProps::RowState hot_pins = readCols();
+
       OUTPUT_TOGGLE(_KeyScannerProps::matrix_row_pins[current_row]);
-      debounceRow(cols ^ keyState_[current_row], current_row);
+
-      keyState_[current_row] = cols;
+      any_debounced_changes |= debounce(hot_pins, &matrix_state_[current_row].debouncer);
+
+      if (any_debounced_changes) {
+        for (uint8_t current_row = 0; current_row < _KeyScannerProps::matrix_rows; current_row++) {
+          matrix_state_[current_row].current = matrix_state_[current_row].debouncer.debounced_state;
+        }
+      }
     }
   }
-
   void scanMatrix() {
     if (do_scan_) {
       do_scan_ = false;
@@ -116,13 +120,12 @@ class ATmega: public kaleidoscope::driver::keyscanner::Base<_KeyScannerProps> {
   void __attribute__((optimize(3))) actOnMatrixScan() {
     for (byte row = 0; row < _KeyScannerProps::matrix_rows; row++) {
       for (byte col = 0; col < _KeyScannerProps::matrix_columns; col++) {
-        uint8_t keyState = (bitRead(previousKeyState_[row], col) << 0) |
+        uint8_t keyState = (bitRead(matrix_state_[row].previous, col) << 0) | (bitRead(matrix_state_[row].current, col) << 1);
-                           (bitRead(keyState_[row], col) << 1);
         if (keyState) {
           ThisType::handleKeyswitchEvent(Key_NoKey, typename _KeyScannerProps::KeyAddr(row, col), keyState);
         }
       }
-      previousKeyState_[row] = keyState_[row];
+      matrix_state_[row].previous = matrix_state_[row].current;
     }
   }
 
@@ -130,25 +133,24 @@ class ATmega: public kaleidoscope::driver::keyscanner::Base<_KeyScannerProps> {
     uint8_t count = 0;
 
     for (int8_t r = 0; r < _KeyScannerProps::matrix_rows; r++) {
-      count += __builtin_popcount(keyState_[r]);
+      count += __builtin_popcount(matrix_state_[r].current);
     }
     return count;
   }
   bool isKeyswitchPressed(typename _KeyScannerProps::KeyAddr key_addr) {
-    return (bitRead(keyState_[key_addr.row()],
+    return (bitRead(matrix_state_[key_addr.row()].current, key_addr.col()) != 0);
-                    key_addr.col()) != 0);
   }
 
   uint8_t previousPressedKeyswitchCount() {
     uint8_t count = 0;
 
     for (int8_t r = 0; r < _KeyScannerProps::matrix_rows; r++) {
-      count += __builtin_popcount(previousKeyState_[r]);
+      count += __builtin_popcount(matrix_state_[r].previous);
     }
     return count;
   }
   bool wasKeyswitchPressed(typename _KeyScannerProps::KeyAddr key_addr) {
-    return (bitRead(previousKeyState_[key_addr.row()],
+    return (bitRead(matrix_state_[key_addr.row()].previous,
                     key_addr.col()) != 0);
   }
 
@@ -156,30 +158,48 @@ class ATmega: public kaleidoscope::driver::keyscanner::Base<_KeyScannerProps> {
     if (!key_addr.isValid())
       return;
 
-    bitWrite(masks_[key_addr.row()], key_addr.col(), 1);
+    bitWrite(matrix_state_[key_addr.row()].masks, key_addr.col(), 1);
   }
   void unMaskKey(typename _KeyScannerProps::KeyAddr key_addr) {
     if (!key_addr.isValid())
       return;
 
-    bitWrite(masks_[key_addr.row()], key_addr.col(), 0);
+    bitWrite(matrix_state_[key_addr.row()].masks, key_addr.col(), 0);
   }
   bool isKeyMasked(typename _KeyScannerProps::KeyAddr key_addr) {
     if (!key_addr.isValid())
       return false;
 
-    return bitRead(masks_[key_addr.row()],
+    return bitRead(matrix_state_[key_addr.row()].masks,
                    key_addr.col());
   }
 
   bool do_scan_;
 
+
+ protected:
+  /*
+    each of these variables are storing the state for a row of keys
+
+    so for key 0, the counter is represented by db0[0] and db1[0]
+    and the state in debounced_state[0].
+  */
+  struct debounce_t {
+    typename _KeyScannerProps::RowState db0;    // counter bit 0
+    typename _KeyScannerProps::RowState db1;    // counter bit 1
+    typename _KeyScannerProps::RowState debounced_state;  // debounced state
+  };
+
+  struct row_state_t {
+    typename _KeyScannerProps::RowState previous;
+    typename _KeyScannerProps::RowState current;
+    typename _KeyScannerProps::RowState masks;
+    debounce_t debouncer;
+  };
+
  private:
   typedef _KeyScannerProps KeyScannerProps_;
-  static uint16_t previousKeyState_[_KeyScannerProps::matrix_rows];
+  static row_state_t matrix_state_[_KeyScannerProps::matrix_rows];
-  static uint16_t keyState_[_KeyScannerProps::matrix_rows];
-  static uint16_t masks_[_KeyScannerProps::matrix_rows];
-  static uint8_t debounce_matrix_[_KeyScannerProps::matrix_rows][_KeyScannerProps::matrix_columns];
 
   /*
    * This function has loop unrolling disabled on purpose: we want to give the
@@ -195,34 +215,39 @@ class ATmega: public kaleidoscope::driver::keyscanner::Base<_KeyScannerProps> {
    * Do not remove the attribute!
    */
   __attribute__((optimize("no-unroll-loops")))
-  uint16_t readCols() {
+  typename _KeyScannerProps::RowState readCols() {
-    uint16_t results = 0x00 ;
+    typename _KeyScannerProps::RowState hot_pins = 0;
     for (uint8_t i = 0; i < _KeyScannerProps::matrix_columns; i++) {
       asm("NOP"); // We need to pause a beat before reading or we may read before the pin is hot
-      results |= (!READ_PIN(_KeyScannerProps::matrix_col_pins[i]) << i);
+      hot_pins |= (!READ_PIN(_KeyScannerProps::matrix_col_pins[i]) << i);
     }
-    return results;
+
+    return hot_pins;
   }
 
-  uint16_t debounceMaskForRow(uint8_t row) {
+  static inline typename _KeyScannerProps::RowState debounce(
-    uint16_t result = 0;
+    typename _KeyScannerProps::RowState sample, debounce_t *debouncer
+  ) {
+    typename _KeyScannerProps::RowState delta, changes;
 
-    for (uint16_t c = 0; c < _KeyScannerProps::matrix_columns; ++c) {
+    // Use xor to detect changes from last stable state:
-      if (debounce_matrix_[row][c]) {
+    // if a key has changed, it's bit will be 1, otherwise 0
-        --debounce_matrix_[row][c];
+    delta = sample ^ debouncer->debounced_state;
-      } else {
-        result |= _BV(c);
-      }
-    }
-    return result;
-  }
 
-  void debounceRow(uint16_t change, uint8_t row) {
+    // Increment counters and reset any unchanged bits:
-    for (uint16_t i = 0; i < _KeyScannerProps::matrix_columns; ++i) {
+    // increment bit 1 for all changed keys
-      if (change & _BV(i)) {
+    debouncer->db1 = ((debouncer->db1) ^ (debouncer->db0)) & delta;
-        debounce_matrix_[row][i] = _KeyScannerProps::debounce;
+    // increment bit 0 for all changed keys
-      }
+    debouncer->db0 = ~(debouncer->db0) & delta;
-    }
+
+    // Calculate returned change set: if delta is still true
+    // and the counter has wrapped back to 0, the key is changed.
+
+    changes = ~(~delta | (debouncer->db0) | (debouncer->db1));
+    // Update state: in this case use xor to flip any bit that is true in changes.
+    debouncer->debounced_state ^= changes;
+
+    return changes;
   }
 };
 #else // ifndef KALEIDOSCOPE_VIRTUAL_BUILD