Skip to content
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

Async WiFi: connect/disconnect/scan/wait_for_event #129

Merged
merged 18 commits into from
Feb 15, 2023

Conversation

MabezDev
Copy link
Member

First off, I'm sorry. This PR got a bit out of hand scope-wise and ends up doing a few things, hopefully reviewing it isn't too much of a pain!

This PR accomplishes the following:

- Don't compile the blocking `WifiStack` when using the embassy-net
  feature
- Improve WifiEventFuture to have a waker for each WifiEvent
- Stub out async versions of embedded-svc trait
- All works first time
  - Fails on reconnect, but this is a bug in the svc implementation
- Implements `IntoFuture` for `WifiEvent`
  - Is it possible to await the event in two seperate futures? or will
    one overwrite the other?
- Now working in fully async fashion
- Improved embedded-svc `Wifi` trait impl
  - is_started etc still needs work
- spotted memory leak when not transmitting anything
Can now succesfully reconnect to a network
Instead of relying on the current state, it now tracks events and clears
the event before trying to listen.
- Split the wifi into two parts, the device part which is used within the
network stack and the controller, which handles the wifi connection
parts

- Remove the uneeded `Wifi` impl now that the two parts are separate
- This solves the waker overwrite issue, as the function takes `&mut
  self`, meaning its only possible to await for the same event once.
- Sadly this means we have to remove the really clean `into_future` impl
  :(, but atleast we don't have to bump MSRV.
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 15, 2023

Nice changes - especially "splits wifi into two parts, device and controlle" is something I already had in mind.

Unfortunately, this breaks esp-now examples since it needs the wifi adapter to be in STA mode and started which always was the case after initialize before

Second thing, compiling embassy-dhcp / embassy-esp-now generates compiler errors

error[E0599]: `&'static AtomicWaker` is not an iterator
   --> esp-wifi\src\wifi\os_adapter.rs:932:19
    |
932 |     event.waker().map(|w| w.wake());
    |                   ^^^ `&'static AtomicWaker` is not an iterator
    |
   ::: ....\.cargo\registry\src\github.com-1ecc6299db9ec823\embassy-sync-0.1.0\src\waitqueue\waker.rs:59:1
    |
59  | pub struct AtomicWaker {
    | ---------------------- doesn't satisfy `AtomicWaker: Iterator`
    |
    = note: the following trait bounds were not satisfied:
            `&'static AtomicWaker: Iterator`
            which is required by `&mut &'static AtomicWaker: Iterator`
            `AtomicWaker: Iterator`
            which is required by `&mut AtomicWaker: Iterator`

- Add wakers for all events
- Add some docs
- Make WifiEventFuture pub(crate)
@MabezDev
Copy link
Member Author

Oops, my commit last night broke it 😅, should be fixed now :)

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I have very limited knowledge about esp-wifi but the changes make sense to me. I've tested the dhcp example to check #120, and it was working fine for me.

Just two small question, that can ofc be addressed, if needed, in other PRs:

  • Shall we cover embassy_dhcp example in CI? embassy_esp_now and esp-now are not covered either.
  • Shall we include a column for async wifi in the current support table?

@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 15, 2023

LGTM! I have very limited knowledge about esp-wifi but the changes make sense to me. I've tested the dhcp example to check #120, and it was working fine for me.

Just two small question, that can ofc be addressed, if needed, in other PRs:

  • Shall we cover embassy_dhcp example in CI? embassy_esp_now and esp-now are not covered either.

That is a good idea! As you said probably in a separate PR

  • Shall we include a column for async wifi in the current support table?

Not sure since it's not chip specific. Now if we have WiFi we also have Async-WiFi

@MabezDev
Copy link
Member Author

Thanks for the reviews! I'll create an issue about adding CI and merge this now :)

@MabezDev MabezDev merged commit e292fbc into esp-rs:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

embedded-svc::Wifi::is_started is wrong wifi_connect and co don't use the svc configuration parameters
3 participants