Rework encoders to enable asymmetric split keyboards

build_test.mk

@@ -51,6 +51,7 @@ include common_features.mk
 include $(TMK_PATH)/common.mk
 include $(QUANTUM_PATH)/sequencer/tests/rules.mk
 include $(QUANTUM_PATH)/serial_link/tests/rules.mk
+include $(QUANTUM_PATH)/encoder/tests/rules.mk
 ifneq ($(filter $(FULL_TESTS),$(TEST)),)
 include build_full_test.mk
 endif

docs/feature_encoders.md

@@ -40,14 +40,27 @@ It can also be defined per-encoder, by instead defining:
 
 ## Split Keyboards
 
-If you are using different pinouts for the encoders on each half of a split keyboard, you can define the pinout (and optionally, resolutions) for the right half like this:
+The above is enough for split keyboards that are symmetrical, i.e. the halves have the same number of encoders and they are on the same pins.
+If the halves are not symmetrical, you can define the pinout (and optionally, resolutions) of the right half separately.
+The left half will use the definitions above.
 
 ```c
 #define ENCODERS_PAD_A_RIGHT { encoder1a, encoder2a }
 #define ENCODERS_PAD_B_RIGHT { encoder1b, encoder2b }
 #define ENCODER_RESOLUTIONS_RIGHT { 2, 4 }
 ```
 
+If only the right half has encoders, you must still define an empty array for the left pads (and resolutions, if you define `ENCODER_RESOLUTIONS_RIGHT`).
+
+```c
+#define ENCODERS_PAD_A {  }
+#define ENCODERS_PAD_B {  }
+#define ENCODER_RESOLUTIONS {  }
+#define ENCODERS_PAD_A_RIGHT { encoder1a, encoder2a }
+#define ENCODERS_PAD_B_RIGHT { encoder1b, encoder2b }
+#define ENCODER_RESOLUTIONS_RIGHT { 2, 4 }
+```
+
 ## Callbacks
 
 The callback functions can be inserted into your `<keyboard>.c`:

quantum/encoder.c

@@ -16,8 +16,17 @@
  */
 
 #include "encoder.h"
-#ifdef SPLIT_KEYBOARD
-#    include "split_util.h"
+
+// this is for unit testing
+#if defined(ENCODER_MOCK_SINGLE)
+#    include "encoder/tests/mock.h"
+#elif defined(ENCODER_MOCK_SPLIT)
+#    include "encoder/tests/mock_split.h"
+#else
+#    include <gpio.h>
+#    ifdef SPLIT_KEYBOARD
+#        include "split_util.h"
+#    endif
 #endif
 
 // for memcpy
@@ -27,17 +36,41 @@
 #    define ENCODER_RESOLUTION 4
 #endif
 
-#if !defined(ENCODERS_PAD_A) || !defined(ENCODERS_PAD_B)
-#    error "No encoder pads defined by ENCODERS_PAD_A and ENCODERS_PAD_B"
+#if (!defined(ENCODERS_PAD_A) || !defined(ENCODERS_PAD_B)) && (!defined(ENCODERS_PAD_A) || !defined(ENCODERS_PAD_B))
+#    error "No encoder pads defined by ENCODERS_PAD_A and ENCODERS_PAD_B or ENCODERS_PAD_A_RIGHT and ENCODERS_PAD_B_RIGHT"
 #endif
 
-#define NUMBER_OF_ENCODERS (sizeof(encoders_pad_a) / sizeof(pin_t))
+// on split keyboards, these are the pads and resolutions for the left half
 static pin_t encoders_pad_a[] = ENCODERS_PAD_A;
 static pin_t encoders_pad_b[] = ENCODERS_PAD_B;
 #ifdef ENCODER_RESOLUTIONS
 static uint8_t encoder_resolutions[] = ENCODER_RESOLUTIONS;
 #endif
 
