Add I2C support for esp32

tinygo-org / tinygo

Go compiler for small places. Microcontrollers, WebAssembly (WASM/WASI), and command-line tools. Based on LLVM.

https://tinygo.org

Other

14.73k stars 859 forks source link

Add I2C support for esp32 #4259

Closed jfreymuth closed 1 month ago

jfreymuth commented 1 month ago

Mostly copied from machine_esp32c3_i2c.go, but there are some differences, especially with timeout handling (see line 165).

Tested on an M5Paper, where I could successfully communicate with multiple devices and recover from errors in a long-running program.

deadprogram commented 1 month ago

This is really great @jfreymuth thank you very much for working on it. Please see my comments inline.

deadprogram commented 1 month ago

Oh yeah, one thing I almost forgot is we need to also add a smoke test for i2c on esp32.

aykevl commented 1 month ago

Mostly copied from machine_esp32c3_i2c.go, but there are some differences, especially with timeout handling (see line 165).

That's going to increase the maintenance burden. Can you refactor the code so that most/all common code is shared and only the chip-specific things are in the appropriate chip specific files? For example:

machine_esp32xx_i2c.go for the esp32 and esp32c3 with most of the I2C code
machine_esp32c3_i2c.go contains the esp32c3 specific bits (for example, constants or functions called from machine_esp32xx_i2c.go)
same for machine_esp32_i2c.go

This is also important because I expect other chips in the ESP32 family to be very similar, they can share code in a similar way.

aykevl commented 1 month ago

We have something similar for the nrf chips:

https://github.com/tinygo-org/tinygo/blob/release/src/machine/machine_nrf528xx.go contains most of the I2C/UART/PWM etc implementation
https://github.com/tinygo-org/tinygo/blob/release/src/machine/machine_nrf52840.go contains a setPins method because it varies by chip

jfreymuth commented 1 month ago

I can try, but I don't have any esp32c3 devices, so I'm not sure how to verify I'm not breaking anything with the refactor

deadprogram commented 1 month ago

Perhaps we can merge this PR (with a smoketest added, please) and then refactor the i2c implementation as described by @aykevl in a separate PR. I have both boards and am willing to help out with this.

aykevl commented 1 month ago

Sounds good to me! (I'm just hoping we won't forget this...)

I'm not sure about the smoketest though: what would be gained from that? We already have a smoketest for the esp32, so any syntax or type errors would already be caught.

deadprogram commented 1 month ago

I'm not sure about the smoketest though: what would be gained from that? We already have a smoketest for the esp32, so any syntax or type errors would already be caught.

Just to ensure that the i2c parts compile for the esp32 build tag.

aykevl commented 1 month ago

In that case I'd suggest adding the ESP32 here: https://github.com/tinygo-org/tinygo/blob/6384ecace093df2d0b93915886954abfc4ecfe01/src/machine/i2c.go#L1

We already have plenty of smoke tests that show the machine package (including I2C) compiles for the ESP32: https://github.com/tinygo-org/tinygo/blob/6384ecace093df2d0b93915886954abfc4ecfe01/GNUmakefile#L751-L764

deadprogram commented 1 month ago

In that case I'd suggest adding the ESP32 here:

https://github.com/tinygo-org/tinygo/pull/4259/files#diff-0fff984543b29e2e510fb57be78ac8464fc10ba2d494ba94fa7b686e275b3ee0R1

That is in fact in this very PR already! :) In which case I think we can merge now.

deadprogram commented 1 month ago

Now going to squash/merge. Thank you @jfreymuth for working on this and to @aykevl for review.