From 9daf0f13528ed19f536bc5483e71a2d2b491866a Mon Sep 17 00:00:00 2001
From: Emilio Corigliano <emilio.corigliano@skywarder.eu>
Date: Sat, 18 Mar 2023 14:18:08 +0000
Subject: [PATCH] [I2C] Added high level methods, modified pin modes, deleted
 copy/move constructors and operators and bugfix in SyncI2C

- Added high level methods to read multiple bytes and write a single register
- Deleted copy/move constructors and operators
- Fixed SyncedI2C::readRegister method
- Changed pin modes from ALTERNATE_OD to ALTERNATE_OD_PULL_UP
- Modified tests in order to try also the new methods
- Uniformed passing of const reference in low-level and high-level drivers
---
 src/shared/drivers/i2c/I2C.cpp       |  54 +++++++++++---
 src/shared/drivers/i2c/I2C.h         | 102 ++++++++++++++++++++++++---
 src/shared/drivers/i2c/I2CDriver.cpp |  12 ++--
 src/shared/drivers/i2c/I2CDriver.h   |  10 ++-
 src/tests/drivers/i2c/test-i2c.cpp   |  33 ++++++---
 5 files changed, 175 insertions(+), 36 deletions(-)

diff --git a/src/shared/drivers/i2c/I2C.cpp b/src/shared/drivers/i2c/I2C.cpp
index 5242318f4..a58a3049d 100644
--- a/src/shared/drivers/i2c/I2C.cpp
+++ b/src/shared/drivers/i2c/I2C.cpp
@@ -25,33 +25,52 @@
 namespace Boardcore
 {
 
-I2C::I2C(I2C_TypeDef *i2c, miosix::GpioPin scl, miosix::GpioPin sda)
+I2C::I2C(I2C_TypeDef *i2c, const miosix::GpioPin &scl,
+         const miosix::GpioPin &sda)
     : i2c(i2c, scl, sda)
 {
 }
 
 bool I2C::read(const I2CDriver::I2CSlaveConfig &slaveConfig, void *buffer,
-               size_t nBytes)
+               const size_t &nBytes)
 {
     i2c.flushBus();
     return i2c.read(slaveConfig, buffer, nBytes);
 }
 
 bool I2C::write(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                const void *buffer, size_t nBytes)
+                const void *buffer, const size_t &nBytes)
 {
     i2c.flushBus();
     return i2c.write(slaveConfig, buffer, nBytes);
 }
 
 bool I2C::readRegister(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                       const uint8_t registerAddress, uint8_t &registerContent)
+                       const uint8_t &registerAddress, uint8_t &registerContent)
 {
     i2c.flushBus();
     return i2c.write(slaveConfig, &registerAddress, 1, false) &&
            i2c.read(slaveConfig, &registerContent, 1);
 }
 
+bool I2C::writeRegister(const I2CDriver::I2CSlaveConfig &slaveConfig,
+                        const uint8_t &registerAddress,
+                        const uint8_t &registerContent)
+{
+    const uint8_t reg[2] = {registerAddress, registerContent};
+    i2c.flushBus();
+    return i2c.write(slaveConfig, reg, 2);
+}
+
+bool I2C::readFromRegister(const I2CDriver::I2CSlaveConfig &slaveConfig,
+                           const uint8_t &registerAddress, void *buffer,
+                           const size_t &nBytes)
+{
+    i2c.flushBus();
+    return i2c.write(slaveConfig, &registerAddress, 1, false) &&
+           i2c.read(slaveConfig, buffer, nBytes);
+}
+
 bool I2C::probe(const I2CDriver::I2CSlaveConfig &slaveConfig)
 {
     i2c.flushBus();
@@ -60,33 +79,50 @@ bool I2C::probe(const I2CDriver::I2CSlaveConfig &slaveConfig)
 
 uint16_t I2C::getLastError() { return i2c.getLastError(); }
 
-SyncedI2C::SyncedI2C(I2C_TypeDef *i2c, miosix::GpioPin scl, miosix::GpioPin sda)
+SyncedI2C::SyncedI2C(I2C_TypeDef *i2c, const miosix::GpioPin &scl,
+                     const miosix::GpioPin &sda)
     : I2C(i2c, scl, sda)
 {
 }
 
 bool SyncedI2C::read(const I2CDriver::I2CSlaveConfig &slaveConfig, void *buffer,
-                     size_t nBytes)
+                     const size_t &nBytes)
 {
     miosix::Lock<miosix::FastMutex> lock(mutex);
     return I2C::read(slaveConfig, buffer, nBytes);
 }
 
 bool SyncedI2C::write(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                      const void *buffer, size_t nBytes)
