NetworkCommissioning: Disconnect previous network when trying a new (#…

…35256)

* NetworkCommissioning: disconnect previous network when attepmting a new connection on other endpoint

* use linked list and disconnect driver when no connected network config

* Add DisconnectFromNetwork for OpenThread and ESP32 platform

* Update src/app/clusters/network-commissioning/network-commissioning.cpp

Co-authored-by: Karsten Sperling <[email protected]>

* resolve comments

* Use IntrusiveList and other minor tweaks

* fix CI

---------

Co-authored-by: Karsten Sperling <[email protected]>
Co-authored-by: Karsten Sperling <[email protected]>

src/app/clusters/network-commissioning/network-commissioning.cpp

@@ -336,6 +336,10 @@ CHIP_ERROR ThreadScanResponseToTLV::EncodeTo(TLV::TLVWriter & writer, TLV::Tag t
 
 } // namespace
 
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+Instance::NetworkInstanceList Instance::sInstances;
+#endif
+
 Instance::Instance(EndpointId aEndpointId, WiFiDriver * apDelegate) :
     CommandHandlerInterface(Optional<EndpointId>(aEndpointId), Id), AttributeAccessInterface(Optional<EndpointId>(aEndpointId), Id),
     mEndpointId(aEndpointId), mFeatureFlags(WiFiFeatures(apDelegate)), mpWirelessDriver(apDelegate), mpBaseDriver(apDelegate)
@@ -365,11 +369,23 @@ CHIP_ERROR Instance::Init()
     mLastNetworkingStatusValue.SetNull();
     mLastConnectErrorValue.SetNull();
     mLastNetworkIDLen = 0;
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    if (!sInstances.Contains(this))
+    {
+        sInstances.PushBack(this);
+    }
+#endif
     return CHIP_NO_ERROR;
 }
 
 void Instance::Shutdown()
 {
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    if (sInstances.Contains(this))
+    {
+        sInstances.Remove(this);
+    }
+#endif
     mpBaseDriver->Shutdown();
 }
 
@@ -1031,6 +1047,16 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec
     mCurrentOperationBreadcrumb = req.breadcrumb;
 
 #if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    // Per spec, lingering connections on any other interfaces need to be disconnected at this point.
+    for (auto & node : sInstances)
+    {
+        Instance * instance = static_cast<Instance *>(&node);
+        if (instance != this)
+        {
+            instance->DisconnectLingeringConnection();
+        }
+    }
+
     mpWirelessDriver->ConnectNetwork(req.networkID, this);
 #else
     // In Non-concurrent mode postpone the final execution of ConnectNetwork until the operational
