Implement Non concurrent mode (#30201)

* Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Correct intermittent missing SSID(#29865) * ConnectNetworkResponse Supression (#29865) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Fix darwin-tests workflow file syntax. (#30162) There were extra curly braces that failed on some (but not all?) CI runs. * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Only start WiFi on BLE Commissioning pass(#29865) * Remove use of Breadcrumb::Get from BLE (#29865) * Add non-concurrent SupportsConcurrentConnection * Address CI build issues (#29865) * Fix style issue (#29865) * Fix read of SupportsConcurrentConnection (#29865) * Fix merge issue with ICD attributes(#29865) * Only start WiFi on BLE Commissioning pass(#29865) * Remove use of Breadcrumb::Get from BLE (#29865) * Tidy comments (#29865) * Default on error, check timeout, tidy up (#29865) * Make log messages user friendly (#29865) * Change to compile time non-concurrent mode(#29865) * Restyled by whitespace * Restyled by clang-format * Improve code style (#29865) * Restyled by clang-format * Add CommandHandler issue reference 30576 (#29865) --------- Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Restyled.io <[email protected]>
project-chip · May 23, 2024 · 3579584 · 3579584
1 parent d7ba504
commit 3579584
Show file tree

Hide file tree

Showing 14 changed files with 220 additions and 49 deletions.
diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp
@@ -20,6 +20,7 @@
 #include <platform/PlatformManager.h>
 
 #include "app/clusters/network-commissioning/network-commissioning.h"
+#include <app/server/Dnssd.h>
 #include <app/server/OnboardingCodesUtil.h>
 #include <app/server/Server.h>
 #include <app/util/endpoint-config-api.h>
@@ -238,15 +239,15 @@ void InitNetworkCommissioning()
 using chip::Shell::Engine;
 #endif
 
-#if CHIP_DEVICE_CONFIG_ENABLE_WPA
+#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
 /*
  * The device shall check every kWiFiStartCheckTimeUsec whether Wi-Fi management
  * has been fully initialized. If after kWiFiStartCheckAttempts Wi-Fi management
  * still hasn't been initialized, the device configuration is reset, and device
  * needs to be paired again.
  */
-static constexpr useconds_t kWiFiStartCheckTimeUsec = 100 * 1000; // 100 ms
-static constexpr uint8_t kWiFiStartCheckAttempts    = 5;
+static constexpr useconds_t kWiFiStartCheckTimeUsec = WIFI_START_CHECK_TIME_USEC;
+static constexpr uint8_t kWiFiStartCheckAttempts    = WIFI_START_CHECK_ATTEMPTS;
 #endif
 
 namespace {
@@ -264,6 +265,11 @@ void EventHandler(const DeviceLayer::ChipDeviceEvent * event, intptr_t arg)
     {
         ChipLogProgress(DeviceLayer, "Receive kCHIPoBLEConnectionEstablished");
     }
+    else if ((event->Type == chip::DeviceLayer::DeviceEventType::kInternetConnectivityChange))
+    {
+        // Restart the server on connectivity change
+        app::DnssdServer::Instance().StartServer();
+    }
 }
 
 void Cleanup()
@@ -298,7 +304,7 @@ void StopSignalHandler(int signal)
 
 } // namespace
 
-#if CHIP_DEVICE_CONFIG_ENABLE_WPA
+#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
 static bool EnsureWiFiIsStarted()
 {
     for (int cnt = 0; cnt < kWiFiStartCheckAttempts; cnt++)
@@ -462,9 +468,10 @@ int ChipLinuxAppInit(int argc, char * const argv[], OptionSet * customOptions,
     DeviceLayer::ConnectivityMgr().SetBLEAdvertisingEnabled(true);
 #endif
 
-#if CHIP_DEVICE_CONFIG_ENABLE_WPA
+#if CHIP_DEVICE_CONFIG_ENABLE_WPA && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
     if (LinuxDeviceOptions::GetInstance().mWiFi)
     {
+        // Start WiFi management in Concurrent mode
         DeviceLayer::ConnectivityMgrImpl().StartWiFiManagement();
         if (!EnsureWiFiIsStarted())
         {

diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp
@@ -171,11 +171,18 @@ void Instance::InvokeCommand(HandlerContext & ctxt)
             ctxt, [this](HandlerContext & ctx, const auto & req) { HandleRemoveNetwork(ctx, req); });
         return;
 
-    case Commands::ConnectNetwork::Id:
+    case Commands::ConnectNetwork::Id: {
         VerifyOrReturn(mFeatureFlags.Has(Feature::kWiFiNetworkInterface) || mFeatureFlags.Has(Feature::kThreadNetworkInterface));
+#if CONFIG_NETWORK_LAYER_BLE && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+        // If commissionee does not support Concurrent Connections, request the BLE to be stopped.
+        // Start the ConnectNetwork, but this will not complete until the BLE is off.
+        ChipLogProgress(NetworkProvisioning, "Closing BLE connections due to non-concurrent mode");
+        DeviceLayer::DeviceControlServer::DeviceControlSvr().PostCloseAllBLEConnectionsToOperationalNetworkEvent();
+#endif
         HandleCommand<Commands::ConnectNetwork::DecodableType>(
             ctxt, [this](HandlerContext & ctx, const auto & req) { HandleConnectNetwork(ctx, req); });
         return;
+    }
 
     case Commands::ReorderNetwork::Id:
         VerifyOrReturn(mFeatureFlags.Has(Feature::kWiFiNetworkInterface) || mFeatureFlags.Has(Feature::kThreadNetworkInterface));
@@ -623,7 +630,20 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec
     memcpy(mConnectingNetworkID, req.networkID.data(), mConnectingNetworkIDLen);
     mAsyncCommandHandle         = CommandHandler::Handle(&ctx.mCommandHandler);
     mCurrentOperationBreadcrumb = req.breadcrumb;
+
+    // In Non-concurrent mode postpone the final execution of ConnectNetwork until the operational
+    // network has been fully brought up and kWiFiDeviceAvailable is delivered.
+    // mConnectingNetworkIDLen and mConnectingNetworkID contains the received SSID
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
     mpWirelessDriver->ConnectNetwork(req.networkID, this);
+#endif
+}
+
+void Instance::HandleNonConcurrentConnectNetwork()
+{
+    ByteSpan nonConcurrentNetworkID = ByteSpan(mConnectingNetworkID, mConnectingNetworkIDLen);
+    ChipLogProgress(NetworkProvisioning, "HandleNonConcurrentConnectNetwork() SSID=%s", mConnectingNetworkID);
+    mpWirelessDriver->ConnectNetwork(nonConcurrentNetworkID, this);
 }
 
 void Instance::HandleReorderNetwork(HandlerContext & ctx, const Commands::ReorderNetwork::DecodableType & req)
@@ -759,7 +779,13 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i
     memcpy(mLastNetworkID, mConnectingNetworkID, mLastNetworkIDLen);
     mLastNetworkingStatusValue.SetNonNull(commissioningError);
 
+#if CONFIG_NETWORK_LAYER_BLE && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    ChipLogProgress(NetworkProvisioning, "Non-concurrent mode, ConnectNetworkResponse will NOT be sent");
+    // Do not send the ConnectNetworkResponse if in non-concurrent mode
+    // Issue #30576 raised to modify CommandHandler to notify it if no response required
+#else
     commandHandle->AddResponse(mPath, response);
+#endif
     if (commissioningError == Status::kSuccess)
     {
         CommitSavedBreadcrumb();
@@ -956,6 +982,10 @@ void Instance::OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event
     {
         this_->OnFailSafeTimerExpired();
     }
+    else if (event->Type == DeviceLayer::DeviceEventType::kWiFiDeviceAvailable)
+    {
+        this_->HandleNonConcurrentConnectNetwork();
+    }
 }
 
 void Instance::OnCommissioningComplete()

diff --git a/src/app/clusters/network-commissioning/network-commissioning.h b/src/app/clusters/network-commissioning/network-commissioning.h
@@ -117,6 +117,7 @@ class Instance : public CommandHandlerInterface,
     void HandleRemoveNetwork(HandlerContext & ctx, const Commands::RemoveNetwork::DecodableType & req);
     void HandleConnectNetwork(HandlerContext & ctx, const Commands::ConnectNetwork::DecodableType & req);
     void HandleReorderNetwork(HandlerContext & ctx, const Commands::ReorderNetwork::DecodableType & req);
+    void HandleNonConcurrentConnectNetwork(void);
     void HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryIdentity::DecodableType & req);
 
 public:

diff --git a/src/app/server/CommissioningWindowManager.cpp b/src/app/server/CommissioningWindowManager.cpp
@@ -64,7 +64,8 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv
         Cleanup();
         mServer->GetSecureSessionManager().ExpireAllPASESessions();
         // That should have cleared out mPASESession.
-#if CONFIG_NETWORK_LAYER_BLE
+#if CONFIG_NETWORK_LAYER_BLE && CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+        // If in NonConcurrentConnection, this will already have been completed
         mServer->GetBleLayerObject()->CloseAllBleConnections();
 #endif
     }
@@ -82,6 +83,13 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv
         app::DnssdServer::Instance().AdvertiseOperational();
         ChipLogProgress(AppServer, "Operational advertising enabled");
     }
