Skip to content

fugit #177

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

Merged
merged 1 commit into from
Mar 6, 2022
Merged

fugit #177

merged 1 commit into from
Mar 6, 2022

Conversation

burrbull
Copy link
Member

@burrbull burrbull commented Mar 3, 2022

No description provided.

@burrbull burrbull force-pushed the fugit branch 2 times, most recently from 707ca24 to f827ed2 Compare March 3, 2022 08:03
@burrbull burrbull closed this Mar 3, 2022
@burrbull burrbull reopened this Mar 3, 2022
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@burrbull
Copy link
Member Author

burrbull commented Mar 3, 2022

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

eldruin commented Mar 3, 2022

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.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the error in CI was just a spurious download issue.
Thank you @burrbull !

@eldruin eldruin merged commit 4a55dd4 into stm32-rs:master Mar 6, 2022
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

Successfully merging this pull request may close these issues.

2 participants