@@ -1150,6 +1176,29 @@ void Instance::HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryId
 }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC
 
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+void Instance::DisconnectLingeringConnection()
+{
+    bool haveConnectedNetwork = false;
+    EnumerateAndRelease(mpBaseDriver->GetNetworks(), [&](const Network & network) {
+        if (network.connected)
+        {
+            haveConnectedNetwork = true;
+            return Loop::Break;
+        }
+        return Loop::Continue;
+    });
+
+    // If none of the configured networks is `connected`, we may have a
+    // lingering connection to a different network that we need to disconnect.
+    // Note: The driver may or may not be actually connected
+    if (!haveConnectedNetwork)
+    {
+        LogErrorOnFailure(mpWirelessDriver->DisconnectFromNetwork());
+    }
+}
+#endif
+
 void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t interfaceStatus)
 {
     auto commandHandleRef = std::move(mAsyncCommandHandle);

src/app/clusters/network-commissioning/network-commissioning.h

@@ -22,6 +22,7 @@
 #include <app/AttributeAccessInterface.h>
 #include <app/CommandHandlerInterface.h>
 #include <app/data-model/Nullable.h>
+#include <lib/support/IntrusiveList.h>
 #include <lib/support/ThreadOperationalDataset.h>
 #include <lib/support/Variant.h>
 #include <platform/NetworkCommissioning.h>
@@ -33,9 +34,17 @@ namespace app {
 namespace Clusters {
 namespace NetworkCommissioning {
 
+// Instance inherits privately from this class to participate in Instance::sInstances
+class InstanceListNode : public IntrusiveListNodeBase<>
+{
+};
+
 // TODO: Use macro to disable some wifi or thread
 class Instance : public CommandHandlerInterface,
                  public AttributeAccessInterface,
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+                 private InstanceListNode,
+#endif
                  public DeviceLayer::NetworkCommissioning::Internal::BaseDriver::NetworkStatusChangeCallback,
                  public DeviceLayer::NetworkCommissioning::Internal::WirelessDriver::ConnectCallback,
                  public DeviceLayer::NetworkCommissioning::WiFiDriver::ScanCallback,
@@ -81,6 +90,17 @@ class Instance : public CommandHandlerInterface,
     void SendNonConcurrentConnectNetworkResponse();
 #endif
 
+// TODO: This could be guarded by a separate multi-interface condition instead
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    class NetworkInstanceList : public IntrusiveList<InstanceListNode>
+    {
+    public:
+        ~NetworkInstanceList() { this->Clear(); }
+    };
+
+    static NetworkInstanceList sInstances;
+#endif
+
     EndpointId mEndpointId = kInvalidEndpointId;
     const BitFlags<Feature> mFeatureFlags;
 
@@ -111,6 +131,11 @@ class Instance : public CommandHandlerInterface,
     void SetLastNetworkId(ByteSpan lastNetworkId);
     void ReportNetworksListChanged() const;
 
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    // Disconnect if the current connection is not in the Networks list
+    void DisconnectLingeringConnection();
+#endif
+
     // Commits the breadcrumb value saved in mCurrentOperationBreadcrumb to the breadcrumb attribute in GeneralCommissioning
     // cluster. Will set mCurrentOperationBreadcrumb to NullOptional.
     void CommitSavedBreadcrumb();
@@ -133,7 +158,15 @@ class Instance : public CommandHandlerInterface,
     Instance(EndpointId aEndpointId, DeviceLayer::NetworkCommissioning::WiFiDriver * apDelegate);
     Instance(EndpointId aEndpointId, DeviceLayer::NetworkCommissioning::ThreadDriver * apDelegate);
     Instance(EndpointId aEndpointId, DeviceLayer::NetworkCommissioning::EthernetDriver * apDelegate);
-    virtual ~Instance() = default;
+    virtual ~Instance()
+    {
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+        if (IsInList())
+        {
+            sInstances.Remove(this);
+        }
+#endif
+    }
 };
 
 // NetworkDriver for the devices that don't have / don't need a real network driver.

src/include/platform/NetworkCommissioning.h

@@ -247,6 +247,13 @@ class WirelessDriver : public Internal::BaseDriver
      * called inside ConnectNetwork.
      */
     virtual void ConnectNetwork(ByteSpan networkId, ConnectCallback * callback) = 0;
+
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    /**
+     * @brief Disconnect from network, if currently connected.
+     */
+    virtual CHIP_ERROR DisconnectFromNetwork() { return CHIP_ERROR_NOT_IMPLEMENTED; }
+#endif
 };
 } // namespace Internal
 

src/platform/ESP32/NetworkCommissioningDriver.cpp

@@ -264,6 +264,18 @@ CHIP_ERROR ESPWiFiDriver::ConnectWiFiNetwork(const char * ssid, uint8_t ssidLen,
     return ConnectivityMgr().SetWiFiStationMode(ConnectivityManager::kWiFiStationMode_Enabled);
 }
 
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+CHIP_ERROR ESPWiFiDriver::DisconnectFromNetwork()
+{
+    if (chip::DeviceLayer::Internal::ESP32Utils::IsStationProvisioned())
+    {
+        // Attaching to an empty network will disconnect the network.
+        ReturnErrorOnFailure(ConnectWiFiNetwork(nullptr, 0, nullptr, 0));
+    }
+    return CHIP_NO_ERROR;
+}
+#endif
+
 void ESPWiFiDriver::OnConnectWiFiNetwork()
 {
     if (mpConnectCallback)

src/platform/ESP32/NetworkCommissioningDriver.h

@@ -107,6 +107,9 @@ class ESPWiFiDriver final : public WiFiDriver
     Status RemoveNetwork(ByteSpan networkId, MutableCharSpan & outDebugText, uint8_t & outNetworkIndex) override;
     Status ReorderNetwork(ByteSpan networkId, uint8_t index, MutableCharSpan & outDebugText) override;
     void ConnectNetwork(ByteSpan networkId, ConnectCallback * callback) override;
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    CHIP_ERROR DisconnectFromNetwork() override;
+#endif
 
     // WiFiDriver
     Status AddOrUpdateNetwork(ByteSpan ssid, ByteSpan credentials, MutableCharSpan & outDebugText,

src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp

@@ -277,6 +277,19 @@ void GenericThreadDriver::CheckInterfaceEnabled()
 #endif
 }
 
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+CHIP_ERROR GenericThreadDriver::DisconnectFromNetwork()
+{
+    if (ThreadStackMgrImpl().IsThreadProvisioned())
+    {
+        Thread::OperationalDataset emptyNetwork = {};
+        // Attach to an empty network will disconnect the driver.
+        ReturnErrorOnFailure(ThreadStackMgrImpl().AttachToThreadNetwork(emptyNetwork, nullptr));
+    }
+    return CHIP_NO_ERROR;
+}
+#endif
+
 size_t GenericThreadDriver::ThreadNetworkIterator::Count()
 {
     return driver->mStagingNetwork.IsCommissioned() ? 1 : 0;

src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.h

@@ -110,6 +110,9 @@ class GenericThreadDriver final : public ThreadDriver
     Status RemoveNetwork(ByteSpan networkId, MutableCharSpan & outDebugText, uint8_t & outNetworkIndex) override;
     Status ReorderNetwork(ByteSpan networkId, uint8_t index, MutableCharSpan & outDebugText) override;
     void ConnectNetwork(ByteSpan networkId, ConnectCallback * callback) override;
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    CHIP_ERROR DisconnectFromNetwork() override;
+#endif
 
     // ThreadDriver
     Status AddOrUpdateNetwork(ByteSpan operationalDataset, MutableCharSpan & outDebugText, uint8_t & outNetworkIndex) override;