espressif: get new I2C driver working

- Starts with @tannewt's changes.
- Make sure proper component is being compiled.
- Added `I2C.probe()` as a visible new method. This was a hidden common-hal method, but the C version of adafruit_bus_device could not use it because it needs to call probe via a Python method call. So make it visible. It's useful, and `I2C.scan()` could be phased out, since now `.scan()` can be implemented in Python with `.probe()`.
- set clock-stretching timeout on espressif to a minimum of 1 second. In all impls of busio.I2C()`, the timeout is ignored and is set to a fixed 1 second. @tannewt's new code was using the passed-in value, which was often too short.

To do:
- switch esp-camera to new-driver version. We have to use the same I2C driver everywhere.
- Check about I2CTarget.

Author: dhalbert

ports/espressif/CMakeLists.txt

@@ -5,7 +5,7 @@ cmake_minimum_required(VERSION 3.16)
 set(ENV{IDF_PATH} ${CMAKE_SOURCE_DIR}/esp-idf)
 
 # The component list here determines what options we get in menuconfig and what the ninja file can build.
-set(COMPONENTS bt driver esp_driver_dac esp_driver_gpio esp_driver_gptimer esp_driver_i2s esp_driver_ledc esp_driver_pcnt esp_driver_rmt esp_driver_spi esp_driver_tsens esp_driver_uart esp-tls esp_adc_cal esp_event esp_netif esp_psram esp_wifi esptool_py freertos log lwip main mbedtls mdns soc ulp usb wpa_supplicant esp-camera esp_lcd vfs esp_vfs_console)
+set(COMPONENTS bt driver esp_driver_dac esp_driver_gpio esp_driver_gptimer esp_driver_i2c esp_driver_i2s esp_driver_ledc esp_driver_pcnt esp_driver_rmt esp_driver_spi esp_driver_tsens esp_driver_uart esp-tls esp_adc_cal esp_event esp_netif esp_psram esp_wifi esptool_py freertos log lwip main mbedtls mdns soc ulp usb wpa_supplicant esp-camera esp_lcd vfs esp_vfs_console)
 set(EXTRA_COMPONENT_DIRS "esp-protocols/components/mdns" "esp-camera")
 
 include($ENV{IDF_PATH}/tools/cmake/project.cmake)

ports/espressif/Makefile

@@ -65,7 +65,6 @@ INC += \
 	-isystem esp-idf/components/bt/host/nimble/nimble/porting/nimble/include \
 	-isystem esp-idf/components/bt/host/nimble/nimble/porting/npl/freertos/include \
 	-isystem esp-idf/components/bt/host/nimble/port/include \
-	-isystem esp-idf/components/driver/i2c/include \
 	-isystem esp-idf/components/driver/touch_sensor/include \
 	-isystem esp-idf/components/driver/touch_sensor/$(IDF_TARGET)/include \
 	-isystem esp-idf/components/driver/twai/include \
@@ -613,7 +612,7 @@ ifeq ($(IDF_TARGET),esp32)
 BINARY_BLOBS += esp-idf/components/esp_phy/lib/$(IDF_TARGET)/librtc.a
 endif
 
-ESP_IDF_COMPONENTS_LINK = $(IDF_TARGET_ARCH) $(CHIP_COMPONENTS) app_update bootloader_support driver esp_driver_gpio esp_driver_gptimer esp_driver_ledc esp_driver_spi esp_driver_uart efuse esp_adc esp_app_format esp_common esp_event esp_hw_support esp_mm esp_partition esp_pm esp_ringbuf esp_rom esp_system esp_timer freertos hal heap log newlib nvs_flash pthread soc spi_flash vfs esp_vfs_console
+ESP_IDF_COMPONENTS_LINK = $(IDF_TARGET_ARCH) $(CHIP_COMPONENTS) app_update bootloader_support driver esp_driver_gpio esp_driver_gptimer esp_driver_i2c esp_driver_ledc esp_driver_spi esp_driver_uart efuse esp_adc esp_app_format esp_common esp_event esp_hw_support esp_mm esp_partition esp_pm esp_ringbuf esp_rom esp_system esp_timer freertos hal heap log newlib nvs_flash pthread soc spi_flash vfs esp_vfs_console
 ifneq ($(CIRCUITPY_WIFI),0)
 	ESP_IDF_COMPONENTS_LINK += esp_coex esp_netif esp-tls esp_wifi lwip mbedtls mdns wpa_supplicant esp_phy
 endif

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

@@ -93,11 +93,15 @@ void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,
     self->scl_pin = scl;
     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;
-    }
+
+    // Ignore the passed-in clock-stretching timeout. It is not used, as documented in shared-bindings.
+    // Instead use 1000 ms, which is standard across ports.
+    // self->timeout_ms = timeout_us / 1000;
+    // // Round up timeout to nearest ms.
+    // if (timeout_us % 1000 != 0) {
+    //     self->timeout_ms += 1;
+    // }
+    self->timeout_ms = 1000;
 
     claim_pin(sda);
     claim_pin(scl);
@@ -164,11 +168,11 @@ static uint8_t convert_esp_err(esp_err_t result) {
     }
 }
 
-static size_t _transaction_duration(size_t frequency, size_t len) {
+static size_t _transaction_duration_ms(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;
+    return (len + 1) / bytes_per_ms + 1000;
 }
 
 uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr, const uint8_t *data, size_t len) {
@@ -179,7 +183,7 @@ uint8_t common_hal_busio_i2c_write(busio_i2c_obj_t *self, uint16_t addr, const u
     };
     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);
+    esp_err_t result = i2c_master_transmit(dev_handle, data, len, _transaction_duration_ms(self->frequency, len) + self->timeout_ms);
     CHECK_ESP_RESULT(i2c_master_bus_rm_device(dev_handle));
     return convert_esp_err(result);
 }
@@ -192,7 +196,7 @@ uint8_t common_hal_busio_i2c_read(busio_i2c_obj_t *self, uint16_t addr, uint8_t
     };
     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);
+    esp_err_t result = i2c_master_receive(dev_handle, data, len, _transaction_duration_ms(self->frequency, len) + self->timeout_ms);
     CHECK_ESP_RESULT(i2c_master_bus_rm_device(dev_handle));
     return convert_esp_err(result);
 }
@@ -206,7 +210,7 @@ uint8_t common_hal_busio_i2c_write_read(busio_i2c_obj_t *self, uint16_t addr,
     };
     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);
+    esp_err_t result = i2c_master_transmit_receive(dev_handle, out_data, out_len, in_data, in_len, _transaction_duration_ms(self->frequency, out_len) + _transaction_duration_ms(self->frequency, in_len) + self->timeout_ms);
     CHECK_ESP_RESULT(i2c_master_bus_rm_device(dev_handle));
     return convert_esp_err(result);
 }

ports/espressif/mpconfigport.mk

@@ -37,7 +37,8 @@ CIRCUITPY_BLEIO ?= 1
 CIRCUITPY_BLEIO_HCI = 0
 CIRCUITPY_CANIO ?= 1
 CIRCUITPY_COUNTIO ?= 1
-CIRCUITPY_ESPCAMERA ?= 1
+############ TEMPORARY while working on new I2C driver
+CIRCUITPY_ESPCAMERA ?= 0
 CIRCUITPY_ESPIDF ?= 1
 CIRCUITPY_ESPULP ?= 1
 CIRCUITPY_FRAMEBUFFERIO ?= 1

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

@@ -30,8 +30,8 @@
 #include "shared-bindings/microcontroller/__init__.h"
 #include "shared-bindings/microcontroller/Pin.h"
 
-STATIC I2CSPM_Init_TypeDef i2cspm_init;
-STATIC bool in_used = false;
+static I2CSPM_Init_TypeDef i2cspm_init;
+static bool in_used = false;
 
 // Construct I2C protocol, this function init i2c peripheral
 void common_hal_busio_i2c_construct(busio_i2c_obj_t *self,

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

@@ -40,7 +40,7 @@
 // Arrays use 0 based numbering: I2C1 is stored at index 0
 #define MAX_I2C 4
 
-STATIC bool reserved_i2c[MAX_I2C];
+static bool reserved_i2c[MAX_I2C];
 
 #define ALL_CLOCKS 0xFF
 static void i2c_clock_enable(uint8_t mask);

shared-bindings/bitbangio/I2C.c

@@ -59,8 +59,7 @@ static mp_obj_t bitbangio_i2c_make_new(const mp_obj_type_t *type, size_t n_args,
     const mcu_pin_obj_t *scl = validate_obj_is_free_pin(args[ARG_scl].u_obj, MP_QSTR_scl);
     const mcu_pin_obj_t *sda = validate_obj_is_free_pin(args[ARG_sda].u_obj, MP_QSTR_sda);
 
-    bitbangio_i2c_obj_t *self = m_new_obj_with_finaliser(bitbangio_i2c_obj_t);
-    self->base.type = &bitbangio_i2c_type;
+    bitbangio_i2c_obj_t *self = mp_obj_malloc_with_finaliser(bitbangio_i2c_obj_t, &bitbangio_i2c_type);
     shared_module_bitbangio_i2c_construct(self, scl, sda, args[ARG_frequency].u_int, args[ARG_timeout].u_int);
     return (mp_obj_t)self;
 }
@@ -103,6 +102,23 @@ static void check_lock(bitbangio_i2c_obj_t *self) {
     }
 }
 
+//|     def probe(self, address: int) -> List[int]:
+//|         """Check if a device at the specified address responds.
+//|
+//|         :param int address: 7-bit device address
+//|         :return: ``True`` if a device at ``address`` responds; ``False`` otherwise
+//|         :rtype: bool"""
+//|         ...
+static mp_obj_t bitbangio_i2c_probe(mp_obj_t self_in, mp_obj_t address_obj) {
+    bitbangio_i2c_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
+    check_lock(self);
+
+    const uint16_t addr = mp_obj_get_int(address_obj);
+    return mp_obj_new_bool(shared_module_bitbangio_i2c_probe(self, addr));
+}
+MP_DEFINE_CONST_FUN_OBJ_2(bitbangio_i2c_probe_obj, bitbangio_i2c_probe);
+
 //|     def scan(self) -> List[int]:
 //|         """Scan all I2C addresses between 0x08 and 0x77 inclusive and return a list of
 //|         those that respond.  A device responds if it pulls the SDA line low after
@@ -331,6 +347,7 @@ static const mp_rom_map_elem_t bitbangio_i2c_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR_deinit), MP_ROM_PTR(&bitbangio_i2c_deinit_obj) },
     { MP_ROM_QSTR(MP_QSTR___enter__), MP_ROM_PTR(&default___enter___obj) },
     { MP_ROM_QSTR(MP_QSTR___exit__), MP_ROM_PTR(&bitbangio_i2c_obj___exit___obj) },
+    { MP_ROM_QSTR(MP_QSTR_probe), MP_ROM_PTR(&bitbangio_i2c_probe_obj) },
     { MP_ROM_QSTR(MP_QSTR_scan), MP_ROM_PTR(&bitbangio_i2c_scan_obj) },
 
     { MP_ROM_QSTR(MP_QSTR_try_lock), MP_ROM_PTR(&bitbangio_i2c_try_lock_obj) },

shared-bindings/busio/I2C.c

@@ -48,7 +48,6 @@
 //|         ...
 static mp_obj_t busio_i2c_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *all_args) {
     #if CIRCUITPY_BUSIO_I2C
-    busio_i2c_obj_t *self = mp_obj_malloc(busio_i2c_obj_t, &busio_i2c_type);
     enum { ARG_scl, ARG_sda, ARG_frequency, ARG_timeout };
     static const mp_arg_t allowed_args[] = {
         { MP_QSTR_scl, MP_ARG_REQUIRED | MP_ARG_OBJ },
@@ -62,8 +61,7 @@ static mp_obj_t busio_i2c_make_new(const mp_obj_type_t *type, size_t n_args, siz
     const mcu_pin_obj_t *scl = validate_obj_is_free_pin(args[ARG_scl].u_obj, MP_QSTR_scl);
     const mcu_pin_obj_t *sda = validate_obj_is_free_pin(args[ARG_sda].u_obj, MP_QSTR_sda);
 
-    busio_i2c_obj_t *self = m_new_obj_with_finaliser(busio_i2c_obj_t);
-    self->base.type = &busio_i2c_type;
+    busio_i2c_obj_t *self = mp_obj_malloc_with_finaliser(busio_i2c_obj_t, &busio_i2c_type);
     common_hal_busio_i2c_construct(self, scl, sda, args[ARG_frequency].u_int, args[ARG_timeout].u_int);
     return (mp_obj_t)self;
     #else
@@ -98,12 +96,12 @@ static void check_for_deinit(busio_i2c_obj_t *self) {
 //|         """Automatically deinitializes the hardware on context exit. See
 //|         :ref:`lifetime-and-contextmanagers` for more info."""
 //|         ...
-STATIC mp_obj_t busio_i2c_obj___exit__(size_t n_args, const mp_obj_t *args) {
+static mp_obj_t busio_i2c_obj___exit__(size_t n_args, const mp_obj_t *args) {
     (void)n_args;
     common_hal_busio_i2c_deinit(MP_OBJ_TO_PTR(args[0]));
     return mp_const_none;
 }
-STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(busio_i2c___exit___obj, 4, 4, busio_i2c_obj___exit__);
+static MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(busio_i2c___exit___obj, 4, 4, busio_i2c_obj___exit__);
 
 static void check_lock(busio_i2c_obj_t *self) {
     asm ("");
@@ -112,6 +110,23 @@ static void check_lock(busio_i2c_obj_t *self) {
     }
 }
 
+//|     def probe(self, address: int) -> List[int]:
+//|         """Check if a device at the specified address responds.
+//|
+//|         :param int address: 7-bit device address
+//|         :return: ``True`` if a device at ``address`` responds; ``False`` otherwise
+//|         :rtype: bool"""
+//|         ...
+static mp_obj_t busio_i2c_probe(mp_obj_t self_in, mp_obj_t address_obj) {
+    busio_i2c_obj_t *self = MP_OBJ_TO_PTR(self_in);
+    check_for_deinit(self);
+    check_lock(self);
+
+    const uint16_t addr = mp_obj_get_int(address_obj);
+    return mp_obj_new_bool(common_hal_busio_i2c_probe(self, addr));
+}
+MP_DEFINE_CONST_FUN_OBJ_2(busio_i2c_probe_obj, busio_i2c_probe);
+
 //|     def scan(self) -> List[int]:
 //|         """Scan all I2C addresses between 0x08 and 0x77 inclusive and return a
 //|         list of those that respond.
@@ -366,6 +381,7 @@ static const mp_rom_map_elem_t busio_i2c_locals_dict_table[] = {
     { MP_ROM_QSTR(MP_QSTR_deinit), MP_ROM_PTR(&busio_i2c_deinit_obj) },
     { MP_ROM_QSTR(MP_QSTR___enter__), MP_ROM_PTR(&default___enter___obj) },
     { MP_ROM_QSTR(MP_QSTR___exit__), MP_ROM_PTR(&busio_i2c___exit___obj) },
+    { MP_ROM_QSTR(MP_QSTR_probe), MP_ROM_PTR(&busio_i2c_probe_obj) },
     { MP_ROM_QSTR(MP_QSTR_scan), MP_ROM_PTR(&busio_i2c_scan_obj) },
 
     { MP_ROM_QSTR(MP_QSTR_try_lock), MP_ROM_PTR(&busio_i2c_try_lock_obj) },

shared-module/adafruit_bus_device/i2c_device/I2CDevice.c

@@ -41,30 +41,14 @@ void common_hal_adafruit_bus_device_i2cdevice_unlock(adafruit_bus_device_i2cdevi
 void common_hal_adafruit_bus_device_i2cdevice_probe_for_device(adafruit_bus_device_i2cdevice_obj_t *self) {
     common_hal_adafruit_bus_device_i2cdevice_lock(self);
 
-    mp_buffer_info_t write_bufinfo;
-    mp_obj_t write_buffer = mp_obj_new_bytearray_of_zeros(0);
-    mp_get_buffer_raise(write_buffer, &write_bufinfo, MP_BUFFER_READ);
+    mp_obj_t dest[3];
+    mp_load_method(self->i2c, MP_QSTR_probe, dest);
+    dest[2] = MP_OBJ_NEW_SMALL_INT(self->device_address);
+    const bool found = mp_obj_is_true(mp_call_method_n_kw(1, 0, dest));
 
-    mp_obj_t dest[4];
-
-    /* catch exceptions that may be thrown while probing for the device */
-    nlr_buf_t nlr;
-    if (nlr_push(&nlr) == 0) {
-        mp_load_method(self->i2c, MP_QSTR_writeto, dest);
-        dest[2] = MP_OBJ_NEW_SMALL_INT(self->device_address);
-        dest[3] = write_buffer;
-        mp_call_method_n_kw(2, 0, dest);
-        nlr_pop();
-    } else {
-        common_hal_adafruit_bus_device_i2cdevice_unlock(self);
+    common_hal_adafruit_bus_device_i2cdevice_unlock(self);
 
-        if (mp_obj_is_subclass_fast(MP_OBJ_FROM_PTR(((mp_obj_base_t *)nlr.ret_val)->type), MP_OBJ_FROM_PTR(&mp_type_OSError))) {
-            mp_raise_ValueError_varg(MP_ERROR_TEXT("No I2C device at address: 0x%x"), self->device_address);
-        } else {
-            /* In case we receive an unrelated exception pass it up */
-            nlr_raise(MP_OBJ_FROM_PTR(nlr.ret_val));
-        }
+    if (!found) {
+        mp_raise_ValueError_varg(MP_ERROR_TEXT("No I2C device at address: 0x%x"), self->device_address);
     }
-
-    common_hal_adafruit_bus_device_i2cdevice_unlock(self);
 }