Commissioning: Fix fabric check stage and add test

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
     }
 }

src/controller/CHIPDeviceController.cpp

@@ -2154,7 +2154,7 @@ void DeviceCommissioner::ParseFabrics()
 
     if (mPairingDelegate != nullptr)
     {
-        mPairingDelegate->OnFabricCheck(info);
+        mPairingDelegate->OnFabricCheck(info.nodeId);
     }
 
     CommissioningDelegate::CommissioningReport report;
@@ -2403,7 +2403,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");

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

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));

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<Controller::OnOpenCommissioningWindow> *
 ScriptDevicePairingDelegate::GetOpenWindowCallback(Controller::CommissioningWindowOpener * context)
 {

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,13 +60,15 @@ 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;
     void OnCommissioningSuccess(PeerId peerId) override;
     void OnCommissioningFailure(PeerId peerId, CHIP_ERROR error, CommissioningStage stageFailed,
                                 Optional<Credentials::AttestationVerificationResult> additionalErrorInfo) override;
     void OnCommissioningStatusUpdate(PeerId peerId, CommissioningStage stageCompleted, CHIP_ERROR error) override;
+    void OnFabricCheck(NodeId matchingNodeId) override;
     Callback::Callback<Controller::OnOpenCommissioningWindow> *
     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<Controller::OnOpenCommissioningWindow> mOpenWindowCallback;
     Controller::CommissioningWindowOpener * mWindowOpener = nullptr;
     bool expectingPairingComplete                         = false;

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:
         '''

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