From a2a5fb166e1e400270004f648d5114e2943ab5a8 Mon Sep 17 00:00:00 2001
From: Mark Thompson <129641948+NotherNgineer@users.noreply.github.com>
Date: Wed, 31 Jan 2024 14:13:21 -0600
Subject: [PATCH] Improved Debounce for Encoders (#1837)

* Fix variable type declaration

* Fix typo

* Two-bit encoder debouncing

* Slight optimization

* Comment change
---
 firmware/application/hw/debounce.cpp  | 23 ++++++++++++++++++++++-
 firmware/application/hw/debounce.hpp  | 12 ++++++++++++
 firmware/application/hw/encoder.cpp   |  9 ++-------
 firmware/application/hw/encoder.hpp   |  4 +---
 firmware/application/irq_controls.cpp | 12 ++++--------
 5 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/firmware/application/hw/debounce.cpp b/firmware/application/hw/debounce.cpp
index 1fb4751b..671b872f 100644
--- a/firmware/application/hw/debounce.cpp
+++ b/firmware/application/hw/debounce.cpp
@@ -25,7 +25,7 @@
 #include "utility.hpp"
 
 uint8_t Debounce::state() {
-    bool v = state_to_report_;
+    uint8_t v = state_to_report_;
     simulated_pulse_ = false;
     return v;
 }
@@ -149,3 +149,24 @@ bool Debounce::feed(const uint8_t bit) {
 
     return false;
 }
+
+uint8_t EncoderDebounce::state() {
+    return state_;
+}
+
+// Returns TRUE if encoder position phase bits changed (after debouncing)
+bool EncoderDebounce::feed(const uint8_t phase_bits) {
+    history_ = (history_ << 2) | phase_bits;
+
+    // Has input been constant for 4 ticks? (phase_bits * 01010101b should be 0x00, 0x55, 0xAA, or 0xFF)
+    if (history_ == (phase_bits * 0x55)) {
+        // Has the debounced input value changed?
+        if (state_ != phase_bits) {
+            state_ = phase_bits;
+            return true;
+        }
+    }
+
+    // Unstable input, or no change
+    return false;
+}
diff --git a/firmware/application/hw/debounce.hpp b/firmware/application/hw/debounce.hpp
index 41ccb9da..6bf1033a 100644
--- a/firmware/application/hw/debounce.hpp
+++ b/firmware/application/hw/debounce.hpp
@@ -64,4 +64,16 @@ class Debounce {
     bool long_press_occurred_{false};  // TRUE when button is being held down and LONG_PRESS_DELAY has been reached (only when long_press_enabled)
 };
 
+class EncoderDebounce {
+   public:
+    bool feed(const uint8_t phase_bits);  // returns TRUE if state changed after debouncing
+
+    uint8_t state();  // returns debounced phase bits from encoder
+
+   private:
+    uint8_t history_{0};  // shift register of previous reads from encoder
+
+    uint8_t state_{0};  // actual encoder output state (after debounce logic)
+};
+
 #endif /*__DEBOUNCE_H__*/
diff --git a/firmware/application/hw/encoder.cpp b/firmware/application/hw/encoder.cpp
index 0bb427c9..2668a69b 100644
--- a/firmware/application/hw/encoder.cpp
+++ b/firmware/application/hw/encoder.cpp
@@ -94,13 +94,8 @@ static const int8_t transition_map[][16] = {
     },
 };
 
-int_fast8_t Encoder::update(
-    const uint_fast8_t phase_0,
-    const uint_fast8_t phase_1) {
-    state <<= 1;
-    state |= phase_0;
-    state <<= 1;
-    state |= phase_1;
+int_fast8_t Encoder::update(const uint_fast8_t phase_bits) {
+    state = (state << 2) | phase_bits;
 
     // dial sensitivity setting is stored in pmem
     return transition_map[portapack::persistent_memory::config_encoder_dial_sensitivity()][state & 0xf];
diff --git a/firmware/application/hw/encoder.hpp b/firmware/application/hw/encoder.hpp
index 3436e071..aa839b4b 100644
--- a/firmware/application/hw/encoder.hpp
+++ b/firmware/application/hw/encoder.hpp
@@ -26,9 +26,7 @@
 
 class Encoder {
    public:
-    int_fast8_t update(
-        const uint_fast8_t phase_0,
-        const uint_fast8_t phase_1);
+    int_fast8_t update(const uint_fast8_t phase_bits);
 
    private:
     uint_fast8_t state{0};
diff --git a/firmware/application/irq_controls.cpp b/firmware/application/irq_controls.cpp
index 68994cac..57db43d3 100644
--- a/firmware/application/irq_controls.cpp
+++ b/firmware/application/irq_controls.cpp
@@ -43,7 +43,7 @@ static Thread* thread_controls_event = NULL;
 
 // Index with the Switch enum.
 static std::array<Debounce, 6> switch_debounce;
-static std::array<Debounce, 2> encoder_debounce;
+static EncoderDebounce encoder_debounce;
 
 static_assert(std::size(switch_debounce) == toUType(Switch::Dfu) + 1);
 
@@ -162,16 +162,12 @@ static bool switches_update(const uint8_t raw) {
 }
 
 static bool encoder_update(const uint8_t raw) {
-    bool encoder_changed = false;
-
-    encoder_changed |= encoder_debounce[0].feed((raw >> 6) & 0x01);
-    encoder_changed |= encoder_debounce[1].feed((raw >> 7) & 0x01);
-
-    return encoder_changed;
+    // TODO: swap +1/-1 directions in encoder.update() to avoid needless swizzing of phase bits here
+    return encoder_debounce.feed(((raw >> 7) & 0x01) | ((raw >> 5) & 0x02));
 }
 
 static bool encoder_read() {
-    auto delta = encoder.update(encoder_debounce[0].state(), encoder_debounce[1].state());
+    auto delta = encoder.update(encoder_debounce.state());
 
     if (injected_encoder > 0) {
         if (injected_encoder == 1) delta = -1;