Skip to content

embassy-nrf: migrate to embedded-hal 1.0, embedded-hal-async #552

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 9 commits into from
Jan 14, 2022
Merged

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Dec 19, 2021

No description provided.

Copy link
Contributor

@huntc huntc left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -47,11 +47,14 @@ embassy = { version = "0.1.0", path = "../embassy" }
embassy-macros = { version = "0.1.0", path = "../embassy-macros", features = ["nrf"]}
embassy-hal-common = {version = "0.1.0", path = "../embassy-hal-common" }

embedded-hal-02 = { package = "embedded-hal", version = "0.2.6" }
embedded-hal-1 = { package = "embedded-hal", version = "1.0.0-alpha.6", git = "https://github.com/embassy-rs/embedded-hal", branch = "async" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this confusing having to depend on two flavours.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean 0.2 and 1.0? This is so the HAL can be used with drivers using either 0.2 or 1.0 traits.

The alternative would be to completely drop 0.2 and tell users to use embedded-hal-compat if they want to use a 0.2 driver.

I prefer directly supporting both however, because 0.2 is massively widespread in the ecosystem, so it'll take years for everything to upgrade to 1.0, so this will be a major usability issue until then.

Copy link
Contributor

Choose a reason for hiding this comment

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

At what point do you think we will let get of 0.2 support? Should we at least mark it as being deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea about the timeline, but I guess years.

Not sure if you can mark a trait impl as deprecated, but even if you could it'd be ineffective: the user getting the warning can do nothing about it, it's the driver author that should do the work of updating it.

Copy link
Member

Choose a reason for hiding this comment

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

Having gone through the embedded-hal-compat annoyance, I think the approach in this PR is much more helpful to end users 🚀

}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Such a pity to lose this fidelity of errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's how the EH ErrorKind is defined.

It makes sense though: These errors are very nrf-specific. Users of embassy-nrf can still check our Error enum, Hal-independent users via the traits can only check the kind, but it will have no idea about the nrf-specific errors, so there's nothing it can do anyway.

@Dirbaio Dirbaio force-pushed the eh1 branch 5 times, most recently from bb63909 to a6f5299 Compare January 13, 2022 22:31
@Dirbaio Dirbaio changed the title WIP: migrate to embedded-hal 1.0, embedded-hal-async embassy-nrf: migrate to embedded-hal 1.0, embedded-hal-async Jan 14, 2022
@Dirbaio Dirbaio marked this pull request as ready for review January 14, 2022 16:29
@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 14, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 14, 2022

Build succeeded:

@bors bors bot merged commit 2310003 into master Jan 14, 2022
@huntc
Copy link
Contributor

huntc commented Jan 14, 2022

Seems like the only majority of changes for end-users here is the removal of some trait imports in their code. Nice!

bors bot added a commit that referenced this pull request Jan 19, 2022
581: stm32: expose all functionality as inherent methods. r=Dirbaio a=Dirbaio

This is the previous step to implementing both the embedded-hal 0.2 and embedded-hal 1.0 + embedded-hal-async traits.

The equivalent in nrf was done in #552 

- Removes need for `unwrap` in gpio.
- Removes need for `use embedded_hal::whatever` in all cases.

Co-authored-by: Dario Nieuwenhuis <[email protected]>
@Dirbaio Dirbaio mentioned this pull request Feb 11, 2022
bors bot added a commit that referenced this pull request Feb 12, 2022
613: Rust stable support r=Dirbaio a=Dirbaio

This PR adds (limited) stable Rust support!

The drawbacks are: 

- No `#[embassy::task]`, `#[embassy::main]`. (requires `type_alias_impl_trait`). You have to manually allocate the tasks somewhere they'll live forever. See [example](https://github.com/embassy-rs/embassy/blob/master/examples/nrf/src/bin/raw_spawn.rs)
- No async trait impls (requires GATs). Note that the full API surface of HALs is still available through inherent methods: #552 #581 
- Some stuff is not constructible in const (requires `const_fn_trait_bound`), although there's an (ugly) workaround for the generic `Mutex`.

So it's not that bad in the end, it's fully usable for shipping production-ready firmwares. We'll still recommend nightly as the default, until GATs and `type_alias_impl_trait` are stable.

Co-authored-by: Dario Nieuwenhuis <[email protected]>
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