[Python] Make Commissioning APIs more pythonic and consistent (project-chip#33905)

* [Python] Make Commissioning APIs more pythonic and consistent

This commit makes the commissioning APIs more pythonic and consistent
by not returning PyChipError but simply raising ChipStackError
exceptions on errors instead.

The return value instead returns the effectively assigned node ID
as defined by the NOC. If the SDK ends up generating that NOC, it
will use the thing passed to PairDevice, so those will match with
what is provided when calling the commissioning API.

* [Python] Adjust tests to use new commissioning error handling

Author: agners

src/controller/python/chip/ChipDeviceCtrl.py

@@ -230,11 +230,14 @@ class CommissionableNode(discovery.CommissionableNode):
     def SetDeviceController(self, devCtrl: 'ChipDeviceController'):
         self._devCtrl = devCtrl
 
-    def Commission(self, nodeId: int, setupPinCode: int) -> PyChipError:
+    def Commission(self, nodeId: int, setupPinCode: int) -> int:
         ''' Commission the device using the device controller discovered this device.
 
         nodeId: The nodeId commissioned to the device
         setupPinCode: The setup pin code of the device
+
+        Returns:
+            - Effective Node ID of the device (as defined by the assigned NOC)
         '''
         return self._devCtrl.CommissionOnNetwork(
             nodeId, setupPinCode, filterType=discovery.FilterType.INSTANCE_NAME, filter=self.instanceName)
@@ -365,7 +368,10 @@ def HandleCommissioningComplete(nodeId: int, err: PyChipError):
                 logging.exception("HandleCommissioningComplete called unexpectedly")
                 return
 
-            self._commissioning_complete_future.set_result(err)
+            if err.is_success:
+                self._commissioning_complete_future.set_result(nodeId)
+            else:
+                self._commissioning_complete_future.set_exception(err.to_exception())
 
         def HandleFabricCheck(nodeId):
             self.fabricCheckNodeId = nodeId
@@ -413,14 +419,17 @@ def HandlePASEEstablishmentComplete(err: PyChipError):
                 # During Commissioning, HandlePASEEstablishmentComplete will also be called.
                 # Only complete the future if PASE session establishment failed.
                 if not err.is_success:
-                    self._commissioning_complete_future.set_result(err)
+                    self._commissioning_complete_future.set_exception(err.to_exception())
                 return
 
             if self._pase_establishment_complete_future is None:
                 logging.exception("HandlePASEEstablishmentComplete called unexpectedly")
                 return
 
-            self._pase_establishment_complete_future.set_result(err)
+            if err.is_success:
+                self._pase_establishment_complete_future.set_result(None)
+            else:
+                self._pase_establishment_complete_future.set_exception(err.to_exception())
 
         self.pairingDelegate = pairingDelegate
         self.devCtrl = devCtrl
@@ -538,7 +547,12 @@ def IsConnected(self):
                 self.devCtrl)
         )
 
-    def ConnectBLE(self, discriminator: int, setupPinCode: int, nodeid: int, isShortDiscriminator: bool = False) -> PyChipError:
+    def ConnectBLE(self, discriminator: int, setupPinCode: int, nodeid: int, isShortDiscriminator: bool = False) -> int:
+        """Connect to a BLE device using the given discriminator and setup pin code.
+
+        Returns:
+            - Effective Node ID of the device (as defined by the assigned NOC)
+        """
         self.CheckIsActive()
 
         self._commissioning_complete_future = concurrent.futures.Future()
@@ -550,11 +564,7 @@ def ConnectBLE(self, discriminator: int, setupPinCode: int, nodeid: int, isShort
                     self.devCtrl, discriminator, isShortDiscriminator, setupPinCode, nodeid)
             ).raise_on_error()
 
-            # TODO: Change return None. Only returning on success is not useful.
-            # but that is what the previous implementation did.
-            res = self._commissioning_complete_future.result()
-            res.raise_on_error()
-            return res
+            return self._commissioning_complete_future.result()
         finally:
             self._commissioning_complete_future = None
 
