-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
extmod/network_cyw43: Allow configuring active AP interface. #16226
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
extmod/network_cyw43: Allow configuring active AP interface. #16226
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16226 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 164 164
Lines 21347 21347
=======================================
Hits 21043 21043
Misses 304 304 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
Code size report: |
|
I think the ordering of active(True) vs configuration is quite a tricky issue. If esp32/8266 really do need the interface to be up before it's configured, then won't they always possibly appear briefly as an unconfigured AP first? IMO for AP it makes sense to first configure everything and then do active(True). If we could make it work that way on esp then I think that would be better. Regardless of that, this PR is fixing an inconsistency so is probably good to merge it. |
Actually I was wrong about esp32, this restriction only exists on esp8266. Will update the commit message and description. esp32 can have It looks like calling |
3eda268 to
4dc55fa
Compare
dpgeorge
left a comment
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.
Tested on PYBD_SF6, changing the ssid while the AP is active and it does indeed cycle the AP and the new SSID is then visible.
Configuring the AP for cyw43 writes to some buffers that are only sent to the modem when the interface is brought up. This means you can't configure the AP after calling active(True), the new settings seem to be accepted but the radio doesn't change. This is different to the WLAN behaviour on other ports. The esp8266 port requires calling active(True) on the AP before configuring, even. Fix this by bouncing the AP interface after a config change, if it's active. Configuring with active(False) still works the same as before. Adds a static variable to track interface active state, rather than relying on the LWIP interface state. This is because the interface state is updated by a driver callback and there's a race: if code calls active(True) and then config(a=b) then the driver doesn't know it's active yet and the changes aren't correctly applied. It is possible this pattern will cause the AP to come up briefly with the default "PICOabcd" SSID before being reconfigured, however (due to the aforementioned race condition) it seems like this may not happen at all before the new config is applied. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
4dc55fa to
181800e
Compare
Summary
Configuring the AP for cyw43 writes to some buffers that are only sent to the modem when the interface is brought up. This means you can't configure the AP after calling active(True), the new settings seem to be accepted but the radio doesn't change.
This is different to the documented WLAN behaviour which says to call active(True) before configuring the interface. The esp8266
and esp32ports require this, even. EDIT: only esp8266 requires this, esp32 works in either orderFix this by bouncing the AP interface after a config change, if it's active. Configuring with active(False) still works the same as before.
Adds a static variable to track interface active state, rather than relying on the LWIP interface state. This is because the interface state is updated by a driver callback and there's a race: if code calls active(True) and then config(a=b) then the driver doesn't know it's active yet and the changes aren't correctly applied.
Closes #14224
This work was funded through GitHub Sponsors.
Testing
This is one of two fixes which are required for RPI_PICO_W board to pass the wlan test added in #16225.
Trade-Offs and Alternatives
It is possible this pattern will cause the AP to come up briefly with the default "PICOabcd" SSID before being reconfigured, however (due to the aforementioned race condition) it seems like this may not happen at all before the new config is applied. However, if that was a concern then we could hold off bringing the AP interface up until after it's configured. Doing it that way would be a potential breaking change, though.