Implement I2CDevice class on EV3

bricks/_common/sources.mk

@@ -57,7 +57,7 @@ PYBRICKS_PYBRICKS_SRC_C = $(addprefix pybricks/,\
 	hubs/pb_type_virtualhub.c \
 	iodevices/pb_module_iodevices.c \
 	iodevices/pb_type_iodevices_analogsensor.c \
-	iodevices/pb_type_iodevices_i2cdevice.c \
+	iodevices/pb_type_i2c_device.c \
 	iodevices/pb_type_iodevices_lwp3device.c \
 	iodevices/pb_type_iodevices_pupdevice.c \
 	iodevices/pb_type_iodevices_xbox_controller.c \

bricks/ev3/mpconfigport.h

@@ -30,6 +30,7 @@
 #define PYBRICKS_PY_EXPERIMENTAL                (1)
 #define PYBRICKS_PY_HUBS                        (1)
 #define PYBRICKS_PY_IODEVICES                   (1)
+#define PYBRICKS_PY_IODEVICES_I2CDEVICE         (1)
 #define PYBRICKS_PY_IODEVICES_XBOX_CONTROLLER   (0)
 #define PYBRICKS_PY_MEDIA                       (1)
 #define PYBRICKS_PY_NXTDEVICES                  (1)

lib/pbio/drv/i2c/i2c_ev3.c

@@ -27,7 +27,7 @@
 #include "../drv/rproc/rproc.h"
 #include "../rproc/rproc_ev3.h"
 
-#define DEBUG 1
+#define DEBUG 0
 #if DEBUG
 #include <stdio.h>
 #include <inttypes.h>
@@ -43,13 +43,18 @@
 // Rounded up to a nice power of 2 and multiple of cache lines
 #define PRU_I2C_MAX_BYTES_PER_TXN   512
 
+// Number of times we try the operation before NAK is raised as an IO error.
+#define PRU_I2C_MAX_NUM_TRIES_ON_NAK (2)
+
 static uint8_t pbdrv_i2c_buffers[PBDRV_RPROC_EV3_PRU1_NUM_I2C_BUSES][PRU_I2C_MAX_BYTES_PER_TXN] PBDRV_DMA_BUF;
 
 struct _pbdrv_i2c_dev_t {
     uint8_t *buffer;
     volatile bool is_busy;
     bool is_initialized;
     uint8_t pru_i2c_idx;
+    pbio_os_timer_t timer;
+    size_t try_count;
 };
 
 static pbdrv_i2c_dev_t i2c_devs[PBDRV_RPROC_EV3_PRU1_NUM_I2C_BUSES];
@@ -105,7 +110,7 @@ pbio_error_t pbdrv_i2c_write_then_read(
     uint8_t dev_addr,
     const uint8_t *wdata,
     size_t wlen,
-    uint8_t *rdata,
+    uint8_t **rdata,
     size_t rlen,
     bool nxt_quirk) {
 
@@ -114,7 +119,7 @@ pbio_error_t pbdrv_i2c_write_then_read(
     if (wlen && !wdata) {
         return PBIO_ERROR_INVALID_ARG;
     }
-    if (rlen && !rdata) {
+    if (*rdata) {
         return PBIO_ERROR_INVALID_ARG;
     }
     if (wlen > 0xff || rlen > 0xff) {
@@ -129,45 +134,59 @@ pbio_error_t pbdrv_i2c_write_then_read(
     if (wlen) {
         memcpy(i2c_dev->buffer, wdata, wlen);
     }
-    i2c_dev->is_busy = true;
-    pbdrv_cache_prepare_before_dma(i2c_dev->buffer, PRU_I2C_MAX_BYTES_PER_TXN);
-
-    // Kick off transfer
-    pbdrv_rproc_ev3_pru1_shared_ram.i2c[i2c_dev->pru_i2c_idx].flags = PBDRV_RPROC_EV3_PRU1_I2C_PACK_FLAGS(
-        dev_addr,
-        rlen,
-        wlen,
-        PBDRV_RPROC_EV3_PRU1_I2C_CMD_START | (nxt_quirk ? PBDRV_RPROC_EV3_PRU1_I2C_CMD_NXT_QUIRK : 0)
-        );
-
-    // Wait for transfer to finish
-    PBIO_OS_AWAIT_WHILE(state, i2c_dev->is_busy);
-
-    uint32_t flags = pbdrv_rproc_ev3_pru1_shared_ram.i2c[i2c_dev->pru_i2c_idx].flags;
-    debug_pr("i2c %d done flags %08x\r\n", i2c_dev->pru_i2c_idx, flags);
-    if (!(flags & PBDRV_RPROC_EV3_PRU1_I2C_STAT_DONE)) {
-        debug_pr("i2c %d not actually done???\r\n", i2c_dev->pru_i2c_idx);
-        return PBIO_ERROR_FAILED;
-    }
-    switch (flags & PBDRV_RPROC_EV3_PRU1_I2C_STAT_MASK) {
-        case PBDRV_RPROC_EV3_PRU1_I2C_STAT_OK:
-            break;
-        case PBDRV_RPROC_EV3_PRU1_I2C_STAT_TIMEOUT:
+
+    for (i2c_dev->try_count = 0; i2c_dev->try_count < PRU_I2C_MAX_NUM_TRIES_ON_NAK; i2c_dev->try_count++) {
+
+        if (nxt_quirk) {
+            // NXT sensors affected by the quirk can't be accessed too quickly.
+            // The timer is set after awaiting so we don't unnecessarily slow
+            // down code that polls less frequently.
+            PBIO_OS_AWAIT_UNTIL(state, pbio_os_timer_is_expired(&i2c_dev->timer));
+            pbio_os_timer_set(&i2c_dev->timer, 100);
+        }
+
+        i2c_dev->is_busy = true;
+        pbdrv_cache_prepare_before_dma(i2c_dev->buffer, PRU_I2C_MAX_BYTES_PER_TXN);
+
+        // Kick off transfer
+        pbdrv_rproc_ev3_pru1_shared_ram.i2c[i2c_dev->pru_i2c_idx].flags = PBDRV_RPROC_EV3_PRU1_I2C_PACK_FLAGS(
+            dev_addr,
+            rlen,
+            wlen,
+            PBDRV_RPROC_EV3_PRU1_I2C_CMD_START | (nxt_quirk ? PBDRV_RPROC_EV3_PRU1_I2C_CMD_NXT_QUIRK : 0)
+            );
+
+        // Wait for transfer to finish
+        PBIO_OS_AWAIT_WHILE(state, i2c_dev->is_busy);
+
+        uint32_t flags = pbdrv_rproc_ev3_pru1_shared_ram.i2c[i2c_dev->pru_i2c_idx].flags;
+        debug_pr("i2c %d done flags %08x\r\n", i2c_dev->pru_i2c_idx, flags);
+        if (!(flags & PBDRV_RPROC_EV3_PRU1_I2C_STAT_DONE)) {
+            debug_pr("i2c %d not actually done\r\n", i2c_dev->pru_i2c_idx);
+            return PBIO_ERROR_FAILED;
+        }
+
+        flags &= PBDRV_RPROC_EV3_PRU1_I2C_STAT_MASK;
+
+        if (flags == PBDRV_RPROC_EV3_PRU1_I2C_STAT_OK) {
+            // Success, exit loop.
+            pbdrv_cache_prepare_after_dma(i2c_dev->buffer, PRU_I2C_MAX_BYTES_PER_TXN);
+            if (rlen) {
+                *rdata = &i2c_dev->buffer[wlen];
+            }
+            return PBIO_SUCCESS;
+        } else if (flags == PBDRV_RPROC_EV3_PRU1_I2C_STAT_TIMEOUT) {
             return PBIO_ERROR_TIMEDOUT;
-        case PBDRV_RPROC_EV3_PRU1_I2C_STAT_NAK:
-            return PBIO_ERROR_IO;
-        default:
-            debug_pr("i2c %d unknown error occurred???\r\n", i2c_dev->pru_i2c_idx);
+        } else if (flags != PBDRV_RPROC_EV3_PRU1_I2C_STAT_NAK) {
+            // Many known NXT devices occasionally get NAK. Retrying usually
+            // helps. Everything else is unexpected and raised immediately.
+            debug_pr("i2c %d unknown error occurred\r\n", i2c_dev->pru_i2c_idx);
             return PBIO_ERROR_FAILED;
+        }
     }
 
-    // If we got here, there's no error. Copy RX data.
-    pbdrv_cache_prepare_after_dma(i2c_dev->buffer, PRU_I2C_MAX_BYTES_PER_TXN);
-    if (rlen) {
-        memcpy(rdata, &i2c_dev->buffer[wlen], rlen);
-    }
-
-    PBIO_OS_ASYNC_END(PBIO_SUCCESS);
+    // Didn't succeed even after trying allowed number of times.
+    PBIO_OS_ASYNC_END(PBIO_ERROR_IO);
 }
 
 static pbio_os_process_t ev3_i2c_init_process;
@@ -217,6 +236,7 @@ void pbdrv_i2c_init(void) {
         i2c->pru_i2c_idx = i;
         i2c->buffer = pbdrv_i2c_buffers[i];
         i2c->is_initialized = true;
+        pbio_os_timer_set(&i2c->timer, 0);
     }
 
     pbio_busy_count_up();

lib/pbio/drv/ioport/ioport.c

@@ -10,41 +10,42 @@
 #include <pbdrv/ioport.h>
 #include <pbdrv/uart.h>
 
-pbio_error_t pbdrv_ioport_p5p6_set_mode(const pbdrv_ioport_pins_t *pins, pbdrv_uart_dev_t *uart_dev, pbdrv_ioport_p5p6_mode_t mode) {
+/**
+ * Resets pins to the default state, which is input with the buffer disabled.
+ */
+static pbio_error_t pbdrv_ioport_p5p6_pin_reset(const pbdrv_ioport_pins_t *pins) {
 
-    if (mode == PBDRV_IOPORT_P5P6_MODE_GPIO_ADC) {
+    if (!pins) {
+        return PBIO_ERROR_NOT_SUPPORTED;
+    }
 
-        // Disables UART IRQ if it was enabled.
-        if (uart_dev) {
-            // Revisit: If we call this we should also re-enable IRQs
-            // when starting a new UART operation.
-            // pbdrv_uart_stop(uart_dev);
-        }
+    pbdrv_gpio_input(&pins->p5);
+    pbdrv_gpio_input(&pins->p6);
+    pbdrv_gpio_input(&pins->uart_tx);
+    pbdrv_gpio_input(&pins->uart_rx);
+    pbdrv_gpio_out_high(&pins->uart_buf);
 
-        // Reset pins if this port has GPIO pins.
-        if (!pins) {
-            return PBIO_ERROR_NOT_SUPPORTED;
-        }
+    // These should be set by default already, but it seems that the
+    // bootloader on the Technic hub changes these and causes wrong
+    // detection if we don't make sure pull is disabled.
+    pbdrv_gpio_set_pull(&pins->p5, PBDRV_GPIO_PULL_NONE);
+    pbdrv_gpio_set_pull(&pins->p6, PBDRV_GPIO_PULL_NONE);
+    pbdrv_gpio_set_pull(&pins->uart_buf, PBDRV_GPIO_PULL_NONE);
+    pbdrv_gpio_set_pull(&pins->uart_tx, PBDRV_GPIO_PULL_NONE);
+    pbdrv_gpio_set_pull(&pins->uart_rx, PBDRV_GPIO_PULL_NONE);
 
-        pbdrv_gpio_input(&pins->p5);
-        pbdrv_gpio_input(&pins->p6);
-        pbdrv_gpio_input(&pins->uart_tx);
-        pbdrv_gpio_input(&pins->uart_rx);
-        pbdrv_gpio_out_high(&pins->uart_buf);
+    return PBIO_SUCCESS;
+}
 
-        // These should be set by default already, but it seems that the
-        // bootloader on the Technic hub changes these and causes wrong
-        // detection if we don't make sure pull is disabled.
-        pbdrv_gpio_set_pull(&pins->p5, PBDRV_GPIO_PULL_NONE);
-        pbdrv_gpio_set_pull(&pins->p6, PBDRV_GPIO_PULL_NONE);
-        pbdrv_gpio_set_pull(&pins->uart_buf, PBDRV_GPIO_PULL_NONE);
-        pbdrv_gpio_set_pull(&pins->uart_tx, PBDRV_GPIO_PULL_NONE);
-        pbdrv_gpio_set_pull(&pins->uart_rx, PBDRV_GPIO_PULL_NONE);
+pbio_error_t pbdrv_ioport_p5p6_set_mode(const pbdrv_ioport_pins_t *pins, pbdrv_ioport_p5p6_mode_t mode) {
 
-        return PBIO_SUCCESS;
+    pbio_error_t err;
+
+    if (mode == PBDRV_IOPORT_P5P6_MODE_GPIO_ADC) {
+        // This is the same as the default mode.
+        return pbdrv_ioport_p5p6_pin_reset(pins);
     } else if (mode == PBDRV_IOPORT_P5P6_MODE_UART) {
-        // First reset all pins to inputs by going to GPIO mode recursively.
-        pbio_error_t err = pbdrv_ioport_p5p6_set_mode(pins, uart_dev, PBDRV_IOPORT_P5P6_MODE_GPIO_ADC);
+        err = pbdrv_ioport_p5p6_pin_reset(pins);
         if (err != PBIO_SUCCESS) {
             return err;
         }
@@ -53,12 +54,23 @@ pbio_error_t pbdrv_ioport_p5p6_set_mode(const pbdrv_ioport_pins_t *pins, pbdrv_u
         pbdrv_gpio_alt(&pins->uart_tx, pins->uart_tx_alt_uart);
         pbdrv_gpio_out_low(&pins->uart_buf);
         return PBIO_SUCCESS;
+    } else if (mode == PBDRV_IOPORT_P5P6_MODE_I2C) {
+        err = pbdrv_ioport_p5p6_pin_reset(pins);
+        if (err != PBIO_SUCCESS) {
+            return err;
+        }
+        // Required for EV3 I2C implementation.
+        pbdrv_gpio_out_low(&pins->p5);
+        pbdrv_gpio_input(&pins->p5);
+        pbdrv_gpio_out_low(&pins->p6);
+        pbdrv_gpio_input(&pins->p6);
+        return PBIO_SUCCESS;
     } else if (mode == PBDRV_IOPORT_P5P6_MODE_QUADRATURE) {
-        // In PoweredUP, this is only used for two motors in boost. Its counter
-        // driver does all the required setup. Its mode can never change. The
-        // initial driver init does not check errors for default modes since
-        // they are supported by definition. We can return an error for all
-        // other ports.
+        // Ports with this mode support only this mode and nothing else. The
+        // counter drivers are automatically started on boot. This mode is only
+        // set at port init when default ports are set, for which the return
+        // value is not checked. This mode should never change at runtime
+        // either, so always just return an error.
         return PBIO_ERROR_NOT_SUPPORTED;
     }
 

lib/pbio/include/pbdrv/i2c.h

@@ -47,8 +47,9 @@ pbio_error_t pbdrv_i2c_get_instance(uint8_t id, pbdrv_i2c_dev_t **i2c_dev);
  *                          If this is not 0 and \p rlen is not 0,
  *                          a write will be sent followed by a read in
  *                          a single transaction.
- * @param [out] rdata       Buffer for data read from the device.
- *                          Can be null if \p rlen is 0.
+ * @param [out] rdata       Pointer to data read from the device. Only valid
+ *                          immediately on successfull completion.
+ *                          Returns null if \p rlen is 0 or the operation failed.
  * @param [in]  rlen        Size of \p rdata.
  * @param [in]  nxt_quirk   Whether to use NXT I2C transaction quirk.
  * @return                  ::PBIO_SUCCESS on success.
@@ -59,7 +60,7 @@ pbio_error_t pbdrv_i2c_write_then_read(
     uint8_t dev_addr,
     const uint8_t *wdata,
     size_t wlen,
-    uint8_t *rdata,
+    uint8_t **rdata,
     size_t rlen,
     bool nxt_quirk);
 
@@ -76,7 +77,7 @@ static inline pbio_error_t pbdrv_i2c_write_then_read(
     uint8_t dev_addr,
     const uint8_t *wdata,
     size_t wlen,
-    uint8_t *rdata,
+    uint8_t **rdata,
     size_t rlen,
     bool nxt_quirk) {
     return PBIO_ERROR_NOT_SUPPORTED;

lib/pbio/include/pbdrv/ioport.h

@@ -138,13 +138,12 @@ static inline void pbdrv_ioport_enable_vcc(bool enable) {
  * pins to inputs even if GPIO mode is already active.
  *
  * @param [in]  pins          The pins to set the mode for.
- * @param [in]  uart_dev      The UART device to use for UART mode.
  * @param [in]  mode          The mode state to set.
  * @return                    ::PBIO_SUCCESS on success, otherwise
  *                            ::PBIO_ERROR_NOT_SUPPORTED if the mode is not supported.
  *                            ::PBIO_ERROR_INVALID_OP if changing the mode is not permitted in the current state.
  */
-pbio_error_t pbdrv_ioport_p5p6_set_mode(const pbdrv_ioport_pins_t *pins, pbdrv_uart_dev_t *uart_dev, pbdrv_ioport_p5p6_mode_t mode);
+pbio_error_t pbdrv_ioport_p5p6_set_mode(const pbdrv_ioport_pins_t *pins, pbdrv_ioport_p5p6_mode_t mode);
 
 extern const pbdrv_ioport_platform_data_t pbdrv_ioport_platform_data[PBDRV_CONFIG_IOPORT_NUM_DEV];
 

lib/pbio/include/pbio/port_dcm.h

@@ -71,6 +71,10 @@ static inline pbio_port_dcm_t *pbio_port_dcm_init_instance(uint8_t index) {
 }
 
 static inline pbio_error_t pbio_port_dcm_assert_type_id(pbio_port_dcm_t *dcm, lego_device_type_id_t *type_id) {
+    // Fixme: need a DCM implementation for CI tests. For now just pass LUMP.
+    if (*type_id == LEGO_DEVICE_TYPE_ID_ANY_LUMP_UART) {
+        return PBIO_SUCCESS;
+    }
     return PBIO_ERROR_NOT_SUPPORTED;
 }
 

lib/pbio/platform/ev3/platform.c

@@ -322,7 +322,7 @@ const pbdrv_ioport_platform_data_t pbdrv_ioport_platform_data[PBDRV_CONFIG_IOPOR
         #if PBDRV_CONFIG_UART_DEBUG_FIRST_PORT
         .supported_modes = PBIO_PORT_MODE_UART,
         #else // PBDRV_CONFIG_UART_DEBUG_FIRST_PORT
-        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_LEGO_DCM,
+        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_I2C | PBIO_PORT_MODE_LEGO_DCM,
         #endif
     },
     {
@@ -345,7 +345,7 @@ const pbdrv_ioport_platform_data_t pbdrv_ioport_platform_data[PBDRV_CONFIG_IOPOR
             .adc_p1 = 8,
             .adc_p6 = 7,
         },
-        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_LEGO_DCM,
+        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_I2C | PBIO_PORT_MODE_LEGO_DCM,
     },
     {
         .port_id = PBIO_PORT_ID_3,
@@ -367,7 +367,7 @@ const pbdrv_ioport_platform_data_t pbdrv_ioport_platform_data[PBDRV_CONFIG_IOPOR
             .adc_p1 = 10,
             .adc_p6 = 9,
         },
-        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_LEGO_DCM,
+        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_I2C | PBIO_PORT_MODE_LEGO_DCM,
     },
     {
         .port_id = PBIO_PORT_ID_4,
@@ -389,7 +389,7 @@ const pbdrv_ioport_platform_data_t pbdrv_ioport_platform_data[PBDRV_CONFIG_IOPOR
             .adc_p1 = 12,
             .adc_p6 = 11,
         },
-        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_LEGO_DCM,
+        .supported_modes = PBIO_PORT_MODE_UART | PBIO_PORT_MODE_I2C | PBIO_PORT_MODE_LEGO_DCM,
     },
 };