[Thread] Improve stability of ScanNetworks command (#21279) (#21353)

* [Thread] Remove unused variable from ThreadDrivers Signed-off-by: Damian Krolik <[email protected]> * [Thread] Improve stability of ScanNetworks command ScanNetworks command sent to a device that has already joined a Thread network causes certain connectivity issues. First of all, the Thread network discovery makes a Sleepy End Device detach from the network, perhaps due to switching between radio channels involved in the discovery mechanism. Secondly, the device fails to deliver an ACK for the command and the controller needlessly keeps sending the command. Fix the first issue, by temporary switching off the SED mode for the duration of the discovery. Fix the second issue by delaying the start of discovery. By the way, remove an unused variable spotted in a few ThreadDriver implementations. Signed-off-by: Damian Krolik <[email protected]> * Replace delay with explicit ACK flush Signed-off-by: Damian Krolik <[email protected]> Co-authored-by: Damian Królik <[email protected]>
project-chip · Jul 28, 2022 · 6126872 · 6126872
1 parent c5e69d7
commit 6126872
Show file tree

Hide file tree

Showing 9 changed files with 33 additions and 29 deletions.
diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp
@@ -271,12 +271,14 @@ void Instance::HandleScanNetworks(HandlerContext & ctx, const Commands::ScanNetw
         }
         mCurrentOperationBreadcrumb = req.breadcrumb;
         mAsyncCommandHandle         = CommandHandler::Handle(&ctx.mCommandHandler);
+        ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand();
         mpDriver.Get<WiFiDriver *>()->ScanNetworks(ssid, this);
     }
     else if (mFeatureFlags.Has(NetworkCommissioningFeature::kThreadNetworkInterface))
     {
         mCurrentOperationBreadcrumb = req.breadcrumb;
         mAsyncCommandHandle         = CommandHandler::Handle(&ctx.mCommandHandler);
+        ctx.mCommandHandler.FlushAcksRightAwayOnSlowCommand();
         mpDriver.Get<ThreadDriver *>()->ScanNetworks(this);
     }
     else

diff --git a/src/platform/Linux/NetworkCommissioningDriver.h b/src/platform/Linux/NetworkCommissioningDriver.h
@@ -104,7 +104,6 @@ class LinuxWiFiDriver final : public WiFiDriver
 
     WiFiNetwork mSavedNetwork;
     WiFiNetwork mStagingNetwork;
-    Optional<Status> mScanStatus;
 };
 #endif // CHIP_DEVICE_CONFIG_ENABLE_WPA
 

diff --git a/src/platform/Linux/NetworkCommissioningWiFiDriver.cpp b/src/platform/Linux/NetworkCommissioningWiFiDriver.cpp
@@ -179,14 +179,8 @@ void LinuxWiFiDriver::ScanNetworks(ByteSpan ssid, WiFiDriver::ScanCallback * cal
     CHIP_ERROR err = DeviceLayer::ConnectivityMgrImpl().StartWiFiScan(ssid, callback);
     if (err != CHIP_NO_ERROR)
     {
-        mScanStatus.SetValue(Status::kUnknownError);
         callback->OnFinished(Status::kUnknownError, CharSpan(), nullptr);
     }
-    else
-    {
-        // On linux platform, once "scan" is started, we can say the result will always be success.
-        mScanStatus.SetValue(Status::kSuccess);
-    }
 }
 
 size_t LinuxWiFiDriver::WiFiNetworkIterator::Count()

diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.cpp
@@ -177,17 +177,10 @@ void GenericThreadDriver::ConnectNetwork(ByteSpan networkId, ConnectCallback * c
 
 void GenericThreadDriver::ScanNetworks(ThreadDriver::ScanCallback * callback)
 {
-    CHIP_ERROR err = DeviceLayer::ThreadStackMgrImpl().StartThreadScan(callback);
-    if (err != CHIP_NO_ERROR)
+    if (DeviceLayer::ThreadStackMgrImpl().StartThreadScan(callback) != CHIP_NO_ERROR)
     {
-        mScanStatus.SetValue(Status::kUnknownError);
         callback->OnFinished(Status::kUnknownError, CharSpan(), nullptr);
     }
-    else
-    {
-        // OpenThread's "scan" will always success once started, so we can set the value of scan result here.
-        mScanStatus.SetValue(Status::kSuccess);
-    }
 }
 
 Status GenericThreadDriver::MatchesNetworkId(const Thread::OperationalDataset & dataset, const ByteSpan & networkId) const

diff --git a/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.h b/src/platform/OpenThread/GenericNetworkCommissioningThreadDriver.h
@@ -27,7 +27,6 @@ template <typename T>
 class otScanResponseIterator : public Iterator<T>
 {
 public:
-    otScanResponseIterator(T * apScanResponse) : mpScanResponse(apScanResponse) {}
     size_t Count() override { return itemCount; }
     bool Next(T & item) override
     {
@@ -62,7 +61,7 @@ class otScanResponseIterator : public Iterator<T>
     size_t currentIterating           = 0;
     size_t itemCount                  = 0;
     static constexpr size_t kItemSize = sizeof(T);
-    T * mpScanResponse;
+    T * mpScanResponse                = nullptr;
 };
 
 class GenericThreadDriver final : public ThreadDriver
@@ -109,7 +108,6 @@ class GenericThreadDriver final : public ThreadDriver
 
     ThreadNetworkIterator mThreadIterator      = ThreadNetworkIterator(this);
     Thread::OperationalDataset mStagingNetwork = {};
-    Optional<Status> mScanStatus;
 };
 
 } // namespace NetworkCommissioning

diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.cpp
@@ -92,8 +92,7 @@ void initNetworkCommissioningThreadDriver(void)
 #endif
 }
 
-NetworkCommissioning::ThreadScanResponse * sScanResult;
-NetworkCommissioning::otScanResponseIterator<NetworkCommissioning::ThreadScanResponse> mScanResponseIter(sScanResult);
+NetworkCommissioning::otScanResponseIterator<NetworkCommissioning::ThreadScanResponse> mScanResponseIter;
 } // namespace
 
 /**
@@ -386,6 +385,9 @@ CHIP_ERROR
 GenericThreadStackManagerImpl_OpenThread<ImplClass>::_StartThreadScan(NetworkCommissioning::ThreadDriver::ScanCallback * callback)
 {
     CHIP_ERROR error = CHIP_NO_ERROR;
+#if CHIP_DEVICE_CONFIG_ENABLE_SED
+    otLinkModeConfig linkMode;
+#endif
 
     // If there is another ongoing scan request, reject the new one.
     VerifyOrReturnError(mpScanCallback == nullptr, CHIP_ERROR_INCORRECT_STATE);
@@ -400,6 +402,18 @@ GenericThreadStackManagerImpl_OpenThread<ImplClass>::_StartThreadScan(NetworkCom
         SuccessOrExit(error = MapOpenThreadError(otIp6SetEnabled(mOTInst, true)));
     }
 
+#if CHIP_DEVICE_CONFIG_ENABLE_SED
+    // Thread network discovery makes Sleepy End Devices detach from a network, so temporarily disable the SED mode.
+    linkMode = otThreadGetLinkMode(mOTInst);
+
+    if (!linkMode.mRxOnWhenIdle)
+    {
+        mTemporaryRxOnWhenIdle = true;
+        linkMode.mRxOnWhenIdle = true;
+        otThreadSetLinkMode(mOTInst, linkMode);
+    }
+#endif
+
     error = MapOpenThreadError(otThreadDiscover(mOTInst, 0,                       /* all channels */
                                                 OT_PANID_BROADCAST, false, false, /* disable PAN ID, EUI64 and Joiner filtering */
                                                 _OnNetworkScanFinished, this));
@@ -426,6 +440,16 @@ void GenericThreadStackManagerImpl_OpenThread<ImplClass>::_OnNetworkScanFinished
 {
     if (aResult == nullptr) // scan completed
     {
+#if CHIP_DEVICE_CONFIG_ENABLE_SED
+        if (mTemporaryRxOnWhenIdle)
+        {
+            otLinkModeConfig linkMode = otThreadGetLinkMode(mOTInst);
+            linkMode.mRxOnWhenIdle    = false;
+            mTemporaryRxOnWhenIdle    = false;
+            otThreadSetLinkMode(mOTInst, linkMode);
+        }
+#endif
+
         // If Thread scanning was done before commissioning, turn off the IPv6 interface.
         if (otThreadGetDeviceRole(mOTInst) == OT_DEVICE_ROLE_DISABLED && !otDatasetIsCommissioned(mOTInst))
         {

diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.h
@@ -150,8 +150,9 @@ class GenericThreadStackManagerImpl_OpenThread
     // ===== Private members for use by this class only.
 
     otInstance * mOTInst;
-    uint64_t mOverrunCount = 0;
-    bool mIsAttached       = false;
+    uint64_t mOverrunCount      = 0;
+    bool mIsAttached            = false;
+    bool mTemporaryRxOnWhenIdle = false;
 
     NetworkCommissioning::ThreadDriver::ScanCallback * mpScanCallback;
     NetworkCommissioning::Internal::WirelessDriver::ConnectCallback * mpConnectCallback;

diff --git a/src/platform/webos/NetworkCommissioningDriver.h b/src/platform/webos/NetworkCommissioningDriver.h
@@ -104,7 +104,6 @@ class LinuxWiFiDriver final : public WiFiDriver
 
     WiFiNetwork mSavedNetwork;
     WiFiNetwork mStagingNetwork;
-    Optional<Status> mScanStatus;
 };
 #endif // CHIP_DEVICE_CONFIG_ENABLE_WPA
 

diff --git a/src/platform/webos/NetworkCommissioningWiFiDriver.cpp b/src/platform/webos/NetworkCommissioningWiFiDriver.cpp
@@ -179,14 +179,8 @@ void LinuxWiFiDriver::ScanNetworks(ByteSpan ssid, WiFiDriver::ScanCallback * cal
     CHIP_ERROR err = DeviceLayer::ConnectivityMgrImpl().StartWiFiScan(ssid, callback);
     if (err != CHIP_NO_ERROR)
     {
-        mScanStatus.SetValue(Status::kUnknownError);
         callback->OnFinished(Status::kUnknownError, CharSpan(), nullptr);
     }
-    else
-    {
-        // On linux platform, once "scan" is started, we can say the result will always be success.
-        mScanStatus.SetValue(Status::kSuccess);
-    }
 }
 
 size_t LinuxWiFiDriver::WiFiNetworkIterator::Count()