-
Notifications
You must be signed in to change notification settings - Fork 12
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
embassy: enable usage of esp-wifi
with riot-rs-threads
#300
base: main
Are you sure you want to change the base?
Conversation
"usually" means, usually |
@kaspar030 noted in an out-of-band discussion that the embassy initalization doesn't have to happen in the This has multiple advantages:
See commit fb9f68e. |
I have now moved the logic for spawning the esp-wifi-thread into The downside of it is that a thread running in the background is probably unexpected for the user and could cause some confusing (e.g. it implicitly reduces the number of threads a user can still spawn; it it high priority, ...). |
esp-wifi
with riot-rs-threads
esp-wifi
with riot-rs-threads
#304 is in, please rebase! |
c348c37
to
0304fbd
Compare
I'm testing this branch, with these changes: https://gist.github.com/kaspar030/833f672c398d20b18695210b08e265f0 I'm trying to figure out the behavior w.r.t. priorities of the executor vs. the thread. Something seems off, this is the output:
... I'd expect the dots and the "tick" to be interleaved, also, the WiFi doesn't connect. I think the executor is not scheduled anymore. |
Thank you for testing. I am currently debugging this, and it's super weird. What I observed is:
I am trying to debug this further (already tried increasing stack sizes, didn't help)... |
ok, that is weird. Could have a lot of reasons ... I'd ensure first that the task switching is triggered only when we actually expect it. E.g., that btw, where is the |
This resolves the conflict between the esp_wifi internal scheduler for the wifi background task, and the riot-rs-threads scheduling. The esp_wifi thread is running at highest prio and frequently woken up to run its tasks.
Fixes future-proof-iot#310 for the system timer interrupt here.
d18117b
to
ddc0975
Compare
ddc0975
to
a266676
Compare
Disable CPU Interrupt 3 when not in `esp_wifi_thread`. This is necessary so that the riot-rs-threads scheduler and the esp-wifi scheduler won't interleave. The `esp_wifi_thread` is frequently woken up by the systimer and will then do the necessary yielding.
Yep, that was the issue. Disabling the CPU Interrupt outside of our esp-wifi-thread solves this issue. But as a result, esp-wifi can't directly yield to its tasks from inside an ISR anymore. Instead it only yields once the systimer fires again. Given that the timer fires very frequently, and from what I have tested, this restriction isn't necessarily a problem. I have explored an alternative implementation, where we don't disable CPU interrupt 3 outside of the esp-wifi-thread. Instead we intercept it and wake up our esp-wifi-thread like we already do it on a systimer interrupt. But this requires some other changes as well, and results in a much more complex code: elenaf9/RIOT-rs@53ffe3b...ddc0975. I also noticed that this alternative implementation doesn't work as reliably, but couldn't fully debug why. I am leaning towards the first implementation, which is the one currently pushed here. We can reiterate on this if an example comes up where it doesn't work anymore. Wdyt? An orthogonal issue that I just want to mention as well: |
|
||
use super::*; | ||
#[cfg(context = "esp32c6")] | ||
use esp_hal::peripherals::INTPRI as SystemPeripheral; | ||
#[cfg(context = "esp32c3")] | ||
use esp_hal::peripherals::SYSTEM as SystemPeripheral; | ||
use esp_hal::{ | ||
interrupt, | ||
peripherals::{Interrupt, SYSTIMER}, | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use super::*; | |
#[cfg(context = "esp32c6")] | |
use esp_hal::peripherals::INTPRI as SystemPeripheral; | |
#[cfg(context = "esp32c3")] | |
use esp_hal::peripherals::SYSTEM as SystemPeripheral; | |
use esp_hal::{ | |
interrupt, | |
peripherals::{Interrupt, SYSTIMER}, | |
}; | |
use esp_hal::{ | |
interrupt, | |
peripherals::{Interrupt, SYSTIMER}, | |
}; | |
#[cfg(context = "esp32c6")] | |
use esp_hal::peripherals::INTPRI as SystemPeripheral; | |
#[cfg(context = "esp32c3")] | |
use esp_hal::peripherals::SYSTEM as SystemPeripheral; | |
use super::*; |
Reordered as per the coding conventions
@@ -15,5 +15,8 @@ embassy-net = { workspace = true, features = ["tcp"] } | |||
embassy-time = { workspace = true, default-features = false } | |||
embedded-io-async = "0.6.1" | |||
heapless = { workspace = true } | |||
riot-rs = { path = "../../src/riot-rs", features = ["override-network-config"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why static address configuration (what the network configuration does) needs to be disabled by default
.set(riot_rs_threads::current_pid().unwrap()) | ||
.unwrap(); | ||
|
||
// Wait until `embassy` was initialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Wait until `embassy` was initialized. | |
// Wait until `embassy` is initialized. |
riot_rs_threads::thread_flags::wait_one(0b1); | ||
|
||
// Bind the periodic systimer that is configured in esp-wifi to our own handler. | ||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get a SAFETY
comment here (I suppose that for instance the interrupt must only be bound to one thing at a time)?
|
||
// Handle the systimer alarm 0 interrupt, configured in esp-wifi. | ||
extern "C" fn systimer_target0_() { | ||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a SAFETY comment, at least to justify that SYSTIMER
is not stolen multiple times.
|
||
// CPU Interrupt 3 triggers the scheduler in `esp-wifi`. | ||
fn yield_to_esp_wifi_scheduler() { | ||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get also a SAFETY comment here (maybe move the comment outside the function inside)?
This implements an example that uses
esp-wifi
together withriot-rs-threads
.I don't intend for this to be merged, but rather to get feedback on the idea, and to start the discussion on how we can solve it directly inside
riot-rs-embassy
.Problem when using
esp-wifi
and RIOT-rs threading togetherAs documented in #289, using the two components together is non trivial because
esp-wifi
internally runs a preemptive scheduler for wif-/ bluetooth related tasks. This scheduler uses the periodic systemtimer alarm0 to switch between its tasks, in addition to cooperative yielding using CPU interrupt 3.The preemptive systemtimer-triggered context switch conflicts with our riot-rs-threads scheduler because it would interleave both schedulers, resulting in undefined behavior.
Proposed Solution
The idea in this PR is to solve the problem as follows:
esp_wifi_thread
) that drives the esp-wifi tasks in a loop. A loop iteration essentially consists of the following two steps:esp_wifi_thread
.threading
feature enabled however, this doesn't happen anymore because threading is started before the call toriot_rs_embassy_init
. Thus theapplication thread has to explicitly call this initialization function. This will also spawn the executor that runs theesp-wifi-thread
tcp_echo
task.Limitations when using
embassy-time
Not relevant for this PR anymore with the change described in #300 (comment). Nevertheless, I still think it's something to consider when we talk about spawning multiple embassy executors in different threads.
Independently of the conflict between the
esp-wifi
scheduler and ourriot-rs-threads
scheduler, there is an issue with timers when using embassy together with our threads on esp.If
embassy-time
is required (which is the case inriot_rs_embassy::wifi::esp_wifi
), the embassy executor tries to allocate a timer upon creation.The concrete timer driver implementation for this is configured through a feature flag on
esp-hal
.Right now, we are using the Timer Group Timer 0 (TIMG0) through the
embassy-time-timg0
feature. The problem is that this gives exactly one timer. So as soon as we start a second embassy executor in a secondriot-rs-thread
Thread, the timer allocation panics.I now switched to using the system timer. However, this also only gives us 3 timers in total. One is already used as described above byesp-wifi
for alarm0. So only 2 remain, which means that we can only ever have two embassy executors if thetime
feature is enabled.Thoughts on this @kaspar030 @ROMemories?
Fixes #289.