Add network retry and case failsafe timer to AutoCommissioner (#23209)

chrisdecenzo · restyled-io[bot] · restyled-commits · pull[bot] · commit 1123570da159 · 2024-02-20T15:45:37.000Z
* Draft: Add network config retry and case failsafe timer to AutoCommissioner * address comments * Address feedback * Restyle Draft: Add network retry and case failsafe timer to AutoCommissioner (#23516) * Restyled by whitespace * Restyled by google-java-format Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>
diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp
@@ -19,13 +19,16 @@
 #include <controller/AutoCommissioner.h>
 
 #include <app/InteractionModelTimeout.h>
+#include <controller-clusters/zap-generated/CHIPClusters.h>
 #include <controller/CHIPDeviceController.h>
 #include <credentials/CHIPCert.h>
 #include <lib/support/SafeInt.h>
 
 namespace chip {
 namespace Controller {
 
+using namespace chip::app::Clusters;
+
 AutoCommissioner::AutoCommissioner()
 {
     SetCommissioningParameters(CommissioningParameters());
@@ -51,6 +54,12 @@ CHIP_ERROR AutoCommissioner::SetCommissioningParameters(const CommissioningParam
         mParams.SetFailsafeTimerSeconds(params.GetFailsafeTimerSeconds().Value());
     }
 
+    if (params.GetCASEFailsafeTimerSeconds().HasValue())
+    {
+        ChipLogProgress(Controller, "Setting CASE failsafe timer from parameters");
+        mParams.SetCASEFailsafeTimerSeconds(params.GetCASEFailsafeTimerSeconds().Value());
+    }
+
     if (params.GetAdminSubject().HasValue())
     {
         ChipLogProgress(Controller, "Setting adminSubject from parameters");
@@ -169,6 +178,27 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStage(CommissioningStag
     return nextStage;
 }
 
+CommissioningStage AutoCommissioner::GetNextCommissioningStageNetworkSetup(CommissioningStage currentStage, CHIP_ERROR & lastErr)
+{
+    if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi.endpoint != kInvalidEndpointId)
+    {
+        return CommissioningStage::kWiFiNetworkSetup;
+    }
+    if (mParams.GetThreadOperationalDataset().HasValue() && mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)
+    {
+        return CommissioningStage::kThreadNetworkSetup;
+    }
+
+    ChipLogError(Controller, "Required network information not provided in commissioning parameters");
+    ChipLogError(Controller, "Parameters supplied: wifi (%s) thread (%s)", mParams.GetWiFiCredentials().HasValue() ? "yes" : "no",
+                 mParams.GetThreadOperationalDataset().HasValue() ? "yes" : "no");
+    ChipLogError(Controller, "Device supports: wifi (%s) thread(%s)",
+                 mDeviceCommissioningInfo.network.wifi.endpoint == kInvalidEndpointId ? "no" : "yes",
+                 mDeviceCommissioningInfo.network.thread.endpoint == kInvalidEndpointId ? "no" : "yes");
+    lastErr = CHIP_ERROR_INVALID_ARGUMENT;
+    return CommissioningStage::kCleanup;
+}
+
 CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(CommissioningStage currentStage, CHIP_ERROR & lastErr)
 {
     if (mStopCommissioning)
@@ -194,27 +224,6 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
         }
         return CommissioningStage::kArmFailsafe;
     case CommissioningStage::kArmFailsafe:
-        if (mNeedsNetworkSetup)
-        {
-            // if there is a WiFi or a Thread endpoint, then perform scan
-            if ((mParams.GetAttemptWiFiNetworkScan().ValueOr(false) &&
-                 mDeviceCommissioningInfo.network.wifi.endpoint != kInvalidEndpointId) ||
-                (mParams.GetAttemptThreadNetworkScan().ValueOr(false) &&
-                 mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId))
-            {
-                return CommissioningStage::kScanNetworks;
-            }
-            ChipLogProgress(Controller, "No NetworkScan enabled or WiFi/Thread endpoint not specified, skipping ScanNetworks");
-        }
-        else
-        {
-            ChipLogProgress(Controller, "Not a BLE connection, skipping ScanNetworks");
-        }
-        // skip scan step
-        return CommissioningStage::kConfigRegulatory;
-    case CommissioningStage::kScanNetworks:
-        return CommissioningStage::kNeedsNetworkCreds;
-    case CommissioningStage::kNeedsNetworkCreds:
         return CommissioningStage::kConfigRegulatory;
     case CommissioningStage::kConfigRegulatory:
         return CommissioningStage::kSendPAICertificateRequest;
@@ -240,34 +249,30 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
         // operational network because the provisioning of certificates will trigger the device to start operational advertising.
         if (mNeedsNetworkSetup)
         {
-            if (mParams.GetWiFiCredentials().HasValue() && mDeviceCommissioningInfo.network.wifi.endpoint != kInvalidEndpointId)
-            {
-                return CommissioningStage::kWiFiNetworkSetup;
-            }
-            if (mParams.GetThreadOperationalDataset().HasValue() &&
-                mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)
+            // if there is a WiFi or a Thread endpoint, then perform scan
+            if (IsScanNeeded())
             {
-                return CommissioningStage::kThreadNetworkSetup;
+                // Perform Scan (kScanNetworks) and collect credentials (kNeedsNetworkCreds) right before configuring network.
+                // This order of steps allows the workflow to return to collect credentials again if network enablement fails.
+                return CommissioningStage::kScanNetworks;
             }
+            ChipLogProgress(Controller, "No NetworkScan enabled or WiFi/Thread endpoint not specified, skipping ScanNetworks");
 
-            ChipLogError(Controller, "Required network information not provided in commissioning parameters");
-            ChipLogError(Controller, "Parameters supplied: wifi (%s) thread (%s)",
-                         mParams.GetWiFiCredentials().HasValue() ? "yes" : "no",
-                         mParams.GetThreadOperationalDataset().HasValue() ? "yes" : "no");
-            ChipLogError(Controller, "Device supports: wifi (%s) thread(%s)",
-                         mDeviceCommissioningInfo.network.wifi.endpoint == kInvalidEndpointId ? "no" : "yes",
-                         mDeviceCommissioningInfo.network.thread.endpoint == kInvalidEndpointId ? "no" : "yes");
-            lastErr = CHIP_ERROR_INVALID_ARGUMENT;
-            return CommissioningStage::kCleanup;
+            return GetNextCommissioningStageNetworkSetup(currentStage, lastErr);
         }
         else
         {
+            SetCASEFailsafeTimerIfNeeded();
             if (mParams.GetSkipCommissioningComplete().ValueOr(false))
             {
                 return CommissioningStage::kCleanup;
             }
             return CommissioningStage::kFindOperational;
         }
+    case CommissioningStage::kScanNetworks:
+        return CommissioningStage::kNeedsNetworkCreds;
+    case CommissioningStage::kNeedsNetworkCreds:
+        return GetNextCommissioningStageNetworkSetup(currentStage, lastErr);
     case CommissioningStage::kWiFiNetworkSetup:
         if (mParams.GetThreadOperationalDataset().HasValue() &&
             mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)
@@ -296,13 +301,16 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
         }
         else if (mParams.GetSkipCommissioningComplete().ValueOr(false))
         {
+            SetCASEFailsafeTimerIfNeeded();
             return CommissioningStage::kCleanup;
         }
         else
         {
+            SetCASEFailsafeTimerIfNeeded();
             return CommissioningStage::kFindOperational;
         }
     case CommissioningStage::kThreadNetworkEnable:
+        SetCASEFailsafeTimerIfNeeded();
         if (mParams.GetSkipCommissioningComplete().ValueOr(false))
         {
             return CommissioningStage::kCleanup;
@@ -321,6 +329,38 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
     return CommissioningStage::kError;
 }
 
+// No specific actions to take when an error happens since this command can fail and commissioning can still succeed.
+static void OnFailsafeFailureForCASE(void * context, CHIP_ERROR error)
+{
+    ChipLogProgress(Controller, "ExtendFailsafe received failure response %s\n", chip::ErrorStr(error));
+}
+
+// No specific actions to take upon success.
+static void
+OnExtendFailsafeSuccessForCASE(void * context,
+                               const app::Clusters::GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data)
+{
+    ChipLogProgress(Controller, "ExtendFailsafe received ArmFailSafe response errorCode=%u", to_underlying(data.errorCode));
+}
+
+void AutoCommissioner::SetCASEFailsafeTimerIfNeeded()
+{
+    // if there is a final fail-safe timer configured then, send it
+    if (mParams.GetCASEFailsafeTimerSeconds().HasValue() && mCommissioneeDeviceProxy != nullptr)
+    {
+        // send the command via the PASE session (mCommissioneeDeviceProxy) since the CASE portion of commissioning
+        // might be done by a different service (ex. PASE is done by a phone app and CASE is done by a Hub).
+        // Also, we want the CASE failsafe timer to apply for the time it takes the Hub to perform operational discovery,
+        // CASE establishment, and receipt of the commissioning complete command.
+        // We know that the mCommissioneeDeviceProxy is still valid at this point since it gets cleared during cleanup
+        // and SetCASEFailsafeTimerIfNeeded is always called before that stage.
+        mCommissioner->ExtendArmFailSafe(mCommissioneeDeviceProxy, CommissioningStage::kFindOperational,
+                                         mParams.GetCASEFailsafeTimerSeconds().Value(),
+                                         GetCommandTimeout(mCommissioneeDeviceProxy, CommissioningStage::kArmFailsafe),
+                                         OnExtendFailsafeSuccessForCASE, OnFailsafeFailureForCASE);
+    }
+}
+
 EndpointId AutoCommissioner::GetEndpoint(const CommissioningStage & stage) const
 {
     switch (stage)
@@ -481,8 +521,23 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
         }
         else if (report.Is<NetworkCommissioningStatusInfo>())
         {
+            // This report type is used when an error happens in either NetworkConfig or ConnectNetwork commands
             completionStatus.networkCommissioningStatus =
                 MakeOptional(report.Get<NetworkCommissioningStatusInfo>().networkCommissioningStatus);
+
+            // If we are configured to scan networks, then don't error out.
+            // Instead, allow the app to try another network.
+            if (IsScanNeeded())
+            {
+                if (completionStatus.err == CHIP_NO_ERROR)
+                {
+                    completionStatus.err = err;
+                }
+                err = CHIP_NO_ERROR;
+                // Walk back the completed stage to kScanNetworks.
+                // This will allow the app to try another network.
+                report.stageCompleted = CommissioningStage::kScanNetworks;
+            }
         }
     }
     else
diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h
@@ -51,6 +51,9 @@ class AutoCommissioner : public CommissioningDelegate
 
 private:
     DeviceProxy * GetDeviceProxyForStep(CommissioningStage nextStage);
+
+    // Adjust the failsafe timer if CommissioningDelegate GetCASEFailsafeTimerSeconds is set
+    void SetCASEFailsafeTimerIfNeeded();
     void ReleaseDAC();
     void ReleasePAI();
 
@@ -69,6 +72,22 @@ class AutoCommissioner : public CommissioningDelegate
     EndpointId GetEndpoint(const CommissioningStage & stage) const;
     CommissioningStage GetNextCommissioningStageInternal(CommissioningStage currentStage, CHIP_ERROR & lastErr);
 
+    // Helper function to determine whether next stage should be kWiFiNetworkSetup,
+    // kThreadNetworkSetup or kCleanup, depending whether network information has
+    // been provided that matches the thread/wifi endpoint of the target.
+    CommissioningStage GetNextCommissioningStageNetworkSetup(CommissioningStage currentStage, CHIP_ERROR & lastErr);
+
+    // Helper function to determine if a scan attempt should be made given the
+    // scan attempt commissioning params and the corresponding network endpoint of
+    // the target.
+    bool IsScanNeeded()
+    {
+        return ((mParams.GetAttemptWiFiNetworkScan().ValueOr(false) &&
+                 mDeviceCommissioningInfo.network.wifi.endpoint != kInvalidEndpointId) ||
+                (mParams.GetAttemptThreadNetworkScan().ValueOr(false) &&
+                 mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId));
+    };
+
     bool mStopCommissioning = false;
 
     DeviceCommissioner * mCommissioner                               = nullptr;
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -1128,6 +1128,18 @@ void DeviceCommissioner::OnFailedToExtendedArmFailSafeDeviceAttestation(void * c
     commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
 }
 
+void DeviceCommissioner::ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
+                                           Optional<System::Clock::Timeout> commandTimeout, OnExtendFailsafeSuccess onSuccess,
+                                           OnExtendFailsafeFailure onFailure)
+{
+    uint64_t breadcrumb = static_cast<uint64_t>(step);
+    GeneralCommissioning::Commands::ArmFailSafe::Type request;
+    request.expiryLengthSeconds = armFailSafeTimeout;
+    request.breadcrumb          = breadcrumb;
+    ChipLogProgress(Controller, "Arming failsafe (%u seconds)", request.expiryLengthSeconds);
+    SendCommand<GeneralCommissioningCluster>(proxy, request, onSuccess, onFailure, kRootEndpointId, commandTimeout);
+}
+
 void DeviceCommissioner::ExtendArmFailSafeForDeviceAttestation(const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
                                                                Credentials::AttestationVerificationResult result)
 {
@@ -2098,11 +2110,9 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
     switch (step)
     {
     case CommissioningStage::kArmFailsafe: {
-        GeneralCommissioning::Commands::ArmFailSafe::Type request;
-        request.expiryLengthSeconds = params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout);
-        request.breadcrumb          = breadcrumb;
-        ChipLogProgress(Controller, "Arming failsafe (%u seconds)", request.expiryLengthSeconds);
-        SendCommand<GeneralCommissioningCluster>(proxy, request, OnArmFailSafe, OnBasicFailure, endpoint, timeout);
+        VerifyOrDie(endpoint == kRootEndpointId);
+        ExtendArmFailSafe(proxy, step, params.GetFailsafeTimerSeconds().ValueOr(kDefaultFailsafeTimeout), timeout, OnArmFailSafe,
+                          OnBasicFailure);
     }
     break;
     case CommissioningStage::kReadCommissioningInfo: {
diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -352,6 +352,13 @@ using UdcTransportMgr = TransportMgr<Transport::UDP /* IPv6 */
                                      >;
 #endif
 
+/**
+ * @brief Callback prototype for ExtendArmFailSafe command.
+ */
+typedef void (*OnExtendFailsafeSuccess)(
+    void * context, const app::Clusters::GeneralCommissioning::Commands::ArmFailSafeResponse::DecodableType & data);
+typedef void (*OnExtendFailsafeFailure)(void * context, CHIP_ERROR error);
+
 /**
  * @brief
  *   The commissioner applications can use this class to pair new/unpaired CHIP devices. The application is
@@ -560,6 +567,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
      * or it may call this method after obtaining network credentials using asyncronous methods (prompting user, cloud API call,
      * etc).
      *
+     * If an error happens in the subsequent network commissioning step (either NetworkConfig or ConnectNetwork commands)
+     * then the DevicePairingDelegate will receive the error in completionStatus.networkCommissioningStatus and the
+     * commissioning stage will return to kNeedsNetworkCreds so that the DevicePairingDelegate can re-attempt with new
+     * network information. The DevicePairingDelegate can exit the commissioning process by calling StopPairing.
+     *
      * @return CHIP_ERROR   The return status. Returns CHIP_ERROR_INCORRECT_STATE if not in the correct state (kNeedsNetworkCreds).
      */
     CHIP_ERROR NetworkCredentialsReady();
@@ -670,6 +682,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
         return mDefaultCommissioner == nullptr ? NullOptional : MakeOptional(mDefaultCommissioner->GetCommissioningParameters());
     }
 
+    // Reset the arm failsafe timer during commissioning.
+    void ExtendArmFailSafe(DeviceProxy * proxy, CommissioningStage step, uint16_t armFailSafeTimeout,
+                           Optional<System::Clock::Timeout> commandTimeout, OnExtendFailsafeSuccess onSuccess,
+                           OnExtendFailsafeFailure onFailure);
+
 private:
     DevicePairingDelegate * mPairingDelegate;
 
diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h
@@ -56,6 +56,9 @@ enum CommissioningStage : uint8_t
     /// ScanNetworks can happen anytime after kArmFailsafe.
     /// However, the cirque tests fail if it is earlier in the list
     kScanNetworks,
+    /// Waiting for the higher layer to provide network credentials before continuing the workflow.
+    /// Call CHIPDeviceController::NetworkCredentialsReady() when CommissioningParameters is populated with
+    /// network credentials to use in kWiFiNetworkSetup or kThreadNetworkSetup steps.
     kNeedsNetworkCreds,
 };
 
@@ -105,6 +108,15 @@ class CommissioningParameters
     // This value should be set before running PerformCommissioningStep for the kArmFailsafe step.
     const Optional<uint16_t> GetFailsafeTimerSeconds() const { return mFailsafeTimerSeconds; }
 
+    // Value to use when re-setting the commissioning failsafe timer immediately prior to operational discovery.
+    // If a CASE failsafe timer value is passed in as part of the commissioning parameters, then the failsafe timer
+    // will be reset using this value before operational discovery begins. If not supplied, then the AutoCommissioner
+    // will not automatically reset the failsafe timer before operational discovery begins. It can be useful for the
+    // commissioner to set the CASE failsafe timer to a small value (ex. 30s) when the regular failsafe timer is set
+    // to a larger value to accommodate user interaction during setup (network credential selection, user consent
+    // after device attestation).
+    const Optional<uint16_t> GetCASEFailsafeTimerSeconds() const { return mCASEFailsafeTimerSeconds; }
+
     // The location (indoor/outdoor) of the node being commissioned.
     // The node regulartory location (indoor/outdoor) should be set by the commissioner explicitly as it may be different than the
     // location of the commissioner. This location will be set on the node if the node supports configurable regulatory location
@@ -237,6 +249,12 @@ class CommissioningParameters
         return *this;
     }
 
+    CommissioningParameters & SetCASEFailsafeTimerSeconds(uint16_t seconds)
+    {
+        mCASEFailsafeTimerSeconds.SetValue(seconds);
+        return *this;
+    }
+
     CommissioningParameters & SetDeviceRegulatoryLocation(app::Clusters::GeneralCommissioning::RegulatoryLocationType location)
     {
         mDeviceRegulatoryLocation.SetValue(location);
@@ -410,6 +428,7 @@ class CommissioningParameters
 private:
     // Items that can be set by the commissioner
     Optional<uint16_t> mFailsafeTimerSeconds;
+    Optional<uint16_t> mCASEFailsafeTimerSeconds;
     Optional<app::Clusters::GeneralCommissioning::RegulatoryLocationType> mDeviceRegulatoryLocation;
     Optional<ByteSpan> mCSRNonce;
     Optional<ByteSpan> mAttestationNonce;
diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp
diff --git a/src/controller/java/src/chip/devicecontroller/ControllerParams.java b/src/controller/java/src/chip/devicecontroller/ControllerParams.java
