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

Decouple default features from esp-hal-common #867

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Oct 24, 2023

There are more assumptions in this PR than I'd like but the point is the same: esp-wifi will no longer enable default features. To prevent depending on esp-hal-common, I'm introducing a new feature in place of esp-hal-common/rv-zero-rtc-bss to clear up esp-wifi examples.

cc esp-rs/esp-wifi-sys#303 that uncovered the need for this.

@jessebraham
Copy link
Member

Does it really make sense to add the esp-idf-boot feature if we're planning on removing the direct-boot feature? What do we actually gain from this?

@bugadani
Copy link
Contributor Author

bugadani commented Oct 24, 2023

The feature name doesn't matter much, but there's also mcu-boot for a third option :) The end goal is to just have a feature in each hal crate that users can use instead of adding a mostly unnecessary dependency to the common crate.

As to my motivation: There are some users who need default-features = false to enable the 26 MHz crytal option.

PS: I do admit I don't actually know exactly when rv-zero-rtc-bs is necessary and where it isn't. It's also unclear to me if direct boot will be removed or not, there seem to be some people using it for one reason or another.

@jessebraham
Copy link
Member

It's also unclear to me if direct boot will be removed or not, there seem to be some people using it for one reason or another.

We've had an RFC and have mentioned it in the last two community meetings and there has been basically zero feedback. As of now the plan is to remove it.

@bugadani bugadani force-pushed the rv-init branch 2 times, most recently from ca1cfd1 to fada16a Compare October 24, 2023 12:28
@bugadani
Copy link
Contributor Author

bugadani commented Oct 24, 2023

I could just enable rv-zero-rtc-bs on esp-hal-common (same as features = ["esp32c3"] e.g.) and not add any new features, I just don't know if there are some cases where someone would want to have it disabled. At any rate, this is a draft for a reason, I'd love to hear opinions.

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 25, 2023

It's also unclear to me if direct boot will be removed or not, there seem to be some people using it for one reason or another.

We've had an RFC and have mentioned it in the last two community meetings and there has been basically zero feedback. As of now the plan is to remove it.

At some point we probably need a decision since it's also broken for some targets (at least ESP32-S3). On the other hand, someone got esp-wifi working with direct-boot on ESP32-C3 ... that makes it a lot more useful now

@bjoernQ
Copy link
Contributor

bjoernQ commented Oct 25, 2023

I guess it's fine to make the features individually available.
I don't think most users would need that granularity, however

@jessebraham
Copy link
Member

I think after a rebase this is probably fine to merge.

@bugadani bugadani force-pushed the rv-init branch 2 times, most recently from 998fd33 to 19bc293 Compare November 15, 2023 16:11
@bugadani bugadani marked this pull request as ready for review November 15, 2023 16:12
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jessebraham jessebraham added this pull request to the merge queue Nov 16, 2023
Merged via the queue into esp-rs:main with commit 6814822 Nov 16, 2023
17 checks passed
@bugadani bugadani deleted the rv-init branch November 16, 2023 14:15
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