+#ifndef SPLIT_KEYBOARD
+#    define NUMBER_OF_ENCODERS (sizeof(encoders_pad_a) / sizeof(pin_t))
+#else
+// if no pads for right half are defined, we assume the keyboard is symmetric (i.e. same pads)
+#    ifndef ENCODERS_PAD_A_RIGHT
+#        define ENCODERS_PAD_A_RIGHT ENCODERS_PAD_A
+#    endif
+#    ifndef ENCODERS_PAD_B_RIGHT
+#        define ENCODERS_PAD_B_RIGHT ENCODERS_PAD_B
+#    endif
+#    if defined(ENCODER_RESOLUTIONS) && !defined(ENCODER_RESOLUTIONS_RIGHT)
+#        define ENCODER_RESOLUTIONS_RIGHT ENCODER_RESOLUTIONS
+#    endif
+
+#    define NUMBER_OF_ENCODERS ((sizeof(encoders_pad_a) + sizeof(encoders_pad_a_right)) / sizeof(pin_t))
+#    define NUMBER_OF_ENCODERS_LEFT (sizeof(encoders_pad_a) / sizeof(pin_t))
+#    define NUMBER_OF_ENCODERS_RIGHT (sizeof(encoders_pad_a_right) / sizeof(pin_t))
+static pin_t   encoders_pad_a_right[]      = ENCODERS_PAD_A_RIGHT;
+static pin_t   encoders_pad_b_right[]      = ENCODERS_PAD_B_RIGHT;
+#    ifdef ENCODER_RESOLUTIONS_RIGHT
+static uint8_t encoder_resolutions_right[] = ENCODER_RESOLUTIONS_RIGHT;
+#    endif
+#endif
+
 #ifndef ENCODER_DIRECTION_FLIP
 #    define ENCODER_CLOCKWISE true
 #    define ENCODER_COUNTER_CLOCKWISE false
