Untangle SetUpCodePairer from CHIPDeviceCommissioner a bit.

src/controller/BUILD.gn

@@ -50,6 +50,7 @@ static_library("controller") {
       "CommissioningWindowOpener.cpp",
       "CommissioningWindowOpener.h",
       "DeviceDiscoveryDelegate.h",
+      "DevicePairingDelegate.h",
       "EmptyDataModelHandler.cpp",
       "ExampleOperationalCredentialsIssuer.cpp",
       "ExampleOperationalCredentialsIssuer.h",

src/controller/CHIPDeviceController.cpp

@@ -481,7 +481,7 @@ CHIP_ERROR DeviceCommissioner::Shutdown()
     if (device != nullptr && device->IsSessionSetupInProgress())
     {
         ChipLogDetail(Controller, "Setup in progress, stopping setup before shutting down");
-        PairingFailed(CHIP_ERROR_CONNECTION_ABORTED);
+        OnSessionEstablishmentError(CHIP_ERROR_CONNECTION_ABORTED);
     }
     // TODO: If we have a commissioning step in progress, is there a way to cancel that callback?
 
@@ -812,27 +812,7 @@ void DeviceCommissioner::RendezvousCleanup(CHIP_ERROR status)
 
 void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err)
 {
-    // Need to null out mDeviceInPASEEstablishment before maybe trying to
-    // establish PASE again, so EstablishPASEConnection won't error out.
-    auto * oldDeviceInPASEEstablishment = mDeviceInPASEEstablishment;
-    mDeviceInPASEEstablishment          = nullptr;
-
-    if (mSetUpCodePairer.TryNextRendezvousParameters())
-    {
-        ChipLogProgress(Controller, "Ignoring PASE error; will try commissioning over a different transport");
-        // We don't need oldDeviceInPASEEstablishment anymore.
-        ReleaseCommissioneeDevice(oldDeviceInPASEEstablishment);
-        return;
-    }
-
-    // Reset mDeviceInPASEEstablishment because PairingFailed checks for that to
-    // send the right notifications.
-    mDeviceInPASEEstablishment = oldDeviceInPASEEstablishment;
-    PairingFailed(err);
-}
-
-void DeviceCommissioner::PairingFailed(CHIP_ERROR err)
-{
+    // PASE session establishment failure.
     mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this);
 
     if (mPairingDelegate != nullptr)
@@ -851,7 +831,7 @@ void DeviceCommissioner::OnSessionEstablished()
     // We are in the callback for this pairing. Reset so we can pair another device.
     mDeviceInPASEEstablishment = nullptr;
 
-    VerifyOrReturn(device != nullptr, PairingFailed(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));
+    VerifyOrReturn(device != nullptr, OnSessionEstablishmentError(CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR));
 
     PASESession * pairing = &device->GetPairing();
 
@@ -862,22 +842,19 @@ void DeviceCommissioner::OnSessionEstablished()
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Failed in setting up secure channel: err %s", ErrorStr(err));
-        PairingFailed(err);
+        OnSessionEstablishmentError(err);
         return;
     }
 
     ChipLogDetail(Controller, "Remote device completed SPAKE2+ handshake");
 
+    mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR);
+
     if (mRunCommissioningAfterConnection)
     {
         mRunCommissioningAfterConnection = false;
         mDefaultCommissioner->StartCommissioning(this, device);
     }
-    else
-    {
-        ChipLogProgress(Controller, "OnPairingComplete");
-        mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR);
-    }
 }
 
 CHIP_ERROR DeviceCommissioner::SendCertificateChainRequestCommand(DeviceProxy * device,

src/controller/CHIPDeviceController.h

@@ -39,6 +39,7 @@
 #include <controller/CHIPDeviceControllerSystemState.h>
 #include <controller/CommissioneeDeviceProxy.h>
 #include <controller/CommissioningDelegate.h>
+#include <controller/DevicePairingDelegate.h>
 #include <controller/OperationalCredentialsDelegate.h>
 #include <controller/SetUpCodePairer.h>
 #include <credentials/FabricTable.h>
@@ -111,47 +112,6 @@ struct ControllerInitParams
     uint16_t controllerVendorId;
 };
 
