rp-rs / rp-hal

A Rust Embedded-HAL for the rp series microcontrollers
https://crates.io/crates/rp2040-hal
Apache License 2.0
1.45k stars 234 forks source link

Zero-length read and write should be allowed by I2C HAL #678

Open agausmann opened 1 year ago

agausmann commented 1 year ago

Masters can probe if an address is responding by sending a "zero-length" transaction, containing only the address bits, read/write bit, and the first slave ACK.

This is a de-facto standard in I2C. It's formalized in at least one spec that I'm aware of, as "acknowledge polling":

image

Right now, the RP2040 I2C HAL validates that the TX and RX buffers are non-empty, which prevents such transactions:

https://github.com/rp-rs/rp-hal/blob/a12bcdae6effc8d9fe58d5de3c8edf67f23fe529/rp2040-hal/src/i2c/controller.rs#L109-L122

As long as the RP2040 hardware supports it, I think this is an unnecessary restriction that should be removed. As discussed below, this is a hardware limitation, but zero-length transactions could be implemented in software.

ithinuel commented 1 year ago

That's the thing, the hardware does not support it. If you need to send zero length transaction, you'll need to resort to i2c pio.

For the details I'll let you have a look at the datasheet but the TL;DR is that the hardware handles the transfer of the address but the only way to make it send the address is to send at least 1 byte of data, hence the impossibility to send 0-length transaction.

jannic commented 1 year ago

We could do something like micropython does, and implement the zero-length transaction in software, transparent to the caller: https://github.com/micropython/micropython/blob/master/ports/rp2/machine_i2c.c#L144-L163

ithinuel commented 1 year ago

This would require to either make the i2c driver explicitly require a time reference or make it rely on the assumption that another time reference (at known frequency) is available somewhere. In addition to that it'd induce some surprising latency (changing modes for the pins etc).

jannic commented 1 year ago

Yes, it's probably a bad idea to do this implicitly like in micropython. This transparency may be nice for the caller, but it's far from zero-cost. But it would be nice to have it as an option. Like a separate function to do zero-length transfers?

(In the end, all I'm saying is that we shouldn't just close this ticket with reference to hardware capabilities, but handle it as a valid feature request.)

ithinuel commented 1 year ago

Indeed, I agree this is a valid usecase and should not be dismissed.

On the other hand, we do not intend to compensate for hardware limitations by emulating bitbanging etc. A dedicated method present there as a "workaround" sounds good to me, but this should not be used in embedded-hal's implementation.

A wrapper for I2C that take the I2C peripheral, the pins and a time reference to use it would be good to me.

As in:

agausmann commented 1 year ago

Thanks for the prompt response! That makes sense, after studying the register interface for a little bit.

I've opened a PR to add these details to the documentation for the relevant I2C errors.