Embedded-hal: Support 7-bit and 10-bit I2C addressing

[BroderickCarlin]

The current I2C traits assume 7-bit addressing. I recently encountered a situation where I needed to talk to a device that utilized a 10-bit address and while it was possible to manipulate the current trait into working for 10-bit addressing it does require an abstraction layer capable of converting the 10-bit address into a corresponding 7-bit address + payload. While it would be a breaking change, I believe it would be beneficial to support both 7-bit and 10-bit addressing in the trait definitions itself to ease usage for developers that are attempting to interface with objects that implement these traits. One potential solution I'd like to put forward is defining an enum such as:

enum I2cAddress {
    SevenBit(u8),
    TenBit(u16),
}

which would be used in a trait definition such as:

pub trait Read {
    type Error;

    fn read(&mut self, address: I2cAddress, buffer: &mut [u8]) -> Result<(), Self::Error>;
}

A quick search showed this topic has been brought up in the past but was not actively discussed and no solutions put forward. Even though the thread has been quiet recently I believe the conversation in https://github.com/rust-embedded/embedded-hal/issues/100 would be relevant to a change such as this.

Let me know thoughts; I'd be happy to do some work and throw up a PR with these changes!

PS: If we are making breaking changes we might want to also discuss standardizing on addr or address in the trait definitions to avoid the mismatch that there is now 😄

[Rahix]

Not all I2C bus drivers support 10-bit addressing so I don't think it is a good idea to allow 10-bit addressing in general. Instead, I'd suggest having separate traits for 7 and 10-bit addressing with an optional default impl of 7-bit addressing for 10-bit implementations.

[ryankurte]

I'd suggest having separate traits for 7 and 10-bit addressing

Good call i think, what about genericising the i2c traits (ie. read<Word>) to be consistent with the spi traits?

[BroderickCarlin]

Not all I2C bus drivers support 10-bit addressing

Can you provide an example of any hardware that would not be able to support 10-bit addressing?

10-bit addressing was developed to be fully backwards compatible with 7-bit addressing and the two can co-exist on the same I2C bus. Because of 10-bit's backwards compatibility, any 7-bit driver can be made to communicate with 10-bit addresses through a software shim layer. It is because of this that I find your claim of Not all I2C bus drivers support 10-bit addressing to be potentially untrue.

While I respect your opinions, I would personally advocate for not splitting 7-bit and 10-bit up into separate traits as I fear this would lead to a trend of polluting the namespace and results in traits that are not generic enough for more abstracted uses.

The issue I foresee with the approach of using generics is burden on crate developers that may be wanting to implement this trait for their I2C drivers. If generics where used it would be crucial to add in a mechanic for determining if the provided type should be interpreted as a 7-bit or a 10-bit address. While this is not an impossible problem to solve I do believe it is an unnecessarily complex issue to thrust upon the developers that may want to work with your crate.

This comes back around to the idea proposed originally of using a simple enum. I would also like to point out that the consuming methods of the proposed "addr enum" return Result<>'s meaning that if a crate developer decided that they did not want to support 10-bit addressing that they can simply return an err if the user attempts to utilize 10-bit addressing. This also results in a low-effort update with minimal code changes for those that have already implemented these I2C traits.

[burrbull]

Another option is to accept the proposed variant with enum, but with different trait name.

New drivers will use new trait, but this will not break existant codebase.

[hannobraun]

It's worth noting that an enum has a small runtime overhead over a generic argument (I don't currently have an opinion on whether that should influence the decision one way or another).

@BroderickCarlin

The issue I foresee with the approach of using generics is burden on crate developers that may be wanting to implement this trait for their I2C drivers. If generics where used it would be crucial to add in a mechanic for determining if the provided type should be interpreted as a 7-bit or a 10-bit address. While this is not an impossible problem to solve I do believe it is an unnecessarily complex issue to thrust upon the developers that may want to work with your crate.

How about this approach:

• Add a generic argument, Address, with the trait bound AddressMode
• AddressMode is just an empty trait that documents which types should be expected there.
• AddressMode is implemented to SevenBitAddress and TenBitAddress.
• HAL implementations are expected to provide an implementation for Read<SevenBitAddress> etc. This is clearly documented in the doc comments of the traits.
• embedded-hal provides a default implementation of Read<TenBitAddress> (etc.) for all types that implement Read<SevenBitAddress> (etc.). HALs can override this implementation.

This way it's clear what the type parameter could be, implementations have exactly one trait to implement, and they can provide a second implementation if that's more efficient.

