Rethink WiFi API #3027
Rethink WiFi API #3027
Conversation
| pub async fn start_async(&mut self) -> Result<(), WifiError> { | ||
| let mode = WifiMode::try_from(&self.config)?; | ||
| if !TAKEN | ||
| .compare_exchange( |
There was a problem hiding this comment.
This can also be swap or fetch_or, which only need a single Ordering and end up in general simpler. In general I don't know if I'm happy with the interface being a completely separate singleton (or singleton pair) - but at the very least, we need an unsafe way to grab them in case they need to be re-created for some reasn.
There was a problem hiding this comment.
I was thinking if the network interfaces (and maybe esp-now, we also might want to add a NAN interface later) should be virtual peripherals but for this first commit in this PoC I didn't want to touch that (and I thought it would be weird but after thinking about it I don't think it's that weird).
Generally, I wanted to illustrate the idea of having the network interfaces/device separated from the controller (and it's life cycle)
There was a problem hiding this comment.
For me it's not clear what these network interfaces are meant to represent. I can create them, then create embassy-net runners, without ever configuring esp-wifi. This is pretty different from any other part of the HAL. The example changes in this PR just make me think the general UX of esp-wifi gets worse. I don't know if you demonstrate switching wifi modes anywhere. Given that is one of the motivators, I would probably appreciate seeing them.
I am fine with keeping the interfaces separate from the controller (as in, the controller returned by wifi::new*), but perhaps we should tie them to esp_wifi::init.
There was a problem hiding this comment.
It's certainly different - it's a very different thing IMHO and I don't think we really need to make different things look the same
Connecting / disconnecting is pretty much like plugging in / removing an ethernet cable.
A disconnected network interface is really just that - it won't send or receive anything but otherwise doesn't cause any harm
The examples are just changed to compile / work for now - no new examples are added for now - can be done if we can align on the general direction here before (but ofc needed to validate the design)
I am fine with keeping the interfaces separate from the controller (as in, the controller returned by wifi::new*), but perhaps we should tie them to esp_wifi::init.
Sounds reasonable - but would require the network interfaces to get teared down to stop esp-wifi completely? It might be desirable to stop esp-wifi (including the scheduler) for some time and periodically connect to some network to e.g. check for OTA updates or push some MQTT packets (without the whole re-create the network interfaces dance)
There was a problem hiding this comment.
Sounds reasonable - but would require the network interfaces to get teared down to stop esp-wifi completely? It might be desirable to stop esp-wifi (including the scheduler) for some time and periodically connect to some network to e.g. check for OTA updates or push some MQTT packets (without the whole re-create the network interfaces dance)
Interesting, why would one want to tear down esp_wifi even after turning of WiFi and BL? Maybe there should some ref counting on these resources.
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
This is a POC to address #2224
The idea is
This might or might not end up being merged. For now, it's here to illustrate how the API can or should be changed
Testing
Run the examples