Timeouts on clusters (#13562)

* Add plumbing for timeouts / add to commissioning This lets us set a custom timeout for specific command like network enable, where it is reasonable for them to take more than 12 seconds on certain platforms. * zapt file change. * zap generated. ZAP - Y U make so many copies of the same code? * Invoke command plumbing for timeout I don't see this actually being used. Where does InvokeCommand get called from? Anyway, here's some plumbing. May as well save it. * Address review comments * Remove non-default timeout on test. * Add todo to remove the timeout setter * Add comment about timeouts. * Update src/controller/AutoCommissioner.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> * Update src/controller/AutoCommissioner.cpp Co-authored-by: Boris Zbarsky <bzbarsky@apple.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
project-chip · Nov 8, 2023 · 1296536 · 1296536
1 parent e9f7cd8
commit 1296536
Show file tree

Hide file tree

Showing 22 changed files with 308 additions and 272 deletions.
diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp
@@ -60,7 +60,7 @@ CHIP_ERROR CommandSender::AllocateBuffer()
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, System::Clock::Timeout timeout)
+CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, Optional<System::Clock::Timeout> timeout)
 {
     VerifyOrReturnError(mState == State::AddedCommand, CHIP_ERROR_INCORRECT_STATE);
 
@@ -70,7 +70,7 @@ CHIP_ERROR CommandSender::SendCommandRequest(const SessionHandle & session, Syst
     mpExchangeCtx = mpExchangeMgr->NewContext(session, this);
     VerifyOrReturnError(mpExchangeCtx != nullptr, CHIP_ERROR_NO_MEMORY);
 
-    mpExchangeCtx->SetResponseTimeout(timeout);
+    mpExchangeCtx->SetResponseTimeout(timeout.ValueOr(kImMessageTimeout));
 
     if (mTimedInvokeTimeoutMs.HasValue())
     {

diff --git a/src/app/CommandSender.h b/src/app/CommandSender.h
@@ -215,7 +215,7 @@ class CommandSender final : public Messaging::ExchangeDelegate
     // Client can specify the maximum time to wait for response (in milliseconds) via timeout parameter.
     // Default timeout value will be used otherwise.
     //
-    CHIP_ERROR SendCommandRequest(const SessionHandle & session, System::Clock::Timeout timeout = kImMessageTimeout);
+    CHIP_ERROR SendCommandRequest(const SessionHandle & session, Optional<System::Clock::Timeout> timeout = NullOptional);
 
 private:
     friend class TestCommandInteraction;

diff --git a/src/app/DeviceProxy.cpp b/src/app/DeviceProxy.cpp
@@ -39,11 +39,11 @@ using namespace chip::Callback;
 
 namespace chip {
 
-CHIP_ERROR DeviceProxy::SendCommands(app::CommandSender * commandObj)
+CHIP_ERROR DeviceProxy::SendCommands(app::CommandSender * commandObj, Optional<System::Clock::Timeout> timeout)
 {
     VerifyOrReturnLogError(IsSecureConnected(), CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(commandObj != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-    return commandObj->SendCommandRequest(GetSecureSession().Value());
+    return commandObj->SendCommandRequest(GetSecureSession().Value(), timeout);
 }
 
 void DeviceProxy::AddIMResponseHandler(void * commandObj, Callback::Cancelable * onSuccessCallback,

diff --git a/src/app/DeviceProxy.h b/src/app/DeviceProxy.h
@@ -53,7 +53,7 @@ class DLL_EXPORT DeviceProxy
 
     virtual CHIP_ERROR ShutdownSubscriptions() { return CHIP_ERROR_NOT_IMPLEMENTED; }
 
-    virtual CHIP_ERROR SendCommands(app::CommandSender * commandObj);
+    virtual CHIP_ERROR SendCommands(app::CommandSender * commandObj, chip::Optional<System::Clock::Timeout> timeout = NullOptional);
 
     // Interaction model uses the object and callback interface instead of sequence number to mark different transactions.
     virtual void AddIMResponseHandler(void * commandObj, Callback::Cancelable * onSuccessCallback,

diff --git a/src/app/tests/integration/chip_im_initiator.cpp b/src/app/tests/integration/chip_im_initiator.cpp
@@ -249,7 +249,7 @@ CHIP_ERROR SendCommandRequest(std::unique_ptr<chip::app::CommandSender> && comma
     err = commandSender->FinishCommand();
     SuccessOrExit(err);
 
-    err = commandSender->SendCommandRequest(gSession.Get(), gMessageTimeout);
+    err = commandSender->SendCommandRequest(gSession.Get(), chip::MakeOptional(gMessageTimeout));
     SuccessOrExit(err);
 
     gCommandCount++;
@@ -285,7 +285,7 @@ CHIP_ERROR SendBadCommandRequest(std::unique_ptr<chip::app::CommandSender> && co
     err = commandSender->FinishCommand();
     SuccessOrExit(err);
 
-    err = commandSender->SendCommandRequest(gSession.Get(), gMessageTimeout);
+    err = commandSender->SendCommandRequest(gSession.Get(), chip::MakeOptional(gMessageTimeout));
     SuccessOrExit(err);
     gCommandCount++;
     commandSender.release();

diff --git a/src/app/zap-templates/templates/app/CHIPClusters-src.zapt b/src/app/zap-templates/templates/app/CHIPClusters-src.zapt
@@ -65,7 +65,7 @@ CHIP_ERROR {{asUpperCamelCase clusterName}}Cluster::{{asUpperCamelCase name}}(Ca
     // #6308: This is a temporary solution before we fully support IM on application side and should be replaced by IMDelegate.
     mDevice->AddIMResponseHandler(sender.get(), onSuccessCallback, onFailureCallback);
 
-    SuccessOrExit(err = mDevice->SendCommands(sender.get()));
+    SuccessOrExit(err = mDevice->SendCommands(sender.get(), mTimeout));
 
     // We have successfully sent the command, and the callback handler will be responsible to free the object, release the object now.
     sender.release();

diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -137,7 +137,21 @@ void AutoCommissioner::StartCommissioning(CommissioneeDeviceProxy * proxy)
 {
     // TODO: check that there is no commissioning in progress currently.
     mCommissioneeDeviceProxy = proxy;
-    mCommissioner->PerformCommissioningStep(mCommissioneeDeviceProxy, CommissioningStage::kArmFailsafe, mParams, this);
+    mCommissioner->PerformCommissioningStep(mCommissioneeDeviceProxy, CommissioningStage::kArmFailsafe, mParams, this, 0,
+                                            GetCommandTimeout(CommissioningStage::kArmFailsafe));
+}
+
+Optional<System::Clock::Timeout> AutoCommissioner::GetCommandTimeout(CommissioningStage stage)
+{
+    switch (stage)
+    {
+    case CommissioningStage::kWiFiNetworkEnable:
+    case CommissioningStage::kThreadNetworkEnable:
+        return MakeOptional(System::Clock::Timeout(System::Clock::Seconds16(30)));
+    default:
+        // Use default timeout specified in the IM.
+        return NullOptional;
+    }
 }
 
 void AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report)
@@ -159,8 +173,10 @@ void AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, CommissioningDe
         ChipLogError(Controller, "Invalid device for commissioning");
         return;
     }
+
     mParams.SetCompletionStatus(err);
-    mCommissioner->PerformCommissioningStep(proxy, nextStage, mParams, this);
+    // TODO: Get real endpoint
+    mCommissioner->PerformCommissioningStep(proxy, nextStage, mParams, this, 0, GetCommandTimeout(nextStage));
 }
 
 } // namespace Controller

diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h
@@ -36,6 +36,8 @@ class AutoCommissioner : public CommissioningDelegate
 
 private:
     CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR lastErr);
+    Optional<System::Clock::Timeout> GetCommandTimeout(CommissioningStage stage);
+
     DeviceCommissioner * mCommissioner;
     CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr;
     OperationalDeviceProxy * mOperationalDeviceProxy   = nullptr;

diff --git a/src/controller/CHIPCluster.h b/src/controller/CHIPCluster.h
@@ -33,6 +33,7 @@
 #include <controller/ReadInteraction.h>
 #include <controller/WriteInteraction.h>
 #include <lib/core/Optional.h>
+#include <system/SystemClock.h>
 
 namespace chip {
 namespace Controller {
@@ -58,6 +59,9 @@ class DLL_EXPORT ClusterBase
     CHIP_ERROR AssociateWithGroup(DeviceProxy * device, GroupId groupId);
 
     void Dissociate();
+    // Temporary function to set command timeout before we move over to InvokeCommand
+    // TODO: remove when we start using InvokeCommand everywhere
+    void SetCommandTimeout(Optional<System::Clock::Timeout> timeout) { mTimeout = timeout; }
 
     ClusterId GetClusterId() const { return mClusterId; }
 
@@ -84,7 +88,7 @@ class DLL_EXPORT ClusterBase
         };
 
         return InvokeCommandRequest(mDevice->GetExchangeManager(), mDevice->GetSecureSession().Value(), mEndpoint, requestData,
-                                    onSuccessCb, onFailureCb, timedInvokeTimeoutMs);
+                                    onSuccessCb, onFailureCb, timedInvokeTimeoutMs, mTimeout);
     }
 
     template <typename RequestDataT>
@@ -337,6 +341,7 @@ class DLL_EXPORT ClusterBase
     DeviceProxy * mDevice;
     EndpointId mEndpoint;
     SessionHolder mGroupSession;
+    Optional<System::Clock::Timeout> mTimeout;
 };
 
 } // namespace Controller

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -1710,13 +1710,22 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer
     commissioner->mPairingDelegate->OnCommissioningComplete(peerId.GetNodeId(), error);
 }
 
+void DeviceCommissioner::SetupCluster(ClusterBase & base, DeviceProxy * proxy, EndpointId endpoint,
+                                      Optional<System::Clock::Timeout> timeout)
+{
+    base.Associate(proxy, 0);
+    base.SetCommandTimeout(timeout);
+}
+
 void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, CommissioningStage step, CommissioningParameters & params,
-                                                  CommissioningDelegate * delegate)
+                                                  CommissioningDelegate * delegate, EndpointId endpoint,
+                                                  Optional<System::Clock::Timeout> timeout)
 {
     // For now, we ignore errors coming in from the device since not all commissioning clusters are implemented on the device
     // side.
     mCommissioningStage    = step;
     mCommissioningDelegate = delegate;
+    // TODO: Extend timeouts to the DAC and Opcert requests.
 
     // TODO(cecille): We probably want something better than this for breadcrumbs.
     uint64_t breadcrumb = static_cast<uint64_t>(step);
@@ -1727,13 +1736,11 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     switch (step)
     {
     case CommissioningStage::kArmFailsafe: {
-        // TODO(cecille): This is NOT the right way to do this - we should consider attaching an im delegate per command or
-        // something. Per exchange context?
         ChipLogProgress(Controller, "Arming failsafe");
         // TODO(cecille): Find a way to enumerate the clusters here.
         GeneralCommissioningCluster genCom;
         // TODO: should get the endpoint information from the descriptor cluster.
-        genCom.Associate(proxy, 0);
+        SetupCluster(genCom, proxy, endpoint, timeout);
         genCom.ArmFailSafe(mSuccess.Cancel(), mFailure.Cancel(), params.GetFailsafeTimerSeconds(), breadcrumb, kCommandTimeoutMs);
     }
     break;
@@ -1774,7 +1781,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         chip::CharSpan countryCode(countryCodeStr, actualCountryCodeSize);
 
         GeneralCommissioningCluster genCom;
-        genCom.Associate(proxy, 0);
+        SetupCluster(genCom, proxy, endpoint, timeout);
         genCom.SetRegulatoryConfig(mSuccess.Cancel(), mFailure.Cancel(), regulatoryLocation, countryCode, breadcrumb,
                                    kCommandTimeoutMs);
     }
@@ -1825,7 +1832,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         }
         ChipLogProgress(Controller, "Adding wifi network");
         NetworkCommissioningCluster netCom;
