From ef43e5b5f9b5642ebb070e2851b09ffb9e6b9ef1 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Fri, 16 Feb 2024 18:12:22 -0500 Subject: [PATCH] Fix NetworkCommissioning post-review from #32156 (#32172) * Fix NetworkCommissioning post-review from #32156 - Found a regression on Thread scanning. - Changed some ConstraintError to InvalidCommand where more applicable. - Removed an update of cluster state on fail safe expiry. Testing done: - Retested on Wi-Fi - Testing on Thread as well * Re-notify errors on empty network at fail-safe expiry * Fix MobileDeviceTest * Fix Cirque tests --- .../network-commissioning.cpp | 42 ++++++++++++++++++- .../test_scripts/network_commissioning.py | 26 ++++++++---- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp index 13c890a61bd1bb..17b36b2cf963f8 100644 --- a/src/app/clusters/network-commissioning/network-commissioning.cpp +++ b/src/app/clusters/network-commissioning/network-commissioning.cpp @@ -266,7 +266,9 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw } if (ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength) { - ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + // Clients should never use too large a SSID. + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError); + SetLastNetworkingStatusValue(MakeNullable(Status::kUnknownError)); return; } mCurrentOperationBreadcrumb = req.breadcrumb; @@ -276,6 +278,13 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw } else if (mFeatureFlags.Has(NetworkCommissioningFeature::kThreadNetworkInterface)) { + // SSID present on Thread violates the `[WI]` conformance. + if (req.ssid.HasValue()) + { + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + return; + } + mCurrentOperationBreadcrumb = req.breadcrumb; mAsyncCommandHandle = CommandHandler::Handle(&ctx.mCommandHandler); ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand(); @@ -322,6 +331,26 @@ void Instance::HandleAddOrUpdateWiFiNetwork(HandlerContext & ctx, const Commands VerifyOrReturn(CheckFailSafeArmed(ctx)); + if (req.ssid.empty() || req.ssid.size() > DeviceLayer::Internal::kMaxWiFiSSIDLength) + { + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::ConstraintError, "ssid"); + return; + } + + // Presence of a Network Identity indicates we're configuring for Per-Device Credentials + if (req.networkIdentity.HasValue()) + { +#if CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC + if (mFeatureFlags.Has(Feature::kWiFiNetworkInterface)) + { + HandleAddOrUpdateWiFiNetworkWithPDC(ctx, req); + return; + } +#endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC + ctx.mCommandHandler.AddStatus(ctx.mRequestPath, Protocols::InteractionModel::Status::InvalidCommand); + return; + } + // Spec 11.8.8.4 // Valid Credentials length are: // - 0 bytes: Unsecured (open) connection @@ -671,6 +700,17 @@ void Instance::OnFailSafeTimerExpired() ChipLogDetail(Zcl, "Failsafe timeout, tell platform driver to revert network credentials."); mpWirelessDriver->RevertConfiguration(); mAsyncCommandHandle.Release(); + + // Mark the network list changed since `mpWirelessDriver->RevertConfiguration()` may have updated it. + ReportNetworksListChanged(); + + // If no networks are left, clear-out errors; + if (mpBaseDriver && (CountAndRelease(mpBaseDriver->GetNetworks()) == 0)) + { + SetLastNetworkId(ByteSpan{}); + SetLastConnectErrorValue(NullNullable); + SetLastNetworkingStatusValue(NullNullable); + } } CHIP_ERROR Instance::EnumerateAcceptedCommands(const ConcreteClusterPath & cluster, CommandIdCallback callback, void * context) diff --git a/src/controller/python/test/test_scripts/network_commissioning.py b/src/controller/python/test/test_scripts/network_commissioning.py index 1b13928358c0a1..4e41bc94965fc7 100644 --- a/src/controller/python/test/test_scripts/network_commissioning.py +++ b/src/controller/python/test/test_scripts/network_commissioning.py @@ -140,10 +140,16 @@ async def test_wifi(self, endpointId): f"LastNetworkID, LastNetworkingStatus and LastConnectErrorValue should be Null") # Scan networks - logger.info(f"Scan networks") - req = Clusters.NetworkCommissioning.Commands.ScanNetworks( - ssid=b'', breadcrumb=self.with_breadcrumb()) - res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req) + logger.info("Scan networks") + req = Clusters.NetworkCommissioning.Commands.ScanNetworks(breadcrumb=self.with_breadcrumb()) + interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000) + logger.info(f"Request: {req}") + res = await self._devCtrl.SendCommand( + nodeid=self._nodeid, + endpoint=endpointId, + payload=req, + interactionTimeoutMs=interactionTimeoutMs + ) logger.info(f"Received response: {res}") if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess: raise AssertionError(f"Unexpected result: {res.networkingStatus}") @@ -274,10 +280,14 @@ async def test_thread(self, endpointId): f"LastNetworkID, LastNetworkingStatus and LastConnectErrorValue should be Null") # Scan networks - logger.info(f"Scan networks") - req = Clusters.NetworkCommissioning.Commands.ScanNetworks( - ssid=b'', breadcrumb=self.with_breadcrumb()) - res = await self._devCtrl.SendCommand(nodeid=self._nodeid, endpoint=endpointId, payload=req) + logger.info("Scan networks") + req = Clusters.NetworkCommissioning.Commands.ScanNetworks(breadcrumb=self.with_breadcrumb()) + logger.info(f"Request: {req}") + interactionTimeoutMs = self._devCtrl.ComputeRoundTripTimeout(self._nodeid, upperLayerProcessingTimeoutMs=30000) + res = await self._devCtrl.SendCommand(nodeid=self._nodeid, + endpoint=endpointId, + payload=req, + interactionTimeoutMs=interactionTimeoutMs) logger.info(f"Received response: {res}") if res.networkingStatus != Clusters.NetworkCommissioning.Enums.NetworkCommissioningStatus.kSuccess: raise AssertionError(f"Unexpected result: {res.networkingStatus}")