[Core] Split ChibiOS usart split driver in protocol and hardware driver part (#16669)

5 files changed, 263 insertions, 226 deletions

diff --git a/platforms/chibios/drivers/serial.c b/platforms/chibios/drivers/serial.c
index 0cff057d1d..e5f346ba33 100644
--- a/platforms/chibios/drivers/serial.c
+++ b/platforms/chibios/drivers/serial.c
@@ -233,7 +233,7 @@ static inline bool initiate_transaction(uint8_t sstd_index) {
     // check if the slave is present
     if (serial_read_pin()) {
         // slave failed to pull the line low, assume not present
-        dprintf("serial::NO_RESPONSE\n");
+        serial_dprintf("serial::NO_RESPONSE\n");
         chSysUnlock();
         return false;
     }
@@ -269,7 +269,7 @@ static inline bool initiate_transaction(uint8_t sstd_index) {
     serial_delay();
 
     if ((checksum_computed) != (checksum_received)) {
-        dprintf("serial::FAIL[%u,%u,%u]\n", checksum_computed, checksum_received, sstd_index);
+        serial_dprintf("serial::FAIL[%u,%u,%u]\n", checksum_computed, checksum_received, sstd_index);
         serial_output();
         serial_high();
 
diff --git a/platforms/chibios/drivers/serial_protocol.c b/platforms/chibios/drivers/serial_protocol.c
new file mode 100644
index 0000000000..3e160e0c5c
--- /dev/null
+++ b/platforms/chibios/drivers/serial_protocol.c
@@ -0,0 +1,164 @@
+// Copyright 2022 Stefan Kerkmann
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <ch.h>
+
+#include "quantum.h"
+#include "serial.h"
+#include "serial_protocol.h"
+#include "printf.h"
+#include "synchronization_util.h"
+
+static inline bool initiate_transaction(uint8_t transaction_id);
+static inline bool react_to_transaction(void);
+
+/**
+ * @brief This thread runs on the slave and responds to transactions initiated
+ * by the master.
+ */
+static THD_WORKING_AREA(waSlaveThread, 1024);
+static THD_FUNCTION(SlaveThread, arg) {
+    (void)arg;
+    chRegSetThreadName("split_protocol_tx_rx");
+
+    while (true) {
+        split_shared_memory_lock();
+        if (unlikely(!react_to_transaction())) {
+            /* Clear the receive queue, to start with a clean slate.
+             * Parts of failed transactions or spurious bytes could still be in it. */
+            serial_transport_driver_clear();
+        }
+        split_shared_memory_unlock();
+    }
+}
+
+/**
+ * @brief Slave specific initializations.
+ */
+void soft_serial_target_init(void) {
+    serial_transport_driver_slave_init();
+
+    /* Start transport thread. */
+    chThdCreateStatic(waSlaveThread, sizeof(waSlaveThread), HIGHPRIO, SlaveThread, NULL);
+}
+
+/**
+ * @brief Master specific initializations.
+ */
+void soft_serial_initiator_init(void) {
+    serial_transport_driver_master_init();
+}
+
+/**
+ * @brief React to transactions started by the master.
+ */
+static inline bool react_to_transaction(void) {
+    uint8_t transaction_id = 0;
+    /* Wait until there is a transaction for us. */
+    if (unlikely(!serial_transport_receive_blocking(&transaction_id, sizeof(transaction_id)))) {
+        return false;
+    }
+
+    /* Sanity check that we are actually responding to a valid transaction. */
+    if (unlikely(transaction_id >= NUM_TOTAL_TRANSACTIONS)) {
+        return false;
+    }
+
+    split_transaction_desc_t* transaction = &split_transaction_table[transaction_id];
+
+    /* Send back the handshake which is XORed as a simple checksum,
+     to signal that the slave is ready to receive possible transaction buffers  */
+    transaction_id ^= NUM_TOTAL_TRANSACTIONS;
+    if (unlikely(!serial_transport_send(&transaction_id, sizeof(transaction_id)))) {
+        return false;
+    }
+
+    /* Receive transaction buffer from the master. If this transaction requires it.*/
+    if (transaction->initiator2target_buffer_size) {
+        if (unlikely(!serial_transport_receive(split_trans_initiator2target_buffer(transaction), transaction->initiator2target_buffer_size))) {
+            return false;
+        }
+    }
+
+    /* Allow any slave processing to occur. */
+    if (transaction->slave_callback) {
+        transaction->slave_callback(transaction->initiator2target_buffer_size, split_trans_initiator2target_buffer(transaction), transaction->initiator2target_buffer_size, split_trans_target2initiator_buffer(transaction));
+    }
+
+    /* Send transaction buffer to the master. If this transaction requires it. */
+    if (transaction->target2initiator_buffer_size) {
+        if (unlikely(!serial_transport_send(split_trans_target2initiator_buffer(transaction), transaction->target2initiator_buffer_size))) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
+/**
+ * @brief Start transaction from the master half to the slave half.
+ *
+ * @param index Transaction Table index of the transaction to start.
+ * @return bool Indicates success of transaction.
+ */
+bool soft_serial_transaction(int index) {
+    split_shared_memory_lock();
+    bool result = initiate_transaction((uint8_t)index);
+    split_shared_memory_unlock();
+
+    if (unlikely(!result)) {
+        /* Clear the receive queue, to start with a clean slate.
+         * Parts of failed transactions or spurious bytes could still be in it. */
+        serial_transport_driver_clear();
+    }
+
+    return result;
+}
+
+/**
+ * @brief Initiate transaction to slave half.
+ */
+static inline bool initiate_transaction(uint8_t transaction_id) {
+    /* Sanity check that we are actually starting a valid transaction. */
+    if (unlikely(transaction_id >= NUM_TOTAL_TRANSACTIONS)) {
+        serial_dprintf("SPLIT: illegal transaction id\n");
+        return false;
+    }
+
+    split_transaction_desc_t* transaction = &split_transaction_table[transaction_id];
+
+    /* Send transaction table index to the slave, which doubles as basic handshake token. */
+    if (unlikely(!serial_transport_send(&transaction_id, sizeof(transaction_id)))) {
+        serial_dprintf("SPLIT: sending handshake failed\n");
+        return false;
+    }
+
+    uint8_t transaction_id_shake = 0xFF;
+
+    /* Which we always read back first so that we can error out correctly.
+     *   - due to the half duplex limitations on return codes, we always have to read *something*.
+     *   - without the read, write only transactions *always* succeed, even during the boot process where the slave is not ready.
+     */
+    if (unlikely(!serial_transport_receive(&transaction_id_shake, sizeof(transaction_id_shake)) || (transaction_id_shake != (transaction_id ^ NUM_TOTAL_TRANSACTIONS)))) {
+        serial_dprintf("SPLIT: receiving handshake failed\n");
+        return false;
+    }
+
+    /* Send transaction buffer to the slave. If this transaction requires it. */
+    if (transaction->initiator2target_buffer_size) {
+        if (unlikely(!serial_transport_send(split_trans_initiator2target_buffer(transaction), transaction->initiator2target_buffer_size))) {
+            serial_dprintf("SPLIT: sending buffer failed\n");
+            return false;
+        }
+    }
+
+    /* Receive transaction buffer from the slave. If this transaction requires it. */
+    if (transaction->target2initiator_buffer_size) {
+        if (unlikely(!serial_transport_receive(split_trans_target2initiator_buffer(transaction), transaction->target2initiator_buffer_size))) {
+            serial_dprintf("SPLIT: receiving buffer failed\n");
+            return false;
+        }
+    }
+
+    return true;
+}
diff --git a/platforms/chibios/drivers/serial_protocol.h b/platforms/chibios/drivers/serial_protocol.h
new file mode 100644
index 0000000000..4275a7f8d8
--- /dev/null
+++ b/platforms/chibios/drivers/serial_protocol.h
@@ -0,0 +1,49 @@
+// Copyright 2022 Stefan Kerkmann
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#include <stddef.h>
+#include <stdint.h>
+#include <stdbool.h>
+
+#pragma once
+
+/**
+ * @brief Clears any intermediate sending or receiving state of the driver to a known good
+ * state. This happens after errors in the middle of transactions, to start with
+ * a clean slate.
+ */
+void serial_transport_driver_clear(void);
+
+/**
+ * @brief Driver specific initialization on the slave half.
+ */
+void serial_transport_driver_slave_init(void);
+
+/**
+ * @brief Driver specific specific initialization on the master half.
+ */
+void serial_transport_driver_master_init(void);
+
+/**
+ * @brief  Blocking receive of size * bytes.
+ *
+ * @return true Receive success.
+ * @return false Receive failed, e.g. by bit errors.
+ */
+bool __attribute__((nonnull, hot)) serial_transport_receive(uint8_t* destination, const size_t size);
+
+/**
+ * @brief Blocking receive of size * bytes with an implicitly defined timeout.
+ *
+ * @return true Receive success.
+ * @return false Receive failed, e.g. by timeout or bit errors.
+ */
+bool __attribute__((nonnull, hot)) serial_transport_receive_blocking(uint8_t* destination, const size_t size);
+
+/**
+ * @brief Blocking send of buffer with timeout.
+ *
+ * @return true Send success.
+ * @return false Send failed, e.g. by timeout or bit errors.
+ */
+bool __attribute__((nonnull, hot)) serial_transport_send(const uint8_t* source, const size_t size);
diff --git a/platforms/chibios/drivers/serial_usart.c b/platforms/chibios/drivers/serial_usart.c
index 7ea25d005b..f76afb5db4 100644
--- a/platforms/chibios/drivers/serial_usart.c
+++ b/platforms/chibios/drivers/serial_usart.c
@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include "serial_usart.h"
+#include "serial_protocol.h"
 #include "synchronization_util.h"
 
 #if defined(SERIAL_USART_CONFIG)
@@ -26,14 +27,6 @@ static QMKSerialConfig serial_config = {
 
 static QMKSerialDriver* serial_driver = (QMKSerialDriver*)&SERIAL_USART_DRIVER;
 
-static inline bool react_to_transactions(void);
-static inline bool __attribute__((nonnull)) receive(uint8_t* destination, const size_t size);
-static inline bool __attribute__((nonnull)) receive_blocking(uint8_t* destination, const size_t size);
-static inline bool __attribute__((nonnull)) send(const uint8_t* source, const size_t size);
-static inline bool initiate_transaction(uint8_t sstd_index);
-static inline void usart_clear(void);
-static inline void usart_driver_start(void);
-
 #if HAL_USE_SERIAL
 
 /**
@@ -43,10 +36,7 @@ static inline void usart_driver_start(void) {
     sdStart(serial_driver, &serial_config);
 }
 
-/**
- * @brief Clear the receive input queue.
- */
-static inline void usart_clear(void) {
+inline void serial_transport_driver_clear(void) {
     osalSysLock();
     bool volatile queue_not_empty = !iqIsEmptyI(&serial_driver->iqueue);
     osalSysUnlock();
@@ -89,10 +79,7 @@ static inline void usart_driver_start(void) {
     sioStartOperation(serial_driver, &serial_usart_operation);
 }
 
-/**
- * @brief Clear the receive input queue, as some MCUs have built-in hardware FIFOs.
- */
-static inline void usart_clear(void) {
+inline void serial_transport_driver_clear(void) {
     osalSysLock();
     while (!sioIsRXEmptyX(serial_driver)) {
         (void)sioGetX(serial_driver);
@@ -106,13 +93,7 @@ static inline void usart_clear(void) {
 
 #endif
 
-/**
- * @brief Blocking send of buffer with timeout.
- *
- * @return true Send success.
- * @return false Send failed.
- */
-static inline bool send(const uint8_t* source, const size_t size) {
+inline bool serial_transport_send(const uint8_t* source, const size_t size) {
     bool success = (size_t)chnWriteTimeout(serial_driver, source, size, TIME_MS2I(SERIAL_USART_TIMEOUT)) == size;
 
 #if !defined(SERIAL_USART_FULL_DUPLEX)
@@ -129,13 +110,13 @@ static inline bool send(const uint8_t* source, const size_t size) {
         uint8_t dump[64];
 
         while (unlikely(bytes_left >= 64)) {
-            if (unlikely(!receive(dump, 64))) {
+            if (unlikely(!serial_transport_receive(dump, 64))) {
                 return false;
             }
             bytes_left -= 64;
         }
 
-        return receive(dump, bytes_left);
+        return serial_transport_receive(dump, bytes_left);
 #    else
         /* The SIO driver directly accesses the hardware FIFOs of the USART
          * peripheral. As these are limited in depth, the RX FIFO might have been
@@ -159,24 +140,12 @@ static inline bool send(const uint8_t* source, const size_t size) {
     return success;
 }
 
-/**
- * @brief  Blocking receive of size * bytes with timeout.
- *
- * @return true Receive success.
- * @return false Receive failed, e.g. by timeout.
- */
-static inline bool receive(uint8_t* destination, const size_t size) {
+inline bool serial_transport_receive(uint8_t* destination, const size_t size) {
     bool success = (size_t)chnReadTimeout(serial_driver, destination, size, TIME_MS2I(SERIAL_USART_TIMEOUT)) == size;
     return success;
 }
 
-/**
- * @brief  Blocking receive of size * bytes.
- *
- * @return true Receive success.
- * @return false Receive failed.
- */
-static inline bool receive_blocking(uint8_t* destination, const size_t size) {
+inline bool serial_transport_receive_blocking(uint8_t* destination, const size_t size) {
     bool success = (size_t)chnRead(serial_driver, destination, size) == size;
     return success;
 }
@@ -243,86 +212,12 @@ __attribute__((weak, nonnull)) void usart_slave_init(QMKSerialDriver** driver) {
     usart_init();
 }
 
-/**
- * @brief This thread runs on the slave and responds to transactions initiated
- * by the master.
- */
-static THD_WORKING_AREA(waSlaveThread, 1024);
-static THD_FUNCTION(SlaveThread, arg) {
-    (void)arg;
-    chRegSetThreadName("usart_tx_rx");
-
-    while (true) {
-        if (unlikely(!react_to_transactions())) {
-            /* Clear the receive queue, to start with a clean slate.
-             * Parts of failed transactions or spurious bytes could still be in it. */
-            usart_clear();
-        }
-        split_shared_memory_unlock();
-    }
-}
-
-/**
- * @brief Slave specific initializations.
- */
-void soft_serial_target_init(void) {
+void serial_transport_driver_slave_init(void) {
     usart_slave_init(&serial_driver);
-
     usart_driver_start();
-
-    /* Start transport thread. */
-    chThdCreateStatic(waSlaveThread, sizeof(waSlaveThread), HIGHPRIO, SlaveThread, NULL);
-}
-
-/**
- * @brief React to transactions started by the master.
- */
-static inline bool react_to_transactions(void) {
-    /* Wait until there is a transaction for us. */
-    uint8_t sstd_index = 0;
-    receive_blocking(&sstd_index, sizeof(sstd_index));
-
-    /* Sanity check that we are actually responding to a valid transaction. */
-    if (unlikely(sstd_index >= NUM_TOTAL_TRANSACTIONS)) {
-        return false;
-    }
-
-    split_shared_memory_lock();
-    split_transaction_desc_t* trans = &split_transaction_table[sstd_index];
-
-    /* Send back the handshake which is XORed as a simple checksum,
-     to signal that the slave is ready to receive possible transaction buffers  */
-    sstd_index ^= HANDSHAKE_MAGIC;
-    if (unlikely(!send(&sstd_index, sizeof(sstd_index)))) {
-        return false;
-    }
-
-    /* Receive transaction buffer from the master. If this transaction requires it.*/
-    if (trans->initiator2target_buffer_size) {
-        if (unlikely(!receive(split_trans_initiator2target_buffer(trans), trans->initiator2target_buffer_size))) {
-            return false;
-        }
-    }
-
-    /* Allow any slave processing to occur. */
-    if (trans->slave_callback) {
-        trans->slave_callback(trans->initiator2target_buffer_size, split_trans_initiator2target_buffer(trans), trans->initiator2target_buffer_size, split_trans_target2initiator_buffer(trans));
-    }
-
-    /* Send transaction buffer to the master. If this transaction requires it. */
-    if (trans->target2initiator_buffer_size) {
-        if (unlikely(!send(split_trans_target2initiator_buffer(trans), trans->target2initiator_buffer_size))) {
-            return false;
-        }
-    }
-
-    return true;
 }
 
-/**
- * @brief Master specific initializations.
- */
-void soft_serial_initiator_init(void) {
+void serial_transport_driver_master_init(void) {
     usart_master_init(&serial_driver);
 
 #if defined(MCU_STM32) && defined(SERIAL_USART_PIN_SWAP)
@@ -331,69 +226,3 @@ void soft_serial_initiator_init(void) {
 
     usart_driver_start();
 }
-
-/**
- * @brief Start transaction from the master half to the slave half.
- *
- * @param index Transaction Table index of the transaction to start.
- * @return bool Indicates success of transaction.
- */
-bool soft_serial_transaction(int index) {
-    /* Clear the receive queue, to start with a clean slate.
-     * Parts of failed transactions or spurious bytes could still be in it. */
-    usart_clear();
-
-    split_shared_memory_lock();
-    bool result = initiate_transaction((uint8_t)index);
-    split_shared_memory_unlock();
-
-    return result;
-}
-
-/**
- * @brief Initiate transaction to slave half.
- */
-static inline bool initiate_transaction(uint8_t sstd_index) {
-    /* Sanity check that we are actually starting a valid transaction. */
-    if (unlikely(sstd_index >= NUM_TOTAL_TRANSACTIONS)) {
-        dprintln("USART: Illegal transaction Id.");
-        return false;
-    }
-
-    split_transaction_desc_t* trans = &split_transaction_table[sstd_index];
-
-    /* Send transaction table index to the slave, which doubles as basic handshake token. */
-    if (unlikely(!send(&sstd_index, sizeof(sstd_index)))) {
-        dprintln("USART: Send Handshake failed.");
-        return false;
-    }
-
-    uint8_t sstd_index_shake = 0xFF;
-
-    /* Which we always read back first so that we can error out correctly.
-     *   - due to the half duplex limitations on return codes, we always have to read *something*.
-     *   - without the read, write only transactions *always* succeed, even during the boot process where the slave is not ready.
-     */
-    if (unlikely(!receive(&sstd_index_shake, sizeof(sstd_index_shake)) || (sstd_index_shake != (sstd_index ^ HANDSHAKE_MAGIC)))) {
-        dprintln("USART: Handshake failed.");
-        return false;
-    }
-
-    /* Send transaction buffer to the slave. If this transaction requires it. */
-    if (trans->initiator2target_buffer_size) {
-        if (unlikely(!send(split_trans_initiator2target_buffer(trans), trans->initiator2target_buffer_size))) {
-            dprintln("USART: Send failed.");
-            return false;
-        }
-    }
-
-    /* Receive transaction buffer from the slave. If this transaction requires it. */
-    if (trans->target2initiator_buffer_size) {
-        if (unlikely(!receive(split_trans_target2initiator_buffer(trans), trans->target2initiator_buffer_size))) {
-            dprintln("USART: Receive failed.");
-            return false;
-        }
-    }
-
-    return true;
-}
diff --git a/platforms/chibios/drivers/serial_usart.h b/platforms/chibios/drivers/serial_usart.h
index 98c634dac0..fa062cd736 100644
--- a/platforms/chibios/drivers/serial_usart.h
+++ b/platforms/chibios/drivers/serial_usart.h
@@ -5,11 +5,46 @@
 
 #include "quantum.h"
 #include "serial.h"
-#include "printf.h"
-
-#include <ch.h>
 #include <hal.h>
 
+#if defined(SOFT_SERIAL_PIN)
+#    define SERIAL_USART_TX_PIN SOFT_SERIAL_PIN
+#endif
+
+#if !defined(SERIAL_USART_TX_PIN)
+#    define SERIAL_USART_TX_PIN A9
+#endif
+
+#if !defined(SERIAL_USART_RX_PIN)
+#    define SERIAL_USART_RX_PIN A10
+#endif
+
+#if !defined(SELECT_SOFT_SERIAL_SPEED)
+#    define SELECT_SOFT_SERIAL_SPEED 1
+#endif
+
+#if defined(SERIAL_USART_SPEED)
+// Allow advanced users to directly set SERIAL_USART_SPEED
+#elif SELECT_SOFT_SERIAL_SPEED == 0
+#    define SERIAL_USART_SPEED 460800
+#elif SELECT_SOFT_SERIAL_SPEED == 1
+#    define SERIAL_USART_SPEED 230400
+#elif SELECT_SOFT_SERIAL_SPEED == 2
+#    define SERIAL_USART_SPEED 115200
+#elif SELECT_SOFT_SERIAL_SPEED == 3
+#    define SERIAL_USART_SPEED 57600
+#elif SELECT_SOFT_SERIAL_SPEED == 4
+#    define SERIAL_USART_SPEED 38400
+#elif SELECT_SOFT_SERIAL_SPEED == 5
+#    define SERIAL_USART_SPEED 19200
+#else
+#    error invalid SELECT_SOFT_SERIAL_SPEED value
+#endif
+
+#if !defined(SERIAL_USART_TIMEOUT)
+#    define SERIAL_USART_TIMEOUT 20
+#endif
+
 #if HAL_USE_SERIAL
 
 typedef SerialDriver QMKSerialDriver;
@@ -40,18 +75,6 @@ typedef SIOConfig QMKSerialConfig;
 #    endif
 #endif
 
-#if defined(SOFT_SERIAL_PIN)
-#    define SERIAL_USART_TX_PIN SOFT_SERIAL_PIN
-#endif
-
-#if !defined(SERIAL_USART_TX_PIN)
-#    define SERIAL_USART_TX_PIN A9
-#endif
-
-#if !defined(SERIAL_USART_RX_PIN)
-#    define SERIAL_USART_RX_PIN A10
-#endif
-
 #if !defined(USART_CR1_M0)
 #    define USART_CR1_M0 USART_CR1_M // some platforms (f1xx) dont have this so
 #endif
@@ -89,31 +112,3 @@ typedef SIOConfig QMKSerialConfig;
             (AFIO->MAPR |= AFIO_MAPR_USART3_REMAP_FULLREMAP); \
         } while (0)
 #endif
-
-#if !defined(SELECT_SOFT_SERIAL_SPEED)
-#    define SELECT_SOFT_SERIAL_SPEED 1
-#endif
-
-#if defined(SERIAL_USART_SPEED)
-// Allow advanced users to directly set SERIAL_USART_SPEED
-#elif SELECT_SOFT_SERIAL_SPEED == 0
-#    define SERIAL_USART_SPEED 460800
-#elif SELECT_SOFT_SERIAL_SPEED == 1
-#    define SERIAL_USART_SPEED 230400
-#elif SELECT_SOFT_SERIAL_SPEED == 2
-#    define SERIAL_USART_SPEED 115200
-#elif SELECT_SOFT_SERIAL_SPEED == 3
-#    define SERIAL_USART_SPEED 57600
-#elif SELECT_SOFT_SERIAL_SPEED == 4
-#    define SERIAL_USART_SPEED 38400
-#elif SELECT_SOFT_SERIAL_SPEED == 5
-#    define SERIAL_USART_SPEED 19200
-#else
-#    error invalid SELECT_SOFT_SERIAL_SPEED value
-#endif
-
-#if !defined(SERIAL_USART_TIMEOUT)
-#    define SERIAL_USART_TIMEOUT 20
-#endif
-
-#define HANDSHAKE_MAGIC 7U