-
Notifications
You must be signed in to change notification settings - Fork 240
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 rt-riscv
and rt-xtensa
features to esp-hal-common
#1057
Conversation
…enabling/disable runtime support
…t default features
@@ -48,7 +48,8 @@ embassy-sync = { version = "0.5.0", optional = true } | |||
embassy-time = { version = "0.2.0", optional = true } | |||
|
|||
# RISC-V | |||
esp-riscv-rt = { version = "0.6.0", optional = true, path = "../esp-riscv-rt" } | |||
riscv = { version = "0.10.1", optional = true } |
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.
is this just to make it look more aligned with Xtensa? Before we just used the re-export from esp-riscv-rt
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 critical section implementation uses riscv
, which wasn't available if the rt-riscv
feature is not enabled. So I've simply included the riscv
dependency directly rather than using the re-exported version from esp-riscv-rt
.
I also see no way to unify the |
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
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, thanks for looking into this!
Not an ideal solution, but it's not immediately obvious how we would unify these features to me. Happy to make any changes if anybody has a better solution. This should be enough to close #579 at least, though.
Once this gets merged I can rebase #1049 on top of it and wrap that up, too.