Untangle SetUpCodePairer from CHIPDeviceCommissioner a bit. (#16890)

Followup to #16802

Instead of making the behavior of CHIPDeviceCommissioner depend on the
state of the SetUpCodePairer, just have the latter register as the
pairing delegate when it cares about PASE establishment, and restore
the logic we used to have in CHIPDeviceCommissioner before PR 16802.

The one logic change in CHIPDeviceCommissioner other than that is a
fix to ensure that we in fact call OnPairingComplete when PASE is
established even if we're going to proceed to auto-commission, so
SetUpCodePairer knows to get out of the way at that point.

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