[{"id":13383,"web_url":"https://patchwork.libcamera.org/comment/13383/","msgid":"<20201022015146.GV3942@pendragon.ideasonboard.com>","date":"2020-10-22T01:51:46","subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: controls: Add\n\tsupported values to ControlInfo","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 Wed, Oct 21, 2020 at 04:36:24PM +0200, Jacopo Mondi wrote:\n> Add to the ControlInfo class a list of supported values that can be\n> provided at construction time and retrieved through an accessor method.\n> \n> This is meant to support controls that have an enumerated list of\n> supported values.\n> \n> Reviewed-by: Kieran Bingham \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 5 ++++-\n> src/libcamera/controls.cpp | 22 +++++++++++++++++++---\n> 2 files changed, 23 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 80944efc133a..d1f6d4490c35 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -267,11 +267,13 @@ class ControlInfo\n> public:\n> \texplicit ControlInfo(const ControlValue &min = 0,\n> \t\t\t const ControlValue &max = 0,\n> -\t\t\t const ControlValue &def = 0);\n> +\t\t\t const ControlValue &def = 0,\n> +\t\t\t const std::vector &values = {});\n\nI don't think we should add a list of values to this constructor.\nEnumerated control types have their minimum and maximum implicitly\ndefined by the supported values. Patch 04/14 adds new constructors which\nlook right to me, I don't really see the use case for this one here.\n\nAnd dropping this, I think you can squash 03/14 and 04/14 together.\n\n> \n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n> \tconst ControlValue &def() const { return def_; }\n> +\tconst std::vector &values() const { return values_; }\n> \n> \tstd::string toString() const;\n> \n> @@ -289,6 +291,7 @@ private:\n> \tControlValue min_;\n> \tControlValue max_;\n> \tControlValue def_;\n> +\tstd::vector values_;\n> };\n> \n> using ControlIdMap = std::unordered_map;\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index dca782667d88..61feee37a1b8 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -479,15 +479,17 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> */\n> \n> /**\n> - * \\brief Construct a ControlInfo with minimum and maximum range parameters\n> + * \\brief Construct a ControlInfo with parameters\n> * \\param[in] min The control minimum value\n> * \\param[in] max The control maximum value\n> * \\param[in] def The control default value\n> + * \\param[in] values The control supported values\n> */\n> ControlInfo::ControlInfo(const ControlValue &min,\n> \t\t\t const ControlValue &max,\n> -\t\t\t const ControlValue &def)\n> -\t: min_(min), max_(max), def_(def)\n> +\t\t\t const ControlValue &def,\n> +\t\t\t const std::vector &values)\n> +\t: min_(min), max_(max), def_(def), values_(values)\n> {\n> }\n> \n> @@ -519,6 +521,20 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> * \\return A ControlValue with the default value for the control\n> */\n> \n> +/**\n> + * \\fn ControlInfo::values()\n> + * \\brief Retrieve the values supported by the control\n> + *\n> + * For controls that support a pre-defined number of values, the enumeration of\n> + * those is reported through a vector of ControlValue instances accessible with\n> + * this method.\n\nShould we explicitly define a concept of enumerated controls in the\ndocumentation ? I'm thinking it would be useful to add a flag passed to\nconstructors of both Control and ControlId, and recorded in ControlId,\nto tell that the control is an enum.\n\nIf we define such a concept in the Control class, then this function can\njust refer to it by saying it reports valid values for enumerated\ncontrols. I think that would be better as it would expose the concept of\nenumerated controls in a more visible place instead of having it more\nhidden here.\n\nI can help write documentation if needed.\n\n> + *\n> + * If the control reports a list of supported values, setting values outside\n> + * of the reported ones results in undefined behaviour.\n\nI think this belongs to Control::set().\n\n> + *\n> + * \\return A vector of ControlValue instances with the supported values\n> + */\n> +\n> /**\n> * \\brief Provide a string representation of the ControlInfo\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 D07C6C3D3C\n\tfor ;\n\tThu, 22 Oct 2020 01:52:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 561756112D;\n\tThu, 22 Oct 2020 03:52:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1F856034F\n\tfor ;\n\tThu, 22 Oct 2020 03:52:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 346BC53;\n\tThu, 22 Oct 2020 03:52:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TtesGX+p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603331552;\n\tbh=UN1PuJOL/F2JU3sFd+dUvn6NMLEsRq76kkn4cVMOaI0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TtesGX+pd2V7170RkxPZRg93CKCAE5XQ5pHdC90mnV2walVLVcf+OUpanM6GxW9nM\n\ttfGI1euiEmFeEZagRO7BJZKXe3oMq5nrP55GD4UM0365+kRih4LwPxJQ5gqXpOeIFa\n\tFlNyUTMKRSrAxtSmKJv+Zf+szed59b7Sm150fWcw=","Date":"Thu, 22 Oct 2020 04:51:46 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201022015146.GV3942@pendragon.ideasonboard.com>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201021143635.22846-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: controls: Add\n\tsupported values to ControlInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13385,"web_url":"https://patchwork.libcamera.org/comment/13385/","msgid":"<20201022023859.GB14061@pendragon.ideasonboard.com>","date":"2020-10-22T02:38:59","subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: controls: Add\n\tsupported values to ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nAnother small comment.\n\nOn Thu, Oct 22, 2020 at 04:51:46AM +0300, Laurent Pinchart wrote:\n> On Wed, Oct 21, 2020 at 04:36:24PM +0200, Jacopo Mondi wrote:\n> > Add to the ControlInfo class a list of supported values that can be\n> > provided at construction time and retrieved through an accessor method.\n> > \n> > This is meant to support controls that have an enumerated list of\n> > supported values.\n> > \n> > Reviewed-by: Kieran Bingham \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 5 ++++-\n> > src/libcamera/controls.cpp | 22 +++++++++++++++++++---\n> > 2 files changed, 23 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 80944efc133a..d1f6d4490c35 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -267,11 +267,13 @@ class ControlInfo\n> > public:\n> > \texplicit ControlInfo(const ControlValue &min = 0,\n> > \t\t\t const ControlValue &max = 0,\n> > -\t\t\t const ControlValue &def = 0);\n> > +\t\t\t const ControlValue &def = 0,\n> > +\t\t\t const std::vector &values = {});\n\nYou need to include vector at the beginning of this file. clang\ncomplains about it.\n\n> I don't think we should add a list of values to this constructor.\n> Enumerated control types have their minimum and maximum implicitly\n> defined by the supported values. Patch 04/14 adds new constructors which\n> look right to me, I don't really see the use case for this one here.\n> \n> And dropping this, I think you can squash 03/14 and 04/14 together.\n> \n> > \n> > \tconst ControlValue &min() const { return min_; }\n> > \tconst ControlValue &max() const { return max_; }\n> > \tconst ControlValue &def() const { return def_; }\n> > +\tconst std::vector &values() const { return values_; }\n> > \n> > \tstd::string toString() const;\n> > \n> > @@ -289,6 +291,7 @@ private:\n> > \tControlValue min_;\n> > \tControlValue max_;\n> > \tControlValue def_;\n> > +\tstd::vector values_;\n> > };\n> > \n> > using ControlIdMap = std::unordered_map;\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index dca782667d88..61feee37a1b8 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -479,15 +479,17 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> > */\n> > \n> > /**\n> > - * \\brief Construct a ControlInfo with minimum and maximum range parameters\n> > + * \\brief Construct a ControlInfo with parameters\n> > * \\param[in] min The control minimum value\n> > * \\param[in] max The control maximum value\n> > * \\param[in] def The control default value\n> > + * \\param[in] values The control supported values\n> > */\n> > ControlInfo::ControlInfo(const ControlValue &min,\n> > \t\t\t const ControlValue &max,\n> > -\t\t\t const ControlValue &def)\n> > -\t: min_(min), max_(max), def_(def)\n> > +\t\t\t const ControlValue &def,\n> > +\t\t\t const std::vector &values)\n> > +\t: min_(min), max_(max), def_(def), values_(values)\n> > {\n> > }\n> > \n> > @@ -519,6 +521,20 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > * \\return A ControlValue with the default value for the control\n> > */\n> > \n> > +/**\n> > + * \\fn ControlInfo::values()\n> > + * \\brief Retrieve the values supported by the control\n> > + *\n> > + * For controls that support a pre-defined number of values, the enumeration of\n> > + * those is reported through a vector of ControlValue instances accessible with\n> > + * this method.\n> \n> Should we explicitly define a concept of enumerated controls in the\n> documentation ? I'm thinking it would be useful to add a flag passed to\n> constructors of both Control and ControlId, and recorded in ControlId,\n> to tell that the control is an enum.\n> \n> If we define such a concept in the Control class, then this function can\n> just refer to it by saying it reports valid values for enumerated\n> controls. I think that would be better as it would expose the concept of\n> enumerated controls in a more visible place instead of having it more\n> hidden here.\n> \n> I can help write documentation if needed.\n> \n> > + *\n> > + * If the control reports a list of supported values, setting values outside\n> > + * of the reported ones results in undefined behaviour.\n> \n> I think this belongs to Control::set().\n> \n> > + *\n> > + * \\return A vector of ControlValue instances with the supported values\n> > + */\n> > +\n> > /**\n> > * \\brief Provide a string representation of the ControlInfo\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 51906C3D3C\n\tfor ;\n\tThu, 22 Oct 2020 02:39:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8021D61059;\n\tThu, 22 Oct 2020 04:39:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46DCE6034F\n\tfor ;\n\tThu, 22 Oct 2020 04:39:46 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFBE553;\n\tThu, 22 Oct 2020 04:39:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZrTiq2N2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603334385;\n\tbh=lVs9b7D0yADWitVIv6xFWCcaNnmM5SFItLqPQEBDYp4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZrTiq2N2KgIPRD6vld7S7UE7KpDWUo+Z/WpLy+BuIsadjUReRaW+5wcYuTnrbLOgi\n\t8LpYQn6WdlUiRqDxuu7VYs40PaExDziKP+RoWXye52n79ZGVZyN3Fh+onNsFFXh352\n\tmokqtdtp0TTjTBAkl00H3HihH5yEf5skbZOWjC/0=","Date":"Thu, 22 Oct 2020 05:38:59 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201022023859.GB14061@pendragon.ideasonboard.com>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-4-jacopo@jmondi.org>\n\t<20201022015146.GV3942@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201022015146.GV3942@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 03/14] libcamera: controls: Add\n\tsupported values to ControlInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]