Issue 21314 - add proper step to commissioner for obtaining network c…

…redentials (project-chip#21876)

* Draft: Address 21314 - add proper step to commissioner for obtaining network credentials

* android stragglers

src/controller/AutoCommissioner.cpp

@@ -203,6 +203,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio
         // skip scan step
         return CommissioningStage::kConfigRegulatory;
     case CommissioningStage::kScanNetworks:
+        return CommissioningStage::kNeedsNetworkCreds;
+    case CommissioningStage::kNeedsNetworkCreds:
         return CommissioningStage::kConfigRegulatory;
     case CommissioningStage::kConfigRegulatory:
         return CommissioningStage::kSendPAICertificateRequest;
@@ -536,17 +538,6 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio
     }
     mParams.SetCompletionStatus(completionStatus);
 
-    if (mCommissioningPaused)
-    {
-        mPausedStage = nextStage;
-
-        if (GetDeviceProxyForStep(nextStage) == nullptr)
-        {
-            ChipLogError(Controller, "Invalid device for commissioning");
-            return CHIP_ERROR_INCORRECT_STATE;
-        }
-        return CHIP_NO_ERROR;
-    }
     return PerformStep(nextStage);
 }
 
@@ -574,28 +565,6 @@ CHIP_ERROR AutoCommissioner::PerformStep(CommissioningStage nextStage)
     return CHIP_NO_ERROR;
 }
 
-void AutoCommissioner::PauseCommissioning()
-{
-    mCommissioningPaused = true;
-}
-
-CHIP_ERROR AutoCommissioner::ResumeCommissioning()
-{
-    VerifyOrReturnError(mCommissioningPaused, CHIP_ERROR_INCORRECT_STATE);
-    mCommissioningPaused = false;
-
-    // if no new step was attempted
-    if (mPausedStage == CommissioningStage::kError)
-    {
-        return CHIP_NO_ERROR;
-    }
-
-    CommissioningStage nextStage = mPausedStage;
-    mPausedStage                 = CommissioningStage::kError;
-
-    return PerformStep(nextStage);
-}
-
 void AutoCommissioner::ReleaseDAC()
 {
     if (mDAC != nullptr)

src/controller/AutoCommissioner.h

@@ -38,25 +38,6 @@ class AutoCommissioner : public CommissioningDelegate
 
     CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override;
 
-    /**
-     * @brief
-     *   This function puts the AutoCommissioner in a paused state to prevent advancing to the next stage.
-     * It is expected that a DevicePairingDelegate may call this method when processing the
-     * OnCommissioningStatusUpdate, for example, in order to obtain network credentials from the user based
-     * upon the results of the NetworkScan.
-     * Use ResumeCommissioning to continue the commissioning process.
-     *
-     */
-    void PauseCommissioning();
-
-    /**
-     * @brief
-     *   An error return value means resume failed, for example:
-     *   - AutoCommissioner was not in a paused state.
-     *   - AutoCommissioner was unable to continue (no DeviceProxy)
-     */
-    CHIP_ERROR ResumeCommissioning();
-
 protected:
     CommissioningStage GetNextCommissioningStage(CommissioningStage currentStage, CHIP_ERROR & lastErr);
     DeviceCommissioner * GetCommissioner() { return mCommissioner; }
@@ -96,9 +77,6 @@ class AutoCommissioner : public CommissioningDelegate
     bool mNeedsNetworkSetup = false;
     ReadCommissioningInfo mDeviceCommissioningInfo;
 
-    CommissioningStage mPausedStage = CommissioningStage::kError;
-    bool mCommissioningPaused       = false;
-
     // TODO: Why were the nonces statically allocated, but the certs dynamically allocated?
     uint8_t * mDAC   = nullptr;
     uint16_t mDACLen = 0;

src/controller/CHIPDeviceController.cpp

@@ -1833,13 +1833,15 @@ void DeviceCommissioner::OnScanNetworksFailure(void * context, CHIP_ERROR error)
     ChipLogProgress(Controller, "Received ScanNetworks failure response %" CHIP_ERROR_FORMAT, error.Format());
 
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
+
+    // advance to the kNeedsNetworkCreds waiting step
+    // clear error so that we don't abort the commissioning when ScanNetworks fails
+    commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
+
     if (commissioner->GetPairingDelegate() != nullptr)
     {
         commissioner->GetPairingDelegate()->OnScanNetworksFailure(error);
     }
-    // need to advance to next step
-    // clear error so that we don't abort the commissioning when ScanNetworks fails
-    commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
 }
 
 void DeviceCommissioner::OnScanNetworksResponse(void * context,
@@ -1853,12 +1855,23 @@ void DeviceCommissioner::OnScanNetworksResponse(void * context,
                                                : "none provided"));
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
 
+    // advance to the kNeedsNetworkCreds waiting step
+    commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
+
     if (commissioner->GetPairingDelegate() != nullptr)
     {
         commissioner->GetPairingDelegate()->OnScanNetworksSuccess(data);
     }
+}
+
+CHIP_ERROR DeviceCommissioner::NetworkCredentialsReady()
+{
+    ReturnErrorCodeIf(mCommissioningStage != CommissioningStage::kNeedsNetworkCreds, CHIP_ERROR_INCORRECT_STATE);
+
     // need to advance to next step
-    commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
+    CommissioningStageComplete(CHIP_NO_ERROR);
+
+    return CHIP_NO_ERROR;
 }
 
 void DeviceCommissioner::OnNetworkConfigResponse(void * context,
@@ -2000,6 +2013,11 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
         SendCommand<NetworkCommissioningCluster>(proxy, request, OnScanNetworksResponse, OnScanNetworksFailure, endpoint, timeout);
         break;
     }
