i2c: add convenience functions to test `AddressMode`

[rmsyn]

Adds convenience functions to test the concrete types of AddressMode trait objects.

[Dirbaio]

what is the intended use case for this? You can already enforce at compile time that an I2C device is used only with 7bit/10bit addresses, why would you want to check it at runtime? For example, it's already impossible for the asserts you've added to the doc examples to fail at runtime, build would fail before that.

I'd rather avoid dyn and Any, while they do work in embedded they result in not-great code size.

[rmsyn]

what is the intended use case for this?

I am writing an I2C driver for a HAL, and the Linux version I'm basing the driver on has a function that sets a mode register based on a bitflag. The equivalent in the HAL driver would be testing based on the AddressMode trait object.

For example, it's already impossible for the asserts you've added to the doc examples to fail at runtime, build would fail before that.

I agree, just added them to have the code path tested with CI checks.

I'd rather avoid dyn and Any, while they do work in embedded they result in not-great code size.

Same, I was looking for alternatives, but this seemed like the most straight-forward method without introducing new dependencies.

Maybe adding a some functions to AddressMode would be better?

Something like:

pub trait AddressMode: ... {
    fn addr_max(&self) -> usize;
    fn is_seven_bit(&self) -> bool {
    self.addr_max() == 0b111_1111usize
    }

    fn is_ten_bit(&self) -> bool {
        self.addr_max() == 0b11_1111_1111usize
    }
}

impl AddressMode for SevenBitAddress {
    fn addr_max(&self) -> usize {
        0b111_1111
    }
}

impl AddressMode for TenBitAddress {
    fn addr_max(&self) -> usize {
        0b11_1111_1111
    }
}

[Dirbaio]

I am writing an I2C driver for a HAL, and the Linux version I'm basing the driver on has a function that sets a mode register based on a bitflag. The equivalent in the HAL driver would be testing based on the AddressMode trait object.

The way HALs add support for both 7bit and 10bit addrs is to do two impls:

struct MyI2c{...}
impl I2c<SevenBitAddress> for MyI2c {..}
impl I2c<TenBitAddress> for MyI2c {..}

Code within these impls already knows the address kind, guaranteed at compile time. they can of course call common code with the address mode as an argument, to avoid duplicating code.

Would this work for your use case?

[rmsyn]

Would this work for your use case?

I think it would, but I also wanted to contribute back a generic convenience function to test the type of AddressMode.

I figured if I ran into the issue, there would likely be other people that reach for similar functionality.

The changes to remove Any are up, and replaced with the AddressMode trait functions.

For reference, here is the function I'm currently working on: https://github.com/rmsyn/jh71xx-hal/blob/feature/i2c/src/i2c.rs#L343

Basically:

fn xfer_init<A: AddressMode>(&mut self, addr: A) {
    // ...

    if addr.is_ten_bit() {
        self.i2c.set_i2c_con_register(I2cCon::MASTER_10BIT);
    }
}

where xfer_init would be called inside both I2c<NBitAddress> impls.

[ryankurte]

The way HALs add support for both 7bit and 10bit addrs is to do two impls

we -could- have a dual-mode address type alongside SevenBitAddress and TenBitAddress but, then we would have to define switching and how HALs were to implement each of these, so i would like to see more evidence that this was worth the additional complexity before we went down that path.

@rmsyn you could move the types up to your interface with another trait?

pub trait I2cXfer<A: AddressMode> {
  xcfer_init(&mut self, a: A) ... ;
}

impl<I2C: I2cPeripheral> I2cXfer<SevenBitAddress> for I2c<I2C> {
  pub fn xfer_init(&mut self, a: SevenBitAddress) { .. }
}

impl<I2C: I2cPeripheral> I2cXfer<SevenBitAddress> for I2c<I2C> {
  pub fn xfer_init(&mut self, a: TenBitAddress) { .. }
}

[rmsyn]

@rmsyn you could move the types up to your interface with another trait?

What I ended up doing was making a new struct for the argument type that is basically a bitfield with a range for the address (7- or 10-bit). Have a feeling that a similar approach is probably going to work for other impls, too.

This PR can be closed if others don't feel like it's a good addition.

[eldruin]

Closing since the discussion seems settled on not merging this. Nevertheless, thank you for your proposal and please feel free to comment or reopen if there are further concerns.