Add I2C slave mode to fix #636

[tgross35]

Summary

Add I2cClient and associated types to allow using I2C as a slave/client

Checklist

• CHANGELOG.md for the BSP or HAL updated
• All new or modified code is well documented, especially public items
• No new warnings or clippy suggestions have been introduced (see CI or check locally)

If Adding a new Board

N/A

If Adding a new cargo feature to the HAL

N/A

[tgross35]

I'm currently just working on the very basic structure. What would be the best way to lay this out - all types in the existing I2C module with a prefix, everything in an entirely separate module?

[jbeaurivage]

I would recommend trying to reuse the existing code rather than copy-pasting into a new module to avoid code duplication. Most likely a new generic parameter to the Config and I2c structs would create the distinction between master and slave. The master-specifc and slave-specific methods could always go in their own modules if you wish.

[tgross35]

Awesome, thanks for the suggestion. That does sound much cleaner, but I was unsure about breaking the existing type API

[jbeaurivage]

Awesome, thanks for the suggestion. That does sound much cleaner, but I was unsure about breaking the existing type API

You can always add the master/slave type at the end, and provide a default type. This will prevent breaking the API in most cases:

pub struct Config<P, M = Master>
where
    P: PadSet,
    M: OpMode
{
    pub(super) registers: Registers<P::Sercom>,
    pads: P,
    op_mode: M
    freq: Hertz,
}

[tgross35]

Alright, I think I got it incorporated it into the type system - no new implementation yet, just a 1:1 change. Would you be able to take a look and see if it seems right?

AnyConfig was my only concern, since it needs a Mode type and associated type defaults aren't stable, so I believe this would be breaking for anyone implementing that specific trait themselves. Which I think is probably unlikely, but unsure how that affects things.

[jbeaurivage]

Looks good so far, I haven't looked at the copy-pasted modules, as I'm assuming they're going away, and only the master/slave specific behavior will be broken out?

[jbeaurivage]

I would make this trait sealed so that it can't be implemented outside of this crate:

pub trait I2cMode: Sealed {}

and also add a short doc comment.

[jbeaurivage]

I would try to standardize what terminology we use: master/slave vs host/client. Personally I don't care either way, with maybe a slight preference towards whatever the datasheet uses.

[tgross35]

Thanks for bringing this up, I meant to ask. The datasheet does use host/client, so I can go ahead with that.

[jbeaurivage]

I don't think AnyConfig is meant to be implemented outside of the HAL anyways.

[tgross35]

Oh good point, Sercom and Padset are sealed so it's de facto sealed anyway. I'll make that explicit just so it's also clear in the docs.

[tgross35]

Looks good so far, I haven't looked at the copy-pasted modules, as I'm assuming they're going away, and only the master/slave specific behavior will be broken out?

Thanks for the quick look! Yeah they will go away, I just need to go through and sort things into master/slave/both impls