-class DLL_EXPORT DevicePairingDelegate
-{
-public:
-    virtual ~DevicePairingDelegate() {}
-
-    enum Status : uint8_t
-    {
-        SecurePairingSuccess = 0,
-        SecurePairingFailed,
-    };
-
-    /**
-     * @brief
-     *   Called when the pairing reaches a certain stage.
-     *
-     * @param status Current status of pairing
-     */
-    virtual void OnStatusUpdate(DevicePairingDelegate::Status status) {}
-
-    /**
-     * @brief
-     *   Called when the pairing is complete (with success or error)
-     *
-     * @param error Error cause, if any
-     */
-    virtual void OnPairingComplete(CHIP_ERROR error) {}
-
-    /**
-     * @brief
-     *   Called when the pairing is deleted (with success or error)
-     *
-     * @param error Error cause, if any
-     */
-    virtual void OnPairingDeleted(CHIP_ERROR error) {}
-
-    /**
-     *   Called when the commissioning process is complete (with success or error)
-     */
-    virtual void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) {}
-};
-
 struct CommissionerInitParams : public ControllerInitParams
 {
     DevicePairingDelegate * pairingDelegate     = nullptr;
@@ -587,6 +547,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     void OnNodeDiscovered(const chip::Dnssd::DiscoveredNodeData & nodeData) override;
 
     void RegisterPairingDelegate(DevicePairingDelegate * pairingDelegate) { mPairingDelegate = pairingDelegate; }
+    DevicePairingDelegate * GetPairingDelegate() const { return mPairingDelegate; }
 
     // AttributeCache::Callback impl
     void OnDone() override;
@@ -623,11 +584,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
 
     static void OnSessionEstablishmentTimeoutCallback(System::Layer * aLayer, void * aAppState);
 
-    // Helper to call once we decide that pairing has failed.  This might be
-    // called from OnSessionEstablishmentError if we have no other transports to
-    // try, or in various other failure cases.
-    void PairingFailed(CHIP_ERROR err);
-
     /* This function sends a Device Attestation Certificate chain request to the device.
        The function does not hold a reference to the device object.
      */

src/controller/DevicePairingDelegate.h

@@ -0,0 +1,74 @@
+/*
+ *    Copyright (c) 2021 Project CHIP Authors
+ *    All rights reserved.
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+#pragma once
+
+#include <lib/core/CHIPError.h>
+#include <lib/core/NodeId.h>
+#include <lib/support/DLLUtil.h>
+#include <stdint.h>
+
+namespace chip {
+namespace Controller {
+
+/**
+ * A delegate that can be notified of progress as a "pairing" (which might mean
+ * "PASE session establishment" or "commissioning") proceeds.
+ */
+class DLL_EXPORT DevicePairingDelegate
+{
+public:
+    virtual ~DevicePairingDelegate() {}
+
+    enum Status : uint8_t
+    {
+        SecurePairingSuccess = 0,
+        SecurePairingFailed,
+    };
+
+    /**
+     * @brief
+     *   Called when the pairing reaches a certain stage.
+     *
+     * @param status Current status of pairing
+     */
+    virtual void OnStatusUpdate(DevicePairingDelegate::Status status) {}
+
+    /**
+     * @brief
+     *   Called when PASE session establishment is complete (with success or error)
+     *
+     * @param error Error cause, if any
+     */
+    virtual void OnPairingComplete(CHIP_ERROR error) {}
+
+    /**
+     * @brief
+     *   Called when the pairing is deleted (with success or error)
+     *
+     * @param error Error cause, if any
+     */
+    virtual void OnPairingDeleted(CHIP_ERROR error) {}
+
+    /**
+     *   Called when the commissioning process is complete (with success or error)
+     */
+    virtual void OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error) {}
+};
+
+} // namespace Controller
+} // namespace chip

src/controller/SetUpCodePairer.cpp