This comes back around to the idea proposed originally of using a simple enum. I would also like to point out that the consuming methods of the proposed "addr enum" return Result<>'s meaning that if a crate developer decided that they did not want to support 10-bit addressing that they can simply return an err if the user attempts to utilize 10-bit addressing. This also results in a low-effort update with minimal code changes for those that have already implemented these I2C traits.

Results can be hard to handle in embedded contexts, for example if you don't have a debugger attached currently. I believe we should use static guarantees as much as feasible, and that it's generally worth it to pay for those with some extra complexity.

[burrbull]

Or simply Read<u8> and Read<u16>.

[Rahix]

@BroderickCarlin: Sorry, my bad, I misremembered how 10-bit addressing works. With it being fully backwards compatible, we have no dependencies on underlying hardware capabilities which makes this a lot easier.

Still, I am not in favor of the enum-implementation. It would mean each HAL-crate had to reimplement the 10-bit addressing scheme while the protocol will always look exactly the same. We will have a lot of code-duplication and by that a lot more places for bugs to be introduced.

Secondly, as @hannobraun mentioned, it has an unnecessary runtime overhead. For any particular driver, it will only ever use one of the two modes. Cases where dynamic switching between the modes is needed will be very rare. From my experience, whenever something can be determined statically, one should use generics. The other way around, whenever something can only be determined at runtime, enums should be used.

simply return an Err if the user attempts to utilize 10-bit addressing

This would mean we cannot statically guarantee a program is using the correct driver and would make tools like panic-never no longer effective. I would prefer a solution where a compiling program also means that the bus and drivers were set up correctly. For me personally, this is a big selling point of the embedded-hal ecosystem right now.

I like @hannobraun's approach, particularly because of the following point they made:

embedded-hal provides a default implementation of Read<TenBitAddress> (etc.) for all types that implement Read<SevenBitAddress> (etc.). HALs can override this implementation.

This means we can have a single implementation which is used by most HAL-crates and thus means we have a lot less code-duplication. If a certain HAL can provide a more efficient implementation, it can still implement Read<TenBitAddress> manually. In practice, I guess this would look similar to the default ToggleableOutputPin implementation.

---

Just as an additional idea, I'd like to propose the following: We could also add more methods to the existing traits, with default implementations in embedded-hal. These methods make use of the exiting ones to implement 10-bit addressing. This would (afaik) not be a breaking change and would allow HAL-crates to continue working and automatically provide 10-bit addressing. As an example:

pub trait Write {
    type Error;

    fn write(&mut self, addr: u8, bytes: &[u8]) -> Result<(), Self::Error>;

    // Name should probably be something else ...
    fn write_10bit(&mut self, addr: u16, bytes: &[u8]) -> Result<(), Self::Error> {
        // Default implementation of 10-bit addressing
    }
}

Sadly, it won't work exactly like that as write_10bit would need to extend the bytes buffer to add the address LSB. Instead, it would have to be based on WriteIter or a transaction model as discussed in #94. For reading, read_10bit would either need a transaction model or WriteRead. This would make the interface a lot less consistent, so I am really not sure if it is a good idea at all.

This issue also applies to @hannobraun's approach, though there the dependencies are easier to model:

// Default implementation
impl<T> Write<TenBitAddress> for T
where
    T: DefaultImpl + WriteIter<SevenBitAddress>,
{
    // ...
}

impl<T> Read<TenBitAddress> for T
where
    T: DefaultImpl + WriteRead<SevenBitAddress>,
{
    // ...
}

[little-arhat]

Any progress on this? Thanks.

[therealprof]

I'm not convinced the 10 bit addressing can be implemented generically on any 7 bit master since it requires a very specific use of conditions which our API doesn't expose so it might break in subtle way in some implementations. To me the most workable approach would indeed be to add a new 10 bit interface.

[bofh]

How do you see driver code in a case with two different interfaces?

[therealprof]

