WIP: I2C bus/device.

[Dirbaio]

This PR refactors the I2C traits into Bus and Device, similar to what was done to SPI a while back.

This is actually much simpler than SPI thanks to not having a CS pin.

TODO

• [ ] Do we impl<T: SpiBus> SpiDevice for T? In SPI it was not possible due to CS, but here it is possible. It can make usage easier (no need for wrapping the bus in an ExclusiveDevice if not sharing) but maybe it's confusing?
• [ ] Update some HALs, to test it out
• [ ] Update some drivers, to test it out
• [ ] Update EHA
• [ ] Cleanup docs.
• [ ] Update changelog

[rust-highfive]

r? @eldruin

(rust-highfive has picked a reviewer for you, use r? to override)

[Rahix]

Do we impl<T: SpiBus> SpiDevice for T? In SPI it was not possible due to CS, but here it is possible. It can make usage easier (no need for wrapping the bus in an ExclusiveDevice if not sharing) but maybe it's confusing?

I think it is better to stay consistent than to add clever tricks. People who learned the pattern for one bus should be able to do the same thing for the other bus without having to "re-learn" what the pattern there needs to be (unless we're talking about bus-protocols which work in fundamentally different ways). It should also lead to more readable application code when the pattern is the same for all bus types.

[Dirbaio]

What would this do, then?

bus.start(0x10)?;
bus.write(...)?;
bus.read(...)?;

• Option 1: it does an "implicit" repeated start.

  • However, "start, write, write" wouldn't do a repeated start, which I think is inconsistent/surprising.

• Option 2: the read returns an error.

  • You can still have errors due to invalid call sequences, so I don't think it's that much of a win.
  • The docs would have to say "after doing a start, the first read/write you do "locks" the direction, after which all the other read/writes must be the same direction until you do another start" which I think is more complex.

Also, what would this do?

bus.start(0x10)?;
bus.stop()?;

• Option 1: Nothing? It's weird.
• Option 2: still do the start condition, "inventing" the direction bit out of thin air. Weird too.

Given the above, I'd rather make the bus API as "explicit" as possible even if it is a bit redundant. Most people will be using the write, read, write_read helpers in I2cDevice, so I think it's OK if the raw bus API is a bit verbose.

Also I'm not sure if all HALs will buffer the address. For example, in STM32 I2C, "send the address+direction byte" is a thing you explicitly do, and have to wait for before doing transfers.

[eldruin]

I had not thought this through. I agree with your assessment and would keep the API as-is, thanks!

[eldruin]

I have adapted my classic ads1x1x I2C ADC driver crate and the necessary changes to use this PR were really minimal :)

[eldruin]

I have adapted linux-embedded-hal to this branch. I have not tested it in hardware yet but if I did not miss anything, I think it should work.

In order to uphold the bus contract, I had to do several things like deferring all actual transfers to the stop() method and keeping temporary objects with information about the operation about to start and the last one. Deferring all transfers until stop() may be unexpected but was necessary so that I can set the appropriate message flags in order to avoid intermediate restarts and stops, detect error conditions and so on. Other than that, the I2C bus has no way to know when a group of messages (with a stop at the end) can be sent since it has no notion of whether it has been acquired (which is fine).

Something weird is that stopping and even flushing is possible within a "transaction", so you can actually make an arbitrary number of I2C transactions within a transaction call.

device.transaction(|bus| { // an I2C transaction means communicating with no stops in between
    bus.start(0x1, Write)?;
    bus.write(&[0])?;
    bus.stop()?; // however, we can add a stop
    bus.flush()? // and even flush
    bus.start(0x1, Write)?; // and start a second transaction
    bus.write(&[0])
});

I think this might be confusing for users since the transaction method is also about the acquisition of the bus and not only about a transaction in the I2C protocol sense.

Another interesting thing is that now we only use the bus and we do not use I2c devices provided by the kernel (intended for a single target address) at all anymore.

Anyway these are just observations and I think this approach can work well and simplifies the code a lot on the HAL impl side.

[ryankurte]

hey i like the idea of unifying the bus sharing code over SPI and I2C, but i'm cautious that it might not always be -possible- to implement an arbitrary collection of operations without foreknowledge (ie. in a closure that may contain other logic vs. providing an unambiguous Transactional style list).

for example, the ESP32 i2c generally requires the whole transaction to be setup and committed as one object.

i think for SPI the coupling of exclusivity and transactions via CS broadly makes sense, but that for I2C the relationship between exclusivity and transactions is less clear / could be better represented by the prior Transaction/Operation api on the bus object?

[Dirbaio]

I no longer think I2C bus/device makes sense. Opened #440 instead.