Task/semaphore tweaking

[bugadani]

get_systimer_count() is an AtomicU64 which is implemented using a critical section. The compiler is not smart enough to remove nested critical sections, so they mostly just waste time. If the slightly closer deadlines (caused by hoisting the timestamp reading out of the loop) can be tolerated, this PR should reduce instruction counts slightly in every loop iteration.

WORKER_HIGH doesn't need to be in an Option. It can be static-initialized instead.

TIMERS are only accessed in critical sections. I don't believe memory fencing is necessary between accessing individual array elements, though I admit I'm not quite sure what a fence achieves at all here.

All in all, this PR results in about 1kB reduction in firmware size for me.

[bjoernQ]

From a brief look it looks good. The whole compat thing isn't exactly pretty but most of the weirdness there originates from problems with the Xtensa backend back then - I think the compiler got much better so it's great to revisit. Back then moving a static variable to a different file made the difference between working and crashing so I was very afraid of touching that code 😆

IMHO we shouldn't check-in the launch config

I'll have another look and run the smoke tests

[bugadani]

IMHO we shouldn't check-in the launch config

Ooops, removed.

I have more changes in mind but I didn't want this PR to grow colossal.

[MabezDev]

LGTM, just one small nit :)

[bugadani]

LGTM, just one small nit :)

Good eyes!

[bjoernQ]

Still doing the smoketesting but there is one small thing:

warning: unused import: `Timer`
 --> esp-wifi\src\tasks.rs:8:24
  |
8 |         timer_compat::{Timer, TIMERS},
  |                        ^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

GH doesn't let me comment on that line

[bjoernQ]

Did the tests and it almost looks good. There is one thing which is a bit sus:

Running the static_ip (cargo run --example static_ip --release --features "wifi") example on ESP32-C6 quite often ends up in a very quick

Wait to get connected
Disconnected

In that case the Disconnected is printed almost at the same time as Wait to get connected

It sometimes works but often not. That is different for me on main.

❯ rustc --version
rustc 1.75.0-nightly (bf9a1c8a1 2023-10-08)

[bugadani]

I really need to source a dev board for each line, don't I? ...

Can one of you guys help me bisect this issue? It seems very weird to me that a particular line would break. I would expect all RISC-V, but I am also not familiar with them.

[bugadani]

One more thing:

Looking at old comments (#235 (review)) it smells to me if something else might be the culprit with static_ip. This isn't the first time you saw problem with that particular example and a RISC-V SoC and I have to wonder if this is really a bug with my changes. I'm not saying it's not, but it's suspicious :)

[bjoernQ]

I'm also surprised it's only on one of the RISC-V socs ... I'll do a git bisect

[bjoernQ]

Bisect got me to the first commit but maybe we should make sure it's not a red herring and have someone else verify this.

@MabezDev or @jessebraham - could you try to run the static_ip example 10 times on main and on this branch each and see if on this branch there are more unsuccessful connection attempts?

[bugadani]

It's not impossible that the first commit has an impact here, as it's changing timer expiration times by a few cycles at least. I'd be very surprised for this to matter.

On the other hand, it turns out that on Xtensa, the timestamps must not be read in a critical section, as due to a race condition the function may occasionally return lower timestamps than expected. So if it matters where and how the code accesses the current time, it's a bit of a conflict between the two architectures.

[MabezDev]

I see the same behaviour in this branch as the current main branch, so I don't think these changes have worsened anything.

[bjoernQ]

I see the same behaviour in this branch as the current main branch, so I don't think these changes have worsened anything.

Thanks for checking this ..... then I guess I will merge it 👍