stm32f7xx-hal icon indicating copy to clipboard operation
stm32f7xx-hal copied to clipboard

Clocks can no longer be specified in MHz

Open rfuest opened this issue 4 years ago • 3 comments

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.

rfuest avatar Jul 16 '21 18:07 rfuest

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!

hannobraun avatar Jul 17 '21 09:07 hannobraun

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.

hellow554 avatar Sep 22 '21 13:09 hellow554

@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!

hannobraun avatar Sep 23 '21 09:09 hannobraun