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.

Sign up for GitHub

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

Jump to bottom

Updated ESP32-C6 valid targets #402

Closed
wants to merge 2 commits into from
Closed

Updated ESP32-C6 valid targets #402

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a5bbf8
Changed to RC5
Nassiel May 8, 2023
11dc672
Passed rustfmt
Nassiel May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions espflash/src/targets/esp32c6.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,6 @@ impl Target for Esp32c6 {
}

fn supported_build_targets(&self) -> &[&str] {
&[
"riscv32imac-unknown-none-elf",
"riscv32imc-esp-espidf",
"riscv32imc-unknown-none-elf",
]
&["riscv32imac-unknown-none-elf", "riscv32imac-esp-espidf"]
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

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.

Copy link
Author

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

Copy link
Member

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?

}
}