Refactor WiFi credential check; [Gen 4] WiFi thread safety issues, USB re-enumeration bugfix for weird USB hubs

hal/network/lwip/esp32/esp32ncpnetif.cpp

@@ -322,7 +322,9 @@ int Esp32NcpNetif::upImpl() {
     // Ensure that we are disconnected
     downImpl();
     r = wifiMan_->connect();
-    if (r) {
+    // FIXME: with just cleared configuration and no 'NetifEvent::Down' issued from SystemNetworkManager
+    // we are still attempting to connect. For now simply suppress the log.
+    if (r && wifiMan_->hasNetworkConfig()) {
         LOG(TRACE, "Failed to connect to WiFi: %d", r);
     }
     return r;

hal/network/lwip/realtek/rtlncpnetif.cpp

@@ -287,7 +287,9 @@ int RealtekNcpNetif::upImpl() {
     // Ensure that we are disconnected
     downImpl();
     r = wifiMan_->connect();
-    if (r) {
+    // FIXME: with just cleared configuration and no 'NetifEvent::Down' issued from SystemNetworkManager
+    // we are still attempting to connect. For now simply suppress the log.
+    if (r && wifiMan_->hasNetworkConfig()) {
         LOG(TRACE, "Failed to connect to WiFi: %d", r);
     }
     return r;

hal/network/ncp/wifi/wifi_network_manager.cpp

@@ -33,10 +33,6 @@
 
 #include "network_config.pb.h"
 
-#include "network/ncp/wifi/ncp.h"
-#include "ifapi.h"
-#include "system_network.h"
-
 #define PB(_name) particle_firmware_##_name
 #define PB_WIFI(_name) particle_ctrl_wifi_##_name
 
@@ -198,6 +194,10 @@ WifiNetworkManager::WifiNetworkManager(WifiNcpClient* client) :
 WifiNetworkManager::~WifiNetworkManager() {
 }
 
+int WifiNetworkManager::connect(WifiNetworkConfig conf) {
+    return client_->connect(conf.ssid(), conf.bssid(), conf.security(), conf.credentials());
+}
+
 int WifiNetworkManager::connect(const char* ssid) {
     // Get known networks
     Vector<WifiNetworkConfig> networks;
@@ -220,7 +220,7 @@ int WifiNetworkManager::connect(const char* ssid) {
     // Perform a network scan on ESP32 devices because ESP32 doesn't support 802.11v/k/r
     int r = SYSTEM_ERROR_INTERNAL;
     if (client_->ncpId() != PlatformNCPIdentifier::PLATFORM_NCP_ESP32) {
-        r = client_->connect(network->ssid(), network->bssid(), network->security(), network->credentials());
+        r = connect(*network);
     }
     if (r < 0) {
         // Perform network scan
@@ -250,7 +250,7 @@ int WifiNetworkManager::connect(const char* ssid) {
             } else if (strcmp(ssid, ap.ssid()) != 0) {
                 continue;
             }
-            r = client_->connect(network->ssid(), ap.bssid(), network->security(), network->credentials());
+            r = connect(*network);
             if (r == 0) {
                 if (network->bssid() != ap.bssid()) {
                     // Update BSSID
@@ -267,7 +267,7 @@ int WifiNetworkManager::connect(const char* ssid) {
             for (; index < networks.size(); ++index) {
                 network = &networks.at(index);
                 if (network->hidden()) {
-                    r = client_->connect(network->ssid(), network->bssid(), network->security(), network->credentials());
+                    r = connect(*network);
                     if (r == 0) {
                         connected = true;
                         break;
@@ -299,11 +299,43 @@ int WifiNetworkManager::connect(const char* ssid) {
     return 0;
 }
 
-int WifiNetworkManager::setNetworkConfig(WifiNetworkConfig conf, bool validate) {
+int WifiNetworkManager::setNetworkConfig(WifiNetworkConfig conf, WifiNetworkConfigFlags flags) {
     CHECK_TRUE(conf.ssid(), SYSTEM_ERROR_INVALID_ARGUMENT);
+    NcpClientLock lock(client_);
+    if (flags & WifiNetworkConfigFlag::VALIDATE) {
+        if (conf.credentials().type() == WifiCredentials::Type::PASSWORD && conf.security() == WifiSecurity::NONE) {
+            Vector<WifiScanResult> networks;
+            CHECK(client_->scan([](WifiScanResult network, void* data) -> int {
+                const auto networks = (Vector<WifiScanResult>*)data;
+                CHECK_TRUE(networks->append(std::move(network)), SYSTEM_ERROR_NO_MEMORY);
+                return 0;
+            }, &networks));
+            for (auto network : networks) {
+                if (!strcmp(conf.ssid(), network.ssid())) {
+                    conf.security((WifiSecurity)network.security());
+                    break;
+                }
+            }
+        }
+        client_->disconnect();
+        bool state = true;
+        if (flags & WifiNetworkConfigFlag::TURN_ON) {
+            state = client_->ncpPowerState() == NcpPowerState::ON;
+            CHECK(client_->on());
+            client_->disable();
+            CHECK(client_->enable());
+            CHECK(client_->on());
+        }
+        SCOPE_GUARD({
+            if (!state) {
+                client_->off();
+            }
+        });
+        CHECK(connect(conf));
+    } else {
+        lock.unlock();
+    }
     Vector<WifiNetworkConfig> networks;
-    WifiNetworkConfig oldConfig = {};
-    bool restoreOldConfig = false;
     CHECK(loadConfig(&networks));
     int index = networkIndexForSsid(conf.ssid(), networks);
     if (index < 0) {
@@ -312,77 +344,9 @@ int WifiNetworkManager::setNetworkConfig(WifiNetworkConfig conf, bool validate)
             CHECK_TRUE(networks.resize(networks.size() + 1), SYSTEM_ERROR_NO_MEMORY);
         }
         index = networks.size() - 1;
-    } else if (validate) {
-        // Has old config
-        oldConfig = networks[index];
     }
     networks[index] = std::move(conf);
     CHECK(saveConfig(networks));
-    if (validate) {
-        restoreOldConfig = true;
-        const auto mgr = wifiNetworkManager();
-        CHECK_TRUE(mgr, SYSTEM_ERROR_UNKNOWN);
-        auto ncpClient = mgr->ncpClient();
-        CHECK_TRUE(ncpClient, SYSTEM_ERROR_UNKNOWN);
-        const NcpClientLock lock(ncpClient);
-
-        NcpPowerState ncpPwrState = ncpClient->ncpPowerState();
-        bool networkOn = network_is_on(NETWORK_INTERFACE_WIFI_STA, nullptr);
-        bool needToConnect = network_connecting(NETWORK_INTERFACE_WIFI_STA, 0, nullptr) || network_ready(NETWORK_INTERFACE_WIFI_STA, NETWORK_READY_TYPE_ANY, nullptr);
-        if_t iface = nullptr;
-        CHECK(if_get_by_index(NETWORK_INTERFACE_WIFI_STA, &iface));
-        CHECK_TRUE(iface, SYSTEM_ERROR_INVALID_STATE);
-
-        SCOPE_GUARD ({
-            if (!needToConnect) {
-                ncpClient->disconnect();
-                network_disconnect(NETWORK_INTERFACE_WIFI_STA, NETWORK_DISCONNECT_REASON_USER, nullptr);
-                // network_disconnect() will disable the NCP client
-                ncpClient->enable();
-                if (!networkOn) {
-                    network_off(NETWORK_INTERFACE_WIFI_STA, 0, 0, nullptr);
-                    if_req_power pwr = {};
-                    pwr.state = IF_POWER_STATE_DOWN;
-                    if_request(iface, IF_REQ_POWER_STATE, &pwr, sizeof(pwr), nullptr);
-                    if (ncpPwrState != NcpPowerState::ON && ncpPwrState != NcpPowerState::TRANSIENT_ON) {
-                        ncpClient->off();
-                    }
-                } 
-#if HAL_PLATFORM_NRF52840
-                else {
-                    // The above enable() puts the NCP client into disabled and powered off state
-                    // The following enable() will actually enable the NCP client and put it to OFF state
-                    ncpClient->enable();
-                    ncpClient->on();
-                }
-#endif
-            }
-            if (restoreOldConfig) {
-                if (oldConfig.ssid() == nullptr) {
-                    networks.removeAt(index);
-                } else {
-                    networks[index] = oldConfig;
-                }
-                saveConfig(networks);
-            }
-        });
-
-        // Connect to the network
-        CHECK(ncpClient->on());
-        // To unblock
-        ncpClient->disable();
-        CHECK(ncpClient->enable());
-        // These two are in sync now
-        ncpClient->disconnect(); // ignore the error
-        network_disconnect(NETWORK_INTERFACE_WIFI_STA, NETWORK_DISCONNECT_REASON_USER, nullptr);
-        // FIXME: We are wiating for ncpNetif to potentially fully disconnect
-        // FIXME: synchronize NCP client / NcpNetif and system network manager state
-        CHECK(ncpClient->enable());
-        CHECK(ncpClient->on());
-        network_connect(NETWORK_INTERFACE_WIFI_STA, 0, 0, nullptr);
-        CHECK(mgr->connect(conf.ssid()));
-        restoreOldConfig = false;
-    }
     return 0;
 }
 

hal/network/ncp/wifi/wifi_network_manager.h

@@ -21,6 +21,7 @@
 #include "c_string.h"
 
 #include <cstdint>
+#include "enumflags.h"
 
 namespace particle {
 
@@ -45,6 +46,16 @@ enum class WifiSecurity {
     WPA2_WPA3_PSK = 6
 };
 
+enum class WifiNetworkConfigFlag {
+    NONE = 0x00,
+    VALIDATE = 0x01,
+    TURN_ON = 0x02
+};
+
+typedef EnumFlags<WifiNetworkConfigFlag> WifiNetworkConfigFlags;
+
+ENABLE_ENUM_CLASS_BITWISE(WifiNetworkConfigFlag);
+
 class WifiCredentials {
 public:
     enum Type {
@@ -152,9 +163,10 @@ class WifiNetworkManager {
     ~WifiNetworkManager();
 
     int connect(const char* ssid);
+    int connect(WifiNetworkConfig conf);
     int connect();
 
-    static int setNetworkConfig(WifiNetworkConfig conf, bool validate = false);
+    int setNetworkConfig(WifiNetworkConfig conf, WifiNetworkConfigFlags flags = WifiNetworkConfigFlag::NONE);
     static int getNetworkConfig(const char* ssid, WifiNetworkConfig* conf);
     static int getNetworkConfig(GetNetworkConfigCallback callback, void* data);
     static void removeNetworkConfig(const char* ssid);

hal/network/ncp/wifi/wlan_hal.cpp

@@ -182,7 +182,12 @@ int wlan_set_credentials(WLanCredentials* halCred) {
     conf.credentials(std::move(cred));
     const auto mgr = wifiNetworkManager();
     CHECK_TRUE(mgr, SYSTEM_ERROR_UNKNOWN);
-    CHECK(mgr->setNetworkConfig(std::move(conf), halCred->flags & WLAN_SET_CREDENTIALS_FLAGS_VALIDATE));
+    WifiNetworkConfigFlags flags = WifiNetworkConfigFlag::NONE;
+    if (halCred->flags & WLAN_SET_CREDENTIALS_FLAGS_VALIDATE) {
+        flags |= WifiNetworkConfigFlag::VALIDATE;
+        flags |= WifiNetworkConfigFlag::TURN_ON;
+    }
+    CHECK(mgr->setNetworkConfig(std::move(conf), flags));
     return 0;
 }
 

hal/network/ncp_client/realtek/rtl_ncp_client.cpp

@@ -235,11 +235,6 @@ int RealtekNcpClient::on() {
     CHECK(rltkOff());
     CHECK(rltkOn());
     ncpState(NcpState::ON);
-    wifi_reg_event_handler(WIFI_EVENT_DISCONNECT, [](char* buf, int buf_len, int flags, void* userdata) -> void {
-        LOG(INFO, "disconnected");
-        RealtekNcpClient* client = (RealtekNcpClient*)userdata;
-        client->connectionState(NcpConnectionState::DISCONNECTED);
-    }, (void*)this);
     return SYSTEM_ERROR_NONE;
 }
 
@@ -304,12 +299,19 @@ NcpConnectionState RealtekNcpClient::connectionState() {
 }
 
 int RealtekNcpClient::connect(const char* ssid, const MacAddress& bssid, WifiSecurity sec, const WifiCredentials& cred) {
+    CHECK_FALSE(needsReset_, SYSTEM_ERROR_BUSY);
     int rtlError = RTW_ERROR;
     for (int i = 0; i < 2; i++) {
         {
             const NcpClientLock lock(this);
             CHECK_TRUE(connState_ == NcpConnectionState::DISCONNECTED, SYSTEM_ERROR_INVALID_STATE);
 
+            wifi_reg_event_handler(WIFI_EVENT_DISCONNECT, [](char* buf, int buf_len, int flags, void* userdata) -> void {
+                LOG(INFO, "disconnected");
+                RealtekNcpClient* client = (RealtekNcpClient*)userdata;
+                client->connectionState(NcpConnectionState::DISCONNECTED);
+            }, (void*)this);
+
             LOG(INFO, "Try to connect to ssid: %s", ssid);
             rtlError = wifi_connect((char*)ssid,
                                     wifiSecurityToRtlSecurity(sec),
@@ -332,8 +334,8 @@ int RealtekNcpClient::connect(const char* ssid, const MacAddress& bssid, WifiSec
 }
 
 int RealtekNcpClient::getNetworkInfo(WifiNetworkInfo* info) {
-    const NcpClientLock lock(this);
     CHECK_TRUE(connState_ == NcpConnectionState::CONNECTED, SYSTEM_ERROR_INVALID_STATE);
+    const NcpClientLock lock(this);
 
     int rtlError = 0;
     // LOG(INFO, "RNCP getNetworkInfo");
@@ -380,9 +382,10 @@ int RealtekNcpClient::getNetworkInfo(WifiNetworkInfo* info) {
 }
 
 int RealtekNcpClient::scan(WifiScanCallback callback, void* data) {
-    const NcpClientLock lock(this);
-
+    CHECK_FALSE(needsReset_, SYSTEM_ERROR_BUSY);
     CHECK_TRUE(ncpState_ == NcpState::ON, SYSTEM_ERROR_INVALID_STATE);
+
+    const NcpClientLock lock(this);
     struct Context {
         WifiScanCallback callback = nullptr;
         void* data = nullptr;
@@ -454,17 +457,19 @@ int RealtekNcpClient::scan(WifiScanCallback callback, void* data) {
     for (int i = 0; i < ctx.results.size(); i++) {
         callback(ctx.results[i], data);
     }
-    if ((rtlError && ctx.results.size() == 0) || rtlError == RTW_TIMEOUT) {
+
+    // XXX:
+    if ((rtlError /* keeping this for now */ && ctx.results.size() == 0) || rtlError == RTW_TIMEOUT) {
         // Workaround for a weird state we might enter where the wifi driver
         // is not returning any results
-        rtwRadioReset();
-        return rtl_error_to_system(rtlError);
+        needsReset_ = true;
     }
 
     return rtl_error_to_system(rtlError);
 }
 
 int RealtekNcpClient::getMacAddress(MacAddress* addr) {
+    const NcpClientLock lock(this);
     char mac[6*2 + 5 + 1] = {};
     wifi_get_mac_address(mac);
     CHECK_TRUE(macAddressFromString(addr, mac), SYSTEM_ERROR_UNKNOWN);
@@ -564,8 +569,14 @@ int RealtekNcpClient::dataChannelWrite(int id, const uint8_t* data, size_t size)
 int RealtekNcpClient::dataChannelFlowControl(bool state) {
     return SYSTEM_ERROR_NONE;
 }
+
 void RealtekNcpClient::processEvents() {
+    if (needsReset_) {
+        rtwRadioReset();
+        needsReset_ = false;
+    }
 }
+
 int RealtekNcpClient::checkParser() {
     return SYSTEM_ERROR_NONE;
 }

hal/network/ncp_client/realtek/rtl_ncp_client.h

@@ -73,6 +73,7 @@ class RealtekNcpClient: public WifiNcpClient {
     volatile NcpState prevNcpState_;
     volatile NcpConnectionState connState_;
     volatile NcpPowerState pwrState_;
+    volatile bool needsReset_ = false;
 
     void ncpState(NcpState state);
     void ncpPowerState(NcpPowerState state);