-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[python] Implement PyChipError for passing detailed error infomation to Python API #22224
Conversation
pychip_GetLastError
for better error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see lots of ChipError::StorageType
sprinkled in the C++ code. Can we please remove all of those as well?
Or is the goal here to slowly transition those away? |
There are 112 remaining instances of those, so perhaps staggering them would be better... |
Wait, I did some search in the code, but did not found those --- $ grep src -nre 'ChipError::StorageType' | wc -l
14 The remaining instances in src/controller/python are not used as error codes.
I would not suggest to do so, one issue is some PyChipError are used as parameters for async callbacks. In fact, I would suggest create a wrapper (and for python, this wrapper can be generated during runtime) for all of these APIs, and avoid calling the foreign functions in the library directly, we can have a wrapper for automatically throw the error in the generated wrappers. This would give us a safer are more readable API as an extra benefit. |
PR #22224: Size comparison from a334714 to 12331a2 Increases (2 builds for cc13x2_26x2, psoc6)
Decreases (9 builds for bl602, bl702, cc13x2_26x2, esp32, nrfconnect, psoc6, telink)
Full report (37 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #22224: Size comparison from a334714 to d5e75a0 Increases (8 builds for bl702, cc13x2_26x2, esp32, psoc6, telink)
Decreases (8 builds for bl602, bl702, cc13x2_26x2, esp32, nrfconnect, qpg, telink)
Full report (37 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #22224: Size comparison from bc6b438 to 69bb325 Increases (3 builds for cc13x2_26x2, esp32, psoc6)
Decreases (8 builds for bl602, bl702, cc13x2_26x2, psoc6, telink)
Full report (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work. It's still a missing a handful of raise_on_error
on a number of call-sites that call chipStack.Call()
E.g:
builtins.chipStack.Call(
lambda: self._dmLib.pychip_GetLocalSessionId(self._deviceProxy, pointer(localSessionId))
)
ae312d6
to
ce4e04f
Compare
Did some search |
PR #22224: Size comparison from 82dc082 to ce4e04f Increases (4 builds for cc13x2_26x2, cyw30739, esp32)
Decreases (4 builds for cc13x2_26x2, cyw30739, qpg, telink)
Full report (38 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
…to Python API (project-chip#22224) * [python] Use PyChipError for more detailed error message * Add pychip_FormatError * Add some getters for PyChipError * Avoid return PyChipError to end user * Fix * Add raise_on_error to existing calls and cleanup
…to Python API (project-chip#22224) * [python] Use PyChipError for more detailed error message * Add pychip_FormatError * Add some getters for PyChipError * Avoid return PyChipError to end user * Fix * Add raise_on_error to existing calls and cleanup
Problem
ChipError
#17952The detailed error infomation is lost when passing an error to Python.
Change overview
PyChipError
Testing
ConnectIP
(still covered by testing script) as the first step and tested manually.