+#if CONFIG_NETWORK_LAYER_BLE
+    else if (event->Type == DeviceLayer::DeviceEventType::kCloseAllBleConnections)
+    {
+        ChipLogProgress(AppServer, "Received kCloseAllBleConnections");
+        mServer->GetBleLayerObject()->CloseAllBleConnections();
+    }
+#endif
 }
 
 void CommissioningWindowManager::Shutdown()

diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -687,35 +687,32 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
             mNeedsDST = false;
             break;
         case CommissioningStage::kReadCommissioningInfo2: {
-            bool shouldReadCommissioningInfo2 =
-                mParams.GetCheckForMatchingFabric() || (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore);
-            if (shouldReadCommissioningInfo2)
+
+            if (!report.Is<ReadCommissioningInfo2>())
             {
-                if (!report.Is<ReadCommissioningInfo2>())
-                {
-                    ChipLogError(
-                        Controller,
-                        "[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG.");
-                }
+                ChipLogError(
+                    Controller,
+                    "[BUG] Should read commissioning info (part 2), but report is not ReadCommissioningInfo2. THIS IS A BUG.");
+            }
 
-                ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
+            ReadCommissioningInfo2 commissioningInfo = report.Get<ReadCommissioningInfo2>();
+            mParams.SetSupportsConcurrentConnection(commissioningInfo.supportsConcurrentConnection);
 
-                if (mParams.GetCheckForMatchingFabric())
+            if (mParams.GetCheckForMatchingFabric())
+            {
+                chip::NodeId nodeId = commissioningInfo.nodeId;
+                if (nodeId != kUndefinedNodeId)
                 {
-                    chip::NodeId nodeId = commissioningInfo.nodeId;
-                    if (nodeId != kUndefinedNodeId)
-                    {
-                        mParams.SetRemoteNodeId(nodeId);
-                    }
+                    mParams.SetRemoteNodeId(nodeId);
                 }
+            }
 
-                if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
+            if (mParams.GetICDRegistrationStrategy() != ICDRegistrationStrategy::kIgnore)
+            {
+                if (commissioningInfo.isIcd)
                 {
-                    if (commissioningInfo.isIcd)
-                    {
-                        mNeedIcdRegistraion = true;
-                        ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
-                    }
+                    mNeedIcdRegistraion = true;
+                    ChipLogDetail(Controller, "AutoCommissioner: Device is ICD");
                 }
             }
             break;

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -1611,6 +1611,27 @@ void OnBasicFailure(void * context, CHIP_ERROR error)
     commissioner->CommissioningStageComplete(error);
 }
 