@@ -600,7 +610,7 @@ def CloseSession(self, nodeid):
                 self.devCtrl, nodeid)
         ).raise_on_error()
 
-    def EstablishPASESessionBLE(self, setupPinCode: int, discriminator: int, nodeid: int):
+    def EstablishPASESessionBLE(self, setupPinCode: int, discriminator: int, nodeid: int) -> None:
         self.CheckIsActive()
 
         self._pase_establishment_complete_future = concurrent.futures.Future()
@@ -611,16 +621,11 @@ def EstablishPASESessionBLE(self, setupPinCode: int, discriminator: int, nodeid:
                     self.devCtrl, setupPinCode, discriminator, nodeid)
             ).raise_on_error()
 
-            # TODO: This is a bit funky, but what the API returned with the previous
-            # implementation. We should revisit this.
-            err = self._pase_establishment_complete_future.result()
-            if not err.is_success:
-                return err.to_exception()
-            return None
+            self._pase_establishment_complete_future.result()
         finally:
             self._pase_establishment_complete_future = None
 
-    def EstablishPASESessionIP(self, ipaddr: str, setupPinCode: int, nodeid: int, port: int = 0):
+    def EstablishPASESessionIP(self, ipaddr: str, setupPinCode: int, nodeid: int, port: int = 0) -> None:
         self.CheckIsActive()
 
         self._pase_establishment_complete_future = concurrent.futures.Future()
@@ -631,16 +636,11 @@ def EstablishPASESessionIP(self, ipaddr: str, setupPinCode: int, nodeid: int, po
                     self.devCtrl, ipaddr.encode("utf-8"), setupPinCode, nodeid, port)
             ).raise_on_error()
 
-            # TODO: This is a bit funky, but what the API returned with the previous
-            # implementation. We should revisit this.
-            err = self._pase_establishment_complete_future.result()
-            if not err.is_success:
-                return err.to_exception()
-            return None
+            self._pase_establishment_complete_future.result()
         finally:
             self._pase_establishment_complete_future = None
 
-    def EstablishPASESession(self, setUpCode: str, nodeid: int):
+    def EstablishPASESession(self, setUpCode: str, nodeid: int) -> None:
         self.CheckIsActive()
 
         self._pase_establishment_complete_future = concurrent.futures.Future()
@@ -651,12 +651,7 @@ def EstablishPASESession(self, setUpCode: str, nodeid: int):
                     self.devCtrl, setUpCode.encode("utf-8"), nodeid)
             ).raise_on_error()
 
-            # TODO: This is a bit funky, but what the API returned with the previous
-            # implementation. We should revisit this.
-            err = self._pase_establishment_complete_future.result()
-            if not err.is_success:
-                return err.to_exception()
-            return None
+            self._pase_establishment_complete_future.result()
         finally:
             self._pase_establishment_complete_future = None
 
@@ -1874,17 +1869,19 @@ def caIndex(self) -> int:
     def fabricAdmin(self) -> FabricAdmin:
         return self._fabricAdmin
 
-    def Commission(self, nodeid) -> PyChipError:
+    def Commission(self, nodeid) -> int:
         '''
         Start the auto-commissioning process on a node after establishing a PASE connection.
         This function is intended to be used in conjunction with `EstablishPASESessionBLE` or
         `EstablishPASESessionIP`. It can be called either before or after the DevicePairingDelegate
         receives the OnPairingComplete call. Commissioners that want to perform simple
-        auto-commissioning should use the supplied "PairDevice" functions above, which will
+        auto-commissioning should use the supplied "CommissionWithCode" function, which will
         establish the PASE connection and commission automatically.
 
-        Return:
-          bool: True if successful, False otherwise.
+        Raises a ChipStackError on failure.
+
+        Returns:
+            - Effective Node ID of the device (as defined by the assigned NOC)
         '''
         self.CheckIsActive()
 
@@ -1900,13 +1897,13 @@ def Commission(self, nodeid) -> PyChipError:
         finally:
             self._commissioning_complete_future = None
 
