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

Don't enable default features for esp*-hal #303

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Conversation

bugadani
Copy link
Contributor

No description provided.

@bugadani bugadani force-pushed the defaults branch 2 times, most recently from 3577237 to d9e9358 Compare October 24, 2023 11:00
@bugadani bugadani marked this pull request as draft October 24, 2023 11:00
@bugadani

This comment was marked as outdated.

@bugadani bugadani force-pushed the defaults branch 2 times, most recently from 997211c to d8842b4 Compare October 24, 2023 11:12
@bugadani bugadani marked this pull request as ready for review October 24, 2023 11:24
@bugadani bugadani force-pushed the defaults branch 3 times, most recently from ed1b738 to 52b2095 Compare November 1, 2023 22:23
@bugadani
Copy link
Contributor Author

bugadani commented Nov 1, 2023

Oookay, an unexpected side-effect of the test unification is that I'm not sure if we can do this. While esp-wifi ideally shouldn't enable default HAL features, we don't want to set every default feature for every example and CI test either. Unsure how to progress.

@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 2, 2023

... we don't want to set every default feature for every example and CI test either ..

Unfortunately, I cannot think of another solution 🤔

@MabezDev
Copy link
Member

MabezDev commented Nov 2, 2023

How about a hidden feature that enables the esp32XX-hal default feature, which we can use for examples/ci? E.g

# This feature is not stable, for internal use only
_default = ["esp32-hal/default", "esp32s3-hal/default"]

@MabezDev
Copy link
Member

MabezDev commented Nov 2, 2023

Actually, it doesn't even need to be a feature. We can just enable default feature of the hal

-esp32c6 = "run --features esp32c6 --target riscv32imac-unknown-none-elf --features esp32c6-hal/embassy-time-timg0"
+esp32c6 = "run --features esp32c6 --target riscv32imac-unknown-none-elf --features esp32c6-hal/embassy-time-timg0 esp32c6-hal/default"

@bugadani bugadani force-pushed the defaults branch 3 times, most recently from 967b61a to f33df47 Compare November 3, 2023 06:56
@bugadani
Copy link
Contributor Author

bugadani commented Nov 9, 2023

IMO this PR isn't blocked on esp-hal any more, whatever solution will be picked there will not affect esp-wifi. Thanks to mabez for the default feature hint, that was something new for me.

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

@bjoernQ bjoernQ merged commit 5a7b8f0 into esp-rs:main Nov 10, 2023
7 checks passed
@bugadani bugadani deleted the defaults branch November 10, 2023 12:51
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