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.

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

Compile for esp32(s2,c3,..) #20

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

Remmirad
Copy link

When trying to compile RIOT with Rust for esp32(s2,c3) targets i encountered some problems:

(The first two problems are unrelated to this crate but are mentioned since they should show up when testing this.)

  • C2Rust compiled with xtensa compatible llvm needed (esp32,s2)
  • C2Rust problematic macro translation Problematic macro translation with --translate-const-macros immunant/c2rust#803
    causing non compiling code (esp32, s2, c3)
  • Some small things here in rust-riot-sys need to be considered:
    • Duplicate definitions are created (activate keep-extern-types to fix)
    • When using e.g. netdev_default C++ files are pulled in and "pollute" compile-commands.json
    • The esps use stdatomic.h when module netdev_default is in

To circumvent the C2Rust macro problem the fork linked in the issue might be used or for simple test-cases the option --translate-const-macros can be removed from build.rs.

For the other 3 things concerning this crate i added ideas how to address them or make them more easily to find, but they definitely need further discussion:

  • Duplicate definition of __lock occurs because it is transpiled differently by c2rust and thus doesn't get removed. I only added a warning for this to alert the user that something didn't go as expected
  • C++ files: added placeholder just kicking clang++ out not sure how to address this correctly
    • The problem here is that C++ files don't have the std=c11 flag and thus it gets kicked out by the consent strategy
  • The atomic case is explained in riot-c2rust.h i thought it might be nice to make this controllable via a feature.

Joel Aschmann added 3 commits February 21, 2023 13:06
On some boards like esp32 c++ files are in compile_commands.json. This prevents them from being considered.
allows to opt in the dummy atomic definitions from riot-c2rust.h by
preventing them to be undefined. esp32 wifi drivers need this for example.
@chrysn
Copy link
Member

chrysn commented Feb 22, 2023

Thanks for starting this.

