`state` functions should be removed, mirror async API for events

[MabezDev]

They are inherently racey, and I think we should mirror the async API (i.e wait_for_event and friends) for this kind of event watching instead.

Impl notes

When creating the blocking variants of wait_for_event, we must use OS aware primitives for waiting.

[bjoernQ]

An alternative would be to remove them and teach users to register event handlers like done here:

esp-hal/examples/wifi/access_point/src/main.rs

Lines 58 to 74 in c648482

	// Set event handlers for wifi before init to avoid missing any.
	let mut connections = 0u32;
	_ = event::ApStart::replace_handler(|_| println!("ap start event"));
	event::ApStaConnected::update_handler(move |event| {
	connections += 1;
	esp_println::println!("connected {}, mac: {:?}", connections, event.mac());
	});
	event::ApStaConnected::update_handler(|event| {
	esp_println::println!("connected aid: {}", event.aid());
	});
	event::ApStaDisconnected::update_handler(|event| {
	println!(
	"disconnected mac: {:?}, reason: {:?}",
	event.mac(),
	event.reason()
	);
	});

The API looks a bit weird however

[MabezDev]

An alternative would be to remove them and teach users to register event handlers like done here:

esp-hal/examples/wifi/access_point/src/main.rs

Lines 58 to 74 in c648482

// Set event handlers for wifi before init to avoid missing any.
let mut connections = 0u32;
_ = event::ApStart::replace_handler(|_| println!("ap start event"));
event::ApStaConnected::update_handler(move |event| {
connections += 1;
esp_println::println!("connected {}, mac: {:?}", connections, event.mac());
});
event::ApStaConnected::update_handler(|event| {
esp_println::println!("connected aid: {}", event.aid());
});
event::ApStaDisconnected::update_handler(|event| {
println!(
"disconnected mac: {:?}, reason: {:?}",
event.mac(),
event.reason()
);
});
The API looks a bit weird however

I'm not sure this API is fully fledged yet tbh, and I recall some issue with memory usage with these event handlers as it stands (although that's a bit hand waivey).

