[{"id":24653,"web_url":"https://patchwork.libcamera.org/comment/24653/","msgid":"<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>","date":"2022-08-18T14:36:06","subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Tomi\n\nOn Fri, Jul 01, 2022 at 11:45:14AM +0300, Tomi Valkeinen wrote:\n> We have multiple methods which return an error code, mimicing the C++\n> API. Using exceptions is more natural in the Python API, so change all\n> those methods to raise an Exception instead.\n>\n> Signed-off-by: Tomi Valkeinen \n> ---\n> src/py/cam/cam.py | 16 +----\n> src/py/examples/simple-cam.py | 15 +---\n> src/py/examples/simple-capture.py | 21 ++----\n> src/py/examples/simple-continuous-capture.py | 21 ++----\n> src/py/libcamera/py_main.cpp | 62 ++++++++++++----\n> test/py/unittests.py | 76 +++++++-------------\n> 6 files changed, 93 insertions(+), 118 deletions(-)\n>\n> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> index 2701d937..6b6b678b 100755\n> --- a/src/py/cam/cam.py\n> +++ b/src/py/cam/cam.py\n> @@ -158,9 +158,7 @@ class CameraContext:\n>\n> print('Camera configuration adjusted')\n>\n> - r = self.camera.configure(camconfig)\n> - if r != 0:\n> - raise Exception('Configure failed')\n> + self.camera.configure(camconfig)\n>\n> self.stream_names = {}\n> self.streams = []\n> @@ -175,12 +173,7 @@ class CameraContext:\n> allocator = libcam.FrameBufferAllocator(self.camera)\n>\n> for stream in self.streams:\n> - ret = allocator.allocate(stream)\n> - if ret < 0:\n> - print('Cannot allocate buffers')\n> - exit(-1)\n> -\n> - allocated = len(allocator.buffers(stream))\n> + allocated = allocator.allocate(stream)\n>\n> print('{}-{}: Allocated {} buffers'.format(self.id, self.stream_names[stream], allocated))\n>\n> @@ -205,10 +198,7 @@ class CameraContext:\n> buffers = self.allocator.buffers(stream)\n> buffer = buffers[buf_num]\n>\n> - ret = request.add_buffer(stream, buffer)\n> - if ret < 0:\n> - print('Can not set buffer for request')\n> - exit(-1)\n> + request.add_buffer(stream, buffer)\n>\n> requests.append(request)\n>\n> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n> index b3e97ca7..1cd1019d 100755\n> --- a/src/py/examples/simple-cam.py\n> +++ b/src/py/examples/simple-cam.py\n> @@ -259,12 +259,7 @@ def main():\n> allocator = libcam.FrameBufferAllocator(camera)\n>\n> for cfg in config:\n> - ret = allocator.allocate(cfg.stream)\n> - if ret < 0:\n> - print('Can\\'t allocate buffers')\n> - return -1\n> -\n> - allocated = len(allocator.buffers(cfg.stream))\n> + allocated = allocator.allocate(cfg.stream)\n> print(f'Allocated {allocated} buffers for stream')\n>\n> # --------------------------------------------------------------------\n> @@ -289,15 +284,9 @@ def main():\n> requests = []\n> for i in range(len(buffers)):\n> request = camera.create_request()\n> - if not request:\n> - print('Can\\'t create request')\n> - return -1\n>\n> buffer = buffers[i]\n> - ret = request.add_buffer(stream, buffer)\n> - if ret < 0:\n> - print('Can\\'t set buffer for request')\n> - return -1\n> + request.add_buffer(stream, buffer)\n>\n> # Controls can be added to a request on a per frame basis.\n> request.set_control(libcam.controls.Brightness, 0.5)\n> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n> index 5f93574f..4b85408f 100755\n> --- a/src/py/examples/simple-capture.py\n> +++ b/src/py/examples/simple-capture.py\n> @@ -43,8 +43,7 @@ def main():\n>\n> # Acquire the camera for our use\n>\n> - ret = cam.acquire()\n> - assert ret == 0\n> + cam.acquire()\n>\n> # Configure the camera\n>\n> @@ -60,8 +59,7 @@ def main():\n> w, h = [int(v) for v in args.size.split('x')]\n> stream_config.size = libcam.Size(w, h)\n>\n> - ret = cam.configure(cam_config)\n> - assert ret == 0\n> + cam.configure(cam_config)\n>\n> print(f'Capturing {TOTAL_FRAMES} frames with {stream_config}')\n>\n> @@ -83,15 +81,13 @@ def main():\n> req = cam.create_request(i)\n>\n> buffer = allocator.buffers(stream)[i]\n> - ret = req.add_buffer(stream, buffer)\n> - assert ret == 0\n> + req.add_buffer(stream, buffer)\n>\n> reqs.append(req)\n>\n> # Start the camera\n>\n> - ret = cam.start()\n> - assert ret == 0\n> + cam.start()\n>\n> # frames_queued and frames_done track the number of frames queued and done\n>\n> @@ -101,8 +97,7 @@ def main():\n> # Queue the requests to the camera\n>\n> for req in reqs:\n> - ret = cam.queue_request(req)\n> - assert ret == 0\n> + cam.queue_request(req)\n> frames_queued += 1\n>\n> # The main loop. Wait for the queued Requests to complete, process them,\n> @@ -155,13 +150,11 @@ def main():\n>\n> # Stop the camera\n>\n> - ret = cam.stop()\n> - assert ret == 0\n> + cam.stop()\n>\n> # Release the camera\n>\n> - ret = cam.release()\n> - assert ret == 0\n> + cam.release()\n>\n> return 0\n>\n> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n> index 26a8060b..e1cb931e 100755\n> --- a/src/py/examples/simple-continuous-capture.py\n> +++ b/src/py/examples/simple-continuous-capture.py\n> @@ -28,8 +28,7 @@ class CameraCaptureContext:\n>\n> # Acquire the camera for our use\n>\n> - ret = cam.acquire()\n> - assert ret == 0\n> + cam.acquire()\n>\n> # Configure the camera\n>\n> @@ -37,8 +36,7 @@ class CameraCaptureContext:\n>\n> stream_config = cam_config.at(0)\n>\n> - ret = cam.configure(cam_config)\n> - assert ret == 0\n> + cam.configure(cam_config)\n>\n> stream = stream_config.stream\n>\n> @@ -62,8 +60,7 @@ class CameraCaptureContext:\n> req = cam.create_request(idx)\n>\n> buffer = allocator.buffers(stream)[i]\n> - ret = req.add_buffer(stream, buffer)\n> - assert ret == 0\n> + req.add_buffer(stream, buffer)\n>\n> self.reqs.append(req)\n>\n> @@ -73,13 +70,11 @@ class CameraCaptureContext:\n> def uninit_camera(self):\n> # Stop the camera\n>\n> - ret = self.cam.stop()\n> - assert ret == 0\n> + self.cam.stop()\n>\n> # Release the camera\n>\n> - ret = self.cam.release()\n> - assert ret == 0\n> + self.cam.release()\n>\n>\n> # A container class for our state\n> @@ -145,8 +140,7 @@ class CaptureContext:\n>\n> for cam_ctx in self.camera_contexts:\n> for req in cam_ctx.reqs:\n> - ret = cam_ctx.cam.queue_request(req)\n> - assert ret == 0\n> + cam_ctx.cam.queue_request(req)\n>\n> # Use Selector to wait for events from the camera and from the keyboard\n>\n> @@ -177,8 +171,7 @@ def main():\n> # Start the cameras\n>\n> for cam_ctx in ctx.camera_contexts:\n> - ret = cam_ctx.cam.start()\n> - assert ret == 0\n> + cam_ctx.cam.start()\n>\n> ctx.capture()\n>\n> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> index e7d078b5..6cd99919 100644\n> --- a/src/py/libcamera/py_main.cpp\n> +++ b/src/py/libcamera/py_main.cpp\n> @@ -111,8 +111,19 @@ PYBIND11_MODULE(_libcamera, m)\n>\n> \tpyCamera\n> \t\t.def_property_readonly(\"id\", &Camera::id)\n> -\t\t.def(\"acquire\", &Camera::acquire)\n> -\t\t.def(\"release\", &Camera::release)\n> +\t\t.def(\"acquire\", [](Camera &self) {\n> +\t\t\tint ret = self.acquire();\n> +\t\t\tif (ret)\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to acquire camera\");\n> +\t\t})\n> +\t\t.def(\"release\", [](Camera &self) {\n> +\t\t\tint ret = self.release();\n> +\t\t\t/* \\todo Should we ignore the error? */\n> +\t\t\tif (ret)\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to release camera\");\n> +\t\t})\n> \t\t.def(\"start\", [](Camera &self,\n> \t\t const std::unordered_map &controls) {\n> \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n> @@ -132,20 +143,20 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\tint ret = self.start(&controlList);\n> \t\t\tif (ret) {\n> \t\t\t\tself.requestCompleted.disconnect();\n> -\t\t\t\treturn ret;\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to start camera\");\n> \t\t\t}\n> -\n> -\t\t\treturn 0;\n> \t\t}, py::arg(\"controls\") = std::unordered_map())\n>\n> \t\t.def(\"stop\", [](Camera &self) {\n> \t\t\tint ret = self.stop();\n> -\t\t\tif (ret)\n> -\t\t\t\treturn ret;\n>\n> \t\t\tself.requestCompleted.disconnect();\n>\n> -\t\t\treturn 0;\n> +\t\t\t/* \\todo Should we just ignore the error? */\n\nThere previously was an assertion, so I think it's fine to keep\nchecking for errors.\n\n> +\t\t\tif (ret)\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to start camera\");\n\ns/start/stop\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> \t\t})\n>\n> \t\t.def(\"__str__\", [](Camera &self) {\n> @@ -154,9 +165,20 @@ PYBIND11_MODULE(_libcamera, m)\n>\n> \t\t/* Keep the camera alive, as StreamConfiguration contains a Stream* */\n> \t\t.def(\"generate_configuration\", &Camera::generateConfiguration, py::keep_alive<0, 1>())\n> -\t\t.def(\"configure\", &Camera::configure)\n> +\t\t.def(\"configure\", [](Camera &self, CameraConfiguration *config) {\n> +\t\t\tint ret = self.configure(config);\n> +\t\t\tif (ret)\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to configure camera\");\n> +\t\t})\n>\n> -\t\t.def(\"create_request\", &Camera::createRequest, py::arg(\"cookie\") = 0)\n> +\t\t.def(\"create_request\", [](Camera &self, uint64_t cookie) {\n> +\t\t\tstd::unique_ptr req = self.createRequest(cookie);\n> +\t\t\tif (!req)\n> +\t\t\t\tthrow std::system_error(ENOMEM, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to create request\");\n> +\t\t\treturn req;\n> +\t\t}, py::arg(\"cookie\") = 0)\n>\n> \t\t.def(\"queue_request\", [](Camera &self, Request *req) {\n> \t\t\tpy::object py_req = py::cast(req);\n> @@ -169,10 +191,11 @@ PYBIND11_MODULE(_libcamera, m)\n> \t\t\tpy_req.inc_ref();\n>\n> \t\t\tint ret = self.queueRequest(req);\n> -\t\t\tif (ret)\n> +\t\t\tif (ret) {\n> \t\t\t\tpy_req.dec_ref();\n> -\n> -\t\t\treturn ret;\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to queue request\");\n> +\t\t\t}\n> \t\t})\n>\n> \t\t.def_property_readonly(\"streams\", [](Camera &self) {\n> @@ -250,7 +273,13 @@ PYBIND11_MODULE(_libcamera, m)\n>\n> \tpyFrameBufferAllocator\n> \t\t.def(py::init>(), py::keep_alive<1, 2>())\n> -\t\t.def(\"allocate\", &FrameBufferAllocator::allocate)\n> +\t\t.def(\"allocate\", [](FrameBufferAllocator &self, Stream *stream) {\n> +\t\t\tint ret = self.allocate(stream);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to queue request\");\n> +\t\t\treturn ret;\n> +\t\t})\n> \t\t.def_property_readonly(\"allocated\", &FrameBufferAllocator::allocated)\n> \t\t/* Create a list of FrameBuffers, where each FrameBuffer has a keep-alive to FrameBufferAllocator */\n> \t\t.def(\"buffers\", [](FrameBufferAllocator &self, Stream *stream) {\n> @@ -327,7 +356,10 @@ PYBIND11_MODULE(_libcamera, m)\n> \tpyRequest\n> \t\t/* \\todo Fence is not supported, so we cannot expose addBuffer() directly */\n> \t\t.def(\"add_buffer\", [](Request &self, const Stream *stream, FrameBuffer *buffer) {\n> -\t\t\treturn self.addBuffer(stream, buffer);\n> +\t\t\tint ret = self.addBuffer(stream, buffer);\n> +\t\t\tif (ret)\n> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> +\t\t\t\t\t\t\t\"Failed to add buffer\");\n> \t\t}, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */\n> \t\t.def_property_readonly(\"status\", &Request::status)\n> \t\t.def_property_readonly(\"buffers\", &Request::buffers)\n> diff --git a/test/py/unittests.py b/test/py/unittests.py\n> index 6dea80fc..794e46be 100755\n> --- a/test/py/unittests.py\n> +++ b/test/py/unittests.py\n> @@ -42,31 +42,26 @@ class SimpleTestMethods(BaseTestCase):\n> cam = cm.get('platform/vimc.0 Sensor B')\n> self.assertIsNotNone(cam)\n>\n> - ret = cam.acquire()\n> - self.assertZero(ret)\n> + cam.acquire()\n>\n> - ret = cam.release()\n> - self.assertZero(ret)\n> + cam.release()\n>\n> def test_double_acquire(self):\n> cm = libcam.CameraManager.singleton()\n> cam = cm.get('platform/vimc.0 Sensor B')\n> self.assertIsNotNone(cam)\n>\n> - ret = cam.acquire()\n> - self.assertZero(ret)\n> + cam.acquire()\n>\n> libcam.log_set_level('Camera', 'FATAL')\n> - ret = cam.acquire()\n> - self.assertEqual(ret, -errno.EBUSY)\n> + with self.assertRaises(RuntimeError):\n> + cam.acquire()\n> libcam.log_set_level('Camera', 'ERROR')\n>\n> - ret = cam.release()\n> - self.assertZero(ret)\n> + cam.release()\n>\n> - ret = cam.release()\n> - # I expected EBUSY, but looks like double release works fine\n> - self.assertZero(ret)\n> + # I expected exception here, but looks like double release works fine\n> + cam.release()\n>\n>\n> class CameraTesterBase(BaseTestCase):\n> @@ -80,11 +75,7 @@ class CameraTesterBase(BaseTestCase):\n> self.cm = None\n> self.skipTest('No vimc found')\n>\n> - ret = self.cam.acquire()\n> - if ret != 0:\n> - self.cam = None\n> - self.cm = None\n> - raise Exception('Failed to acquire camera')\n> + self.cam.acquire()\n>\n> self.wr_cam = weakref.ref(self.cam)\n> self.wr_cm = weakref.ref(self.cm)\n> @@ -93,9 +84,7 @@ class CameraTesterBase(BaseTestCase):\n> # If a test fails, the camera may be in running state. So always stop.\n> self.cam.stop()\n>\n> - ret = self.cam.release()\n> - if ret != 0:\n> - raise Exception('Failed to release camera')\n> + self.cam.release()\n>\n> self.cam = None\n> self.cm = None\n> @@ -115,8 +104,7 @@ class AllocatorTestMethods(CameraTesterBase):\n> streamconfig = camconfig.at(0)\n> wr_streamconfig = weakref.ref(streamconfig)\n>\n> - ret = cam.configure(camconfig)\n> - self.assertZero(ret)\n> + cam.configure(camconfig)\n>\n> stream = streamconfig.stream\n> wr_stream = weakref.ref(stream)\n> @@ -129,8 +117,8 @@ class AllocatorTestMethods(CameraTesterBase):\n> self.assertIsNotNone(wr_streamconfig())\n>\n> allocator = libcam.FrameBufferAllocator(cam)\n> - ret = allocator.allocate(stream)\n> - self.assertTrue(ret > 0)\n> + num_bufs = allocator.allocate(stream)\n> + self.assertTrue(num_bufs > 0)\n> wr_allocator = weakref.ref(allocator)\n>\n> buffers = allocator.buffers(stream)\n> @@ -173,14 +161,13 @@ class SimpleCaptureMethods(CameraTesterBase):\n> self.assertIsNotNone(fmts)\n> fmts = None\n>\n> - ret = cam.configure(camconfig)\n> - self.assertZero(ret)\n> + cam.configure(camconfig)\n>\n> stream = streamconfig.stream\n>\n> allocator = libcam.FrameBufferAllocator(cam)\n> - ret = allocator.allocate(stream)\n> - self.assertTrue(ret > 0)\n> + num_bufs = allocator.allocate(stream)\n> + self.assertTrue(num_bufs > 0)\n>\n> num_bufs = len(allocator.buffers(stream))\n>\n> @@ -190,19 +177,16 @@ class SimpleCaptureMethods(CameraTesterBase):\n> self.assertIsNotNone(req)\n>\n> buffer = allocator.buffers(stream)[i]\n> - ret = req.add_buffer(stream, buffer)\n> - self.assertZero(ret)\n> + req.add_buffer(stream, buffer)\n>\n> reqs.append(req)\n>\n> buffer = None\n>\n> - ret = cam.start()\n> - self.assertZero(ret)\n> + cam.start()\n>\n> for req in reqs:\n> - ret = cam.queue_request(req)\n> - self.assertZero(ret)\n> + cam.queue_request(req)\n>\n> reqs = None\n> gc.collect()\n> @@ -230,8 +214,7 @@ class SimpleCaptureMethods(CameraTesterBase):\n> reqs = None\n> gc.collect()\n>\n> - ret = cam.stop()\n> - self.assertZero(ret)\n> + cam.stop()\n>\n> def test_select(self):\n> cm = self.cm\n> @@ -245,14 +228,13 @@ class SimpleCaptureMethods(CameraTesterBase):\n> self.assertIsNotNone(fmts)\n> fmts = None\n>\n> - ret = cam.configure(camconfig)\n> - self.assertZero(ret)\n> + cam.configure(camconfig)\n>\n> stream = streamconfig.stream\n>\n> allocator = libcam.FrameBufferAllocator(cam)\n> - ret = allocator.allocate(stream)\n> - self.assertTrue(ret > 0)\n> + num_bufs = allocator.allocate(stream)\n> + self.assertTrue(num_bufs > 0)\n>\n> num_bufs = len(allocator.buffers(stream))\n>\n> @@ -262,19 +244,16 @@ class SimpleCaptureMethods(CameraTesterBase):\n> self.assertIsNotNone(req)\n>\n> buffer = allocator.buffers(stream)[i]\n> - ret = req.add_buffer(stream, buffer)\n> - self.assertZero(ret)\n> + req.add_buffer(stream, buffer)\n>\n> reqs.append(req)\n>\n> buffer = None\n>\n> - ret = cam.start()\n> - self.assertZero(ret)\n> + cam.start()\n>\n> for req in reqs:\n> - ret = cam.queue_request(req)\n> - self.assertZero(ret)\n> + cam.queue_request(req)\n>\n> reqs = None\n> gc.collect()\n> @@ -303,8 +282,7 @@ class SimpleCaptureMethods(CameraTesterBase):\n> reqs = None\n> gc.collect()\n>\n> - ret = cam.stop()\n> - self.assertZero(ret)\n> + cam.stop()\n>\n>\n> # Recursively expand slist's objects into olist, using seen to track already\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 05CE2C3272\n\tfor ;\n\tThu, 18 Aug 2022 14:36:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB51161FC0;\n\tThu, 18 Aug 2022 16:36:12 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5463861FA7\n\tfor ;\n\tThu, 18 Aug 2022 16:36:11 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 6B976200008;\n\tThu, 18 Aug 2022 14:36:09 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660833373;\n\tbh=23OWuuWccvhGPzDbTGBXIUwlt8SHmGBskqqBR8Mk2oM=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=c6F1TU1vDJ+vutsJYbe25weG9KcTXUlz1duWu/VPMbSp6F2q8QBG58mxeKJ5xPEEd\n\tc7itMuPvlHVEtl6zfdZf7m+v+sq62bfwhcrF1masTtn/jScXVr6AZYeHwBe6TevJDO\n\tH5+WJA80GqFgBOpRN2nodAhxnPzXnppRg0HXzoXwVUnDd2CGP8/WjEVPq2QMkiOFvl\n\t6mJu9bo5yi7DiGimDM1WUZvgeG0bbjSAJxFASrdlJk3yycqLB2kPyPXGUan5oTGCUd\n\ttABN7As2mgorj79hGuGoXEWUnN90+Rd3t3a5k+nEgxrKwWau9UhunRRCZc+arFuDWJ\n\tbAwR+RdOINnHA==","Date":"Thu, 18 Aug 2022 16:36:06 +0200","To":"Tomi Valkeinen ","Message-ID":"<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-11-tomi.valkeinen@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220701084521.31831-11-tomi.valkeinen@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24686,"web_url":"https://patchwork.libcamera.org/comment/24686/","msgid":"","date":"2022-08-18T20:56:37","subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Aug 18, 2022 at 04:36:06PM +0200, Jacopo Mondi wrote:\n> On Fri, Jul 01, 2022 at 11:45:14AM +0300, Tomi Valkeinen wrote:\n> > We have multiple methods which return an error code, mimicing the C++\n\ns/mimicing/mimicking/\n\n> > API. Using exceptions is more natural in the Python API, so change all\n> > those methods to raise an Exception instead.\n> >\n> > Signed-off-by: Tomi Valkeinen \n> > ---\n> > src/py/cam/cam.py | 16 +----\n> > src/py/examples/simple-cam.py | 15 +---\n> > src/py/examples/simple-capture.py | 21 ++----\n> > src/py/examples/simple-continuous-capture.py | 21 ++----\n> > src/py/libcamera/py_main.cpp | 62 ++++++++++++----\n> > test/py/unittests.py | 76 +++++++-------------\n> > 6 files changed, 93 insertions(+), 118 deletions(-)\n> >\n> > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n> > index 2701d937..6b6b678b 100755\n> > --- a/src/py/cam/cam.py\n> > +++ b/src/py/cam/cam.py\n> > @@ -158,9 +158,7 @@ class CameraContext:\n> >\n> > print('Camera configuration adjusted')\n> >\n> > - r = self.camera.configure(camconfig)\n> > - if r != 0:\n> > - raise Exception('Configure failed')\n> > + self.camera.configure(camconfig)\n> >\n> > self.stream_names = {}\n> > self.streams = []\n> > @@ -175,12 +173,7 @@ class CameraContext:\n> > allocator = libcam.FrameBufferAllocator(self.camera)\n> >\n> > for stream in self.streams:\n> > - ret = allocator.allocate(stream)\n> > - if ret < 0:\n> > - print('Cannot allocate buffers')\n> > - exit(-1)\n> > -\n> > - allocated = len(allocator.buffers(stream))\n> > + allocated = allocator.allocate(stream)\n> >\n> > print('{}-{}: Allocated {} buffers'.format(self.id, self.stream_names[stream], allocated))\n> >\n> > @@ -205,10 +198,7 @@ class CameraContext:\n> > buffers = self.allocator.buffers(stream)\n> > buffer = buffers[buf_num]\n> >\n> > - ret = request.add_buffer(stream, buffer)\n> > - if ret < 0:\n> > - print('Can not set buffer for request')\n> > - exit(-1)\n> > + request.add_buffer(stream, buffer)\n> >\n> > requests.append(request)\n> >\n> > diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n> > index b3e97ca7..1cd1019d 100755\n> > --- a/src/py/examples/simple-cam.py\n> > +++ b/src/py/examples/simple-cam.py\n> > @@ -259,12 +259,7 @@ def main():\n> > allocator = libcam.FrameBufferAllocator(camera)\n> >\n> > for cfg in config:\n> > - ret = allocator.allocate(cfg.stream)\n> > - if ret < 0:\n> > - print('Can\\'t allocate buffers')\n> > - return -1\n> > -\n> > - allocated = len(allocator.buffers(cfg.stream))\n> > + allocated = allocator.allocate(cfg.stream)\n> > print(f'Allocated {allocated} buffers for stream')\n> >\n> > # --------------------------------------------------------------------\n> > @@ -289,15 +284,9 @@ def main():\n> > requests = []\n> > for i in range(len(buffers)):\n> > request = camera.create_request()\n> > - if not request:\n> > - print('Can\\'t create request')\n> > - return -1\n> >\n> > buffer = buffers[i]\n> > - ret = request.add_buffer(stream, buffer)\n> > - if ret < 0:\n> > - print('Can\\'t set buffer for request')\n> > - return -1\n> > + request.add_buffer(stream, buffer)\n> >\n> > # Controls can be added to a request on a per frame basis.\n> > request.set_control(libcam.controls.Brightness, 0.5)\n> > diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n> > index 5f93574f..4b85408f 100755\n> > --- a/src/py/examples/simple-capture.py\n> > +++ b/src/py/examples/simple-capture.py\n> > @@ -43,8 +43,7 @@ def main():\n> >\n> > # Acquire the camera for our use\n> >\n> > - ret = cam.acquire()\n> > - assert ret == 0\n> > + cam.acquire()\n> >\n> > # Configure the camera\n> >\n> > @@ -60,8 +59,7 @@ def main():\n> > w, h = [int(v) for v in args.size.split('x')]\n> > stream_config.size = libcam.Size(w, h)\n> >\n> > - ret = cam.configure(cam_config)\n> > - assert ret == 0\n> > + cam.configure(cam_config)\n> >\n> > print(f'Capturing {TOTAL_FRAMES} frames with {stream_config}')\n> >\n> > @@ -83,15 +81,13 @@ def main():\n> > req = cam.create_request(i)\n> >\n> > buffer = allocator.buffers(stream)[i]\n> > - ret = req.add_buffer(stream, buffer)\n> > - assert ret == 0\n> > + req.add_buffer(stream, buffer)\n> >\n> > reqs.append(req)\n> >\n> > # Start the camera\n> >\n> > - ret = cam.start()\n> > - assert ret == 0\n> > + cam.start()\n> >\n> > # frames_queued and frames_done track the number of frames queued and done\n> >\n> > @@ -101,8 +97,7 @@ def main():\n> > # Queue the requests to the camera\n> >\n> > for req in reqs:\n> > - ret = cam.queue_request(req)\n> > - assert ret == 0\n> > + cam.queue_request(req)\n> > frames_queued += 1\n> >\n> > # The main loop. Wait for the queued Requests to complete, process them,\n> > @@ -155,13 +150,11 @@ def main():\n> >\n> > # Stop the camera\n> >\n> > - ret = cam.stop()\n> > - assert ret == 0\n> > + cam.stop()\n> >\n> > # Release the camera\n> >\n> > - ret = cam.release()\n> > - assert ret == 0\n> > + cam.release()\n> >\n> > return 0\n> >\n> > diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n> > index 26a8060b..e1cb931e 100755\n> > --- a/src/py/examples/simple-continuous-capture.py\n> > +++ b/src/py/examples/simple-continuous-capture.py\n> > @@ -28,8 +28,7 @@ class CameraCaptureContext:\n> >\n> > # Acquire the camera for our use\n> >\n> > - ret = cam.acquire()\n> > - assert ret == 0\n> > + cam.acquire()\n> >\n> > # Configure the camera\n> >\n> > @@ -37,8 +36,7 @@ class CameraCaptureContext:\n> >\n> > stream_config = cam_config.at(0)\n> >\n> > - ret = cam.configure(cam_config)\n> > - assert ret == 0\n> > + cam.configure(cam_config)\n> >\n> > stream = stream_config.stream\n> >\n> > @@ -62,8 +60,7 @@ class CameraCaptureContext:\n> > req = cam.create_request(idx)\n> >\n> > buffer = allocator.buffers(stream)[i]\n> > - ret = req.add_buffer(stream, buffer)\n> > - assert ret == 0\n> > + req.add_buffer(stream, buffer)\n> >\n> > self.reqs.append(req)\n> >\n> > @@ -73,13 +70,11 @@ class CameraCaptureContext:\n> > def uninit_camera(self):\n> > # Stop the camera\n> >\n> > - ret = self.cam.stop()\n> > - assert ret == 0\n> > + self.cam.stop()\n> >\n> > # Release the camera\n> >\n> > - ret = self.cam.release()\n> > - assert ret == 0\n> > + self.cam.release()\n> >\n> >\n> > # A container class for our state\n> > @@ -145,8 +140,7 @@ class CaptureContext:\n> >\n> > for cam_ctx in self.camera_contexts:\n> > for req in cam_ctx.reqs:\n> > - ret = cam_ctx.cam.queue_request(req)\n> > - assert ret == 0\n> > + cam_ctx.cam.queue_request(req)\n> >\n> > # Use Selector to wait for events from the camera and from the keyboard\n> >\n> > @@ -177,8 +171,7 @@ def main():\n> > # Start the cameras\n> >\n> > for cam_ctx in ctx.camera_contexts:\n> > - ret = cam_ctx.cam.start()\n> > - assert ret == 0\n> > + cam_ctx.cam.start()\n> >\n> > ctx.capture()\n> >\n> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n> > index e7d078b5..6cd99919 100644\n> > --- a/src/py/libcamera/py_main.cpp\n> > +++ b/src/py/libcamera/py_main.cpp\n> > @@ -111,8 +111,19 @@ PYBIND11_MODULE(_libcamera, m)\n> >\n> > \tpyCamera\n> > \t\t.def_property_readonly(\"id\", &Camera::id)\n> > -\t\t.def(\"acquire\", &Camera::acquire)\n> > -\t\t.def(\"release\", &Camera::release)\n> > +\t\t.def(\"acquire\", [](Camera &self) {\n> > +\t\t\tint ret = self.acquire();\n> > +\t\t\tif (ret)\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to acquire camera\");\n> > +\t\t})\n> > +\t\t.def(\"release\", [](Camera &self) {\n> > +\t\t\tint ret = self.release();\n> > +\t\t\t/* \\todo Should we ignore the error? */\n\nShould we ? It indicates an error in the caller, right ?\n\n> > +\t\t\tif (ret)\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to release camera\");\n> > +\t\t})\n> > \t\t.def(\"start\", [](Camera &self,\n> > \t\t const std::unordered_map &controls) {\n> > \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n> > @@ -132,20 +143,20 @@ PYBIND11_MODULE(_libcamera, m)\n> > \t\t\tint ret = self.start(&controlList);\n> > \t\t\tif (ret) {\n> > \t\t\t\tself.requestCompleted.disconnect();\n> > -\t\t\t\treturn ret;\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to start camera\");\n> > \t\t\t}\n> > -\n> > -\t\t\treturn 0;\n> > \t\t}, py::arg(\"controls\") = std::unordered_map())\n> >\n> > \t\t.def(\"stop\", [](Camera &self) {\n> > \t\t\tint ret = self.stop();\n> > -\t\t\tif (ret)\n> > -\t\t\t\treturn ret;\n> >\n> > \t\t\tself.requestCompleted.disconnect();\n> >\n> > -\t\t\treturn 0;\n> > +\t\t\t/* \\todo Should we just ignore the error? */\n> \n> There previously was an assertion, so I think it's fine to keep\n> checking for errors.\n> \n> > +\t\t\tif (ret)\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to start camera\");\n> \n> s/start/stop\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > \t\t})\n> >\n> > \t\t.def(\"__str__\", [](Camera &self) {\n> > @@ -154,9 +165,20 @@ PYBIND11_MODULE(_libcamera, m)\n> >\n> > \t\t/* Keep the camera alive, as StreamConfiguration contains a Stream* */\n> > \t\t.def(\"generate_configuration\", &Camera::generateConfiguration, py::keep_alive<0, 1>())\n> > -\t\t.def(\"configure\", &Camera::configure)\n> > +\t\t.def(\"configure\", [](Camera &self, CameraConfiguration *config) {\n> > +\t\t\tint ret = self.configure(config);\n> > +\t\t\tif (ret)\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to configure camera\");\n> > +\t\t})\n> >\n> > -\t\t.def(\"create_request\", &Camera::createRequest, py::arg(\"cookie\") = 0)\n> > +\t\t.def(\"create_request\", [](Camera &self, uint64_t cookie) {\n> > +\t\t\tstd::unique_ptr req = self.createRequest(cookie);\n> > +\t\t\tif (!req)\n> > +\t\t\t\tthrow std::system_error(ENOMEM, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to create request\");\n> > +\t\t\treturn req;\n> > +\t\t}, py::arg(\"cookie\") = 0)\n> >\n> > \t\t.def(\"queue_request\", [](Camera &self, Request *req) {\n> > \t\t\tpy::object py_req = py::cast(req);\n> > @@ -169,10 +191,11 @@ PYBIND11_MODULE(_libcamera, m)\n> > \t\t\tpy_req.inc_ref();\n> >\n> > \t\t\tint ret = self.queueRequest(req);\n> > -\t\t\tif (ret)\n> > +\t\t\tif (ret) {\n> > \t\t\t\tpy_req.dec_ref();\n> > -\n> > -\t\t\treturn ret;\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to queue request\");\n> > +\t\t\t}\n> > \t\t})\n> >\n> > \t\t.def_property_readonly(\"streams\", [](Camera &self) {\n> > @@ -250,7 +273,13 @@ PYBIND11_MODULE(_libcamera, m)\n> >\n> > \tpyFrameBufferAllocator\n> > \t\t.def(py::init>(), py::keep_alive<1, 2>())\n> > -\t\t.def(\"allocate\", &FrameBufferAllocator::allocate)\n> > +\t\t.def(\"allocate\", [](FrameBufferAllocator &self, Stream *stream) {\n> > +\t\t\tint ret = self.allocate(stream);\n> > +\t\t\tif (ret < 0)\n\nShouldn't 0 be also considered an error ?\n\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to queue request\");\n\nBad copy & paste.\n\n> > +\t\t\treturn ret;\n> > +\t\t})\n> > \t\t.def_property_readonly(\"allocated\", &FrameBufferAllocator::allocated)\n> > \t\t/* Create a list of FrameBuffers, where each FrameBuffer has a keep-alive to FrameBufferAllocator */\n> > \t\t.def(\"buffers\", [](FrameBufferAllocator &self, Stream *stream) {\n> > @@ -327,7 +356,10 @@ PYBIND11_MODULE(_libcamera, m)\n> > \tpyRequest\n> > \t\t/* \\todo Fence is not supported, so we cannot expose addBuffer() directly */\n> > \t\t.def(\"add_buffer\", [](Request &self, const Stream *stream, FrameBuffer *buffer) {\n> > -\t\t\treturn self.addBuffer(stream, buffer);\n> > +\t\t\tint ret = self.addBuffer(stream, buffer);\n\nExtra space after =\n\n> > +\t\t\tif (ret)\n> > +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n> > +\t\t\t\t\t\t\t\"Failed to add buffer\");\n> > \t\t}, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */\n> > \t\t.def_property_readonly(\"status\", &Request::status)\n> > \t\t.def_property_readonly(\"buffers\", &Request::buffers)\n> > diff --git a/test/py/unittests.py b/test/py/unittests.py\n> > index 6dea80fc..794e46be 100755\n> > --- a/test/py/unittests.py\n> > +++ b/test/py/unittests.py\n> > @@ -42,31 +42,26 @@ class SimpleTestMethods(BaseTestCase):\n> > cam = cm.get('platform/vimc.0 Sensor B')\n> > self.assertIsNotNone(cam)\n> >\n> > - ret = cam.acquire()\n> > - self.assertZero(ret)\n> > + cam.acquire()\n\nWill the test framework produce an error report that provide as much\ndebugging information as the assert functions ?\n\n> >\n> > - ret = cam.release()\n> > - self.assertZero(ret)\n> > + cam.release()\n> >\n> > def test_double_acquire(self):\n> > cm = libcam.CameraManager.singleton()\n> > cam = cm.get('platform/vimc.0 Sensor B')\n> > self.assertIsNotNone(cam)\n> >\n> > - ret = cam.acquire()\n> > - self.assertZero(ret)\n> > + cam.acquire()\n> >\n> > libcam.log_set_level('Camera', 'FATAL')\n> > - ret = cam.acquire()\n> > - self.assertEqual(ret, -errno.EBUSY)\n> > + with self.assertRaises(RuntimeError):\n> > + cam.acquire()\n> > libcam.log_set_level('Camera', 'ERROR')\n> >\n> > - ret = cam.release()\n> > - self.assertZero(ret)\n> > + cam.release()\n> >\n> > - ret = cam.release()\n> > - # I expected EBUSY, but looks like double release works fine\n> > - self.assertZero(ret)\n> > + # I expected exception here, but looks like double release works fine\n> > + cam.release()\n> >\n> >\n> > class CameraTesterBase(BaseTestCase):\n> > @@ -80,11 +75,7 @@ class CameraTesterBase(BaseTestCase):\n> > self.cm = None\n> > self.skipTest('No vimc found')\n> >\n> > - ret = self.cam.acquire()\n> > - if ret != 0:\n> > - self.cam = None\n> > - self.cm = None\n> > - raise Exception('Failed to acquire camera')\n> > + self.cam.acquire()\n> >\n> > self.wr_cam = weakref.ref(self.cam)\n> > self.wr_cm = weakref.ref(self.cm)\n> > @@ -93,9 +84,7 @@ class CameraTesterBase(BaseTestCase):\n> > # If a test fails, the camera may be in running state. So always stop.\n> > self.cam.stop()\n> >\n> > - ret = self.cam.release()\n> > - if ret != 0:\n> > - raise Exception('Failed to release camera')\n> > + self.cam.release()\n> >\n> > self.cam = None\n> > self.cm = None\n> > @@ -115,8 +104,7 @@ class AllocatorTestMethods(CameraTesterBase):\n> > streamconfig = camconfig.at(0)\n> > wr_streamconfig = weakref.ref(streamconfig)\n> >\n> > - ret = cam.configure(camconfig)\n> > - self.assertZero(ret)\n> > + cam.configure(camconfig)\n> >\n> > stream = streamconfig.stream\n> > wr_stream = weakref.ref(stream)\n> > @@ -129,8 +117,8 @@ class AllocatorTestMethods(CameraTesterBase):\n> > self.assertIsNotNone(wr_streamconfig())\n> >\n> > allocator = libcam.FrameBufferAllocator(cam)\n> > - ret = allocator.allocate(stream)\n> > - self.assertTrue(ret > 0)\n> > + num_bufs = allocator.allocate(stream)\n> > + self.assertTrue(num_bufs > 0)\n> > wr_allocator = weakref.ref(allocator)\n> >\n> > buffers = allocator.buffers(stream)\n> > @@ -173,14 +161,13 @@ class SimpleCaptureMethods(CameraTesterBase):\n> > self.assertIsNotNone(fmts)\n> > fmts = None\n> >\n> > - ret = cam.configure(camconfig)\n> > - self.assertZero(ret)\n> > + cam.configure(camconfig)\n> >\n> > stream = streamconfig.stream\n> >\n> > allocator = libcam.FrameBufferAllocator(cam)\n> > - ret = allocator.allocate(stream)\n> > - self.assertTrue(ret > 0)\n> > + num_bufs = allocator.allocate(stream)\n> > + self.assertTrue(num_bufs > 0)\n> >\n> > num_bufs = len(allocator.buffers(stream))\n> >\n> > @@ -190,19 +177,16 @@ class SimpleCaptureMethods(CameraTesterBase):\n> > self.assertIsNotNone(req)\n> >\n> > buffer = allocator.buffers(stream)[i]\n> > - ret = req.add_buffer(stream, buffer)\n> > - self.assertZero(ret)\n> > + req.add_buffer(stream, buffer)\n> >\n> > reqs.append(req)\n> >\n> > buffer = None\n> >\n> > - ret = cam.start()\n> > - self.assertZero(ret)\n> > + cam.start()\n> >\n> > for req in reqs:\n> > - ret = cam.queue_request(req)\n> > - self.assertZero(ret)\n> > + cam.queue_request(req)\n> >\n> > reqs = None\n> > gc.collect()\n> > @@ -230,8 +214,7 @@ class SimpleCaptureMethods(CameraTesterBase):\n> > reqs = None\n> > gc.collect()\n> >\n> > - ret = cam.stop()\n> > - self.assertZero(ret)\n> > + cam.stop()\n> >\n> > def test_select(self):\n> > cm = self.cm\n> > @@ -245,14 +228,13 @@ class SimpleCaptureMethods(CameraTesterBase):\n> > self.assertIsNotNone(fmts)\n> > fmts = None\n> >\n> > - ret = cam.configure(camconfig)\n> > - self.assertZero(ret)\n> > + cam.configure(camconfig)\n> >\n> > stream = streamconfig.stream\n> >\n> > allocator = libcam.FrameBufferAllocator(cam)\n> > - ret = allocator.allocate(stream)\n> > - self.assertTrue(ret > 0)\n> > + num_bufs = allocator.allocate(stream)\n> > + self.assertTrue(num_bufs > 0)\n> >\n> > num_bufs = len(allocator.buffers(stream))\n> >\n> > @@ -262,19 +244,16 @@ class SimpleCaptureMethods(CameraTesterBase):\n> > self.assertIsNotNone(req)\n> >\n> > buffer = allocator.buffers(stream)[i]\n> > - ret = req.add_buffer(stream, buffer)\n> > - self.assertZero(ret)\n> > + req.add_buffer(stream, buffer)\n> >\n> > reqs.append(req)\n> >\n> > buffer = None\n> >\n> > - ret = cam.start()\n> > - self.assertZero(ret)\n> > + cam.start()\n> >\n> > for req in reqs:\n> > - ret = cam.queue_request(req)\n> > - self.assertZero(ret)\n> > + cam.queue_request(req)\n> >\n> > reqs = None\n> > gc.collect()\n> > @@ -303,8 +282,7 @@ class SimpleCaptureMethods(CameraTesterBase):\n> > reqs = None\n> > gc.collect()\n> >\n> > - ret = cam.stop()\n> > - self.assertZero(ret)\n> > + cam.stop()\n> >\n> >\n> > # Recursively expand slist's objects into olist, using seen to track already","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7822BC3272\n\tfor ;\n\tThu, 18 Aug 2022 20:56:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBE6C61FC0;\n\tThu, 18 Aug 2022 22:56:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6899E61FA7\n\tfor ;\n\tThu, 18 Aug 2022 22:56:41 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A7CA38B;\n\tThu, 18 Aug 2022 22:56:40 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660856202;\n\tbh=IOTneFoOmvrG1dnlPSSqKQSxJTobhVwEdWUR95qS4wE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CyzbRvxCgV61LI0ahhvpNzR1acq4DVoXioO1+j/iAJPSOTq9xVaf5PHExXdPec+8z\n\tM/+TrMXgXBTKKyOlidVYE1zY9aN1vomEEkEcPI7bI2bH4a8kTg8LO3gBEH/xOSjKC9\n\tK+/RKehPvsNYem0zIe3yqPUEyzqd+52sAZvPfS2WtAUzsaTqz47mlVb6ftk1xzRG4c\n\t+mNxSRPp6fqkOubWJkge6PH9VXpNQui6ZW+NMBT/cmYztQfNE+2iL5egzmlTyGhpbG\n\t5deXENT09GPexQPUrWAZ3G0RExlHg6ZIt3t0k9JEL8DhtoWXRr1C0/vCSa/ifqD3JL\n\tYDFDzW92SUz4Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660856200;\n\tbh=IOTneFoOmvrG1dnlPSSqKQSxJTobhVwEdWUR95qS4wE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RF4bJeAfazmC0O9mGMp4he5SXyio2kttF+CFmZp6Hw90w0Zn570VXLn58BFT/72ae\n\tc0ykg3f8YKdnOEzWgd7gZEsecNVn/jYCovj/2NbWBvIp5ONjaEkNtmkJ9AXN6iXuj/\n\tnlgFzGv1OFMDHoU1mTlUn58prcl7QGNvr2nNyQfQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RF4bJeAf\"; dkim-atps=neutral","Date":"Thu, 18 Aug 2022 23:56:37 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-11-tomi.valkeinen@ideasonboard.com>\n\t<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24705,"web_url":"https://patchwork.libcamera.org/comment/24705/","msgid":"<59015b24-db67-3b73-ff19-fd63c7d16327@ideasonboard.com>","date":"2022-08-19T06:43:02","subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 17:36, Jacopo Mondi wrote:\n> Hi Tomi\n> \n> On Fri, Jul 01, 2022 at 11:45:14AM +0300, Tomi Valkeinen wrote:\n>> We have multiple methods which return an error code, mimicing the C++\n>> API. Using exceptions is more natural in the Python API, so change all\n>> those methods to raise an Exception instead.\n>>\n>> Signed-off-by: Tomi Valkeinen \n>> ---\n>> src/py/cam/cam.py | 16 +----\n>> src/py/examples/simple-cam.py | 15 +---\n>> src/py/examples/simple-capture.py | 21 ++----\n>> src/py/examples/simple-continuous-capture.py | 21 ++----\n>> src/py/libcamera/py_main.cpp | 62 ++++++++++++----\n>> test/py/unittests.py | 76 +++++++-------------\n>> 6 files changed, 93 insertions(+), 118 deletions(-)\n>>\n>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n>> index 2701d937..6b6b678b 100755\n>> --- a/src/py/cam/cam.py\n>> +++ b/src/py/cam/cam.py\n>> @@ -158,9 +158,7 @@ class CameraContext:\n>>\n>> print('Camera configuration adjusted')\n>>\n>> - r = self.camera.configure(camconfig)\n>> - if r != 0:\n>> - raise Exception('Configure failed')\n>> + self.camera.configure(camconfig)\n>>\n>> self.stream_names = {}\n>> self.streams = []\n>> @@ -175,12 +173,7 @@ class CameraContext:\n>> allocator = libcam.FrameBufferAllocator(self.camera)\n>>\n>> for stream in self.streams:\n>> - ret = allocator.allocate(stream)\n>> - if ret < 0:\n>> - print('Cannot allocate buffers')\n>> - exit(-1)\n>> -\n>> - allocated = len(allocator.buffers(stream))\n>> + allocated = allocator.allocate(stream)\n>>\n>> print('{}-{}: Allocated {} buffers'.format(self.id, self.stream_names[stream], allocated))\n>>\n>> @@ -205,10 +198,7 @@ class CameraContext:\n>> buffers = self.allocator.buffers(stream)\n>> buffer = buffers[buf_num]\n>>\n>> - ret = request.add_buffer(stream, buffer)\n>> - if ret < 0:\n>> - print('Can not set buffer for request')\n>> - exit(-1)\n>> + request.add_buffer(stream, buffer)\n>>\n>> requests.append(request)\n>>\n>> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n>> index b3e97ca7..1cd1019d 100755\n>> --- a/src/py/examples/simple-cam.py\n>> +++ b/src/py/examples/simple-cam.py\n>> @@ -259,12 +259,7 @@ def main():\n>> allocator = libcam.FrameBufferAllocator(camera)\n>>\n>> for cfg in config:\n>> - ret = allocator.allocate(cfg.stream)\n>> - if ret < 0:\n>> - print('Can\\'t allocate buffers')\n>> - return -1\n>> -\n>> - allocated = len(allocator.buffers(cfg.stream))\n>> + allocated = allocator.allocate(cfg.stream)\n>> print(f'Allocated {allocated} buffers for stream')\n>>\n>> # --------------------------------------------------------------------\n>> @@ -289,15 +284,9 @@ def main():\n>> requests = []\n>> for i in range(len(buffers)):\n>> request = camera.create_request()\n>> - if not request:\n>> - print('Can\\'t create request')\n>> - return -1\n>>\n>> buffer = buffers[i]\n>> - ret = request.add_buffer(stream, buffer)\n>> - if ret < 0:\n>> - print('Can\\'t set buffer for request')\n>> - return -1\n>> + request.add_buffer(stream, buffer)\n>>\n>> # Controls can be added to a request on a per frame basis.\n>> request.set_control(libcam.controls.Brightness, 0.5)\n>> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n>> index 5f93574f..4b85408f 100755\n>> --- a/src/py/examples/simple-capture.py\n>> +++ b/src/py/examples/simple-capture.py\n>> @@ -43,8 +43,7 @@ def main():\n>>\n>> # Acquire the camera for our use\n>>\n>> - ret = cam.acquire()\n>> - assert ret == 0\n>> + cam.acquire()\n>>\n>> # Configure the camera\n>>\n>> @@ -60,8 +59,7 @@ def main():\n>> w, h = [int(v) for v in args.size.split('x')]\n>> stream_config.size = libcam.Size(w, h)\n>>\n>> - ret = cam.configure(cam_config)\n>> - assert ret == 0\n>> + cam.configure(cam_config)\n>>\n>> print(f'Capturing {TOTAL_FRAMES} frames with {stream_config}')\n>>\n>> @@ -83,15 +81,13 @@ def main():\n>> req = cam.create_request(i)\n>>\n>> buffer = allocator.buffers(stream)[i]\n>> - ret = req.add_buffer(stream, buffer)\n>> - assert ret == 0\n>> + req.add_buffer(stream, buffer)\n>>\n>> reqs.append(req)\n>>\n>> # Start the camera\n>>\n>> - ret = cam.start()\n>> - assert ret == 0\n>> + cam.start()\n>>\n>> # frames_queued and frames_done track the number of frames queued and done\n>>\n>> @@ -101,8 +97,7 @@ def main():\n>> # Queue the requests to the camera\n>>\n>> for req in reqs:\n>> - ret = cam.queue_request(req)\n>> - assert ret == 0\n>> + cam.queue_request(req)\n>> frames_queued += 1\n>>\n>> # The main loop. Wait for the queued Requests to complete, process them,\n>> @@ -155,13 +150,11 @@ def main():\n>>\n>> # Stop the camera\n>>\n>> - ret = cam.stop()\n>> - assert ret == 0\n>> + cam.stop()\n>>\n>> # Release the camera\n>>\n>> - ret = cam.release()\n>> - assert ret == 0\n>> + cam.release()\n>>\n>> return 0\n>>\n>> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n>> index 26a8060b..e1cb931e 100755\n>> --- a/src/py/examples/simple-continuous-capture.py\n>> +++ b/src/py/examples/simple-continuous-capture.py\n>> @@ -28,8 +28,7 @@ class CameraCaptureContext:\n>>\n>> # Acquire the camera for our use\n>>\n>> - ret = cam.acquire()\n>> - assert ret == 0\n>> + cam.acquire()\n>>\n>> # Configure the camera\n>>\n>> @@ -37,8 +36,7 @@ class CameraCaptureContext:\n>>\n>> stream_config = cam_config.at(0)\n>>\n>> - ret = cam.configure(cam_config)\n>> - assert ret == 0\n>> + cam.configure(cam_config)\n>>\n>> stream = stream_config.stream\n>>\n>> @@ -62,8 +60,7 @@ class CameraCaptureContext:\n>> req = cam.create_request(idx)\n>>\n>> buffer = allocator.buffers(stream)[i]\n>> - ret = req.add_buffer(stream, buffer)\n>> - assert ret == 0\n>> + req.add_buffer(stream, buffer)\n>>\n>> self.reqs.append(req)\n>>\n>> @@ -73,13 +70,11 @@ class CameraCaptureContext:\n>> def uninit_camera(self):\n>> # Stop the camera\n>>\n>> - ret = self.cam.stop()\n>> - assert ret == 0\n>> + self.cam.stop()\n>>\n>> # Release the camera\n>>\n>> - ret = self.cam.release()\n>> - assert ret == 0\n>> + self.cam.release()\n>>\n>>\n>> # A container class for our state\n>> @@ -145,8 +140,7 @@ class CaptureContext:\n>>\n>> for cam_ctx in self.camera_contexts:\n>> for req in cam_ctx.reqs:\n>> - ret = cam_ctx.cam.queue_request(req)\n>> - assert ret == 0\n>> + cam_ctx.cam.queue_request(req)\n>>\n>> # Use Selector to wait for events from the camera and from the keyboard\n>>\n>> @@ -177,8 +171,7 @@ def main():\n>> # Start the cameras\n>>\n>> for cam_ctx in ctx.camera_contexts:\n>> - ret = cam_ctx.cam.start()\n>> - assert ret == 0\n>> + cam_ctx.cam.start()\n>>\n>> ctx.capture()\n>>\n>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>> index e7d078b5..6cd99919 100644\n>> --- a/src/py/libcamera/py_main.cpp\n>> +++ b/src/py/libcamera/py_main.cpp\n>> @@ -111,8 +111,19 @@ PYBIND11_MODULE(_libcamera, m)\n>>\n>> \tpyCamera\n>> \t\t.def_property_readonly(\"id\", &Camera::id)\n>> -\t\t.def(\"acquire\", &Camera::acquire)\n>> -\t\t.def(\"release\", &Camera::release)\n>> +\t\t.def(\"acquire\", [](Camera &self) {\n>> +\t\t\tint ret = self.acquire();\n>> +\t\t\tif (ret)\n>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>> +\t\t\t\t\t\t\t\"Failed to acquire camera\");\n>> +\t\t})\n>> +\t\t.def(\"release\", [](Camera &self) {\n>> +\t\t\tint ret = self.release();\n>> +\t\t\t/* \\todo Should we ignore the error? */\n>> +\t\t\tif (ret)\n>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>> +\t\t\t\t\t\t\t\"Failed to release camera\");\n>> +\t\t})\n>> \t\t.def(\"start\", [](Camera &self,\n>> \t\t const std::unordered_map &controls) {\n>> \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n>> @@ -132,20 +143,20 @@ PYBIND11_MODULE(_libcamera, m)\n>> \t\t\tint ret = self.start(&controlList);\n>> \t\t\tif (ret) {\n>> \t\t\t\tself.requestCompleted.disconnect();\n>> -\t\t\t\treturn ret;\n>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>> +\t\t\t\t\t\t\t\"Failed to start camera\");\n>> \t\t\t}\n>> -\n>> -\t\t\treturn 0;\n>> \t\t}, py::arg(\"controls\") = std::unordered_map())\n>>\n>> \t\t.def(\"stop\", [](Camera &self) {\n>> \t\t\tint ret = self.stop();\n>> -\t\t\tif (ret)\n>> -\t\t\t\treturn ret;\n>>\n>> \t\t\tself.requestCompleted.disconnect();\n>>\n>> -\t\t\treturn 0;\n>> +\t\t\t/* \\todo Should we just ignore the error? */\n> \n> There previously was an assertion, so I think it's fine to keep\n> checking for errors.\n> \n>> +\t\t\tif (ret)\n>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>> +\t\t\t\t\t\t\t\"Failed to start camera\");\n> \n> s/start/stop\n\nIndeed, thanks =).\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 31F04C3272\n\tfor ;\n\tFri, 19 Aug 2022 06:43:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5176C61FC0;\n\tFri, 19 Aug 2022 08:43:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 56AED603E2\n\tfor ;\n\tFri, 19 Aug 2022 08:43:06 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7686F3F1;\n\tFri, 19 Aug 2022 08:43:05 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660891387;\n\tbh=GgvjjqE9M8GKOQICZREjl5G8N5Pb2T0jEIfTRyL/bC0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=2LncJlQ9AZc/ofxBsjlF6iOuswFuVq+j9iTMLFCHD1m//KbcPFjhZZacmOhPedzTy\n\tq2t2i5uxC4RAnzut6oFiuqH3rzVyhE9IN1WDZsHuCaFigZBjGR6/g4peQL4XGUB7JK\n\tJv+O5Xz9hd4Jnm2zoaA9HChobH8vf5zLcYQhMHEnz0YR5os+J8JJqQ2Z5Y2roN0sk1\n\t1LSsoRNkqhN97RB+XFanwvZ8G3AbB/Q80Qa/jvWMyT7+KzvsNKaWk1s2zg1/C3UkcG\n\t1SoX7ayILa0XBEHMw5Gx3zzaUAReeV6lfZPAjvbZd7uGy19gk/BXk6KeXfiSIDAscw\n\t66oAHtjHvc0vA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660891385;\n\tbh=GgvjjqE9M8GKOQICZREjl5G8N5Pb2T0jEIfTRyL/bC0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=f3he1rwYalw6QLb4hK9kQaVf7d5qRz1g0NKEAgA9Q53YvaEd97Ib3gGUkJ+c14E0K\n\tERquzPhG35SL5lKqGWphSWx2d59hZaRuyqnVlQHr3KwpKhkBc7zk56RDhWyRI9owNG\n\tZAvYwTrrSru01on5+YyErvrtg5CVzKip3UlVHVNs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"f3he1rwY\"; dkim-atps=neutral","Message-ID":"<59015b24-db67-3b73-ff19-fd63c7d16327@ideasonboard.com>","Date":"Fri, 19 Aug 2022 09:43:02 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-11-tomi.valkeinen@ideasonboard.com>\n\t<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>","In-Reply-To":"<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24706,"web_url":"https://patchwork.libcamera.org/comment/24706/","msgid":"","date":"2022-08-19T06:51:41","subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","submitter":{"id":109,"url":"https://patchwork.libcamera.org/api/people/109/","name":"Tomi Valkeinen","email":"tomi.valkeinen@ideasonboard.com"},"content":"On 18/08/2022 23:56, Laurent Pinchart wrote:\n> On Thu, Aug 18, 2022 at 04:36:06PM +0200, Jacopo Mondi wrote:\n>> On Fri, Jul 01, 2022 at 11:45:14AM +0300, Tomi Valkeinen wrote:\n>>> We have multiple methods which return an error code, mimicing the C++\n> \n> s/mimicing/mimicking/\n> \n>>> API. Using exceptions is more natural in the Python API, so change all\n>>> those methods to raise an Exception instead.\n>>>\n>>> Signed-off-by: Tomi Valkeinen \n>>> ---\n>>> src/py/cam/cam.py | 16 +----\n>>> src/py/examples/simple-cam.py | 15 +---\n>>> src/py/examples/simple-capture.py | 21 ++----\n>>> src/py/examples/simple-continuous-capture.py | 21 ++----\n>>> src/py/libcamera/py_main.cpp | 62 ++++++++++++----\n>>> test/py/unittests.py | 76 +++++++-------------\n>>> 6 files changed, 93 insertions(+), 118 deletions(-)\n>>>\n>>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py\n>>> index 2701d937..6b6b678b 100755\n>>> --- a/src/py/cam/cam.py\n>>> +++ b/src/py/cam/cam.py\n>>> @@ -158,9 +158,7 @@ class CameraContext:\n>>>\n>>> print('Camera configuration adjusted')\n>>>\n>>> - r = self.camera.configure(camconfig)\n>>> - if r != 0:\n>>> - raise Exception('Configure failed')\n>>> + self.camera.configure(camconfig)\n>>>\n>>> self.stream_names = {}\n>>> self.streams = []\n>>> @@ -175,12 +173,7 @@ class CameraContext:\n>>> allocator = libcam.FrameBufferAllocator(self.camera)\n>>>\n>>> for stream in self.streams:\n>>> - ret = allocator.allocate(stream)\n>>> - if ret < 0:\n>>> - print('Cannot allocate buffers')\n>>> - exit(-1)\n>>> -\n>>> - allocated = len(allocator.buffers(stream))\n>>> + allocated = allocator.allocate(stream)\n>>>\n>>> print('{}-{}: Allocated {} buffers'.format(self.id, self.stream_names[stream], allocated))\n>>>\n>>> @@ -205,10 +198,7 @@ class CameraContext:\n>>> buffers = self.allocator.buffers(stream)\n>>> buffer = buffers[buf_num]\n>>>\n>>> - ret = request.add_buffer(stream, buffer)\n>>> - if ret < 0:\n>>> - print('Can not set buffer for request')\n>>> - exit(-1)\n>>> + request.add_buffer(stream, buffer)\n>>>\n>>> requests.append(request)\n>>>\n>>> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py\n>>> index b3e97ca7..1cd1019d 100755\n>>> --- a/src/py/examples/simple-cam.py\n>>> +++ b/src/py/examples/simple-cam.py\n>>> @@ -259,12 +259,7 @@ def main():\n>>> allocator = libcam.FrameBufferAllocator(camera)\n>>>\n>>> for cfg in config:\n>>> - ret = allocator.allocate(cfg.stream)\n>>> - if ret < 0:\n>>> - print('Can\\'t allocate buffers')\n>>> - return -1\n>>> -\n>>> - allocated = len(allocator.buffers(cfg.stream))\n>>> + allocated = allocator.allocate(cfg.stream)\n>>> print(f'Allocated {allocated} buffers for stream')\n>>>\n>>> # --------------------------------------------------------------------\n>>> @@ -289,15 +284,9 @@ def main():\n>>> requests = []\n>>> for i in range(len(buffers)):\n>>> request = camera.create_request()\n>>> - if not request:\n>>> - print('Can\\'t create request')\n>>> - return -1\n>>>\n>>> buffer = buffers[i]\n>>> - ret = request.add_buffer(stream, buffer)\n>>> - if ret < 0:\n>>> - print('Can\\'t set buffer for request')\n>>> - return -1\n>>> + request.add_buffer(stream, buffer)\n>>>\n>>> # Controls can be added to a request on a per frame basis.\n>>> request.set_control(libcam.controls.Brightness, 0.5)\n>>> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py\n>>> index 5f93574f..4b85408f 100755\n>>> --- a/src/py/examples/simple-capture.py\n>>> +++ b/src/py/examples/simple-capture.py\n>>> @@ -43,8 +43,7 @@ def main():\n>>>\n>>> # Acquire the camera for our use\n>>>\n>>> - ret = cam.acquire()\n>>> - assert ret == 0\n>>> + cam.acquire()\n>>>\n>>> # Configure the camera\n>>>\n>>> @@ -60,8 +59,7 @@ def main():\n>>> w, h = [int(v) for v in args.size.split('x')]\n>>> stream_config.size = libcam.Size(w, h)\n>>>\n>>> - ret = cam.configure(cam_config)\n>>> - assert ret == 0\n>>> + cam.configure(cam_config)\n>>>\n>>> print(f'Capturing {TOTAL_FRAMES} frames with {stream_config}')\n>>>\n>>> @@ -83,15 +81,13 @@ def main():\n>>> req = cam.create_request(i)\n>>>\n>>> buffer = allocator.buffers(stream)[i]\n>>> - ret = req.add_buffer(stream, buffer)\n>>> - assert ret == 0\n>>> + req.add_buffer(stream, buffer)\n>>>\n>>> reqs.append(req)\n>>>\n>>> # Start the camera\n>>>\n>>> - ret = cam.start()\n>>> - assert ret == 0\n>>> + cam.start()\n>>>\n>>> # frames_queued and frames_done track the number of frames queued and done\n>>>\n>>> @@ -101,8 +97,7 @@ def main():\n>>> # Queue the requests to the camera\n>>>\n>>> for req in reqs:\n>>> - ret = cam.queue_request(req)\n>>> - assert ret == 0\n>>> + cam.queue_request(req)\n>>> frames_queued += 1\n>>>\n>>> # The main loop. Wait for the queued Requests to complete, process them,\n>>> @@ -155,13 +150,11 @@ def main():\n>>>\n>>> # Stop the camera\n>>>\n>>> - ret = cam.stop()\n>>> - assert ret == 0\n>>> + cam.stop()\n>>>\n>>> # Release the camera\n>>>\n>>> - ret = cam.release()\n>>> - assert ret == 0\n>>> + cam.release()\n>>>\n>>> return 0\n>>>\n>>> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py\n>>> index 26a8060b..e1cb931e 100755\n>>> --- a/src/py/examples/simple-continuous-capture.py\n>>> +++ b/src/py/examples/simple-continuous-capture.py\n>>> @@ -28,8 +28,7 @@ class CameraCaptureContext:\n>>>\n>>> # Acquire the camera for our use\n>>>\n>>> - ret = cam.acquire()\n>>> - assert ret == 0\n>>> + cam.acquire()\n>>>\n>>> # Configure the camera\n>>>\n>>> @@ -37,8 +36,7 @@ class CameraCaptureContext:\n>>>\n>>> stream_config = cam_config.at(0)\n>>>\n>>> - ret = cam.configure(cam_config)\n>>> - assert ret == 0\n>>> + cam.configure(cam_config)\n>>>\n>>> stream = stream_config.stream\n>>>\n>>> @@ -62,8 +60,7 @@ class CameraCaptureContext:\n>>> req = cam.create_request(idx)\n>>>\n>>> buffer = allocator.buffers(stream)[i]\n>>> - ret = req.add_buffer(stream, buffer)\n>>> - assert ret == 0\n>>> + req.add_buffer(stream, buffer)\n>>>\n>>> self.reqs.append(req)\n>>>\n>>> @@ -73,13 +70,11 @@ class CameraCaptureContext:\n>>> def uninit_camera(self):\n>>> # Stop the camera\n>>>\n>>> - ret = self.cam.stop()\n>>> - assert ret == 0\n>>> + self.cam.stop()\n>>>\n>>> # Release the camera\n>>>\n>>> - ret = self.cam.release()\n>>> - assert ret == 0\n>>> + self.cam.release()\n>>>\n>>>\n>>> # A container class for our state\n>>> @@ -145,8 +140,7 @@ class CaptureContext:\n>>>\n>>> for cam_ctx in self.camera_contexts:\n>>> for req in cam_ctx.reqs:\n>>> - ret = cam_ctx.cam.queue_request(req)\n>>> - assert ret == 0\n>>> + cam_ctx.cam.queue_request(req)\n>>>\n>>> # Use Selector to wait for events from the camera and from the keyboard\n>>>\n>>> @@ -177,8 +171,7 @@ def main():\n>>> # Start the cameras\n>>>\n>>> for cam_ctx in ctx.camera_contexts:\n>>> - ret = cam_ctx.cam.start()\n>>> - assert ret == 0\n>>> + cam_ctx.cam.start()\n>>>\n>>> ctx.capture()\n>>>\n>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp\n>>> index e7d078b5..6cd99919 100644\n>>> --- a/src/py/libcamera/py_main.cpp\n>>> +++ b/src/py/libcamera/py_main.cpp\n>>> @@ -111,8 +111,19 @@ PYBIND11_MODULE(_libcamera, m)\n>>>\n>>> \tpyCamera\n>>> \t\t.def_property_readonly(\"id\", &Camera::id)\n>>> -\t\t.def(\"acquire\", &Camera::acquire)\n>>> -\t\t.def(\"release\", &Camera::release)\n>>> +\t\t.def(\"acquire\", [](Camera &self) {\n>>> +\t\t\tint ret = self.acquire();\n>>> +\t\t\tif (ret)\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to acquire camera\");\n>>> +\t\t})\n>>> +\t\t.def(\"release\", [](Camera &self) {\n>>> +\t\t\tint ret = self.release();\n>>> +\t\t\t/* \\todo Should we ignore the error? */\n> \n> Should we ? It indicates an error in the caller, right ?\n\nI find errors in stop/release/free style functions a bit difficult to \nunderstand. What does it mean? What is the caller supposed to do? Always \nignore? Or always abort()?\n\nHowever, here, in the bindings, I think it makes sense to throw the \nexception if the C++ API gives an error, whether the C++ API's behavior \nmakes sense or not. We're trying to expose the C++ API as directly as \npossible.\n\nSo I'll drop these todo comments.\n\n>>> +\t\t\tif (ret)\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to release camera\");\n>>> +\t\t})\n>>> \t\t.def(\"start\", [](Camera &self,\n>>> \t\t const std::unordered_map &controls) {\n>>> \t\t\t/* \\todo What happens if someone calls start() multiple times? */\n>>> @@ -132,20 +143,20 @@ PYBIND11_MODULE(_libcamera, m)\n>>> \t\t\tint ret = self.start(&controlList);\n>>> \t\t\tif (ret) {\n>>> \t\t\t\tself.requestCompleted.disconnect();\n>>> -\t\t\t\treturn ret;\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to start camera\");\n>>> \t\t\t}\n>>> -\n>>> -\t\t\treturn 0;\n>>> \t\t}, py::arg(\"controls\") = std::unordered_map())\n>>>\n>>> \t\t.def(\"stop\", [](Camera &self) {\n>>> \t\t\tint ret = self.stop();\n>>> -\t\t\tif (ret)\n>>> -\t\t\t\treturn ret;\n>>>\n>>> \t\t\tself.requestCompleted.disconnect();\n>>>\n>>> -\t\t\treturn 0;\n>>> +\t\t\t/* \\todo Should we just ignore the error? */\n>>\n>> There previously was an assertion, so I think it's fine to keep\n>> checking for errors.\n>>\n>>> +\t\t\tif (ret)\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to start camera\");\n>>\n>> s/start/stop\n>>\n>> Reviewed-by: Jacopo Mondi \n>>\n>>> \t\t})\n>>>\n>>> \t\t.def(\"__str__\", [](Camera &self) {\n>>> @@ -154,9 +165,20 @@ PYBIND11_MODULE(_libcamera, m)\n>>>\n>>> \t\t/* Keep the camera alive, as StreamConfiguration contains a Stream* */\n>>> \t\t.def(\"generate_configuration\", &Camera::generateConfiguration, py::keep_alive<0, 1>())\n>>> -\t\t.def(\"configure\", &Camera::configure)\n>>> +\t\t.def(\"configure\", [](Camera &self, CameraConfiguration *config) {\n>>> +\t\t\tint ret = self.configure(config);\n>>> +\t\t\tif (ret)\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to configure camera\");\n>>> +\t\t})\n>>>\n>>> -\t\t.def(\"create_request\", &Camera::createRequest, py::arg(\"cookie\") = 0)\n>>> +\t\t.def(\"create_request\", [](Camera &self, uint64_t cookie) {\n>>> +\t\t\tstd::unique_ptr req = self.createRequest(cookie);\n>>> +\t\t\tif (!req)\n>>> +\t\t\t\tthrow std::system_error(ENOMEM, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to create request\");\n>>> +\t\t\treturn req;\n>>> +\t\t}, py::arg(\"cookie\") = 0)\n>>>\n>>> \t\t.def(\"queue_request\", [](Camera &self, Request *req) {\n>>> \t\t\tpy::object py_req = py::cast(req);\n>>> @@ -169,10 +191,11 @@ PYBIND11_MODULE(_libcamera, m)\n>>> \t\t\tpy_req.inc_ref();\n>>>\n>>> \t\t\tint ret = self.queueRequest(req);\n>>> -\t\t\tif (ret)\n>>> +\t\t\tif (ret) {\n>>> \t\t\t\tpy_req.dec_ref();\n>>> -\n>>> -\t\t\treturn ret;\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to queue request\");\n>>> +\t\t\t}\n>>> \t\t})\n>>>\n>>> \t\t.def_property_readonly(\"streams\", [](Camera &self) {\n>>> @@ -250,7 +273,13 @@ PYBIND11_MODULE(_libcamera, m)\n>>>\n>>> \tpyFrameBufferAllocator\n>>> \t\t.def(py::init>(), py::keep_alive<1, 2>())\n>>> -\t\t.def(\"allocate\", &FrameBufferAllocator::allocate)\n>>> +\t\t.def(\"allocate\", [](FrameBufferAllocator &self, Stream *stream) {\n>>> +\t\t\tint ret = self.allocate(stream);\n>>> +\t\t\tif (ret < 0)\n> \n> Shouldn't 0 be also considered an error ?\n\nThe function documentation says \"The number of allocated buffers on \nsuccess or a negative error code otherwise\". So 0 doesn't seem to be an \nerror.\n\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to queue request\");\n> \n> Bad copy & paste.\n\nIndeed.\n\n>>> +\t\t\treturn ret;\n>>> +\t\t})\n>>> \t\t.def_property_readonly(\"allocated\", &FrameBufferAllocator::allocated)\n>>> \t\t/* Create a list of FrameBuffers, where each FrameBuffer has a keep-alive to FrameBufferAllocator */\n>>> \t\t.def(\"buffers\", [](FrameBufferAllocator &self, Stream *stream) {\n>>> @@ -327,7 +356,10 @@ PYBIND11_MODULE(_libcamera, m)\n>>> \tpyRequest\n>>> \t\t/* \\todo Fence is not supported, so we cannot expose addBuffer() directly */\n>>> \t\t.def(\"add_buffer\", [](Request &self, const Stream *stream, FrameBuffer *buffer) {\n>>> -\t\t\treturn self.addBuffer(stream, buffer);\n>>> +\t\t\tint ret = self.addBuffer(stream, buffer);\n> \n> Extra space after =\n\nYep.\n\n>>> +\t\t\tif (ret)\n>>> +\t\t\t\tthrow std::system_error(-ret, std::generic_category(),\n>>> +\t\t\t\t\t\t\t\"Failed to add buffer\");\n>>> \t\t}, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */\n>>> \t\t.def_property_readonly(\"status\", &Request::status)\n>>> \t\t.def_property_readonly(\"buffers\", &Request::buffers)\n>>> diff --git a/test/py/unittests.py b/test/py/unittests.py\n>>> index 6dea80fc..794e46be 100755\n>>> --- a/test/py/unittests.py\n>>> +++ b/test/py/unittests.py\n>>> @@ -42,31 +42,26 @@ class SimpleTestMethods(BaseTestCase):\n>>> cam = cm.get('platform/vimc.0 Sensor B')\n>>> self.assertIsNotNone(cam)\n>>>\n>>> - ret = cam.acquire()\n>>> - self.assertZero(ret)\n>>> + cam.acquire()\n> \n> Will the test framework produce an error report that provide as much\n> debugging information as the assert functions ?\n\nYes. Although those two cases are slightly different: a failing \ntest-assert results in test failure, and an exception results in test \nerror. The end result is the same.\n\n Tomi","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 90C56BE173\n\tfor ;\n\tFri, 19 Aug 2022 06:51:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 00AAE61FC0;\n\tFri, 19 Aug 2022 08:51:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 538AE603E2\n\tfor ;\n\tFri, 19 Aug 2022 08:51:45 +0200 (CEST)","from [192.168.1.111] (91-158-154-79.elisa-laajakaista.fi\n\t[91.158.154.79])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7486A3F1;\n\tFri, 19 Aug 2022 08:51:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660891907;\n\tbh=zQUm8NAHYzEpBI7ehQaAlhpALYm6tB+1iqCzTmhzvXE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=zlF7z8a2q7aX5Rus2lm9pmp5Y5oeHLr7SOIFCMQ8s/MHc3VD12pjtgVVgSQrViLRW\n\tvKS9D8L92lQB1MgH6bhcIBgxNFR5MRRzkjHaNK7RS4I6KMZqO0Z+T4jJ+c586BzZsR\n\tDBIK3LchI+89FC9Nk01ErJlmEUJ+naN9LjiazxTC47cJANEN0XApIEx9wJQRpp8gM9\n\tzXCMUKH7bwrL43HBpFRiMfdT1tTa2KKg7pYb/YPRf3VTlLS8MvIQugOETVd+bqsQmW\n\to6O8W2joBRGQrW7+housZ5cohB+rxNsW3rHnJiI9wjoHYBwkpn5cF8gf7fe40B0et0\n\tJDYgtGexk9ObA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1660891904;\n\tbh=zQUm8NAHYzEpBI7ehQaAlhpALYm6tB+1iqCzTmhzvXE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VKnqxw5MHS6caMvbRYXD6lxnkbQ+v+mVXprgnzcri3uLOs+6eqrTVQeejqUe1MswC\n\tcB4BF0YpecvUzDvuMXMdGdLG2r+y6WzQLpdPdpKzVNns+N+9RgsQQJKB7xWLXQKCec\n\tagnr4h1J/a/CsVsgJy5Lowfr8CcrNrTk0IUtfwM8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VKnqxw5M\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 19 Aug 2022 09:51:41 +0300","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.11.0","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tJacopo Mondi ","References":"<20220701084521.31831-1-tomi.valkeinen@ideasonboard.com>\n\t<20220701084521.31831-11-tomi.valkeinen@ideasonboard.com>\n\t<20220818143606.y6nti4hnl2j67ru7@uno.localdomain>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3 10/17] py: Use exceptions instead\n\tof returning error codes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Tomi Valkeinen via libcamera-devel\n\t","Reply-To":"Tomi Valkeinen ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]