-        netCom.Associate(proxy, 0);
+        SetupCluster(netCom, proxy, endpoint, timeout);
         netCom.AddOrUpdateWiFiNetwork(mSuccess.Cancel(), mFailure.Cancel(), params.GetWiFiCredentials().Value().ssid,
                                       params.GetWiFiCredentials().Value().credentials, breadcrumb);
     }
@@ -1840,7 +1847,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         }
         ChipLogProgress(Controller, "Adding thread network");
         NetworkCommissioningCluster netCom;
-        netCom.Associate(proxy, 0);
+        SetupCluster(netCom, proxy, endpoint, timeout);
         netCom.AddOrUpdateThreadNetwork(mSuccess.Cancel(), mFailure.Cancel(), params.GetThreadOperationalDataset().Value(),
                                         breadcrumb);
     }
@@ -1855,7 +1862,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         }
         ChipLogProgress(Controller, "Enabling wifi network");
         NetworkCommissioningCluster netCom;
-        netCom.Associate(proxy, 0);
+        SetupCluster(netCom, proxy, endpoint, timeout);
         netCom.ConnectNetwork(mSuccess.Cancel(), mFailure.Cancel(), params.GetWiFiCredentials().Value().ssid, breadcrumb);
     }
     break;
@@ -1874,7 +1881,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         }
         ChipLogProgress(Controller, "Enabling thread network");
         NetworkCommissioningCluster netCom;
-        netCom.Associate(proxy, 0);
+        SetupCluster(netCom, proxy, endpoint, timeout);
         netCom.ConnectNetwork(mSuccess.Cancel(), mFailure.Cancel(), extendedPanId, breadcrumb);
     }
     break;
@@ -1885,7 +1892,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     case CommissioningStage::kSendComplete: {
         ChipLogProgress(Controller, "Calling commissioning complete");
         GeneralCommissioningCluster genCom;
-        genCom.Associate(proxy, 0);
+        SetupCluster(genCom, proxy, endpoint, timeout);
         genCom.CommissioningComplete(mSuccess.Cancel(), mFailure.Cancel());
     }
     break;

diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -37,6 +37,7 @@
 #include <controller-clusters/zap-generated/CHIPClientCallbacks.h>
 #include <controller/AbstractDnssdDiscoveryController.h>
 #include <controller/AutoCommissioner.h>
+#include <controller/CHIPCluster.h>
 #include <controller/CHIPDeviceControllerSystemState.h>
 #include <controller/CommissioneeDeviceProxy.h>
 #include <controller/CommissioningDelegate.h>
@@ -561,7 +562,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     void RendezvousCleanup(CHIP_ERROR status);
 
     void PerformCommissioningStep(DeviceProxy * device, CommissioningStage step, CommissioningParameters & params,
-                                  CommissioningDelegate * delegate);
+                                  CommissioningDelegate * delegate, EndpointId endpoint, Optional<System::Clock::Timeout> timeout);
 
     void CommissioningStageComplete(CHIP_ERROR err);
 
@@ -677,6 +678,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     uint16_t mUdcListenPort                 = CHIP_UDC_PORT;
 #endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY
 
+    void SetupCluster(ClusterBase & base, DeviceProxy * proxy, EndpointId endpoint, Optional<System::Clock::Timeout> timeout);
+
     void FreeRendezvousSession();
 
     CHIP_ERROR LoadKeyId(PersistentStorageDelegate * delegate, uint16_t & out);

diff --git a/src/controller/CommissioneeDeviceProxy.cpp b/src/controller/CommissioneeDeviceProxy.cpp
@@ -77,12 +77,12 @@ CHIP_ERROR CommissioneeDeviceProxy::LoadSecureSessionParametersIfNeeded(bool & d
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR CommissioneeDeviceProxy::SendCommands(app::CommandSender * commandObj)
+CHIP_ERROR CommissioneeDeviceProxy::SendCommands(app::CommandSender * commandObj, Optional<System::Clock::Timeout> timeout)
 {
     bool loadedSecureSession = false;
     ReturnErrorOnFailure(LoadSecureSessionParametersIfNeeded(loadedSecureSession));
     VerifyOrReturnError(commandObj != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-    return commandObj->SendCommandRequest(mSecureSession.Get());
+    return commandObj->SendCommandRequest(mSecureSession.Get(), timeout);
 }
 
 void CommissioneeDeviceProxy::OnSessionReleased()

diff --git a/src/controller/CommissioneeDeviceProxy.h b/src/controller/CommissioneeDeviceProxy.h
@@ -91,7 +91,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
      * @brief
      *   Send the command in internal command sender.
      */
-    CHIP_ERROR SendCommands(app::CommandSender * commandObj) override;
+    CHIP_ERROR SendCommands(app::CommandSender * commandObj, Optional<System::Clock::Timeout> timeout) override;
 
     /**
      * @brief Get the IP address and port assigned to the device.

diff --git a/src/controller/InvokeInteraction.h b/src/controller/InvokeInteraction.h
@@ -48,7 +48,8 @@ InvokeCommandRequest(Messaging::ExchangeManager * aExchangeMgr, const SessionHan
                      const RequestObjectT & requestCommandData,
                      typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnSuccessCallbackType onSuccessCb,
                      typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnErrorCallbackType onErrorCb,
-                     const Optional<uint16_t> & timedInvokeTimeoutMs)
+                     const Optional<uint16_t> & timedInvokeTimeoutMs,
+                     const Optional<System::Clock::Timeout> & responseTimeout = NullOptional)
 {
     app::CommandPathParams commandPath = { endpointId, 0, RequestObjectT::GetClusterId(), RequestObjectT::GetCommandId(),
                                            (app::CommandPathFlags::kEndpointIdValid) };
@@ -75,7 +76,7 @@ InvokeCommandRequest(Messaging::ExchangeManager * aExchangeMgr, const SessionHan
     VerifyOrReturnError(commandSender != nullptr, CHIP_ERROR_NO_MEMORY);
 
     ReturnErrorOnFailure(commandSender->AddRequestData(commandPath, requestCommandData, timedInvokeTimeoutMs));
-    ReturnErrorOnFailure(commandSender->SendCommandRequest(sessionHandle));
+    ReturnErrorOnFailure(commandSender->SendCommandRequest(sessionHandle, responseTimeout));
 
     //
     // We've effectively transferred ownership of the above allocated objects to CommandSender, and we need to wait for it to call
@@ -95,20 +96,22 @@ InvokeCommandRequest(Messaging::ExchangeManager * exchangeMgr, const SessionHand
                      const RequestObjectT & requestCommandData,
                      typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnSuccessCallbackType onSuccessCb,
                      typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnErrorCallbackType onErrorCb,
-                     uint16_t timedInvokeTimeoutMs)
+                     uint16_t timedInvokeTimeoutMs, const Optional<System::Clock::Timeout> & responseTimeout = NullOptional)
 {
     return InvokeCommandRequest(exchangeMgr, sessionHandle, endpointId, requestCommandData, onSuccessCb, onErrorCb,
-                                MakeOptional(timedInvokeTimeoutMs));
+                                MakeOptional(timedInvokeTimeoutMs), responseTimeout);
 }
 
 template <typename RequestObjectT, typename std::enable_if_t<!RequestObjectT::MustUseTimedInvoke(), int> = 0>
 CHIP_ERROR
 InvokeCommandRequest(Messaging::ExchangeManager * exchangeMgr, const SessionHandle & sessionHandle, chip::EndpointId endpointId,
                      const RequestObjectT & requestCommandData,
                      typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnSuccessCallbackType onSuccessCb,
-                     typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnErrorCallbackType onErrorCb)
+                     typename TypedCommandCallback<typename RequestObjectT::ResponseType>::OnErrorCallbackType onErrorCb,
+                     const Optional<System::Clock::Timeout> & responseTimeout = NullOptional)
 {
-    return InvokeCommandRequest(exchangeMgr, sessionHandle, endpointId, requestCommandData, onSuccessCb, onErrorCb, NullOptional);
+    return InvokeCommandRequest(exchangeMgr, sessionHandle, endpointId, requestCommandData, onSuccessCb, onErrorCb, NullOptional,
+                                responseTimeout);
 }
 
 } // namespace Controller

diff --git a/zzz_generated/all-clusters-app/zap-generated/CHIPClusters.cpp b/zzz_generated/all-clusters-app/zap-generated/CHIPClusters.cpp