-
Notifications
You must be signed in to change notification settings - Fork 205
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
Add initial support for embedded-hal
version 1.0
#715
Conversation
This adds a dependency and feature for embedded-hal v1.0 and a basic implementation for the `DelayNs` trait.
00bf609
to
ae531a4
Compare
What is the best way to make sure this gets tested in CI? I am not sure if I should update the matrix for this. |
// Determine how many cycles we need to run for this delay, if any | ||
// Avoid timers that run longer than a second because for 48 MHz-based timers, | ||
// there is no valid divisor + cycle count greater than ~1.3s, so we'd panic. | ||
let mut loop_count: u32 = 1 + (ns / NUM_NS_IN_S); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our timers take a u32 nanosecond count, so this loop may be able to be removed. Need to verify
// TODO: why does this use nanos? | ||
self.timer.start((us / count).nanos()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must not understand fugit correctly, but it looks to me like this is treating microseconds as nanos with no conversion. What is going on here?
Thanks @tgross35! I'll create an embedded-hal-1 branch in the I'll have to review your code bit more in detail concerning the timer stuff, but I don't think we should hide As far as naming, I think I would reexport |
Hey, thanks for looking @jbeaurivage! The reason I started doing it in this additive way, rather than replacing, is that v1.0 has a list of removed traits that https://github.com/rust-embedded/embedded-hal/blob/master/docs/migrating-from-0.2-to-1.0.md#removed-traits. In particular, But maybe it is okay to switch to eh1.0 as the primary implementation while still keeping eh0.2 around to fill in the missing traits. In this case, I think it would be better to not gate either of them behind a feature because some of the eh1.0 relies on eh0.2 (e.g. in this PR |
Yeah, what I meant is for the transition release, we should provide implementations for both versions of ehal in parallel. Furthermore, I would suggest we take the actual logic out of the ehal v0.2 implementations, and instead put them in inherent methods on the peripherals (or on the ehal v1.0 traits of you prefer) . Implementing v0.2 traits then becomes a matter of forwarding to the correct method call. The idea being that 1.0 implementations should be totally independent from 0.2 impls (the reverse isn't necessary, however). Then, when we'll want to remove 0.2, we can simply delete everything related to 0.2 without affecting anything else. |
superceded by #723 |
Summary
This adds a dependency and feature for embedded-hal v1.0 and a basic implementation for the
DelayNs
trait. I think it is best to start with this small subset to make sure CI and everything are set up properly, then add other traits one at a time.Part of #332
Checklist
CHANGELOG.md
for the BSP or HAL updated#[allow]
certain lints where reasonable, but ideally justify those with a short comment.If Adding a new Board
crates.json
If Adding a new cargo
feature
to the HALcrates.json