+static void NonConcurrentTimeout(void * context, CHIP_ERROR error)
+{
+    if (error == CHIP_ERROR_TIMEOUT)
+    {
+        ChipLogProgress(Controller, "Non-concurrent mode: Expected NetworkResponse Timeout, do nothing");
+    }
+    else
+    {
+        ChipLogProgress(Controller, "Non-concurrent mode: Received failure response %" CHIP_ERROR_FORMAT, error.Format());
+    }
+}
+
+static void NonConcurrentNetworkResponse(void * context,
+                                         const NetworkCommissioning::Commands::ConnectNetworkResponse::DecodableType & data)
+{
+    // In Non Concurrent mode the commissioning network should have been shut down and not sent the
+    // ConnectNetworkResponse. In case it does send it this handles the message
+    ChipLogError(Controller, "Non-concurrent Mode : Received Unexpected ConnectNetwork response, ignoring. Status=%u",
+                 to_underlying(data.networkingStatus));
+}
+
 void DeviceCommissioner::CleanupCommissioning(DeviceProxy * proxy, NodeId nodeId, const CompletionStatus & completionStatus)
 {
     commissioningCompletionStatus = completionStatus;
@@ -2111,6 +2132,16 @@ void DeviceCommissioner::ParseCommissioningInfo2()
     ReadCommissioningInfo2 info;
     CHIP_ERROR return_err = CHIP_NO_ERROR;
 
+    using namespace chip::app::Clusters::GeneralCommissioning::Attributes;
+    CHIP_ERROR err =
+        mAttributeCache->Get<SupportsConcurrentConnection::TypeInfo>(kRootEndpointId, info.supportsConcurrentConnection);
+    if (err != CHIP_NO_ERROR)
+    {
+        // May not be present so don't return the error code, non fatal, default concurrent
+        ChipLogError(Controller, "Failed to read SupportsConcurrentConnection: %" CHIP_ERROR_FORMAT, err.Format());
+        info.supportsConcurrentConnection = true;
+    }
+
     return_err = ParseFabrics(info);
 
     if (return_err == CHIP_NO_ERROR)
@@ -2206,6 +2237,8 @@ CHIP_ERROR DeviceCommissioner::ParseICDInfo(ReadCommissioningInfo2 & info)
     }
     else if (err == CHIP_ERROR_KEY_NOT_FOUND)
     {
+        // This key is optional so not an error
+        err        = CHIP_NO_ERROR;
         info.isIcd = false;
     }
     else if (err == CHIP_ERROR_IM_STATUS_CODE_RECEIVED)
