-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add a switch to enable unstable esp-hal features #104
Conversation
|
6a82959
to
a788161
Compare
name: unstable-hal | ||
display_name: Enable unstable HAL features. | ||
help: esp-hal contains many unstable features that are not yet ready for general use. This configuration enables using them. | ||
|
||
- !Option | ||
name: alloc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should esp-alloc require unstable? (I guess yes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests passed, so why?
I think the separate "unstable-dependencies" we had is unnecessary noise, people can decide if they want a dependency or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the separate "unstable-dependencies" we had is unnecessary noise
I agree just the linked issue still says otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want me to call out for each 0.x dependency that they are unstable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't want that at all- but
Crates like esp-println, esp-backtrace whilst powerful, I don't think are ready for stabilization yet and should be opt in.
reads to me like that was the idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right, I planned to make them opt-in in an upcoming PR. Updated the description to not close that issue yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - if @MabezDev is fine with it, too I guess it's ready to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Probably closes #84