[libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead of returning error codes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Aug 18 22:56:37 CEST 2022
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 ?
> > + 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 ?
> > + throw std::system_error(-ret, std::generic_category(),
> > + "Failed to queue request");
Bad copy & paste.
> > + 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 =
> > + 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 ?
> >
> > - ret = cam.release()
> > - self.assertZero(ret)
> > + cam.release()
> >
> > def test_double_acquire(self):
> > cm = libcam.CameraManager.singleton()
> > cam = cm.get('platform/vimc.0 Sensor B')
> > self.assertIsNotNone(cam)
> >
> > - ret = cam.acquire()
> > - self.assertZero(ret)
> > + cam.acquire()
> >
> > libcam.log_set_level('Camera', 'FATAL')
> > - ret = cam.acquire()
> > - self.assertEqual(ret, -errno.EBUSY)
> > + with self.assertRaises(RuntimeError):
> > + cam.acquire()
> > libcam.log_set_level('Camera', 'ERROR')
> >
> > - ret = cam.release()
> > - self.assertZero(ret)
> > + cam.release()
> >
> > - ret = cam.release()
> > - # I expected EBUSY, but looks like double release works fine
> > - self.assertZero(ret)
> > + # I expected exception here, but looks like double release works fine
> > + cam.release()
> >
> >
> > class CameraTesterBase(BaseTestCase):
> > @@ -80,11 +75,7 @@ class CameraTesterBase(BaseTestCase):
> > self.cm = None
> > self.skipTest('No vimc found')
> >
> > - ret = self.cam.acquire()
> > - if ret != 0:
> > - self.cam = None
> > - self.cm = None
> > - raise Exception('Failed to acquire camera')
> > + self.cam.acquire()
> >
> > self.wr_cam = weakref.ref(self.cam)
> > self.wr_cm = weakref.ref(self.cm)
> > @@ -93,9 +84,7 @@ class CameraTesterBase(BaseTestCase):
> > # If a test fails, the camera may be in running state. So always stop.
> > self.cam.stop()
> >
> > - ret = self.cam.release()
> > - if ret != 0:
> > - raise Exception('Failed to release camera')
> > + self.cam.release()
> >
> > self.cam = None
> > self.cm = None
> > @@ -115,8 +104,7 @@ class AllocatorTestMethods(CameraTesterBase):
> > streamconfig = camconfig.at(0)
> > wr_streamconfig = weakref.ref(streamconfig)
> >
> > - ret = cam.configure(camconfig)
> > - self.assertZero(ret)
> > + cam.configure(camconfig)
> >
> > stream = streamconfig.stream
> > wr_stream = weakref.ref(stream)
> > @@ -129,8 +117,8 @@ class AllocatorTestMethods(CameraTesterBase):
> > self.assertIsNotNone(wr_streamconfig())
> >
> > allocator = libcam.FrameBufferAllocator(cam)
> > - ret = allocator.allocate(stream)
> > - self.assertTrue(ret > 0)
> > + num_bufs = allocator.allocate(stream)
> > + self.assertTrue(num_bufs > 0)
> > wr_allocator = weakref.ref(allocator)
> >
> > buffers = allocator.buffers(stream)
> > @@ -173,14 +161,13 @@ class SimpleCaptureMethods(CameraTesterBase):
> > self.assertIsNotNone(fmts)
> > fmts = None
> >
> > - ret = cam.configure(camconfig)
> > - self.assertZero(ret)
> > + cam.configure(camconfig)
> >
> > stream = streamconfig.stream
> >
> > allocator = libcam.FrameBufferAllocator(cam)
> > - ret = allocator.allocate(stream)
> > - self.assertTrue(ret > 0)
> > + num_bufs = allocator.allocate(stream)
> > + self.assertTrue(num_bufs > 0)
> >
> > num_bufs = len(allocator.buffers(stream))
> >
> > @@ -190,19 +177,16 @@ class SimpleCaptureMethods(CameraTesterBase):
> > self.assertIsNotNone(req)
> >
> > buffer = allocator.buffers(stream)[i]
> > - ret = req.add_buffer(stream, buffer)
> > - self.assertZero(ret)
> > + req.add_buffer(stream, buffer)
> >
> > reqs.append(req)
> >
> > buffer = None
> >
> > - ret = cam.start()
> > - self.assertZero(ret)
> > + cam.start()
> >
> > for req in reqs:
> > - ret = cam.queue_request(req)
> > - self.assertZero(ret)
> > + cam.queue_request(req)
> >
> > reqs = None
> > gc.collect()
> > @@ -230,8 +214,7 @@ class SimpleCaptureMethods(CameraTesterBase):
> > reqs = None
> > gc.collect()
> >
> > - ret = cam.stop()
> > - self.assertZero(ret)
> > + cam.stop()
> >
> > def test_select(self):
> > cm = self.cm
> > @@ -245,14 +228,13 @@ class SimpleCaptureMethods(CameraTesterBase):
> > self.assertIsNotNone(fmts)
> > fmts = None
> >
> > - ret = cam.configure(camconfig)
> > - self.assertZero(ret)
> > + cam.configure(camconfig)
> >
> > stream = streamconfig.stream
> >
> > allocator = libcam.FrameBufferAllocator(cam)
> > - ret = allocator.allocate(stream)
> > - self.assertTrue(ret > 0)
> > + num_bufs = allocator.allocate(stream)
> > + self.assertTrue(num_bufs > 0)
> >
> > num_bufs = len(allocator.buffers(stream))
> >
> > @@ -262,19 +244,16 @@ class SimpleCaptureMethods(CameraTesterBase):
> > self.assertIsNotNone(req)
> >
> > buffer = allocator.buffers(stream)[i]
> > - ret = req.add_buffer(stream, buffer)
> > - self.assertZero(ret)
> > + req.add_buffer(stream, buffer)
> >
> > reqs.append(req)
> >
> > buffer = None
> >
> > - ret = cam.start()
> > - self.assertZero(ret)
> > + cam.start()
> >
> > for req in reqs:
> > - ret = cam.queue_request(req)
> > - self.assertZero(ret)
> > + cam.queue_request(req)
> >
> > reqs = None
> > gc.collect()
> > @@ -303,8 +282,7 @@ class SimpleCaptureMethods(CameraTesterBase):
> > reqs = None
> > gc.collect()
> >
> > - ret = cam.stop()
> > - self.assertZero(ret)
> > + cam.stop()
> >
> >
> > # Recursively expand slist's objects into olist, using seen to track already
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list