I think there is still a lot of value in the simpler wait_for_event API even once we have the full event API sorted (sometimes, maybe most of the time, you don't care about the data associated with these events).

[MabezDev]

Writing my comment in here, after the thought provoking PR from @bjoernQ in #4571 (thank you!).

Problem statement

We need a way of a user knowing some event relating to WiFi. There are a few challenges with this:

• We may miss events if we listen too late.
• We may miss events between beginning to listen for events and the events occurring.
• We may wish to know the current state of the stack without waiting for some specific event.

Solutions

These are the three solutions I considered.

polling global state

Each event that comes in sets a global state variable which a user can poll at anytime.

Pro's:

• Can be used anywhere

Con's:

• Liable to miss events, especially if multiple are posted in quick succession
• Inefficient, must be polled continually
• Event data cannot be coupled with the event

Tracking the state of the wifi blobs and exposing the state carefully

This is an extension of the first approach, where by we curate the state a little more carefully. Instead of using the state straight from the blob events, we curate it based on user actions instead.

Pro's:

• Can be used anywhere
• These event's shouldn't change rapidly, only with user action and should be more "stable".

Con's:

• Inefficient, must be polled continually
• Event data cannot be coupled with the event

callback's for events

Register a call back for an event, which gets executed when the event occurs

Pro's:

• Efficient
• Won't miss events generally, unless the event occurred before we registered the handler
• Data can be provided with the event (passed into the callback)

Con's:

• More memory usage to store callbacks
• call backs introduce non-linear flow into the program (i.e, callback happens and callback needs to access something local to notify - it can't, it has to go through global state mostly.

wait_for API

Register what events you're interested, and be notified when they occur.

Pro's:

• Efficient
• Won't miss events generally, unless the event occurred before we started listening
• Data can be coupled with the event (though we'll likely want to box the innards to avoid massive event enums)
• Asynchronous events can be modeled as a linear flow of code i.e do something -> wait -> do something, with no indirection

Con's:

• Some memory usage to store what events wish to be listened for (this is drastically less than most callback memory usages though)

With all these in mind, I decided on expanding the wait_for API. I'm open to critique of any of these ideas/points, and its highly possible we may need a combination or utilization of more than one of these ideas in the final solution.

I'd like to hear more about the "async iteratator style API" idea you had from #4571 @bjoernQ.

[bjoernQ]

(Sorry for being a constant pain on this topic 😄 - and even more sorry to not going into details in the comment which spawned this )

My main concern about the API is exposing the user to the the raw events emitted by the drivers (yes - we map them to an non-exhaustive enum, loosing the interesting payload of some events). We are not only going to stabilize this - we even "force" the user to use that currently.

Users are required to know which events to expect when - and even more - when not.

e.g. the connect function in our async examples is not super complex - but I have seen users getting it wrong more than once.

And things get even more more complex in a combined-ap-sta setup.

I see value in giving users a way to access the low-level events - but including the additional payload and probably not stabilized from the start.

Try to forget about our current API for a second and see what we could have instead.

I could think of something like this for the connect task

#[embassy_executor::task]
async fn connection(mut controller: WifiController<'static>) {
    println!("start connection task");
    println!("Device capabilities: {:?}", controller.capabilities());

    let station_config = ModeConfig::Station(
        StationConfig::default()
            .with_ssid(SSID.into())
            .with_password(PASSWORD.into()),
    );
    controller.set_config(&station_config).unwrap();
    println!("Starting wifi");
    controller.start_async().await.unwrap();
    println!("Wifi started!");

    println!("Scan");
    let scan_config = ScanConfig::default().with_max(10);
    let result = controller
        .scan_with_config_async(scan_config)
        .await
        .unwrap();
    for ap in result {
        println!("{:?}", ap);
    }

    loop {
        println!("About to connect...");

        match controller.connect_async().await {
            Ok(_) => {
                println!("Wifi connected!");

                let disconnected_station = controller.wait_for_station_disconnect().await.unwrap();
                println!("Wifi disconnected from {:?}", disconnected_station);
                println!("Trying to reconnecting in 5 seconds");
                Timer::after(Duration::from_millis(5000)).await
            }
            Err(e) => {
                println!("Failed to connect to wifi: {e:?}, retry in 5 seconds");
                Timer::after(Duration::from_millis(5000)).await
            }
        }
    }
}

Besides shuffling around some code to make it more readable (which we could do right now) the interesting change is having dedicated methods for the really interesting / needed events.

e.g. waiting for a disconnect could look like this

let disconnected_station = controller.wait_for_station_disconnect().await.unwrap();

This could return an error if we are not in the correct state (i.e. connected) and return the payload (in this case mapped from wifi_event_sta_disconnected_t - which will also include the disconnect-reason which might be the most interesting information for this event)

While there are quite a few events I don't think this will lead to a overly bloated API - and we already handle some of these in a similar way already (by accident) - e.g. connect_async - this just lacks a useful return type

Also this will move the responsibility to not miss relevant events and not waiting for events which will never arrive from the user to us - we have already seen a situation where we got things wrong (#4481) - but getting things right in the code might be better than explaining how to get things right on the user side.

While we are questioning the API - would it be a good idea to stuff all of this into the WifiController? While we currently don't have much for AP, it might be a good thing to have a split for AP and STA specific functionality? i.e. some methods are STA specific and some will be AP specific.

---

Side note: This issue sounds like the state returned by the two free-standing functions would be the same as the events used by the wait_for_event functions .... it's not. The state is something we derive from the events we see - it's really the current state the interface is in

---

With "async iterator style API" I meant something like https://rust-lang.github.io/rfcs/2394-async_await.html#for-await-and-processing-streams - it's not there yet but we might be able to "emulate" it to some extend. Basically a channel

It would match your "callback's for events" paragraph - just async.

[bjoernQ]

The above is just about the async API (which I admit is not even the scope of this issue) - how should things look like for the non-async case?

I'm thinking about the "... and be notified when they occur" part of the blocking "wait_for API"

I guess we'd need some kind of callbacks since blocking until we see a specific event wouldn't make much sense - but then we are quickly at a polished version of the already existing unstable event module? But maybe you had something different in mind