[libcamera-devel,v2,09/14] py: Use exceptions instead of returning error codes
diff mbox series

Message ID 20220629070416.17550-10-tomi.valkeinen@ideasonboard.com
State Superseded
Headers show
Series
  • Python bindings event handling
Related show

Commit Message

Tomi Valkeinen June 29, 2022, 7:04 a.m. UTC
We have multiple methods which return an error code, mimicing the C++
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@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(-)

Patch
diff mbox series

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 2b81bb65..e88a4737 100755
--- a/src/py/examples/simple-cam.py
+++ b/src/py/examples/simple-cam.py
@@ -258,12 +258,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')
 
     # --------------------------------------------------------------------
@@ -288,15 +283,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 a6a9b33e..07d12dae 100755
--- a/src/py/examples/simple-capture.py
+++ b/src/py/examples/simple-capture.py
@@ -42,8 +42,7 @@  def main():
 
     # Acquire the camera for our use
 
-    ret = cam.acquire()
-    assert ret == 0
+    cam.acquire()
 
     # Configure the camera
 
@@ -59,8 +58,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}')
 
@@ -82,15 +80,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
 
@@ -100,8 +96,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,
@@ -147,13 +142,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 fe78a2dd..ef3f87d1 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
@@ -144,8 +139,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
 
@@ -176,8 +170,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..1ef1384e 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? */
+			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? */
+			if (ret)
+				throw std::system_error(-ret, std::generic_category(),
+				                        "Failed to start camera");
 		})
 
 		.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)
+				throw std::system_error(-ret, std::generic_category(),
+				                        "Failed to queue request");
+			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);
+			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 9adc4337..b90b5fec 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()
 
-        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()
@@ -223,8 +207,7 @@  class SimpleCaptureMethods(CameraTesterBase):
         reqs = None
         gc.collect()
 
-        ret = cam.stop()
-        self.assertZero(ret)
+        cam.stop()
 
     def test_select(self):
         cm = self.cm
@@ -238,14 +221,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))
 
@@ -255,19 +237,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()
@@ -296,8 +275,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