From bd07120d3303ee38498e01bbef9052d4d50f77f5 Mon Sep 17 00:00:00 2001
From: Dimitris Papavasiliou <dpapavas@protonmail.ch>
Date: Tue, 20 Apr 2021 20:17:39 +0300
Subject: [PATCH] [Keyboard]  Fix and improve SPI transport in the Lagrange
 (#12606)

Co-authored-by: Dimitris Papavasiliou <dpapavas@gmail.com>
---
 keyboards/handwired/lagrange/transport.c | 81 +++++++++++++++++-------
 1 file changed, 57 insertions(+), 24 deletions(-)

diff --git a/keyboards/handwired/lagrange/transport.c b/keyboards/handwired/lagrange/transport.c
index 2a567f24b94..8f6973925ff 100644
--- a/keyboards/handwired/lagrange/transport.c
+++ b/keyboards/handwired/lagrange/transport.c
@@ -18,6 +18,7 @@
 
 #include "quantum.h"
 #include "split_util.h"
+#include "transport.h"
 #include "timer.h"
 
 #include "lagrange.h"
@@ -32,15 +33,16 @@ uint8_t transceive(uint8_t b) {
     return SPDR;
 }
 
-/* The SPI bus, doens't have any form of protocol built in, so when
+/* The SPI bus, doesn't have any form of protocol built in, so when
  * the other side isn't present, any old noise on the line will appear
  * as matrix data.  To avoid interpreting data as keystrokes, we do a
  * simple n-way (8-way here) handshake before each scan, where each
  * side sends a prearranged sequence of bytes. */
 
-void shake_hands(bool master) {
+bool shake_hands(bool master) {
     const uint8_t m = master ? 0xf8 : 0;
     const uint8_t a = 0xa8 ^ m, b = 0x50 ^ m;
+    bool synchronized = true;
 
     uint8_t i;
 
@@ -48,7 +50,7 @@ void shake_hands(bool master) {
     i = SPDR;
 
     do {
-        /* Cylcling the SS pin on each attempt is necessary, as it
+        /* Cycling the SS pin on each attempt is necessary, as it
          * resets the AVR's SPI core and guarantees proper
          * alignment. */
 
@@ -58,6 +60,7 @@ void shake_hands(bool master) {
 
         for (i = 0 ; i < 8 ; i += 1) {
             if (transceive(a + i) != b + i) {
+                synchronized = false;
                 break;
             }
         }
@@ -66,9 +69,11 @@ void shake_hands(bool master) {
             writePinHigh(SPI_SS_PIN);
         }
     } while (i < 8);
+
+    return synchronized;
 }
 
-bool transport_master(matrix_row_t matrix[]) {
+bool transport_master(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
     const struct led_context context = {
         host_keyboard_led_state(),
         layer_state
@@ -76,32 +81,58 @@ bool transport_master(matrix_row_t matrix[]) {
 
     uint8_t i;
 
-    /* Shake hands and then receive the matrix from the other side,
-     * while transmitting LED and layer states. */
+    /* We shake hands both before and after transmitting the matrix.
+     * Doing it before transmitting is necessary to ensure
+     * synchronization: Due to the master-slave nature of the SPI bus,
+     * the master calls the shots.  If we just go ahead and start
+     * clocking bits, the slave side might be otherwise engaged at
+     * that moment, so we'll initially read zeros, or garbage.  Then
+     * when the slave gets around to transmitting its matrix, we'll
+     * misinterpret the keys it sends, leading to spurious
+     * keypresses. */
 
-    shake_hands(true);
+    /* The handshake forces the master to wait for the slave to be
+     * ready to start transmitting. */
 
-    spi_start(SPI_SS_PIN, 0, 0, 4);
+    do {
+        shake_hands(true);
 
-    for (i = 0 ; i < sizeof(matrix_row_t[MATRIX_ROWS / 2]) ; i += 1) {
-        spi_status_t x;
+        /* Receive the matrix from the other side, while transmitting
+         * LED and layer states. */
 
-        x = spi_write(i < sizeof(struct led_context) ?
-                      ((uint8_t *)&context)[i] : 0);
+        spi_start(SPI_SS_PIN, 0, 0, 4);
 
-        if (x == SPI_STATUS_TIMEOUT) {
-            return false;
+        for (i = 0 ; i < sizeof(matrix_row_t[MATRIX_ROWS / 2]) ; i += 1) {
+            spi_status_t x;
+
+            x = spi_write(i < sizeof(struct led_context) ?
+                          ((uint8_t *)&context)[i] : 0);
+
+            if (x == SPI_STATUS_TIMEOUT) {
+                return false;
+            }
+
+            ((uint8_t *)slave_matrix)[i] = (uint8_t)x;
         }
 
-        ((uint8_t *)matrix)[i] = (uint8_t)x;
-    }
+        spi_stop();
 
-    spi_stop();
+        /* In case of errors during the transmission, e.g. if the
+         * cable was disconnected and since there is no inherent
+         * error-checking protocol, we would simply interpret noise as
+         * data. */
+
+        /* To avoid this, both sides shake hands after transmitting.
+         * If synchronization was lost during transmission, the (first)
+         * handshake will fail.  In that case we go around and
+         * re-transmit. */
+
+    } while (!shake_hands(true));
 
     return true;
 }
 
-void transport_slave(matrix_row_t matrix[]) {
+void transport_slave(matrix_row_t master_matrix[], matrix_row_t slave_matrix[]) {
     static struct led_context context;
     struct led_context new_context;
 
@@ -113,15 +144,17 @@ void transport_slave(matrix_row_t matrix[]) {
     cli();
     shake_hands(false);
 
-    for (i = 0 ; i < sizeof(matrix_row_t[MATRIX_ROWS / 2]) ; i += 1) {
-        uint8_t b;
+    do {
+        for (i = 0 ; i < sizeof(matrix_row_t[MATRIX_ROWS / 2]) ; i += 1) {
+            uint8_t b;
 
-        b = transceive(((uint8_t *)matrix)[i]);
+            b = transceive(((uint8_t *)slave_matrix)[i]);
 
-        if (i < sizeof(struct led_context)) {
-            ((uint8_t *)&new_context)[i] = b;
+            if (i < sizeof(struct led_context)) {
+                ((uint8_t *)&new_context)[i] = b;
+            }
         }
-    }
+    } while (!shake_hands(false));
 
     sei();