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

Remove most trait implementation features from esp-hal #2047

Closed

Conversation

jessebraham
Copy link
Member

This began as a quest to eliminate the async feature, however we came to the conclusion that none of these features are really necessary, and they just needlessly complicate things. These packages largely contain a couple traits and/or types only, so build times should not be affected significantly.

If anybody thinks any of these features should remain I'm willing to hear your arguments, but as I said I really don't think there is any need.

Closes #2007

@jessebraham jessebraham added the skip-changelog No changelog modification needed label Aug 30, 2024
@jessebraham
Copy link
Member Author

The changes to esp-wifi do not warrant a CHANGELOG.md entry IMO, so I have added skip-changelog; I have still recorded the changes for esp-hal.

embassy-futures = "0.1.1"
embassy-sync = "0.6.0"
embassy-usb-driver = "0.1.0"
embassy-usb-synopsys-otg = "0.1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think embassy-usb* should still be gated, behind usb-otg.

@Dominaezzz
Copy link
Collaborator

I'm glad async going away as it was annoying to deal with within the hal, but the dependency based features on the other hand I'm not so sure.

I have two arguments:

  1. This conversation, sometimes the overloads from these traits can get in your way. I can also imagine conflicts between e-hal v1 and v0.2 .
  2. It doesn't happen often but some dependencies can have issues building and it's nice to be able to not include it. Particularly pre-1.0 libraries. I'd rather not have my hands tied by esp-hal hard including a problematic (to me) version of a package.

@bugadani
Copy link
Contributor

My suggestion is to roll with this and deal with the fallout later, if there is any. embassy is usually a pretty good example to follow and they don't have any issues with similarly unconditional implementations.

Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

Thanks!

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 2, 2024

Reminder to not merge before we released #2004 is released

@jessebraham
Copy link
Member Author

jessebraham commented Sep 3, 2024

Welp just nuked this branch by accident, oops. Will fix at some point and either re-open this or open a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove or enable the async feature
5 participants