search

stm32-rs / stm32g0xx-hal

Peripheral access API for STM32G0 series microcontrollers
Apache License 2.0
74 stars 51 forks source link

i2c: Take pins in DefaultMode rather than Output<OpenDrain> #88

Closed davidlattimore closed 2 years ago

davidlattimore commented 2 years ago

Putting pins into open-drain mode causes them to be pulled low, which can confuse other devices on the bus.

This is a breaking change. Calling code can be fixed by not calling into_open_drain_output() on pins when setting up I2C.

Fixes #87

davidlattimore commented 2 years ago

I'm happy to make this a non-breaking change if you'd prefer. Restoring the pin mode in the release method would be the most complicated part. I think I'd need to create a new trait that reinitializes the current mode of the pin then implement that for Analog and Output.

andresv commented 2 years ago

I think we can easily allow such breaking change. @dotcypress?

dotcypress commented 2 years ago

This is a breaking change, but I'm inclined to merge this anyway, cause this level glitch on i2c initialization phase is something that we need to remove asap.

dotcypress commented 2 years ago

Ok, now I'm 99% sure that using OpenDrain mode for i2c was my design mistake. I'll mention that breaking change on release notes for the next release.

dotcypress commented 2 years ago

Looks like i2c stop working after merging this PR, I will dive in to find a root cause.

dotcypress commented 2 years ago

@davidlattimore @andresv I rolled back this commit until I found a solution.

davidlattimore commented 2 years ago

No problem. I just updated our application to use the HAL with this commit and it seems to be working for me. I'll be interested in what you find.

dotcypress commented 2 years ago

@davidlattimore I just try to run https://github.com/dotcypress/minesweeper with that change and display stop working. I believe we still need to switch mode to OpenDrainOutput before changing alt function.

The mistake I've made: forcing user to switch pin mode to OpenDrainOutput. Instead we need to use DefaultMode and change mode to OpenDrainOutput internally, with High level by default. It will take some changes on i2c macro, I will try couple of ideas and ping you.

davidlattimore commented 2 years ago

It looks like into_open_drain_output is enabling the internal pull-up resistors for the pin. I couldn't tell from looking at minesweeper whether you had external pull-ups or not. The internal pull-ups are fairly weak, so not sufficient for many applications (by my limited understanding of this stuff), but perhaps they're enough for your application depending on the I2C speed and timing settings.

We're using 2.2K resistors on our I2C bus, which perhaps explains why it still works for us.

Assuming that is the problem, do you think it should be left up to the user code whether to enable the internal pull-ups or not? Perhaps a config option, then passing an argument to SDAPin::setup.

dotcypress commented 2 years ago

I've tried with resistors(2.2K) and without: not working on both cases.

I think this happens because pin was switched to open_drain_output with default value is LOW, pulling pins down, then after set_alt_mode was called, i2c engine start controlling pins output.