incorrect use of i2c api leads to crash with idf4.4

[anecdata]

CircuitPython version

Adafruit CircuitPython 7.2.0-alpha.0-43-g45caaa8e2 on 2021-12-07; Adafruit Feather ESP32S2

Code/REPL

Adafruit CircuitPython 7.2.0-alpha.0-43-g45caaa8e2 on 2021-12-07; Adafruit Feather ESP32S2 with ESP32S2
>>> import board
>>> i2c = board.I2C()
>>> while not i2c.try_lock():
...     pass
>>> dev_addrs = i2c.scan()
>>> i2c.unlock()
>>> for dev_addr in dev_addrs:
...     print(f'{dev_addr:02X}', end=' ')
...     from adafruit_lc709203f import LC709203F
...     bat_dev = LC709203F(i2c)
...     print(f'LC709203F {bat_dev.ic_version:04X} {bat_dev.cell_voltage:0.1f}v {bat_dev.cell_percent:0.0f}%')
0B 
[tio 14:47:47] Disconnected
[tio 14:47:49] Connected
Running in safe mode! Not running saved code.

Behavior

Safe mode

Description

Initially noted by @Jerryn on Discord, also seen by @KeithTheEE in the same discussion.

Additional information

No response

[jerryneedell]

I also caused a crash with another I2C sensor - si7021

[jerryneedell]

hmmm -- this crashes

import board
import digitalio
import adafruit_si7021

ip = digitalio.DigitalInOut(board.I2C_POWER_INVERTED)
ip.switch_to_output()
ip.value = False

si = adafruit_si7021.SI7021(board.I2C())

print(si.temperature)

but this runs

import board
import digitalio
from adafruit_bme280 import basic as adafruit_bme280

ip = digitalio.DigitalInOut(board.I2C_POWER_INVERTED)
ip.switch_to_output()
ip.value = False

bme280 = adafruit_bme280.Adafruit_BME280_I2C(board.I2C())
bme280.sea_level_pressure = 1013.25

print(bme280.temperature)

[anecdata]

@jerryneedell did you intend to close this?

[jerryneedell]

No.. sorry

[jerryneedell]

Closed by mistake

[jepler]

Crashes a Feather ESP32-S2 TFT with an adalogger featherwing:

import board
i = board.I2C()
i.try_lock()
b = bytearray(1)
i.writeto_then_readfrom(0x68, b, b)

It's fine in 7.1.0, broken in 7.2.0-alpha.1.

[atctwo]

Hi! I've been having a similar problem, but on an ESP32-S3 (ESP32-S3-DevKitC-1 w/ 2MB PSRAM), running on tinyuf2 (although i had the same problem without tinyuf2).

I first had the problem trying to use a DS1337 (with the DS1307 library), and a PCF8523 (CircuitPython hard crashed when calling the libraries' constructors). I looked at the libraries in the issue history, and the SI7021, DS1307, PCF8523 libraries call write_then_readinto() in their constructors (which calls writeto_then_readfrom()), but the BME280.basic library doesn't. I tried some other I2C sensors that call writeto_then_readfrom() via Adafruit_Register, and they crashed too.

I rebuilt CircuitPython with DEBUG=1 and tried to initalise the PCF8523, which crashed the ESP32. I was able to listen to the trace log with idf.py monitor, which pointed me to ESP-IDF's i2c.c, line 1344, which is a part of i2c_master_cmd_begin_static(). I don't know anything about how ESP-IDF's i2c implementation works internally, so I had (and still have) no idea what's wrong.

I googled that function name, and found someone on the Micropython forums that had a very similar problem. They fixed it with a patch to ESP-IDF v4.4. I applied it to my local circuitpython copy, and rebuilt, and i2c seems to work fine.

I got successful tests with a TLV493D, SHTC3, and DS1337 (using the DS1307 library), but I couldn't get the PCF8523 to work (I think that might be for an unrelated reason?)

I checked the ESP-IDF versions, and CircuitPython v7.1.0 uses ESP-IDF v4.3, while v7.2.0-alpha1 uses v4.4. The ESP-IDF version was bumped to v4.4 on 2021-12-30 (4daa7b5), but the build of circuitpython in the original issue post was built on 2021-12-07, and I experienced the bug on a version built on 2021-12-29, so it would have still been using v4.3? I'm not totally convinced this is the cause, but I think it was worth mentioning.

[jepler]

Investigating this further I found espressif/esp-idf#7405

Per the comments there, our sequence of API calls in common_hal_busio_write is not acceptable in esp-idf, because we omit the stop call for writeto_then_readfrom according to the transmit_stop_bit flag:

    if (transmit_stop_bit) {
        i2c_master_stop(cmd);
    }                                                                           
    esp_err_t result = i2c_master_cmd_begin(self->i2c_num, cmd, 100 /* wait in ticks */);                                                                       

[atctwo]

I rebuilt CircuitPython with an unpatched ESP-IDF (release-v4.4), and changed busio's writeto_then_readfrom() to send stop bits:

// line 328
writeto(self, args[ARG_address].u_int, args[ARG_out_buffer].u_obj, args[ARG_out_start].u_int,
        args[ARG_out_end].u_int, true);    // <- set to true to send stop bits
readfrom(self, args[ARG_address].u_int, args[ARG_in_buffer].u_obj, args[ARG_in_start].u_int,
        args[ARG_in_end].u_int);

original code here

I tested this with the I2C devices I used in my last post, and they all seem to work fine. Although I don't own a lot of I2C devices, so I don't know if enabling stop bits would break anything else in the CircuitPython ecosystem. Another thing that I've been thinking about is that there must have been some reason to set the stop bit flag to false in the first place.

[tannewt]

I don't think there is supposed to be a stop bit before the repeated start. I suspect we'll need to have a common-hal API for writeto_then_readfrom so we can do one call to the IDF for it. What is the right call to the IDF for this?

[atctwo]

I don't know the IDF's I2C API that well, but I couldn't find a writeto_then_readfrom equivalent that queues commands to an I2C command list like i2c_master_write does. However, since v4.4, there are wrapper methods for write, read, and writeto then readfrom that encapsulate the I2C command list stuff. Source code for the writeto readfrom function

It made me realise that CircuitPython's writeto_then_readfrom breaks the process into two parts / calls (using writeto() and readfrom(), hence it uses two I2C command lists. The write part's command list gets sent without any stop bits, and it crashes. ESP-IDF's repeated-start function does everything all in one list.

[tannewt]

@atctwo Great find! We'll need to "push down" the writeto() and readfrom() calls into a common_hal function in CP. That way the ESP version can call i2c_master_write_read_device directly in the IDF instead of doing two calls.

[dhalbert]

I am debugging #5934. I don't know if this is related at all or not.

[dhalbert]

I am implementing what @tannewt suggested, pushing write/read down to common_hal. This simplifies a bunch of things, but will take a bit of time to do for all the ports.

@atctwo Those wrapper routines are great -- thank you very much for pointing them out!