preempt improvements

[MabezDev]

First, commit fixes a bug discovered by @bugadani, where preconfigured interrupts were disabled once the preempt module started.

The second commit reduces the task switching frequency on Xtensa to the same values as on RISCV. This should improve performance as less time will now be spent in the switching code. We should do some profiling at some point to determine the best default value for this (I still think 1000hz could be too high), but for now 1000hz seems reasonable. It's no set to 100hz.

Closes #184

[MabezDev]

@bugadani That's odd. Whilst I would expect a loss in perf, I would expect it to be relatively small. I've just pushed a commit which reduces the switching frequency to freeRTOS' default of 100hz. Can you see if this has any affect at all?

[bugadani]

Yes, 100Hz makes a big difference.

[bjoernQ]

Changes look sane and I gave it a try - works fine except on ESP32

This

esp-wifi/esp-wifi/src/timer_esp32.rs

Line 88 in 5ce1c7f

| xtensa_lx_rt::interrupt::CpuInterruptLevel::Level6.mask() | enabled,

is what breaks things.

There are definitely strange things going on with the pre-enabled interrupts on ESP32 (see https://github.com/esp-rs/esp-wifi/pull/186/files#diff-e7c01fb52e0e17b32c13a65823644ff20c6a47e64d12cdf7474b17efce28f83f )

I also think the insane high task-switching frequency was needed in the beginning since there was no yield_task and connecting to WiFi took a very long time ... now things look quite good. But I don't think we should further lower the task-switching frequency (at least not by default)

[bugadani]

I've tried 8175ecd again and it looks like it works well, too.

It's possible I've been trying the patch accidentally in debug mode yesterday, and that might have caused my perf problems. It would certainly explain some of the weirdness I've seen.

[MabezDev]

Changes look sane and I gave it a try - works fine except on ESP32

This

esp-wifi/esp-wifi/src/timer_esp32.rs

Line 88 in 5ce1c7f

| xtensa_lx_rt::interrupt::CpuInterruptLevel::Level6.mask() | enabled,

is what breaks things.

There are definitely strange things going on with the pre-enabled interrupts on ESP32 (see https://github.com/esp-rs/esp-wifi/pull/186/files#diff-e7c01fb52e0e17b32c13a65823644ff20c6a47e64d12cdf7474b17efce28f83f )

I also think the insane high task-switching frequency was needed in the beginning since there was no yield_task and connecting to WiFi took a very long time ... now things look quite good. But I don't think we should further lower the task-switching frequency (at least not by default)

Yeah, this is really weird. For some reason when keeping the interrupts enabled the WIFI_BB interrupt fires constantly, I guess this was happening before though. With the change we keep the already enabled interrupts enabled, WIFI_BB being one of them.

I disabled the WIFI_BB interrupt (by commenting this section:

esp-wifi/esp-wifi/src/timer_esp32.rs

Lines 57 to 62 in 8e35b68

	#[cfg(feature = "wifi")]
	interrupt::enable(
	peripherals::Interrupt::WIFI_BB,
	interrupt::Priority::Priority1,
	)
	.unwrap();

) and everything works just fine...

So I guess the question is what is causing WIFI_BB to fire and how do we clear it?

[bjoernQ]

Great you figured that out. Oh, I remember WIFI_BB from ESP32-C6.

Yes - there is a workaround (which doesn't apply here): https://github.com/esp-rs/esp-wifi/blob/8e35b68c4aaed2c6a4d1159dd1c1287a5a2359be/esp-wifi/src/timer_esp32c6.rs#L47C2-L48

[MabezDev]

I'm a bit unfamiliar with each interrupt type, what does the BB interrupt do? Do we actually need it enabled? It seems this is slightly different to the C6 where it can be disabled, but once enabled it just fires continuously (perhaps we're missing some code to clear it?).

Removing it completely feels weird, but if it works.. it works :D.

[bjoernQ]

mhhh if you comment it out and everything works, we might not need it - I remember there were some interrupt numbers mixed up at some point of time in the PAC ... maybe it originates from that. Especially since we delegate WIFI_BB and WIFI_MAC both to ISR_INTERRUPT_1 callback 🤔

[MabezDev]

Okay, I've removed WIFI_BB from ESP32, are we happy with the task-switching frequency? In my limited testing, 100hz seems to work fine on all the examples I've tried.

[bjoernQ]

Okay, I've removed WIFI_BB from ESP32, are we happy with the task-switching frequency? In my limited testing, 100hz seems to work fine on all the examples I've tried.

I also did some limited testing and it looks good to me, too. Ideally, I'd like to run all the examples on all targets to verify it - not sure if I will get to that today

[bugadani]

I wonder if it's necessary to reduce to 50Hz if debug_asserts are enabled. 100Hz is already lower than any previous values, maybe a single value might work fine for simplicity's sake?

[bjoernQ]

I tested on all targets and it looks fine to me. There is apparently a problem with BLE on ESP32-C2 but I there are problems in the implementation anyways so I would be fine merging this ... probably it increases the pressure a bit to fix ESP32-C2 BLE and also we need to fix that before C6 BLE

[bjoernQ]

LGTM