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

fix: RP2040 I2C fundamental issues #2031

Closed
wants to merge 1 commit into from

Conversation

jcdickinson
Copy link
Contributor

@jcdickinson jcdickinson commented Oct 10, 2023

This is more to open a discussion about the current I2C implementation and the issues with it.

I was experiencing severe stability issues with I2cSlave, which I was not experiencing with the rp-rs embedded-hal crate. The maximum speed possible seems to be 5KHz (as per the current example in the repo, so I assume this problem is tribal knowledge?). This is probably because the current slave implementation doesn't set IC_RX_FULL_HLD_BUS_EN - which essentially enables clock stretching.

The current I2cSlave interface seems to be incompatible with clock stretching. I have brought over the "event iterator" interface from rp-rs and made it async. This is really sad because the existing interface seemed much nicer to use than the event iterator one.

The combination of my changes to I2c and I2cSlaveEventIterator bring things up to 400KHz on my prototype. Unfortunately it also looks like this completely breaks I2cSlave.

Also:

  • Limit slew rate, per RP2040 spec.
  • Move pin configuration to single fn.
  • In the Arduino/C sources, and rp-rs, the system clock is used as for the timing calculations. The peripheral clock makes more sense, so I'm not entirely sure what's correct here.
  • Stop/start flags for async methods have been corrected.
  • write_read method added to I2c (which is required for idiomatic i2c)

@@ -55,23 +30,28 @@ pub struct Config {
pub addr: u16,
}

struct InternalConfig {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A poor attempt at keeping the existing stuff working, it doesn't seem to work.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 10, 2023

Thanks for the PR.

This seems to fix several unrelated issues and do some refactors too. Could you separate them out in several PRs, so they can be reviewed independently? Thanks!

The current I2cSlave interface seems to be incompatible with clock stretching.

Why is that? I don't think it's fundamentally incompatible, it should be possible to first read the request, then hold the bus with clock stretching while user code decides what to do, then respond.

Ideally we should fix just the clock stretching, without replacing the entire API.

@jcdickinson
Copy link
Contributor Author

I'll have another go at getting the existing interface working, but will provide some of the other fixes as separate PRs first.

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