Well, if your slave can support 10 bit addressing (and I don't recall ever having seen a datasheet of a discrete chip with 10 bit addressing support) then you'll likely have to implement 2 interfaces if you want to support both 7 and 10 bit addressing.

[BroderickCarlin]

As has been mentioned above, 10-bit addressing is fully backwards compatible and designed to exist in parallel to 7-bit addressing and utilize a 7-bit master. Can you give any examples of a case where this would not be the case or where it would cause problems?

Also to give an example, it is not uncommon to see I2C EEPROM devices use 10-bit addressing

[therealprof]

Can you give any examples of a case where this would not be the case or where it would cause problems?

You got this backwards, it's up to you to demonstrate it will not cause problems.

The issue here is not the parallel use of 10bit and 7bit addressing but that the controllers either need to be switched to 10bit addressing mode or simulate 10bit mode in software by manually setting the conditions.

My worry with that is that there might be cases where it will not be possible to implement 10bit adressing on the controller side at all (for whatever reason) but we're implying that this is always the case.

[ryankurte]

We have a window for breaking changes prior to the release of v1.0.0 (#177), I think it would be good to change the I2C trait to be generic over address size, so we can support both 7 and 10 bit addresses. Other opinions @therealprof @eldruin?

[eldruin]

TL;DR: I think @hannobraun's idea is the best.
Here is why, broken down per use cases:

• Device driver which only supports one addressing mode:
  The driver looks almost the same as now. Only one additional parameter to the I2C trait bounds:

impl<I2C, E> MyDriver<I2C>
where I2C: i2c::Write<SevenBitAddress, Error = E> {
  pub fn do_cool_stuff(&mut self) // ...
}

• Driver for device supporting both addressing modes:
  Complexity can be abstracted away into additional internal traits which can handle the addressing stuff. Driver code stays clean.
  This is nothing new. We already do this on drivers for devices compatible with both I2C and SPI. No need for duplicated code.
  Here a real example: usage, traits

impl<DI, E> MyDriver<DI>
where DI: WriteData<Error = E> {
  pub fn do_cool_stuff(&mut self) {} // ...
}

pub trait WriteData {
// ...
}

impl<I2C, E> WriteData for I2cInterface<I2C, SevenBitAddress>
where
    I2C: i2c::Write<SevenBitAddress, Error = E>,
{
  // ...
}

impl<I2C, E> WriteData for I2cInterface<I2C, TenBitAddress>
where
    I2C: i2c::Write<TenBitAddress, Error = E>,
{
  // ...
}

• Bus controller impl supporting only 7-bit addressing mode:
  Code stays almost the same, just adding one addressing mode parameter. Additionally, _if desired_:
  
  
  
  • 10-bit addressing can be software-emulated:
    
    Emulate by extending and copying payload in separate TenBitAddress implementation. Total flexibility to do whatever is necessary in this case since the code is independent.
  
  
  • 10-bit addressing cannot be software-emulated:
    
    Implementation does not offer implementation for TenBitAddress variant. The user gets a compilation error and everything is clear.
  
  
• Bus controller impl supporting both addressing modes:
  No problem. Two separate implementations guarantee as much flexibility as necessary. At the same time, sharing generic code is possible.

Additional benefits:

• No runtime performance cost
• No runtime switching, duplicated code or panics for unsupported modes.
• Consistent with what we do for code paths that can be determined statically by the compiler.
• To my taste elegant, simple and very descriptive.

write_10bit

I think the write_10bit() method proposal would only make sense in a scenario where breaking changes must be avoided at any cost, but we are breaking everything already :)

enum

While I understand the appeal of just transforming the u8 to an enum, the drawbacks have already been highlighted above. In summary:

• Need for match at runtime.
• Linker cannot drop unused code since the mode is selected at runtime.
• Unsupported modes can lead to errors or panics (in this case making stuff like panic-never unusable if any dependency panics)

Read<u8> / Read<u16>

The implications of 7-bit vs. 10-bit addressing are very significant. I think this variant can be quite misleading for users since it is trivial to convert an u8 to an u16. However, something very different would be happening on the wire.
Additionally, I do not think this is significantly easier to implement than @hannobraun's proposal.

Note: I just wrote the code down, please disregard small mistakes.
PS: FWIW, I3C supports only 7-bit addressing :D

[BroderickCarlin]

+1 for @hannobraun's proposed solution

If one point of contention is adding a default implementation for doing 10-bit addressing on a 7-bit bus controller in software I do want to point out that this should not be a breaking change and therefore could be done post 1.0 release.

[therealprof]

@eldruin Thanks for the exhaustive analysis and write-up, this coming from a driver specialist is very confidence inspiring.

I still am of the opinion that we cannot guarantee a default implement for 10bit address emulation to work with all existing implementations so we probably should not add one.

I would like to point out that I'm looking for feedback on #229. Since we're at the verge of breaking all impls anyway it would be great to sort that out at the same time.

[eldruin]

I have implemented @hannobraun's solution at #230

[eldruin]

PR #230 was merged to master 🎉
Is there need for further discussion or can this issue be closed now?