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.

[eldruin]

This seems quite interesting, thank you!
I just added a couple nitpicks.

One thing I have been thinking about is how it is necessary to specify the direction (R/W) when calling start and then you need to call the appropriate method (read/write), which introduces a new error source: saying you are going to read and then calling write or viceversa.

This is correctly documented and matches exactly how the protocol works but since implementations will most probably cache the address and direction and send it when an actual operation is done, I wonder if we can avoid this error source by omitting the direction in the start and let it be determined by the next operation:

bus.start(0x10)?; // HAL impl stores address for next operation
bus.read(&[0]) // HAL impl sends [0x21, 0]

Just doing an ACK would still be possible by sending an empty array (although HAL implementations need to be made aware of this possibility so that they do not return an error due to empty data).

bus.start(0x10)?; // HAL impl stores address for next operation
bus.read(&[]) // HAL impl sends [0x21]

One could also rename the start method something like set_address but start seems more appropriate and all the operations will probably run deferred anyway.

Thoughts?

[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 :)

[Rahix]

After seeing @eldruin's code, I think it is a bit unfortunate to see driver code requiring two bounds each time, like here:

impl<I2C, E> WriteData for I2cInterface<I2C>
where
    I2C: embedded_hal::i2c::blocking::I2cDevice<Error = E>,
    I2C::Bus: embedded_hal::i2c::blocking::I2cBus<Error = E>,

Unless I am completely off, this is caused by the way the AddressMode is implemented, right? I wonder if there ever is a need to address a device in both seven-bit and ten-bit mode from the same I2cDevice, like the current implementation is structured to allow.

I think it might be better to make the I2cDevice also generic over the AddressMode:

pub trait I2cDevice<A: AddressMode = SevenBitAddress>: ErrorType {
    type Bus: I2cBus<A, Error = Self::Error>;
}

This should allow drivers to only require a single bound.

[Dirbaio]

I don't think this'd work, because all I2cDevice methods would be "duplicated" for each addr type.

For example, if a driver needs to access both 7bit and 10bit addrs, it'd do

impl<I2C, E> WriteData for I2cInterface<I2C>
where
    I2C: I2cDevice<SevenBitAddress, Error = E> + I2cDevice<TenBitAddress, Error = E>,

but then when it does i2c.transaction(...), it'll be ambiguous whether it's <bus as I2cDevice<SevenBitAddress>>::transaction() or <bus as I2cDevice<TenBitAddress>>::transaction(), which makes no sense. Opening a transaction shouldn't care about the address mode.

[Dirbaio]

Also there'd be no guarantee <I2C as I2cDevice<SevenBitAddress>>::Bus and <I2C as I2cDevice<TenBitAddress>>::Bus are the same type, which is something we do want IMO.

[Rahix]

For example, if a driver needs to access both 7bit and 10bit addrs, it'd do

My thought was that this usecase is exceedingly rare. Maybe I am missing something but so far I have not seen any driver where this is required.

[Rahix]

I know this goes into a completely different direction but I want to at least have the idea discussed: Should we tie the device address to the I2cDevice instead, such that the driver code never mentions it directly?

Prior art for this is the way the Linux kernel manages I2C devices. Drivers get a struct i2c_client* which is already tied to a specific address. The driver then only issues writes and reads to the i2c_client without ever bothering with the devices specific address.

I see a number of advantages to this:

• Device-drivers cannot accidentally interact with a wrong device on the bus - they only see the specific client that they are supposed to interact with.
• The address, which is part of the board configuration, is no longer managed by the driver and thus something like a device-tree structure can be implemented here as well. Right now, drivers all have their own creative ways of address initialization which makes this rather difficult.
• We can have "bus" implementations that hide details behind the address like an I2C mux. The I2cDevice handles the muxing opaquely so the driver still sees no difference.

The two downsides I see are

• If there ever is a driver that needs to access two devices with separate addresses, it needs to get two separate I2cDevice handles. This doesn't seem to be a problem in Linux so I'm not sure how bad it would be for us.
• The initialization code will be a bit more verbose because there are now two steps (address/bus, then device). Roughly:

  use i2c_encabulator::{EncabulatorAddress, EncabulatorDriver};
  
  let i2c_dev = bus.acquire(EncabulatorAddress::with_config_pins(true, false, true).into());
  let encabulator = EncabulatorDriver::new(i2c_dev);

[Rahix]

(And I think this would be more inline with the way we handle SpiDevice: The SpiDevice contains the chip-select management so a driver doesn't care about the "address" on the SPI-bus)

[eldruin]

This is an interesting proposal. Here some additional background info from my own experience.

There definitely are devices that use several addresses. From the top of my head, for example the fairly popular lsm303agr has two addresses: accelerometer at 0x19, magnetometer at 0x1E (the one in the newer micro:bit boards).

Another downside is that the user needs to know the address of the device to use the driver, while at the moment this is all handled by the driver. This will lead again to a fair amount of creative ways to expose the address in the driver via some public constant (which may actually be wrongly aliased on import if using multiple devices: use mlx90614::ADDRESS as DISPLAY_ADDRESS, this is speculative though) or whatever.

One additional downside is when using devices which have a software-programmable address. For example, the classic MLX90614 IR thermometer. At the moment supporting this is trivial, just store the address as a data member and update it as necessary.
If the address is tied to the I2cDevice this would be slightly more complicated. For example it would need a method that takes the new address and a new I2cDevice with the new address (note: these have to match, unclear how to verify this, maybe we should then expose the address in the device) and replaces the device in the driver (assuming the I2cDevice implementation type does not change, please do not add the address as part of the type in your impl :).
Going from a 7-bit address to a 10-bit address would be more complicated since the actual I2cDevice type changes, but 10-bit devices are less common anyway (I3C removed 10-bit addressing completely).

There is also stuff like the general call address (0x0 + command byte), which may be used to issue a command for all devices, for example a software reset (0x0+0x6). In that case, we would need to provide the driver with the whole bus, which actually makes sense, although it makes it slightly more complicated for the case of a single device.

Still I think this proposal is interesting and I like it from a pure-software perspective.

[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.