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

Time types #24

Closed
MabezDev opened this issue Mar 3, 2022 · 7 comments · Fixed by #64
Closed

Time types #24

MabezDev opened this issue Mar 3, 2022 · 7 comments · Fixed by #64
Assignees
Milestone

Comments

@MabezDev
Copy link
Member

MabezDev commented Mar 3, 2022

How do we plan on handling time types in this new hal, seems currently we are just using a u32 for frequency etc (perhaps not a bad thing). Historically each hal defined its own time types, but nowadays there are a couple of external crates that will do the job.

@jessebraham
Copy link
Member

I didn't want to decide too early on this which is why I used u32 initially, but I guess we've made decent enough progress that we should discuss this!

Out of the two libraries mentioned I think I'd prefer fugit just because it's all done at compile-time, but I don't have too strong of feelings about this.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 3, 2022

I also already though about it and I think fugit looks quite good. Heard good things about it on the Embedded Rust matrix channel

@MabezDev
Copy link
Member Author

MabezDev commented Mar 3, 2022

I didn't want to decide too early on this which is why I used u32 initially, but I guess we've made decent enough progress that we should discuss this!

I opened this because I'm taking a stab at dropping in some of the drivers from esp32-hal (the sooner we kill this dinosaur the better :D) and my first issue is the lack of time types 😄.

I also already though about it and I think fugit looks quite good

I've heard good things about fugit too, seems a bit simpler to use.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 4, 2022

Would be nice to be able to retire esp32-hal

One bigger difference there is that esp32-hal does locking internally where necessary while here it's up to the caller (we probably need to document that somewhere). But it's how most HALs do it (while there are not that many multi-core MCUs besides some of the ESP32 family)

I have to admit that I never personally used fugit but worked on a HAL that uses embedded-time - worked fine but definitely fugit looks better

@MabezDev
Copy link
Member Author

MabezDev commented Mar 6, 2022

I spent a bit of time playing with fugit, it's very nice in some respects, but there might be a few issues.

  1. The biggest one, short functions which return fixed types korken89/fugit#26 (comment). Meaning that calling let hz = 1.MHz() will not compile because the NOM & DENOM const generics don't have defaults. Looks like this will be fixed in the next stable release of Rust though.
  2. The timer traits in e-hal 0.2 are crippled making it very difficult to use the fugit types (the trait has recently been removed for the 1.0 eh release:tm:)
  3. Lack of baud type, not really a big deal we can make one

I think it would be good reevaluate fugit 6 weeks time with the new rust release.

One bigger difference there is that esp32-hal does locking internally where necessary while here it's up to the caller (we probably need to document that somewhere). But it's how most HALs do it (while there are not that many multi-core MCUs besides some of the ESP32 family)

This is good point, I'll create another issue to discuss how we'll handle multicore MCU's.

@jessebraham
Copy link
Member

There have been a couple new comments in the linked issue, doesn't seem like the next release will fix things after all. There does seem to be a (slightly ugly) workaround, so maybe we can just use that for the time being instead?

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 14, 2022

I'd say let us try it - there are already HALs depending on fugit (so it's generally usable) and there are workarounds for the things that don't work nicely right now (plus there is hope that those things will get sorted out)

@jessebraham jessebraham self-assigned this Apr 4, 2022
@jessebraham jessebraham added this to the v0.1.0 milestone Apr 28, 2022
@ducktec ducktec mentioned this issue May 15, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants