[{"id":28832,"web_url":"https://patchwork.libcamera.org/comment/28832/","msgid":"","date":"2024-03-04T17:37:15","subject":"Re: [PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Fri, Mar 01, 2024 at 11:21:05PM +0200, Laurent Pinchart wrote:\n> The CameraSensor class has grown a lot since its creation, with many\n> functions added for different types of purposes. They are not grouped by\n> categories in the class definition, generating confusion when reading\n> the header file. Improve readability by sorting functions by category:\n>\n> - Getters for static data (model, entity, focus lens, ...)\n> - Format and sensor configuration accessors\n> - Properties and controls (including test pattern mode)\n>\n> Update the .cpp file accordingly.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/internal/camera_sensor.h | 29 +-\n> src/libcamera/sensor/camera_sensor.cpp | 328 ++++++++++-----------\n> 2 files changed, 179 insertions(+), 178 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index b2f077b9cc75..750d6d729cac 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -46,15 +46,15 @@ public:\n>\n> \tconst std::string &model() const { return model_; }\n> \tconst std::string &id() const { return id_; }\n> +\n> \tconst MediaEntity *entity() const { return entity_; }\n> +\tV4L2Subdevice *device() { return subdev_.get(); }\n> +\n> +\tCameraLens *focusLens() { return focusLens_.get(); }\n> +\n\nShould you move\n const ControlList &properties() const { return properties_; }\n const ControlInfoMap &controls() const;\n\nhere ?\n\n(I understand if you want to keep them grouped though)\n\nThis can be fast-tracked too!\n\nReviewed-by: Jacopo Mondi \n\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tstd::vector sizes(unsigned int mbusCode) const;\n> \tSize resolution() const;\n> -\tconst std::vector &testPatternModes() const\n> -\t{\n> -\t\treturn testPatternModes_;\n> -\t}\n> -\tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n>\n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -66,18 +66,19 @@ public:\n> \t\t\t Transform transform = Transform::Identity,\n> \t\t\t V4L2SubdeviceFormat *sensorFormat = nullptr);\n>\n> +\tconst ControlList &properties() const { return properties_; }\n> +\tint sensorInfo(IPACameraSensorInfo *info) const;\n> +\tTransform computeTransform(Orientation *orientation) const;\n> +\n> \tconst ControlInfoMap &controls() const;\n> \tControlList getControls(const std::vector &ids);\n> \tint setControls(ControlList *ctrls);\n>\n> -\tV4L2Subdevice *device() { return subdev_.get(); }\n> -\n> -\tconst ControlList &properties() const { return properties_; }\n> -\tint sensorInfo(IPACameraSensorInfo *info) const;\n> -\n> -\tCameraLens *focusLens() { return focusLens_.get(); }\n> -\n> -\tTransform computeTransform(Orientation *orientation) const;\n> +\tconst std::vector &testPatternModes() const\n> +\t{\n> +\t\treturn testPatternModes_;\n> +\t}\n> +\tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n>\n> protected:\n> \tstd::string logPrefix() const override;\n> @@ -91,8 +92,8 @@ private:\n> \tvoid initStaticProperties();\n> \tvoid initTestPatternModes();\n> \tint initProperties();\n> -\tint applyTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \tint discoverAncillaryDevices();\n> +\tint applyTestPatternMode(controls::draft::TestPatternModeEnum mode);\n>\n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 545f89d036df..86ad9a85371c 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -215,6 +215,30 @@ int CameraSensor::init()\n> \treturn applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> }\n>\n> +int CameraSensor::generateId()\n> +{\n> +\tconst std::string devPath = subdev_->devicePath();\n> +\n> +\t/* Try to get ID from firmware description. */\n> +\tid_ = sysfs::firmwareNodePath(devPath);\n> +\tif (!id_.empty())\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * Virtual sensors not described in firmware\n> +\t *\n> +\t * Verify it's a platform device and construct ID from the device path\n> +\t * and model of sensor.\n> +\t */\n> +\tif (devPath.find(\"/sys/devices/platform/\", 0) == 0) {\n> +\t\tid_ = devPath.substr(strlen(\"/sys/devices/\")) + \" \" + model();\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tLOG(CameraSensor, Error) << \"Can't generate sensor ID\";\n> +\treturn -EINVAL;\n> +}\n> +\n> int CameraSensor::validateSensorDriver()\n> {\n> \tint err = 0;\n> @@ -580,6 +604,21 @@ int CameraSensor::discoverAncillaryDevices()\n> * \\return The sensor media entity\n> */\n>\n> +/**\n> + * \\fn CameraSensor::device()\n> + * \\brief Retrieve the camera sensor device\n> + * \\todo Remove this function by integrating DelayedControl with CameraSensor\n> + * \\return The camera sensor device\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::focusLens()\n> + * \\brief Retrieve the focus lens controller\n> + *\n> + * \\return The focus lens controller. nullptr if no focus lens controller is\n> + * connected to the sensor\n> + */\n> +\n> /**\n> * \\fn CameraSensor::mbusCodes()\n> * \\brief Retrieve the media bus codes supported by the camera sensor\n> @@ -632,64 +671,6 @@ Size CameraSensor::resolution() const\n> \treturn std::min(sizes_.back(), activeArea_.size());\n> }\n>\n> -/**\n> - * \\fn CameraSensor::testPatternModes()\n> - * \\brief Retrieve all the supported test pattern modes of the camera sensor\n> - * The test pattern mode values correspond to the controls::TestPattern control.\n> - *\n> - * \\return The list of test pattern modes\n> - */\n> -\n> -/**\n> - * \\brief Set the test pattern mode for the camera sensor\n> - * \\param[in] mode The test pattern mode\n> - *\n> - * The new \\a mode is applied to the sensor if it differs from the active test\n> - * pattern mode. Otherwise, this function is a no-op. Setting the same test\n> - * pattern mode for every frame thus incurs no performance penalty.\n> - */\n> -int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> -{\n> -\tif (testPatternMode_ == mode)\n> -\t\treturn 0;\n> -\n> -\tif (testPatternModes_.empty()) {\n> -\t\tLOG(CameraSensor, Error)\n> -\t\t\t<< \"Camera sensor does not support test pattern modes.\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\treturn applyTestPatternMode(mode);\n> -}\n> -\n> -int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> -{\n> -\tif (testPatternModes_.empty())\n> -\t\treturn 0;\n> -\n> -\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> -\t\t\t mode);\n> -\tif (it == testPatternModes_.end()) {\n> -\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> -\t\t\t\t\t << mode;\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tLOG(CameraSensor, Debug) << \"Apply test pattern mode \" << mode;\n> -\n> -\tint32_t index = staticProps_->testPatternModes.at(mode);\n> -\tControlList ctrls{ controls() };\n> -\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> -\n> -\tint ret = setControls(&ctrls);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\ttestPatternMode_ = mode;\n> -\n> -\treturn 0;\n> -}\n> -\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> @@ -919,80 +900,6 @@ int CameraSensor::applyConfiguration(const SensorConfiguration &config,\n> \treturn 0;\n> }\n>\n> -/**\n> - * \\brief Retrieve the supported V4L2 controls and their information\n> - *\n> - * Control information is updated automatically to reflect the current sensor\n> - * configuration when the setFormat() function is called, without invalidating\n> - * any iterator on the ControlInfoMap.\n> - *\n> - * \\return A map of the V4L2 controls supported by the sensor\n> - */\n> -const ControlInfoMap &CameraSensor::controls() const\n> -{\n> -\treturn subdev_->controls();\n> -}\n> -\n> -/**\n> - * \\brief Read V4L2 controls from the sensor\n> - * \\param[in] ids The list of controls to read, specified by their ID\n> - *\n> - * This function reads the value of all controls contained in \\a ids, and\n> - * returns their values as a ControlList. The control identifiers are defined by\n> - * the V4L2 specification (V4L2_CID_*).\n> - *\n> - * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> - * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs\n> - * during validation of the requested controls, no control is read and this\n> - * function returns an empty control list.\n> - *\n> - * \\sa V4L2Device::getControls()\n> - *\n> - * \\return The control values in a ControlList on success, or an empty list on\n> - * error\n> - */\n> -ControlList CameraSensor::getControls(const std::vector &ids)\n> -{\n> -\treturn subdev_->getControls(ids);\n> -}\n> -\n> -/**\n> - * \\brief Write V4L2 controls to the sensor\n> - * \\param[in] ctrls The list of controls to write\n> - *\n> - * This function writes the value of all controls contained in \\a ctrls, and\n> - * stores the values actually applied to the device in the corresponding \\a\n> - * ctrls entry. The control identifiers are defined by the V4L2 specification\n> - * (V4L2_CID_*).\n> - *\n> - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> - * error occurs during validation of the requested controls, no control is\n> - * written and this function returns -EINVAL.\n> - *\n> - * If an error occurs while writing the controls, the index of the first\n> - * control that couldn't be written is returned. All controls below that index\n> - * are written and their values are updated in \\a ctrls, while all other\n> - * controls are not written and their values are not changed.\n> - *\n> - * \\sa V4L2Device::setControls()\n> - *\n> - * \\return 0 on success or an error code otherwise\n> - * \\retval -EINVAL One of the control is not supported or not accessible\n> - * \\retval i The index of the control that failed\n> - */\n> -int CameraSensor::setControls(ControlList *ctrls)\n> -{\n> -\treturn subdev_->setControls(ctrls);\n> -}\n> -\n> -/**\n> - * \\fn CameraSensor::device()\n> - * \\brief Retrieve the camera sensor device\n> - * \\todo Remove this function by integrating DelayedControl with CameraSensor\n> - * \\return The camera sensor device\n> - */\n> -\n> /**\n> * \\fn CameraSensor::properties()\n> * \\brief Retrieve the camera sensor properties\n> @@ -1088,14 +995,6 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const\n> \treturn 0;\n> }\n>\n> -/**\n> - * \\fn CameraSensor::focusLens()\n> - * \\brief Retrieve the focus lens controller\n> - *\n> - * \\return The focus lens controller. nullptr if no focus lens controller is\n> - * connected to the sensor\n> - */\n> -\n> /**\n> * \\brief Compute the Transform that gives the requested \\a orientation\n> * \\param[inout] orientation The desired image orientation\n> @@ -1155,33 +1054,134 @@ Transform CameraSensor::computeTransform(Orientation *orientation) const\n> \treturn transform;\n> }\n>\n> +/**\n> + * \\brief Retrieve the supported V4L2 controls and their information\n> + *\n> + * Control information is updated automatically to reflect the current sensor\n> + * configuration when the setFormat() function is called, without invalidating\n> + * any iterator on the ControlInfoMap.\n> + *\n> + * \\return A map of the V4L2 controls supported by the sensor\n> + */\n> +const ControlInfoMap &CameraSensor::controls() const\n> +{\n> +\treturn subdev_->controls();\n> +}\n> +\n> +/**\n> + * \\brief Read V4L2 controls from the sensor\n> + * \\param[in] ids The list of controls to read, specified by their ID\n> + *\n> + * This function reads the value of all controls contained in \\a ids, and\n> + * returns their values as a ControlList. The control identifiers are defined by\n> + * the V4L2 specification (V4L2_CID_*).\n> + *\n> + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs\n> + * during validation of the requested controls, no control is read and this\n> + * function returns an empty control list.\n> + *\n> + * \\sa V4L2Device::getControls()\n> + *\n> + * \\return The control values in a ControlList on success, or an empty list on\n> + * error\n> + */\n> +ControlList CameraSensor::getControls(const std::vector &ids)\n> +{\n> +\treturn subdev_->getControls(ids);\n> +}\n> +\n> +/**\n> + * \\brief Write V4L2 controls to the sensor\n> + * \\param[in] ctrls The list of controls to write\n> + *\n> + * This function writes the value of all controls contained in \\a ctrls, and\n> + * stores the values actually applied to the device in the corresponding \\a\n> + * ctrls entry. The control identifiers are defined by the V4L2 specification\n> + * (V4L2_CID_*).\n> + *\n> + * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> + * error occurs during validation of the requested controls, no control is\n> + * written and this function returns -EINVAL.\n> + *\n> + * If an error occurs while writing the controls, the index of the first\n> + * control that couldn't be written is returned. All controls below that index\n> + * are written and their values are updated in \\a ctrls, while all other\n> + * controls are not written and their values are not changed.\n> + *\n> + * \\sa V4L2Device::setControls()\n> + *\n> + * \\return 0 on success or an error code otherwise\n> + * \\retval -EINVAL One of the control is not supported or not accessible\n> + * \\retval i The index of the control that failed\n> + */\n> +int CameraSensor::setControls(ControlList *ctrls)\n> +{\n> +\treturn subdev_->setControls(ctrls);\n> +}\n> +\n> +/**\n> + * \\fn CameraSensor::testPatternModes()\n> + * \\brief Retrieve all the supported test pattern modes of the camera sensor\n> + * The test pattern mode values correspond to the controls::TestPattern control.\n> + *\n> + * \\return The list of test pattern modes\n> + */\n> +\n> +/**\n> + * \\brief Set the test pattern mode for the camera sensor\n> + * \\param[in] mode The test pattern mode\n> + *\n> + * The new \\a mode is applied to the sensor if it differs from the active test\n> + * pattern mode. Otherwise, this function is a no-op. Setting the same test\n> + * pattern mode for every frame thus incurs no performance penalty.\n> + */\n> +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> +{\n> +\tif (testPatternMode_ == mode)\n> +\t\treturn 0;\n> +\n> +\tif (testPatternModes_.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Camera sensor does not support test pattern modes.\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn applyTestPatternMode(mode);\n> +}\n> +\n> +int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> +{\n> +\tif (testPatternModes_.empty())\n> +\t\treturn 0;\n> +\n> +\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> +\t\t\t mode);\n> +\tif (it == testPatternModes_.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> +\t\t\t\t\t << mode;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tLOG(CameraSensor, Debug) << \"Apply test pattern mode \" << mode;\n> +\n> +\tint32_t index = staticProps_->testPatternModes.at(mode);\n> +\tControlList ctrls{ controls() };\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> +\n> +\tint ret = setControls(&ctrls);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttestPatternMode_ = mode;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n>\n> -int CameraSensor::generateId()\n> -{\n> -\tconst std::string devPath = subdev_->devicePath();\n> -\n> -\t/* Try to get ID from firmware description. */\n> -\tid_ = sysfs::firmwareNodePath(devPath);\n> -\tif (!id_.empty())\n> -\t\treturn 0;\n> -\n> -\t/*\n> -\t * Virtual sensors not described in firmware\n> -\t *\n> -\t * Verify it's a platform device and construct ID from the device path\n> -\t * and model of sensor.\n> -\t */\n> -\tif (devPath.find(\"/sys/devices/platform/\", 0) == 0) {\n> -\t\tid_ = devPath.substr(strlen(\"/sys/devices/\")) + \" \" + model();\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tLOG(CameraSensor, Error) << \"Can't generate sensor ID\";\n> -\treturn -EINVAL;\n> -}\n> -\n> } /* namespace libcamera */\n> --\n> Regards,\n>\n> Laurent Pinchart\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 AFC8BBD160\n\tfor ;\n\tMon, 4 Mar 2024 17:37:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C40D262872;\n\tMon, 4 Mar 2024 18:37:20 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C21A627FC\n\tfor ;\n\tMon, 4 Mar 2024 18:37:19 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E0D46BD1;\n\tMon, 4 Mar 2024 18:37:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"k9/Usyyg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709573823;\n\tbh=OjkIjzBS/7CSXHZvtQ3ejNBTZBj8M6J08qkbwzhox8Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=k9/Usyygz24XXowmQ8p7ReuqwgyDUQUJ5/iJB3rnp9jYL9gFvt+34gplcrBuZQLoA\n\tbBvx8i+4UaoMmLa65Lr9N8xdnoBPvBcTcGDXQSCBZY+eTv0Bx/DesrCauHtVRU6n+A\n\tt6EbZyKo69q4m434VfWaGudQFiYBqM3P/bZ5755s=","Date":"Mon, 4 Mar 2024 18:37:15 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Subject":"Re: [PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions","Message-ID":"","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-17-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240301212121.9072-17-laurent.pinchart@ideasonboard.com>","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, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28840,"web_url":"https://patchwork.libcamera.org/comment/28840/","msgid":"<20240304224808.GF9233@pendragon.ideasonboard.com>","date":"2024-03-04T22:48:08","subject":"Re: [PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 04, 2024 at 06:37:15PM +0100, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Fri, Mar 01, 2024 at 11:21:05PM +0200, Laurent Pinchart wrote:\n> > The CameraSensor class has grown a lot since its creation, with many\n> > functions added for different types of purposes. They are not grouped by\n> > categories in the class definition, generating confusion when reading\n> > the header file. Improve readability by sorting functions by category:\n> >\n> > - Getters for static data (model, entity, focus lens, ...)\n> > - Format and sensor configuration accessors\n> > - Properties and controls (including test pattern mode)\n> >\n> > Update the .cpp file accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/internal/camera_sensor.h | 29 +-\n> > src/libcamera/sensor/camera_sensor.cpp | 328 ++++++++++-----------\n> > 2 files changed, 179 insertions(+), 178 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> > index b2f077b9cc75..750d6d729cac 100644\n> > --- a/include/libcamera/internal/camera_sensor.h\n> > +++ b/include/libcamera/internal/camera_sensor.h\n> > @@ -46,15 +46,15 @@ public:\n> >\n> > \tconst std::string &model() const { return model_; }\n> > \tconst std::string &id() const { return id_; }\n> > +\n> > \tconst MediaEntity *entity() const { return entity_; }\n> > +\tV4L2Subdevice *device() { return subdev_.get(); }\n> > +\n> > +\tCameraLens *focusLens() { return focusLens_.get(); }\n> > +\n> \n> Should you move\n> const ControlList &properties() const { return properties_; }\n> const ControlInfoMap &controls() const;\n> \n> here ?\n> \n> (I understand if you want to keep them grouped though)\n\nI specifically decided to put the properties just before the controls\n:-)\n\n> This can be fast-tracked too!\n> \n> Reviewed-by: Jacopo Mondi \n> \n> > \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> > \tstd::vector sizes(unsigned int mbusCode) const;\n> > \tSize resolution() const;\n> > -\tconst std::vector &testPatternModes() const\n> > -\t{\n> > -\t\treturn testPatternModes_;\n> > -\t}\n> > -\tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> >\n> > \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> > \t\t\t\t const Size &size) const;\n> > @@ -66,18 +66,19 @@ public:\n> > \t\t\t Transform transform = Transform::Identity,\n> > \t\t\t V4L2SubdeviceFormat *sensorFormat = nullptr);\n> >\n> > +\tconst ControlList &properties() const { return properties_; }\n> > +\tint sensorInfo(IPACameraSensorInfo *info) const;\n> > +\tTransform computeTransform(Orientation *orientation) const;\n> > +\n> > \tconst ControlInfoMap &controls() const;\n> > \tControlList getControls(const std::vector &ids);\n> > \tint setControls(ControlList *ctrls);\n> >\n> > -\tV4L2Subdevice *device() { return subdev_.get(); }\n> > -\n> > -\tconst ControlList &properties() const { return properties_; }\n> > -\tint sensorInfo(IPACameraSensorInfo *info) const;\n> > -\n> > -\tCameraLens *focusLens() { return focusLens_.get(); }\n> > -\n> > -\tTransform computeTransform(Orientation *orientation) const;\n> > +\tconst std::vector &testPatternModes() const\n> > +\t{\n> > +\t\treturn testPatternModes_;\n> > +\t}\n> > +\tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> >\n> > protected:\n> > \tstd::string logPrefix() const override;\n> > @@ -91,8 +92,8 @@ private:\n> > \tvoid initStaticProperties();\n> > \tvoid initTestPatternModes();\n> > \tint initProperties();\n> > -\tint applyTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> > \tint discoverAncillaryDevices();\n> > +\tint applyTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> >\n> > \tconst MediaEntity *entity_;\n> > \tstd::unique_ptr subdev_;\n> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> > index 545f89d036df..86ad9a85371c 100644\n> > --- a/src/libcamera/sensor/camera_sensor.cpp\n> > +++ b/src/libcamera/sensor/camera_sensor.cpp\n> > @@ -215,6 +215,30 @@ int CameraSensor::init()\n> > \treturn applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> > }\n> >\n> > +int CameraSensor::generateId()\n> > +{\n> > +\tconst std::string devPath = subdev_->devicePath();\n> > +\n> > +\t/* Try to get ID from firmware description. */\n> > +\tid_ = sysfs::firmwareNodePath(devPath);\n> > +\tif (!id_.empty())\n> > +\t\treturn 0;\n> > +\n> > +\t/*\n> > +\t * Virtual sensors not described in firmware\n> > +\t *\n> > +\t * Verify it's a platform device and construct ID from the device path\n> > +\t * and model of sensor.\n> > +\t */\n> > +\tif (devPath.find(\"/sys/devices/platform/\", 0) == 0) {\n> > +\t\tid_ = devPath.substr(strlen(\"/sys/devices/\")) + \" \" + model();\n> > +\t\treturn 0;\n> > +\t}\n> > +\n> > +\tLOG(CameraSensor, Error) << \"Can't generate sensor ID\";\n> > +\treturn -EINVAL;\n> > +}\n> > +\n> > int CameraSensor::validateSensorDriver()\n> > {\n> > \tint err = 0;\n> > @@ -580,6 +604,21 @@ int CameraSensor::discoverAncillaryDevices()\n> > * \\return The sensor media entity\n> > */\n> >\n> > +/**\n> > + * \\fn CameraSensor::device()\n> > + * \\brief Retrieve the camera sensor device\n> > + * \\todo Remove this function by integrating DelayedControl with CameraSensor\n> > + * \\return The camera sensor device\n> > + */\n> > +\n> > +/**\n> > + * \\fn CameraSensor::focusLens()\n> > + * \\brief Retrieve the focus lens controller\n> > + *\n> > + * \\return The focus lens controller. nullptr if no focus lens controller is\n> > + * connected to the sensor\n> > + */\n> > +\n> > /**\n> > * \\fn CameraSensor::mbusCodes()\n> > * \\brief Retrieve the media bus codes supported by the camera sensor\n> > @@ -632,64 +671,6 @@ Size CameraSensor::resolution() const\n> > \treturn std::min(sizes_.back(), activeArea_.size());\n> > }\n> >\n> > -/**\n> > - * \\fn CameraSensor::testPatternModes()\n> > - * \\brief Retrieve all the supported test pattern modes of the camera sensor\n> > - * The test pattern mode values correspond to the controls::TestPattern control.\n> > - *\n> > - * \\return The list of test pattern modes\n> > - */\n> > -\n> > -/**\n> > - * \\brief Set the test pattern mode for the camera sensor\n> > - * \\param[in] mode The test pattern mode\n> > - *\n> > - * The new \\a mode is applied to the sensor if it differs from the active test\n> > - * pattern mode. Otherwise, this function is a no-op. Setting the same test\n> > - * pattern mode for every frame thus incurs no performance penalty.\n> > - */\n> > -int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> > -{\n> > -\tif (testPatternMode_ == mode)\n> > -\t\treturn 0;\n> > -\n> > -\tif (testPatternModes_.empty()) {\n> > -\t\tLOG(CameraSensor, Error)\n> > -\t\t\t<< \"Camera sensor does not support test pattern modes.\";\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> > -\treturn applyTestPatternMode(mode);\n> > -}\n> > -\n> > -int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> > -{\n> > -\tif (testPatternModes_.empty())\n> > -\t\treturn 0;\n> > -\n> > -\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > -\t\t\t mode);\n> > -\tif (it == testPatternModes_.end()) {\n> > -\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> > -\t\t\t\t\t << mode;\n> > -\t\treturn -EINVAL;\n> > -\t}\n> > -\n> > -\tLOG(CameraSensor, Debug) << \"Apply test pattern mode \" << mode;\n> > -\n> > -\tint32_t index = staticProps_->testPatternModes.at(mode);\n> > -\tControlList ctrls{ controls() };\n> > -\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> > -\n> > -\tint ret = setControls(&ctrls);\n> > -\tif (ret)\n> > -\t\treturn ret;\n> > -\n> > -\ttestPatternMode_ = mode;\n> > -\n> > -\treturn 0;\n> > -}\n> > -\n> > /**\n> > * \\brief Retrieve the best sensor format for a desired output\n> > * \\param[in] mbusCodes The list of acceptable media bus codes\n> > @@ -919,80 +900,6 @@ int CameraSensor::applyConfiguration(const SensorConfiguration &config,\n> > \treturn 0;\n> > }\n> >\n> > -/**\n> > - * \\brief Retrieve the supported V4L2 controls and their information\n> > - *\n> > - * Control information is updated automatically to reflect the current sensor\n> > - * configuration when the setFormat() function is called, without invalidating\n> > - * any iterator on the ControlInfoMap.\n> > - *\n> > - * \\return A map of the V4L2 controls supported by the sensor\n> > - */\n> > -const ControlInfoMap &CameraSensor::controls() const\n> > -{\n> > -\treturn subdev_->controls();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Read V4L2 controls from the sensor\n> > - * \\param[in] ids The list of controls to read, specified by their ID\n> > - *\n> > - * This function reads the value of all controls contained in \\a ids, and\n> > - * returns their values as a ControlList. The control identifiers are defined by\n> > - * the V4L2 specification (V4L2_CID_*).\n> > - *\n> > - * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> > - * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs\n> > - * during validation of the requested controls, no control is read and this\n> > - * function returns an empty control list.\n> > - *\n> > - * \\sa V4L2Device::getControls()\n> > - *\n> > - * \\return The control values in a ControlList on success, or an empty list on\n> > - * error\n> > - */\n> > -ControlList CameraSensor::getControls(const std::vector &ids)\n> > -{\n> > -\treturn subdev_->getControls(ids);\n> > -}\n> > -\n> > -/**\n> > - * \\brief Write V4L2 controls to the sensor\n> > - * \\param[in] ctrls The list of controls to write\n> > - *\n> > - * This function writes the value of all controls contained in \\a ctrls, and\n> > - * stores the values actually applied to the device in the corresponding \\a\n> > - * ctrls entry. The control identifiers are defined by the V4L2 specification\n> > - * (V4L2_CID_*).\n> > - *\n> > - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> > - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> > - * error occurs during validation of the requested controls, no control is\n> > - * written and this function returns -EINVAL.\n> > - *\n> > - * If an error occurs while writing the controls, the index of the first\n> > - * control that couldn't be written is returned. All controls below that index\n> > - * are written and their values are updated in \\a ctrls, while all other\n> > - * controls are not written and their values are not changed.\n> > - *\n> > - * \\sa V4L2Device::setControls()\n> > - *\n> > - * \\return 0 on success or an error code otherwise\n> > - * \\retval -EINVAL One of the control is not supported or not accessible\n> > - * \\retval i The index of the control that failed\n> > - */\n> > -int CameraSensor::setControls(ControlList *ctrls)\n> > -{\n> > -\treturn subdev_->setControls(ctrls);\n> > -}\n> > -\n> > -/**\n> > - * \\fn CameraSensor::device()\n> > - * \\brief Retrieve the camera sensor device\n> > - * \\todo Remove this function by integrating DelayedControl with CameraSensor\n> > - * \\return The camera sensor device\n> > - */\n> > -\n> > /**\n> > * \\fn CameraSensor::properties()\n> > * \\brief Retrieve the camera sensor properties\n> > @@ -1088,14 +995,6 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const\n> > \treturn 0;\n> > }\n> >\n> > -/**\n> > - * \\fn CameraSensor::focusLens()\n> > - * \\brief Retrieve the focus lens controller\n> > - *\n> > - * \\return The focus lens controller. nullptr if no focus lens controller is\n> > - * connected to the sensor\n> > - */\n> > -\n> > /**\n> > * \\brief Compute the Transform that gives the requested \\a orientation\n> > * \\param[inout] orientation The desired image orientation\n> > @@ -1155,33 +1054,134 @@ Transform CameraSensor::computeTransform(Orientation *orientation) const\n> > \treturn transform;\n> > }\n> >\n> > +/**\n> > + * \\brief Retrieve the supported V4L2 controls and their information\n> > + *\n> > + * Control information is updated automatically to reflect the current sensor\n> > + * configuration when the setFormat() function is called, without invalidating\n> > + * any iterator on the ControlInfoMap.\n> > + *\n> > + * \\return A map of the V4L2 controls supported by the sensor\n> > + */\n> > +const ControlInfoMap &CameraSensor::controls() const\n> > +{\n> > +\treturn subdev_->controls();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Read V4L2 controls from the sensor\n> > + * \\param[in] ids The list of controls to read, specified by their ID\n> > + *\n> > + * This function reads the value of all controls contained in \\a ids, and\n> > + * returns their values as a ControlList. The control identifiers are defined by\n> > + * the V4L2 specification (V4L2_CID_*).\n> > + *\n> > + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs\n> > + * during validation of the requested controls, no control is read and this\n> > + * function returns an empty control list.\n> > + *\n> > + * \\sa V4L2Device::getControls()\n> > + *\n> > + * \\return The control values in a ControlList on success, or an empty list on\n> > + * error\n> > + */\n> > +ControlList CameraSensor::getControls(const std::vector &ids)\n> > +{\n> > +\treturn subdev_->getControls(ids);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Write V4L2 controls to the sensor\n> > + * \\param[in] ctrls The list of controls to write\n> > + *\n> > + * This function writes the value of all controls contained in \\a ctrls, and\n> > + * stores the values actually applied to the device in the corresponding \\a\n> > + * ctrls entry. The control identifiers are defined by the V4L2 specification\n> > + * (V4L2_CID_*).\n> > + *\n> > + * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> > + * error occurs during validation of the requested controls, no control is\n> > + * written and this function returns -EINVAL.\n> > + *\n> > + * If an error occurs while writing the controls, the index of the first\n> > + * control that couldn't be written is returned. All controls below that index\n> > + * are written and their values are updated in \\a ctrls, while all other\n> > + * controls are not written and their values are not changed.\n> > + *\n> > + * \\sa V4L2Device::setControls()\n> > + *\n> > + * \\return 0 on success or an error code otherwise\n> > + * \\retval -EINVAL One of the control is not supported or not accessible\n> > + * \\retval i The index of the control that failed\n> > + */\n> > +int CameraSensor::setControls(ControlList *ctrls)\n> > +{\n> > +\treturn subdev_->setControls(ctrls);\n> > +}\n> > +\n> > +/**\n> > + * \\fn CameraSensor::testPatternModes()\n> > + * \\brief Retrieve all the supported test pattern modes of the camera sensor\n> > + * The test pattern mode values correspond to the controls::TestPattern control.\n> > + *\n> > + * \\return The list of test pattern modes\n> > + */\n> > +\n> > +/**\n> > + * \\brief Set the test pattern mode for the camera sensor\n> > + * \\param[in] mode The test pattern mode\n> > + *\n> > + * The new \\a mode is applied to the sensor if it differs from the active test\n> > + * pattern mode. Otherwise, this function is a no-op. Setting the same test\n> > + * pattern mode for every frame thus incurs no performance penalty.\n> > + */\n> > +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> > +{\n> > +\tif (testPatternMode_ == mode)\n> > +\t\treturn 0;\n> > +\n> > +\tif (testPatternModes_.empty()) {\n> > +\t\tLOG(CameraSensor, Error)\n> > +\t\t\t<< \"Camera sensor does not support test pattern modes.\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn applyTestPatternMode(mode);\n> > +}\n> > +\n> > +int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> > +{\n> > +\tif (testPatternModes_.empty())\n> > +\t\treturn 0;\n> > +\n> > +\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> > +\t\t\t mode);\n> > +\tif (it == testPatternModes_.end()) {\n> > +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> > +\t\t\t\t\t << mode;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tLOG(CameraSensor, Debug) << \"Apply test pattern mode \" << mode;\n> > +\n> > +\tint32_t index = staticProps_->testPatternModes.at(mode);\n> > +\tControlList ctrls{ controls() };\n> > +\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> > +\n> > +\tint ret = setControls(&ctrls);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\ttestPatternMode_ = mode;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > std::string CameraSensor::logPrefix() const\n> > {\n> > \treturn \"'\" + entity_->name() + \"'\";\n> > }\n> >\n> > -int CameraSensor::generateId()\n> > -{\n> > -\tconst std::string devPath = subdev_->devicePath();\n> > -\n> > -\t/* Try to get ID from firmware description. */\n> > -\tid_ = sysfs::firmwareNodePath(devPath);\n> > -\tif (!id_.empty())\n> > -\t\treturn 0;\n> > -\n> > -\t/*\n> > -\t * Virtual sensors not described in firmware\n> > -\t *\n> > -\t * Verify it's a platform device and construct ID from the device path\n> > -\t * and model of sensor.\n> > -\t */\n> > -\tif (devPath.find(\"/sys/devices/platform/\", 0) == 0) {\n> > -\t\tid_ = devPath.substr(strlen(\"/sys/devices/\")) + \" \" + model();\n> > -\t\treturn 0;\n> > -\t}\n> > -\n> > -\tLOG(CameraSensor, Error) << \"Can't generate sensor ID\";\n> > -\treturn -EINVAL;\n> > -}\n> > -\n> > } /* namespace libcamera */","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 D8C65BD160\n\tfor ;\n\tMon, 4 Mar 2024 22:48:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8F6176286F;\n\tMon, 4 Mar 2024 23:48:08 +0100 (CET)","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 39704627FC\n\tfor ;\n\tMon, 4 Mar 2024 23:48:07 +0100 (CET)","from pendragon.ideasonboard.com (89-27-53-110.bb.dnainternet.fi\n\t[89.27.53.110])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95EE422E8;\n\tMon, 4 Mar 2024 23:47:50 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ksGnsTl1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709592470;\n\tbh=5nTGfyHd3z5QH/Etq8jT7Fx51csYvEI3U6k3KFZt5Pw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ksGnsTl1lUyuCFse+QQUd2UhmU8hJikfD0Iv6AEBf87dg9JgjE3Gqf7HdpEcN/njj\n\t6V/N2+kD47zHe1SNH88ouVa1TQCzvw1HNghu9q4dmhDhjuvqIGbSCoG3CuYY5GPxnx\n\tu/lmhcShKPczFZ5wQvpO4AIVmooh0FqljapDNIC4=","Date":"Tue, 5 Mar 2024 00:48:08 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Subject":"Re: [PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions","Message-ID":"<20240304224808.GF9233@pendragon.ideasonboard.com>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-17-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28886,"web_url":"https://patchwork.libcamera.org/comment/28886/","msgid":"<20240306162058.n7bpofdvq56jukav@jasper>","date":"2024-03-06T16:20:58","subject":"Re: [PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Mar 01, 2024 at 11:21:05PM +0200, Laurent Pinchart wrote:\n> The CameraSensor class has grown a lot since its creation, with many\n> functions added for different types of purposes. They are not grouped by\n> categories in the class definition, generating confusion when reading\n> the header file. Improve readability by sorting functions by category:\n> \n> - Getters for static data (model, entity, focus lens, ...)\n> - Format and sensor configuration accessors\n> - Properties and controls (including test pattern mode)\n> \n> Update the .cpp file accordingly.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> ---\n> include/libcamera/internal/camera_sensor.h | 29 +-\n> src/libcamera/sensor/camera_sensor.cpp | 328 ++++++++++-----------\n> 2 files changed, 179 insertions(+), 178 deletions(-)\n> \n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index b2f077b9cc75..750d6d729cac 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -46,15 +46,15 @@ public:\n> \n> \tconst std::string &model() const { return model_; }\n> \tconst std::string &id() const { return id_; }\n> +\n> \tconst MediaEntity *entity() const { return entity_; }\n> +\tV4L2Subdevice *device() { return subdev_.get(); }\n> +\n> +\tCameraLens *focusLens() { return focusLens_.get(); }\n> +\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> \tstd::vector sizes(unsigned int mbusCode) const;\n> \tSize resolution() const;\n> -\tconst std::vector &testPatternModes() const\n> -\t{\n> -\t\treturn testPatternModes_;\n> -\t}\n> -\tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \n> \tV4L2SubdeviceFormat getFormat(const std::vector &mbusCodes,\n> \t\t\t\t const Size &size) const;\n> @@ -66,18 +66,19 @@ public:\n> \t\t\t Transform transform = Transform::Identity,\n> \t\t\t V4L2SubdeviceFormat *sensorFormat = nullptr);\n> \n> +\tconst ControlList &properties() const { return properties_; }\n> +\tint sensorInfo(IPACameraSensorInfo *info) const;\n> +\tTransform computeTransform(Orientation *orientation) const;\n> +\n> \tconst ControlInfoMap &controls() const;\n> \tControlList getControls(const std::vector &ids);\n> \tint setControls(ControlList *ctrls);\n> \n> -\tV4L2Subdevice *device() { return subdev_.get(); }\n> -\n> -\tconst ControlList &properties() const { return properties_; }\n> -\tint sensorInfo(IPACameraSensorInfo *info) const;\n> -\n> -\tCameraLens *focusLens() { return focusLens_.get(); }\n> -\n> -\tTransform computeTransform(Orientation *orientation) const;\n> +\tconst std::vector &testPatternModes() const\n> +\t{\n> +\t\treturn testPatternModes_;\n> +\t}\n> +\tint setTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \n> protected:\n> \tstd::string logPrefix() const override;\n> @@ -91,8 +92,8 @@ private:\n> \tvoid initStaticProperties();\n> \tvoid initTestPatternModes();\n> \tint initProperties();\n> -\tint applyTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \tint discoverAncillaryDevices();\n> +\tint applyTestPatternMode(controls::draft::TestPatternModeEnum mode);\n> \n> \tconst MediaEntity *entity_;\n> \tstd::unique_ptr subdev_;\n> diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp\n> index 545f89d036df..86ad9a85371c 100644\n> --- a/src/libcamera/sensor/camera_sensor.cpp\n> +++ b/src/libcamera/sensor/camera_sensor.cpp\n> @@ -215,6 +215,30 @@ int CameraSensor::init()\n> \treturn applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);\n> }\n> \n> +int CameraSensor::generateId()\n> +{\n> +\tconst std::string devPath = subdev_->devicePath();\n> +\n> +\t/* Try to get ID from firmware description. */\n> +\tid_ = sysfs::firmwareNodePath(devPath);\n> +\tif (!id_.empty())\n> +\t\treturn 0;\n> +\n> +\t/*\n> +\t * Virtual sensors not described in firmware\n> +\t *\n> +\t * Verify it's a platform device and construct ID from the device path\n> +\t * and model of sensor.\n> +\t */\n> +\tif (devPath.find(\"/sys/devices/platform/\", 0) == 0) {\n> +\t\tid_ = devPath.substr(strlen(\"/sys/devices/\")) + \" \" + model();\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tLOG(CameraSensor, Error) << \"Can't generate sensor ID\";\n> +\treturn -EINVAL;\n> +}\n> +\n> int CameraSensor::validateSensorDriver()\n> {\n> \tint err = 0;\n> @@ -580,6 +604,21 @@ int CameraSensor::discoverAncillaryDevices()\n> * \\return The sensor media entity\n> */\n> \n> +/**\n> + * \\fn CameraSensor::device()\n> + * \\brief Retrieve the camera sensor device\n> + * \\todo Remove this function by integrating DelayedControl with CameraSensor\n> + * \\return The camera sensor device\n> + */\n> +\n> +/**\n> + * \\fn CameraSensor::focusLens()\n> + * \\brief Retrieve the focus lens controller\n> + *\n> + * \\return The focus lens controller. nullptr if no focus lens controller is\n> + * connected to the sensor\n> + */\n> +\n> /**\n> * \\fn CameraSensor::mbusCodes()\n> * \\brief Retrieve the media bus codes supported by the camera sensor\n> @@ -632,64 +671,6 @@ Size CameraSensor::resolution() const\n> \treturn std::min(sizes_.back(), activeArea_.size());\n> }\n> \n> -/**\n> - * \\fn CameraSensor::testPatternModes()\n> - * \\brief Retrieve all the supported test pattern modes of the camera sensor\n> - * The test pattern mode values correspond to the controls::TestPattern control.\n> - *\n> - * \\return The list of test pattern modes\n> - */\n> -\n> -/**\n> - * \\brief Set the test pattern mode for the camera sensor\n> - * \\param[in] mode The test pattern mode\n> - *\n> - * The new \\a mode is applied to the sensor if it differs from the active test\n> - * pattern mode. Otherwise, this function is a no-op. Setting the same test\n> - * pattern mode for every frame thus incurs no performance penalty.\n> - */\n> -int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> -{\n> -\tif (testPatternMode_ == mode)\n> -\t\treturn 0;\n> -\n> -\tif (testPatternModes_.empty()) {\n> -\t\tLOG(CameraSensor, Error)\n> -\t\t\t<< \"Camera sensor does not support test pattern modes.\";\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\treturn applyTestPatternMode(mode);\n> -}\n> -\n> -int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> -{\n> -\tif (testPatternModes_.empty())\n> -\t\treturn 0;\n> -\n> -\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> -\t\t\t mode);\n> -\tif (it == testPatternModes_.end()) {\n> -\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> -\t\t\t\t\t << mode;\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\tLOG(CameraSensor, Debug) << \"Apply test pattern mode \" << mode;\n> -\n> -\tint32_t index = staticProps_->testPatternModes.at(mode);\n> -\tControlList ctrls{ controls() };\n> -\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> -\n> -\tint ret = setControls(&ctrls);\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\ttestPatternMode_ = mode;\n> -\n> -\treturn 0;\n> -}\n> -\n> /**\n> * \\brief Retrieve the best sensor format for a desired output\n> * \\param[in] mbusCodes The list of acceptable media bus codes\n> @@ -919,80 +900,6 @@ int CameraSensor::applyConfiguration(const SensorConfiguration &config,\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\brief Retrieve the supported V4L2 controls and their information\n> - *\n> - * Control information is updated automatically to reflect the current sensor\n> - * configuration when the setFormat() function is called, without invalidating\n> - * any iterator on the ControlInfoMap.\n> - *\n> - * \\return A map of the V4L2 controls supported by the sensor\n> - */\n> -const ControlInfoMap &CameraSensor::controls() const\n> -{\n> -\treturn subdev_->controls();\n> -}\n> -\n> -/**\n> - * \\brief Read V4L2 controls from the sensor\n> - * \\param[in] ids The list of controls to read, specified by their ID\n> - *\n> - * This function reads the value of all controls contained in \\a ids, and\n> - * returns their values as a ControlList. The control identifiers are defined by\n> - * the V4L2 specification (V4L2_CID_*).\n> - *\n> - * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> - * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs\n> - * during validation of the requested controls, no control is read and this\n> - * function returns an empty control list.\n> - *\n> - * \\sa V4L2Device::getControls()\n> - *\n> - * \\return The control values in a ControlList on success, or an empty list on\n> - * error\n> - */\n> -ControlList CameraSensor::getControls(const std::vector &ids)\n> -{\n> -\treturn subdev_->getControls(ids);\n> -}\n> -\n> -/**\n> - * \\brief Write V4L2 controls to the sensor\n> - * \\param[in] ctrls The list of controls to write\n> - *\n> - * This function writes the value of all controls contained in \\a ctrls, and\n> - * stores the values actually applied to the device in the corresponding \\a\n> - * ctrls entry. The control identifiers are defined by the V4L2 specification\n> - * (V4L2_CID_*).\n> - *\n> - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> - * error occurs during validation of the requested controls, no control is\n> - * written and this function returns -EINVAL.\n> - *\n> - * If an error occurs while writing the controls, the index of the first\n> - * control that couldn't be written is returned. All controls below that index\n> - * are written and their values are updated in \\a ctrls, while all other\n> - * controls are not written and their values are not changed.\n> - *\n> - * \\sa V4L2Device::setControls()\n> - *\n> - * \\return 0 on success or an error code otherwise\n> - * \\retval -EINVAL One of the control is not supported or not accessible\n> - * \\retval i The index of the control that failed\n> - */\n> -int CameraSensor::setControls(ControlList *ctrls)\n> -{\n> -\treturn subdev_->setControls(ctrls);\n> -}\n> -\n> -/**\n> - * \\fn CameraSensor::device()\n> - * \\brief Retrieve the camera sensor device\n> - * \\todo Remove this function by integrating DelayedControl with CameraSensor\n> - * \\return The camera sensor device\n> - */\n> -\n> /**\n> * \\fn CameraSensor::properties()\n> * \\brief Retrieve the camera sensor properties\n> @@ -1088,14 +995,6 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const\n> \treturn 0;\n> }\n> \n> -/**\n> - * \\fn CameraSensor::focusLens()\n> - * \\brief Retrieve the focus lens controller\n> - *\n> - * \\return The focus lens controller. nullptr if no focus lens controller is\n> - * connected to the sensor\n> - */\n> -\n> /**\n> * \\brief Compute the Transform that gives the requested \\a orientation\n> * \\param[inout] orientation The desired image orientation\n> @@ -1155,33 +1054,134 @@ Transform CameraSensor::computeTransform(Orientation *orientation) const\n> \treturn transform;\n> }\n> \n> +/**\n> + * \\brief Retrieve the supported V4L2 controls and their information\n> + *\n> + * Control information is updated automatically to reflect the current sensor\n> + * configuration when the setFormat() function is called, without invalidating\n> + * any iterator on the ControlInfoMap.\n> + *\n> + * \\return A map of the V4L2 controls supported by the sensor\n> + */\n> +const ControlInfoMap &CameraSensor::controls() const\n> +{\n> +\treturn subdev_->controls();\n> +}\n> +\n> +/**\n> + * \\brief Read V4L2 controls from the sensor\n> + * \\param[in] ids The list of controls to read, specified by their ID\n> + *\n> + * This function reads the value of all controls contained in \\a ids, and\n> + * returns their values as a ControlList. The control identifiers are defined by\n> + * the V4L2 specification (V4L2_CID_*).\n> + *\n> + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs\n> + * during validation of the requested controls, no control is read and this\n> + * function returns an empty control list.\n> + *\n> + * \\sa V4L2Device::getControls()\n> + *\n> + * \\return The control values in a ControlList on success, or an empty list on\n> + * error\n> + */\n> +ControlList CameraSensor::getControls(const std::vector &ids)\n> +{\n> +\treturn subdev_->getControls(ids);\n> +}\n> +\n> +/**\n> + * \\brief Write V4L2 controls to the sensor\n> + * \\param[in] ctrls The list of controls to write\n> + *\n> + * This function writes the value of all controls contained in \\a ctrls, and\n> + * stores the values actually applied to the device in the corresponding \\a\n> + * ctrls entry. The control identifiers are defined by the V4L2 specification\n> + * (V4L2_CID_*).\n> + *\n> + * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other\n> + * error occurs during validation of the requested controls, no control is\n> + * written and this function returns -EINVAL.\n> + *\n> + * If an error occurs while writing the controls, the index of the first\n> + * control that couldn't be written is returned. All controls below that index\n> + * are written and their values are updated in \\a ctrls, while all other\n> + * controls are not written and their values are not changed.\n> + *\n> + * \\sa V4L2Device::setControls()\n> + *\n> + * \\return 0 on success or an error code otherwise\n> + * \\retval -EINVAL One of the control is not supported or not accessible\n> + * \\retval i The index of the control that failed\n> + */\n> +int CameraSensor::setControls(ControlList *ctrls)\n> +{\n> +\treturn subdev_->setControls(ctrls);\n> +}\n> +\n> +/**\n> + * \\fn CameraSensor::testPatternModes()\n> + * \\brief Retrieve all the supported test pattern modes of the camera sensor\n> + * The test pattern mode values correspond to the controls::TestPattern control.\n> + *\n> + * \\return The list of test pattern modes\n> + */\n> +\n> +/**\n> + * \\brief Set the test pattern mode for the camera sensor\n> + * \\param[in] mode The test pattern mode\n> + *\n> + * The new \\a mode is applied to the sensor if it differs from the active test\n> + * pattern mode. Otherwise, this function is a no-op. Setting the same test\n> + * pattern mode for every frame thus incurs no performance penalty.\n> + */\n> +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> +{\n> +\tif (testPatternMode_ == mode)\n> +\t\treturn 0;\n> +\n> +\tif (testPatternModes_.empty()) {\n> +\t\tLOG(CameraSensor, Error)\n> +\t\t\t<< \"Camera sensor does not support test pattern modes.\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn applyTestPatternMode(mode);\n> +}\n> +\n> +int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)\n> +{\n> +\tif (testPatternModes_.empty())\n> +\t\treturn 0;\n> +\n> +\tauto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),\n> +\t\t\t mode);\n> +\tif (it == testPatternModes_.end()) {\n> +\t\tLOG(CameraSensor, Error) << \"Unsupported test pattern mode \"\n> +\t\t\t\t\t << mode;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tLOG(CameraSensor, Debug) << \"Apply test pattern mode \" << mode;\n> +\n> +\tint32_t index = staticProps_->testPatternModes.at(mode);\n> +\tControlList ctrls{ controls() };\n> +\tctrls.set(V4L2_CID_TEST_PATTERN, index);\n> +\n> +\tint ret = setControls(&ctrls);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\ttestPatternMode_ = mode;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string CameraSensor::logPrefix() const\n> {\n> \treturn \"'\" + entity_->name() + \"'\";\n> }\n> \n> -int CameraSensor::generateId()\n> -{\n> -\tconst std::string devPath = subdev_->devicePath();\n> -\n> -\t/* Try to get ID from firmware description. */\n> -\tid_ = sysfs::firmwareNodePath(devPath);\n> -\tif (!id_.empty())\n> -\t\treturn 0;\n> -\n> -\t/*\n> -\t * Virtual sensors not described in firmware\n> -\t *\n> -\t * Verify it's a platform device and construct ID from the device path\n> -\t * and model of sensor.\n> -\t */\n> -\tif (devPath.find(\"/sys/devices/platform/\", 0) == 0) {\n> -\t\tid_ = devPath.substr(strlen(\"/sys/devices/\")) + \" \" + model();\n> -\t\treturn 0;\n> -\t}\n> -\n> -\tLOG(CameraSensor, Error) << \"Can't generate sensor ID\";\n> -\treturn -EINVAL;\n> -}\n> -\n> } /* namespace libcamera */\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 91148BD160\n\tfor ;\n\tWed, 6 Mar 2024 16:21:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 945F562871;\n\tWed, 6 Mar 2024 17:21:01 +0100 (CET)","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 AFDA562867\n\tfor ;\n\tWed, 6 Mar 2024 17:21:00 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:dd65:a3ea:5f4d:989a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0988BD1;\n\tWed, 6 Mar 2024 17:20:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jEBC01X8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1709742042;\n\tbh=nMb+EaNZI1UkBguGVBFVSTGfSiz2drl7UKNkZ7chA20=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jEBC01X8ZsH5IFzK+OWrQFF08yHpTfO8Fb96Br7ogYWNDWANbiOBz4NdM7WKB1jJX\n\tOqCEh0K+1cacXkEvYsgQLkxzQc5Pu6I+gBXbq3AacMB2/7eAnYzctqspbgaSHK1NJP\n\tsL76qjGmSSIVHO+DK26ksHq6qCv0rO7AjACn6Xw8=","Date":"Wed, 6 Mar 2024 17:20:58 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Subject":"Re: [PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions","Message-ID":"<20240306162058.n7bpofdvq56jukav@jasper>","References":"<20240301212121.9072-1-laurent.pinchart@ideasonboard.com>\n\t<20240301212121.9072-17-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240301212121.9072-17-laurent.pinchart@ideasonboard.com>","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, Sakari Ailus ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]