Multicore-aware executors #723
Conversation
bugadani
changed the title
bugadani
force-pushed
the
executor
branch
12 times, most recently
from
198f505 to
5fbf286
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bugadani
force-pushed
the
executor
branch
5 times, most recently
from
19b35a5 to
e07f869
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bugadani
force-pushed
the
executor
branch
3 times, most recently
from
c0197f3 to
8ffc5db
Compare
This comment was marked as outdated.
This comment was marked as outdated.
bugadani
force-pushed
the
executor
branch
2 times, most recently
from
a29a353 to
304d6fe
Compare
bugadani
force-pushed
the
executor
branch
8 times, most recently
from
9aa84e7 to
e2f47a2
Compare
|
Who may I curse with this monster? @MabezDev I hope you don't mind if I ping you for a review? |
MabezDev
left a comment
MabezDev
left a comment
There was a problem hiding this comment.
Thanks for looking into this! It looks good from my perspective, working on all Xtensa chips :). Just a couple of comments to address.
| /// # Safety | ||
| /// | ||
| /// You MUST call this from the interrupt handler, and from nowhere else. | ||
| // TODO: it would be pretty sweet if we could register our own interrupt handler |
There was a problem hiding this comment.
This would be pretty nice, however, I can't think of a way to do it with the current interrupt registering mechanism. Unless we do some weird stuff with weak linkage, but that would introduce errors on its own. I think the examples for now explain the steps that need to be taken well enough.
There was a problem hiding this comment.
I'm not sure about weak linkage, in theory (unless there are details I need to know but don't) we should be able to copy peripherals::__INTERRUPTS into RAM, change https://github.com/esp-rs/esp-hal/blob/6c2659f9e4db7fdc1d5431c267b56ec6caa02ebe/esp-hal-common/src/interrupt/xtensa.rs#L440C9-L440C87 to use the copied handlers and roll with that. I do agree that it's not absolutely necessary
|
Bleh, global copy-and-replace just made this worse 🤣 |
|
Ok I ended up going all in against |
jessebraham
left a comment
jessebraham
left a comment
There was a problem hiding this comment.
LGTM, thanks for taking care of this!
* Implement multicore-aware executors * Add examples * Use pre-defined config to import SystemPeripheral * Use static_cell::make_static
3 participants
Thank you!
Thank you for your contribution.
Please make sure that your submission includes the following:
Must
errorsorwarnings.cargo fmtwas run.CHANGELOG.mdin the proper section.Nice to have
The Xtensa executor in embassy-executor is not safe on multiple cores. There are two reasons for this:
This PR remedies this issue, implementing:
FROM_CPU_0FROM_CPU_xinterrupts (one per executor)On single-core (S2) we could probably use other interrupts as well. I'll probably need to add a list of crosscore-compatible, sw pendable interrupts for ESP32 and S3 (which is probably just the 4 FROM_CPU interrupt), so that we can safely disallow arbitrary interrupts on multicore.
We can't implement this in embassy, because there is no cross-core communication possibility in the Xtensa ISA. However, for consistency's sake I've also included non-multicore support (S2), which is basically identical to what's in embassy.
No, this isn't an executor that spreads load to two cores. Instead, this is an executor that is safe to use on both cores where one core's tasks can wake up the other core's tasks.