@@ -2440,6 +2473,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     case CommissioningStage::kReadCommissioningInfo: {
         ChipLogProgress(Controller, "Sending read request for commissioning information");
         // NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9
+        // See R1.1, 2.11.2 Interaction Model Limits
         app::AttributePathParams readPaths[9];
         // Read all the feature maps for all the networking clusters on any endpoint to determine what is supported
         readPaths[0] = app::AttributePathParams(app::Clusters::NetworkCommissioning::Id,
@@ -2471,7 +2505,14 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     case CommissioningStage::kReadCommissioningInfo2: {
         size_t numberOfAttributes = 0;
         // This is done in a separate step since we've already used up all the available read paths in the previous read step
-        app::AttributePathParams readPaths[9];
+        // NOTE: this array cannot have more than 9 entries, since the spec mandates that server only needs to support 9
+        // See R1.1, 2.11.2 Interaction Model Limits
+        app::AttributePathParams readPaths[3];
+
+        // Mandatory attribute
+        readPaths[numberOfAttributes++] =
+            app::AttributePathParams(endpoint, app::Clusters::GeneralCommissioning::Id,
+                                     app::Clusters::GeneralCommissioning::Attributes::SupportsConcurrentConnection::Id);
 
         // Read the current fabrics
         if (params.GetCheckForMatchingFabric())
@@ -2486,18 +2527,6 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
                 app::AttributePathParams(endpoint, IcdManagement::Id, IcdManagement::Attributes::FeatureMap::Id);
         }
 
-        // Current implementation makes sense when we only have a few attributes to read with conditions. Should revisit this if we
-        // are adding more attributes here.
-
-        if (numberOfAttributes == 0)
-        {
-            // We don't actually want to do this step, so just bypass it
-            ChipLogProgress(Controller, "kReadCommissioningInfo2 step called without parameter set, skipping");
-            CommissioningStageComplete(CHIP_NO_ERROR);
-            return;
-        }
-
-        ChipLogProgress(Controller, "Sending request for commissioning information -- Part 2");
         SendCommissioningReadRequest(proxy, timeout, readPaths, numberOfAttributes);
     }
     break;
@@ -2918,7 +2947,31 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         NetworkCommissioning::Commands::ConnectNetwork::Type request;
         request.networkID = params.GetWiFiCredentials().Value().ssid;
         request.breadcrumb.Emplace(breadcrumb);
-        CHIP_ERROR err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout);
+
+        CHIP_ERROR err = CHIP_NO_ERROR;
+        GeneralCommissioning::Attributes::SupportsConcurrentConnection::TypeInfo::Type supportsConcurrentConnection;
+        supportsConcurrentConnection = params.GetSupportsConcurrentConnection().Value();
+        ChipLogProgress(Controller, "SendCommand kWiFiNetworkEnable, supportsConcurrentConnection=%d",
+                        supportsConcurrentConnection);
+        if (supportsConcurrentConnection)
+        {
+            err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout);
+        }
+        else
+        {
+            // Concurrent Connections not allowed. Send the ConnectNetwork command but do not wait for the
+            // ConnectNetworkResponse on the Commissioning network as it will not be present. Log the expected timeout
+            // and run what would have been in the onConnectNetworkResponse callback.
+            err = SendCommand(proxy, request, NonConcurrentNetworkResponse, NonConcurrentTimeout, endpoint, NullOptional);
+            if (err == CHIP_NO_ERROR)
+            {
+                // As there will be no ConnectNetworkResponse, it is an implicit kSuccess so a default report is fine
+                CommissioningDelegate::CommissioningReport report;
+                CommissioningStageComplete(CHIP_NO_ERROR, report);
+                return;
+            }
+        }
+
         if (err != CHIP_NO_ERROR)
         {
             // We won't get any async callbacks here, so just complete our stage.