From 1084dd53d50b54e52cc0d91a0f39f84dc68b7039 Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Mon, 28 Aug 2023 21:15:43 +1200 Subject: [PATCH] Stop the SetupCodePairer when StopPairing is called (#28881) Prior to this change the SetupCodePairer was left running, and in particular its discovery timeout timer was not correctly cancelled. --- src/controller/CHIPDeviceController.cpp | 14 +++++++--- src/controller/SetUpCodePairer.cpp | 35 ++++++++++++++----------- src/controller/SetUpCodePairer.h | 14 +++++----- 3 files changed, 37 insertions(+), 26 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index e37b058f7a67e4..ee2c36d8bd761e 100644 --- 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 index e1ab5e778dd6df..db09d91fd74441 100644 --- 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 resolutionData) { VerifyOrReturnError(mSystemLayer != nullptr, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(remoteId != kUndefinedNodeId, CHIP_ERROR_INVALID_ARGUMENT); SetupPayload payload; ReturnErrorOnFailure(GetPayload(setUpCode, payload)); @@ -405,9 +406,19 @@ void SetUpCodePairer::NotifyCommissionableDeviceDiscovered(const Dnssd::CommonRe ConnectToDiscoveredDevice(); } -void SetUpCodePairer::CommissionerShuttingDown() +bool SetUpCodePairer::StopPairing(NodeId remoteId) { + VerifyOrReturnValue(mRemoteId != kUndefinedNodeId, false); + VerifyOrReturnValue(remoteId == kUndefinedNodeId || remoteId == mRemoteId, false); + + if (mWaitingForPASE) + { + PASEEstablishmentComplete(); + } + ResetDiscoveryState(); + mRemoteId = kUndefinedNodeId; + return true; } bool SetUpCodePairer::TryNextRendezvousParameters() @@ -452,32 +463,26 @@ 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() { + VerifyOrDie(mWaitingForPASE); mWaitingForPASE = false; mCommissioner->RegisterPairingDelegate(mPairingDelegate); mPairingDelegate = nullptr; @@ -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 index 04177ec313182a..f9dc542f4258a0 100644 --- 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,9 @@ 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, or of + // whichever node we're pairing if kUndefinedNodeId is passed. + bool StopPairing(NodeId remoteId = kUndefinedNodeId); private: // DevicePairingDelegate implementation. @@ -177,9 +177,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;