Add I2C support for esp32 #4259
Conversation
| } | ||
|
|
||
| func (p Pin) pinReg() *volatile.Register32 { | ||
| return (*volatile.Register32)(unsafe.Pointer((uintptr(unsafe.Pointer(&esp.GPIO.PIN0)) + uintptr(p)*4))) |
There was a problem hiding this comment.
Why not also use unsafe.Add() here in similar fashion to the implementation of nextAddress() function just below?
There was a problem hiding this comment.
The function is copied from machine_esp32c3.go#L114, which uses uintptr addition (but it seems to be an outlier in that file too). For consistency this function should probably be moved to machine_esp32.go?
There was a problem hiding this comment.
For consistency this function should probably be moved to machine_esp32.go
Agreed, but that can happen in a separate PR.
| return (*volatile.Register32)(unsafe.Add(unsafe.Pointer(reg), 4)) | ||
| } | ||
|
|
||
| // CheckDevice does an empty I2C transaction at the specified address. |
There was a problem hiding this comment.
Seems like a cool function, but I am just wondering if it really belongs in this PR? Perhaps it can either be added as something available to all i2c implementations, which as least right now it is not. What do you think?
There was a problem hiding this comment.
Another option would be to modify Tx so that Tx(addr, nil, nil) returns an error if there is no device with that address. This seems to be how other microcontroller libraries do it, e.g. Arduino:
WIRE.beginTransmission(address);
error = WIRE.endTransmission();That behavior also matches the method comment better, which implies that the address is always written:
// It clocks out the given address, writes the bytes in w, reads back len(r)
// bytes and stores them in r, and generates a stop condition on the bus.(currently the method just does nothing if both slices are empty)
Ideally that would be reflected in the generic i2c documentation and other implementations as well, which – I agree – is not part of this PR
There was a problem hiding this comment.
Ideally that would be reflected in the generic i2c documentation and other implementations as well, which – I agree – is not part of this PR
That makes sense to me! 😄
|
This is really great @jfreymuth thank you very much for working on it. Please see my comments inline. |
|
Oh yeah, one thing I almost forgot is we need to also add a smoke test for i2c on esp32. |
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?
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. |
|
We have something similar for the nrf chips:
|
|
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 |
|
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. |
|
Sounds good to me! 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 |
That is in fact in this very PR already! :) In which case I think we can merge now. |
|
Now going to squash/merge. Thank you @jfreymuth for working on this and to @aykevl for review. |
| // timeout in microseconds. | ||
| const timeout = 40 // 40ms is a reasonable time for a real-time system. | ||
|
|
||
| cmd := make([]i2cCommand, 0, 8) |
There was a problem hiding this comment.
This could be added as a field to I2C to not risk heap allocation
cmdbuf [8]i2cCommand
| // timeout in microseconds. | ||
| const timeout = 40 // 40ms is a reasonable time for a real-time system. | ||
|
|
||
| cmd := []i2cCommand{ |
There was a problem hiding this comment.
Same as above, we could store this memory in I2C.
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.