[{"id":1965,"web_url":"https://patchwork.libcamera.org/comment/1965/","msgid":"<20190622171652.GC8156@pendragon.ideasonboard.com>","date":"2019-06-22T17:16:52","subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","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:40PM +0200, Jacopo Mondi wrote:\n> Enumerate all the valid controls a device supports at open() time.\n> A control is valid only if its type is supported.\n> \n> Store the control informations in a map inside the device to save\n\ns/informations/information/\n\nhttps://en.wiktionary.org/wiki/informations\n\n\"Most senses of the word “information” are uncountable. The legal sense,\nreferring to court filings, is one that does form a plural.\"\n\n> querying the control when setting or getting its value from the device\n> and provide an operation to retrieve information by control ID.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_device.h | 9 ++++\n> src/libcamera/v4l2_device.cpp | 72 +++++++++++++++++++++++++++++\n> 2 files changed, 81 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 75f23a05b903..91a72fcecbcc 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -7,18 +7,23 @@\n> #ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> #define __LIBCAMERA_V4L2_DEVICE_H__\n> \n> +#include \n> #include \n> \n> #include \"log.h\"\n> \n> namespace libcamera {\n> \n> +class V4L2ControlInfo;\n> +\n> class V4L2Device : protected Loggable\n> {\n> public:\n> \tvoid close();\n> \tbool isOpen() const { return fd_ != -1; }\n> \n> +\tconst V4L2ControlInfo *getControlInfo(unsigned int id) const;\n> +\n> \tconst std::string &deviceNode() const { return deviceNode_; }\n> \n> protected:\n> @@ -32,6 +37,10 @@ protected:\n> \tint fd() { return fd_; }\n> \n> private:\n> +\tvoid listControls();\n> +\tint validateControlType(const V4L2ControlInfo *info);\n> +\n> +\tstd::map controls_;\n> \tstd::string deviceNode_;\n> \tint fd_;\n> };\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 99621a724b96..b113ff77e3cf 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -13,6 +13,7 @@\n> #include \n> \n> #include \"log.h\"\n> +#include \"v4l2_controls.h\"\n> \n> /**\n> * \\file v4l2_device.h\n> @@ -82,6 +83,8 @@ int V4L2Device::open(unsigned int flags)\n> \n> \tfd_ = ret;\n> \n> +\tlistControls();\n> +\n> \treturn 0;\n> }\n> \n> @@ -107,6 +110,21 @@ void V4L2Device::close()\n> * \\return True if the V4L2 device node is open, false otherwise\n> */\n> \n> +/**\n> + * \\brief Retrieve information about a control\n> + * \\param[in] id The control ID\n> + * \\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\n> + * if the device doesn't have such a control\n> + */\n> +const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const\n> +{\n> +\tauto it = controls_.find(id);\n> +\tif (it == controls_.end())\n> +\t\treturn nullptr;\n> +\n> +\treturn &it->second;\n> +}\n> +\n> /**\n> * \\brief Perform an IOCTL system call on the device node\n> * \\param[in] request The IOCTL request code\n> @@ -137,4 +155,58 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> * \\return The V4L2 device file descriptor, -1 if the device node is not open\n> */\n> \n> +/*\n> + * \\brief List and store information about all controls supported by the\n> + * V4L2 device\n> + */\n> +void V4L2Device::listControls()\n> +{\n> +\tstruct v4l2_query_ext_ctrl ctrl = {};\n> +\n> +\t/* \\todo Add support for menu and compound controls. */\n> +\tctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;\n> +\twhile (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {\n> +\t\tif (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||\n> +\t\t ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {\n> +\t\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tV4L2ControlInfo info(ctrl);\n> +\t\tif (validateControlType(&info))\n> +\t\t\tcontinue;\n> +\n> +\t\tcontrols_.insert(std::pair\n> +\t\t\t\t (ctrl.id, info));\n\nAnything wrong with\n\n\t\tcontrols_[ctrl.id] = info;\n\n?\n\n> +\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> +\t}\n> +}\n> +\n> +/*\n> + * \\brief Make sure the control type is supported\n> + * \\param[in] info The V4L2ControlInfo to inspect type of\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EINVAL The control type is not supported\n> + */\n> +int V4L2Device::validateControlType(const V4L2ControlInfo *info)\n\nAs this method has a single user, how about inlining it ?\nV4L2Device::listControls() isn't big.\n\n> +{\n> +\t/* \\todo Support compound controls. */\n> +\tswitch (info->type()) {\n> +\tcase V4L2_CTRL_TYPE_INTEGER:\n> +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> +\tcase V4L2_CTRL_TYPE_MENU:\n> +\tcase V4L2_CTRL_TYPE_BUTTON:\n> +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\tcase V4L2_CTRL_TYPE_BITMASK:\n> +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> +\t\t\t\t << \"' not supported\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> } /* namespace libcamera */","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 CC4BE61580\n\tfor ;\n\tSat, 22 Jun 2019 19:17:10 +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 49DF267;\n\tSat, 22 Jun 2019 19:17:10 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561223830;\n\tbh=ZXL/fJSjAeBA0eDst5qoJavNqS929XyuqPbP2kn88x8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZEPMEUnZn2sq3aB5UB1F0uJHpEaD+9CzPaWG5y+f/LuZ0usn2pCcrEirPa63ynStv\n\t2Ge2NkbTruqsUuzXn+vuNSfkfF3EDht9k1TSU9CDvRkCAmdP1Ox1CBj+NhnuXaDRuY\n\tKNBCHFqKhP5sDcrP3O9whiakn0tehPP6E/TOdY5M=","Date":"Sat, 22 Jun 2019 20:16:52 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190622171652.GC8156@pendragon.ideasonboard.com>","References":"<20190620153144.5394-1-jacopo@jmondi.org>\n\t<20190620153144.5394-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190620153144.5394-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","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 17:17:11 -0000"}},{"id":1988,"web_url":"https://patchwork.libcamera.org/comment/1988/","msgid":"<20190624081917.so5rphwwcsy7cpkl@uno.localdomain>","date":"2019-06-24T08:19:17","subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","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 08:16:52PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Jun 20, 2019 at 05:31:40PM +0200, Jacopo Mondi wrote:\n> > Enumerate all the valid controls a device supports at open() time.\n> > A control is valid only if its type is supported.\n> >\n> > Store the control informations in a map inside the device to save\n>\n> s/informations/information/\n>\n> https://en.wiktionary.org/wiki/informations\n>\n> \"Most senses of the word “information” are uncountable. The legal sense,\n> referring to court filings, is one that does form a plural.\"\n>\n> > querying the control when setting or getting its value from the device\n> > and provide an operation to retrieve information by control ID.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_device.h | 9 ++++\n> > src/libcamera/v4l2_device.cpp | 72 +++++++++++++++++++++++++++++\n> > 2 files changed, 81 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index 75f23a05b903..91a72fcecbcc 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -7,18 +7,23 @@\n> > #ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> > #define __LIBCAMERA_V4L2_DEVICE_H__\n> >\n> > +#include \n> > #include \n> >\n> > #include \"log.h\"\n> >\n> > namespace libcamera {\n> >\n> > +class V4L2ControlInfo;\n> > +\n> > class V4L2Device : protected Loggable\n> > {\n> > public:\n> > \tvoid close();\n> > \tbool isOpen() const { return fd_ != -1; }\n> >\n> > +\tconst V4L2ControlInfo *getControlInfo(unsigned int id) const;\n> > +\n> > \tconst std::string &deviceNode() const { return deviceNode_; }\n> >\n> > protected:\n> > @@ -32,6 +37,10 @@ protected:\n> > \tint fd() { return fd_; }\n> >\n> > private:\n> > +\tvoid listControls();\n> > +\tint validateControlType(const V4L2ControlInfo *info);\n> > +\n> > +\tstd::map controls_;\n> > \tstd::string deviceNode_;\n> > \tint fd_;\n> > };\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 99621a724b96..b113ff77e3cf 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -13,6 +13,7 @@\n> > #include \n> >\n> > #include \"log.h\"\n> > +#include \"v4l2_controls.h\"\n> >\n> > /**\n> > * \\file v4l2_device.h\n> > @@ -82,6 +83,8 @@ int V4L2Device::open(unsigned int flags)\n> >\n> > \tfd_ = ret;\n> >\n> > +\tlistControls();\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -107,6 +110,21 @@ void V4L2Device::close()\n> > * \\return True if the V4L2 device node is open, false otherwise\n> > */\n> >\n> > +/**\n> > + * \\brief Retrieve information about a control\n> > + * \\param[in] id The control ID\n> > + * \\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\n> > + * if the device doesn't have such a control\n> > + */\n> > +const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const\n> > +{\n> > +\tauto it = controls_.find(id);\n> > +\tif (it == controls_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn &it->second;\n> > +}\n> > +\n> > /**\n> > * \\brief Perform an IOCTL system call on the device node\n> > * \\param[in] request The IOCTL request code\n> > @@ -137,4 +155,58 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> > * \\return The V4L2 device file descriptor, -1 if the device node is not open\n> > */\n> >\n> > +/*\n> > + * \\brief List and store information about all controls supported by the\n> > + * V4L2 device\n> > + */\n> > +void V4L2Device::listControls()\n> > +{\n> > +\tstruct v4l2_query_ext_ctrl ctrl = {};\n> > +\n> > +\t/* \\todo Add support for menu and compound controls. */\n> > +\tctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;\n> > +\twhile (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {\n> > +\t\tif (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||\n> > +\t\t ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {\n> > +\t\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\n> > +\t\tV4L2ControlInfo info(ctrl);\n> > +\t\tif (validateControlType(&info))\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tcontrols_.insert(std::pair\n> > +\t\t\t\t (ctrl.id, info));\n>\n> Anything wrong with\n>\n> \t\tcontrols_[ctrl.id] = info;\n>\n> ?\n\nNo, I could change it. I used std::vector::insert when I was\nconstructing the V4L2ControlInfo directly in the operation call, it's\na leftover.\n\n>\n> > +\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> > +\t}\n> > +}\n> > +\n> > +/*\n> > + * \\brief Make sure the control type is supported\n> > + * \\param[in] info The V4L2ControlInfo to inspect type of\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -EINVAL The control type is not supported\n> > + */\n> > +int V4L2Device::validateControlType(const V4L2ControlInfo *info)\n>\n> As this method has a single user, how about inlining it ?\n> V4L2Device::listControls() isn't big.\n>\n\nI would rather keep the type validation centralized in one place, and in\nthe caller it's easy enough to parse if (validateControlType(type))\nfor a reader than going through the switch cases.\n\nThe cost of a function call is negligible considering this happens\nonly at device open time.\n\nThanks\n j\n\n> > +{\n> > +\t/* \\todo Support compound controls. */\n> > +\tswitch (info->type()) {\n> > +\tcase V4L2_CTRL_TYPE_INTEGER:\n> > +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > +\tcase V4L2_CTRL_TYPE_MENU:\n> > +\tcase V4L2_CTRL_TYPE_BUTTON:\n> > +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\tcase V4L2_CTRL_TYPE_BITMASK:\n> > +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> > +\t\t\t\t << \"' not supported\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > } /* namespace libcamera */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65C4160C29\n\tfor ;\n\tMon, 24 Jun 2019 10:18:03 +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 relay12.mail.gandi.net (Postfix) with ESMTPSA id BE621200013;\n\tMon, 24 Jun 2019 08:18:01 +0000 (UTC)"],"Date":"Mon, 24 Jun 2019 10:19:17 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190624081917.so5rphwwcsy7cpkl@uno.localdomain>","References":"<20190620153144.5394-1-jacopo@jmondi.org>\n\t<20190620153144.5394-3-jacopo@jmondi.org>\n\t<20190622171652.GC8156@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"n7tnxl4r2afkvghw\"","Content-Disposition":"inline","In-Reply-To":"<20190622171652.GC8156@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","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:18:03 -0000"}},{"id":1995,"web_url":"https://patchwork.libcamera.org/comment/1995/","msgid":"<20190624092612.g6oedjnzvwgzsb4m@uno.localdomain>","date":"2019-06-24T09:26:12","subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Jun 24, 2019 at 10:19:17AM +0200, Jacopo Mondi wrote:\n> Hi Laurent,\n>\n> On Sat, Jun 22, 2019 at 08:16:52PM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Thu, Jun 20, 2019 at 05:31:40PM +0200, Jacopo Mondi wrote:\n> > > Enumerate all the valid controls a device supports at open() time.\n> > > A control is valid only if its type is supported.\n> > >\n> > > Store the control informations in a map inside the device to save\n> >\n> > s/informations/information/\n> >\n> > https://en.wiktionary.org/wiki/informations\n> >\n> > \"Most senses of the word “information” are uncountable. The legal sense,\n> > referring to court filings, is one that does form a plural.\"\n> >\n> > > querying the control when setting or getting its value from the device\n> > > and provide an operation to retrieve information by control ID.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/include/v4l2_device.h | 9 ++++\n> > > src/libcamera/v4l2_device.cpp | 72 +++++++++++++++++++++++++++++\n> > > 2 files changed, 81 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > > index 75f23a05b903..91a72fcecbcc 100644\n> > > --- a/src/libcamera/include/v4l2_device.h\n> > > +++ b/src/libcamera/include/v4l2_device.h\n> > > @@ -7,18 +7,23 @@\n> > > #ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> > > #define __LIBCAMERA_V4L2_DEVICE_H__\n> > >\n> > > +#include \n> > > #include \n> > >\n> > > #include \"log.h\"\n> > >\n> > > namespace libcamera {\n> > >\n> > > +class V4L2ControlInfo;\n> > > +\n> > > class V4L2Device : protected Loggable\n> > > {\n> > > public:\n> > > \tvoid close();\n> > > \tbool isOpen() const { return fd_ != -1; }\n> > >\n> > > +\tconst V4L2ControlInfo *getControlInfo(unsigned int id) const;\n> > > +\n> > > \tconst std::string &deviceNode() const { return deviceNode_; }\n> > >\n> > > protected:\n> > > @@ -32,6 +37,10 @@ protected:\n> > > \tint fd() { return fd_; }\n> > >\n> > > private:\n> > > +\tvoid listControls();\n> > > +\tint validateControlType(const V4L2ControlInfo *info);\n> > > +\n> > > +\tstd::map controls_;\n> > > \tstd::string deviceNode_;\n> > > \tint fd_;\n> > > };\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index 99621a724b96..b113ff77e3cf 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -13,6 +13,7 @@\n> > > #include \n> > >\n> > > #include \"log.h\"\n> > > +#include \"v4l2_controls.h\"\n> > >\n> > > /**\n> > > * \\file v4l2_device.h\n> > > @@ -82,6 +83,8 @@ int V4L2Device::open(unsigned int flags)\n> > >\n> > > \tfd_ = ret;\n> > >\n> > > +\tlistControls();\n> > > +\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -107,6 +110,21 @@ void V4L2Device::close()\n> > > * \\return True if the V4L2 device node is open, false otherwise\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Retrieve information about a control\n> > > + * \\param[in] id The control ID\n> > > + * \\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\n> > > + * if the device doesn't have such a control\n> > > + */\n> > > +const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const\n> > > +{\n> > > +\tauto it = controls_.find(id);\n> > > +\tif (it == controls_.end())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn &it->second;\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Perform an IOCTL system call on the device node\n> > > * \\param[in] request The IOCTL request code\n> > > @@ -137,4 +155,58 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> > > * \\return The V4L2 device file descriptor, -1 if the device node is not open\n> > > */\n> > >\n> > > +/*\n> > > + * \\brief List and store information about all controls supported by the\n> > > + * V4L2 device\n> > > + */\n> > > +void V4L2Device::listControls()\n> > > +{\n> > > +\tstruct v4l2_query_ext_ctrl ctrl = {};\n> > > +\n> > > +\t/* \\todo Add support for menu and compound controls. */\n> > > +\tctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;\n> > > +\twhile (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {\n> > > +\t\tif (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||\n> > > +\t\t ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {\n> > > +\t\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> > > +\t\t\tcontinue;\n> > > +\t\t}\n> > > +\n> > > +\t\tV4L2ControlInfo info(ctrl);\n> > > +\t\tif (validateControlType(&info))\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tcontrols_.insert(std::pair\n> > > +\t\t\t\t (ctrl.id, info));\n> >\n> > Anything wrong with\n> >\n> > \t\tcontrols_[ctrl.id] = info;\n> >\n> > ?\n>\n> No, I could change it. I used std::vector::insert when I was\n> constructing the V4L2ControlInfo directly in the operation call, it's\n> a leftover.\n\nAdtually, I'm a bit puzzled here. My understanding is that\n \t\tcontrols_[ctrl.id] = info;\n\nshould insert a new element by calling the copy constructor of\nV4L2ControlInfo (which is defaulted and accessible)\n\nHowevere, I get this back from the compiler\n\n/usr/include/c++/8.3.0/tuple:1668:70: error: no matching function for call to ‘libcamera::V4L2ControlInfo::V4L2ControlInfo()’\n second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)\n ^\nIn file included from ../src/libcamera/v4l2_device.cpp:16:\n../src/libcamera/include/v4l2_controls.h:25:2: note: candidate: ‘libcamera::V4L2ControlInfo::V4L2ControlInfo(const libcamera::V4L2ControlInfo&)’\n V4L2ControlInfo(const V4L2ControlInfo &) = default;\n ^~~~~~~~~~~~~~~\n../src/libcamera/include/v4l2_controls.h:25:2: note: candidate expects 1 argument, 0 provided\n../src/libcamera/include/v4l2_controls.h:24:2: note: candidate: ‘libcamera::V4L2ControlInfo::V4L2ControlInfo(const v4l2_query_ext_ctrl&)’\n V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n ^~~~~~~~~~~~~~~\n../src/libcamera/include/v4l2_controls.h:24:2: note: candidate expects 1 argument, 0 provided\n\n\nI'm now keeping the explicit ::insert() call for now.\n\nThanks\n j\n\n>\n> >\n> > > +\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> > > +\t}\n> > > +}\n> > > +\n> > > +/*\n> > > + * \\brief Make sure the control type is supported\n> > > + * \\param[in] info The V4L2ControlInfo to inspect type of\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + * \\retval -EINVAL The control type is not supported\n> > > + */\n> > > +int V4L2Device::validateControlType(const V4L2ControlInfo *info)\n> >\n> > As this method has a single user, how about inlining it ?\n> > V4L2Device::listControls() isn't big.\n> >\n>\n> I would rather keep the type validation centralized in one place, and in\n> the caller it's easy enough to parse if (validateControlType(type))\n> for a reader than going through the switch cases.\n>\n> The cost of a function call is negligible considering this happens\n> only at device open time.\n>\n> Thanks\n> j\n>\n> > > +{\n> > > +\t/* \\todo Support compound controls. */\n> > > +\tswitch (info->type()) {\n> > > +\tcase V4L2_CTRL_TYPE_INTEGER:\n> > > +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> > > +\tcase V4L2_CTRL_TYPE_MENU:\n> > > +\tcase V4L2_CTRL_TYPE_BUTTON:\n> > > +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > > +\tcase V4L2_CTRL_TYPE_BITMASK:\n> > > +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> > > +\t\t\t\t << \"' not supported\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > } /* namespace libcamera */\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n\n\n\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2290760C29\n\tfor ;\n\tMon, 24 Jun 2019 11:24:57 +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 relay6-d.mail.gandi.net (Postfix) with ESMTPSA id A5165C000D;\n\tMon, 24 Jun 2019 09:24:56 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 24 Jun 2019 11:26:12 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190624092612.g6oedjnzvwgzsb4m@uno.localdomain>","References":"<20190620153144.5394-1-jacopo@jmondi.org>\n\t<20190620153144.5394-3-jacopo@jmondi.org>\n\t<20190622171652.GC8156@pendragon.ideasonboard.com>\n\t<20190624081917.so5rphwwcsy7cpkl@uno.localdomain>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"lbcgrcmzzy7ypbhv\"","Content-Disposition":"inline","In-Reply-To":"<20190624081917.so5rphwwcsy7cpkl@uno.localdomain>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","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 09:24:57 -0000"}},{"id":1999,"web_url":"https://patchwork.libcamera.org/comment/1999/","msgid":"<20190624114919.GG5737@pendragon.ideasonboard.com>","date":"2019-06-24T11:49:19","subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jun 24, 2019 at 11:26:12AM +0200, Jacopo Mondi wrote:\n> On Mon, Jun 24, 2019 at 10:19:17AM +0200, Jacopo Mondi wrote:\n> > On Sat, Jun 22, 2019 at 08:16:52PM +0300, Laurent Pinchart wrote:\n> >> On Thu, Jun 20, 2019 at 05:31:40PM +0200, Jacopo Mondi wrote:\n> >>> Enumerate all the valid controls a device supports at open() time.\n> >>> A control is valid only if its type is supported.\n> >>>\n> >>> Store the control informations in a map inside the device to save\n> >>\n> >> s/informations/information/\n> >>\n> >> https://en.wiktionary.org/wiki/informations\n> >>\n> >> \"Most senses of the word “information” are uncountable. The legal sense,\n> >> referring to court filings, is one that does form a plural.\"\n> >>\n> >>> querying the control when setting or getting its value from the device\n> >>> and provide an operation to retrieve information by control ID.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> src/libcamera/include/v4l2_device.h | 9 ++++\n> >>> src/libcamera/v4l2_device.cpp | 72 +++++++++++++++++++++++++++++\n> >>> 2 files changed, 81 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >>> index 75f23a05b903..91a72fcecbcc 100644\n> >>> --- a/src/libcamera/include/v4l2_device.h\n> >>> +++ b/src/libcamera/include/v4l2_device.h\n> >>> @@ -7,18 +7,23 @@\n> >>> #ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> >>> #define __LIBCAMERA_V4L2_DEVICE_H__\n> >>>\n> >>> +#include \n> >>> #include \n> >>>\n> >>> #include \"log.h\"\n> >>>\n> >>> namespace libcamera {\n> >>>\n> >>> +class V4L2ControlInfo;\n> >>> +\n> >>> class V4L2Device : protected Loggable\n> >>> {\n> >>> public:\n> >>> \tvoid close();\n> >>> \tbool isOpen() const { return fd_ != -1; }\n> >>>\n> >>> +\tconst V4L2ControlInfo *getControlInfo(unsigned int id) const;\n> >>> +\n> >>> \tconst std::string &deviceNode() const { return deviceNode_; }\n> >>>\n> >>> protected:\n> >>> @@ -32,6 +37,10 @@ protected:\n> >>> \tint fd() { return fd_; }\n> >>>\n> >>> private:\n> >>> +\tvoid listControls();\n> >>> +\tint validateControlType(const V4L2ControlInfo *info);\n> >>> +\n> >>> +\tstd::map controls_;\n> >>> \tstd::string deviceNode_;\n> >>> \tint fd_;\n> >>> };\n> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>> index 99621a724b96..b113ff77e3cf 100644\n> >>> --- a/src/libcamera/v4l2_device.cpp\n> >>> +++ b/src/libcamera/v4l2_device.cpp\n> >>> @@ -13,6 +13,7 @@\n> >>> #include \n> >>>\n> >>> #include \"log.h\"\n> >>> +#include \"v4l2_controls.h\"\n> >>>\n> >>> /**\n> >>> * \\file v4l2_device.h\n> >>> @@ -82,6 +83,8 @@ int V4L2Device::open(unsigned int flags)\n> >>>\n> >>> \tfd_ = ret;\n> >>>\n> >>> +\tlistControls();\n> >>> +\n> >>> \treturn 0;\n> >>> }\n> >>>\n> >>> @@ -107,6 +110,21 @@ void V4L2Device::close()\n> >>> * \\return True if the V4L2 device node is open, false otherwise\n> >>> */\n> >>>\n> >>> +/**\n> >>> + * \\brief Retrieve information about a control\n> >>> + * \\param[in] id The control ID\n> >>> + * \\return A pointer to the V4L2ControlInfo for control \\a id, or nullptr\n> >>> + * if the device doesn't have such a control\n> >>> + */\n> >>> +const V4L2ControlInfo *V4L2Device::getControlInfo(unsigned int id) const\n> >>> +{\n> >>> +\tauto it = controls_.find(id);\n> >>> +\tif (it == controls_.end())\n> >>> +\t\treturn nullptr;\n> >>> +\n> >>> +\treturn &it->second;\n> >>> +}\n> >>> +\n> >>> /**\n> >>> * \\brief Perform an IOCTL system call on the device node\n> >>> * \\param[in] request The IOCTL request code\n> >>> @@ -137,4 +155,58 @@ int V4L2Device::ioctl(unsigned long request, void *argp)\n> >>> * \\return The V4L2 device file descriptor, -1 if the device node is not open\n> >>> */\n> >>>\n> >>> +/*\n> >>> + * \\brief List and store information about all controls supported by the\n> >>> + * V4L2 device\n> >>> + */\n> >>> +void V4L2Device::listControls()\n> >>> +{\n> >>> +\tstruct v4l2_query_ext_ctrl ctrl = {};\n> >>> +\n> >>> +\t/* \\todo Add support for menu and compound controls. */\n> >>> +\tctrl.id = V4L2_CTRL_FLAG_NEXT_CTRL;\n> >>> +\twhile (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl) == 0) {\n> >>> +\t\tif (ctrl.type == V4L2_CTRL_TYPE_CTRL_CLASS ||\n> >>> +\t\t ctrl.flags & V4L2_CTRL_FLAG_DISABLED) {\n> >>> +\t\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> >>> +\t\t\tcontinue;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\tV4L2ControlInfo info(ctrl);\n> >>> +\t\tif (validateControlType(&info))\n> >>> +\t\t\tcontinue;\n> >>> +\n> >>> +\t\tcontrols_.insert(std::pair\n> >>> +\t\t\t\t (ctrl.id, info));\n> >>\n> >> Anything wrong with\n> >>\n> >> \t\tcontrols_[ctrl.id] = info;\n> >>\n> >> ?\n> >\n> > No, I could change it. I used std::vector::insert when I was\n> > constructing the V4L2ControlInfo directly in the operation call, it's\n> > a leftover.\n> \n> Adtually, I'm a bit puzzled here. My understanding is that\n> \t\tcontrols_[ctrl.id] = info;\n> \n> should insert a new element by calling the copy constructor of\n> V4L2ControlInfo (which is defaulted and accessible)\n> \n> Howevere, I get this back from the compiler\n> \n> /usr/include/c++/8.3.0/tuple:1668:70: error: no matching function for call to ‘libcamera::V4L2ControlInfo::V4L2ControlInfo()’\n> second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)\n> ^\n> In file included from ../src/libcamera/v4l2_device.cpp:16:\n> ../src/libcamera/include/v4l2_controls.h:25:2: note: candidate: ‘libcamera::V4L2ControlInfo::V4L2ControlInfo(const libcamera::V4L2ControlInfo&)’\n> V4L2ControlInfo(const V4L2ControlInfo &) = default;\n> ^~~~~~~~~~~~~~~\n> ../src/libcamera/include/v4l2_controls.h:25:2: note: candidate expects 1 argument, 0 provided\n> ../src/libcamera/include/v4l2_controls.h:24:2: note: candidate: ‘libcamera::V4L2ControlInfo::V4L2ControlInfo(const v4l2_query_ext_ctrl&)’\n> V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n> ^~~~~~~~~~~~~~~\n> ../src/libcamera/include/v4l2_controls.h:24:2: note: candidate expects 1 argument, 0 provided\n\ncontrols_[ctrl.id] will create an empty element, and thus needs a\ndefault constructor that V4L2ControlInfo doesn't provide. The operator\nreturns a reference to the newly created element, and the copy operator=\nis then invoked. The copy constructor isn't involved at any time in that\nline.\n\n> I'm now keeping the explicit ::insert() call for now.\n\nIf you want to make it efficient, use map::emplace()\n\n\tcontrols_.emplace(ctrl.id, info);\n\n> >>> +\t\tctrl.id |= V4L2_CTRL_FLAG_NEXT_CTRL;\n> >>> +\t}\n> >>> +}\n> >>> +\n> >>> +/*\n> >>> + * \\brief Make sure the control type is supported\n> >>> + * \\param[in] info The V4L2ControlInfo to inspect type of\n> >>> + * \\return 0 on success or a negative error code otherwise\n> >>> + * \\retval -EINVAL The control type is not supported\n> >>> + */\n> >>> +int V4L2Device::validateControlType(const V4L2ControlInfo *info)\n> >>\n> >> As this method has a single user, how about inlining it ?\n> >> V4L2Device::listControls() isn't big.\n> >\n> > I would rather keep the type validation centralized in one place, and in\n> > the caller it's easy enough to parse if (validateControlType(type))\n> > for a reader than going through the switch cases.\n> >\n> > The cost of a function call is negligible considering this happens\n> > only at device open time.\n\nIt's not about the cost of a function call, I would find it more\nreadable :-)\n\n> >>> +{\n> >>> +\t/* \\todo Support compound controls. */\n> >>> +\tswitch (info->type()) {\n> >>> +\tcase V4L2_CTRL_TYPE_INTEGER:\n> >>> +\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> >>> +\tcase V4L2_CTRL_TYPE_MENU:\n> >>> +\tcase V4L2_CTRL_TYPE_BUTTON:\n> >>> +\tcase V4L2_CTRL_TYPE_INTEGER64:\n> >>> +\tcase V4L2_CTRL_TYPE_BITMASK:\n> >>> +\tcase V4L2_CTRL_TYPE_INTEGER_MENU:\n> >>> +\t\tbreak;\n> >>> +\tdefault:\n> >>> +\t\tLOG(V4L2, Error) << \"Control type '\" << info->type()\n> >>> +\t\t\t\t << \"' not supported\";\n> >>> +\t\treturn -EINVAL;\n> >>> +\t}\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> } /* namespace libcamera */","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 645D9600EB\n\tfor ;\n\tMon, 24 Jun 2019 13:49:38 +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 D7260323;\n\tMon, 24 Jun 2019 13:49:37 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561376978;\n\tbh=Kz8T72afAV00HwgyUAJsz44i6Js7MDe3FypVxR8nnuc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S2/CBoZ2gM0QOhIz2UPAwdN38D/s2OB6nMTFC43g2fgcCfworRZ9snT88FPWqMe3L\n\tNV8xkO0TAC+9j3OK2upl16XgGkU1dCiZIxnd1kdN+g8YOBtF3LvjYwk4gosLpSJb+U\n\tpgydnnygcgLwl6kRFS79P264N2XySWSjBM3nBjL8=","Date":"Mon, 24 Jun 2019 14:49:19 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190624114919.GG5737@pendragon.ideasonboard.com>","References":"<20190620153144.5394-1-jacopo@jmondi.org>\n\t<20190620153144.5394-3-jacopo@jmondi.org>\n\t<20190622171652.GC8156@pendragon.ideasonboard.com>\n\t<20190624081917.so5rphwwcsy7cpkl@uno.localdomain>\n\t<20190624092612.g6oedjnzvwgzsb4m@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190624092612.g6oedjnzvwgzsb4m@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 2/6] libcamera: v4l2_device: List\n\tvalid controls at open","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 11:49:38 -0000"}}]