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

Clocks can no longer be specified in MHz #136

Open rfuest opened 3 years ago

rfuest commented 3 years ago

Before the migration to embedded-time it was possible to write HSEClock::new(216.mhz(), HSEClockMode::Oscillator). This isn't possible anymore, because Into<Hertz> isn't implemented for embedded-times Megahertz.

The way the conversion is implemented in embedded-time makes more sense, because it prevents possible integer overflows. But the use of Into<Hertz> in e.g. HSEClock::new suggests that values with any frequency unit could be used. I didn't expect this behavior while porting to the new version and had to look into why this didn't work anymore.

Would it be better to change the API to take Hertz values instead of impl Into<Hertz> values? This way the types should make the user aware that they need to convert the value into hertz first if they want to use another unit (e.g. 216.MHz().try_into().unwrap()).

Another option would be to change the type bounds from Into<Hertz> to TryInto<Hertz>, but this would mean that all functions that take a frequency would need to return a Result. I don't think this would be a good idea.

hannobraun commented 3 years ago

I agree with your conclusion that taking a Hertz value directly would be better and less confusing. The conversion to embedded-time was a rather minimally invasive procedure, and I didn't think a lot about how it affected usability.

I'd definitely merge a PR making the change from Into<Hertz> to Hertz!

hellow554 commented 3 years ago

While #137 was merged and resolves the issue with rcc.rs, other places are left:

rg 'Into<Hertz>' -n

src/sdmmc.rs:502:                pub fn init_card(&mut self, freq: impl Into<Hertz>) -> Result<(), Error> {
src/sai/pdm.rs:183:        T: Into<Hertz>;
src/sai/pdm.rs:201:                    T: Into<Hertz>,
src/sai/i2s.rs:357:        T: Into<Hertz>;
src/sai/i2s.rs:369:        T: Into<Hertz>;
src/sai/i2s.rs:388:                    T: Into<Hertz>,
src/sai/i2s.rs:411:                    T: Into<Hertz>,
src/i2c.rs:110:        F: Into<Hertz>;
src/i2c.rs:119:        F: Into<Hertz>;
src/i2c.rs:297:                    F: Into<Hertz>,
src/i2c.rs:548:                    F: Into<Hertz>
src/i2c.rs:570:                    F: Into<Hertz>
src/pwm.rs:979:        T: Into<Hertz>;
src/pwm.rs:1010:                T: Into<Hertz>,
src/pwm.rs:1205:                pub fn frequency<T: Into<Hertz>>(mut self, freq: T) -> Self {
src/qspi.rs:132:    pub fn new<T: Into<Hertz>>(freq: T) -> Self {
src/qspi.rs:211:impl<T: Into<Hertz>> From<T> for Config {
src/serial.rs:107:        pub fn baudrate(mut self, baudrate: impl Into<Hertz>) -> Self {
src/serial.rs:157:    impl<T: Into<Hertz>> From<T> for Config {
src/spi.rs:517:        T: Into<Hertz>,
src/spi.rs:528:        T: Into<Hertz>,
src/spi.rs:565:                        T: Into<Hertz>,
src/spi.rs:903:                     T: Into<Hertz>,
src/spi.rs:921:                     T: Into<Hertz>,
src/rcc/mco.rs:157:                    F: Into<Hertz>,
src/rcc/mco.rs:188:                    F: Into<Hertz>,
src/rcc/mod.rs:267:                F: Into<Hertz>,
src/rcc/mod.rs:284:                    F: Into<Hertz>,
src/rcc/mod.rs:315:        F: Into<Hertz>,
src/rcc/mod.rs:331:        F: Into<Hertz>,
src/rcc/mod.rs:340:        F: Into<Hertz>,
src/rcc/mod.rs:349:        F: Into<Hertz>,
src/rcc/mod.rs:361:        F: Into<Hertz>,
src/timer.rs:163:        T: Into<Hertz>;
src/timer.rs:177:        T: Into<Hertz>;
src/timer.rs:214:                    T: Into<Hertz>,
src/timer.rs:254:                    T: Into<Hertz>,
src/timer.rs:265:                    T: Into<Hertz>,
src/timer.rs:311:                    T: Into<Hertz>,
src/timer.rs:378:                    T: Into<Hertz>,
src/timer.rs:528:                    T: Into<Hertz>,
src/timer.rs:563:                        T: Into<Hertz>,
src/timer.rs:572:                    T: Into<Hertz>,
src/timer.rs:584:                    T: Into<Hertz>,
src/timer.rs:632:                    T: Into<Hertz>,
src/timer.rs:687:                    T: Into<Hertz>,

Would be nice if they could be resolved as well.

This is more informative thatn a request ;) So don't feel pushed. When I have time in a few weeks, I'll try to resolve these, but feel free to do it in the meantime.

hannobraun commented 3 years ago

@hellow554

When I have time in a few weeks, I'll try to resolve these, but feel free to do it in the meantime.

I don't have the time to spare right now, but I'd be happy to review/merge any pull requests that address is, so feel free to look into it!