@@ -162,10 +162,10 @@ CHIP_ERROR SetUpCodePairer::StopConnectOverSoftAP()
 
 bool SetUpCodePairer::ConnectToDiscoveredDevice()
 {
-    if (mWaitingForConnection)
+    if (mWaitingForPASE)
     {
-        // Nothing to do.  Just wait until we either succeed at that connection
-        // or fail and get asked to try another device.
+        // Nothing to do.  Just wait until we either succeed or fail at that
+        // PASE session establishment.
         return false;
     }
 
@@ -189,6 +189,9 @@ bool SetUpCodePairer::ConnectToDiscoveredDevice()
         ChipLogProgress(Controller, "Attempting PASE connection to %s", buf);
 #endif // CHIP_PROGRESS_LOGGING
 
+        // Handle possibly-sync call backs from attempts to establish PASE.
+        ExpectPASEEstablishment();
+
         CHIP_ERROR err;
         if (mConnectionType == SetupCodePairerBehaviour::kCommission)
         {
@@ -198,12 +201,15 @@ bool SetUpCodePairer::ConnectToDiscoveredDevice()
         {
             err = mCommissioner->EstablishPASEConnection(mRemoteId, params);
         }
+
         LogErrorOnFailure(err);
         if (err == CHIP_NO_ERROR)
         {
-            mWaitingForConnection = true;
             return true;
         }
+
+        // Failed to start establishing PASE.
+        PASEEstablishmentComplete();
     }
 
     return false;
@@ -284,12 +290,6 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::Discover
 
 bool SetUpCodePairer::TryNextRendezvousParameters()
 {
-    // Don't call PairDevice again; just do PASE as needed.  If we called
-    // PairDevice before and PASE succeeds, the commmissioning that's been
-    // queued up will happen.
-    mConnectionType       = SetupCodePairerBehaviour::kPaseOnly;
-    mWaitingForConnection = false;
-
     if (ConnectToDiscoveredDevice())
     {
         ChipLogProgress(Controller, "Trying connection to commissionee over different transport");
@@ -321,5 +321,91 @@ void SetUpCodePairer::ResetDiscoveryState()
     }
 }
 
+void SetUpCodePairer::ExpectPASEEstablishment()
+{
+    mWaitingForPASE = true;
+    auto * delegate = mCommissioner->GetPairingDelegate();
+    if (this == delegate)
+    {
+        // This should really not happen, but if it does, do nothing, to avoid
+        // delegate loops.
+        return;
+    }
+
+    mPairingDelegate = delegate;
+    mCommissioner->RegisterPairingDelegate(this);
+}
+
+void SetUpCodePairer::PASEEstablishmentComplete()
+{
+    mWaitingForPASE = false;
+    mCommissioner->RegisterPairingDelegate(mPairingDelegate);
+    mPairingDelegate = nullptr;
+}
+
+void SetUpCodePairer::OnStatusUpdate(DevicePairingDelegate::Status status)
+{
+    if (mPairingDelegate)
+    {
+        mPairingDelegate->OnStatusUpdate(status);
+    }
+}
+
+void SetUpCodePairer::OnPairingComplete(CHIP_ERROR error)
+{
+    // Save the pairing delegate so we can notify it.  We want to notify it
+    // _after_ we restore the state on the commissioner, in case the delegate
+    // ends up immediately calling back into the commissioner again when
+    // notified.
+    auto * pairingDelegate = mPairingDelegate;
+    PASEEstablishmentComplete();
+
+    if (CHIP_NO_ERROR == error)
+    {
+        // StopConnectOverBle calls CancelBleIncompleteConnection, which will
+        // cancel connections that are in fact completed. In particular, if we
+        // just established PASE over BLE calling StopConnectOverBle here
+        // unconditionally would cancel the BLE connection underlying the PASE
+        // session.  So make sure to only call StopConnectOverBle if we're still
+        // waiting to hear back on the BLE discovery bits.
+        if (mWaitingForDiscovery[kBLETransport])
+        {
+            StopConnectOverBle();
+        }
+        StopConnectOverIP();
+        StopConnectOverSoftAP();
+        ResetDiscoveryState();
+        pairingDelegate->OnPairingComplete(error);
+        return;
+    }
+
+    // We failed to establish PASE.  Try the next thing we have discovered, if
+    // any.
+    if (TryNextRendezvousParameters())
+    {
+        // Keep waiting until that finishes.  Don't call OnPairingComplete yet.
+        return;
+    }
+
+    pairingDelegate->OnPairingComplete(error);
+}
+
+void SetUpCodePairer::OnPairingDeleted(CHIP_ERROR error)
+{
+    if (mPairingDelegate)
+    {
+        mPairingDelegate->OnPairingDeleted(error);
+    }
+}
+
+void SetUpCodePairer::OnCommissioningComplete(NodeId deviceId, CHIP_ERROR error)
+{
+    // Not really expecting this, but handle it anyway.
+    if (mPairingDelegate)
+    {
+        mPairingDelegate->OnCommissioningComplete(deviceId, error);
+    }
+}
+
 } // namespace Controller
 } // namespace chip