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

I2C is easy to misuse #13

Open
multiplemonomials opened this issue May 22, 2022 · 3 comments
Open

I2C is easy to misuse #13

multiplemonomials opened this issue May 22, 2022 · 3 comments
Labels
Usability This makes Mbed harder to use, and needs to be cleaned up.

Comments

@multiplemonomials
Copy link
Collaborator

The Mbed I2C implementation has two restrictions that are easy to violate:

  • Only one I2C object may be created for each I2C peripheral on the chip
  • The frequency must be set at the bus level, not at the per-device level, because I2C has to run at the lowest-common-demoninator speed of the slowest device on the bus.

Unfortunately, these are not made clear in the docs, and even some of the built-in drivers (I2CEEBlockDevice) violate them e.g. by having a constructor that takes pins instead of an I2C object. To make matters worse, violating the restrictions will appear to work fine in some scenarios, only to cause undefined behavior later on.

To fix this, we should:

  • Implement asserts (or at least warnings) that detect when multiple I2C instances are created on the same hardware, and when the frequency of a bus is set multiple times.
  • Refactor all first-party code to only take an I2C instance, not pins, and not set the frequency after construction
  • Add information to the docs about this.
@multiplemonomials multiplemonomials added the Usability This makes Mbed harder to use, and needs to be cleaned up. label May 22, 2022
@JohnK1987
Copy link
Member

  • Implement asserts (or at least warnings) that detect when multiple I2C instances are created on the same hardware, and when the frequency of a bus is set multiple times.

I can Imagine we will take address of current I2C controller in class constructor and place it into an static array for already reserved peripherals. There we can also check if the peripheral address is already present or not in the array.
With frequency seems to be similar technique.
Maybe functions from mbed_pinmap_common.c could be handy for that.

But the other hand, is it really necessary? This seems to be too much complicated with very low benefit. Unnecessary code with another wasted memory. Potentially issue with backwards compatibility.
Wouldn't it be solved, if we add some proper information to the documentation and introduce to others how Mbed I2C was designed, make examples of usage and recommendation how use it in own application/libraries?

Or exists another way?

BR, Jan

@chrissnow
Copy link

chrissnow commented Sep 12, 2024

I think I have come across this today, it was particularly aggravated by multiple threads both trying to use I2C, it was using STM32 V2 HAL

The symptoms were looping in the I2C ISR and no user coded executing afterwards.

I think it should really be possible to have multiple instances of I2C per bus like SPI
We could handle the speed restriction by not allowing the bus to exceed the slowest requested speed since the physical bus was initialised?

@multiplemonomials
Copy link
Collaborator Author

That could be really cool. Basically, what it would take would be adding the logic into I2C that SPI already has to share instances of the peripheral between multiple objects. Unfortunately it ends up being rather complex...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Usability This makes Mbed harder to use, and needs to be cleaned up.
Projects
None yet
Development

No branches or pull requests

3 participants