-    def CommissionThread(self, discriminator, setupPinCode, nodeId, threadOperationalDataset: bytes, isShortDiscriminator: bool = False) -> PyChipError:
+    def CommissionThread(self, discriminator, setupPinCode, nodeId, threadOperationalDataset: bytes, isShortDiscriminator: bool = False) -> int:
         ''' Commissions a Thread device over BLE
         '''
         self.SetThreadOperationalDataset(threadOperationalDataset)
         return self.ConnectBLE(discriminator, setupPinCode, nodeId, isShortDiscriminator)
 
-    def CommissionWiFi(self, discriminator, setupPinCode, nodeId, ssid: str, credentials: str, isShortDiscriminator: bool = False) -> PyChipError:
+    def CommissionWiFi(self, discriminator, setupPinCode, nodeId, ssid: str, credentials: str, isShortDiscriminator: bool = False) -> int:
         ''' Commissions a Wi-Fi device over BLE.
         '''
         self.SetWiFiCredentials(ssid, credentials)
@@ -2009,7 +2006,7 @@ 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:
+                            filterType: DiscoveryFilterType = DiscoveryFilterType.NONE, filter: typing.Any = None, discoveryTimeoutMsec: int = 30000) -> int:
         '''
         Does the routine for OnNetworkCommissioning, with a filter for mDNS discovery.
         Supported filters are:
@@ -2025,6 +2022,11 @@ def CommissionOnNetwork(self, nodeId: int, setupPinCode: int,
             DiscoveryFilterType.COMPRESSED_FABRIC_ID
 
         The filter can be an integer, a string or None depending on the actual type of selected filter.
+
+        Raises a ChipStackError on failure.
+
+        Returns:
+            - Effective Node ID of the device (as defined by the assigned NOC)
         '''
         self.CheckIsActive()
 
