From 5f05a6fce89be595641cdfb43fd01adc7ac73531 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 18 May 2022 13:06:47 -0400 Subject: [PATCH] Stop calling OnPairingComplete on CASE session establishment. (#18524) * Stop calling OnPairingComplete on CASE session establishment. OnPairingComplete is documented as indicating the end of PASE establishment, but our implementation calls it for _some_ CASE sessions as well: only the ones we kick off from UpdateDevice. This is unexpected by API consumers, and we should not be doing it. Fixes https://github.com/project-chip/connectedhomeip/issues/18230 * Address review comments. * Stop calling UpdateDevice from the Python controller. Use GetConnectedDevice instead, so we don't have to rely on global callbacks. --- src/controller/CHIPDeviceController.cpp | 45 +++++++------------ .../ChipDeviceController-ScriptBinding.cpp | 6 --- src/controller/python/chip/ChipDeviceCtrl.py | 27 +++++------ 3 files changed, 28 insertions(+), 50 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 3c27cd54b303a5..fd5e6b5fa50507 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -1528,32 +1528,25 @@ void DeviceCommissioner::OnDeviceConnectedFn(void * context, OperationalDevicePr DeviceCommissioner * commissioner = static_cast(context); VerifyOrReturn(commissioner != nullptr, ChipLogProgress(Controller, "Device connected callback with null context. Ignoring")); - if (commissioner->mCommissioningStage == CommissioningStage::kFindOperational) + if (commissioner->mCommissioningStage != CommissioningStage::kFindOperational) { - if (commissioner->mDeviceBeingCommissioned != nullptr && - commissioner->mDeviceBeingCommissioned->GetDeviceId() == device->GetDeviceId() && - commissioner->mCommissioningDelegate != nullptr) - { - CommissioningDelegate::CommissioningReport report; - report.Set(OperationalNodeFoundData(device)); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); - } + // This call is definitely not us finding our commissionee device. + // This is presumably us trying to re-establish CASE on MRP failure. + return; } - else + + if (commissioner->mDeviceBeingCommissioned == nullptr || + commissioner->mDeviceBeingCommissioned->GetDeviceId() != device->GetDeviceId()) { - if (commissioner->mPairingDelegate != nullptr) - { - commissioner->mPairingDelegate->OnPairingComplete(CHIP_NO_ERROR); - } - // Only release the PASE session if we're not commissioning. If we're commissioning, we're going to hold onto that PASE - // session until we send the commissioning complete command just in case it fails and we need to go back to the PASE - // connection to re-setup the network. This is unlikely, given that we just connected over the operational network, but is - // required by the spec. - CommissioneeDeviceProxy * commissionee = commissioner->FindCommissioneeDevice(device->GetDeviceId()); - if (commissionee != nullptr) - { - commissioner->ReleaseCommissioneeDevice(commissionee); - } + // Not the device we are trying to commission. + return; + } + + if (commissioner->mCommissioningDelegate != nullptr) + { + CommissioningDelegate::CommissioningReport report; + report.Set(OperationalNodeFoundData(device)); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR, report); } } @@ -1565,8 +1558,6 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer ChipLogProgress(Controller, "Device connection failed. Error %s", ErrorStr(error)); VerifyOrReturn(commissioner != nullptr, ChipLogProgress(Controller, "Device connection failure callback with null context. Ignoring")); - VerifyOrReturn(commissioner->mPairingDelegate != nullptr, - ChipLogProgress(Controller, "Device connection failure callback with null pairing delegate. Ignoring")); // Ensure that commissioning stage advancement is done based on seeing an error. if (error == CHIP_NO_ERROR) @@ -1580,10 +1571,6 @@ void DeviceCommissioner::OnDeviceConnectionFailureFn(void * context, PeerId peer { commissioner->CommissioningStageComplete(error); } - else - { - commissioner->mPairingDelegate->OnPairingComplete(error); - } commissioner->mSystemState->CASESessionMgr()->ReleaseSession(peerId); } diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index aa1a38bb3de8ee..6be1d8be5579d1 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -152,7 +152,6 @@ ChipError::StorageType pychip_DeviceController_OpenCommissioningWindow(chip::Con void pychip_DeviceController_PrintDiscoveredDevices(chip::Controller::DeviceCommissioner * devCtrl); bool pychip_DeviceController_GetIPForDiscoveredDevice(chip::Controller::DeviceCommissioner * devCtrl, int idx, char * addrStr, uint32_t len); -ChipError::StorageType pychip_DeviceController_UpdateDevice(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid); // Pairing Delegate ChipError::StorageType @@ -538,11 +537,6 @@ ChipError::StorageType pychip_ScriptDevicePairingDelegate_SetCommissioningStatus return CHIP_NO_ERROR.AsInteger(); } -ChipError::StorageType pychip_DeviceController_UpdateDevice(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid) -{ - return devCtrl->UpdateDevice(nodeid).AsInteger(); -} - ChipError::StorageType pychip_Stack_Init() { CHIP_ERROR err = CHIP_NO_ERROR; diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 63f4b50ab4736c..b926d12dfad00a 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -342,10 +342,11 @@ def SetThreadOperationalDataset(self, threadOperationalDataset): def ResolveNode(self, nodeid): self.CheckIsActive() - return self._ChipStack.CallAsync( - lambda: self._dmLib.pychip_DeviceController_UpdateDevice( - self.devCtrl, nodeid) - ) + try: + self.GetConnectedDeviceSync(nodeid, allowPASE=False) + return 0 + except ChipStackError as ex: + return ex.err def GetAddressAndPort(self, nodeid): self.CheckIsActive() @@ -479,7 +480,7 @@ def GetClusterHandler(self): return self._Cluster - def GetConnectedDeviceSync(self, nodeid): + def GetConnectedDeviceSync(self, nodeid, allowPASE=True): self.CheckIsActive() returnDevice = c_void_p(None) @@ -496,12 +497,12 @@ def DeviceAvailableCallback(device, err): print("Failed in getting the connected device: {}".format(err)) raise self._ChipStack.ErrorToException(err) - res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetDeviceBeingCommissioned( - self.devCtrl, nodeid, byref(returnDevice))) - if res == 0: - # TODO: give users more contrtol over whether they want to send this command over a PASE established connection - print('Using PASE connection') - return returnDevice + if allowPASE: + res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetDeviceBeingCommissioned( + self.devCtrl, nodeid, byref(returnDevice))) + if res == 0: + print('Using PASE connection') + return returnDevice res = self._ChipStack.Call(lambda: self._dmLib.pychip_GetConnectedDeviceByNodeId( self.devCtrl, nodeid, DeviceAvailableCallback)) @@ -1019,10 +1020,6 @@ def _InitLib(self): c_void_p, _DevicePairingDelegate_OnCommissioningStatusUpdateFunct] self._dmLib.pychip_ScriptDevicePairingDelegate_SetCommissioningCompleteCallback.restype = c_uint32 - self._dmLib.pychip_DeviceController_UpdateDevice.argtypes = [ - c_void_p, c_uint64] - self._dmLib.pychip_DeviceController_UpdateDevice.restype = c_uint32 - self._dmLib.pychip_GetConnectedDeviceByNodeId.argtypes = [ c_void_p, c_uint64, _DeviceAvailableFunct] self._dmLib.pychip_GetConnectedDeviceByNodeId.restype = c_uint32