@@ -50,97 +83,100 @@ static int8_t encoder_LUT[] = {0, -1, 1, 0, 1, 0, 0, -1, -1, 0, 0, 1, 0, 1, -1,
 static uint8_t encoder_state[NUMBER_OF_ENCODERS]  = {0};
 static int8_t  encoder_pulses[NUMBER_OF_ENCODERS] = {0};
 
-#ifdef SPLIT_KEYBOARD
-// right half encoders come over as second set of encoders
-static uint8_t encoder_value[NUMBER_OF_ENCODERS * 2] = {0};
-// row offsets for each hand
-static uint8_t thisHand, thatHand;
-#else
 static uint8_t encoder_value[NUMBER_OF_ENCODERS] = {0};
-#endif
 
 __attribute__((weak)) void encoder_update_user(int8_t index, bool clockwise) {}
 
 __attribute__((weak)) void encoder_update_kb(int8_t index, bool clockwise) { encoder_update_user(index, clockwise); }
 
+// number of encoders connected to this controller
+static uint8_t numEncodersHere;
+// index of the first encoder connected to this controller (only for right halves, this will be nonzero)
+static uint8_t firstEncoderHere;
+#ifdef SPLIT_KEYBOARD
+// index of the first encoder connected to the other half
+static uint8_t firstEncoderThere;
+#endif
+// the pads for this controller
+static pin_t* pad_a;
+static pin_t* pad_b;
+
 void encoder_init(void) {
-#if defined(SPLIT_KEYBOARD) && defined(ENCODERS_PAD_A_RIGHT) && defined(ENCODERS_PAD_B_RIGHT)
-    if (!isLeftHand) {
-        const pin_t encoders_pad_a_right[] = ENCODERS_PAD_A_RIGHT;
-        const pin_t encoders_pad_b_right[] = ENCODERS_PAD_B_RIGHT;
-#    if defined(ENCODER_RESOLUTIONS_RIGHT)
-        const uint8_t encoder_resolutions_right[] = ENCODER_RESOLUTIONS_RIGHT;
-#    endif
-        for (uint8_t i = 0; i < NUMBER_OF_ENCODERS; i++) {
-            encoders_pad_a[i] = encoders_pad_a_right[i];
-            encoders_pad_b[i] = encoders_pad_b_right[i];
-#    if defined(ENCODER_RESOLUTIONS_RIGHT)
-            encoder_resolutions[i] = encoder_resolutions_right[i];
-#    endif
-        }
+#ifndef SPLIT_KEYBOARD
+    numEncodersHere  = NUMBER_OF_ENCODERS;
+    pad_a            = encoders_pad_a;
+    pad_b            = encoders_pad_b;
+    firstEncoderHere = 0;
+#else
+    if (isLeftHand) {
+        numEncodersHere   = NUMBER_OF_ENCODERS_LEFT;
+        pad_a             = encoders_pad_a;
+        pad_b             = encoders_pad_b;
+        firstEncoderHere  = 0;
+        firstEncoderThere = NUMBER_OF_ENCODERS_LEFT;
+    } else {
+        numEncodersHere   = NUMBER_OF_ENCODERS_RIGHT;
+        pad_a             = encoders_pad_a_right;
+        pad_b             = encoders_pad_b_right;
+        firstEncoderHere  = NUMBER_OF_ENCODERS_LEFT;
+        firstEncoderThere = 0;
     }
 #endif
 
-    for (int i = 0; i < NUMBER_OF_ENCODERS; i++) {
-        setPinInputHigh(encoders_pad_a[i]);
-        setPinInputHigh(encoders_pad_b[i]);
+    for (int i = 0; i < numEncodersHere; i++) {
+        setPinInputHigh(pad_a[i]);
+        setPinInputHigh(pad_b[i]);
 
-        encoder_state[i] = (readPin(encoders_pad_a[i]) << 0) | (readPin(encoders_pad_b[i]) << 1);
+        encoder_state[firstEncoderHere + i] = (readPin(pad_a[i]) << 0) | (readPin(pad_b[i]) << 1);
     }
-
-#ifdef SPLIT_KEYBOARD
-    thisHand = isLeftHand ? 0 : NUMBER_OF_ENCODERS;
-    thatHand = NUMBER_OF_ENCODERS - thisHand;
-#endif
 }
 
 static bool encoder_update(int8_t index, uint8_t state) {
-    bool    changed = false;
-    uint8_t i       = index;
+    bool changed = false;
 
 #ifdef ENCODER_RESOLUTIONS
-    int8_t resolution = encoder_resolutions[i];
+#    ifndef SPLIT_KEYBOARD
+    int8_t resolution = encoder_resolutions[index];
+#    else
+    int8_t resolution = isLeftHand ? encoder_resolutions[index] : encoder_resolutions_right[index - NUMBER_OF_ENCODERS_LEFT];
+#    endif
 #else
     int8_t resolution = ENCODER_RESOLUTION;
 #endif
-
-#ifdef SPLIT_KEYBOARD
-    index += thisHand;
-#endif
-    encoder_pulses[i] += encoder_LUT[state & 0xF];
-    if (encoder_pulses[i] >= resolution) {
+    encoder_pulses[index] += encoder_LUT[state & 0xF];
+    if (encoder_pulses[index] >= resolution) {
         encoder_value[index]++;
         changed = true;
         encoder_update_kb(index, ENCODER_COUNTER_CLOCKWISE);
     }
-    if (encoder_pulses[i] <= -resolution) {  // direction is arbitrary here, but this clockwise
+    if (encoder_pulses[index] <= -resolution) {  // direction is arbitrary here, but this clockwise
         encoder_value[index]--;
         changed = true;
         encoder_update_kb(index, ENCODER_CLOCKWISE);
     }
-    encoder_pulses[i] %= resolution;
+    encoder_pulses[index] %= resolution;
     return changed;
 }
 
 bool encoder_read(void) {
     bool changed = false;
-    for (uint8_t i = 0; i < NUMBER_OF_ENCODERS; i++) {
-        encoder_state[i] <<= 2;
-        encoder_state[i] |= (readPin(encoders_pad_a[i]) << 0) | (readPin(encoders_pad_b[i]) << 1);
-        changed |= encoder_update(i, encoder_state[i]);
+    for (uint8_t i = 0; i < numEncodersHere; i++) {
+        encoder_state[firstEncoderHere + i] <<= 2;
+        encoder_state[firstEncoderHere + i] |= (readPin(pad_a[i]) << 0) | (readPin(pad_b[i]) << 1);
+        changed |= encoder_update(firstEncoderHere + i, encoder_state[firstEncoderHere + i]);
     }
     return changed;
 }
 
 #ifdef SPLIT_KEYBOARD
 void last_encoder_activity_trigger(void);
 
-void encoder_state_raw(uint8_t* slave_state) { memcpy(slave_state, &encoder_value[thisHand], sizeof(uint8_t) * NUMBER_OF_ENCODERS); }
+void encoder_state_raw(uint8_t* slave_state) { memcpy(slave_state, &encoder_value[firstEncoderHere], sizeof(uint8_t) * numEncodersHere); }
 
 void encoder_update_raw(uint8_t* slave_state) {
     bool changed = false;
-    for (uint8_t i = 0; i < NUMBER_OF_ENCODERS; i++) {
-        uint8_t index = i + thatHand;
+    for (uint8_t i = 0; i < NUMBER_OF_ENCODERS - numEncodersHere; i++) {
+        uint8_t index = firstEncoderThere + i;
         int8_t  delta = slave_state[i] - encoder_value[index];
         while (delta > 0) {
             delta--;

quantum/encoder.h

@@ -17,7 +17,8 @@
 
 #pragma once
 
-#include "quantum.h"
+#include <stdbool.h>
+#include <stdint.h>
 
 void encoder_init(void);
 bool encoder_read(void);

quantum/encoder/tests/encoder_tests.cpp

@@ -0,0 +1,130 @@
+
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"
+#include <vector>
+#include <algorithm>
+#include <stdio.h>
+
+extern "C" {
+    #include "encoder.h"
+    #include "encoder/tests/mock.h"
+}
+
+struct update {
+    int8_t index;
+    bool clockwise;
+};
+
+uint8_t uidx = 0;
+update updates[32];
+
+void encoder_update_kb(int8_t index, bool clockwise) {
+    updates[uidx % 32] = {index, clockwise};
+    uidx++;
+}
+
+bool setAndRead(pin_t pin, bool val) {
+    setPin(pin, val);
+    return encoder_read();
+}
+
+class EncoderTest : public ::testing::Test {
+
+};
+
+TEST_F(EncoderTest, TestInit) {
+    uidx = 0;
+    encoder_init();
+    EXPECT_EQ(pinIsInputHigh[0], true);
+    EXPECT_EQ(pinIsInputHigh[1], true);
+    EXPECT_EQ(uidx, 0);
+}
+
+TEST_F(EncoderTest, TestOneClockwise) {
+    uidx = 0;
+    encoder_init();
+    // send 4 pulses. with resolution 4, that's one step and we should get 1 update.
+    setAndRead(0, false);
+    setAndRead(1, false);
+    setAndRead(0, true);
+    setAndRead(1, true);
+
+    EXPECT_EQ(uidx, 1);
+    EXPECT_EQ(updates[0].index, 0);
+    EXPECT_EQ(updates[0].clockwise, true);
+}
+
+TEST_F(EncoderTest, TestOneCounterClockwise) {
+    uidx = 0;
+    encoder_init();
+    setAndRead(1, false);
+    setAndRead(0, false);
+    setAndRead(1, true);
+    setAndRead(0, true);
+
+    EXPECT_EQ(uidx, 1);
+    EXPECT_EQ(updates[0].index, 0);
+    EXPECT_EQ(updates[0].clockwise, false);
+}
+
+TEST_F(EncoderTest, TestTwoClockwiseOneCC) {
+    uidx = 0;
+    encoder_init();
+    setAndRead(0, false);
+    setAndRead(1, false);
+    setAndRead(0, true);
+    setAndRead(1, true);
+    setAndRead(0, false);
+    setAndRead(1, false);
+    setAndRead(0, true);
+    setAndRead(1, true);
+    setAndRead(1, false);
+    setAndRead(0, false);
+    setAndRead(1, true);
+    setAndRead(0, true);
+
+    EXPECT_EQ(uidx, 3);
+    EXPECT_EQ(updates[0].index, 0);
+    EXPECT_EQ(updates[0].clockwise, true);
+    EXPECT_EQ(updates[1].index, 0);
+    EXPECT_EQ(updates[1].clockwise, true);
+    EXPECT_EQ(updates[2].index, 0);
+    EXPECT_EQ(updates[2].clockwise, false);
+}
+
+TEST_F(EncoderTest, TestNoEarly) {
+    uidx = 0;
+    encoder_init();
+    // send 3 pulses. with resolution 4, that's not enough for a step.
+    setAndRead(0, false);
+    setAndRead(1, false);
+    setAndRead(0, true);
+    EXPECT_EQ(uidx, 0);
+    // now send last pulse
+    setAndRead(1, true);
+    EXPECT_EQ(uidx, 1);
+    EXPECT_EQ(updates[0].index, 0);
+    EXPECT_EQ(updates[0].clockwise, true);
+}
+
+TEST_F(EncoderTest, TestHalfway) {
+    uidx = 0;
+    encoder_init();
+    // go halfway
+    setAndRead(0, false);
+    setAndRead(1, false);
+    EXPECT_EQ(uidx, 0);
+    // back off
+    setAndRead(1, true);
+    setAndRead(0, true);
+    EXPECT_EQ(uidx, 0);
+    // go all the way
+    setAndRead(0, false);
+    setAndRead(1, false);
+    setAndRead(0, true);
+    setAndRead(1, true);
+    // should result in 1 update
+    EXPECT_EQ(uidx, 1);
+    EXPECT_EQ(updates[0].index, 0);
+    EXPECT_EQ(updates[0].clockwise, true);
+}