-
Notifications
You must be signed in to change notification settings - Fork 118
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
Updated ESP32-C6 valid targets #402
Conversation
Removed uncompatible targets of ESP32-C6 and added IMAC ones
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.
Could you run rustfmt to fix the CI, and also revert the version change, we haven't actually released rc4 yet. Other than that, looks good to me!
Reverted to rc.4
"riscv32imc-esp-espidf", | ||
"riscv32imc-unknown-none-elf", | ||
] | ||
&["riscv32imac-unknown-none-elf", "riscv32imac-esp-espidf"] |
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 riscv32imc
really "invalid"? I'm not sure why you'd want to build without the atomic extension but it's perfectly valid to do so. I don't really understand the motivation behind removing it.
If we are removing it, then the ESP32-H2 needs to be updated as well.
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.
Not invalid, but almost certainly a mistake. Tbh, I've never really liked that we hard error here on a target mismatch. I think it should be a warning instead, but we can discuss that another time :D.
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.
It wasn't a mistake, it was to be consistent with the other RISC-V chips. The C2 and C3 both have riscv32imac
listed as supported, which they do not technically support either. But I guess it doesn't really matter that much.
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'm not saying using riscv32imac
for C2/C3 is a mistake, I'm saying using riscv32imc
on a target that has native support for atomics would be a mistake, there isn't a reason not to use that target for the C6.
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.
For IDF, the std
launches a huge linker error trying to find the functions for "A" for std
unsuccessfully. So there, it is "invalid". I'll try imc
for no_std
, but I believe It'll fail for the same reason but core
this time instead of std
.
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 tested the riscv32imc
on the C6. If you don't try to use core::sync::atomic
, it compiles.
But, and this is my opinion: This is a new release of hardware, where among other things, the nice thing is the atomic support and no backward compatibility breaks by changing the target to imac
. So, if you prefer to support riscv32imac-unknown-none-elf
and riscv32imc-unknown-none-elf
, let me know to add it too.
On the other hand, if you're already deciding to migrate to this hardware, and that "forces" you to change to imac
the only work you have to do is move out from polyfill, and most of the embedded libs out there already have automatic detection and start using native support if it's available.
But for sure, if you try to use riscv32imc-esp-espidf
, it doesn't compile. I opened an issue with the whole detail
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.
Sorry, for whatever reason GitHub doesn't let you control which lines are shown in the preview (kind of defeats the purpose, doesn't it?).
I was referring only to the bare-metal targets; I don't use the IDF stuff and frankly don't really care about it, you can do whatever you want with those targets.
Regardless, my opinion seems to be controversial for whatever reason, so as I requested earlier can we just update the H2 as well please so we can get this wrapped up?
I installed this branch locally but I'm unable to successfully flash. I copied the target config riscv32imac-esp-espidf.json to the root and run
|
Yeah this is the downside of it being an error when the target mismatches (see here: #402 (comment)) I often run custom targets via json files and always run into this. The work around is to use |
Since the requested changes have not yet been made and I'm making a push to get a release out, I've opened #424 which supersedes this PR. Thanks for you contribution regardless, though! |
You're welcome, but sorry to ask, what changes where requested? Formating was done and I'm not aware of any other ones. In the end now it doesn't matter anymore but I'd like to know for the next time :) Anyhow, sorry that I didn't make them and you have to do it again! |
The ESP32-C6 was updated but the ESP32-H2 never was. Sorry, I guess I wasn't very clear about that in hindsight. No big deal, I'm just impatient 😁 |
Fair enough! Truth being told, I didn't understand I should add that and on the other hand, I wasn't capable to test it. I don't have an H2 in my hands (yet) so it's ok. Thanks @jessebraham ! |
Removed incompatible targets of ESP32-C6 and added only IMAC ones
Updated to RC5