+                      const void *buffer, const size_t &nBytes)
 {
     miosix::Lock<miosix::FastMutex> lock(mutex);
     return I2C::write(slaveConfig, buffer, nBytes);
 }
 
 bool SyncedI2C::readRegister(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                             const uint8_t registerAddress,
-                             uint8_t registerContent)
+                             const uint8_t &registerAddress,
+                             uint8_t &registerContent)
 {
     miosix::Lock<miosix::FastMutex> lock(mutex);
     return I2C::readRegister(slaveConfig, registerAddress, registerContent);
 }
 
+bool SyncedI2C::writeRegister(const I2CDriver::I2CSlaveConfig &slaveConfig,
+                              const uint8_t &registerAddress,
+                              const uint8_t &registerContent)
+{
+    miosix::Lock<miosix::FastMutex> lock(mutex);
+    return I2C::writeRegister(slaveConfig, registerAddress, registerContent);
+}
+
+bool SyncedI2C::readFromRegister(const I2CDriver::I2CSlaveConfig &slaveConfig,
+                                 const uint8_t &registerAddress, void *buffer,
+                                 const size_t &nBytes)
+{
+    miosix::Lock<miosix::FastMutex> lock(mutex);
+    return I2C::readFromRegister(slaveConfig, registerAddress, buffer, nBytes);
+}
+
 bool SyncedI2C::probe(const I2CDriver::I2CSlaveConfig &slaveConfig)
 {
     miosix::Lock<miosix::FastMutex> lock(mutex);
diff --git a/src/shared/drivers/i2c/I2C.h b/src/shared/drivers/i2c/I2C.h
index 071925e8e..42835840e 100644
--- a/src/shared/drivers/i2c/I2C.h
+++ b/src/shared/drivers/i2c/I2C.h
@@ -49,7 +49,14 @@ public:
      * @param scl Serial clock GpioPin of the relative I2C peripheral.
      * @param sda Serial data GpioPin of the relative I2C peripheral.
      */
-    I2C(I2C_TypeDef *i2c, miosix::GpioPin scl, miosix::GpioPin sda);
+    I2C(I2C_TypeDef *i2c, const miosix::GpioPin &scl,
+        const miosix::GpioPin &sda);
+
+    ///< Delete copy/move constructors/operators.
+    I2C(const I2C &)            = delete;
+    I2C &operator=(const I2C &) = delete;
+    I2C(I2C &&)                 = delete;
+    I2C &operator=(I2C &&)      = delete;
 
     /**
      * @brief Non blocking read operation to read nBytes.
@@ -64,7 +71,7 @@ public:
      * @returns True if the read is successful, false otherwise.
      */
     [[nodiscard]] bool read(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                            void *buffer, size_t nBytes);
+                            void *buffer, const size_t &nBytes);
 
     /**
      * @brief Non blocking write operation to write nBytes.
@@ -79,7 +86,7 @@ public:
      * @returns True if the write is successful, false otherwise.
      */
     [[nodiscard]] bool write(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                             const void *buffer, size_t nBytes);
