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

Feature gate embedded-hal@0.2.x implementations, rename eh1 feature to embedded-hal #1273

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

jessebraham
Copy link
Member

Opening this as a draft, as I haven't really discussed this with anybody else yet 😁

Many of our APIs are currently quite tightly coupled to the embedded-hal packages, and I would like to begin working towards rectifying this.

This PR serves as a first step, and will allow us to build the HAL without any embedded-hal traits being implemented, which should rather quickly show us what we need to fix I'd imagine. I've left the embedded-hal-02 feature enabled by default, so this shouldn't really change anything from the user's perspective quite yet. I guess we will need to discuss our plans regarding these packages moving forward.

This leaves the TWAI driver in a bit of a weird spot for now, but see #1123 for more details (this issue should be prioritized sooner than later, I think).

Closes #849

@jessebraham
Copy link
Member Author

cc @MabezDev @bjoernQ

@jessebraham jessebraham changed the title Feature gate embedded-hal@0.2.x implementations, rename eh feature to embedded-hal Feature gate embedded-hal@0.2.x implementations, rename eh1 feature to embedded-hal Mar 12, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 13, 2024

Generally fine. Not sure about the embedded-hal-1 -> embedded-hal renaming. I guess it could be somewhat confusing that the embedded-hal feature changed its meaning from eh0.2 to eh1.0

Maybe having embedded-hal-02 and embedded-hal-1 would be more explicit

@jessebraham
Copy link
Member Author

jessebraham commented Mar 13, 2024

I don't think that the embedded-hal feature using the latest stable release of the package is especially confusing, personally. I also wanted to outright remove the old trait implementations, but there was push back there.

The old eh1 feature was originally intended to be temporary, as I naively thought that embedded-hal@1.0.0 would take <4 years to get out, so it was poorly named from the get-go.

I guess we can see what other people think.

@bjoernQ
Copy link
Contributor

bjoernQ commented Mar 13, 2024

I don't think that the embedded-hal feature using the latest stable release of the package is especially confusing, personally.

It's not confusing on its own - I just meant it's confusing that an existing feature changed what it is doing. Just tried to think about it from a user's perspective who wants to update their projects. It's also just a comment - not really an opinion

But I totally agree - let's hear other's voices

@jessebraham jessebraham marked this pull request as ready for review March 13, 2024 17:40
@burrbull
Copy link
Contributor

burrbull commented Mar 13, 2024

I recommend to move hal implementations to inner modules. See https://github.com/stm32-rs/stm32f4xx-hal/blob/master/src/gpio/hal_02.rs

Also I see you don't use inherent infallible set_high pin methods. Is there any reason?

@burrbull burrbull mentioned this pull request Mar 13, 2024
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM! I'm indifferent about the impls being in a module or as they are now - I'll let someone else decide that :).

@jessebraham jessebraham added this pull request to the merge queue Mar 14, 2024
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - I think having the impls in modules looks cleaner but probably something for a follow up PR (if we want that)

Merged via the queue into esp-rs:main with commit 3079d9c Mar 14, 2024
18 checks passed
@jessebraham jessebraham deleted the feature/embedded-hal branch March 14, 2024 15:41
yanshay pushed a commit to yanshay/esp-hal that referenced this pull request Mar 16, 2024
…re to `embedded-hal` (esp-rs#1273)

* Create an `embedded-hal-02` feature and gate implementations, rename `eh1` to `embedded-hal`

* Use native `Delay` APIs in examples where applicable

* Fix example imports, be explicit about about `embedded-hal-02` feature requirements

* Update `CHANGELOG.md`
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.

Something needs to be done about the prelude
4 participants