-
Notifications
You must be signed in to change notification settings - Fork 36
MIsc changes #50
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
base: main
Are you sure you want to change the base?
MIsc changes #50
Conversation
#|esp-bootloader-esp-idf ={ git = "https://github.com/esp-rs/esp-hal.git" } | ||
#|esp-hal = { git = "https://github.com/esp-rs/esp-hal.git" } | ||
#|esp-hal-embassy = { git = "https://github.com/esp-rs/esp-hal.git" } | ||
#|esp-println = { git = "https://github.com/esp-rs/esp-hal.git" } |
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.
This helps people override with the latest main
when they want. Useful, at least as long as esp-hal
APIs are in flux.
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.
Seems the patch needs to be specified "at workspace root". That's not ideal, since the development needs and spikes for the two crates are likely very unrelated.
}; | ||
|
||
esp_bootloader_esp_idf::esp_app_desc!(); | ||
|
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 this was accidentially omitted in hello_rgbw
.
channel = "stable" | ||
components = ["rust-src"] | ||
targets = ["riscv32imac-unknown-none-elf", "riscv32imac-unknown-none-elf"] | ||
targets = ["riscv32imac-unknown-none-elf", "riscv32imc-unknown-none-elf"] |
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 presume the intention has been to have both targets available? riscv32imc
is what ESP32-C3 uses.
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.
to be honest, I am not sure what this does for multiple targets. I don´t use it anymore and instead use the justfile which explicitly sets the targets. Mixed target repositories are tricky in Rust...
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.
Ok. Would there be any reason to keep the rust-toolchain*.toml
's around?
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.
hmm, I think it might be good to get rid of them. For me, these solutions, where you support configs by expecting the user to copy a file to another, are a necessary evil, and I do not think those are required anymore here. The justfile
solution is a lot more explicit.
Note: It would be nice if #47 gets merged before this PR. It has some of the same changes, with different placement. I'm not trying to overrun it!
The changes don't affect the
lib
but do fix problems with examples.hello_rgb_async
needs deeper changes. I might append them to this, or present as a separate PR, later.