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

feat(xtensa): add support for esp32s3 with xtensa arch #399

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

elenaf9
Copy link
Collaborator

@elenaf9 elenaf9 commented Sep 6, 2024

Description

  • Add support for xtensa arch in riot-rs-threads
  • Add support for esp32s3 chip in riot-rs-embassy (cc @ROMemories given that these are minimal changes, I don't think there will be many merge conflicts with refactor(embassy)!: extract arch modules as crates #392. I'll rebase once that PR is in)
  • Add espressif-esp32-s3-wroom-1 board
  • Adapt the laze-project.yml based on prio work from @kaspar030.

Testing

Rust on xtensa requires the installation of the esp toolchain and setting up the environment correctly. See the Rust on ESP Book. It can then be run as usual with

laze build -C examples/threading -b espressif-esp32-s3-wroom-1 run

Issues/PRs references

Fixes #359.

Open TODOs

There are still linker issues that have to be fixed. Right now I simply worked around them by commenting out the conflicting parts, and somehow it worked. See self-review.

Change checklist

  • I have cleaned up my commit history and squashed fixup commits.
  • I have followed the Coding Conventions.
  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.

laze-project.yml Outdated Show resolved Hide resolved
laze-project.yml Outdated Show resolved Hide resolved
// The xtensa-lx-rt does this when calling the exception handler using
// call4, which shifts the window by 4.
// See `xtensa_lx_rt::exception::asm::__default_naked_exception`.
// (At least that's what I assume is happening)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
// (At least that's what I assume is happening)

@kaspar030
Copy link
Collaborator

@elenaf9 do you mind rebasing this?

Move `waiti` out of `THREADS` closure.
The esp-hal implementation of critical-section doesn't disable all
interrupts. So another interrupt handler could run while we waiti, and
cause a panic if it tries to borrow `THREADS`.
Comment on lines +136 to +139
// The esp-hal implementation of critical-section doesn't disable all interrupts.
// Thus we should release our hold on `THREADS` before we `waiti`, to prevent
// that another interrupt handler will try to borrow it while we still have it borrowed.
unsafe { core::arch::asm!("waiti 0") };
Copy link
Collaborator Author

@elenaf9 elenaf9 Sep 18, 2024

Choose a reason for hiding this comment

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

It can still happen right now that an esp_hal::embassy interrupt interrupts a running thread that is currently accessing THREADS. If that interrupt is then also accessing THREADS (e.g. because it's a _pender), it panics.

We could instead disabled all interrupts when accessing THREADS on xtensa. esp-hal originally disabled all interrupts in their critical section, but changed it to only disable interrupts with prio <=5 so that the debugging at interrupt level 6 still works (see esp-rs/esp-hal#360).
So if we'd change it, debugging wouldn't work inside critical sections anymore I think.

The closures for accessing THREADS are very short, so I don't know.

Either way the waiti here still would need to be outside of the closure, because IIUC it only returns if an interrupt at the currently enabled level occurs. If all interrupts are disabled, it will never return.

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

Successfully merging this pull request may close these issues.

Add Xtensa support
3 participants