search

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

fugit #177

Closed burrbull closed 2 years ago

burrbull commented 2 years ago

Thank you for this work! This looks good to me in principle, there is just some failure in the CI. Could you find out whether Into<Hertz> is still problematic or not for fugit? For the record, we briefly talked about this in rust-embedded/embedded-hal#352

Those problem was related to interacting RateExt32 trait with Into. Looks like it works fine now.

But I'm still insist don't use Into there as it complicate creating other high-level things which dependent from Pwm or CountDown. As we use Hertz almost everywhere (or time units), but I never seen using Kilohertz or Megahertz for such traits it's much better create/convert Hertz inplace before passing to pwm. With RateExt32 (which converts to requested) or like in stm32g0xx-hal where calling 10.mhz() just creates Hertz(10_000_000) instead of creating Megahertz(10) and then calling Into to convert to Hertz. Yes, without Into we need to convert obviously when passing core::Duration for example. But I don't think adding .into() or .to_rate() makes code worse. Furthermore if we plan to support different time types (with different ranges) on CountDown for example, it is better to make different implementations which can use different prescaler/reload formulas for those types. As automatic conversions can overflow/underflow/zero_div in unpredictable places. In my view using From/Into as generics for time/rate creates more problems then solves.

eldruin commented 2 years ago

I see. Thanks for the detailed insight. I agree with your analysis and would merge this once the CI passes.

For embedded-hal itself, I think we need some kind of generic bounds, though, so that different time representations can be used but that is a topic for embedded-hal.