Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new ESP-IDF I2C driver #9671

Merged
merged 11 commits into from
Oct 1, 2024
Merged

Use new ESP-IDF I2C driver #9671

merged 11 commits into from
Oct 1, 2024

Conversation

dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Sep 29, 2024

Update to use new ESP-IDF I2C driver. @tannewt did most of this work in tannewt@8a4ad54 (https://github.com/tannewt/circuitpython/tree/idf5.2_i2c). I started with that, fixed a few things, and fixed some issues with the esp32-camera library, which recently had a PR to use the new driver.

I retested the problems reported in #9535 and #9561 amd they seem to be fixed.

I added an I2C.probe(address) method. There has to be a method in the class dictionary so that the native adafruit_bus_device.I2CDevice can do a probe for a specific address on anything that acts like I2C. Previously it was not actually using the common-hal probe() operation, but instead was "manually" doing a zero-length write. Now it uses whatever probe() is provided by bitbangio.I2C or busio.I2C.

In the long run the presence of .probe() means that uses of .scan() could be replaced with a tiny amount of Python code:
[address for address in range(128) if i2c.probe(address)]. But this incompatible change would come at a major version bump.

tannewt and others added 6 commits September 19, 2024 14:47
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".
- 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.
@dhalbert dhalbert requested review from tannewt and jepler September 29, 2024 20:56
@dhalbert
Copy link
Collaborator Author

A few builds overflowed. I turned off some unused pins. I also did CIRCUITPY_CODEOP = 0 on one or two boards. That could be done on more if necessary later.

lib/tinyusb Outdated Show resolved Hide resolved
ports/atmel-samd/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/broadcom/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/broadcom/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/busio/I2C.c Show resolved Hide resolved
ports/espressif/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/espressif/common-hal/espcamera/Camera.c Show resolved Hide resolved
ports/mimxrt10xx/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/mimxrt10xx/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
ports/nordic/common-hal/busio/I2C.c Outdated Show resolved Hide resolved
@jepler
Copy link
Member

jepler commented Sep 30, 2024

thanks for working on this!

@dhalbert dhalbert requested a review from jepler September 30, 2024 02:41
jepler
jepler previously approved these changes Sep 30, 2024
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think this addresses all the issues I raised. I would like @tannewt 's review still on the broadcom stuff, as it looks to me like protection against resetting the HDMI outputs might have been lost.

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I2C problems on ESP32-S2 after scan ESP32-S3 I2C timeout with seesaw
3 participants