From 329978430a16456d6da536e02a99999e8e573f0f Mon Sep 17 00:00:00 2001 From: C Freeman Date: Wed, 16 Aug 2023 10:30:29 -0400 Subject: [PATCH] Commissioning: Fix fabric check stage and add test (#28531) * Fix fabric check stage and add test also lots of plumbing to the python layers. * Missed one. --- examples/platform/linux/CommissionerMain.cpp | 8 +++--- src/controller/CHIPDeviceController.cpp | 4 +-- src/controller/DevicePairingDelegate.h | 2 +- .../ChipDeviceController-ScriptBinding.cpp | 16 ++++++++++++ ...Controller-ScriptDevicePairingDelegate.cpp | 21 ++++++++++++++++ ...ceController-ScriptDevicePairingDelegate.h | 4 +++ src/controller/python/chip/ChipDeviceCtrl.py | 25 +++++++++++++++++++ .../TestCommissioningTimeSync.py | 22 ++++++++++++++++ 8 files changed, 95 insertions(+), 7 deletions(-) diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index 73d9a146879b42..64a7a47d2dffcb 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -248,7 +248,7 @@ class PairingCommand : public Controller::DevicePairingDelegate void OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error) override; void OnReadCommissioningInfo(const ReadCommissioningInfo & info) override; - void OnFabricCheck(const MatchingFabricInfo & info) override; + void OnFabricCheck(NodeId matchingNodeId) override; private: #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED @@ -347,11 +347,11 @@ void PairingCommand::OnReadCommissioningInfo(const ReadCommissioningInfo & info) info.basic.productId); } -void PairingCommand::OnFabricCheck(const MatchingFabricInfo & info) +void PairingCommand::OnFabricCheck(NodeId matchingNodeId) { - if (info.nodeId != kUndefinedNodeId) + if (matchingNodeId != kUndefinedNodeId) { - ChipLogProgress(AppServer, "ALREADY ON FABRIC WITH nodeId=0x" ChipLogFormatX64, ChipLogValueX64(info.nodeId)); + ChipLogProgress(AppServer, "ALREADY ON FABRIC WITH nodeId=0x" ChipLogFormatX64, ChipLogValueX64(matchingNodeId)); // wait until attestation verification before cancelling so we can validate vid/pid } } diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 8cb41b167169cc..e37b058f7a67e4 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -2150,7 +2150,7 @@ void DeviceCommissioner::ParseFabrics() if (mPairingDelegate != nullptr) { - mPairingDelegate->OnFabricCheck(info); + mPairingDelegate->OnFabricCheck(info.nodeId); } CommissioningDelegate::CommissioningReport report; @@ -2399,7 +2399,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio break; case CommissioningStage::kCheckForMatchingFabric: { // Read the current fabrics - if (params.GetCheckForMatchingFabric()) + if (!params.GetCheckForMatchingFabric()) { // We don't actually want to do this step, so just bypass it ChipLogProgress(Controller, "kCheckForMatchingFabric step called without parameter set, skipping"); diff --git a/src/controller/DevicePairingDelegate.h b/src/controller/DevicePairingDelegate.h index e46d7c158ffb62..80d6296d52109c 100644 --- a/src/controller/DevicePairingDelegate.h +++ b/src/controller/DevicePairingDelegate.h @@ -87,7 +87,7 @@ class DLL_EXPORT DevicePairingDelegate * @brief * Called when MatchingFabricInfo returned from target */ - virtual void OnFabricCheck(const MatchingFabricInfo & info) {} + virtual void OnFabricCheck(NodeId matchingNodeId) {} /** * @brief diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 65c8e086fae385..a7732a48361db0 100644 --- a/src/controller/python/ChipDeviceController-ScriptBinding.cpp +++ b/src/controller/python/ChipDeviceController-ScriptBinding.cpp @@ -144,6 +144,7 @@ PyChipError pychip_DeviceController_SetTimeZone(int32_t offset, uint64_t validAt PyChipError pychip_DeviceController_SetDSTOffset(int32_t offset, uint64_t validStarting, uint64_t validUntil); PyChipError pychip_DeviceController_SetDefaultNtp(const char * defaultNTP); PyChipError pychip_DeviceController_SetTrustedTimeSource(chip::NodeId nodeId, chip::EndpointId endpoint); +PyChipError pychip_DeviceController_SetCheckMatchingFabric(bool check); PyChipError pychip_DeviceController_ResetCommissioningParameters(); PyChipError pychip_DeviceController_CloseSession(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeid); PyChipError pychip_DeviceController_EstablishPASESessionIP(chip::Controller::DeviceCommissioner * devCtrl, const char * peerAddrStr, @@ -189,6 +190,8 @@ PyChipError pychip_ScriptDevicePairingDelegate_SetCommissioningCompleteCallback( PyChipError pychip_ScriptDevicePairingDelegate_SetCommissioningStatusUpdateCallback( chip::Controller::DeviceCommissioner * devCtrl, chip::Controller::DevicePairingDelegate_OnCommissioningStatusUpdateFunct callback); +PyChipError +pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback(chip::Controller::DevicePairingDelegate_OnFabricCheckFunct callback); PyChipError pychip_ScriptDevicePairingDelegate_SetOpenWindowCompleteCallback( chip::Controller::DeviceCommissioner * devCtrl, chip::Controller::DevicePairingDelegate_OnWindowOpenCompleteFunct callback); @@ -541,6 +544,12 @@ PyChipError pychip_DeviceController_SetTrustedTimeSource(chip::NodeId nodeId, ch return ToPyChipError(CHIP_NO_ERROR); } +PyChipError pychip_DeviceController_SetCheckMatchingFabric(bool check) +{ + sCommissioningParameters.SetCheckForMatchingFabric(check); + return ToPyChipError(CHIP_NO_ERROR); +} + PyChipError pychip_DeviceController_ResetCommissioningParameters() { sCommissioningParameters = CommissioningParameters(); @@ -693,6 +702,13 @@ PyChipError pychip_ScriptDevicePairingDelegate_SetCommissioningStatusUpdateCallb return ToPyChipError(CHIP_NO_ERROR); } +PyChipError +pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback(chip::Controller::DevicePairingDelegate_OnFabricCheckFunct callback) +{ + sPairingDelegate.SetFabricCheckCallback(callback); + return ToPyChipError(CHIP_NO_ERROR); +} + const char * pychip_Stack_ErrorToString(ChipError::StorageType err) { return chip::ErrorStr(CHIP_ERROR(err)); diff --git a/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp b/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp index b1e7fa98c9df1d..9908c53c7eeeae 100644 --- a/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp +++ b/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.cpp @@ -60,6 +60,10 @@ void ScriptDevicePairingDelegate::SetCommissioningFailureCallback(DevicePairingD { mOnCommissioningFailureCallback = callback; } +void ScriptDevicePairingDelegate::SetFabricCheckCallback(DevicePairingDelegate_OnFabricCheckFunct callback) +{ + mOnFabricCheckCallback = callback; +} void ScriptDevicePairingDelegate::SetCommissioningStatusUpdateCallback( DevicePairingDelegate_OnCommissioningStatusUpdateFunct callback) @@ -150,6 +154,23 @@ void ScriptDevicePairingDelegate::OnOpenCommissioningWindow(NodeId deviceId, CHI mWindowOpener = nullptr; } } + +void ScriptDevicePairingDelegate::OnFabricCheck(NodeId matchingNodeId) +{ + if (matchingNodeId == kUndefinedNodeId) + { + ChipLogProgress(Zcl, "No matching fabric found"); + } + else + { + ChipLogProgress(Zcl, "Matching fabric found"); + } + if (mOnFabricCheckCallback != nullptr) + { + mOnFabricCheckCallback(matchingNodeId); + } +} + Callback::Callback * ScriptDevicePairingDelegate::GetOpenWindowCallback(Controller::CommissioningWindowOpener * context) { diff --git a/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.h b/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.h index b93ba94f532270..2740b6eb85e983 100644 --- a/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.h +++ b/src/controller/python/ChipDeviceController-ScriptDevicePairingDelegate.h @@ -46,6 +46,7 @@ typedef void (*DevicePairingDelegate_OnCommissioningFailureFunct)( typedef void (*DevicePairingDelegate_OnCommissioningStatusUpdateFunct)(PeerId peerId, chip::Controller::CommissioningStage stageCompleted, CHIP_ERROR err); +typedef void (*DevicePairingDelegate_OnFabricCheckFunct)(NodeId nodeId); } class ScriptDevicePairingDelegate final : public Controller::DevicePairingDelegate @@ -59,6 +60,7 @@ class ScriptDevicePairingDelegate final : public Controller::DevicePairingDelega void SetCommissioningSuccessCallback(DevicePairingDelegate_OnCommissioningSuccessFunct callback); void SetCommissioningFailureCallback(DevicePairingDelegate_OnCommissioningFailureFunct callback); void SetCommissioningWindowOpenCallback(DevicePairingDelegate_OnWindowOpenCompleteFunct callback); + void SetFabricCheckCallback(DevicePairingDelegate_OnFabricCheckFunct callback); void OnStatusUpdate(Controller::DevicePairingDelegate::Status status) override; void OnPairingComplete(CHIP_ERROR error) override; void OnCommissioningComplete(NodeId nodeId, CHIP_ERROR err) override; @@ -66,6 +68,7 @@ class ScriptDevicePairingDelegate final : public Controller::DevicePairingDelega void OnCommissioningFailure(PeerId peerId, CHIP_ERROR error, CommissioningStage stageFailed, Optional additionalErrorInfo) override; void OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error) override; + void OnFabricCheck(NodeId matchingNodeId) override; Callback::Callback * GetOpenWindowCallback(Controller::CommissioningWindowOpener * context); void OnOpenCommissioningWindow(NodeId deviceId, CHIP_ERROR status, SetupPayload payload); @@ -78,6 +81,7 @@ class ScriptDevicePairingDelegate final : public Controller::DevicePairingDelega DevicePairingDelegate_OnCommissioningSuccessFunct mOnCommissioningSuccessCallback = nullptr; DevicePairingDelegate_OnCommissioningFailureFunct mOnCommissioningFailureCallback = nullptr; DevicePairingDelegate_OnCommissioningStatusUpdateFunct mOnCommissioningStatusUpdateCallback = nullptr; + DevicePairingDelegate_OnFabricCheckFunct mOnFabricCheckCallback = nullptr; Callback::Callback mOpenWindowCallback; Controller::CommissioningWindowOpener * mWindowOpener = nullptr; bool expectingPairingComplete = false; diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py index 5062102d3073e1..55a325fa8d3432 100644 --- a/src/controller/python/chip/ChipDeviceCtrl.py +++ b/src/controller/python/chip/ChipDeviceCtrl.py @@ -70,6 +70,8 @@ None, c_uint64, c_uint32, c_char_p, c_char_p, PyChipError) _DevicePairingDelegate_OnCommissioningStatusUpdateFunct = CFUNCTYPE( None, c_uint64, c_uint8, PyChipError) +_DevicePairingDelegate_OnFabricCheckFunct = CFUNCTYPE( + None, c_uint64) # void (*)(Device *, CHIP_ERROR). # # CHIP_ERROR is actually signed, so using c_uint32 is weird, but everything @@ -248,6 +250,7 @@ def __init__(self, name: str = ''): self.devCtrl = devCtrl self.name = name + self.fabricCheckNodeId = -1 self._Cluster = ChipClusters(builtins.chipStack) self._Cluster.InitLib(self._dmLib) @@ -267,6 +270,9 @@ def HandleCommissioningComplete(nodeid, err): self._ChipStack.commissioningCompleteEvent.set() self._ChipStack.completeEvent.set() + def HandleFabricCheck(nodeId): + self.fabricCheckNodeId = nodeId + def HandleOpenWindowComplete(nodeid: int, setupPinCode: int, setupManualCode: str, setupQRCode: str, err: PyChipError) -> None: if err.is_success: @@ -319,6 +325,9 @@ def HandlePASEEstablishmentComplete(err: PyChipError): self._dmLib.pychip_ScriptDevicePairingDelegate_SetCommissioningCompleteCallback( self.devCtrl, self.cbHandleCommissioningCompleteFunct) + self.cbHandleFabricCheckFunct = _DevicePairingDelegate_OnFabricCheckFunct(HandleFabricCheck) + self._dmLib.pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback(self.cbHandleFabricCheckFunct) + self.cbHandleOpenWindowCompleteFunct = _DevicePairingDelegate_OnOpenWindowCompleteFunct( HandleOpenWindowComplete) self._dmLib.pychip_ScriptDevicePairingDelegate_SetOpenWindowCompleteCallback( @@ -1348,6 +1357,9 @@ def _InitLib(self): self._dmLib.pychip_DeviceController_SetTrustedTimeSource.restype = PyChipError self._dmLib.pychip_DeviceController_SetTrustedTimeSource.argtypes = [c_uint64, c_uint16] + self._dmLib.pychip_DeviceController_SetCheckMatchingFabric.restype = PyChipError + self._dmLib.pychip_DeviceController_SetCheckMatchingFabric.argtypes = [c_bool] + self._dmLib.pychip_DeviceController_ResetCommissioningParameters.restype = PyChipError self._dmLib.pychip_DeviceController_ResetCommissioningParameters.argtypes = [] @@ -1441,6 +1453,10 @@ def _InitLib(self): c_void_p, _DevicePairingDelegate_OnCommissioningStatusUpdateFunct] self._dmLib.pychip_ScriptDevicePairingDelegate_SetCommissioningStatusUpdateCallback.restype = PyChipError + self._dmLib.pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback.argtypes = [ + _DevicePairingDelegate_OnFabricCheckFunct] + self._dmLib.pychip_ScriptDevicePairingDelegate_SetFabricCheckCallback.restype = PyChipError + self._dmLib.pychip_GetConnectedDeviceByNodeId.argtypes = [ c_void_p, c_uint64, _DeviceAvailableFunct] self._dmLib.pychip_GetConnectedDeviceByNodeId.restype = PyChipError @@ -1662,6 +1678,15 @@ def SetTrustedTimeSource(self, nodeId: int, endpoint: int): lambda: self._dmLib.pychip_DeviceController_SetTrustedTimeSource(nodeId, endpoint) ).raise_on_error() + def SetCheckMatchingFabric(self, check: bool): + self.CheckIsActive() + self._ChipStack.Call( + lambda: self._dmLib.pychip_DeviceController_SetCheckMatchingFabric(check) + ).raise_on_error() + + def GetFabricCheckResult(self) -> int: + return self.fabricCheckNodeId + def CommissionOnNetwork(self, nodeId: int, setupPinCode: int, filterType: DiscoveryFilterType = DiscoveryFilterType.NONE, filter: typing.Any = None, discoveryTimeoutMsec: int = 30000) -> PyChipError: ''' diff --git a/src/python_testing/TestCommissioningTimeSync.py b/src/python_testing/TestCommissioningTimeSync.py index efbb77ebdbfdfc..dfd7c55f8d21f4 100644 --- a/src/python_testing/TestCommissioningTimeSync.py +++ b/src/python_testing/TestCommissioningTimeSync.py @@ -26,6 +26,7 @@ # We don't have a good pipe between the c++ enums in CommissioningDelegate and python # so this is hardcoded. # I realize this is dodgy, not sure how to cross the enum from c++ to python cleanly +kCheckForMatchingFabric = 3 kConfigureUTCTime = 6 kConfigureTimeZone = 7 kConfigureDSTOffset = 8 @@ -208,6 +209,27 @@ async def test_CommissioningPreSetValues(self): asserts.assert_false(self.commissioner.CheckStageSuccessful( kConfigureTrustedTimeSource), 'kConfigureTrustedTimeSource incorrectly set') + @async_test_body + async def test_FabricCheckStage(self): + await self.create_commissioner() + + # This was moved into a different stage when the time sync stuff was added + asserts.assert_equal(self.commissioner.GetFabricCheckResult(), -1, "Fabric check result is already set") + self.commissioner.SetCheckMatchingFabric(True) + await self.commission_and_base_checks() + asserts.assert_true(self.commissioner.CheckStageSuccessful( + kCheckForMatchingFabric), "Did not run check for matching fabric stage") + asserts.assert_equal(self.commissioner.GetFabricCheckResult(), 0, "Fabric check result did not get set by pairing delegate") + + # Let's try it again with no check + await self.create_commissioner() + asserts.assert_equal(self.commissioner.GetFabricCheckResult(), -1, "Fabric check result is already set") + self.commissioner.SetCheckMatchingFabric(False) + await self.commission_and_base_checks() + asserts.assert_false(self.commissioner.CheckStageSuccessful( + kCheckForMatchingFabric), "Incorrectly ran check for matching fabric stage") + asserts.assert_equal(self.commissioner.GetFabricCheckResult(), -1, "Fabric check result incorrectly set") + # TODO(cecille): Test - Add hooks to change the time zone response to indicate no DST is needed # TODO(cecille): Test - Set commissioningParameters TimeZone and DST list size to > node list size to ensure they get truncated