Just for me to get the context right: -S2 and -C3 are the RISC-V variants of the ESP processors? Why do they need an adjusted LLVM toolchain, but more importantly, is there any guide I can follow to get that? Is there a rough schedule when they would be upstreamed? Or can they be installed in parallel? (To have this all testable, we'd eventually need to get that toolchain into riotdocker).

@Remmirad
Copy link
Author

The -S2 is still on espressifs lx7 architecture. But yes c3 is risc-v and does not need the special llvm. We somehow got the llvm thing to work, but i did not do that part myself, if you are interested in the details i can ask the person who did that stuff to join us here.

@chrysn
Copy link
Member

chrysn commented Feb 22, 2023

In this case I'd like to address this in slices, doing -C3 first because it's what should be testable on the images we have now.

Does the C2Rust macro translation issue occur there as well, or is that limited to the -S2 case?

(Depending on how fast you want this to progress, it might be good to have your colleague CC'd as well, or ask them right away if they have any suggestions on what to add to the riotdocker repository -- when opening issues related to Rust there, please tag me).

@Remmirad
Copy link
Author

The macro issue occurred on both -S2 and -C3. Also all rust-riot-sys specific things seemed to affect both -S2 and -C3 equally. So hopefully "fixing"(Not so much to be fixed here) -C3 might fix -S2 issues related to this crate simultaneously.

@chrysn
Copy link
Member

chrysn commented Feb 24, 2023

Sounds good, I'll look into what's open.

On a side note, if we're lucky with timing, and the patches keep coming fast, the issue with needing a forked LLVM seems like it's about to be resolved.

@chrysn chrysn mentioned this pull request Mar 2, 2023
@chrysn
Copy link
Member

chrysn commented Mar 4, 2023

Have you collected the changes you had to make on the RIOT side while testing this?

I've started, and wound up with

diff --git a/.cargo/config.toml b/.cargo/config.toml
index 03fd973948..a50f522beb 100644
--- a/.cargo/config.toml
+++ b/.cargo/config.toml
@@ -8 +8,2 @@
-riot-sys = { git = "https://github.com/RIOT-OS/rust-riot-sys" }
+#riot-sys = { git = "https://github.com/RIOT-OS/rust-riot-sys" }
+riot-sys = { git = "https://github.com/Remmirad/rust-riot-sys", branch = "compile-esp32" }
diff --git a/cpu/esp_common/Makefile.features b/cpu/esp_common/Makefile.features
index d36e47dcee..83767e7a3c 100644
--- a/cpu/esp_common/Makefile.features
+++ b/cpu/esp_common/Makefile.features
@@ -30,0 +31 @@ ifeq (rv32,$(CPU_ARCH))
+  FEATURES_PROVIDED += rust_target
diff --git a/cpu/esp_common/Makefile.include b/cpu/esp_common/Makefile.include
index 4b4d3e3de3..c8965468c2 100644
--- a/cpu/esp_common/Makefile.include
+++ b/cpu/esp_common/Makefile.include
@@ -40,0 +41,7 @@ endif
+ifeq (rv32,$(CPU_ARCH))
+  RUST_TARGET = riscv32i-unknown-none-elf
+  # we don't have it in the riotbuild images yet
+  CARGO_OPTIONS += -Zbuild-std=core
+  CARGO_CHANNEL = $(CARGO_CHANNEL_NIGHTLY)
+endif
+

but it's still not going through when running

$ make -C examples/rust-hello-world all BOARD=hip-badge BUILD_IN_DOCKER=1 DOCKER=podman

(although maybe I'm seeing just the issues you pointed out to be open, didn't check yet)

@Remmirad
Copy link
Author

Remmirad commented Mar 6, 2023

Oh yes thanks for the reminder. We did something there, but did not yet open a PR for RIOT I think. (But we did not place the cargo flags here and instead put them in the Makefile, might be a nice idea to put them here as you suggested)

What we're currently using looks like this in RIOT/cpu/esp32/Makefile.features

Note: we had to use riscv32imac-unknown-none-elf since we needed the atomics, if I remember correctly

ifeq (esp32,$(CPU_FAM))
  CPU_ARCH = xtensa
  CPU_CORE = xtensa-lx6
  RUST_TARGET = xtensa-esp32-none-elf
else ifneq (,$(filter esp32c3,$(CPU_FAM)))
  CPU_ARCH = rv32
  CPU_CORE = rv32imc
  RUST_TARGET = riscv32imac-unknown-none-elf
else ifneq (,$(filter esp32s2,$(CPU_FAM)))
  CPU_ARCH = xtensa
  CPU_CORE = xtensa-lx7
  RUST_TARGET = xtensa-esp32s2-none-elf
else ifneq (,$(filter esp32s3,$(CPU_FAM)))
  CPU_ARCH = xtensa
  CPU_CORE = xtensa-lx7
  RUST_TARGET = xtensa-esp32s3-none-elf
else
  $(error Unkwnown ESP32x SoC variant (family))
endif

ifneq (,$(RUST_TARGET))
  FEATURES_PROVIDED += rust_target
endif

@chrysn
Copy link
Member

chrysn commented Mar 6, 2023

Note: we had to use riscv32imac-unknown-none-elf since we needed the atomics, if I remember correctly

Just setting up the compiler for that target won't make the machine process them -- the ESP32-C3 is advertised to be a RV32IMC, and hitting atomic instruction will probably just trigger a fault. I know that some of the (esp. new) Rust code does rely on atomics, but that'll need to be switched to the polyfill (which will effectively just disable interrupts, run stuff non-atomically and enable interrupts again) to work on anything that doesn't have atomics (also necessary for the smaller ARMs).

@Remmirad
Copy link
Author

Remmirad commented Mar 7, 2023

Hm you're right there, interesting that it still worked in our tests, but should definitely be changed. If I remember correctly we had the problem that something rust-riot-sys complained about not having atomics available.

@chrysn
Copy link
Member

chrysn commented Mar 7, 2023

It's not too surprising: I've assumed the presence of atomics pretty generously, and rust-riot-sys processes a lot of code that's not necessarily ever run. There are two possible reasons why things could have worked (the first one is likely, the second is pure speculation):

  • The atomics were just not hit -- and code that's never executed doesn't create faults.
  • Less likely, but possible: There is some atomic support in the chips, but it's so broken / unreliable the vendor could not afford to advertise it as RV32IMAC, and chose to advertise it as RF32IMC instead.

@Remmirad
Copy link
Author

Remmirad commented Mar 7, 2023

I would definitely go with the first one too :)

O.k. so to make this right one would use atomic_polyfill and provide it with a critical-section implementation via. RIOTs interfaces (e.g. disable interrupts)?

And where would one put these things? Inside this crate and enable disable critical-section implementations based on the platform? (Maybe not necessary when done via RIOT)

@Flole998
Copy link

Flole998 commented Mar 9, 2023

I'd like to point out that the heapless-library which we are using is considering moving away from atomic-polyfill in favor for portable-atomic.

See rust-embedded/heapless#328 for the reasons, so maybe portable-atomic might be better suited for this project aswell.

@Remmirad
Copy link
Author

In c7df4f1 I added a first draft how to use these portable atomics. It provides a critical section implementation that uses RIOTs interrupt interface to just disable and enable interrupts in a critical section. (Thus only working for single core cpus atm)
Maybe we even want to introduce this in a separate PR because this is not wholly esp related...
It at least solves the atomic problem occurring when compiling for esp32c3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants