Lots of usability improvements for the I2C API. Better docs and new top-level functions. (ARMmbed#64)

Jamie Smith · web-flow · commit ffc33676d1c4 · 2022-10-02T21:27:33.000-07:00
* Lots of usability improvements for the I2C API.  Better docs and new top-level functions.

* Document frequencies

* Tabs to spaces

* More style fixes

* Run astyle

* Clean up docs

* Add note about addressing, change 10 bit to 11 bit

* Fix spellcheck

* Fix paste error

* Oops, fix accidental change
diff --git a/doxyfile_options b/doxyfile_options
@@ -571,7 +571,7 @@ INLINE_INFO            = NO
 # name. If set to NO, the members will appear in declaration order.
 # The default value is: YES.
 
-SORT_MEMBER_DOCS       = YES
+SORT_MEMBER_DOCS       = NO
 
 # If the SORT_BRIEF_DOCS tag is set to YES then doxygen will sort the brief
 # descriptions of file, namespace and class members alphabetically by member
diff --git a/drivers/include/drivers/I2C.h b/drivers/include/drivers/I2C.h
diff --git a/drivers/source/I2C.cpp b/drivers/source/I2C.cpp
@@ -23,6 +23,7 @@
 
 #if DEVICE_I2C_ASYNCH
 #include "platform/mbed_power_mgmt.h"
+#include "rtos/EventFlags.h"
 #endif
 
 namespace mbed {
@@ -69,38 +70,41 @@ void I2C::frequency(int hz)
 }
 
 // write - Master Transmitter Mode
-int I2C::write(int address, const char *data, int length, bool repeated)
+I2C::Result I2C::write(int address, const char *data, int length, bool repeated)
 {
     lock();
 
     int stop = (repeated) ? 0 : 1;
     int written = i2c_write(&_i2c, address, data, length, stop);
 
     unlock();
-    return length != written;
-}
 
-int I2C::write(int data)
-{
-    lock();
-    int ret = i2c_byte_write(&_i2c, data);
-    unlock();
-    return ret;
+    // Note: C i2c_write() function does not distinguish between NACKs and errors, so assume NACK if read did not go through
+    return length == written ? Result::ACK : Result::NACK;
 }
 
 // read - Master Receiver Mode
-int I2C::read(int address, char *data, int length, bool repeated)
+I2C::Result I2C::read(int address, char *data, int length, bool repeated)
 {
     lock();
 
     int stop = (repeated) ? 0 : 1;
     int read = i2c_read(&_i2c, address, data, length, stop);
 
     unlock();
-    return length != read;
+
+    // Note: C i2c_read() function does not distinguish between NACKs and errors, so assume NACK if read did not go through
+    return length == read ? Result::ACK : Result::NACK;
+}
+
+void I2C::start(void)
+{
+    lock();
+    i2c_start(&_i2c);
+    unlock();
 }
 
-int I2C::read(int ack)
+int I2C::read_byte(bool ack)
 {
     lock();
     int ret;
@@ -113,11 +117,39 @@ int I2C::read(int ack)
     return ret;
 }
 
-void I2C::start(void)
+I2C::Result I2C::write_byte(int data)
 {
     lock();
-    i2c_start(&_i2c);
+    int ret = i2c_byte_write(&_i2c, data);
     unlock();
+
+    switch (ret) {
+        case 0:
+            return Result::NACK;
+        case 1:
+            return Result::ACK;
+        case 2:
+            return Result::TIMEOUT;
+        default:
+            return Result::OTHER_ERROR;
+    }
+}
+
+int I2C::write(int data)
+{
+    auto result = write_byte(data);
+
+    // Replicate the legacy return code
+    switch (result) {
+        case Result::ACK:
+            return 1;
+        case Result::NACK:
+            return 0;
+        case Result::TIMEOUT:
+            return 2;
+        default:
+            return static_cast<int>(result);
+    }
 }
 
 void I2C::stop(void)
@@ -210,6 +242,49 @@ void I2C::abort_transfer(void)
     unlock();
 }
 
+I2C::Result I2C::transfer_and_wait(int address, const char *tx_buffer, int tx_length, char *rx_buffer, int rx_length, rtos::Kernel::Clock::duration_u32 timeout, bool repeated)
+{
+    // Use EventFlags to suspend the thread until the transfer finishes
+    rtos::EventFlags transferResultFlags("I2C::Result EvFlags");
+
+    // Simple callback from the transfer that sets the EventFlags using the I2C result event
+    event_callback_t transferCallback([&](int event) {
+        transferResultFlags.set(event);
+    });
+
+    transfer(address, tx_buffer, tx_length, rx_buffer, rx_length, transferCallback, I2C_EVENT_ALL, repeated);
+
+    // Wait until transfer complete, error, or timeout
+    uint32_t result = transferResultFlags.wait_any_for(I2C_EVENT_ALL, timeout);
+
+    if (result & osFlagsError) {
+        if (result == osFlagsErrorTimeout) {
+            // Timeout expired, cancel transfer.
+            abort_transfer();
+            return Result::TIMEOUT;
+        } else {
+            // Other event flags error.  Transfer might be still running so cancel it.
+            abort_transfer();
+            return Result::OTHER_ERROR;
+        }
+    } else {
+        // Note: Cannot use a switch here because multiple flags might be set at the same time (possible
+        // in the STM32 HAL code at least).
+        if (result & I2C_EVENT_TRANSFER_COMPLETE) {
+            return Result::ACK;
+        } else if ((result & I2C_EVENT_ERROR_NO_SLAVE) || (result & I2C_EVENT_TRANSFER_EARLY_NACK)) {
+            // Both of these events mean that a NACK was received somewhere.  Theoretically NO_SLAVE means
+            // NACK while transmitting address and EARLY_NACK means nack during the write operation.
+            // But these aren't distinguished in the Result enum and even some of the HALs treat them
+            // interchangeably.
+            return Result::NACK;
+        } else {
+            // Other / unknown error code
+            return Result::OTHER_ERROR;
+        }
+    }
+}
+
 void I2C::irq_handler_asynch(void)
 {
     int event = i2c_irq_handler_asynch(&_i2c);
diff --git a/hal/include/hal/i2c_api.h b/hal/include/hal/i2c_api.h
@@ -190,7 +190,7 @@ int  i2c_stop(i2c_t *obj);
 /** Blocking reading data
  *
  *  @param obj     The I2C object
- *  @param address 7-bit address (last bit is 1)
+ *  @param address 8/11-bit address (last bit is 1)
  *  @param data    The buffer for receiving
  *  @param length  Number of bytes to read
  *  @param stop    Stop to be generated after the transfer is done
@@ -201,7 +201,7 @@ int i2c_read(i2c_t *obj, int address, char *data, int length, int stop);
 /** Blocking sending data
  *
  *  @param obj     The I2C object
- *  @param address 7-bit address (last bit is 0)
+ *  @param address 8/11-bit address (last bit is 0)
  *  @param data    The buffer for sending
  *  @param length  Number of bytes to write
  *  @param stop    Stop to be generated after the transfer is done
@@ -334,7 +334,7 @@ void i2c_slave_address(i2c_t *obj, int idx, uint32_t address, uint32_t mask);
  *  @param tx_length The number of bytes to transmit
  *  @param rx        The receive buffer
  *  @param rx_length The number of bytes to receive
- *  @param address   The address to be set - 7bit or 9bit
+ *  @param address   The address to be set - 8bit or 11bit
  *  @param stop      If true, stop will be generated after the transfer is done
  *  @param handler   The I2C IRQ handler to be set
  *  @param event     Event mask for the transfer. See \ref hal_I2CEvents
diff --git a/rtos/include/rtos/EventFlags.h b/rtos/include/rtos/EventFlags.h
@@ -101,7 +101,7 @@ class EventFlags : private mbed::NonCopyable<EventFlags> {
 
     /** Wait for all of the specified event flags to become signaled.
       @param   flags    the flags to wait for.
-      @param   rel_time timeout value.
+      @param   rel_time timeout value.  Use \link Kernel::wait_for_u32_forever \endlink to wait forever.
       @param   clear    clear specified event flags after waiting for them (default: true).
       @return  event flags before clearing or error code if highest bit set (see @a osFlagsError for details).
 
@@ -131,7 +131,7 @@ class EventFlags : private mbed::NonCopyable<EventFlags> {
 
     /** Wait for any of the specified event flags to become signaled.
       @param   flags    the flags to wait for.
-      @param   rel_time timeout value.
+      @param   rel_time timeout value.  Use \link Kernel::wait_for_u32_forever \endlink to wait forever.
       @param   clear    clear specified event flags after waiting for them (default: true).
       @return  event flags before clearing or error code if highest bit set (see @a osFlagsError for details).
 
diff --git a/storage/blockdevice/COMPONENT_I2CEE/source/I2CEEBlockDevice.cpp b/storage/blockdevice/COMPONENT_I2CEE/source/I2CEEBlockDevice.cpp
@@ -73,21 +73,25 @@ int I2CEEBlockDevice::read(void *buffer, bd_addr_t addr, bd_size_t size)
 
     _i2c->start();
 
-    if (1 != _i2c->write(get_paged_device_address(addr))) {
+    if (I2C::ACK != _i2c->write_byte(get_paged_device_address(addr))) {
+        _i2c->stop();
         return BD_ERROR_DEVICE_ERROR;
     }
 
-    if (!_address_is_eight_bit && 1 != _i2c->write((char)(addr >> 8u))) {
+    if (!_address_is_eight_bit && I2C::ACK != _i2c->write_byte((char)(addr >> 8u))) {
+        _i2c->stop();
         return BD_ERROR_DEVICE_ERROR;
     }
 
-    if (1 != _i2c->write((char)(addr & 0xffu))) {
+    if (I2C::ACK != _i2c->write_byte((char)(addr & 0xffu))) {
+        _i2c->stop();
         return BD_ERROR_DEVICE_ERROR;
     }
 
-    _i2c->stop();
+    // Note: We do not send an I2C stop in this case, because we will do a repeated start in the next
+    // call.
 
-    if (0 != _i2c->read(_i2c_addr, pBuffer, size)) {
+    if (I2C::ACK != _i2c->read(_i2c_addr | 1, pBuffer, static_cast<int>(size))) {
         return BD_ERROR_DEVICE_ERROR;
     }
 
@@ -108,20 +112,24 @@ int I2CEEBlockDevice::program(const void *buffer, bd_addr_t addr, bd_size_t size
 
         _i2c->start();
 
-        if (1 != _i2c->write(get_paged_device_address(addr))) {
+        if (I2C::ACK != _i2c->write_byte(get_paged_device_address(addr))) {
+            _i2c->stop();
             return BD_ERROR_DEVICE_ERROR;
         }
 
-        if (!_address_is_eight_bit && 1 != _i2c->write((char)(addr >> 8u))) {
+        if (!_address_is_eight_bit && I2C::ACK != _i2c->write_byte((char)(addr >> 8u))) {
+            _i2c->stop();
             return BD_ERROR_DEVICE_ERROR;
         }
 
-        if (1 != _i2c->write((char)(addr & 0xffu))) {
+        if (I2C::ACK != _i2c->write_byte((char)(addr & 0xffu))) {
+            _i2c->stop();
             return BD_ERROR_DEVICE_ERROR;
         }
 
         for (unsigned i = 0; i < chunk; i++) {
-            if (1 != _i2c->write(pBuffer[i])) {
+            if (I2C::ACK != _i2c->write_byte(pBuffer[i])) {
+                _i2c->stop();
                 return BD_ERROR_DEVICE_ERROR;
             }
         }
@@ -154,7 +162,7 @@ int I2CEEBlockDevice::_sync()
     // so loop trying to do a zero byte write until it is ACKed
     // by the chip.
     for (int i = 0; i < I2CEE_TIMEOUT; i++) {
-        if (_i2c->write(_i2c_addr | 0, 0, 0) < 1) {
+        if (_i2c->write(_i2c_addr | 0, 0, 0) == I2C::ACK) {
             return 0;
         }
         wait_us(100);
diff --git a/tools/test/ci/doxy-spellchecker/ignore.en.pws b/tools/test/ci/doxy-spellchecker/ignore.en.pws
@@ -123,3 +123,6 @@ KVStore
 _doxy_
 nothrow
 conf
+transactional
+errored
+natively
