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

I2C timing calculation broken #71

Closed sentinelt closed 3 years ago

sentinelt commented 4 years ago

Looks like the logic which calculates the value of the TIMINGR register is broken.

The code is located here: https://github.com/stm32-rs/stm32f7xx-hal/blob/master/src/i2c.rs#L338 One obvious problem with it, is that after: let fscll_mhz: f32 = base_clk_mhz / (scll as f32 + 1.0); the prescaler is then calculated with let presc = base_clk_mhz / fscll_mhz; which could be simplified to just let presc = sccl + 1; which is obiously wrong.

That said, I could not decipher how the entire logic is even supposed to work...

I see two possible ways to fix the problem:

  1. Instead of calculating this at runtime, have a way to provide this parameter as an argument to I2C initialization. TIMINGR can then be calculated using STM32CubeMX tool as can be seen in the screenshot below:

image

  1. Improve the calculation logic. I found pretty complex (and probably slow) implementation in the STM32Duino probject:

https://github.com/stm32duino/Arduino_Core_STM32/blob/master/libraries/Wire/src/utility/twi.c#L383

craigjb commented 4 years ago

I think #72 should resolve this.

hardfau1t commented 4 years ago

Instead of adding complex calculation which is likely prone to error, why dont we use some pre configured values, like libopencm3 also does like that http://libopencm3.org/docs/latest/stm32f7/html/i2c__common__v2_8c_source.html#l00456 , that would be easy on cpu also. i find no reason for anyone to use other than standard frequency