+                             const void *buffer, const size_t &nBytes);
 
     /**
      * @brief Non blocking operation to read a 1-byte register from a slave.
@@ -95,7 +102,40 @@ public:
      */
     [[nodiscard]] bool readRegister(
         const I2CDriver::I2CSlaveConfig &slaveConfig,
-        const uint8_t registerAddress, uint8_t &registerContent);
+        const uint8_t &registerAddress, uint8_t &registerContent);
+
+    /**
+     * @brief Non blocking operation to write a 1-byte register from a slave.
+     *
+     * This method, if necessary, flushes the bus before the write operation is
+     * performed. In case of an error during the communication, this method
+     * returns false immediately.
+     * @warning Check always if the operation succeeded or not!
+     * @param slaveConfig The configuration struct of the slave device.
+     * @param registerAddress Byte that represents the address of the register.
+     * @param registerContent The content to write on the register.
+     * @returns True if the write is successful, false otherwise.
+     */
+    [[nodiscard]] bool writeRegister(
+        const I2CDriver::I2CSlaveConfig &slaveConfig,
+        const uint8_t &registerAddress, const uint8_t &registerContent);
+
+    /**
+     * @brief Non blocking operation to read n-bytes from register from a slave.
+     *
+     * This method, if necessary, flushes the bus before the read operation is
+     * performed. In case of an error during the communication, this method
+     * returns false immediately.
+     * @warning Check always if the operation succeeded or not!
+     * @param slaveConfig The configuration struct of the slave device.
+     * @param registerAddress Byte that represents the address of the register.
+     * @param buffer Data buffer where to store the data read.
+     * @param nBytes Number of bytes to read.
+     * @returns True if the write is successful, false otherwise.
+     */
+    [[nodiscard]] bool readFromRegister(
+        const I2CDriver::I2CSlaveConfig &slaveConfig,
+        const uint8_t &registerAddress, void *buffer, const size_t &nBytes);
 
     /**
      * @brief Non blocking operation to check if a slave is available.
@@ -110,7 +150,7 @@ public:
      * @brief Returns the last errors happened in the communication.
      *
      * For checking if a specific error occurred in the last transaction you can
-     * do `if(getLastError() & Errors::<error-to-check>)`. Do not use `==` to
+     * do `if(getLastError()  &Errors::<error-to-check>)`. Do not use `==` to
      * check for errors because there could be more errors at once. To check if
      * no errors occurred use `if(getLastError() == Errors::NO_ERROR)` or simply
      * `if(!getLastError())`
@@ -138,7 +178,14 @@ public:
      * @param scl Serial clock GpioPin of the relative I2C peripheral.
      * @param sda Serial data GpioPin of the relative I2C peripheral.
      */
-    SyncedI2C(I2C_TypeDef *i2c, miosix::GpioPin scl, miosix::GpioPin sda);
+    SyncedI2C(I2C_TypeDef *i2c, const miosix::GpioPin &scl,
+              const miosix::GpioPin &sda);
+
+    ///< Delete copy/move constructors/operators.
+    SyncedI2C(const SyncedI2C &)            = delete;
+    SyncedI2C &operator=(const SyncedI2C &) = delete;
+    SyncedI2C(SyncedI2C &&)                 = delete;
+    SyncedI2C &operator=(SyncedI2C &&)      = delete;
 
     /**
      * @brief Read operation to read nBytes.
@@ -153,7 +200,7 @@ public:
      * @returns True if the read is successful, false otherwise.
      */
     [[nodiscard]] bool read(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                            void *buffer, size_t nBytes);
