From 36bbee32ce37c97c46608a7ecd0ec8b8d878b0da Mon Sep 17 00:00:00 2001
From: Alberto Nidasio <alberto.nidasio@skywarder.eu>
Date: Wed, 15 Mar 2023 09:31:13 +0100
Subject: [PATCH] [SPI] Added comments for the new byteOrder config parameter

---
 src/shared/drivers/spi/SPIBus.h          | 10 +++++-----
 src/shared/drivers/spi/SPIBusInterface.h | 24 ++++++++++++++++++++++--
 src/shared/drivers/spi/SPIDriver.h       | 13 +++++++++++++
 src/tests/drivers/spi/test-spi.cpp       | 17 +++++++++++------
 4 files changed, 51 insertions(+), 13 deletions(-)

diff --git a/src/shared/drivers/spi/SPIBus.h b/src/shared/drivers/spi/SPIBus.h
index ae55504f0..e7c4d4923 100644
--- a/src/shared/drivers/spi/SPIBus.h
+++ b/src/shared/drivers/spi/SPIBus.h
@@ -50,11 +50,11 @@ namespace Boardcore
  *
  * Supported SPI main features:
  * - Full-duplex synchronous transfers on three lines
- * - 8- or 16-bit transfer frame format selection
+ * - 8 or 16-bit transfer frame formats
  * - Master operation
- * - 8 master mode baud rate prescaler (f_PCLK/2 max.)
+ * - 8 master mode baud rate prescaler values (f_PCLK/2 max.)
  * - Programmable clock polarity and phase
- * - Programmable data order with MSB-first or LSB-first shifting
+ * - Programmable data order (MSBit-first or LSBit-first)
  */
 class SPIBus : public SPIBusInterface
 {
@@ -544,7 +544,7 @@ inline uint16_t SPIBus::transfer16(uint16_t data)
 
 inline uint32_t SPIBus::transfer24(uint32_t data)
 {
-    uint32_t res = transfer16(data >> 8) << 8;
+    uint32_t res = static_cast<uint32_t>(transfer16(data >> 8)) << 8;
     res |= transfer(data);
 
     return res;
@@ -552,7 +552,7 @@ inline uint32_t SPIBus::transfer24(uint32_t data)
 
 inline uint32_t SPIBus::transfer32(uint32_t data)
 {
-    uint32_t res = transfer16(data >> 16) << 16;
+    uint32_t res = static_cast<uint32_t>(transfer16(data >> 16)) << 16;
     res |= transfer16(data);
 
     return res;
diff --git a/src/shared/drivers/spi/SPIBusInterface.h b/src/shared/drivers/spi/SPIBusInterface.h
index 438df30a7..ff501c219 100644
--- a/src/shared/drivers/spi/SPIBusInterface.h
+++ b/src/shared/drivers/spi/SPIBusInterface.h
@@ -50,10 +50,30 @@ struct SPIBusConfig
     ///< Clock polarity and phase configuration
     SPI::Mode mode;
 
-    ///< MSB or LSB first
+    ///< MSBit or LSBit first
     SPI::Order bitOrder;
 
-    ///< MSB or LSB first
+    /**
+     * @brief MSByte or LSByte first
+     *
+     * This parameter is used when reading and writing registers 16 bit wide or
+     * more.
+     *
+     * A device features MSByte first ordering if the most significant byte is
+     * at the lowest address. Example of a 24bit register:
+     *   Address:  0x06  0x07  0x08
+     *     value:  MSB   MID   LSB
+     *
+     * Conversely, an LSByte first ordering starts with the lowest significant
+     * byte first.
+     *
+     * Also, in every device used since now, in multiple registers accesses, the
+     * device always increments the address. So the user has always to provide
+     * the lowest address.
+     *
+     * @warning This driver does not support devices which decrements registers
+     * address during multiple registers accesses.
+     */
     SPI::Order byteOrder;
 
     ///< Write bit behaviour, default high when reading
diff --git a/src/shared/drivers/spi/SPIDriver.h b/src/shared/drivers/spi/SPIDriver.h
index 7bdb64ef3..7712958a0 100644
--- a/src/shared/drivers/spi/SPIDriver.h
+++ b/src/shared/drivers/spi/SPIDriver.h
@@ -22,6 +22,19 @@
 
 #pragma once
 
+/**
+ * This header file is inteded to provide all the necessary files needed to use
+ * the SPI driver.
+ *
+ * The driver is divided into 3 levels:
+ * - Low: SPIBus is the actual driver that talks directly to the STM32
+ * pheripheral
+ * - Middle: SPIBusInterface is a common interface that defines which operations
+ * the implementation has to privde
+ * - High: SPITransaction is a RAII abstraction that manages slave selection
+ * through the chip select signal
+ */
+
 #include "SPIBus.h"
 #include "SPIBusInterface.h"
 #include "SPITransaction.h"
diff --git a/src/tests/drivers/spi/test-spi.cpp b/src/tests/drivers/spi/test-spi.cpp
index cf4c00ca8..39b068863 100644
--- a/src/tests/drivers/spi/test-spi.cpp
+++ b/src/tests/drivers/spi/test-spi.cpp
@@ -66,9 +66,9 @@ int main()
         delayMs(1);
         transaction.read32();
         delayMs(1);
-        transaction.read(buffer8, 6);
+        transaction.read(buffer8, sizeof(buffer8));
         delayMs(1);
-        transaction.read16(buffer16, 6);
+        transaction.read16(buffer16, sizeof(buffer16));
         delayMs(1);
     }
 
@@ -88,7 +88,7 @@ int main()
         buffer8[3] = 0x67;
         buffer8[4] = 0x89;
         buffer8[5] = 0xAB;
-        transaction.write(buffer8, 6);
+        transaction.write(buffer8, sizeof(buffer8));
         delayMs(1);
         buffer16[0] = 0x0101;
         buffer16[1] = 0x2323;
@@ -96,7 +96,7 @@ int main()
         buffer16[3] = 0x6767;
         buffer16[4] = 0x8989;
         buffer16[5] = 0xABAB;
-        transaction.write16(buffer16, 6);
+        transaction.write16(buffer16, sizeof(buffer16));
         delayMs(1);
     }
 
@@ -110,9 +110,10 @@ int main()
         delayMs(1);
         transaction.transfer32((uint32_t)0xABCDEF01);
         delayMs(1);
-        transaction.transfer(buffer8, 6);
+        transaction.transfer(buffer8, sizeof(buffer8));
+        delayMs(1);
+        transaction.transfer16(buffer16, sizeof(buffer16));
         delayMs(1);
-        transaction.transfer16(buffer16, 6);
     }
 
     // Registers
@@ -131,6 +132,10 @@ int main()
         delayMs(1);
     }
 
+    transaction.writeRegister(0x62, 4);
+    auto whoAmIVaule = transaction.readRegister(0x4f);
+    printf("Who am I value: %x\n", whoAmIVaule);
+
     while (true)
         delayMs(1000);
 }
-- 
GitLab