atsamd icon indicating copy to clipboard operation
atsamd copied to clipboard

Add I2C slave mode to fix #636

Open tgross35 opened this issue 2 years ago • 6 comments

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 avatar Feb 22 '23 07:02 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?

tgross35 avatar Feb 22 '23 07:02 tgross35

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.

jbeaurivage avatar Feb 22 '23 12:02 jbeaurivage

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

tgross35 avatar Feb 22 '23 15:02 tgross35

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,
}

jbeaurivage avatar Feb 22 '23 15:02 jbeaurivage

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.

tgross35 avatar Feb 22 '23 17:02 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

tgross35 avatar Feb 22 '23 19:02 tgross35