+                            void *buffer, const size_t &nBytes);
 
     /**
      * @brief Write operation to write nBytes.
@@ -168,7 +215,7 @@ public:
      * @returns True if the write is successful, false otherwise.
      */
     [[nodiscard]] bool write(const I2CDriver::I2CSlaveConfig &slaveConfig,
-                             const void *buffer, size_t nBytes);
+                             const void *buffer, const size_t &nBytes);
 
     /**
      * @brief Read a one-byte register from the device.
@@ -180,11 +227,44 @@ public:
      * @param slaveConfig The configuration struct of the slave device.
      * @param registerAddress Byte that represents the address of the register.
      * @param registerContent Where to store the content of the register.
-     * @returns True if the write is successful, false otherwise.
+     * @returns True if the read is successful, false otherwise.
      */
     [[nodiscard]] bool readRegister(
         const I2CDriver::I2CSlaveConfig &slaveConfig,
-        const uint8_t registerAddress, uint8_t registerContent);
+        const uint8_t &registerAddress, uint8_t &registerContent);
+
+    /**
+     * @brief Write a one-byte register from the device.
+     *
+     * This method could have to wait that no other thread is trying to do some
+     * operation on the bus. In case of an error during the communication, this
+     * method returns false immediately.
+     * @warning Check always if the operation succeeded or not!
+     * @param slaveConfig The configuration struct of the slave device.
+     * @param registerAddress Byte that represents the address of the register.
+     * @param registerContent The content to write on the register.
+     * @returns True if the write is successful, false otherwise.
+     */
+    [[nodiscard]] bool writeRegister(
+        const I2CDriver::I2CSlaveConfig &slaveConfig,
+        const uint8_t &registerAddress, const uint8_t &registerContent);
+
+    /**
+     * @brief Read n-bytes from register from a slave.
+     *
+     * This method could have to wait that no other thread is trying to do some
+     * operation on the bus. In case of an error during the communication, this
+     * method returns false immediately.
+     * @warning Check always if the operation succeeded or not!
+     * @param slaveConfig The configuration struct of the slave device.
+     * @param registerAddress Byte that represents the address of the register.
+     * @param buffer Data buffer where to store the data read.
+     * @param nBytes Number of bytes to read.
+     * @returns True if the write is successful, false otherwise.
+     */
+    [[nodiscard]] bool readFromRegister(
+        const I2CDriver::I2CSlaveConfig &slaveConfig,
+        const uint8_t &registerAddress, void *buffer, const size_t &nBytes);
 
     /**
      * @brief Check if a slave is available.
@@ -201,7 +281,7 @@ public:
      * @brief Returns the last errors happened in the communication.
      *
      * For checking if a specific error occurred in the last transaction you can
-     * do `if(getLastError() & Errors::<error-to-check>)`. Do not use `==` to
+     * do `if(getLastError()  &Errors::<error-to-check>)`. Do not use `==` to
      * check for errors because there could be more errors at once. To check if
      * no errors occurred use `if(getLastError() == Errors::NO_ERROR)` or simply
      * `if(!getLastError())`
diff --git a/src/shared/drivers/i2c/I2CDriver.cpp b/src/shared/drivers/i2c/I2CDriver.cpp
index 99a879920..53a938783 100644
--- a/src/shared/drivers/i2c/I2CDriver.cpp
+++ b/src/shared/drivers/i2c/I2CDriver.cpp
@@ -278,8 +278,8 @@ I2CDriver::I2CDriver(I2C_TypeDef *i2c, miosix::GpioPin scl, miosix::GpioPin sda)
         // logical values.
         scl.alternateFunction(I2CConsts::I2C_PIN_ALTERNATE_FUNCTION);
         sda.alternateFunction(I2CConsts::I2C_PIN_ALTERNATE_FUNCTION);
-        scl.mode(miosix::Mode::ALTERNATE_OD);
-        sda.mode(miosix::Mode::ALTERNATE_OD);
+        scl.mode(miosix::Mode::ALTERNATE_OD_PULL_UP);
+        sda.mode(miosix::Mode::ALTERNATE_OD_PULL_UP);
     }
 
     // Checking that this particular I2C port hasn't been already instantiated
@@ -382,7 +382,7 @@ void I2CDriver::setupPeripheral(const I2CSlaveConfig &slaveConfig)
 }
 
 bool I2CDriver::read(const I2CSlaveConfig &slaveConfig, void *buffer,
-                     size_t nBytes)
+                     const size_t &nBytes)
 {
     // Setting up the read transaction
     transaction.operation    = Operation::READ;
@@ -403,7 +403,7 @@ bool I2CDriver::read(const I2CSlaveConfig &slaveConfig, void *buffer,
 };
 
 bool I2CDriver::write(const I2CSlaveConfig &slaveConfig, const void *buffer,
-                      size_t nBytes, bool generateStop)
+                      const size_t &nBytes, bool generateStop)
 {
     // Setting up the write transaction
     transaction.operation    = Operation::WRITE;
@@ -520,7 +520,7 @@ void I2CDriver::flushBus()
         // Recovery from the locked state due to a stuck Slave.
         // We bit-bang 16 clocks on the scl line in order to restore pending
         // packets of the slaves.
-        scl.mode(miosix::Mode::OPEN_DRAIN);
+        scl.mode(miosix::Mode::OPEN_DRAIN_PULL_UP);
     }
 
     for (size_t c = 0; c < I2CConsts::N_SCL_BITBANG; c++)
@@ -535,7 +535,7 @@ void I2CDriver::flushBus()
         miosix::FastInterruptDisableLock dLock;
 
         // We set again the scl pin to the correct Alternate function
-        scl.mode(miosix::Mode::ALTERNATE_OD);
+        scl.mode(miosix::Mode::ALTERNATE_OD_PULL_UP);
         scl.alternateFunction(I2CConsts::I2C_PIN_ALTERNATE_FUNCTION);
     }
 
diff --git a/src/shared/drivers/i2c/I2CDriver.h b/src/shared/drivers/i2c/I2CDriver.h
index d1301e336..cf205d404 100644
--- a/src/shared/drivers/i2c/I2CDriver.h
+++ b/src/shared/drivers/i2c/I2CDriver.h
@@ -121,6 +121,12 @@ public:
      */
     I2CDriver(I2C_TypeDef *i2c, miosix::GpioPin scl, miosix::GpioPin sda);
 
