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

Rework i2c #2437

Closed
wants to merge 18 commits into from
Closed

Rework i2c #2437

wants to merge 18 commits into from

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 31, 2024

  • Split implementation in versioned modules
  • No more doc(hidden) trait methods
  • apply_config, with_sda/scl
  • embassy_embedded_hal::SetConfig
  • the master driver has been moved to i2c::master so we don't have to move it when introducing slave
  • transaction now chunks reads and writes

cc #2416 and closes #1919

@bugadani bugadani added the skip-changelog No changelog modification needed label Oct 31, 2024
esp-hal/src/i2c.rs Outdated Show resolved Hide resolved
@bugadani
Copy link
Contributor Author

bugadani commented Oct 31, 2024

This commit is a bit of an experimental idea, I'd love to hear feedback on it!

@bugadani bugadani force-pushed the i2c-driver branch 12 times, most recently from a9ffb92 to 9581054 Compare November 4, 2024 15:59
@bugadani bugadani marked this pull request as ready for review November 4, 2024 18:46
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 5, 2024

This commit is a bit of an experimental idea, I'd love to hear feedback on it!

I wonder which commit that was since the link goes nowhere now

@bugadani
Copy link
Contributor Author

bugadani commented Nov 5, 2024

This commit is a bit of an experimental idea, I'd love to hear feedback on it!

I wonder which commit that was since the link goes nowhere now

2013299

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 5, 2024

I like the version module - wonder if any other peripheral would be a good fit for something like that (I think most peripherals are not that bad in terms of different versions - so maybe none)

I also like the idea of pulling eh impls into separate (quite small) files

@bugadani
Copy link
Contributor Author

bugadani commented Nov 5, 2024

We don't need to split apart every other driver and we can only extract the differing bits as well. There is enough overlapping code to make this scheme somewhat awkward although I'm biased towards liking the idea.

@bugadani bugadani removed the skip-changelog No changelog modification needed label Nov 5, 2024
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

I think for I2C, given the wild differences between chips, this version file structure makes sense. I think it's unlikely that we'll adopt this for any other driver (unless I'm missing one, AFAIK the rest of the drivers are similar enough between chips) until new chips come out that warrant the change.

esp-hal/src/i2c/master/mod.rs Outdated Show resolved Hide resolved
hil-test/tests/i2c.rs Show resolved Hide resolved
esp-hal/src/i2c/master/support/eh.rs Show resolved Hide resolved
@@ -97,12 +99,129 @@ The `with_timeout` constructors have been removed in favour of `set_timeout` or
+let i2c = I2c::new(peripherals.I2C0, io.pins.gpio4, io.pins.gpio5, 100.kHz()).with_timeout(timeout);
```

## Changes to half-duplex SPI
### I2C drivers can now be configured using `i2c::master::Config`
Copy link
Member

Choose a reason for hiding this comment

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

This diff looks quite goofy, but I'm trusting that you've just been rearranging some things and that it is correct.

Copy link
Contributor Author

@bugadani bugadani Nov 6, 2024

Choose a reason for hiding this comment

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

Yeah I'm not that confident (I mean the changes are OK, the reason behind them is shaky). I couldn't decide how to organize things, and I think per-peripheral is better than per-topic (e.g. config changes, constructor changes, etc.).

@jessebraham
Copy link
Member

This should be split into multiple PRs IMO, it's essentially impossible to review something of this size in any meaningful way.

@bugadani
Copy link
Contributor Author

bugadani commented Nov 6, 2024

I can send in each commit separately if that helps.

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.

Implement embassy_embedded_hal::SetConfig
4 participants