+    case CommissioningStage::kNeedsNetworkCreds: {
+        // nothing to do, the OnScanNetworksSuccess and OnScanNetworksFailure callbacks provide indication to the
+        // DevicePairingDelegate that network credentials are needed.
+        break;
+    }
     case CommissioningStage::kConfigRegulatory: {
         // To set during config phase:
         // UTC time

src/controller/CHIPDeviceController.h

@@ -547,6 +547,20 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     CommissioningStageComplete(CHIP_ERROR err,
                                CommissioningDelegate::CommissioningReport report = CommissioningDelegate::CommissioningReport());
 
+    /**
+     * @brief
+     *   This function is called by the DevicePairingDelegate to indicate that network credentials have been set
+     * on the CommissioningParameters of the CommissioningDelegate using CommissioningDelegate.SetCommissioningParameters().
+     * As a result, commissioning can advance to the next stage.
+     *
+     * The DevicePairingDelegate may call this method from the OnScanNetworksSuccess and OnScanNetworksFailure callbacks,
+     * or it may call this method after obtaining network credentials using asyncronous methods (prompting user, cloud API call,
+     * etc).
+     *
+     * @return CHIP_ERROR   The return status. Returns CHIP_ERROR_INCORRECT_STATE if not in the correct state (kNeedsNetworkCreds).
+     */
+    CHIP_ERROR NetworkCredentialsReady();
+
 #if CONFIG_NETWORK_LAYER_BLE
 #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE
     /**

src/controller/CommissioningDelegate.h

@@ -55,6 +55,7 @@ enum CommissioningStage : uint8_t
     // ScanNetworks can happen anytime after kArmFailsafe.
     // However, the circ tests fail if it is earlier in the list
     kScanNetworks,
+    kNeedsNetworkCreds,
 };
 
 const char * StageToString(CommissioningStage stage);

src/controller/DevicePairingDelegate.h

@@ -86,7 +86,13 @@ class DLL_EXPORT DevicePairingDelegate
 
     /**
      * @brief
-     *  Called with the NetworkScanResponse returned from the target
+     *  Called with the NetworkScanResponse returned from the target.
+     *
+     * The DeviceCommissioner will be waiting in the kNeedsNetworkCreds step and not advancing the commissioning process.
+     *
+     * The implementation should set the network credentials on the CommissioningParameters of the CommissioningDelegate
+     * using CommissioningDelegate.SetCommissioningParameters(), and then call DeviceCommissioner.NetworkCredentialsReady()
+     * in order to resume the commissioning process.
      */
     virtual void
     OnScanNetworksSuccess(const app::Clusters::NetworkCommissioning::Commands::ScanNetworksResponse::DecodableType & dataResponse)
@@ -95,6 +101,12 @@ class DLL_EXPORT DevicePairingDelegate
     /**
      * @brief
      *  Called when the NetworkScan request fails.
+     *
+     * The DeviceCommissioner will be waiting in the kNeedsNetworkCreds step and not advancing the commissioning process.
+     *
+     * The implementation should set the network credentials on the CommissioningParameters of the CommissioningDelegate
+     * using CommissioningDelegate.SetCommissioningParameters(), and then call DeviceCommissioner.NetworkCredentialsReady()
+     * in order to resume the commissioning process.
      */
     virtual void OnScanNetworksFailure(CHIP_ERROR error) {}
 };

src/controller/java/CHIPDeviceController-JNI.cpp

@@ -513,26 +513,6 @@ JNI_METHOD(void, establishPaseConnectionByAddress)
     }
 }
 
-JNI_METHOD(void, pauseCommissioning)
-(JNIEnv * env, jobject self, jlong handle)
-{
-    ChipLogProgress(Controller, "pauseCommissioning() called");
-    chip::DeviceLayer::StackLock lock;
-    AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle);
-
-    wrapper->GetAutoCommissioner()->PauseCommissioning();
-}
-
-JNI_METHOD(void, resumeCommissioning)
-(JNIEnv * env, jobject self, jlong handle)
-{
-    ChipLogProgress(Controller, "resumeCommissioning() called");
-    chip::DeviceLayer::StackLock lock;
-    AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle);
-
-    wrapper->GetAutoCommissioner()->ResumeCommissioning();
-}
-
 JNI_METHOD(void, setUseJavaCallbackForNOCRequest)
 (JNIEnv * env, jobject self, jlong handle, jboolean useCallback)
 {

src/controller/java/src/chip/devicecontroller/ChipDeviceController.java

@@ -188,14 +188,6 @@ public void commissionDevice(
     commissionDevice(deviceControllerPtr, deviceId, csrNonce, networkCredentials);
   }
 
-  public void pauseCommissioning() {
-    pauseCommissioning(deviceControllerPtr);
-  }
-
-  public void resumeCommissioning() {
-    resumeCommissioning(deviceControllerPtr);
-  }
-
   /**
    * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be
    * called when the NOC CSR needs to be signed. This allows for custom credentials issuer
@@ -670,10 +662,6 @@ private native boolean openPairingWindowWithPINCallback(
 
   private native byte[] getAttestationChallenge(long deviceControllerPtr, long devicePtr);
 
-  private native void pauseCommissioning(long deviceControllerPtr);
-
-  private native void resumeCommissioning(long deviceControllerPtr);
-
   private native void setUseJavaCallbackForNOCRequest(
       long deviceControllerPtr, boolean useCallback);