[{"id":11815,"web_url":"https://patchwork.libcamera.org/comment/11815/","msgid":"<20200804100617.zz4nbelt2llrmln5@uno.localdomain>","date":"2020-08-04T10:06:17","subject":"Re: [libcamera-devel] [PATCH v6 5/9] libcamera: camera_sensor:\n\tGenerate a sensor ID","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n if I were to be picky, this patch actually contains two things:\n- id generation\n- accessor\n\nI don't mind too much though\n\nOn Mon, Aug 03, 2020 at 11:17:29PM +0200, Niklas Söderlund wrote:\n> Generate a constant and unique string ID for the sensor. The ID is\n> generated from information from the firmware description of the camera\n> image sensor. The ID is unique and persistent across reboots of the\n> system and may be used as a camera ID.\n\nI know what 'camera ID' is, but what about\n'may be used to identify the camera uniquely in the system'\n>\n> The CameraSensor is intended to be sub-classed for specific sensor\n> modules in the future to enable more advanced use-cases. Controlling the\n\nAhem, yes, no, maybe...\nI presume this will happen sooner or later, but since we trashed the\ncamera sensor factory we have been working to be able to standardize\nthings as much as we could to use a single camera sensor class.\n\n> ID is one such feature that is already needed by the VIMC sensor. As the\n> VIMC sensor is a virtual pipeline its sensor is not described in the\n> device firmware and a subclass of CameraSensor is needed to generate a\n> ID for VIMC sensors.\n\nI understand the issue, and actually sub-classing CameraSensor in the\npipeline handler seems acceptable to me, as it does not go in the\ndirection of having to define a derived class which depends on the\nsensor model, as we did with the CameraSensorFactory.\n\nBut now I wonder, if pipeline handlers sub-class CameraSensor, they\ncannot do that to augment it with information specific to a camera\nsensor, unless we assume some pipelines -know- they will work with a\nfixed set of sensors and implement a way to identify them at run-time\nand instantiate the right derived class, which seems a custom solution\nI would prefer not to see in mainline libcamera to be honest.\n\nWhat remains is that pipeline handlers, being unable to extend\nCameraSensor class with sensor-specific information, will have to\ndefine a derived class for... well, right now for the ID only :)\n\nI have not looked at the VIMC pipeline handler patch yet, but I wonder\nif that we should not avoid making it extend the camera sensor class\nand compute the camera ID without involving it at all.\n\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v5\n> - Use new utils:: helper.\n> - Allow for subclassing\n>\n> * Changes since v4\n> - Fix spelling.\n>\n> * Changes since v3\n> - Update commit message.\n> - Add description of how ID are generated to comment.\n> ---\n> include/libcamera/internal/camera_sensor.h | 3 ++\n> src/libcamera/camera_sensor.cpp | 40 +++++++++++++++++++\n> src/libcamera/pipeline/vimc/vimc.cpp | 19 ++++++++-\n> test/camera-sensor.cpp | 3 +-\n> test/libtest/vimc_sensor_test.h | 29 ++++++++++++++\n> .../v4l2_videodevice_test.cpp | 4 +-\n> 6 files changed, 94 insertions(+), 4 deletions(-)\n> create mode 100644 test/libtest/vimc_sensor_test.h\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index 06c8292ca30129de..b3aaaeaf829d97c0 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -47,6 +47,7 @@ public:\n> \tint init();\n>\n> \tconst std::string &model() const { return model_; }\n> +\tconst std::string &id() const { return id_; }\n> \tconst MediaEntity *entity() const { return entity_; }\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tconst std::vector &sizes() const { return sizes_; }\n> @@ -65,6 +66,7 @@ public:\n>\n> protected:\n> \tstd::string logPrefix() const override;\n> +\tvirtual std::string generateID() const;\n>\n> private:\n> \tconst MediaEntity *entity_;\n> @@ -72,6 +74,7 @@ private:\n> \tunsigned int pad_;\n>\n> \tstd::string model_;\n> +\tstd::string id_;\n>\n> \tV4L2Subdevice::Formats formats_;\n> \tSize resolution_;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 350f49accad99c7b..282cedecaf28fb3c 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -204,6 +204,11 @@ int CameraSensor::init()\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> +\t/* Generate a unique ID for the sensor. */\n> +\tid_ = generateID();\n> +\tif (id_.empty())\n> +\t\treturn -EINVAL;\n> +\n> \t/* Retrieve and store the camera sensor properties. */\n> \tconst ControlInfoMap &controls = subdev_->controls();\n> \tint32_t propertyValue;\n> @@ -283,6 +288,16 @@ int CameraSensor::init()\n> * \\return The sensor model name\n> */\n>\n> +/**\n> + * \\fn CameraSensor::id()\n> + * \\brief Retrieve the sensor ID\n> + *\n> + * The sensor ID is a free-form string that uniquely identifies the sensor in\n> + * the system. The ID satisfy the requirements to be used as a camera ID.\n> + *\n> + * \\return The sensor ID\n> + */\n> +\n> /**\n> * \\fn CameraSensor::entity()\n> * \\brief Retrieve the sensor media entity\n> @@ -541,4 +556,29 @@ std::string CameraSensor::logPrefix() const\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n>\n> +/**\n> + * \\brief Generate a sensor ID for the sensor\n> + *\n> + * The generated string satisfy the requirements to be used as ID for a camera.\n\nWhat are those requirments, one could wonder when reading this comment\nwithout the background provided by this series ?\n\nI would, even if I understand it might be 'boring', repeat them here.\nThey're just three after all\n\n> + * This default implementation retrieves the ID form the firmware ID associated\n> + * with the sensors V4L2 subdevice, that meets this criteria.\n> + *\n> + * It is possible to override this function to allow for more advanced IDs to be\n> + * generated for specific sensors. Any alternative implementation is responsible\n> + * for that the generated ID satisfy the requirements to be used as a camera ID.\n> + *\n\nSee above on the part\n\n> + * \\sa id()\n> + * \\return Sensor ID on success or empty string on failure\n\nA string representing the sensor ID, or an empty string on failure\n\n?\n\n> + */\n> +std::string CameraSensor::generateID() const\n> +{\n> +\tstd::string id;\n> +\tif (utils::tryLookupFirmwareID(subdev_->devicePath(), &id)) {\n> +\t\tLOG(CameraSensor, Error) << \"Can't get sensor ID from firmware\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn id;\n> +}\n\nLong time debated issue: this is a private function which returns a string\nwhich is then assigned to a class private field.\nI wonder if:\n1) it could inlined in the caller. it's 3 lines of code\n2) if not, shouldn't it assign id_ here and return an error code ?\n\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 4f461b928514022d..b3348ab5d987d506 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -38,6 +38,21 @@ namespace libcamera {\n>\n> LOG_DEFINE_CATEGORY(VIMC)\n>\n> +class VimcCameraSensor : public CameraSensor\n> +{\n> +public:\n> +\tVimcCameraSensor(const MediaEntity *entity)\n> +\t\t: CameraSensor(entity)\n> +\t{\n> +\t}\n> +\n> +protected:\n> +\tstd::string generateID() const override\n> +\t{\n> +\t\treturn \"VIMC \" + entity()->name();\n> +\t}\n> +};\n> +\n\nAh here it is.\n\nIn my opinion, this is not necessary, and it could be done in the\npipeline handler by accessing the CameraSensor entity name.\n\nI think the fact you need to declare a fake CameraSensor derived class\nfor tests is a sign this is not the best direction to take ?\n\nThanks\n j\n\n> class VimcCameraData : public CameraData\n> {\n> public:\n> @@ -61,7 +76,7 @@ public:\n> \tvoid bufferReady(FrameBuffer *buffer);\n>\n> \tMediaDevice *media_;\n> -\tCameraSensor *sensor_;\n> +\tVimcCameraSensor *sensor_;\n> \tV4L2Subdevice *debayer_;\n> \tV4L2Subdevice *scaler_;\n> \tV4L2VideoDevice *video_;\n> @@ -457,7 +472,7 @@ int VimcCameraData::init()\n> \t\treturn ret;\n>\n> \t/* Create and open the camera sensor, debayer, scaler and video device. */\n> -\tsensor_ = new CameraSensor(media_->getEntityByName(\"Sensor B\"));\n> +\tsensor_ = new VimcCameraSensor(media_->getEntityByName(\"Sensor B\"));\n> \tret = sensor_->init();\n> \tif (ret)\n> \t\treturn ret;\n> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> index 8c7fd1d2d4445db3..50157b234fd40933 100644\n> --- a/test/camera-sensor.cpp\n> +++ b/test/camera-sensor.cpp\n> @@ -17,6 +17,7 @@\n> #include \"libcamera/internal/v4l2_subdevice.h\"\n>\n> #include \"test.h\"\n> +#include \"vimc_sensor_test.h\"\n>\n> using namespace std;\n> using namespace libcamera;\n> @@ -50,7 +51,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tsensor_ = new CameraSensor(entity);\n> +\t\tsensor_ = new TestVimcCameraSensor(entity);\n> \t\tif (sensor_->init() < 0) {\n> \t\t\tcerr << \"Unable to initialise camera sensor\" << endl;\n> \t\t\treturn TestFail;\n> diff --git a/test/libtest/vimc_sensor_test.h b/test/libtest/vimc_sensor_test.h\n> new file mode 100644\n> index 0000000000000000..1454a1c310f0dc9c\n> --- /dev/null\n> +++ b/test/libtest/vimc_sensor_test.h\n> @@ -0,0 +1,29 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * vimc_sensor_test.h - vimc_sensor test class\n> + */\n> +#ifndef __LIBCAMERA_VIMC_SENSOR_TEST_H__\n> +#define __LIBCAMERA_VIMC_SENSOR_TEST_H__\n> +\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class TestVimcCameraSensor : public CameraSensor\n> +{\n> +public:\n> +\tTestVimcCameraSensor(const MediaEntity *entity)\n> +\t\t: CameraSensor(entity)\n> +\t{\n> +\t}\n> +\n> +protected:\n> +\tstd::string generateID() const override\n> +\t{\n> +\t\treturn \"VIMC \" + entity()->name();\n> +\t}\n> +};\n> +\n> +#endif /* __LIBCAMERA_VIMC_SENSOR_TEST_H__ */\n> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> index f23aaf8f514bc050..c7a3479963be9b78 100644\n> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp\n> @@ -12,6 +12,8 @@\n> #include \"libcamera/internal/device_enumerator.h\"\n> #include \"libcamera/internal/media_device.h\"\n>\n> +#include \"vimc_sensor_test.h\"\n> +\n> #include \"v4l2_videodevice_test.h\"\n>\n> using namespace std;\n> @@ -61,7 +63,7 @@ int V4L2VideoDeviceTest::init()\n> \t\treturn TestFail;\n>\n> \tif (driver_ == \"vimc\") {\n> -\t\tsensor_ = new CameraSensor(media_->getEntityByName(\"Sensor A\"));\n> +\t\tsensor_ = new TestVimcCameraSensor(media_->getEntityByName(\"Sensor A\"));\n> \t\tif (sensor_->init())\n> \t\t\treturn TestFail;\n>\n> --\n> 2.28.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 5E3A8BD87A\n\tfor ;\n\tTue, 4 Aug 2020 10:02:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 096256043C;\n\tTue, 4 Aug 2020 12:02:38 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C828603C9\n\tfor ;\n\tTue, 4 Aug 2020 12:02:36 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id E4F5C1BF211;\n\tTue, 4 Aug 2020 10:02:35 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 4 Aug 2020 12:06:17 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200804100617.zz4nbelt2llrmln5@uno.localdomain>","References":"<20200803211733.1037019-1-niklas.soderlund@ragnatech.se>\n\t<20200803211733.1037019-6-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200803211733.1037019-6-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v6 5/9] libcamera: camera_sensor:\n\tGenerate a sensor ID","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]