@@ -2044,9 +2046,14 @@ def CommissionOnNetwork(self, nodeId: int, setupPinCode: int,
         finally:
             self._commissioning_complete_future = None
 
-    def CommissionWithCode(self, setupPayload: str, nodeid: int, discoveryType: DiscoveryType = DiscoveryType.DISCOVERY_ALL) -> PyChipError:
+    def CommissionWithCode(self, setupPayload: str, nodeid: int, discoveryType: DiscoveryType = DiscoveryType.DISCOVERY_ALL) -> int:
         ''' Commission with the given nodeid from the setupPayload.
             setupPayload may be a QR or manual code.
+
+            Raises a ChipStackError on failure.
+
+        Returns:
+            - Effective Node ID of the device (as defined by the assigned NOC)
         '''
         self.CheckIsActive()
 
@@ -2063,8 +2070,14 @@ def CommissionWithCode(self, setupPayload: str, nodeid: int, discoveryType: Disc
         finally:
             self._commissioning_complete_future = None
 
-    def CommissionIP(self, ipaddr: str, setupPinCode: int, nodeid: int) -> PyChipError:
-        """ DEPRECATED, DO NOT USE! Use `CommissionOnNetwork` or `CommissionWithCode` """
+    def CommissionIP(self, ipaddr: str, setupPinCode: int, nodeid: int) -> int:
+        """ DEPRECATED, DO NOT USE! Use `CommissionOnNetwork` or `CommissionWithCode`
+
+        Raises a ChipStackError on failure.
+
+        Returns:
+            - Effective Node ID of the device (as defined by the assigned NOC)
+        """
         self.CheckIsActive()
 
         self._commissioning_complete_future = concurrent.futures.Future()

src/controller/python/chip/yaml/runner.py

@@ -664,10 +664,10 @@ async def run_action(self, dev_ctrl: ChipDeviceController) -> _ActionResult:
         if self._command == 'GetCommissionerNodeId':
             return _ActionResult(status=_ActionStatus.SUCCESS, response=_GetCommissionerNodeIdResult(dev_ctrl.nodeId))
 
-        resp = dev_ctrl.CommissionWithCode(self._setup_payload, self._node_id)
-        if resp:
+        try:
+            dev_ctrl.CommissionWithCode(self._setup_payload, self._node_id)
             return _ActionResult(status=_ActionStatus.SUCCESS, response=None)
-        else:
+        except ChipStackError:
             return _ActionResult(status=_ActionStatus.ERROR, response=None)
 
 

src/controller/python/test/test_scripts/base.py

@@ -41,6 +41,7 @@
 from chip import ChipDeviceCtrl
 from chip.ChipStack import ChipStack
 from chip.crypto import p256keypair
+from chip.exceptions import ChipStackException
 from chip.utils import CommissioningBuildingBlocks
 from cirque_restart_remote_device import restartRemoteDevice
 from ecdsa import NIST256p
@@ -256,8 +257,9 @@ def TestPaseOnly(self, ip: str, setuppin: int, nodeid: int, devCtrl=None):
             devCtrl = self.devCtrl
         self.logger.info(
             "Attempting to establish PASE session with device id: {} addr: {}".format(str(nodeid), ip))
-        if devCtrl.EstablishPASESessionIP(
-                ip, setuppin, nodeid) is not None:
+        try:
+            devCtrl.EstablishPASESessionIP(ip, setuppin, nodeid)
+        except ChipStackException:
             self.logger.info(
                 "Failed to establish PASE session with device id: {} addr: {}".format(str(nodeid), ip))
             return False
@@ -268,7 +270,9 @@ def TestPaseOnly(self, ip: str, setuppin: int, nodeid: int, devCtrl=None):
     def TestCommissionOnly(self, nodeid: int):
         self.logger.info(
             "Commissioning device with id {}".format(nodeid))
-        if not self.devCtrl.Commission(nodeid):
+        try:
+            self.devCtrl.Commission(nodeid)
+        except ChipStackException:
             self.logger.info(
                 "Failed to commission device with id {}".format(str(nodeid)))
             return False
@@ -311,17 +315,21 @@ def TestCommissionFailureOnReport(self, nodeid: int, failAfter: int):
 
     def TestCommissioning(self, ip: str, setuppin: int, nodeid: int):
         self.logger.info("Commissioning device {}".format(ip))
-        if not self.devCtrl.CommissionIP(ip, setuppin, nodeid):
-            self.logger.info(
+        try:
+            self.devCtrl.CommissionIP(ip, setuppin, nodeid)
+        except ChipStackException:
+            self.logger.exception(
                 "Failed to finish commissioning device {}".format(ip))
             return False
         self.logger.info("Commissioning finished.")
         return True
 
     def TestCommissioningWithSetupPayload(self, setupPayload: str, nodeid: int, discoveryType: int = 2):
         self.logger.info("Commissioning device with setup payload {}".format(setupPayload))
-        if not self.devCtrl.CommissionWithCode(setupPayload, nodeid, chip.discovery.DiscoveryType(discoveryType)):
-            self.logger.info(
+        try:
+            self.devCtrl.CommissionWithCode(setupPayload, nodeid, chip.discovery.DiscoveryType(discoveryType))
+        except ChipStackException:
+            self.logger.exception(
                 "Failed to finish commissioning device {}".format(setupPayload))
             return False
         self.logger.info("Commissioning finished.")
@@ -783,8 +791,10 @@ async def TestMultiFabric(self, ip: str, setuppin: int, nodeid: int):
         self.devCtrl2 = self.fabricAdmin2.NewController(
             self.controllerNodeId, self.paaTrustStorePath)
 
-        if not self.devCtrl2.CommissionIP(ip, setuppin, nodeid):
-            self.logger.info(
+        try:
+            self.devCtrl2.CommissionIP(ip, setuppin, nodeid)
+        except ChipStackException:
+            self.logger.exception(
                 "Failed to finish key exchange with device {}".format(ip))
             return False
 

src/python_testing/TC_ACE_1_5.py

@@ -54,10 +54,10 @@ async def test_TC_ACE_1_5(self):
         params = self.openCommissioningWindow(self.th1, self.dut_node_id)
         self.print_step(2, "TH1 opens the commissioning window on the DUT")
 
-        errcode = self.th2.CommissionOnNetwork(
+        self.th2.CommissionOnNetwork(
             nodeId=self.dut_node_id, setupPinCode=params.commissioningParameters.setupPinCode,
             filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=params.randomDiscriminator)
-        logging.info('Commissioning complete done. Successful? {}, errorcode = {}'.format(errcode.is_success, errcode))
+        logging.info('Commissioning complete done. Successful.')
         self.print_step(3, "TH2 commissions DUT using admin node ID N2")
 
         self.print_step(4, "TH2 reads its fabric index from the Operational Credentials cluster CurrentFabricIndex attribute")

src/python_testing/TC_CGEN_2_4.py

@@ -25,6 +25,7 @@
 import chip.FabricAdmin
 from chip import ChipDeviceCtrl
 from chip.ChipDeviceCtrl import CommissioningParameters
+from chip.exceptions import ChipStackError
 from matter_testing_support import MatterBaseTest, async_test_body, default_matter_test_main
 from mobly import asserts
 
@@ -60,11 +61,12 @@ async def CommissionToStageSendCompleteAndCleanup(
         # This will run the commissioning up to the point where stage x is run and the
         # response is sent before the test commissioner simulates a failure
         self.th2.SetTestCommissionerPrematureCompleteAfter(stage)
-        errcode = self.th2.CommissionOnNetwork(
-            nodeId=self.dut_node_id, setupPinCode=params.setupPinCode,
-            filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=self.discriminator)
-        logging.info('Commissioning complete done. Successful? {}, errorcode = {}'.format(errcode.is_success, errcode))
-        asserts.assert_false(errcode.is_success, 'Commissioning complete did not error as expected')
+        ctx = asserts.assert_raises(ChipStackError)
+        with ctx:
+            self.th2.CommissionOnNetwork(
+                nodeId=self.dut_node_id, setupPinCode=params.setupPinCode,
+                filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=self.discriminator)
+        errcode = ctx.exception.chip_error
         asserts.assert_true(errcode.sdk_part == expectedErrorPart, 'Unexpected error type returned from CommissioningComplete')
         asserts.assert_true(errcode.sdk_code == expectedErrCode, 'Unexpected error code returned from CommissioningComplete')
         revokeCmd = Clusters.AdministratorCommissioning.Commands.RevokeCommissioning()
@@ -101,10 +103,14 @@ async def test_TC_CGEN_2_4(self):
 
         logging.info('Step 16 - TH2 fully commissions the DUT')
         self.th2.ResetTestCommissioner()
-        errcode = self.th2.CommissionOnNetwork(
-            nodeId=self.dut_node_id, setupPinCode=params.setupPinCode,
-            filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=self.discriminator)
-        logging.info('Commissioning complete done. Successful? {}, errorcode = {}'.format(errcode.is_success, errcode))
+
+        ctx = asserts.assert_raises(ChipStackError)
+        with ctx:
+            self.th2.CommissionOnNetwork(
+                nodeId=self.dut_node_id, setupPinCode=params.setupPinCode,
+                filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=self.discriminator)
+        asserts.assert_true(ctx.exception.chip_error.sdk_code == 0x02, 'Unexpected error code returned from CommissioningComplete')
+        logging.info('Commissioning complete done.')
 
         logging.info('Step 17 - TH1 sends an arm failsafe')
         cmd = Clusters.GeneralCommissioning.Commands.ArmFailSafe(expiryLengthSeconds=900, breadcrumb=0)

src/python_testing/TC_DA_1_5.py

@@ -170,10 +170,9 @@ async def test_TC_DA_1_5(self):
         new_fabric_admin = new_certificate_authority.NewFabricAdmin(vendorId=0xFFF1, fabricId=2)
         TH2 = new_fabric_admin.NewController(nodeId=112233)
 
-        errcode = TH2.CommissionOnNetwork(
+        TH2.CommissionOnNetwork(
             nodeId=self.dut_node_id, setupPinCode=params.setupPinCode,
             filterType=ChipDeviceCtrl.DiscoveryFilterType.LONG_DISCRIMINATOR, filter=1234)
-        asserts.assert_true(errcode.is_success, 'Commissioning on TH2 did not complete successfully')
 
         self.print_step(15, "Read NOCs list for TH1")
         temp = await self.read_single_attribute_check_success(