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

pkg/wamr: add riscv_esp32 architecture #18338

Closed
wants to merge 1 commit into from
Closed

pkg/wamr: add riscv_esp32 architecture #18338

wants to merge 1 commit into from

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Jul 20, 2022 •
edited
Loading

Contribution description

This PR is a split-off from PR #17844, which extends the makefile of pkg/wamr by ESP32 RISC-V architecture.

Testing procedure

Here we have a Hen-and-egg problem. It is not possible to cover this PR by compiling it in CI without having a board definition with an ESP32-C3. Conversely, the ESP32-C3 port that requires this PR cannot be compiled in CI without it.

Therefore a green CI with the label CI: skip compile test must be sufficient for the moment. If it doesn't work, the problem will pop up as soon as ESP32-C3 support is provided. And of course, I have tested it together with the ESP32-C3 support.

Issues/PRs references

Split-off from PR #17844

@github-actions github-actions bot added the Area: pkg Area: External package ports label Jul 20, 2022
@gschorcht gschorcht requested a review from benpicco July 20, 2022 07:51
@gschorcht gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs labels Jul 20, 2022
@benpicco benpicco requested a review from kfessel July 20, 2022 08:50
@kfessel
Copy link
Contributor

kfessel commented Jul 20, 2022

Seems like I should have used CPU_CORE but even the somthing like this had to be added for this (at least right now https://github.com/RIOT-OS/RIOT/pull/17844/files#r925545350

@gschorcht gschorcht mentioned this pull request Jul 20, 2022
Merged
benpicco
benpicco reviewed Jul 20, 2022
View reviewed changes
pkg/wamr/Makefile
@@ -47,6 +47,8 @@ else ifeq ($(CPU_ARCH),xtensa)
WAMR_BUILD_TARGET = XTENSA
else ifeq ($(CPU_ARCH),rv32)
WAMR_BUILD_TARGET = RISCV32
else ifeq ($(CPU_ARCH),riscv_esp32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unhappy that there is a special esp32 riscv CPU_ARCH now.
Why can't this be rv32?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unhappy that there is a special esp32 riscv CPU_ARCH now.

I know.

Why can't this be rv32?

We discussed it already in #17841 (review) and #17841 (comment). When I ported RIOT to ESP32-C3, I definitely tried to reuse common parts from cpu/riscv_common and implement only some specific parts for ESP32-C3. But since using a cpu/* directory means all or nothing and some parts in cpu/riscv_common are not compatible with ESP-IDF and ESP32-C3 SoC, a specific architecture had to be defined with its own implementation in PR #18260. I am not happy with it either, but there was no other option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also "Background" in #18260 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/RIOT-OS/RIOT/pull/17844/files#r925545350

Copy link
Contributor Author

@gschorcht gschorcht Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this be rv32?

@benpicco If I understand you right, you suggest to use the following for RISC-V based ESP32x SoC?

CPU_ARCH = rv32
CPU_CORE = rv32imc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's possible it would certainly make the most sense from a consistency point of view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work for ESP32-C3 with these changes so that we could close this PR. I have to check Kconfig before.

@gschorcht
Copy link
Contributor Author

CPU_ARCH is changed in PR #18341 from riscv_esp32 to rv32 so that this change isn't needed any longer.

@gschorcht gschorcht closed this Jul 20, 2022
@gschorcht gschorcht deleted the pkg/wamr/add_riscv_esp32_architecture branch July 20, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco left review comments

@kfessel kfessel Awaiting requested review from kfessel

Assignees
No one assigned
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: skip compile test If set, CI server will run only non-compile jobs, but no compile jobs or their dependent jobs Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gschorcht @kfessel @benpicco