Non concurrent mode now sends connect network response as per spec (project-chip#31739)

* Update Non-concurrent mode to latest spec (project-chip#31660)

* Handle unused variable (project-chip#31660)

* Restyled by clang-format

* Trigger from BTP, use regular member (project-chip#31660)

* Fix override (project-chip#31660)

* Restyled by whitespace

* Restyled by clang-format

* Use generic OperationalNetworkStarted (project-chip#31660)

* Replace variable with mState (project-chip#31660)
Use mState instead of mTerminateOnPacketTxComplete

* Sorted namespace out (project-chip#31660)

* Restyled by clang-format

* Address review comments (project-chip#31660)ffffffffffffffffffff

Close GATT so that BlueZ will disconnect.
Add kOperationalNetworkStarted event type

* Restyled by clang-format

* Access BleLayer mState through function (project-chip#31660)

* Restyled by clang-format

---------

Co-authored-by: Restyled.io <commits@restyled.io>

Author: simonhmorris1

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

@@ -34,6 +34,7 @@
 #include <lib/support/SortUtils.h>
 #include <lib/support/ThreadOperationalDataset.h>
 #include <platform/CHIPDeviceConfig.h>
+#include <platform/ConnectivityManager.h>
 #include <platform/DeviceControlServer.h>
 #include <platform/PlatformManager.h>
 #include <platform/internal/DeviceNetworkInfo.h>
@@ -135,6 +136,26 @@ void Instance::Shutdown()
     mpBaseDriver->Shutdown();
 }
 
+#if !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+void Instance::SendNonConcurrentConnectNetworkResponse()
+{
+    auto commandHandleRef = std::move(mAsyncCommandHandle);
+    auto commandHandle    = commandHandleRef.Get();
+    if (commandHandle == nullptr)
+    {
+        return;
+    }
+
+#if CONFIG_NETWORK_LAYER_BLE
+    DeviceLayer::ConnectivityMgr().GetBleLayer()->IndicateBleClosing();
+#endif // CONFIG_NETWORK_LAYER_BLE
+    ChipLogProgress(NetworkProvisioning, "Non-concurrent mode. Send ConnectNetworkResponse(Success)");
+    Commands::ConnectNetworkResponse::Type response;
+    response.networkingStatus = NetworkCommissioning::Status::kSuccess;
+    commandHandle->AddResponse(mPath, response);
+}
+#endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+
 void Instance::InvokeCommand(HandlerContext & ctxt)
 {
     if (mAsyncCommandHandle.Get() != nullptr)
@@ -177,12 +198,7 @@ void Instance::InvokeCommand(HandlerContext & ctxt)
 
     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;
@@ -708,18 +724,22 @@ void Instance::HandleConnectNetwork(HandlerContext & ctx, const Commands::Connec
     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);
+#else
+    // In Non-concurrent mode postpone the final execution of ConnectNetwork until the operational
+    // network has been fully brought up and kOperationalNetworkStarted is delivered.
+    // mConnectingNetworkIDLen and mConnectingNetworkID contain the received SSID
+    // As per spec, send the ConnectNetworkResponse(Success) prior to releasing the commissioning channel
+    SendNonConcurrentConnectNetworkResponse();
 #endif
 }
 
 void Instance::HandleNonConcurrentConnectNetwork()
 {
     ByteSpan nonConcurrentNetworkID = ByteSpan(mConnectingNetworkID, mConnectingNetworkIDLen);
-    ChipLogProgress(NetworkProvisioning, "HandleNonConcurrentConnectNetwork() SSID=%s", mConnectingNetworkID);
+    ChipLogProgress(NetworkProvisioning, "Non-concurrent mode, Connect to Network SSID=%.*s", mConnectingNetworkIDLen,
+                    mConnectingNetworkID);
     mpWirelessDriver->ConnectNetwork(nonConcurrentNetworkID, this);
 }
 
@@ -826,13 +846,19 @@ void Instance::HandleQueryIdentity(HandlerContext & ctx, const Commands::QueryId
 void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t interfaceStatus)
 {
     auto commandHandleRef = std::move(mAsyncCommandHandle);
-    auto commandHandle    = commandHandleRef.Get();
+
+    // In Non-concurrent mode the commandHandle will be null here, the ConnectNetworkResponse
+    // has already been sent and the BLE will have been stopped, however the other functionality
+    // is still required
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    auto commandHandle = commandHandleRef.Get();
     if (commandHandle == nullptr)
     {
         // When the platform shutted down, interaction model engine will invalidate all commandHandle to avoid dangling references.
         // We may receive the callback after it and should make it noop.
         return;
     }
+#endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
 
     Commands::ConnectNetworkResponse::Type response;
     response.networkingStatus = commissioningError;
@@ -856,13 +882,10 @@ 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
+#if CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
     commandHandle->AddResponse(mPath, response);
-#endif
+#endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+
     if (commissioningError == Status::kSuccess)
     {
         CommitSavedBreadcrumb();
@@ -1063,8 +1086,11 @@ void Instance::OnPlatformEventHandler(const DeviceLayer::ChipDeviceEvent * event
     {
         this_->OnFailSafeTimerExpired();
     }
-    else if (event->Type == DeviceLayer::DeviceEventType::kWiFiDeviceAvailable)
+    else if ((event->Type == DeviceLayer::DeviceEventType::kWiFiDeviceAvailable) ||
+             (event->Type == DeviceLayer::DeviceEventType::kOperationalNetworkStarted))
+
     {
+        // In Non-Concurrent mode connect the operational channel, as BLE has been stopped
         this_->HandleNonConcurrentConnectNetwork();
     }
 }

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

@@ -78,6 +78,9 @@ class Instance : public CommandHandlerInterface,
     void OnCommissioningComplete();
     void OnFailSafeTimerExpired();
 
+#if !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    void SendNonConcurrentConnectNetworkResponse();
+#endif
     const BitFlags<Feature> mFeatureFlags;
 
     DeviceLayer::NetworkCommissioning::Internal::WirelessDriver * const mpWirelessDriver;

src/app/server/CommissioningWindowManager.cpp

@@ -89,8 +89,8 @@ void CommissioningWindowManager::OnPlatformEvent(const DeviceLayer::ChipDeviceEv
 #if CONFIG_NETWORK_LAYER_BLE
     else if (event->Type == DeviceLayer::DeviceEventType::kCloseAllBleConnections)
     {
-        ChipLogProgress(AppServer, "Received kCloseAllBleConnections");
-        mServer->GetBleLayerObject()->CloseAllBleConnections();
+        ChipLogProgress(AppServer, "Received kCloseAllBleConnections:%d", static_cast<int>(event->Type));
+        mServer->GetBleLayerObject()->Shutdown();
     }
 #endif
 }

src/ble/BLEEndPoint.cpp

@@ -1040,6 +1040,7 @@ CHIP_ERROR BLEEndPoint::DriveSending()
         else
         {
             // Nothing to send!
+            mBle->mApplicationDelegate->CheckNonConcurrentBleClosing();
         }
     }
 

src/ble/BleApplicationDelegate.h

@@ -42,6 +42,10 @@ class DLL_EXPORT BleApplicationDelegate
     // The application can use this callback to e.g. close the underlying BLE connection if it is no longer needed,
     // decrement the connection's refcount if it has one, or perform any other sort of cleanup as desired.
     virtual void NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT connObj) = 0;
+
+    // Called to determine whether the BLE connection should be closed when in Non-concurrent mode if sending
+    // ConnectNetworkResponse. The BTP will be in kState_Complete when all fragments have been sent.
+    virtual void CheckNonConcurrentBleClosing() {}
 };
 
 } /* namespace Ble */

src/ble/BleLayer.cpp

@@ -300,6 +300,11 @@ CHIP_ERROR BleLayer::Init(BlePlatformDelegate * platformDelegate, BleApplication
     return Init(platformDelegate, nullptr, appDelegate, systemLayer);
 }
 
+void BleLayer::IndicateBleClosing()
+{
+    mState = kState_Disconnecting;
+}
+
 void BleLayer::Shutdown()
 {
     mState = kState_NotInitialized;

src/ble/BleLayer.h

@@ -226,7 +226,8 @@ class DLL_EXPORT BleLayer
     enum
     {
         kState_NotInitialized = 0,
-        kState_Initialized    = 1
+        kState_Initialized    = 1,
+        kState_Disconnecting  = 2
     } mState; ///< [READ-ONLY] Current state
 
     // This app state is not used by ble transport etc, it will be used by external ble implementation like Android
@@ -243,6 +244,7 @@ class DLL_EXPORT BleLayer
                     chip::System::Layer * systemLayer);
     CHIP_ERROR Init(BlePlatformDelegate * platformDelegate, BleConnectionDelegate * connDelegate,
                     BleApplicationDelegate * appDelegate, chip::System::Layer * systemLayer);
+    void IndicateBleClosing();
     void Shutdown();
 
     CHIP_ERROR CancelBleIncompleteConnection();

src/controller/CHIPDeviceController.cpp

@@ -1636,27 +1636,6 @@ 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;
@@ -3061,28 +3040,9 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         request.breadcrumb.Emplace(breadcrumb);
 
         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;
-            }
-        }
+                        params.GetSupportsConcurrentConnection().Value());
+        err = SendCommand(proxy, request, OnConnectNetworkResponse, OnBasicFailure, endpoint, timeout);
 
         if (err != CHIP_NO_ERROR)
         {

src/include/platform/CHIPDeviceEvent.h

@@ -169,9 +169,10 @@ enum PublicEventTypes
 
     /**
      * When supportsConcurrentConnection = False, the ConnectNetwork command cannot start until
-     * the BLE device is closed and the WiFi device has been started.
+     * the BLE device is closed and the Operation Network device (e.g. WiFi) has been started.
      */
     kWiFiDeviceAvailable,
+    kOperationalNetworkStarted,
 
     /**
      * Thread State Change

src/include/platform/DeviceControlServer.h

@@ -38,6 +38,7 @@ class DeviceControlServer final
     CHIP_ERROR PostConnectedToOperationalNetworkEvent(ByteSpan networkID);
     CHIP_ERROR PostCloseAllBLEConnectionsToOperationalNetworkEvent();
     CHIP_ERROR PostWiFiDeviceAvailableNetworkEvent();
+    CHIP_ERROR PostOperationalNetworkStartedEvent();
     static DeviceControlServer & DeviceControlSvr();
 
 private:

src/platform/DeviceControlServer.cpp

@@ -87,5 +87,12 @@ CHIP_ERROR DeviceControlServer::PostWiFiDeviceAvailableNetworkEvent()
     return PlatformMgr().PostEvent(&event);
 }
 
+CHIP_ERROR DeviceControlServer::PostOperationalNetworkStartedEvent()
+{
+    ChipDeviceEvent event;
+    event.Type = DeviceEventType::kOperationalNetworkStarted;
+    return PlatformMgr().PostEvent(&event);
+}
+
 } // namespace DeviceLayer
 } // namespace chip

src/platform/Linux/BLEManagerImpl.cpp

@@ -43,6 +43,10 @@
 
 #include "bluez/BluezEndpoint.h"
 
+#if !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+#include <platform/DeviceControlServer.h>
+#endif
+
 using namespace ::nl;
 using namespace ::chip::Ble;
 
@@ -650,8 +654,23 @@ void BLEManagerImpl::NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT conId)
 {
     ChipLogProgress(Ble, "Got notification regarding chip connection closure");
 #if CHIP_DEVICE_CONFIG_ENABLE_WPA && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
-    // In Non-Concurrent mode start the Wi-Fi, as BLE has been stopped
-    DeviceLayer::ConnectivityMgrImpl().StartNonConcurrentWiFiManagement();
+    if (mState == kState_NotInitialized)
+    {
+        // Close BLE GATT connections to disconnect BlueZ
+        CloseConnection(conId);
+        // In Non-Concurrent mode start the Wi-Fi, as BLE has been stopped
+        DeviceLayer::ConnectivityMgrImpl().StartNonConcurrentWiFiManagement();
+    }
+#endif // CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+}
+
+void BLEManagerImpl::CheckNonConcurrentBleClosing()
+{
+#if CHIP_DEVICE_CONFIG_ENABLE_WPA && !CHIP_DEVICE_CONFIG_SUPPORTS_CONCURRENT_CONNECTION
+    if (mState == kState_Disconnecting)
+    {
+        DeviceLayer::DeviceControlServer::DeviceControlSvr().PostCloseAllBLEConnectionsToOperationalNetworkEvent();
+    }
 #endif
 }
 

src/platform/Linux/BLEManagerImpl.h

@@ -133,6 +133,7 @@ class BLEManagerImpl final : public BLEManager,
     // ===== Members that implement virtual methods on BleApplicationDelegate.
 
     void NotifyChipConnectionClosed(BLE_CONNECTION_OBJECT conId) override;
+    void CheckNonConcurrentBleClosing() override;
 
     // ===== Members that implement virtual methods on BleConnectionDelegate.
 

src/platform/Linux/ConnectivityManagerImpl.cpp

@@ -774,7 +774,7 @@ void ConnectivityManagerImpl::StartNonConcurrentWiFiManagement()
     {
         if (IsWiFiManagementStarted())
         {
-            DeviceControlServer::DeviceControlSvr().PostWiFiDeviceAvailableNetworkEvent();
+            DeviceControlServer::DeviceControlSvr().PostOperationalNetworkStartedEvent();
             ChipLogProgress(DeviceLayer, "Non-concurrent mode Wi-Fi Management Started.");
             return;
         }