Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add I2C slave mode to fix #636 #671

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Feb 22, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@jbeaurivage
Copy link
Contributor

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
Copy link
Contributor Author

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.

Copy link
Contributor

@jbeaurivage jbeaurivage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@@ -293,18 +296,30 @@ pub enum InactiveTimeout {
Us205 = 0x3,
}

pub trait I2cMode {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -276,6 +276,9 @@ pub use config::*;

mod impl_ehal;

// mod client;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -253,31 +258,33 @@ impl<P: PadSet> Config<P> {
pub trait AnyConfig: Is<Type = SpecificConfig<Self>> {
type Sercom: Sercom;
type Pads: PadSet<Sercom = Self::Sercom>;
type Mode: I2cMode; // default to super::Master when associated_type_defaults is stable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants