Implement full I2C timing using Arduino algorithm

stm32-rs / stm32f7xx-hal

A Rust embedded-hal HAL for all MCUs in the STM32 F7 family

Apache License 2.0

117 stars 68 forks source link

Implement full I2C timing using Arduino algorithm #72

Closed craigjb closed 3 years ago

craigjb commented 4 years ago

The Arduino project on has a very nice algorithm for determining STM32 I2C timing parameters based on the I2C timing specs for each mode. I ported the algorithm to Rust, and Rust-ified it a bit. The C origins show through, but it works well. Test for Standard and Fast modes on real hardware. Also, my previous testing of the current implementation on hardware showed a very slow SCL frequency.

craigjb commented 4 years ago

This can increase startup time a little when running in debug builds. In release, not a problem. I think this should resolve #71

sentinelt commented 4 years ago

I cherry picked this change onto v0.2.0 and tested it on my NUCLEO-F767ZI board. I got: panicked at 'attempt to multiply with overflow', stm32f7xx-hal/src/i2c.rs:415:23

craigjb commented 4 years ago

Thanks for testing. Could you let me know what parameters you used (clock, I2C speed) when you hit an overflow?

sentinelt commented 4 years ago

My bad, I've set to big value for data_timeout_us.

I did some testing for different clock speeds and measured real I2C speed using an oscilloscope, here are the results:

sysclk	requested I2C speed	measured I2C speed
216Mhz	Standard 100Khz	109Khz
216Mhz	Fast 400Khz	417Khz
192Mhz	Standard 100Khz	104Khz
192Mhz	Fast 400Khz	417Khz

I believe this is good enough in practice.

Still, setting TIMINGR to the values calculated using STM32CubeMX gives better results:

sysclk	requested I2C speed	measured I2C speed
216Mhz	Standard 100Khz	100Khz
192Mhz	Standard 100Khz	100Khz

So in addition to this automatic calculation, I would still like to have an option to specify custom TIMINGR value. I can add pull request with that change, if you're ok with that.

craigjb commented 4 years ago

@sentinelt An option to manually set a TIMINGR value would be awesome.

I pretty much directly ported the Arduino I2C timing calculation. It looks like that was pulled from ST code at some point, and then modified from there. It makes sense that the CubeMX software may provide better results, and I wonder if we can duplicate that calculation here.

pftbest commented 4 years ago

I dumped a bunch of TIMINGR values from the cube:

std-50.txt std-100.txt fast-50.txt fast-100.txt fast-150.txt fast-350.txt fast-400.txt

There are some patterns in the values.

(sclh + scll) * presc is linear in respect to frequency in all the modes with the same exact slope for fast and standard mode
scldel is linear when presc == 1
sdadel is always 0
scll / sclh is roughly 1.5 for standard mode and about 3.3 for fast mode, but it's very noisy.

It seems easy to calculate exact values for (sclh + scll) * presc and scldel * presc with just a few multiplications, however the actual values for sclh, scll and presc vary a lot with a little change to the frequency.

https://gist.github.com/pftbest/a2f182336bdd5dd4eef087ebdcb94ebc

hannobraun commented 4 years ago

I don't have time to properly review and test, but if it works for you folks, I'm happy to merge it. Open questions:

Are there any concerns about copyright, given that this is a port of Arduino code?
Do we want to merge this as-is, or are you going to try and improve this based on CubeMX?

mvertescher commented 4 years ago

Yeah, it'd be nice to land this change. Some units test on calculate_timing might be helpful so we can refactor that function, but it's not a major issue and we can follow up on it later.

hannobraun commented 3 years ago

Closing in favor of #95, which has just been merged.