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

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

Open
agausmann opened this issue Aug 30, 2023 · 6 comments
Open

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

agausmann opened this issue Aug 30, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@agausmann
Copy link
Contributor

agausmann commented Aug 30, 2023

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:

fn validate(
addr: u16,
opt_tx_empty: Option<bool>,
opt_rx_empty: Option<bool>,
) -> Result<(), Error> {
// validate tx parameters if present
if opt_tx_empty.unwrap_or(false) {
return Err(Error::InvalidWriteBufferLength);
}
// validate rx parameters if present
if opt_rx_empty.unwrap_or(false) {
return Err(Error::InvalidReadBufferLength);
}

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
Copy link
Member

ithinuel commented Aug 30, 2023

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
Copy link
Member

jannic commented Aug 30, 2023

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
Copy link
Member

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
Copy link
Member

jannic commented Aug 30, 2023

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
Copy link
Member

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:

  • 1 bare i2c implementation backed by strict hardware and its limitation.
  • a wrapper relying on the previous hw backed implementation + software based workaround for 0length transaction.

agausmann added a commit to agausmann/rp-hal that referenced this issue Aug 30, 2023
As discussed in rp-rs#678, the hardware does not support zero-length read/write transactions.
@agausmann
Copy link
Contributor Author

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.

@jannic jannic added the enhancement New feature or request label Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants