Switch to IDF 5.2 I2C API

Make busio.I2C use finalizers for reset instead of bulk reset. This
makes it easier to track and free the memory that the IDF allocates
internally for its "handle".

Author: tannewt

ports/atmel-samd/common-hal/busio/I2C.c

@@ -108,21 +108,29 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
         mp_arg_error_invalid(MP_QSTR_frequency);
     }
 
-    self->sda_pin = sda->number;
-    self->scl_pin = scl->number;
-    claim_pin(sda);
-    claim_pin(scl);
 
     if (i2c_m_sync_enable(&self->i2c_desc) != ERR_NONE) {
         common_hal_busio_i2c_deinit(self);
         mp_raise_OSError(MP_EIO);
     }
+
+    self->sda_pin = sda->number;
+    self->scl_pin = scl->number;
+    claim_pin(sda);
+    claim_pin(scl);
+
+    // Prevent bulk sercom reset from resetting us. The finalizer will instead.
+    never_reset_sercom(self->i2c_desc.device.hw);
 }
 
 bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
     return self->sda_pin == NO_PIN;
 }
 
+void common_hal_busio_i2c_mark_deinit(busio_i2c_obj_t *self) {
+    self->sda_pin = NO_PIN;
+}
+
 void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     if (common_hal_busio_i2c_deinited(self)) {
         return;
@@ -135,6 +143,7 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     reset_pin_number(self->scl_pin);
     self->sda_pin = NO_PIN;
     self->scl_pin = NO_PIN;
+    common_hal_busio_i2c_mark_deinit(self);
 }
 
 bool common_hal_busio_i2c_probe(busio_i2c_obj_t *self, uint8_t addr) {
@@ -236,8 +245,6 @@ uint8_t common_hal_busio_i2c_write_read(busio_i2c_obj_t *self, uint16_t addr,
 }
 
 void common_hal_busio_i2c_never_reset(busio_i2c_obj_t *self) {
-    never_reset_sercom(self->i2c_desc.device.hw);
-
     never_reset_pin_number(self->scl_pin);
     never_reset_pin_number(self->sda_pin);
 }

ports/broadcom/common-hal/busio/I2C.c

@@ -23,28 +23,8 @@ static BSC0_Type *i2c[NUM_I2C] = {BSC0, BSC1, NULL, BSC3, BSC4, BSC5, BSC6, NULL
 static BSC0_Type *i2c[NUM_I2C] = {BSC0, BSC1, NULL};
 #endif
 
-static bool never_reset_i2c[NUM_I2C];
 static bool i2c_in_use[NUM_I2C];
 
-void reset_i2c(void) {
-    // BSC2 is dedicated to the first HDMI output.
-    never_reset_i2c[2] = true;
-    i2c_in_use[2] = true;
-    #if BCM_VERSION == 2711
-    // BSC7 is dedicated to the second HDMI output.
-    never_reset_i2c[7] = true;
-    i2c_in_use[7] = true;
-    #endif
-    for (size_t i = 0; i < NUM_I2C; i++) {
-        if (never_reset_i2c[i]) {
-            continue;
-        }
-        i2c_in_use[i] = false;
-        i2c[i]->C_b.I2CEN = false;
-        COMPLETE_MEMORY_READS;
-    }
-}
-
 void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     const mcu_pin_obj_t *scl, const mcu_pin_obj_t *sda, uint32_t frequency, uint32_t timeout) {
     size_t instance_index = NUM_I2C;
@@ -90,17 +70,21 @@ bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
     return self->sda_pin == NULL;
 }
 
+void common_hal_busio_i2c_mark_deinit(busio_i2c_obj_t *self) {
+    self->sda_pin = NULL;
+}
+
 void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     if (common_hal_busio_i2c_deinited(self)) {
         return;
     }
-    never_reset_i2c[self->index] = false;
     i2c_in_use[self->index] = false;
 
     common_hal_reset_pin(self->sda_pin);
     common_hal_reset_pin(self->scl_pin);
     self->sda_pin = NULL;
     self->scl_pin = NULL;
+    common_hal_busio_i2c_mark_deinit(self);
 }
 
 bool common_hal_busio_i2c_probe(busio_i2c_obj_t *self, uint8_t addr) {
@@ -246,8 +230,6 @@ uint8_t common_hal_busio_i2c_write_read(busio_i2c_obj_t *self, uint16_t addr,
 }
 
 void common_hal_busio_i2c_never_reset(busio_i2c_obj_t *self) {
-    never_reset_i2c[self->index] = true;
-
     common_hal_never_reset_pin(self->scl_pin);
     common_hal_never_reset_pin(self->sda_pin);
 }

ports/broadcom/common-hal/busio/I2C.h

@@ -23,5 +23,3 @@ typedef struct {
     bool finish_write;
     uint8_t last_write_data;
 } busio_i2c_obj_t;
-
-void reset_i2c(void);

ports/broadcom/supervisor/port.c

@@ -13,7 +13,6 @@
 #include "genhdr/mpversion.h"
 
 #include "common-hal/rtc/RTC.h"
-#include "common-hal/busio/I2C.h"
 #include "common-hal/busio/SPI.h"
 #include "common-hal/busio/UART.h"
 
@@ -66,7 +65,6 @@ safe_mode_t port_init(void) {
 
 void reset_port(void) {
     #if CIRCUITPY_BUSIO
-    reset_i2c();
     reset_spi();
     reset_uart();
     #endif

ports/cxd56/common-hal/busio/I2C.c

@@ -44,12 +44,18 @@ void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
 
     reset_pin_number(self->scl_pin->number);
     reset_pin_number(self->sda_pin->number);
+
+    common_hal_busio_i2c_mark_deinit(self);
 }
 
 bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
     return self->i2c_dev == NULL;
 }
 
+void common_hal_busio_i2c_mark_deinit(busio_i2c_obj_t *self) {
+    self->i2c_dev = NULL;
+}
+
 bool common_hal_busio_i2c_try_lock(busio_i2c_obj_t *self) {
     if (common_hal_busio_i2c_deinited(self)) {
         return false;

ports/espressif/Makefile

@@ -392,7 +392,6 @@ SRC_C += \
 	boards/$(BOARD)/board.c \
 	boards/$(BOARD)/pins.c \
 	shared/netutils/netutils.c \
-	peripherals/i2c.c \
 	peripherals/$(IDF_TARGET)/pins.c
 
 ifeq ($(CIRCUITPY_SSL),1)

ports/espressif/common-hal/busio/I2C.c

@@ -10,11 +10,12 @@
 
 #include "components/driver/i2c/include/driver/i2c.h"
 
+#include "bindings/espidf/__init__.h"
 #include "shared-bindings/microcontroller/__init__.h"
 #include "shared-bindings/microcontroller/Pin.h"
 
 void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
-    const mcu_pin_obj_t *scl, const mcu_pin_obj_t *sda, uint32_t frequency, uint32_t timeout) {
+    const mcu_pin_obj_t *scl, const mcu_pin_obj_t *sda, uint32_t frequency, uint32_t timeout_us) {
     // Pins 45 and 46 are "strapping" pins that impact start up behavior. They usually need to
     // be pulled-down so pulling them up for I2C is a bad idea. To make this hard, we don't
     // support I2C on these pins.
@@ -60,44 +61,42 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     }
     #endif
 
+
+    i2c_master_bus_config_t config = {
+        .i2c_port = -1, // auto
+        .sda_io_num = sda->number,
+        .scl_io_num = scl->number,
+        .clk_source = I2C_CLK_SRC_DEFAULT,
+        .glitch_ignore_cnt = 7,
+        .flags = {
+            #if CIRCUITPY_I2C_ALLOW_INTERNAL_PULL_UP
+            .enable_internal_pullup = true, /*!< Internal GPIO pull mode for I2C sda signal*/
+            #else
+            .enable_internal_pullup = false, /*!< Internal GPIO pull mode for I2C sda signal*/
+            #endif
+        }
+    };
+    esp_err_t result = i2c_new_master_bus(&config, &self->handle);
+
+    if (result == ESP_ERR_NOT_FOUND) {
+        mp_raise_ValueError(MP_ERROR_TEXT("All I2C peripherals are in use"));
+    }
+    CHECK_ESP_RESULT(result);
+
     self->xSemaphore = xSemaphoreCreateMutex();
     if (self->xSemaphore == NULL) {
+        i2c_del_master_bus(self->handle);
+        self->handle = NULL;
         mp_raise_RuntimeError(MP_ERROR_TEXT("Unable to create lock"));
     }
     self->sda_pin = sda;
     self->scl_pin = scl;
-    self->i2c_num = peripherals_i2c_get_free_num();
-    self->has_lock = 0;
-
-    if (self->i2c_num == I2C_NUM_MAX) {
-        mp_raise_ValueError(MP_ERROR_TEXT("All I2C peripherals are in use"));
-    }
-
-    const i2c_config_t i2c_conf = {
-        .mode = I2C_MODE_MASTER,
-        .sda_io_num = self->sda_pin->number,
-        .scl_io_num = self->scl_pin->number,
-        #if CIRCUITPY_I2C_ALLOW_INTERNAL_PULL_UP
-        .sda_pullup_en = GPIO_PULLUP_ENABLE,  /*!< Internal GPIO pull mode for I2C sda signal*/
-        .scl_pullup_en = GPIO_PULLUP_ENABLE,  /*!< Internal GPIO pull mode for I2C scl signal*/
-        #else
-        .sda_pullup_en = GPIO_PULLUP_DISABLE,  /*!< Internal GPIO pull mode for I2C sda signal*/
-        .scl_pullup_en = GPIO_PULLUP_DISABLE,  /*!< Internal GPIO pull mode for I2C scl signal*/
-        #endif
-
-        .master = {
-            .clk_speed = frequency,
-        }
-    };
-
-    // Initialize I2C.
-    esp_err_t err = peripherals_i2c_init(self->i2c_num, &i2c_conf);
-    if (err != ESP_OK) {
-        if (err == ESP_FAIL) {
-            mp_raise_OSError(MP_EIO);
-        } else {
-            mp_raise_RuntimeError(MP_ERROR_TEXT("init I2C"));
-        }
+    self->has_lock = false;
+    self->frequency = frequency;
+    self->timeout_ms = timeout_us / 1000;
+    // Round up timeout to nearest ms.
+    if (timeout_us % 1000 != 0) {
+        self->timeout_ms += 1;
     }
 
     claim_pin(sda);
@@ -108,32 +107,27 @@ bool common_hal_busio_i2c_deinited(busio_i2c_obj_t *self) {
     return self->sda_pin == NULL;
 }
 
+void common_hal_busio_i2c_mark_deinit(busio_i2c_obj_t *self) {
+    self->sda_pin = NULL;
+}
+
 void common_hal_busio_i2c_deinit(busio_i2c_obj_t *self) {
     if (common_hal_busio_i2c_deinited(self)) {
         return;
     }
 
-    peripherals_i2c_deinit(self->i2c_num);
+    i2c_del_master_bus(self->handle);
+    self->handle = NULL;
 
     common_hal_reset_pin(self->sda_pin);
     common_hal_reset_pin(self->scl_pin);
     self->sda_pin = NULL;
     self->scl_pin = NULL;
-}
-
-static esp_err_t i2c_zero_length_write(busio_i2c_obj_t *self, uint8_t addr, TickType_t timeout) {
-    // i2c_master_write_to_device() won't do zero-length writes, so we do it by hand.
-    i2c_cmd_handle_t cmd = i2c_cmd_link_create();
-    i2c_master_start(cmd);
-    i2c_master_write_byte(cmd, addr << 1, true);
-    i2c_master_stop(cmd);
-    esp_err_t result = i2c_master_cmd_begin(self->i2c_num, cmd, timeout);
-    i2c_cmd_link_delete(cmd);
-    return result;
+    common_hal_busio_i2c_mark_deinit(self);
 }
 
 bool common_hal_busio_i2c_probe(busio_i2c_obj_t *self, uint8_t addr) {
-    esp_err_t result = i2c_zero_length_write(self, addr, pdMS_TO_TICKS(10));
+    esp_err_t result = i2c_master_probe(self->handle, addr, 1);
     return result == ESP_OK;
 }
 
@@ -170,28 +164,54 @@ static uint8_t convert_esp_err(esp_err_t result) {
     }
 }
 
+static size_t _transaction_duration(size_t frequency, size_t len) {
+    size_t khz = frequency / 1000;
+    size_t bytes_per_ms = khz / 8;
+    // + 1 for the address byte
+    return (len + 1) / bytes_per_ms;
+}
+
 uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr, const uint8_t *data, size_t len) {
-    return convert_esp_err(len == 0
-        ? i2c_zero_length_write(self, addr, pdMS_TO_TICKS(1000))
-        : i2c_master_write_to_device(self->i2c_num, (uint8_t)addr, data, len, pdMS_TO_TICKS(1000))
-        );
+    i2c_device_config_t dev_config = {
+        .dev_addr_length = I2C_ADDR_BIT_LEN_7,
+        .device_address = addr,
+        .scl_speed_hz = self->frequency
+    };
+    i2c_master_dev_handle_t dev_handle;
+    CHECK_ESP_RESULT(i2c_master_bus_add_device(self->handle, &dev_config, &dev_handle));
+    esp_err_t result = i2c_master_transmit(dev_handle, data, len, _transaction_duration(self->frequency, len) + self->timeout_ms);
+    CHECK_ESP_RESULT(i2c_master_bus_rm_device(dev_handle));
+    return convert_esp_err(result);
 }
 
 uint8_t common_hal_busio_i2c_read(busio_i2c_obj_t *self, uint16_t addr, uint8_t *data, size_t len) {
-    return convert_esp_err(
-        i2c_master_read_from_device(self->i2c_num, (uint8_t)addr, data, len, pdMS_TO_TICKS(1000)));
+    i2c_device_config_t dev_config = {
+        .dev_addr_length = I2C_ADDR_BIT_LEN_7,
+        .device_address = addr,
+        .scl_speed_hz = self->frequency
+    };
+    i2c_master_dev_handle_t dev_handle;
+    CHECK_ESP_RESULT(i2c_master_bus_add_device(self->handle, &dev_config, &dev_handle));
+    esp_err_t result = i2c_master_receive(dev_handle, data, len, _transaction_duration(self->frequency, len) + self->timeout_ms);
+    CHECK_ESP_RESULT(i2c_master_bus_rm_device(dev_handle));
+    return convert_esp_err(result);
 }
 
 uint8_t common_hal_busio_i2c_write_read(busio_i2c_obj_t *self, uint16_t addr,
     uint8_t *out_data, size_t out_len, uint8_t *in_data, size_t in_len) {
-    return convert_esp_err(
-        i2c_master_write_read_device(self->i2c_num, (uint8_t)addr,
-            out_data, out_len, in_data, in_len, pdMS_TO_TICKS(1000)));
+    i2c_device_config_t dev_config = {
+        .dev_addr_length = I2C_ADDR_BIT_LEN_7,
+        .device_address = addr,
+        .scl_speed_hz = self->frequency
+    };
+    i2c_master_dev_handle_t dev_handle;
+    CHECK_ESP_RESULT(i2c_master_bus_add_device(self->handle, &dev_config, &dev_handle));
+    esp_err_t result = i2c_master_transmit_receive(dev_handle, out_data, out_len, in_data, in_len, _transaction_duration(self->frequency, out_len) + _transaction_duration(self->frequency, in_len) + self->timeout_ms);
+    CHECK_ESP_RESULT(i2c_master_bus_rm_device(dev_handle));
+    return convert_esp_err(result);
 }
 
 void common_hal_busio_i2c_never_reset(busio_i2c_obj_t *self) {
-    never_reset_i2c(self->i2c_num);
-
     common_hal_never_reset_pin(self->scl_pin);
     common_hal_never_reset_pin(self->sda_pin);
 }

ports/espressif/common-hal/busio/I2C.h

@@ -13,13 +13,15 @@
 #include "freertos/semphr.h"
 #include "py/obj.h"
 
-#include "peripherals/i2c.h"
+#include "driver/i2c_master.h"
 
 typedef struct {
     mp_obj_base_t base;
     const mcu_pin_obj_t *scl_pin;
     const mcu_pin_obj_t *sda_pin;
-    i2c_port_t i2c_num;
+    size_t timeout_ms;
+    size_t frequency;
+    i2c_master_bus_handle_t handle;
     SemaphoreHandle_t xSemaphore;
     bool has_lock;
 } busio_i2c_obj_t;