[{"id":1966,"web_url":"https://patchwork.libcamera.org/comment/1966/","msgid":"<20190622202420.GD8156@pendragon.ideasonboard.com>","date":"2019-06-22T20:24:20","subject":"Re: [libcamera-devel] [PATCH v5 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Thu, Jun 20, 2019 at 05:31:41PM +0200, Jacopo Mondi wrote:\n> Implement getControls() and setControls() operations in V4L2Device class.\n> \n> Both operations take a V4L2Controls instance and read or write the V4L2\n> controls on the V4L2 device.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_device.h | 3 +\n> src/libcamera/v4l2_device.cpp | 193 ++++++++++++++++++++++++++++\n> 2 files changed, 196 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 91a72fcecbcc..3af27c9f7af5 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -15,6 +15,7 @@\n> namespace libcamera {\n> \n> class V4L2ControlInfo;\n> +class V4L2Controls;\n> \n> class V4L2Device : protected Loggable\n> {\n> @@ -23,6 +24,8 @@ public:\n> \tbool isOpen() const { return fd_ != -1; }\n> \n> \tconst V4L2ControlInfo *getControlInfo(unsigned int id) const;\n> +\tint getControls(V4L2Controls *ctrls);\n> +\tint setControls(V4L2Controls *ctrls);\n> \n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> \n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index b113ff77e3cf..efe30ef856ae 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -125,6 +125,199 @@ const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const\n> \treturn &it->second;\n> }\n> \n> +/**\n> + * \\brief Read controls from the device\n> + * \\param[inout] ctrls The list of controls to read\n> + *\n> + * Read the value of each control contained in \\a ctrls, and store the\n> + * value in the corresponding \\a ctrls entry.\n> + *\n> + * If an integer number less than the requested number of controls is returned,\n> + * only controls up to that index have been successfully read.\n> + *\n> + * Each V4L2Control instance in \\a ctrls should be supported by the device and\n> + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise\n> + * a negative error code (-EINVAL) is returned\n\n * This method reads the value of all controls contained in \\a ctrls, and stores\n * their values in the corresponding \\a ctrls entry.\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 a compound control, or if any\n * other error occurs during validation of the requested controls, no control is\n * read and this method returns -EINVAL.\n *\n * If an error occurs while reading the controls, the index of the first\n * control that couldn't be read is returned. The value of all controls\n * below that index are updated in \\a ctrls, while the value of all the\n * other controls are not changed.\n\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 V4L2Device::getControls(V4L2Controls *ctrls)\n> +{\n> +\tunsigned int count = ctrls->size();\n> +\tif (count == 0)\n> +\t\treturn 0;\n> +\n> +\tconst V4L2ControlInfo *controlInfo[count] = {};\n> +\tstruct v4l2_ext_control v4l2Ctrls[count] = {};\n> +\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\tconst V4L2ControlInfo *info = getControlInfo(ctrl->id());\n> +\t\tif (!info) {\n> +\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t<< \"Control '\" << ctrl->id() << \"' not found\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tcontrolInfo[i] = info;\n> +\t\tv4l2Ctrls[i].id = info->id();\n> +\t}\n> +\n> +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> +\tv4l2ExtCtrls.count = count;\n> +\n> +\tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> +\tif (ret) {\n> +\t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> +\n> +\t\t/* Generic validation error. */\n> +\t\tif (errorIdx == count) {\n> +\t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> +\t\t\t\t\t << strerror(ret);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\t/* A specific control failed. */\n> +\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> +\t\t\t\t << \": \" << strerror(ret);\n> +\t\treturn errorIdx;\n\nThis contradicts with the documentation (OK, with my documentation only\n:-)), the values of all controls under the error index should be stored\nin ctrls. Alternatively we could modify the documentation and not update\nany value, but then would returning the error index be useful ? I would\nthus write this\n\n\tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n\tif (ret) {\n\t{\n\t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n\n\t\t/* Generic validation error. */\n\t\tif (errorIdx == 0 || errorIdx >= count) {\n\t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n\t\t\t\t\t << strerror(ret);\n\t\t\treturn -EINVAL;\n\t\t}\n\n\t\t/* A specific control failed. */\n\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n\t\t\t\t << \": \" << strerror(ret);\n\t\tcount = errorIdx - 1;\n\t\tret = errorIdx;\n\t}\n\nand return ret at the end of the function.\n\n> +\t}\n> +\n> +\t/*\n> +\t * For each control read from the device, set the value in the\n> +\t * V4L2Control provided by the caller.\n> +\t */\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> +\t\tconst V4L2ControlInfo *info = controlInfo[i];\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\tswitch (info->type()) {\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\t/*\n> +\t\t\t * \\todo To be changed when support for string and\n> +\t\t\t * compound controls will be added.\n> +\t\t\t */\n> +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n\nDo you need the cast ? If you do it should be cast to int64_t.\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Write controls to the device\n> + * \\param[in] ctrls The list of controls to write\n> + *\n> + * Write the value of each V4L2Control contained in \\a ctrls. Each\n> + * control should be initialized by the caller with a value, otherwise\n> + * the result of the operation is undefined.\n> + *\n> + * The value of each control is updated to reflect what has been\n> + * actually applied on the device.\n\nNo need to wrap lines well below the 80 characters limit :-)\n\n> + *\n> + * If an integer number less than the requested number of controls is returned,\n> + * only controls up to that index have been successfully applied.\n> + *\n> + * Each V4L2Control instance in \\a ctrls should be supported by the device and\n> + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise\n> + * a negative error code (-EINVAL) is returned\n> + *\n\nTo mimick getControls(),\n\n\n * This method writes the value of all controls contained in \\a ctrls, and\n * stores the values actually applied to the device in the corresponding\n * \\a ctrls entry.\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, is a\n * compound control, or if any other error occurs during validation of\n * the requested controls, no control is written and this method returns\n * -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> + * \\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 V4L2Device::setControls(V4L2Controls *ctrls)\n> +{\n> +\tunsigned int count = ctrls->size();\n> +\tif (count == 0)\n> +\t\treturn 0;\n> +\n> +\tconst V4L2ControlInfo *controlInfo[count] = {};\n> +\tstruct v4l2_ext_control v4l2Ctrls[count] = {};\n> +\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\tconst V4L2ControlInfo *info = getControlInfo(ctrl->id());\n> +\t\tif (!info) {\n> +\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t<< \"Control '\" << ctrl->id() << \"' not found\";\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tcontrolInfo[i] = info;\n> +\t\tv4l2Ctrls[i].id = info->id();\n> +\n> +\t\t/* Set the v4l2_ext_control value for the write operation. */\n> +\t\tswitch (info->type()) {\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\tv4l2Ctrls[i].value64 = ctrl->value();\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\t/*\n> +\t\t\t * \\todo To be changed when support for string and\n> +\t\t\t * compound controls will be added.\n> +\t\t\t */\n> +\t\t\tv4l2Ctrls[i].value = static_cast(ctrl->value());\n\nSame here, do we need the cast ?\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> +\tv4l2ExtCtrls.count = count;\n> +\n> +\tint ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);\n> +\tif (ret) {\n> +\t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> +\n> +\t\t/* Generic validation error. */\n> +\t\tif (errorIdx == count) {\n> +\t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> +\t\t\t\t\t << strerror(ret);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\t/* A specific control failed. */\n> +\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> +\t\t\t\t << \": \" << strerror(ret);\n> +\t\treturn errorIdx;\n\nSame here, I think you should update the controls that have been\nwritten. \n\n> +\t}\n> +\n> +\t/* Update the control value to what have actually been applied. */\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> +\t\tconst V4L2ControlInfo *info = controlInfo[i];\n> +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> +\n> +\t\tswitch (info->type()) {\n> +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\t/*\n> +\t\t\t * \\todo To be changed when support for string and\n> +\t\t\t * compound controls will be added.\n> +\t\t\t */\n> +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n\nAnd here too.\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n\nThis loop could possibly be moved to an updateControls() function and\nshared between getControls() and setControls().\n\n> +\n> +\treturn 0;\n> +}\n> +\n> /**\n> * \\brief Perform an IOCTL system call on the device node\n> * \\param[in] request The IOCTL request code","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4E6BB61580\n\tfor ;\n\tSat, 22 Jun 2019 22:24:39 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B59A567;\n\tSat, 22 Jun 2019 22:24:38 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561235078;\n\tbh=qtc2g7A+TBUm6qo1ki0csG1dHguGHt0J9iJv8/vK3hw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s1Udk2a1T/hC/vnBDq7WR/BFxJtUj4LyAoI+wbOgwEau69zohXP4EsNYQ0qYxRW8y\n\tIzg/wmVPrFIJTQFiKPS00KPGTLp1Fya3kSKUYO/UUXP5qa9y2+6inL/FVPDDZg+JXr\n\tHrSQp/wTHYmOU3p+GZWO2FJEBtPNW4YDDG6uC+LI=","Date":"Sat, 22 Jun 2019 23:24:20 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190622202420.GD8156@pendragon.ideasonboard.com>","References":"<20190620153144.5394-1-jacopo@jmondi.org>\n\t<20190620153144.5394-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190620153144.5394-4-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 22 Jun 2019 20:24:39 -0000"}},{"id":1990,"web_url":"https://patchwork.libcamera.org/comment/1990/","msgid":"<20190624082216.to4posl5adqwrwql@uno.localdomain>","date":"2019-06-24T08:22:16","subject":"Re: [libcamera-devel] [PATCH v5 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Jun 22, 2019 at 11:24:20PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n\nThanks, I'll take comments on this patch in.\n\n>\n> On Thu, Jun 20, 2019 at 05:31:41PM +0200, Jacopo Mondi wrote:\n> > Implement getControls() and setControls() operations in V4L2Device class.\n> >\n> > Both operations take a V4L2Controls instance and read or write the V4L2\n> > controls on the V4L2 device.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_device.h | 3 +\n> > src/libcamera/v4l2_device.cpp | 193 ++++++++++++++++++++++++++++\n> > 2 files changed, 196 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index 91a72fcecbcc..3af27c9f7af5 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -15,6 +15,7 @@\n> > namespace libcamera {\n> >\n> > class V4L2ControlInfo;\n> > +class V4L2Controls;\n> >\n> > class V4L2Device : protected Loggable\n> > {\n> > @@ -23,6 +24,8 @@ public:\n> > \tbool isOpen() const { return fd_ != -1; }\n> >\n> > \tconst V4L2ControlInfo *getControlInfo(unsigned int id) const;\n> > +\tint getControls(V4L2Controls *ctrls);\n> > +\tint setControls(V4L2Controls *ctrls);\n> >\n> > \tconst std::string &deviceNode() const { return deviceNode_; }\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index b113ff77e3cf..efe30ef856ae 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -125,6 +125,199 @@ const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const\n> > \treturn &it->second;\n> > }\n> >\n> > +/**\n> > + * \\brief Read controls from the device\n> > + * \\param[inout] ctrls The list of controls to read\n> > + *\n> > + * Read the value of each control contained in \\a ctrls, and store the\n> > + * value in the corresponding \\a ctrls entry.\n> > + *\n> > + * If an integer number less than the requested number of controls is returned,\n> > + * only controls up to that index have been successfully read.\n> > + *\n> > + * Each V4L2Control instance in \\a ctrls should be supported by the device and\n> > + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise\n> > + * a negative error code (-EINVAL) is returned\n>\n> * This method reads the value of all controls contained in \\a ctrls, and stores\n> * their values in the corresponding \\a ctrls entry.\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 a compound control, or if any\n> * other error occurs during validation of the requested controls, no control is\n> * read and this method returns -EINVAL.\n> *\n> * If an error occurs while reading the controls, the index of the first\n> * control that couldn't be read is returned. The value of all controls\n> * below that index are updated in \\a ctrls, while the value of all the\n> * other controls are not changed.\n>\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 V4L2Device::getControls(V4L2Controls *ctrls)\n> > +{\n> > +\tunsigned int count = ctrls->size();\n> > +\tif (count == 0)\n> > +\t\treturn 0;\n> > +\n> > +\tconst V4L2ControlInfo *controlInfo[count] = {};\n> > +\tstruct v4l2_ext_control v4l2Ctrls[count] = {};\n> > +\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\tconst V4L2ControlInfo *info = getControlInfo(ctrl->id());\n> > +\t\tif (!info) {\n> > +\t\t\tLOG(V4L2, Error)\n> > +\t\t\t\t<< \"Control '\" << ctrl->id() << \"' not found\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tcontrolInfo[i] = info;\n> > +\t\tv4l2Ctrls[i].id = info->id();\n> > +\t}\n> > +\n> > +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > +\tv4l2ExtCtrls.count = count;\n> > +\n> > +\tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> > +\tif (ret) {\n> > +\t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> > +\n> > +\t\t/* Generic validation error. */\n> > +\t\tif (errorIdx == count) {\n> > +\t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> > +\t\t\t\t\t << strerror(ret);\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\t/* A specific control failed. */\n> > +\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > +\t\t\t\t << \": \" << strerror(ret);\n> > +\t\treturn errorIdx;\n>\n> This contradicts with the documentation (OK, with my documentation only\n> :-)), the values of all controls under the error index should be stored\n> in ctrls. Alternatively we could modify the documentation and not update\n> any value, but then would returning the error index be useful ? I would\n> thus write this\n>\n> \tint ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls);\n> \tif (ret) {\n> \t{\n> \t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n>\n> \t\t/* Generic validation error. */\n> \t\tif (errorIdx == 0 || errorIdx >= count) {\n> \t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> \t\t\t\t\t << strerror(ret);\n> \t\t\treturn -EINVAL;\n> \t\t}\n>\n> \t\t/* A specific control failed. */\n> \t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> \t\t\t\t << \": \" << strerror(ret);\n> \t\tcount = errorIdx - 1;\n> \t\tret = errorIdx;\n> \t}\n>\n> and return ret at the end of the function.\n>\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * For each control read from the device, set the value in the\n> > +\t * V4L2Control provided by the caller.\n> > +\t */\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > +\t\tconst V4L2ControlInfo *info = controlInfo[i];\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\tswitch (info->type()) {\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\t/*\n> > +\t\t\t * \\todo To be changed when support for string and\n> > +\t\t\t * compound controls will be added.\n> > +\t\t\t */\n> > +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n>\n> Do you need the cast ? If you do it should be cast to int64_t.\n>\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Write controls to the device\n> > + * \\param[in] ctrls The list of controls to write\n> > + *\n> > + * Write the value of each V4L2Control contained in \\a ctrls. Each\n> > + * control should be initialized by the caller with a value, otherwise\n> > + * the result of the operation is undefined.\n> > + *\n> > + * The value of each control is updated to reflect what has been\n> > + * actually applied on the device.\n>\n> No need to wrap lines well below the 80 characters limit :-)\n>\n> > + *\n> > + * If an integer number less than the requested number of controls is returned,\n> > + * only controls up to that index have been successfully applied.\n> > + *\n> > + * Each V4L2Control instance in \\a ctrls should be supported by the device and\n> > + * accessible (ie the V4L2_CTRL_FLAG_DISABLED flag should not be set), otherwise\n> > + * a negative error code (-EINVAL) is returned\n> > + *\n>\n> To mimick getControls(),\n>\n>\n> * This method writes the value of all controls contained in \\a ctrls, and\n> * stores the values actually applied to the device in the corresponding\n> * \\a ctrls entry.\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, is a\n> * compound control, or if any other error occurs during validation of\n> * the requested controls, no control is written and this method returns\n> * -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> > + * \\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 V4L2Device::setControls(V4L2Controls *ctrls)\n> > +{\n> > +\tunsigned int count = ctrls->size();\n> > +\tif (count == 0)\n> > +\t\treturn 0;\n> > +\n> > +\tconst V4L2ControlInfo *controlInfo[count] = {};\n> > +\tstruct v4l2_ext_control v4l2Ctrls[count] = {};\n> > +\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\tconst V4L2ControlInfo *info = getControlInfo(ctrl->id());\n> > +\t\tif (!info) {\n> > +\t\t\tLOG(V4L2, Error)\n> > +\t\t\t\t<< \"Control '\" << ctrl->id() << \"' not found\";\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\tcontrolInfo[i] = info;\n> > +\t\tv4l2Ctrls[i].id = info->id();\n> > +\n> > +\t\t/* Set the v4l2_ext_control value for the write operation. */\n> > +\t\tswitch (info->type()) {\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\tv4l2Ctrls[i].value64 = ctrl->value();\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\t/*\n> > +\t\t\t * \\todo To be changed when support for string and\n> > +\t\t\t * compound controls will be added.\n> > +\t\t\t */\n> > +\t\t\tv4l2Ctrls[i].value = static_cast(ctrl->value());\n>\n> Same here, do we need the cast ?\n>\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tstruct v4l2_ext_controls v4l2ExtCtrls = {};\n> > +\tv4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL;\n> > +\tv4l2ExtCtrls.controls = v4l2Ctrls;\n> > +\tv4l2ExtCtrls.count = count;\n> > +\n> > +\tint ret = ioctl(VIDIOC_S_EXT_CTRLS, &v4l2ExtCtrls);\n> > +\tif (ret) {\n> > +\t\tunsigned int errorIdx = v4l2ExtCtrls.error_idx;\n> > +\n> > +\t\t/* Generic validation error. */\n> > +\t\tif (errorIdx == count) {\n> > +\t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> > +\t\t\t\t\t << strerror(ret);\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\t/* A specific control failed. */\n> > +\t\tLOG(V4L2, Error) << \"Unable to read control \" << errorIdx\n> > +\t\t\t\t << \": \" << strerror(ret);\n> > +\t\treturn errorIdx;\n>\n> Same here, I think you should update the controls that have been\n> written.\n>\n> > +\t}\n> > +\n> > +\t/* Update the control value to what have actually been applied. */\n> > +\tfor (unsigned int i = 0; i < count; ++i) {\n> > +\t\tstruct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > +\t\tconst V4L2ControlInfo *info = controlInfo[i];\n> > +\t\tV4L2Control *ctrl = (*ctrls)[i];\n> > +\n> > +\t\tswitch (info->type()) {\n> > +\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\t/*\n> > +\t\t\t * \\todo To be changed when support for string and\n> > +\t\t\t * compound controls will be added.\n> > +\t\t\t */\n> > +\t\t\tctrl->setValue(static_cast(v4l2Ctrl->value));\n>\n> And here too.\n>\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n>\n> This loop could possibly be moved to an updateControls() function and\n> shared between getControls() and setControls().\n>\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > /**\n> > * \\brief Perform an IOCTL system call on the device node\n> > * \\param[in] request The IOCTL request code\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8A4560C29\n\tfor ;\n\tMon, 24 Jun 2019 10:21:02 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 43402FF80F;\n\tMon, 24 Jun 2019 08:21:01 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 24 Jun 2019 10:22:16 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190624082216.to4posl5adqwrwql@uno.localdomain>","References":"<20190620153144.5394-1-jacopo@jmondi.org>\n\t<20190620153144.5394-4-jacopo@jmondi.org>\n\t<20190622202420.GD8156@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"wih36ikgrxdzzejk\"","Content-Disposition":"inline","In-Reply-To":"<20190622202420.GD8156@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 3/6] libcamera: v4l2_device:\n\tImplement get and set controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 08:21:02 -0000"}}]