Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clocks can no longer be specified in MHz #136

Open
rfuest opened this issue Jul 16, 2021 · 3 comments
Open

Clocks can no longer be specified in MHz #136

rfuest opened this issue Jul 16, 2021 · 3 comments

Comments

@rfuest
Copy link
Contributor

rfuest commented Jul 16, 2021

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants