Stop the SetupCodePairer when StopPairing is called

Prior to this change the SetupCodePairer was left running, and in particular its discovery timeout timer was not correctly cancelled.
project-chip · Aug 25, 2023 · a088911 · a088911
1 parent f23326b
commit a088911
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 29 deletions.
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -477,7 +477,7 @@ void DeviceCommissioner::Shutdown()
 
     ChipLogDetail(Controller, "Shutting down the commissioner");
 
-    mSetUpCodePairer.CommissionerShuttingDown();
+    mSetUpCodePairer.StopPairing();
 
     // Check to see if pairing in progress before shutting down
     CommissioneeDeviceProxy * device = mDeviceInPASEEstablishment;
@@ -916,12 +916,18 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de
 CHIP_ERROR DeviceCommissioner::StopPairing(NodeId remoteDeviceId)
 {
     VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(remoteDeviceId != kUndefinedNodeId, CHIP_ERROR_INVALID_ARGUMENT);
+
+    bool stopped = mSetUpCodePairer.StopPairing(remoteDeviceId);
 
     CommissioneeDeviceProxy * device = FindCommissioneeDevice(remoteDeviceId);
-    VerifyOrReturnError(device != nullptr, CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);
+    if (device != nullptr)
+    {
+        ReleaseCommissioneeDevice(device);
+        stopped = true;
+    }
 
-    ReleaseCommissioneeDevice(device);
-    return CHIP_NO_ERROR;
+    return (stopped) ? CHIP_NO_ERROR : CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR;
 }
 
 CHIP_ERROR DeviceCommissioner::UnpairDevice(NodeId remoteDeviceId)

diff --git a/src/controller/SetUpCodePairer.cpp b/src/controller/SetUpCodePairer.cpp
@@ -60,6 +60,7 @@ CHIP_ERROR SetUpCodePairer::PairDevice(NodeId remoteId, const char * setUpCode,
                                        DiscoveryType discoveryType, Optional<Dnssd::CommonResolutionData> resolutionData)
 {
     VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(remoteId != kUndefinedNodeId, CHIP_ERROR_INVALID_ARGUMENT);
 
     SetupPayload payload;
     ReturnErrorOnFailure(GetPayload(setUpCode, payload));
@@ -405,9 +406,17 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::CommonRe
     ConnectToDiscoveredDevice();
 }
 
-void SetUpCodePairer::CommissionerShuttingDown()
+bool SetUpCodePairer::StopPairing(NodeId remoteId)
 {
+    if ((remoteId != kUndefinedNodeId && remoteId != mRemoteId) || mRemoteId == kUndefinedNodeId)
+    {
+        return false; // Not discovering the specified node (or any node)
+    }
+
+    PASEEstablishmentComplete();
     ResetDiscoveryState();
+    mRemoteId = kUndefinedNodeId;
+    return true;
 }
 
 bool SetUpCodePairer::TryNextRendezvousParameters()
@@ -452,35 +461,31 @@ void SetUpCodePairer::ResetDiscoveryState()
         waiting = false;
     }
 
-    while (!mDiscoveredParameters.empty())
-    {
-        mDiscoveredParameters.pop_front();
-    }
-
+    mDiscoveredParameters.clear();
     mCurrentPASEParameters.ClearValue();
     mLastPASEError = CHIP_NO_ERROR;
+
+    mSystemLayer->CancelTimer(OnDeviceDiscoveredTimeoutCallback, this);
 }
 
 void SetUpCodePairer::ExpectPASEEstablishment()
 {
+    VerifyOrDie(!mWaitingForPASE);
     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;
-    }
-
+    VerifyOrDie(delegate != this);
     mPairingDelegate = delegate;
     mCommissioner->RegisterPairingDelegate(this);
 }
 
 void SetUpCodePairer::PASEEstablishmentComplete()
 {
-    mWaitingForPASE = false;
-    mCommissioner->RegisterPairingDelegate(mPairingDelegate);
-    mPairingDelegate = nullptr;
+    if (mWaitingForPASE)
+    {
+        mWaitingForPASE = false;
+        mCommissioner->RegisterPairingDelegate(mPairingDelegate);
+        mPairingDelegate = nullptr;
+    }
 }
 
 void SetUpCodePairer::OnStatusUpdate(DevicePairingDelegate::Status status)
@@ -524,9 +529,9 @@ void SetUpCodePairer::OnPairingComplete(CHIP_ERROR error)
 
     if (CHIP_NO_ERROR == error)
     {
-        mSystemLayer->CancelTimer(OnDeviceDiscoveredTimeoutCallback, this);
-
+        ChipLogProgress(Controller, "Pairing with commissionee successful, stopping discovery");
         ResetDiscoveryState();
+        mRemoteId = kUndefinedNodeId;
         if (pairingDelegate != nullptr)
         {
             pairingDelegate->OnPairingComplete(error);

diff --git a/src/controller/SetUpCodePairer.h b/src/controller/SetUpCodePairer.h
@@ -76,7 +76,7 @@ enum class DiscoveryType : uint8_t
 class DLL_EXPORT SetUpCodePairer : public DevicePairingDelegate
 {
 public:
-    SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) { ResetDiscoveryState(); }
+    SetUpCodePairer(DeviceCommissioner * commissioner) : mCommissioner(commissioner) {}
     virtual ~SetUpCodePairer() {}
 
     CHIP_ERROR PairDevice(chip::NodeId remoteId, const char * setUpCode,
@@ -93,9 +93,8 @@ class DLL_EXPORT SetUpCodePairer : public DevicePairingDelegate
     void SetBleLayer(Ble::BleLayer * bleLayer) { mBleLayer = bleLayer; };
 #endif // CONFIG_NETWORK_LAYER_BLE
 
-    // Called to notify us that the DeviceCommissioner is shutting down and we
-    // should not try to do any more new work.
-    void CommissionerShuttingDown();
+    // Stop ongoing discovery / pairing (of the specified node)
+    bool StopPairing(NodeId remoteId = kUndefinedNodeId);
 
 private:
     // DevicePairingDelegate implementation.
@@ -177,9 +176,9 @@ class DLL_EXPORT SetUpCodePairer : public DevicePairingDelegate
     uint16_t mPayloadVendorID               = kNotAvailable;
     uint16_t mPayloadProductID              = kNotAvailable;
 
-    DeviceCommissioner * mCommissioner = nullptr;
-    System::Layer * mSystemLayer       = nullptr;
-    chip::NodeId mRemoteId;
+    DeviceCommissioner * mCommissioner       = nullptr;
+    System::Layer * mSystemLayer             = nullptr;
+    chip::NodeId mRemoteId                   = kUndefinedNodeId;
     uint32_t mSetUpPINCode                   = 0;
     SetupCodePairerBehaviour mConnectionType = SetupCodePairerBehaviour::kCommission;
     DiscoveryType mDiscoveryType             = DiscoveryType::kAll;