[Python] Store original PyChipError in ChipStackException (#33954)

agners · web-flow · commit ab2907b5d3fe · 2024-06-18T12:45:11.000Z
* [Python] Drop unused ErrorToException function

Also remove the now unused pychip_Stack_ErrorToString function. This
is handled in pychip_FormatError today.

* [Python] Cleanup PyChipError return values

Use PyChipError return value where PyChipErrors are actually returned.
This then also allows us to use the typical .raise_on_error() pattern.

* [Python] Store original PyChipError in ChipStackException

This change stores the original PyChipError in ChipStackException so
that details of the original error code can still be retrieved. This
is interesting to use the properties returning processed information
about the original error code. It also preserves the line and code
file which can be helpful.

* [Python] Fix Command API argument type errors

NativeLibraryHandleMethodArguments correctly setting the arguments
uncovered some incorrectly set arguments.

* [Python] Use to_exception() to convert PyChipError to ChipStackError

* [Python] Fix Cert API argument type errors

NativeLibraryHandleMethodArguments correctly setting the argument
types causes argument type errors:
ctypes.ArgumentError: argument 1: TypeError: expected LP_c_ubyte instance instead of bytes

We can safely cast bytes as the native side marks it const.
diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp
@@ -210,7 +210,6 @@ pychip_ScriptDevicePairingDelegate_SetExpectingPairingComplete(chip::Controller:
 // BLE
 PyChipError pychip_DeviceCommissioner_CloseBleConnection(chip::Controller::DeviceCommissioner * devCtrl);
 
-const char * pychip_Stack_ErrorToString(ChipError::StorageType err);
 const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t statusCode);
 
 PyChipError pychip_GetConnectedDeviceByNodeId(chip::Controller::DeviceCommissioner * devCtrl, chip::NodeId nodeId,
@@ -366,11 +365,6 @@ PyChipError pychip_DeviceController_GetNodeId(chip::Controller::DeviceCommission
     return ToPyChipError(CHIP_NO_ERROR);
 }
 
-const char * pychip_DeviceController_ErrorToString(ChipError::StorageType err)
-{
-    return chip::ErrorStr(CHIP_ERROR(err));
-}
-
 const char * pychip_DeviceController_StatusReportToString(uint32_t profileId, uint16_t statusCode)
 {
     // return chip::StatusReportStr(profileId, statusCode);
@@ -769,11 +763,6 @@ pychip_ScriptDevicePairingDelegate_SetExpectingPairingComplete(chip::Controller:
     return ToPyChipError(CHIP_NO_ERROR);
 }
 
-const char * pychip_Stack_ErrorToString(ChipError::StorageType err)
-{
-    return chip::ErrorStr(CHIP_ERROR(err));
-}
-
 const char * pychip_Stack_StatusReportToString(uint32_t profileId, uint16_t statusCode)
 {
     // return chip::StatusReportStr(profileId, statusCode);
diff --git a/src/controller/python/chip/ChipDeviceCtrl.py b/src/controller/python/chip/ChipDeviceCtrl.py
@@ -1013,12 +1013,8 @@ def GetRemoteSessionParameters(self, nodeid) -> typing.Optional[SessionParameter
         sessionParametersStruct = SessionParametersStruct.parse(b'\x00' * SessionParametersStruct.sizeof())
         sessionParametersByteArray = SessionParametersStruct.build(sessionParametersStruct)
         device = self.GetConnectedDeviceSync(nodeid)
-        res = self._ChipStack.Call(lambda: self._dmLib.pychip_DeviceProxy_GetRemoteSessionParameters(
-            device.deviceProxy, ctypes.c_char_p(sessionParametersByteArray)))
-
-        # 0 is CHIP_NO_ERROR
-        if res != 0:
-            return None
+        self._ChipStack.Call(lambda: self._dmLib.pychip_DeviceProxy_GetRemoteSessionParameters(
+            device.deviceProxy, ctypes.c_char_p(sessionParametersByteArray))).raise_on_error()
 
         sessionParametersStruct = SessionParametersStruct.parse(sessionParametersByteArray)
         return SessionParameters(
@@ -1030,8 +1026,6 @@ def GetRemoteSessionParameters(self, nodeid) -> typing.Optional[SessionParameter
             specficiationVersion=sessionParametersStruct.SpecificationVersion if sessionParametersStruct.SpecificationVersion != 0 else None,
             maxPathsPerInvoke=sessionParametersStruct.MaxPathsPerInvoke)
 
-        return res
-
     async def TestOnlySendBatchCommands(self, nodeid: int, commands: typing.List[ClusterCommand.InvokeRequestInfo],
                                         timedRequestTimeoutMs: typing.Optional[int] = None,
                                         interactionTimeoutMs: typing.Optional[int] = None, busyWaitMs: typing.Optional[int] = None,
@@ -1804,6 +1798,9 @@ def _InitLib(self):
 
             self._dmLib.pychip_CheckInDelegate_SetOnCheckInCompleteCallback(_OnCheckInComplete)
 
+            self._dmLib.pychip_DeviceProxy_GetRemoteSessionParameters.restype = PyChipError
+            self._dmLib.pychip_DeviceProxy_GetRemoteSessionParameters.argtypes = [c_void_p, c_char_p]
+
 
 class ChipDeviceController(ChipDeviceControllerBase):
     ''' The ChipDeviceCommissioner binding, named as ChipDeviceController
diff --git a/src/controller/python/chip/ChipStack.py b/src/controller/python/chip/ChipStack.py
@@ -35,7 +35,6 @@
 import chip.native
 from chip.native import PyChipError
 
-from .ChipUtility import ChipUtility
 from .clusters import Attribute as ClusterAttribute
 from .clusters import Command as ClusterCommand
 from .exceptions import ChipStackError, ChipStackException, DeviceError
@@ -247,33 +246,6 @@ def PostTaskOnChipThread(self, callFunct) -> AsyncCallableHandle:
             raise res.to_exception()
         return callObj
 
-    def ErrorToException(self, err, devStatusPtr=None):
-        if err == 0x2C and devStatusPtr:
-            devStatus = devStatusPtr.contents
-            msg = ChipUtility.CStringToString(
-                (
-                    self._ChipStackLib.pychip_Stack_StatusReportToString(
-                        devStatus.ProfileId, devStatus.StatusCode
-                    )
-                )
-            )
-            sysErrorCode = (
-                devStatus.SysErrorCode if (
-                    devStatus.SysErrorCode != 0) else None
-            )
-            if sysErrorCode is not None:
-                msg = msg + " (system err %d)" % (sysErrorCode)
-            return DeviceError(
-                devStatus.ProfileId, devStatus.StatusCode, sysErrorCode, msg
-            )
-        else:
-            return ChipStackError(
-                err,
-                ChipUtility.CStringToString(
-                    (self._ChipStackLib.pychip_Stack_ErrorToString(err))
-                ),
-            )
-
     def LocateChipDLL(self):
         self._loadLib()
         return self._chipDLLPath
@@ -302,8 +274,6 @@ def _loadLib(self):
                 c_uint16,
             ]
             self._ChipStackLib.pychip_Stack_StatusReportToString.restype = c_char_p
-            self._ChipStackLib.pychip_Stack_ErrorToString.argtypes = [c_uint32]
-            self._ChipStackLib.pychip_Stack_ErrorToString.restype = c_char_p
 
             self._ChipStackLib.pychip_DeviceController_PostTaskOnChipThread.argtypes = [
                 _ChipThreadTaskRunnerFunct, py_object]
diff --git a/src/controller/python/chip/clusters/Attribute.py b/src/controller/python/chip/clusters/Attribute.py
@@ -651,7 +651,7 @@ def __init__(self, future: Future, eventLoop, devCtrl, returnClusterObject: bool
         self._changedPathSet = set()
         self._pReadClient = None
         self._pReadCallback = None
-        self._resultError = None
+        self._resultError: Optional[PyChipError] = None
 
     def SetClientObjPointers(self, pReadClient, pReadCallback):
         self._pReadClient = pReadClient
@@ -718,7 +718,7 @@ def handleEventData(self, header: EventHeader, path: EventPath, data: bytes, sta
             logging.exception(ex)
 
     def handleError(self, chipError: PyChipError):
-        self._resultError = chipError.code
+        self._resultError = chipError
 
     def _handleSubscriptionEstablished(self, subscriptionId):
         if not self._future.done():
@@ -777,11 +777,11 @@ def _handleDone(self):
         # move on, possibly invalidating the provided _event_loop.
         #
         if not self._future.done():
-            if self._resultError:
+            if self._resultError is not None:
                 if self._subscription_handler:
-                    self._subscription_handler.OnErrorCb(self._resultError, self._subscription_handler)
+                    self._subscription_handler.OnErrorCb(self._resultError.code, self._subscription_handler)
                 else:
-                    self._future.set_exception(chip.exceptions.ChipStackError(self._resultError))
+                    self._future.set_exception(self._resultError.to_exception())
             else:
                 self._future.set_result(AsyncReadTransaction.ReadResponse(
                     attributes=self._cache.attributeCache, events=self._events, tlvAttributes=self._cache.attributeTLVCache))
@@ -809,7 +809,7 @@ def __init__(self, future: Future, eventLoop):
         self._event_loop = eventLoop
         self._future = future
         self._resultData = []
-        self._resultError = None
+        self._resultError: Optional[PyChipError] = None
 
     def handleResponse(self, path: AttributePath, status: int):
         try:
diff --git a/src/controller/python/chip/clusters/Command.py b/src/controller/python/chip/clusters/Command.py
@@ -467,13 +467,13 @@ def Init():
         setter = chip.native.NativeLibraryHandleMethodArguments(handle)
 
         setter.Set('pychip_CommandSender_SendCommand',
-                   PyChipError, [py_object, c_void_p, c_uint16, c_uint32, c_uint32, c_char_p, c_size_t, c_uint16, c_bool])
+                   PyChipError, [py_object, c_void_p, c_uint16, c_uint16, c_uint32, c_uint32, c_char_p, c_size_t, c_uint16, c_uint16, c_bool])
         setter.Set('pychip_CommandSender_SendBatchCommands',
                    PyChipError, [py_object, c_void_p, c_uint16, c_uint16, c_uint16, c_bool, POINTER(PyInvokeRequestData), c_size_t])
         setter.Set('pychip_CommandSender_TestOnlySendBatchCommands',
                    PyChipError, [py_object, c_void_p, c_uint16, c_uint16, c_uint16, c_bool, TestOnlyPyBatchCommandsOverrides, POINTER(PyInvokeRequestData), c_size_t])
         setter.Set('pychip_CommandSender_TestOnlySendCommandTimedRequestNoTimedInvoke',
-                   PyChipError, [py_object, c_void_p, c_uint32, c_uint32, c_char_p, c_size_t, c_uint16, c_bool])
+                   PyChipError, [py_object, c_void_p, c_uint16, c_uint32, c_uint32, c_char_p, c_size_t, c_uint16, c_uint16, c_bool])
         setter.Set('pychip_CommandSender_SendGroupCommand',
                    PyChipError, [c_uint16, c_void_p, c_uint32, c_uint32, c_char_p, c_size_t, c_uint16])
         setter.Set('pychip_CommandSender_InitCallbacks', None, [
diff --git a/src/controller/python/chip/credentials/cert.py b/src/controller/python/chip/credentials/cert.py
@@ -35,8 +35,10 @@ def convert_x509_cert_to_chip_cert(x509Cert: bytes) -> bytes:
     """Converts a x509 certificate to CHIP Certificate."""
     output_buffer = (ctypes.c_uint8 * 1024)()
     output_size = ctypes.c_size_t(1024)
+    ptr_type = ctypes.POINTER(ctypes.c_uint8)
 
-    _handle().pychip_ConvertX509CertToChipCert(x509Cert, len(x509Cert), output_buffer, ctypes.byref(output_size)).raise_on_error()
+    _handle().pychip_ConvertX509CertToChipCert(ctypes.cast(x509Cert, ptr_type), len(x509Cert),
+                                               ctypes.cast(output_buffer, ptr_type), ctypes.byref(output_size)).raise_on_error()
 
     return bytes(output_buffer)[:output_size.value]
 
@@ -45,7 +47,9 @@ def convert_chip_cert_to_x509_cert(chipCert: bytes) -> bytes:
     """Converts a x509 certificate to CHIP Certificate."""
     output_buffer = (ctypes.c_byte * 1024)()
     output_size = ctypes.c_size_t(1024)
+    ptr_type = ctypes.POINTER(ctypes.c_uint8)
 
-    _handle().pychip_ConvertChipCertToX509Cert(chipCert, len(chipCert), output_buffer, ctypes.byref(output_size)).raise_on_error()
+    _handle().pychip_ConvertChipCertToX509Cert(ctypes.cast(chipCert, ptr_type), len(chipCert),
+                                               ctypes.cast(output_buffer, ptr_type), ctypes.byref(output_size)).raise_on_error()
 
     return bytes(output_buffer)[:output_size.value]
diff --git a/src/controller/python/chip/discovery/__init__.py b/src/controller/python/chip/discovery/__init__.py
@@ -236,8 +236,7 @@ def FindAddressAsync(fabricid: int, nodeid: int, callback, timeout_ms=1000):
     )
 
     res = _GetDiscoveryLibraryHandle().pychip_discovery_resolve(fabricid, nodeid)
-    if res != 0:
-        raise Exception("Failed to start node resolution")
+    res.raise_on_error()
 
 
 class _SyncAddressFinder:
diff --git a/src/controller/python/chip/exceptions/__init__.py b/src/controller/python/chip/exceptions/__init__.py
@@ -15,6 +15,8 @@
 #    limitations under the License.
 #
 
+from __future__ import annotations
+
 __all__ = [
     "ChipStackException",
     "ChipStackError",
@@ -26,15 +28,32 @@
     "UnknownCommand",
 ]
 
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+    from chip.native import PyChipError
+
 
 class ChipStackException(Exception):
     pass
 
 
 class ChipStackError(ChipStackException):
-    def __init__(self, err, msg=None):
-        self.err = err
-        self.msg = msg if msg else "Chip Stack Error %d" % err
+    def __init__(self, chip_error: PyChipError, msg=None):
+        self._chip_error = chip_error
+        self.msg = msg if msg else "Chip Stack Error %d" % chip_error.code
+
+    @classmethod
+    def from_chip_error(cls, chip_error: PyChipError) -> ChipStackError:
+        return cls(chip_error, str(chip_error))
+
+    @property
+    def chip_error(self) -> PyChipError | None:
+        return self._chip_error
+
+    @property
+    def err(self) -> int:
+        return self._chip_error.code
 
     def __str__(self):
         return self.msg
diff --git a/src/controller/python/chip/interaction_model/delegate.py b/src/controller/python/chip/interaction_model/delegate.py
@@ -330,7 +330,7 @@ def InitIMDelegate():
         setter.Set("pychip_InteractionModelDelegate_SetCommandResponseErrorCallback", None, [
                    _OnCommandResponseFunct])
         setter.Set("pychip_InteractionModel_GetCommandSenderHandle",
-                   c_uint32, [ctypes.POINTER(c_uint64)])
+                   chip.native.PyChipError, [ctypes.POINTER(c_uint64)])
         setter.Set("pychip_InteractionModelDelegate_SetOnWriteResponseStatusCallback", None, [
                    _OnWriteResponseStatusFunct])
 
@@ -389,10 +389,8 @@ def WaitCommandIndexStatus(commandHandle: int, commandIndex: int):
 def GetCommandSenderHandle() -> int:
     handle = chip.native.GetLibraryHandle()
     resPointer = c_uint64()
-    res = handle.pychip_InteractionModel_GetCommandSenderHandle(
-        ctypes.pointer(resPointer))
-    if res != 0:
-        raise chip.exceptions.ChipStackError(res)
+    handle.pychip_InteractionModel_GetCommandSenderHandle(
+        ctypes.pointer(resPointer)).raise_on_error()
     ClearCommandStatus(resPointer.value)
     return resPointer.value
 
diff --git a/src/controller/python/chip/native/__init__.py b/src/controller/python/chip/native/__init__.py
@@ -116,7 +116,7 @@ def sdk_code(self) -> int:
 
     def to_exception(self) -> typing.Union[None, chip.exceptions.ChipStackError]:
         if not self.is_success:
-            return chip.exceptions.ChipStackError(self.code, str(self))
+            return chip.exceptions.ChipStackError.from_chip_error(self)
 
     def __str__(self):
         buf = ctypes.create_string_buffer(256)
@@ -199,7 +199,7 @@ def __init__(self, handle):
     def Set(self, methodName: str, resultType, argumentTypes: list):
         method = getattr(self.handle, methodName)
         method.restype = resultType
-        method.argtype = argumentTypes
+        method.argtypes = argumentTypes
 
 
 @dataclass
diff --git a/src/controller/python/chip/setup_payload/setup_payload.py b/src/controller/python/chip/setup_payload/setup_payload.py
@@ -14,11 +14,10 @@
 #    limitations under the License.
 #
 
-from ctypes import CFUNCTYPE, c_char_p, c_int32, c_uint8, c_uint16, c_uint32
+from ctypes import CFUNCTYPE, c_char_p, c_uint8, c_uint16, c_uint32
 from typing import Optional
 
-from chip.exceptions import ChipStackError
-from chip.native import GetLibraryHandle, NativeLibraryHandleMethodArguments
+from chip.native import GetLibraryHandle, NativeLibraryHandleMethodArguments, PyChipError
 
 
 class SetupPayload:
@@ -46,34 +45,25 @@ def AddVendorAttribute(tag, value):
 
     def ParseQrCode(self, qrCode: str):
         self.Clear()
-        err = self.chipLib.pychip_SetupPayload_ParseQrCode(qrCode.upper().encode(),
-                                                           self.attribute_visitor,
-                                                           self.vendor_attribute_visitor)
-
-        if err != 0:
-            raise ChipStackError(err)
+        self.chipLib.pychip_SetupPayload_ParseQrCode(qrCode.upper().encode(),
+                                                     self.attribute_visitor,
+                                                     self.vendor_attribute_visitor).raise_on_error()
 
         return self
 
     def ParseManualPairingCode(self, manualPairingCode: str):
         self.Clear()
-        err = self.chipLib.pychip_SetupPayload_ParseManualPairingCode(manualPairingCode.encode(),
-                                                                      self.attribute_visitor,
-                                                                      self.vendor_attribute_visitor)
-
-        if err != 0:
-            raise ChipStackError(err)
+        self.chipLib.pychip_SetupPayload_ParseManualPairingCode(manualPairingCode.encode(),
+                                                                self.attribute_visitor,
+                                                                self.vendor_attribute_visitor).raise_on_error()
 
         return self
 
     # DEPRECATED
     def PrintOnboardingCodes(self, passcode, vendorId, productId, discriminator, customFlow, capabilities, version):
         self.Clear()
-        err = self.chipLib.pychip_SetupPayload_PrintOnboardingCodes(
-            passcode, vendorId, productId, discriminator, customFlow, capabilities, version)
-
-        if err != 0:
-            raise ChipStackError(err)
+        self.chipLib.pychip_SetupPayload_PrintOnboardingCodes(
+            passcode, vendorId, productId, discriminator, customFlow, capabilities, version).raise_on_error()
 
     # DEPRECATED
     def Print(self):
@@ -106,17 +96,17 @@ def __DecorateValue(self, name, value):
         return None
 
     def __InitNativeFunctions(self, chipLib):
-        if chipLib.pychip_SetupPayload_ParseQrCode is not None:
+        if chipLib.pychip_SetupPayload_ParseQrCode.argtypes is not None:
             return
         setter = NativeLibraryHandleMethodArguments(chipLib)
         setter.Set("pychip_SetupPayload_ParseQrCode",
-                   c_int32,
+                   PyChipError,
                    [c_char_p, SetupPayload.AttributeVisitor, SetupPayload.VendorAttributeVisitor])
         setter.Set("pychip_SetupPayload_ParseManualPairingCode",
-                   c_int32,
+                   PyChipError,
                    [c_char_p, SetupPayload.AttributeVisitor, SetupPayload.VendorAttributeVisitor])
         setter.Set("pychip_SetupPayload_PrintOnboardingCodes",
-                   c_int32,
+                   PyChipError,
                    [c_uint32, c_uint16, c_uint16, c_uint16, c_uint8, c_uint8, c_uint8])
 
     # Getters from parsed contents.

Original file line number	Diff line number	Diff line change
`@@ -236,8 +236,7 @@ def FindAddressAsync(fabricid: int, nodeid: int, callback, timeout_ms=1000):`
`236`	`236`	`)`
`237`	`237`
`238`	`238`	`res = _GetDiscoveryLibraryHandle().pychip_discovery_resolve(fabricid, nodeid)`
`239`		`- if res != 0:`
`240`		`- raise Exception("Failed to start node resolution")`
	`239`	`+ res.raise_on_error()`
`241`	`240`
`242`	`241`
`243`	`242`	`class _SyncAddressFinder:`