-
Notifications
You must be signed in to change notification settings - Fork 369
WiFi Event API #4677
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
base: main
Are you sure you want to change the base?
WiFi Event API #4677
Conversation
esp-radio/src/wifi/mod.rs
Outdated
| } | ||
|
|
||
| /// Wait for the station to disconnect. | ||
| pub async fn wait_for_station_disconnect(&mut self) { |
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.
I guess the idea is in the proper impl this will have state checks to ensure we're connected or in a state that emit the disconnection event?
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.
yes - I was a bit too lazy in the first iteration 👍
but I admit if this is meant to demonstrate my idea I have to show that
| } | ||
|
|
||
| /// Wait for the access point to stop. | ||
| pub async fn wait_for_access_point_stopped(&mut self) { |
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.
I understand this might be for demonstrative purposes, but isn't this something the user will request to close, not wait for close?
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.
I admit I just mechanically translated the usage in our examples ... thanks for bringing it up, I think at least the comment in the examples is questionable and confusing
This PR is not necessarily meant to get merged as is. It's here to illustrate part of the "wall of text" in #4364
High-level Assessment of the WiFi API
In the first rounds of our efforts to make the crate fit for (partial) stabilization we focused on checking the API we had and identifying things we shouldn't stabilize as is. That's fine as a first step but we should also look at the API from a high-level perspective.
Events
We expose users to the events emitted by the WiFi drivers via the
wait_for_eventfamily of functions and also via theeventsmodule (which should be unstable but apparently isn't yet on main)By doing that we
wait_for_eventfunctions don't have a way to tell the payload of events (while the unstable "event" module does)wait_for_eventasync functions (e.g. to know about a disconnect event)Looking at similar crates (there aren't a lot of them apparently) it seems they don't expose underlying events in a similar way.
While that makes a lot of sense for technical reasons - i.e. a crate abstracting away WiFi control over multiple operating systems would have a harder time to do that - they also offer simpler APIs for basic use-cases (e..g knowing when a station got disconnected).
So at least for simpler things we should offer more specialized functions like
wait_for_station_disconnected- and ideally return an error if we are in the wrong state and we could even return the event payload (e.g. the disconnect reason in this case).For the basic use-cases at least we won't need to offer too much of these functions.
Currently
WifiControlleralso contains functions which are STA or AP exclusiveDo we want to split out
WifiStationController/WifiAccessPointController? (And keepWifiControllerfor the shared functionality)It would make things more clear which functions apply to which interface and makes the API docs cleaner. Especially if we decide to offer the functions mentioned above.
How to "await" events in a non-async way?
One way to await events without async seems to be using a callback-style API.
Given
allocis already a hard requirement to use the crate, the API to allow closures shouldn't be too bad and should offer nice usability. The tricky part would be that (without doing anything special) it will run in the context ofevent_post(which is called in the wifi-task and probably has some timing requirements)An alternative could be polling for events.
There is another alternative: Do nothing - we have the unstable WiFi event API (which we are not happy with yet) but more important: as our examples show we don't even need more than we already have for the non-async case. (i.e. we have functions to query the current state). At least there is little to no pressure to stabilize anything in this regards right now.