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

Wait for Wi-Fi management to start up (#8834) #9322

Merged
merged 5 commits into from
Aug 31, 2021

Conversation

tewarid
Copy link
Contributor

@tewarid tewarid commented Aug 30, 2021

Problem

What is being fixed?

  • Wi-Fi management takes a while to startup asynchrounously on Linux and this causes the app to assume Wi-Fi is not provisioned, reset fabric setting, and continue in pairing mode. What a user perceives is that the device has been unpaired and needs to be paired with again.

Change overview

Poll for Wi-Fi Management to start for five attempts every 100 ms.

Testing

How was this tested?

  • Manually tested with all-clusters-app on a custom Linux system build.

@CLAassistant
Copy link

CLAassistant commented Aug 30, 2021

CLA assistant check
All committers have signed the CLA.

@woody-apple woody-apple merged commit 7b0afe0 into project-chip:master Aug 31, 2021
@bzbarsky-apple
Copy link
Contributor

@tewarid @andy31415 I'm a little confused by the changes here. We're sleeping and waiting for some state to change... does it get changed by some other thread? If so, where is the synchronization with the other thread? Or are we just touching members from multiple threads without any synchronization?

@tewarid tewarid deleted the issue-8834 branch September 4, 2021 11:10
@tewarid
Copy link
Contributor Author

tewarid commented Sep 4, 2021

@bzbarsky-apple Async DBUS wpa_supplicant proxy initialization in ConnectivityMgrImpl is the source for parallelism - see sequenced calls to wpa_fi_w1_wpa_supplicant1_proxy_new_for_bus, wpa_fi_w1_wpa_supplicant1_call_get_interface, and wpa_fi_w1_wpa_supplicant1_interface_proxy_new_for_bus in src/platform/Linux/ConnectivityManagerImpl.cpp. During program initialization, the only thread changing state is the one on which DBUS callback happens. I don't see why lack of synchronization should lead to non determinism. Async init is perhaps overkill, in hindsight.

@tewarid
Copy link
Contributor Author

tewarid commented Sep 10, 2021

@bzbarsky-apple Although synchronization may not be necessary, state in GDBusWpaSupplicant (src/platform/Linux/ConnectivityManagerImpl.h) should be declared volatile.

@bzbarsky-apple
Copy link
Contributor

the only thread changing state is the one on which DBUS callback happens

@tewarid Yes, but if it's doing unsynchronized writes and this thread is doing unsynchronized reads, then depending on the exact size of mWpaSupplicant.state and the values of various states the value might test to be equal to GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED even if the actual value being written to the member is not.

What we should probably do is just have that state member be an std::atomic to prevent those sorts of problems.

mkardous-silabs pushed a commit to mkardous-silabs/connectedhomeip that referenced this pull request Sep 24, 2021
…ip#9322)

* Wait for Wi-Fi management to start up (project-chip#8834)

* Restyled by clang-format

* Extract code in function EnsureWifiIsStarted

* Restyled by whitespace

* Fix unused variable warning/error

Co-authored-by: Restyled.io <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants