[libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead of returning error codes
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Fri Aug 19 08:51:41 CEST 2022
On 18/08/2022 23:56, Laurent Pinchart wrote:
> On Thu, Aug 18, 2022 at 04:36:06PM +0200, Jacopo Mondi wrote:
>> On Fri, Jul 01, 2022 at 11:45:14AM +0300, Tomi Valkeinen wrote:
>>> We have multiple methods which return an error code, mimicing the C++
>
> s/mimicing/mimicking/
>
>>> API. Using exceptions is more natural in the Python API, so change all
>>> those methods to raise an Exception instead.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>>> ---
>>> src/py/cam/cam.py | 16 +----
>>> src/py/examples/simple-cam.py | 15 +---
>>> src/py/examples/simple-capture.py | 21 ++----
>>> src/py/examples/simple-continuous-capture.py | 21 ++----
>>> src/py/libcamera/py_main.cpp | 62 ++++++++++++----
>>> test/py/unittests.py | 76 +++++++-------------
>>> 6 files changed, 93 insertions(+), 118 deletions(-)
>>>
>>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
>>> index 2701d937..6b6b678b 100755
>>> --- a/src/py/cam/cam.py
>>> +++ b/src/py/cam/cam.py
>>> @@ -158,9 +158,7 @@ class CameraContext:
>>>
>>> print('Camera configuration adjusted')
>>>
>>> - r = self.camera.configure(camconfig)
>>> - if r != 0:
>>> - raise Exception('Configure failed')
>>> + self.camera.configure(camconfig)
>>>
>>> self.stream_names = {}
>>> self.streams = []
>>> @@ -175,12 +173,7 @@ class CameraContext:
>>> allocator = libcam.FrameBufferAllocator(self.camera)
>>>
>>> for stream in self.streams:
>>> - ret = allocator.allocate(stream)
>>> - if ret < 0:
>>> - print('Cannot allocate buffers')
>>> - exit(-1)
>>> -
>>> - allocated = len(allocator.buffers(stream))
>>> + allocated = allocator.allocate(stream)
>>>
>>> print('{}-{}: Allocated {} buffers'.format(self.id, self.stream_names[stream], allocated))
>>>
>>> @@ -205,10 +198,7 @@ class CameraContext:
>>> buffers = self.allocator.buffers(stream)
>>> buffer = buffers[buf_num]
>>>
>>> - ret = request.add_buffer(stream, buffer)
>>> - if ret < 0:
>>> - print('Can not set buffer for request')
>>> - exit(-1)
>>> + request.add_buffer(stream, buffer)
>>>
>>> requests.append(request)
>>>
>>> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py
>>> index b3e97ca7..1cd1019d 100755
>>> --- a/src/py/examples/simple-cam.py
>>> +++ b/src/py/examples/simple-cam.py
>>> @@ -259,12 +259,7 @@ def main():
>>> allocator = libcam.FrameBufferAllocator(camera)
>>>
>>> for cfg in config:
>>> - ret = allocator.allocate(cfg.stream)
>>> - if ret < 0:
>>> - print('Can\'t allocate buffers')
>>> - return -1
>>> -
>>> - allocated = len(allocator.buffers(cfg.stream))
>>> + allocated = allocator.allocate(cfg.stream)
>>> print(f'Allocated {allocated} buffers for stream')
>>>
>>> # --------------------------------------------------------------------
>>> @@ -289,15 +284,9 @@ def main():
>>> requests = []
>>> for i in range(len(buffers)):
>>> request = camera.create_request()
>>> - if not request:
>>> - print('Can\'t create request')
>>> - return -1
>>>
>>> buffer = buffers[i]
>>> - ret = request.add_buffer(stream, buffer)
>>> - if ret < 0:
>>> - print('Can\'t set buffer for request')
>>> - return -1
>>> + request.add_buffer(stream, buffer)
>>>
>>> # Controls can be added to a request on a per frame basis.
>>> request.set_control(libcam.controls.Brightness, 0.5)
>>> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py
>>> index 5f93574f..4b85408f 100755
>>> --- a/src/py/examples/simple-capture.py
>>> +++ b/src/py/examples/simple-capture.py
>>> @@ -43,8 +43,7 @@ def main():
>>>
>>> # Acquire the camera for our use
>>>
>>> - ret = cam.acquire()
>>> - assert ret == 0
>>> + cam.acquire()
>>>
>>> # Configure the camera
>>>
>>> @@ -60,8 +59,7 @@ def main():
>>> w, h = [int(v) for v in args.size.split('x')]
>>> stream_config.size = libcam.Size(w, h)
>>>
>>> - ret = cam.configure(cam_config)
>>> - assert ret == 0
>>> + cam.configure(cam_config)
>>>
>>> print(f'Capturing {TOTAL_FRAMES} frames with {stream_config}')
>>>
>>> @@ -83,15 +81,13 @@ def main():
>>> req = cam.create_request(i)
>>>
>>> buffer = allocator.buffers(stream)[i]
>>> - ret = req.add_buffer(stream, buffer)
>>> - assert ret == 0
>>> + req.add_buffer(stream, buffer)
>>>
>>> reqs.append(req)
>>>
>>> # Start the camera
>>>
>>> - ret = cam.start()
>>> - assert ret == 0
>>> + cam.start()
>>>
>>> # frames_queued and frames_done track the number of frames queued and done
>>>
>>> @@ -101,8 +97,7 @@ def main():
>>> # Queue the requests to the camera
>>>
>>> for req in reqs:
>>> - ret = cam.queue_request(req)
>>> - assert ret == 0
>>> + cam.queue_request(req)
>>> frames_queued += 1
>>>
>>> # The main loop. Wait for the queued Requests to complete, process them,
>>> @@ -155,13 +150,11 @@ def main():
>>>
>>> # Stop the camera
>>>
>>> - ret = cam.stop()
>>> - assert ret == 0
>>> + cam.stop()
>>>
>>> # Release the camera
>>>
>>> - ret = cam.release()
>>> - assert ret == 0
>>> + cam.release()
>>>
>>> return 0
>>>
>>> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py
>>> index 26a8060b..e1cb931e 100755
>>> --- a/src/py/examples/simple-continuous-capture.py
>>> +++ b/src/py/examples/simple-continuous-capture.py
>>> @@ -28,8 +28,7 @@ class CameraCaptureContext:
>>>
>>> # Acquire the camera for our use
>>>
>>> - ret = cam.acquire()
>>> - assert ret == 0
>>> + cam.acquire()
>>>
>>> # Configure the camera
>>>
>>> @@ -37,8 +36,7 @@ class CameraCaptureContext:
>>>
>>> stream_config = cam_config.at(0)
>>>
>>> - ret = cam.configure(cam_config)
>>> - assert ret == 0
>>> + cam.configure(cam_config)
>>>
>>> stream = stream_config.stream
>>>
>>> @@ -62,8 +60,7 @@ class CameraCaptureContext:
>>> req = cam.create_request(idx)
>>>
>>> buffer = allocator.buffers(stream)[i]
>>> - ret = req.add_buffer(stream, buffer)
>>> - assert ret == 0
>>> + req.add_buffer(stream, buffer)
>>>
>>> self.reqs.append(req)
>>>
>>> @@ -73,13 +70,11 @@ class CameraCaptureContext:
>>> def uninit_camera(self):
>>> # Stop the camera
>>>
>>> - ret = self.cam.stop()
>>> - assert ret == 0
>>> + self.cam.stop()
>>>
>>> # Release the camera
>>>
>>> - ret = self.cam.release()
>>> - assert ret == 0
>>> + self.cam.release()
>>>
>>>
>>> # A container class for our state
>>> @@ -145,8 +140,7 @@ class CaptureContext:
>>>
>>> for cam_ctx in self.camera_contexts:
>>> for req in cam_ctx.reqs:
>>> - ret = cam_ctx.cam.queue_request(req)
>>> - assert ret == 0
>>> + cam_ctx.cam.queue_request(req)
>>>
>>> # Use Selector to wait for events from the camera and from the keyboard
>>>
>>> @@ -177,8 +171,7 @@ def main():
>>> # Start the cameras
>>>
>>> for cam_ctx in ctx.camera_contexts:
>>> - ret = cam_ctx.cam.start()
>>> - assert ret == 0
>>> + cam_ctx.cam.start()
>>>
>>> ctx.capture()
>>>
>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>>> index e7d078b5..6cd99919 100644
>>> --- a/src/py/libcamera/py_main.cpp
>>> +++ b/src/py/libcamera/py_main.cpp
>>> @@ -111,8 +111,19 @@ PYBIND11_MODULE(_libcamera, m)
>>>
>>> pyCamera
>>> .def_property_readonly("id", &Camera::id)
>>> - .def("acquire", &Camera::acquire)
>>> - .def("release", &Camera::release)
>>> + .def("acquire", [](Camera &self) {
>>> + int ret = self.acquire();
>>> + if (ret)
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to acquire camera");
>>> + })
>>> + .def("release", [](Camera &self) {
>>> + int ret = self.release();
>>> + /* \todo Should we ignore the error? */
>
> Should we ? It indicates an error in the caller, right ?
I find errors in stop/release/free style functions a bit difficult to
understand. What does it mean? What is the caller supposed to do? Always
ignore? Or always abort()?
However, here, in the bindings, I think it makes sense to throw the
exception if the C++ API gives an error, whether the C++ API's behavior
makes sense or not. We're trying to expose the C++ API as directly as
possible.
So I'll drop these todo comments.
>>> + if (ret)
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to release camera");
>>> + })
>>> .def("start", [](Camera &self,
>>> const std::unordered_map<const ControlId *, py::object> &controls) {
>>> /* \todo What happens if someone calls start() multiple times? */
>>> @@ -132,20 +143,20 @@ PYBIND11_MODULE(_libcamera, m)
>>> int ret = self.start(&controlList);
>>> if (ret) {
>>> self.requestCompleted.disconnect();
>>> - return ret;
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to start camera");
>>> }
>>> -
>>> - return 0;
>>> }, py::arg("controls") = std::unordered_map<const ControlId *, py::object>())
>>>
>>> .def("stop", [](Camera &self) {
>>> int ret = self.stop();
>>> - if (ret)
>>> - return ret;
>>>
>>> self.requestCompleted.disconnect();
>>>
>>> - return 0;
>>> + /* \todo Should we just ignore the error? */
>>
>> There previously was an assertion, so I think it's fine to keep
>> checking for errors.
>>
>>> + if (ret)
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to start camera");
>>
>> s/start/stop
>>
>> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>>
>>> })
>>>
>>> .def("__str__", [](Camera &self) {
>>> @@ -154,9 +165,20 @@ PYBIND11_MODULE(_libcamera, m)
>>>
>>> /* Keep the camera alive, as StreamConfiguration contains a Stream* */
>>> .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
>>> - .def("configure", &Camera::configure)
>>> + .def("configure", [](Camera &self, CameraConfiguration *config) {
>>> + int ret = self.configure(config);
>>> + if (ret)
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to configure camera");
>>> + })
>>>
>>> - .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
>>> + .def("create_request", [](Camera &self, uint64_t cookie) {
>>> + std::unique_ptr<Request> req = self.createRequest(cookie);
>>> + if (!req)
>>> + throw std::system_error(ENOMEM, std::generic_category(),
>>> + "Failed to create request");
>>> + return req;
>>> + }, py::arg("cookie") = 0)
>>>
>>> .def("queue_request", [](Camera &self, Request *req) {
>>> py::object py_req = py::cast(req);
>>> @@ -169,10 +191,11 @@ PYBIND11_MODULE(_libcamera, m)
>>> py_req.inc_ref();
>>>
>>> int ret = self.queueRequest(req);
>>> - if (ret)
>>> + if (ret) {
>>> py_req.dec_ref();
>>> -
>>> - return ret;
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to queue request");
>>> + }
>>> })
>>>
>>> .def_property_readonly("streams", [](Camera &self) {
>>> @@ -250,7 +273,13 @@ PYBIND11_MODULE(_libcamera, m)
>>>
>>> pyFrameBufferAllocator
>>> .def(py::init<std::shared_ptr<Camera>>(), py::keep_alive<1, 2>())
>>> - .def("allocate", &FrameBufferAllocator::allocate)
>>> + .def("allocate", [](FrameBufferAllocator &self, Stream *stream) {
>>> + int ret = self.allocate(stream);
>>> + if (ret < 0)
>
> Shouldn't 0 be also considered an error ?
The function documentation says "The number of allocated buffers on
success or a negative error code otherwise". So 0 doesn't seem to be an
error.
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to queue request");
>
> Bad copy & paste.
Indeed.
>>> + return ret;
>>> + })
>>> .def_property_readonly("allocated", &FrameBufferAllocator::allocated)
>>> /* Create a list of FrameBuffers, where each FrameBuffer has a keep-alive to FrameBufferAllocator */
>>> .def("buffers", [](FrameBufferAllocator &self, Stream *stream) {
>>> @@ -327,7 +356,10 @@ PYBIND11_MODULE(_libcamera, m)
>>> pyRequest
>>> /* \todo Fence is not supported, so we cannot expose addBuffer() directly */
>>> .def("add_buffer", [](Request &self, const Stream *stream, FrameBuffer *buffer) {
>>> - return self.addBuffer(stream, buffer);
>>> + int ret = self.addBuffer(stream, buffer);
>
> Extra space after =
Yep.
>>> + if (ret)
>>> + throw std::system_error(-ret, std::generic_category(),
>>> + "Failed to add buffer");
>>> }, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */
>>> .def_property_readonly("status", &Request::status)
>>> .def_property_readonly("buffers", &Request::buffers)
>>> diff --git a/test/py/unittests.py b/test/py/unittests.py
>>> index 6dea80fc..794e46be 100755
>>> --- a/test/py/unittests.py
>>> +++ b/test/py/unittests.py
>>> @@ -42,31 +42,26 @@ class SimpleTestMethods(BaseTestCase):
>>> cam = cm.get('platform/vimc.0 Sensor B')
>>> self.assertIsNotNone(cam)
>>>
>>> - ret = cam.acquire()
>>> - self.assertZero(ret)
>>> + cam.acquire()
>
> Will the test framework produce an error report that provide as much
> debugging information as the assert functions ?
Yes. Although those two cases are slightly different: a failing
test-assert results in test failure, and an exception results in test
error. The end result is the same.
Tomi
More information about the libcamera-devel
mailing list