Question: Generic Traits and I2C

[ScottGibb]

Hi all,

First off thank you very much for the awesome work in getting rust on the RP2040 and following pico boards. Im very excited to get using it.

My question is if there is a better way to handle I2C creation and the types that go along with it. Im currently working on a testing structure using the embedded-test crate. The code in question can be found in this PR dysonltd/tmag5273#53.

Ive highlighted a snippet of it below:

#![no_std]
#![no_main]

#[cfg(test)]
#[embedded_test::tests]
mod cold_start_tests {
    use defmt_rtt as _;
    use fugit::RateExtU32;
    use rp_pico::{
        hal::{
            self,
            clocks::ClockSource,
            gpio::{self, Pin},
            i2c::I2C,
        },
        pac::{self, I2C1},
    };
    use tests_common::generic_cold_start_tests::*;
    //TODO: Find a way to generify
    type PicoI2c = I2C<
        I2C1, // This could be agnostic. We don't need to know whether its I2C1 or I2C0
        (
            Pin<gpio::bank0::Gpio18, gpio::FunctionI2c, gpio::PullUp>, // Or this
            Pin<gpio::bank0::Gpio19, gpio::FunctionI2c, gpio::PullUp>, // Or This
        ),
    >;
    #[init]
    fn init() -> PicoI2c {
        let mut pac = pac::Peripherals::take().unwrap();
        let mut watchdog = hal::watchdog::Watchdog::new(pac.WATCHDOG);

        // Configure the clocks
        let clocks = hal::clocks::init_clocks_and_plls(
            rp_pico::XOSC_CRYSTAL_FREQ,
            pac.XOSC,
            pac.CLOCKS,
            pac.PLL_SYS,
            pac.PLL_USB,
            &mut pac.RESETS,
            &mut watchdog,
        )
        .unwrap();

        // The single-cycle I/O block controls our GPIO pins
        let sio = hal::sio::Sio::new(pac.SIO);

        // Set the pins to their default state
        let pins = gpio::Pins::new(
            pac.IO_BANK0,
            pac.PADS_BANK0,
            sio.gpio_bank0,
            &mut pac.RESETS,
        );

        // Create the I²C drive, using the two pre-configured pins. This will fail
        // at compile time if the pins are in the wrong mode, or if this I²C
        // peripheral isn't available on these pins!

        let i2c_controller = hal::i2c::I2C::new_controller(
            pac.I2C1, / I2C Definition
            pins.gpio18.reconfigure(), // Pin Definitions 
            pins.gpio19.reconfigure(), / Pin Definitions  
            400.kHz(),
            &mut pac.RESETS,
            clocks.system_clock.get_freq(),
        );
        i2c_controller
    }

    #[test]
    fn test_device_id(i2c: PicoI2c) {
        generic_test_device_id(i2c); // Pass the i2c variable to the inner test function
    }

In this code snippet in order to pass into my function I had to declare a PicoI2C type, however this type is very hardcoded such that its got I2C1, its got the pins declared in it. But the rest of the functions shouldn't care about this. I would like the code to be more generic such that you had it an I2C instance. In the esp-hal you can do this equivelant:

#![no_std]
#![no_main]

#[cfg(test)]
#[embedded_test::tests]
mod cold_start_tests {
    use defmt_rtt as _;
    use esp_hal::{
        i2c::master::{AnyI2c, I2c},
        prelude::*,
        Blocking,
    };
    use tests_common::generic_cold_start_tests::*;
    // Optional: A init function which is called before every test
    #[init]
    fn init() -> I2c<'static, Blocking, AnyI2c> {
        let peripherals = esp_hal::init({
            let mut config = esp_hal::Config::default();
            config.cpu_clock = CpuClock::max();
            config
        });
        let i2c = I2c::new(peripherals.I2C0, esp_hal::i2c::master::Config::default())
            .with_sda(peripherals.GPIO5)
            .with_scl(peripherals.GPIO6);
        i2c
    }

    #[test]
    fn test_device_id(i2c: I2c<'static, Blocking, AnyI2c>) {
        generic_test_device_id(i2c); // Pass the i2c variable to the inner test function
    }

This allows each of the test functions to not carte about the I2c instance and thus i can swap it in the init.

Is there a better way to do this in the rp-hal

[ScottGibb]

Upon further research and looking through the Embassy Project. I believe the rp-hal took a different approach to handling I2C, meaning that I cannot abstract the I2C type away any further.

Based on a dig through Embassy and esp_hal and finding the following snippets:

// ESP HAL
///I2C driver
pub struct I2c<'d, Dm: DriverMode, T = AnyI2c> {
    i2c: PeripheralRef<'d, T>,
    phantom: PhantomData<Dm>,
    config: Config,
    guard: PeripheralGuard,
}

//embassy-rp
/// I2C driver.
pub struct I2c<'d, T: Instance, M: Mode> {
    phantom: PhantomData<(&'d mut T, M)>,
}

This structure is what allows the generics at my level. However the way the rp-hal is setup, currently doesn't support this. Therefore the best way to abstract away the implementation details is the following:

    type PicoI2c = I2C<
        I2C1, // I Dont like this type should care if its I2C1 or other
        (
            Pin<Gpio18, FunctionI2c, PullUp>, // Or this
            Pin<Gpio19, FunctionI2c, PullUp>, // Or This
        ),
    >;

If anyone has any better ideas please respond to this post :)

[ithinuel]

Hi there, thank you for your comment.

The rp-hal’s I2C driver captures at compile time both the I2C block (I2C0/I2C1) as well as the pin mapping. It garanties at compile time that the pin used are correct.

So at the point of integration (where you select which peripheral & pins) are used, you need to know that those pins+peripheral are.

That being said your generic_test_device_id can definitely be generic over any I2C it receives.
You can use the embedded-hal’s trait to bind the type and access the operations that you need.
If there are more operations that you’d need, we can discuss of an extra trait to define & implement on the I2C and let generic_test_device_id bind over.

Alternatively, IIRC the embedded-hal’s I2c trait is dyn compatible so you should be able to make

fn generic_test_device_id(i2c: &mut dyn I2c);