From ee2e57d702692907449f4dd9bcdfdd39df5133e0 Mon Sep 17 00:00:00 2001
From: Mark Thompson <129641948+NotherNgineer@users.noreply.github.com>
Date: Thu, 1 Feb 2024 14:13:28 -0600
Subject: [PATCH] 2nd-Level Encoder Debouncing for scrappy encoder dials
 (#1838)

* Experimental encoder debouncing

* Experimental encoder debouncing

* Clang

* Comment changed
---
 firmware/application/hw/encoder.cpp   | 112 ++++++++++----------------
 firmware/application/hw/encoder.hpp   |   1 +
 firmware/application/irq_controls.cpp |   3 +-
 3 files changed, 45 insertions(+), 71 deletions(-)

diff --git a/firmware/application/hw/encoder.cpp b/firmware/application/hw/encoder.cpp
index 2668a69b..89733393 100644
--- a/firmware/application/hw/encoder.cpp
+++ b/firmware/application/hw/encoder.cpp
@@ -26,77 +26,51 @@
 #include "portapack.hpp"
 #include "portapack_persistent_memory.hpp"
 
-// Now supporting multiple levels of rotary encoder dial sensitivity
-//
-// Portapack H2 normally has a 30-step encoder, meaning one step (pulse) every
-// 12 degrees of rotation.
-//
-// For each encoder "pulse" there are 4 state transitions, and we can choose
-// between looking at all of them (high sensitivity), half of them (medium/default),
-// or one quarter of them (low sensitivity).
-static const int8_t transition_map[][16] = {
-    // Normal (Medium) Sensitivity -- default
-    {
-        0,   // 0000: noop
-        0,   // 0001: ccw start
-        0,   // 0010: cw start
-        0,   // 0011: rate
-        1,   // 0100: cw end
-        0,   // 0101: noop
-        0,   // 0110: rate
-        -1,  // 0111: ccw end
-        -1,  // 1000: ccw end
-        0,   // 1001: rate
-        0,   // 1010: noop
-        1,   // 1011: cw end
-        0,   // 1100: rate
-        0,   // 1101: cw start
-        0,   // 1110: ccw start
-        0,   // 1111: noop
-    },
-    // Low Sensitivity
-    {
-        0,   // 0000: noop
-        0,   // 0001: ccw start
-        0,   // 0010: cw start
-        0,   // 0011: rate
-        1,   // 0100: cw end
-        0,   // 0101: noop
-        0,   // 0110: rate
-        0,   // 0111: ccw end
-        -1,  // 1000: ccw end
-        0,   // 1001: rate
-        0,   // 1010: noop
-        0,   // 1011: cw end
-        0,   // 1100: rate
-        0,   // 1101: cw start
-        0,   // 1110: ccw start
-        0,   // 1111: noop
-    },
-    // High Sensitivity
-    {
-        0,   // 0000: noop
-        -1,  // 0001: ccw start
-        1,   // 0010: cw start
-        0,   // 0011: rate
-        1,   // 0100: cw end
-        0,   // 0101: noop
-        0,   // 0110: rate
-        -1,  // 0111: ccw end
-        -1,  // 1000: ccw end
-        0,   // 1001: rate
-        0,   // 1010: noop
-        1,   // 1011: cw end
-        0,   // 1100: rate
-        1,   // 1101: cw start
-        -1,  // 1110: ccw start
-        0,   // 1111: noop
-    },
+// Transition map for rotary encoder phase bits
+//  00 -> 01 -> 11 -> 10 cw
+//  00 -> 10 -> 11 -> 01 ccw
+// NB: Bits are swapped versus older FW versions
+static const int8_t transition_map[16] = {
+    0,   // 00->00: noop
+    1,   // 00->01: cw start
+    -1,  // 00->10: ccw start
+    0,   // 00->11: rate
+    -1,  // 01->00: ccw end
+    0,   // 01->01: noop
+    0,   // 01->10: rate
+    1,   // 01->11: cw end
+    1,   // 10->00: cw end
+    0,   // 10->01: rate
+    0,   // 10->10: noop
+    -1,  // 10->11: ccw end
+    0,   // 11->00: rate
+    -1,  // 11->01: ccw start
+    1,   // 11->10: cw start
+    0,   // 11->11: noop
+};
+
+// Rotary encoder dial sensitivity (transition ignored if bit is 0)
+static const uint16_t sensitivity_map[] = {
+    0x0990,  // DIAL_SENSITIVITY_NORMAL
+    0x0110,  // DIAL_SENSITIVITY_LOW
+    0x6996,  // DIAL_SENSITIVITY_HIGH
 };
 
 int_fast8_t Encoder::update(const uint_fast8_t phase_bits) {
-    state = (state << 2) | phase_bits;
+    state = ((state << 2) | phase_bits) & 0x0F;
 
-    // dial sensitivity setting is stored in pmem
-    return transition_map[portapack::persistent_memory::config_encoder_dial_sensitivity()][state & 0xf];
+    int_fast8_t direction = transition_map[state];
+
+    // Require 2 state changes in same direction to register movement -- for additional level of contact switch debouncing
+    if (direction == prev_direction) {
+        if ((sensitivity_map[portapack::persistent_memory::config_encoder_dial_sensitivity()] & (1 << state)) == 0)
+            return 0;
+        return direction;
+    }
+
+    // It's normal for transition map to return 0 between every +1/-1, so discarding those
+    if (direction != 0)
+        prev_direction = direction;
+
+    return 0;
 }
diff --git a/firmware/application/hw/encoder.hpp b/firmware/application/hw/encoder.hpp
index aa839b4b..8ce02b57 100644
--- a/firmware/application/hw/encoder.hpp
+++ b/firmware/application/hw/encoder.hpp
@@ -30,6 +30,7 @@ class Encoder {
 
    private:
     uint_fast8_t state{0};
+    int_fast8_t prev_direction{0};
 };
 
 #endif /*__ENCODER_H__*/
diff --git a/firmware/application/irq_controls.cpp b/firmware/application/irq_controls.cpp
index 57db43d3..db9d31ef 100644
--- a/firmware/application/irq_controls.cpp
+++ b/firmware/application/irq_controls.cpp
@@ -162,8 +162,7 @@ static bool switches_update(const uint8_t raw) {
 }
 
 static bool encoder_update(const uint8_t raw) {
-    // 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));
+    return encoder_debounce.feed(raw >> 6);
 }
 
 static bool encoder_read() {