+    ///< Delete copy/move constructors/operators.
+    I2CDriver(const I2CDriver &)            = delete;
+    I2CDriver &operator=(const I2CDriver &) = delete;
+    I2CDriver(I2CDriver &&)                 = delete;
+    I2CDriver &operator=(I2CDriver &&)      = delete;
+
     /**
      * @brief Disables the peripheral, the interrupts in the NVIC and the
      * peripheral's clock.
@@ -138,7 +144,7 @@ public:
      * @return True if the read is successful, false otherwise.
      */
     [[nodiscard]] bool read(const I2CSlaveConfig &slaveConfig, void *buffer,
-                            size_t nBytes);
+                            const size_t &nBytes);
 
     /**
      * @brief Write operation to write nBytes. In case of an error during the
@@ -152,7 +158,7 @@ public:
      * @return True if the write is successful, false otherwise.
      */
     [[nodiscard]] bool write(const I2CSlaveConfig &slaveConfig,
-                             const void *buffer, size_t nBytes,
+                             const void *buffer, const size_t &nBytes,
                              bool generateStop = true);
 
     /**
diff --git a/src/tests/drivers/i2c/test-i2c.cpp b/src/tests/drivers/i2c/test-i2c.cpp
index ee9ba2587..38d1554ef 100644
--- a/src/tests/drivers/i2c/test-i2c.cpp
+++ b/src/tests/drivers/i2c/test-i2c.cpp
@@ -67,8 +67,6 @@ typedef miosix::Gpio<GPIOH_BASE, 8> i3scl2;
  * one sensor (in order to provoke a locked state).
  */
 
-uint8_t buffer = 0;
-
 typedef struct
 {
     // cppcheck-suppress unusedStructMember
@@ -91,16 +89,21 @@ I2CDriver::I2CSlaveConfig BME280Config{BME280.addressSensor,
 I2CDriver::I2CSlaveConfig OLEDConfig{
     OLED.addressSensor, I2CDriver::Addressing::BIT7, I2CDriver::Speed::FAST};
 
-bool i2cDriver(I2C &i2c, I2CSensor sensor,
+bool i2cDriver(SyncedI2C &i2c, I2CSensor sensor,
                I2CDriver::I2CSlaveConfig sensorConfig)
 {
-    buffer = 0;
+    uint8_t whoamiContent = 0;
+    const size_t nRead    = 300;
+    uint8_t buffer[nRead] = {0};
 
     // reset the sensor and then read the whoami
     if (!(i2c.probe(sensorConfig) &&
-          i2c.write(sensorConfig, sensor.softReset, 2) &&
-          i2c.readRegister(sensorConfig, sensor.whoamiRegister, buffer) &&
-          buffer == sensor.whoamiContent))
+          i2c.writeRegister(sensorConfig, sensor.softReset[0],
+                            sensor.softReset[1]) &&
+          i2c.readRegister(sensorConfig, sensor.whoamiRegister,
+                           whoamiContent) &&
+          i2c.readFromRegister(sensorConfig, sensor.whoamiRegister, buffer,
+                               nRead)))
     {
         uint16_t lastError{i2c.getLastError()};
         if (!(lastError & I2CDriver::Errors::AF))
@@ -110,12 +113,26 @@ bool i2cDriver(I2C &i2c, I2CSensor sensor,
         return false;
     }
 
+    if (whoamiContent != sensor.whoamiContent)
+    {
+        printf("whoami expected %d, received %d\n", sensor.whoamiContent,
+               whoamiContent);
+        return false;
+    }
+
+    if (buffer[0] != sensor.whoamiContent)
+    {
+        printf("buffer[0] expected %d, received %d\n", sensor.whoamiContent,
+               buffer[0]);
+        return false;
+    }
+
     return true;
 }
 
 int main()
 {
-    int nRepeat = 50;
+    int nRepeat = 10;
 
     // thread that uses 100% CPU
     std::thread t(
-- 
GitLab