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

Add initial support for embedded-hal version 1.0 #715

Closed
wants to merge 1 commit into from

Conversation

tgross35
Copy link
Contributor

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
  • All new or modified code is well documented, especially public items
  • No new warnings or clippy suggestions have been introduced - CI will deny clippy warnings by default! You may #[allow] certain lints where reasonable, but ideally justify those with a short comment.

If Adding a new Board

  • Board CI added to crates.json
  • Board is properly following "Tier 2" conventions, unless otherwise decided to be "Tier 1"

If Adding a new cargo feature to the HAL

  • Feature is added to the test matrix for applicable boards / PACs in crates.json

This adds a dependency and feature for embedded-hal v1.0 and a basic
implementation for the `DelayNs` trait.
@tgross35
Copy link
Contributor Author

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);
Copy link
Contributor Author

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

Comment on lines +84 to 85
// TODO: why does this use nanos?
self.timer.start((us / count).nanos());
Copy link
Contributor Author

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?

@jbeaurivage
Copy link
Contributor

jbeaurivage commented Jan 13, 2024

Thanks @tgross35! I'll create an embedded-hal-1 branch in the atsamd-hal repo, so that we can split the effort in multiple PR, and merge that once we're ready. Can you please change this PR to use that as its base branch?

I'll have to review your code bit more in detail concerning the timer stuff, but I don't think we should hide embedded-hal v1.0 behind a feature. Implementations for the 0.2 and 1.0 versions can easily coexist for one release, after which I plan on removing 0.2 if everyone else agrees. It think it's also a good opportunity to remove the unproven feature, which imp doesn't really serve a purpose at this point. Added advantage is that CI will check the new ehal v1.0 additions without further configuration.

As far as naming, I think I would reexport embedded-hal v1.0 as ehal, and v0.2 as ehal_0_2. To me it makes it a bit clearer that 1.0 is the "default". What do you think?

@jbeaurivage jbeaurivage changed the base branch from master to embedded-hal-1 January 13, 2024 16:28
@tgross35
Copy link
Contributor Author

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, CountDown, Periodic, and Pwm all provide pretty important functionality.

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 eh1::DelayNs makes use of CountDown). What are your thoughts?

@jbeaurivage
Copy link
Contributor

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.

@jbeaurivage jbeaurivage mentioned this pull request Apr 29, 2024
13 tasks
@sajattack
Copy link
Member

superceded by #723

@sajattack sajattack closed this May 1, 2024
@tgross35 tgross35 deleted the embedded-hal-1 branch May 1, 2024 06:45
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.

3 participants