[{"id":18262,"web_url":"https://patchwork.libcamera.org/comment/18262/","msgid":"<20210722130609.ih4igh37s4lm33vl@uno.localdomain>","date":"2021-07-22T13:06:09","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:\n> It would be convenient to be able to iterate over available boolean\n> values, for example for controls that designate if some function can be\n> enabled/disabled. The current min/max/def constructor is insufficient,\n> as .values() is empty, so the values cannot be easily iterated over, and\n> creating a Span of booleans does not work for the values constructor.\n>\n> Add new constructors to ControlInfo that takes a set of booleans (if\n> both booleans are valid values) plus a default, and another that takes\n> only one boolean (if only one boolean is a valid value).\n>\n> Update the ControlInfo test accordingly.\n\nThank you for considering my suggestion!\n\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v5:\n> - break away the single-value one to a different constructor\n>\n> Changes in v2:\n> - use set instead of span of bools\n> - add assertion to make sure that the default is a valid value\n> - update the test\n> ---\n> include/libcamera/controls.h | 3 +++\n> src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> 3 files changed, 65 insertions(+)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1bc958a4..de733bd8 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -9,6 +9,7 @@\n> #define __LIBCAMERA_CONTROLS_H__\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -272,6 +273,8 @@ public:\n> \t\t\t const ControlValue &def = 0);\n> \texplicit ControlInfo(Span values,\n> \t\t\t const ControlValue &def = {});\n> +\texplicit ControlInfo(std::set values, bool def);\n\nNo need for explicit if the constructor accepts two arguments\n\n> +\texplicit ControlInfo(bool value);\n>\n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 78109f41..283472c5 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span values,\n> \t\tvalues_.push_back(value);\n> }\n>\n> +/**\n> + * \\brief Construct a boolean ControlInfo with both boolean values\n> + * \\param[in] values The control valid boolean values (both true and false)\n> + * \\param[in] def The control default boolean value\n\nI'm still not super happy with the outcome, as the 'valid values' can\nbe true/false and nothing else, so passing them in makes not much\nsense. But as said in the previous review that's how we could\ndisambiguate between a ControlInfo that supports both values and one\nthat only supports true or false. Unless something smarter comes to\nmind, I guess we could now live with this ?\n\n> + *\n> + * Construct a ControlInfo for a boolean control, where both true and false are\n> + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> + * The minimum value will always be false, and the maximum always true. The\n> + * default value is \\a def.\n> + */\n> +ControlInfo::ControlInfo(std::set values, bool def)\n> +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> +{\n> +\tassert(values.count(def) && values.size() == 2);\n\nBe aware this will accept ControlInfo({true, true}, false);\n\n> +}\n> +\n> +/**\n> + * \\brief Construct a boolean ControlInfo with only one valid value\n> + * \\param[in] value The control valid boolean value\n> + *\n> + * Construct a ControlInfo for a boolean control, where there is only valid\n> + * value. The minimum, maximum, and default values will all be \\a value.\n> + */\n> +ControlInfo::ControlInfo(bool value)\n> +\t: min_(value), max_(value), def_(value)\n> +{\n> +\tvalues_ = { value };\n> +}\n> +\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control\n> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> index 1e05e131..2827473b 100644\n> --- a/test/controls/control_info.cpp\n> +++ b/test/controls/control_info.cpp\n> @@ -44,6 +44,39 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> +\t\t/*\n> +\t\t * Test information retrieval from a control with boolean\n> +\t\t * values.\n> +\t\t */\n> +\t\tControlInfo aeEnable({ false, true }, false);\n> +\n> +\t\tif (aeEnable.min().get() != false ||\n> +\t\t aeEnable.def().get() != false ||\n> +\t\t aeEnable.max().get() != true) {\n> +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (aeEnable.values()[0].get() != false ||\n> +\t\t aeEnable.values()[1].get() != true) {\n> +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tControlInfo awbEnable(true);\n> +\n> +\t\tif (awbEnable.min().get() != true ||\n> +\t\t awbEnable.def().get() != true ||\n> +\t\t awbEnable.max().get() != true) {\n> +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (awbEnable.values()[0].get() != true) {\n> +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n\nAs said, not in love with this, but I don't have anything better to\nsuggest :)\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t\treturn TestPass;\n> \t}\n> };\n> --\n> 2.27.0\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 A139CC322B\n\tfor ;\n\tThu, 22 Jul 2021 13:05:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28EB268543;\n\tThu, 22 Jul 2021 15:05:24 +0200 (CEST)","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 90DE56027A\n\tfor ;\n\tThu, 22 Jul 2021 15:05:22 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 10AD2C0007;\n\tThu, 22 Jul 2021 13:05:21 +0000 (UTC)"],"Date":"Thu, 22 Jul 2021 15:06:09 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210722130609.ih4igh37s4lm33vl@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210720101307.26010-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18274,"web_url":"https://patchwork.libcamera.org/comment/18274/","msgid":"<20210723054552.GA63622@pyrite.rasen.tech>","date":"2021-07-23T05:45:52","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:\n> > It would be convenient to be able to iterate over available boolean\n> > values, for example for controls that designate if some function can be\n> > enabled/disabled. The current min/max/def constructor is insufficient,\n> > as .values() is empty, so the values cannot be easily iterated over, and\n> > creating a Span of booleans does not work for the values constructor.\n> >\n> > Add new constructors to ControlInfo that takes a set of booleans (if\n> > both booleans are valid values) plus a default, and another that takes\n> > only one boolean (if only one boolean is a valid value).\n> >\n> > Update the ControlInfo test accordingly.\n> \n> Thank you for considering my suggestion!\n> \n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v5:\n> > - break away the single-value one to a different constructor\n> >\n> > Changes in v2:\n> > - use set instead of span of bools\n> > - add assertion to make sure that the default is a valid value\n> > - update the test\n> > ---\n> > include/libcamera/controls.h | 3 +++\n> > src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> > 3 files changed, 65 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1bc958a4..de733bd8 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -9,6 +9,7 @@\n> > #define __LIBCAMERA_CONTROLS_H__\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -272,6 +273,8 @@ public:\n> > \t\t\t const ControlValue &def = 0);\n> > \texplicit ControlInfo(Span values,\n> > \t\t\t const ControlValue &def = {});\n> > +\texplicit ControlInfo(std::set values, bool def);\n> \n> No need for explicit if the constructor accepts two arguments\n> \n> > +\texplicit ControlInfo(bool value);\n> >\n> > \tconst ControlValue &min() const { return min_; }\n> > \tconst ControlValue &max() const { return max_; }\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 78109f41..283472c5 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span values,\n> > \t\tvalues_.push_back(value);\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a boolean ControlInfo with both boolean values\n> > + * \\param[in] values The control valid boolean values (both true and false)\n> > + * \\param[in] def The control default boolean value\n> \n> I'm still not super happy with the outcome, as the 'valid values' can\n> be true/false and nothing else, so passing them in makes not much\n> sense. But as said in the previous review that's how we could\n> disambiguate between a ControlInfo that supports both values and one\n> that only supports true or false. Unless something smarter comes to\n\nYeah that was the idea. It's unnecessary information but it makes the\ndistinction clear :/\n\n> mind, I guess we could now live with this ?\n> \n> > + *\n> > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > + * The minimum value will always be false, and the maximum always true. The\n> > + * default value is \\a def.\n> > + */\n> > +ControlInfo::ControlInfo(std::set values, bool def)\n> > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > +{\n> > +\tassert(values.count(def) && values.size() == 2);\n> \n> Be aware this will accept ControlInfo({true, true}, false);\n\nIt's a std::set so if you do { true, true } then values.size()\nshould be 1.\n\nAt least that was my reasoning. Is it wrong?\n\n> \n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > + * \\param[in] value The control valid boolean value\n> > + *\n> > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > + * value. The minimum, maximum, and default values will all be \\a value.\n> > + */\n> > +ControlInfo::ControlInfo(bool value)\n> > +\t: min_(value), max_(value), def_(value)\n> > +{\n> > +\tvalues_ = { value };\n> > +}\n> > +\n> > /**\n> > * \\fn ControlInfo::min()\n> > * \\brief Retrieve the minimum value of the control\n> > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > index 1e05e131..2827473b 100644\n> > --- a/test/controls/control_info.cpp\n> > +++ b/test/controls/control_info.cpp\n> > @@ -44,6 +44,39 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > +\t\t/*\n> > +\t\t * Test information retrieval from a control with boolean\n> > +\t\t * values.\n> > +\t\t */\n> > +\t\tControlInfo aeEnable({ false, true }, false);\n> > +\n> > +\t\tif (aeEnable.min().get() != false ||\n> > +\t\t aeEnable.def().get() != false ||\n> > +\t\t aeEnable.max().get() != true) {\n> > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (aeEnable.values()[0].get() != false ||\n> > +\t\t aeEnable.values()[1].get() != true) {\n> > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tControlInfo awbEnable(true);\n> > +\n> > +\t\tif (awbEnable.min().get() != true ||\n> > +\t\t awbEnable.def().get() != true ||\n> > +\t\t awbEnable.max().get() != true) {\n> > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (awbEnable.values()[0].get() != true) {\n> > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> \n> As said, not in love with this, but I don't have anything better to\n> suggest :)\n> \n> Reviewed-by: Jacopo Mondi \n\n\nThanks,\n\nPaul\n\n> \n> > \t\treturn TestPass;\n> > \t}\n> > };\n> > --\n> > 2.27.0\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 8BD7AC0109\n\tfor ;\n\tFri, 23 Jul 2021 05:46:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4F266877B;\n\tFri, 23 Jul 2021 07:46:01 +0200 (CEST)","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 69CD56877A\n\tfor ;\n\tFri, 23 Jul 2021 07:46:00 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE32C255;\n\tFri, 23 Jul 2021 07:45:58 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LWNe6Ih6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627019160;\n\tbh=dAyvkZqk8W/cAZLy7UTIgcQEvqLySfVMRIsXCVRaxX8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LWNe6Ih6EQqkSO8tH26q/qtIzwHDFFReQND85PtwGo/wuOHzjo8W+ETCuOFW6Tep6\n\tZHZmXj7hoh8oQmRPytKf6l8/nHuem3vP6sDwt3vEAkUoCTPz52+zfB5+y3kujowI4b\n\tElHa6zGvNakLFnhRrn4yne3HTXOAApCvW4A2omIc=","Date":"Fri, 23 Jul 2021 14:45:52 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210723054552.GA63622@pyrite.rasen.tech>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210722130609.ih4igh37s4lm33vl@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18275,"web_url":"https://patchwork.libcamera.org/comment/18275/","msgid":"<20210723071421.uxvwuovgpzk5v2h2@uno.localdomain>","date":"2021-07-23T07:14:21","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:\n> > > It would be convenient to be able to iterate over available boolean\n> > > values, for example for controls that designate if some function can be\n> > > enabled/disabled. The current min/max/def constructor is insufficient,\n> > > as .values() is empty, so the values cannot be easily iterated over, and\n> > > creating a Span of booleans does not work for the values constructor.\n> > >\n> > > Add new constructors to ControlInfo that takes a set of booleans (if\n> > > both booleans are valid values) plus a default, and another that takes\n> > > only one boolean (if only one boolean is a valid value).\n> > >\n> > > Update the ControlInfo test accordingly.\n> >\n> > Thank you for considering my suggestion!\n> >\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v5:\n> > > - break away the single-value one to a different constructor\n> > >\n> > > Changes in v2:\n> > > - use set instead of span of bools\n> > > - add assertion to make sure that the default is a valid value\n> > > - update the test\n> > > ---\n> > > include/libcamera/controls.h | 3 +++\n> > > src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> > > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> > > 3 files changed, 65 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 1bc958a4..de733bd8 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -9,6 +9,7 @@\n> > > #define __LIBCAMERA_CONTROLS_H__\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -272,6 +273,8 @@ public:\n> > > \t\t\t const ControlValue &def = 0);\n> > > \texplicit ControlInfo(Span values,\n> > > \t\t\t const ControlValue &def = {});\n> > > +\texplicit ControlInfo(std::set values, bool def);\n> >\n> > No need for explicit if the constructor accepts two arguments\n> >\n> > > +\texplicit ControlInfo(bool value);\n> > >\n> > > \tconst ControlValue &min() const { return min_; }\n> > > \tconst ControlValue &max() const { return max_; }\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 78109f41..283472c5 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span values,\n> > > \t\tvalues_.push_back(value);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Construct a boolean ControlInfo with both boolean values\n> > > + * \\param[in] values The control valid boolean values (both true and false)\n> > > + * \\param[in] def The control default boolean value\n> >\n> > I'm still not super happy with the outcome, as the 'valid values' can\n> > be true/false and nothing else, so passing them in makes not much\n> > sense. But as said in the previous review that's how we could\n> > disambiguate between a ControlInfo that supports both values and one\n> > that only supports true or false. Unless something smarter comes to\n>\n> Yeah that was the idea. It's unnecessary information but it makes the\n> distinction clear :/\n>\n> > mind, I guess we could now live with this ?\n> >\n> > > + *\n> > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > + * The minimum value will always be false, and the maximum always true. The\n> > > + * default value is \\a def.\n> > > + */\n> > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > +{\n> > > +\tassert(values.count(def) && values.size() == 2);\n> >\n> > Be aware this will accept ControlInfo({true, true}, false);\n>\n> It's a std::set so if you do { true, true } then values.size()\n> should be 1.\n>\n> At least that was my reasoning. Is it wrong?\n>\n\nOh you're right, I've overlooked the container type! Thanks for\nthe clarification!\n\n\n> >\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > + * \\param[in] value The control valid boolean value\n> > > + *\n> > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > + */\n> > > +ControlInfo::ControlInfo(bool value)\n> > > +\t: min_(value), max_(value), def_(value)\n> > > +{\n> > > +\tvalues_ = { value };\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn ControlInfo::min()\n> > > * \\brief Retrieve the minimum value of the control\n> > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > index 1e05e131..2827473b 100644\n> > > --- a/test/controls/control_info.cpp\n> > > +++ b/test/controls/control_info.cpp\n> > > @@ -44,6 +44,39 @@ protected:\n> > > \t\t\treturn TestFail;\n> > > \t\t}\n> > >\n> > > +\t\t/*\n> > > +\t\t * Test information retrieval from a control with boolean\n> > > +\t\t * values.\n> > > +\t\t */\n> > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > +\n> > > +\t\tif (aeEnable.min().get() != false ||\n> > > +\t\t aeEnable.def().get() != false ||\n> > > +\t\t aeEnable.max().get() != true) {\n> > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > +\t\t aeEnable.values()[1].get() != true) {\n> > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tControlInfo awbEnable(true);\n> > > +\n> > > +\t\tif (awbEnable.min().get() != true ||\n> > > +\t\t awbEnable.def().get() != true ||\n> > > +\t\t awbEnable.max().get() != true) {\n> > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> >\n> > As said, not in love with this, but I don't have anything better to\n> > suggest :)\n> >\n> > Reviewed-by: Jacopo Mondi \n>\n>\n> Thanks,\n>\n> Paul\n>\n> >\n> > > \t\treturn TestPass;\n> > > \t}\n> > > };\n> > > --\n> > > 2.27.0\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 67966C322B\n\tfor ;\n\tFri, 23 Jul 2021 07:13:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4F3F687A3;\n\tFri, 23 Jul 2021 09:13:35 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C0AB6853F\n\tfor ;\n\tFri, 23 Jul 2021 09:13:35 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 687E5100010;\n\tFri, 23 Jul 2021 07:13:34 +0000 (UTC)"],"Date":"Fri, 23 Jul 2021 09:14:21 +0200","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20210723071421.uxvwuovgpzk5v2h2@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210723054552.GA63622@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18334,"web_url":"https://patchwork.libcamera.org/comment/18334/","msgid":"","date":"2021-07-25T03:05:50","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:\n> > On Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:\n> > > It would be convenient to be able to iterate over available boolean\n> > > values, for example for controls that designate if some function can be\n> > > enabled/disabled. The current min/max/def constructor is insufficient,\n> > > as .values() is empty, so the values cannot be easily iterated over, and\n> > > creating a Span of booleans does not work for the values constructor.\n> > >\n> > > Add new constructors to ControlInfo that takes a set of booleans (if\n> > > both booleans are valid values) plus a default, and another that takes\n> > > only one boolean (if only one boolean is a valid value).\n> > >\n> > > Update the ControlInfo test accordingly.\n> > \n> > Thank you for considering my suggestion!\n> > \n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v5:\n> > > - break away the single-value one to a different constructor\n> > >\n> > > Changes in v2:\n> > > - use set instead of span of bools\n> > > - add assertion to make sure that the default is a valid value\n> > > - update the test\n> > > ---\n> > > include/libcamera/controls.h | 3 +++\n> > > src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> > > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> > > 3 files changed, 65 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 1bc958a4..de733bd8 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -9,6 +9,7 @@\n> > > #define __LIBCAMERA_CONTROLS_H__\n> > >\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -272,6 +273,8 @@ public:\n> > > \t\t\t const ControlValue &def = 0);\n> > > \texplicit ControlInfo(Span values,\n> > > \t\t\t const ControlValue &def = {});\n> > > +\texplicit ControlInfo(std::set values, bool def);\n> > \n> > No need for explicit if the constructor accepts two arguments\n> > \n> > > +\texplicit ControlInfo(bool value);\n> > >\n> > > \tconst ControlValue &min() const { return min_; }\n> > > \tconst ControlValue &max() const { return max_; }\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 78109f41..283472c5 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span values,\n> > > \t\tvalues_.push_back(value);\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Construct a boolean ControlInfo with both boolean values\n> > > + * \\param[in] values The control valid boolean values (both true and false)\n> > > + * \\param[in] def The control default boolean value\n> > \n> > I'm still not super happy with the outcome, as the 'valid values' can\n> > be true/false and nothing else, so passing them in makes not much\n> > sense. But as said in the previous review that's how we could\n> > disambiguate between a ControlInfo that supports both values and one\n> > that only supports true or false. Unless something smarter comes to\n> \n> Yeah that was the idea. It's unnecessary information but it makes the\n> distinction clear :/\n\nSo we all agree it's not great. The question is how it could be better\n:-)\n\nTurning brainstorming mode on.\n\nWe have two types of integer-based controls, numerical and enumerated.\nThe numerical controls have a minimum, maximum and default, while the\nenumerated controls have a list of possible values and a default.\nConstructing a ControlInfo for enumerated controls requires providing a\nspan that contains all the possible values.\n\nWhen generating control_ids.h from control_ids.yaml, the python script\ncreates, for each enumerated control, an array containing all possible\nvalues. This can be used as an argument to the ControlInfo constructor,\nas a span can be implicitly constructed from an std::array:\n\n\tControlInfo info{ controls::AeExposureModeValues };\n\nIf we were to create a ControlInfo that only supports a subset of those\nmodes, however, the following syntax, would result in a compilation\nerror:\n\n\tControlInfo info{ {\n\t\tcontrols::ExposureNormal,\n\t\tcontrols::ExposureShort,\n\t\tcontrols::ExposureLong,\n\t} };\n\nTo make it compile, we would need to write\n\n\tControlInfo info{ std::array{\n\t\tstatic_cast(controls::ExposureNormal),\n\t\tstatic_cast(controls::ExposureShort),\n\t\tstatic_cast(controls::ExposureLong),\n\t} };\n\nwhich isn't exactly great.\n\nThe bools fall in the same category,\n\n\tControlInfo info{ { false, true }, true };\n\nwon't compile, while\n\n\tControlInfo info{ std::array{ false, true }, true };\n\nwould. Arguably not the greatest either.\n\nIf we want to improve this, the first question we should ask ourselves\nis if a nicer syntax common to all enumerated controls is achievable.\nFor instance, we could perhaps define the following constructor:\n\n\ttemplate\n\tControlInfo(std::initializer_list values, T def);\n\nWith appropriate implementations for the types we want to support. This\nshould work for bools, allowing us to write\n\n\tControlInfo info{ { false, true }, true };\n\nbut won't work nicely for AeExposureMode. Due to the values being\nenumerated, the compiler will want and implementation for\n\n\tControlInfo::ControlInfo(std::initializer_list,\n\t\t\t\t\t\t\t controls::AeExposureModeEnum)\n\nand not\n\n\tControlInfo::ControlInfo, int32_t)\n\nMaybe we could work around that by creating an inline constructor that\nwill only match enumerated types (using std::enable_if) and casting to\nint32_t. I'm not sure if\n\n\tstatic_cast>(values)\n\nwould work though, I have a feeling it won't be that easy.\n\nI'm sure other options may be possible too.\n\nMaybe a nice solution for this doesn't exist, in which case we could\nfocus on bools only. In that case, I see four different cases for a\nControlInfo related to bools:\n\n- values = { false, true }, default = false\n- values = { false, true }, default = true\n- values = { false }, default = false\n- values = { true }, default = true\n\nAnything else is invalid. Could we improve this patch by catching more\ninvalid combinations are compile time ? One of the things that bother me\nis the increase in the number of constructors, which could create\nambiguous constructs due to the implicit constructors for ControlValue.\n\nThe following would be one way to add a bool-specific constructor that\nwill have no risk of clashing with anything in the future:\n\n\tenum Mutable {\n\t\tReadOnly,\n\t\tReadWrite,\n\t};\n\n\tControlInfo(bool default, Mutable mutable);\n\n(the Mutable, ReadOnly and ReadWrite names can most probably be\nimproved). One would use this, mapping to the four cases above, as\n\n\tControlInfo(false, ControlInfo::ReadWrite);\n\tControlInfo(true, ControlInfo::ReadWrite);\n\tControlInfo(false, ControlInfo::ReadOnly);\n\tControlInfo(true, ControlInfo::ReadOnly);\n\nMaybe writing it differently would be more explicit:\n\n\tControlInfo(ControlInfo::MutableBool, false);\n\tControlInfo(ControlInfo::MutableBool, true);\n\tControlInfo(ControlInfo::ImmutableBool, false);\n\tControlInfo(ControlInfo::ImmutableBool, true);\n\nNow that I've started the brainstorming, I'll let you unleash your brain\npower to shoot this down or improve it :-)\n\n> > mind, I guess we could now live with this ?\n> > \n> > > + *\n> > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > + * The minimum value will always be false, and the maximum always true. The\n> > > + * default value is \\a def.\n> > > + */\n> > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > +{\n> > > +\tassert(values.count(def) && values.size() == 2);\n> > \n> > Be aware this will accept ControlInfo({true, true}, false);\n> \n> It's a std::set so if you do { true, true } then values.size()\n> should be 1.\n> \n> At least that was my reasoning. Is it wrong?\n> \n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > + * \\param[in] value The control valid boolean value\n> > > + *\n> > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > + */\n> > > +ControlInfo::ControlInfo(bool value)\n> > > +\t: min_(value), max_(value), def_(value)\n> > > +{\n> > > +\tvalues_ = { value };\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn ControlInfo::min()\n> > > * \\brief Retrieve the minimum value of the control\n> > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > index 1e05e131..2827473b 100644\n> > > --- a/test/controls/control_info.cpp\n> > > +++ b/test/controls/control_info.cpp\n> > > @@ -44,6 +44,39 @@ protected:\n> > > \t\t\treturn TestFail;\n> > > \t\t}\n> > >\n> > > +\t\t/*\n> > > +\t\t * Test information retrieval from a control with boolean\n> > > +\t\t * values.\n> > > +\t\t */\n> > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > +\n> > > +\t\tif (aeEnable.min().get() != false ||\n> > > +\t\t aeEnable.def().get() != false ||\n> > > +\t\t aeEnable.max().get() != true) {\n> > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > +\t\t aeEnable.values()[1].get() != true) {\n> > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tControlInfo awbEnable(true);\n> > > +\n> > > +\t\tif (awbEnable.min().get() != true ||\n> > > +\t\t awbEnable.def().get() != true ||\n> > > +\t\t awbEnable.max().get() != true) {\n> > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > \n> > As said, not in love with this, but I don't have anything better to\n> > suggest :)\n> > \n> > Reviewed-by: Jacopo Mondi \n> > \n> > > \t\treturn TestPass;\n> > > \t}\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 39C90C0109\n\tfor ;\n\tSun, 25 Jul 2021 03:05:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AFD7687B5;\n\tSun, 25 Jul 2021 05:05:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20D3F60274\n\tfor ;\n\tSun, 25 Jul 2021 05:05:55 +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 3FB43255;\n\tSun, 25 Jul 2021 05:05:54 +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=\"jRkMcIwD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627182354;\n\tbh=Kac3+f27MAWYZmXQP51CNI89d07bTpxQPFTCKrtrbhM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jRkMcIwDk9X8stmhPvPbk1CIg4HmC2MhAFKrEizdQ2wGNHG2pSUKdM2pOckM7yFZT\n\t4/mt116tCKeBmckw6kifLqkRJ/7z5rfP6yBzVk381ykUXz7ahuDnK64rWXMEpWyOEx\n\teixEp5E7xl0+n/xqPuuBENVUbXoNyGLUmVf2LLHg=","Date":"Sun, 25 Jul 2021 06:05:50 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210723054552.GA63622@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18361,"web_url":"https://patchwork.libcamera.org/comment/18361/","msgid":"<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>","date":"2021-07-26T14:08:07","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Jul 25, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:\n> > > On Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:\n> > > > It would be convenient to be able to iterate over available boolean\n> > > > values, for example for controls that designate if some function can be\n> > > > enabled/disabled. The current min/max/def constructor is insufficient,\n> > > > as .values() is empty, so the values cannot be easily iterated over, and\n> > > > creating a Span of booleans does not work for the values constructor.\n> > > >\n> > > > Add new constructors to ControlInfo that takes a set of booleans (if\n> > > > both booleans are valid values) plus a default, and another that takes\n> > > > only one boolean (if only one boolean is a valid value).\n> > > >\n> > > > Update the ControlInfo test accordingly.\n> > >\n> > > Thank you for considering my suggestion!\n> > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v5:\n> > > > - break away the single-value one to a different constructor\n> > > >\n> > > > Changes in v2:\n> > > > - use set instead of span of bools\n> > > > - add assertion to make sure that the default is a valid value\n> > > > - update the test\n> > > > ---\n> > > > include/libcamera/controls.h | 3 +++\n> > > > src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> > > > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> > > > 3 files changed, 65 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index 1bc958a4..de733bd8 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -9,6 +9,7 @@\n> > > > #define __LIBCAMERA_CONTROLS_H__\n> > > >\n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -272,6 +273,8 @@ public:\n> > > > \t\t\t const ControlValue &def = 0);\n> > > > \texplicit ControlInfo(Span values,\n> > > > \t\t\t const ControlValue &def = {});\n> > > > +\texplicit ControlInfo(std::set values, bool def);\n> > >\n> > > No need for explicit if the constructor accepts two arguments\n> > >\n> > > > +\texplicit ControlInfo(bool value);\n> > > >\n> > > > \tconst ControlValue &min() const { return min_; }\n> > > > \tconst ControlValue &max() const { return max_; }\n> > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > index 78109f41..283472c5 100644\n> > > > --- a/src/libcamera/controls.cpp\n> > > > +++ b/src/libcamera/controls.cpp\n> > > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span values,\n> > > > \t\tvalues_.push_back(value);\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Construct a boolean ControlInfo with both boolean values\n> > > > + * \\param[in] values The control valid boolean values (both true and false)\n> > > > + * \\param[in] def The control default boolean value\n> > >\n> > > I'm still not super happy with the outcome, as the 'valid values' can\n> > > be true/false and nothing else, so passing them in makes not much\n> > > sense. But as said in the previous review that's how we could\n> > > disambiguate between a ControlInfo that supports both values and one\n> > > that only supports true or false. Unless something smarter comes to\n> >\n> > Yeah that was the idea. It's unnecessary information but it makes the\n> > distinction clear :/\n>\n> So we all agree it's not great. The question is how it could be better\n> :-)\n>\n> Turning brainstorming mode on.\n>\n> We have two types of integer-based controls, numerical and enumerated.\n> The numerical controls have a minimum, maximum and default, while the\n> enumerated controls have a list of possible values and a default.\n> Constructing a ControlInfo for enumerated controls requires providing a\n> span that contains all the possible values.\n>\n> When generating control_ids.h from control_ids.yaml, the python script\n> creates, for each enumerated control, an array containing all possible\n> values. This can be used as an argument to the ControlInfo constructor,\n> as a span can be implicitly constructed from an std::array:\n>\n> \tControlInfo info{ controls::AeExposureModeValues };\n>\n> If we were to create a ControlInfo that only supports a subset of those\n> modes, however, the following syntax, would result in a compilation\n> error:\n>\n> \tControlInfo info{ {\n> \t\tcontrols::ExposureNormal,\n> \t\tcontrols::ExposureShort,\n> \t\tcontrols::ExposureLong,\n> \t} };\n>\n> To make it compile, we would need to write\n>\n> \tControlInfo info{ std::array{\n> \t\tstatic_cast(controls::ExposureNormal),\n> \t\tstatic_cast(controls::ExposureShort),\n> \t\tstatic_cast(controls::ExposureLong),\n> \t} };\n>\n> which isn't exactly great.\n>\n> The bools fall in the same category,\n>\n> \tControlInfo info{ { false, true }, true };\n\nI appreciate the thoughtful analysis of the issue, but my concern was\ndifferent and could be expressed as:\n - For a boolean control info, does it make sense to pass in\n {true, false} as possible values as\n - The order is not relevant\n - The only values a boolean control info can support are, by\n definition, true or false\n\nAll of this assuming a boolean control info that only support true OR\nfalse is constructed by passing in the single supported value.\n\nEven if we make the constructors nicer, my issue was conceptually on\nthe requirement to pass {false, true} in, even if that's not required\nby definition\n\n\n>\n> won't compile, while\n>\n> \tControlInfo info{ std::array{ false, true }, true };\n>\n> would. Arguably not the greatest either.\n>\n> If we want to improve this, the first question we should ask ourselves\n> is if a nicer syntax common to all enumerated controls is achievable.\n> For instance, we could perhaps define the following constructor:\n>\n> \ttemplate\n> \tControlInfo(std::initializer_list values, T def);\n>\n> With appropriate implementations for the types we want to support. This\n> should work for bools, allowing us to write\n>\n> \tControlInfo info{ { false, true }, true };\n\nThis is possible today with the simple constructor added by Paul, but\ndoes not solve the issue with the two values being passed in\n\n>\n> but won't work nicely for AeExposureMode. Due to the values being\n> enumerated, the compiler will want and implementation for\n>\n> \tControlInfo::ControlInfo(std::initializer_list,\n> \t\t\t\t\t\t\t controls::AeExposureModeEnum)\n>\n> and not\n>\n> \tControlInfo::ControlInfo, int32_t)\n>\n> Maybe we could work around that by creating an inline constructor that\n> will only match enumerated types (using std::enable_if) and casting to\n> int32_t. I'm not sure if\n>\n> \tstatic_cast>(values)\n>\n> would work though, I have a feeling it won't be that easy.\n>\n> I'm sure other options may be possible too.\n>\n> Maybe a nice solution for this doesn't exist, in which case we could\n> focus on bools only. In that case, I see four different cases for a\n> ControlInfo related to bools:\n>\n> - values = { false, true }, default = false\n> - values = { false, true }, default = true\n> - values = { false }, default = false\n> - values = { true }, default = true\n>\n> Anything else is invalid. Could we improve this patch by catching more\n> invalid combinations are compile time ? One of the things that bother me\n\nI think this version only accepts the 4 of them, according to Paul's\nreply to my comment that ({false, false}, true) was possible.\n\nIt currently goes through two constructors, one for 'mutable' booleans\ncontrol info:\n ControlInfo::ControlInfo(std::set values, bool def)\n\nAs this is an std::set, {false, false} or {true, true} will be refused\nby the assertion\n assert(values.count(def) && values.size() == 2);\n\n (note: asserting for values.size() == 2 is probably enough)\n\nAnd one for 'non mutable' ones\n ControlInfo::ControlInfo(bool value)\n\nWhich was suggested as it reduces the possibility of mis-use as\n ControlInfo( {false}, true)\nis not possible with this constructor\n\nAll in all I think this version is safe, I'm just bothered by the fact\n{true, false} have to passed in when those are implicitly the only\nvalues supported by a boolean control info\n\n> is the increase in the number of constructors, which could create\n> ambiguous constructs due to the implicit constructors for ControlValue.\n\nThat's a reasonable concern\n\n>\n> The following would be one way to add a bool-specific constructor that\n> will have no risk of clashing with anything in the future:\n>\n> \tenum Mutable {\n> \t\tReadOnly,\n> \t\tReadWrite,\n> \t};\n>\n> \tControlInfo(bool default, Mutable mutable);\n>\n> (the Mutable, ReadOnly and ReadWrite names can most probably be\n> improved). One would use this, mapping to the four cases above, as\n>\n> \tControlInfo(false, ControlInfo::ReadWrite);\n> \tControlInfo(true, ControlInfo::ReadWrite);\n> \tControlInfo(false, ControlInfo::ReadOnly);\n> \tControlInfo(true, ControlInfo::ReadOnly);\n>\n> Maybe writing it differently would be more explicit:\n>\n> \tControlInfo(ControlInfo::MutableBool, false);\n> \tControlInfo(ControlInfo::MutableBool, true);\n> \tControlInfo(ControlInfo::ImmutableBool, false);\n> \tControlInfo(ControlInfo::ImmutableBool, true);\n\nAh, this could solve the concerns I expressed above.\n\nWe have the general need to express when a Control is RO or RW, don't\nwe ? We could add an rw_ flag to the ControlInfo class for that and\nderive a read-only variant maybe ?\n\ndiff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex 1bc958a43b22..5ef84a92f219 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -272,11 +272,16 @@ public:\n const ControlValue &def = 0);\n explicit ControlInfo(Span values,\n const ControlValue &def = {});\n+ explicit ControlInfo(bool value)\n+ : min_(false), max_(true), def_(value)\n+ {\n+ }\n\n const ControlValue &min() const { return min_; }\n const ControlValue &max() const { return max_; }\n const ControlValue &def() const { return def_; }\n const std::vector &values() const { return values_; }\n+ bool rw() const { return rw_; }\n\n std::string toString() const;\n\n@@ -290,13 +295,41 @@ public:\n return !(*this == other);\n }\n\n-private:\n+protected:\n+ bool rw_ : true;\n ControlValue min_;\n ControlValue max_;\n ControlValue def_;\n std::vector values_;\n };\n\n+class ROControlInfo : public ControlInfo\n+{\n+ LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n+\n+ explicit ROControlInfo(const ControlValue &min = 0,\n+ const ControlValue &max = 0,\n+ const ControlValue &def = 0)\n+ : ControlInfo(min, max, def)\n+ {\n+ rw_ = false;\n+ }\n+\n+ explicit ROControlInfo(Span values,\n+ const ControlValue &def = {})\n+ : ControlInfo(values, def)\n+ {\n+ rw_ = false;\n+ }\n+\n+ explicit ROControlInfo(bool value)\n+ {\n+ min_ = max_ = def_ = value;\n+ rw_ = false;\n+ }\n+};\n+\n\nI'm sure it could be done in a smarter way than this, I'm just wonder\nif it's worth it and IPA/ph should be in charge of deciding what type\nof ControlInfo to construct. For the generator, once we have a\nread_only flag, this should be trivial instead.\n\n>\n> Now that I've started the brainstorming, I'll let you unleash your brain\n> power to shoot this down or improve it :-)\n>\n> > > mind, I guess we could now live with this ?\n> > >\n> > > > + *\n> > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > + * default value is \\a def.\n> > > > + */\n> > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > +{\n> > > > +\tassert(values.count(def) && values.size() == 2);\n> > >\n> > > Be aware this will accept ControlInfo({true, true}, false);\n> >\n> > It's a std::set so if you do { true, true } then values.size()\n> > should be 1.\n> >\n> > At least that was my reasoning. Is it wrong?\n> >\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > + * \\param[in] value The control valid boolean value\n> > > > + *\n> > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > + */\n> > > > +ControlInfo::ControlInfo(bool value)\n> > > > +\t: min_(value), max_(value), def_(value)\n> > > > +{\n> > > > +\tvalues_ = { value };\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\fn ControlInfo::min()\n> > > > * \\brief Retrieve the minimum value of the control\n> > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > index 1e05e131..2827473b 100644\n> > > > --- a/test/controls/control_info.cpp\n> > > > +++ b/test/controls/control_info.cpp\n> > > > @@ -44,6 +44,39 @@ protected:\n> > > > \t\t\treturn TestFail;\n> > > > \t\t}\n> > > >\n> > > > +\t\t/*\n> > > > +\t\t * Test information retrieval from a control with boolean\n> > > > +\t\t * values.\n> > > > +\t\t */\n> > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > +\n> > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > +\t\t aeEnable.def().get() != false ||\n> > > > +\t\t aeEnable.max().get() != true) {\n> > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > +\t\t\treturn TestFail;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > +\t\t\treturn TestFail;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tControlInfo awbEnable(true);\n> > > > +\n> > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > +\t\t awbEnable.def().get() != true ||\n> > > > +\t\t awbEnable.max().get() != true) {\n> > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > +\t\t\treturn TestFail;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > +\t\t\treturn TestFail;\n> > > > +\t\t}\n> > > > +\n> > >\n> > > As said, not in love with this, but I don't have anything better to\n> > > suggest :)\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > > \t\treturn TestPass;\n> > > > \t}\n> > > > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 461DCC0109\n\tfor ;\n\tMon, 26 Jul 2021 14:07:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8EDC687BE;\n\tMon, 26 Jul 2021 16:07:21 +0200 (CEST)","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 28969687AF\n\tfor ;\n\tMon, 26 Jul 2021 16:07:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 6E431C0007;\n\tMon, 26 Jul 2021 14:07:19 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 16:08:07 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18366,"web_url":"https://patchwork.libcamera.org/comment/18366/","msgid":"","date":"2021-07-26T16:36:54","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","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, Jul 26, 2021 at 04:08:07PM +0200, Jacopo Mondi wrote:\n> On Sun, Jul 25, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:\n> > On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:\n> > > On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:\n> > > > On Tue, Jul 20, 2021 at 07:12:59PM +0900, Paul Elder wrote:\n> > > > > It would be convenient to be able to iterate over available boolean\n> > > > > values, for example for controls that designate if some function can be\n> > > > > enabled/disabled. The current min/max/def constructor is insufficient,\n> > > > > as .values() is empty, so the values cannot be easily iterated over, and\n> > > > > creating a Span of booleans does not work for the values constructor.\n> > > > >\n> > > > > Add new constructors to ControlInfo that takes a set of booleans (if\n> > > > > both booleans are valid values) plus a default, and another that takes\n> > > > > only one boolean (if only one boolean is a valid value).\n> > > > >\n> > > > > Update the ControlInfo test accordingly.\n> > > >\n> > > > Thank you for considering my suggestion!\n> > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > >\n> > > > > ---\n> > > > > Changes in v5:\n> > > > > - break away the single-value one to a different constructor\n> > > > >\n> > > > > Changes in v2:\n> > > > > - use set instead of span of bools\n> > > > > - add assertion to make sure that the default is a valid value\n> > > > > - update the test\n> > > > > ---\n> > > > > include/libcamera/controls.h | 3 +++\n> > > > > src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> > > > > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> > > > > 3 files changed, 65 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > index 1bc958a4..de733bd8 100644\n> > > > > --- a/include/libcamera/controls.h\n> > > > > +++ b/include/libcamera/controls.h\n> > > > > @@ -9,6 +9,7 @@\n> > > > > #define __LIBCAMERA_CONTROLS_H__\n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -272,6 +273,8 @@ public:\n> > > > > \t\t\t const ControlValue &def = 0);\n> > > > > \texplicit ControlInfo(Span values,\n> > > > > \t\t\t const ControlValue &def = {});\n> > > > > +\texplicit ControlInfo(std::set values, bool def);\n> > > >\n> > > > No need for explicit if the constructor accepts two arguments\n> > > >\n> > > > > +\texplicit ControlInfo(bool value);\n> > > > >\n> > > > > \tconst ControlValue &min() const { return min_; }\n> > > > > \tconst ControlValue &max() const { return max_; }\n> > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > > > index 78109f41..283472c5 100644\n> > > > > --- a/src/libcamera/controls.cpp\n> > > > > +++ b/src/libcamera/controls.cpp\n> > > > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span values,\n> > > > > \t\tvalues_.push_back(value);\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Construct a boolean ControlInfo with both boolean values\n> > > > > + * \\param[in] values The control valid boolean values (both true and false)\n> > > > > + * \\param[in] def The control default boolean value\n> > > >\n> > > > I'm still not super happy with the outcome, as the 'valid values' can\n> > > > be true/false and nothing else, so passing them in makes not much\n> > > > sense. But as said in the previous review that's how we could\n> > > > disambiguate between a ControlInfo that supports both values and one\n> > > > that only supports true or false. Unless something smarter comes to\n> > >\n> > > Yeah that was the idea. It's unnecessary information but it makes the\n> > > distinction clear :/\n> >\n> > So we all agree it's not great. The question is how it could be better\n> > :-)\n> >\n> > Turning brainstorming mode on.\n> >\n> > We have two types of integer-based controls, numerical and enumerated.\n> > The numerical controls have a minimum, maximum and default, while the\n> > enumerated controls have a list of possible values and a default.\n> > Constructing a ControlInfo for enumerated controls requires providing a\n> > span that contains all the possible values.\n> >\n> > When generating control_ids.h from control_ids.yaml, the python script\n> > creates, for each enumerated control, an array containing all possible\n> > values. This can be used as an argument to the ControlInfo constructor,\n> > as a span can be implicitly constructed from an std::array:\n> >\n> > \tControlInfo info{ controls::AeExposureModeValues };\n> >\n> > If we were to create a ControlInfo that only supports a subset of those\n> > modes, however, the following syntax, would result in a compilation\n> > error:\n> >\n> > \tControlInfo info{ {\n> > \t\tcontrols::ExposureNormal,\n> > \t\tcontrols::ExposureShort,\n> > \t\tcontrols::ExposureLong,\n> > \t} };\n> >\n> > To make it compile, we would need to write\n> >\n> > \tControlInfo info{ std::array{\n> > \t\tstatic_cast(controls::ExposureNormal),\n> > \t\tstatic_cast(controls::ExposureShort),\n> > \t\tstatic_cast(controls::ExposureLong),\n> > \t} };\n> >\n> > which isn't exactly great.\n> >\n> > The bools fall in the same category,\n> >\n> > \tControlInfo info{ { false, true }, true };\n> \n> I appreciate the thoughtful analysis of the issue, but my concern was\n> different and could be expressed as:\n> - For a boolean control info, does it make sense to pass in\n> {true, false} as possible values as\n> - The order is not relevant\n> - The only values a boolean control info can support are, by\n> definition, true or false\n\nAgreed, hence the alternative, bool-only proposal below :-) The downside\nis that it won't help us with enum-based controls, but maybe that can be\naddressed separately.\n\n> All of this assuming a boolean control info that only support true OR\n> false is constructed by passing in the single supported value.\n> \n> Even if we make the constructors nicer, my issue was conceptually on\n> the requirement to pass {false, true} in, even if that's not required\n> by definition\n> \n> > won't compile, while\n> >\n> > \tControlInfo info{ std::array{ false, true }, true };\n> >\n> > would. Arguably not the greatest either.\n> >\n> > If we want to improve this, the first question we should ask ourselves\n> > is if a nicer syntax common to all enumerated controls is achievable.\n> > For instance, we could perhaps define the following constructor:\n> >\n> > \ttemplate\n> > \tControlInfo(std::initializer_list values, T def);\n> >\n> > With appropriate implementations for the types we want to support. This\n> > should work for bools, allowing us to write\n> >\n> > \tControlInfo info{ { false, true }, true };\n> \n> This is possible today with the simple constructor added by Paul, but\n> does not solve the issue with the two values being passed in\n> \n> > but won't work nicely for AeExposureMode. Due to the values being\n> > enumerated, the compiler will want and implementation for\n> >\n> > \tControlInfo::ControlInfo(std::initializer_list,\n> > \t\t\t\t\t\t\t controls::AeExposureModeEnum)\n> >\n> > and not\n> >\n> > \tControlInfo::ControlInfo, int32_t)\n> >\n> > Maybe we could work around that by creating an inline constructor that\n> > will only match enumerated types (using std::enable_if) and casting to\n> > int32_t. I'm not sure if\n> >\n> > \tstatic_cast>(values)\n> >\n> > would work though, I have a feeling it won't be that easy.\n> >\n> > I'm sure other options may be possible too.\n\nIf someone has a great idea that would address both the boolean and\nenumerated controls, I'm still all ears :-)\n\n> > Maybe a nice solution for this doesn't exist, in which case we could\n> > focus on bools only. In that case, I see four different cases for a\n> > ControlInfo related to bools:\n> >\n> > - values = { false, true }, default = false\n> > - values = { false, true }, default = true\n> > - values = { false }, default = false\n> > - values = { true }, default = true\n> >\n> > Anything else is invalid. Could we improve this patch by catching more\n> > invalid combinations are compile time ? One of the things that bother me\n> \n> I think this version only accepts the 4 of them, according to Paul's\n> reply to my comment that ({false, false}, true) was possible.\n> \n> It currently goes through two constructors, one for 'mutable' booleans\n> control info:\n> ControlInfo::ControlInfo(std::set values, bool def)\n> \n> As this is an std::set, {false, false} or {true, true} will be refused\n> by the assertion\n> assert(values.count(def) && values.size() == 2);\n> \n> (note: asserting for values.size() == 2 is probably enough)\n> \n> And one for 'non mutable' ones\n> ControlInfo::ControlInfo(bool value)\n> \n> Which was suggested as it reduces the possibility of mis-use as\n> ControlInfo( {false}, true)\n> is not possible with this constructor\n> \n> All in all I think this version is safe, I'm just bothered by the fact\n> {true, false} have to passed in when those are implicitly the only\n> values supported by a boolean control info\n> \n> > is the increase in the number of constructors, which could create\n> > ambiguous constructs due to the implicit constructors for ControlValue.\n> \n> That's a reasonable concern\n> \n> > The following would be one way to add a bool-specific constructor that\n> > will have no risk of clashing with anything in the future:\n> >\n> > \tenum Mutable {\n> > \t\tReadOnly,\n> > \t\tReadWrite,\n> > \t};\n> >\n> > \tControlInfo(bool default, Mutable mutable);\n> >\n> > (the Mutable, ReadOnly and ReadWrite names can most probably be\n> > improved). One would use this, mapping to the four cases above, as\n> >\n> > \tControlInfo(false, ControlInfo::ReadWrite);\n> > \tControlInfo(true, ControlInfo::ReadWrite);\n> > \tControlInfo(false, ControlInfo::ReadOnly);\n> > \tControlInfo(true, ControlInfo::ReadOnly);\n> >\n> > Maybe writing it differently would be more explicit:\n> >\n> > \tControlInfo(ControlInfo::MutableBool, false);\n> > \tControlInfo(ControlInfo::MutableBool, true);\n> > \tControlInfo(ControlInfo::ImmutableBool, false);\n> > \tControlInfo(ControlInfo::ImmutableBool, true);\n> \n> Ah, this could solve the concerns I expressed above.\n> \n> We have the general need to express when a Control is RO or RW, don't\n> we ? We could add an rw_ flag to the ControlInfo class for that and\n> derive a read-only variant maybe ?\n\nThinking more about this, the read-only concept bothers me a bit. A\nread-only control is essentially either a property or a metadata in the\ngeneral case (depending on whether the value is static or dynamic). What\nwe're dealing with here is a control that is meant to be writable, but\nhappens to only have a single supported value.\n\nTaking one step back, what are our use cases for boolean controls that\naccept a single value ?\n\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1bc958a43b22..5ef84a92f219 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -272,11 +272,16 @@ public:\n> const ControlValue &def = 0);\n> explicit ControlInfo(Span values,\n> const ControlValue &def = {});\n> + explicit ControlInfo(bool value)\n> + : min_(false), max_(true), def_(value)\n> + {\n> + }\n> \n> const ControlValue &min() const { return min_; }\n> const ControlValue &max() const { return max_; }\n> const ControlValue &def() const { return def_; }\n> const std::vector &values() const { return values_; }\n> + bool rw() const { return rw_; }\n> \n> std::string toString() const;\n> \n> @@ -290,13 +295,41 @@ public:\n> return !(*this == other);\n> }\n> \n> -private:\n> +protected:\n> + bool rw_ : true;\n> ControlValue min_;\n> ControlValue max_;\n> ControlValue def_;\n> std::vector values_;\n> };\n> \n> +class ROControlInfo : public ControlInfo\n> +{\n> + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n> +\n> + explicit ROControlInfo(const ControlValue &min = 0,\n> + const ControlValue &max = 0,\n> + const ControlValue &def = 0)\n> + : ControlInfo(min, max, def)\n> + {\n> + rw_ = false;\n> + }\n> +\n> + explicit ROControlInfo(Span values,\n> + const ControlValue &def = {})\n> + : ControlInfo(values, def)\n> + {\n> + rw_ = false;\n> + }\n> +\n> + explicit ROControlInfo(bool value)\n> + {\n> + min_ = max_ = def_ = value;\n> + rw_ = false;\n> + }\n> +};\n> +\n> \n> I'm sure it could be done in a smarter way than this, I'm just wonder\n> if it's worth it and IPA/ph should be in charge of deciding what type\n> of ControlInfo to construct. For the generator, once we have a\n> read_only flag, this should be trivial instead.\n> \n> > Now that I've started the brainstorming, I'll let you unleash your brain\n> > power to shoot this down or improve it :-)\n> >\n> > > > mind, I guess we could now live with this ?\n> > > >\n> > > > > + *\n> > > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > > + * default value is \\a def.\n> > > > > + */\n> > > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > > +{\n> > > > > +\tassert(values.count(def) && values.size() == 2);\n> > > >\n> > > > Be aware this will accept ControlInfo({true, true}, false);\n> > >\n> > > It's a std::set so if you do { true, true } then values.size()\n> > > should be 1.\n> > >\n> > > At least that was my reasoning. Is it wrong?\n> > >\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > > + * \\param[in] value The control valid boolean value\n> > > > > + *\n> > > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > > + */\n> > > > > +ControlInfo::ControlInfo(bool value)\n> > > > > +\t: min_(value), max_(value), def_(value)\n> > > > > +{\n> > > > > +\tvalues_ = { value };\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\fn ControlInfo::min()\n> > > > > * \\brief Retrieve the minimum value of the control\n> > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > > index 1e05e131..2827473b 100644\n> > > > > --- a/test/controls/control_info.cpp\n> > > > > +++ b/test/controls/control_info.cpp\n> > > > > @@ -44,6 +44,39 @@ protected:\n> > > > > \t\t\treturn TestFail;\n> > > > > \t\t}\n> > > > >\n> > > > > +\t\t/*\n> > > > > +\t\t * Test information retrieval from a control with boolean\n> > > > > +\t\t * values.\n> > > > > +\t\t */\n> > > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > > +\n> > > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > > +\t\t aeEnable.def().get() != false ||\n> > > > > +\t\t aeEnable.max().get() != true) {\n> > > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > > +\t\t\treturn TestFail;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > > +\t\t\treturn TestFail;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tControlInfo awbEnable(true);\n> > > > > +\n> > > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > > +\t\t awbEnable.def().get() != true ||\n> > > > > +\t\t awbEnable.max().get() != true) {\n> > > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > > +\t\t\treturn TestFail;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > > +\t\t\treturn TestFail;\n> > > > > +\t\t}\n> > > > > +\n> > > >\n> > > > As said, not in love with this, but I don't have anything better to\n> > > > suggest :)\n> > > >\n> > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > > \t\treturn TestPass;\n> > > > > \t}\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 C03CDC322C\n\tfor ;\n\tMon, 26 Jul 2021 16:37:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 08B58687BE;\n\tMon, 26 Jul 2021 18:37:01 +0200 (CEST)","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 D8201687AF\n\tfor ;\n\tMon, 26 Jul 2021 18:36:59 +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 437DE332;\n\tMon, 26 Jul 2021 18:36:59 +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=\"TWLoAqII\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627317419;\n\tbh=q6ovoxFAjyuIMBHvg+iQ7IhV6hUk8VZEm9DnJfIYDeQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TWLoAqIIrk4gEPURLrCr6UsTnh9eIlywd9uhrPBymj+XHomIYmlc8K2/SYE2llV2S\n\tpFE528LxaKeW9QPilvR9B8KWO2qZnA38Gx7x/Xr1wZckGwAXP68GKgmX8jxueAauqJ\n\txlbgqZNyxvBKO6UV1y5kFm4FMLo9/NzTKtajbOlY=","Date":"Mon, 26 Jul 2021 19:36:54 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t\n\t<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18408,"web_url":"https://patchwork.libcamera.org/comment/18408/","msgid":"<20210728163159.ote6eckfegk3xe76@uno.localdomain>","date":"2021-07-28T16:31:59","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n\n[snip]\n\n> > > The following would be one way to add a bool-specific constructor that\n> > > will have no risk of clashing with anything in the future:\n> > >\n> > > \tenum Mutable {\n> > > \t\tReadOnly,\n> > > \t\tReadWrite,\n> > > \t};\n> > >\n> > > \tControlInfo(bool default, Mutable mutable);\n> > >\n> > > (the Mutable, ReadOnly and ReadWrite names can most probably be\n> > > improved). One would use this, mapping to the four cases above, as\n> > >\n> > > \tControlInfo(false, ControlInfo::ReadWrite);\n> > > \tControlInfo(true, ControlInfo::ReadWrite);\n> > > \tControlInfo(false, ControlInfo::ReadOnly);\n> > > \tControlInfo(true, ControlInfo::ReadOnly);\n> > >\n> > > Maybe writing it differently would be more explicit:\n> > >\n> > > \tControlInfo(ControlInfo::MutableBool, false);\n> > > \tControlInfo(ControlInfo::MutableBool, true);\n> > > \tControlInfo(ControlInfo::ImmutableBool, false);\n> > > \tControlInfo(ControlInfo::ImmutableBool, true);\n> >\n> > Ah, this could solve the concerns I expressed above.\n> >\n> > We have the general need to express when a Control is RO or RW, don't\n> > we ? We could add an rw_ flag to the ControlInfo class for that and\n> > derive a read-only variant maybe ?\n>\n> Thinking more about this, the read-only concept bothers me a bit. A\n> read-only control is essentially either a property or a metadata in the\n> general case (depending on whether the value is static or dynamic). What\n> we're dealing with here is a control that is meant to be writable, but\n> happens to only have a single supported value.\n\nThis has bothered me all the time I tried to find a good term to\nidentify those \"booleans with a single value\". I proposed 'identity'\nbut that's not really up to the point. The fact I can't find a term\nfor that makes me think I'm missing something and we should probably\nbe doing something different. Hence the proposal to have a dedicated\nconstructor, but yes, more constructors are ambiguos.\n\n>\n> Taking one step back, what are our use cases for boolean controls that\n> accept a single value ?\n>\n\nI think the original intent was to be able to express things like: the\nIPA only supports AE_ENABLE, so that you register a\n\n {controls::AeEnable, ControlInfo(true) }\n\nWhile if you can enable/disable it, and it's enabled by default:\n\n {controls::AeEnable, ControlInfo({true, false}, true) }\n\nI would now suggest to simply not register the control at all if it's\nnot mutable, but I'm sure right now I'm missing why this was not\npossible in first place. Maybe it's just a matter of defining a policy\nlike this one: if a control (not a property, nor a metadata) cannot be\nchanged, just don't register it as part of the Camera::controls\" ?\n\nThanks\n j\n\n\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 1bc958a43b22..5ef84a92f219 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -272,11 +272,16 @@ public:\n> > const ControlValue &def = 0);\n> > explicit ControlInfo(Span values,\n> > const ControlValue &def = {});\n> > + explicit ControlInfo(bool value)\n> > + : min_(false), max_(true), def_(value)\n> > + {\n> > + }\n> >\n> > const ControlValue &min() const { return min_; }\n> > const ControlValue &max() const { return max_; }\n> > const ControlValue &def() const { return def_; }\n> > const std::vector &values() const { return values_; }\n> > + bool rw() const { return rw_; }\n> >\n> > std::string toString() const;\n> >\n> > @@ -290,13 +295,41 @@ public:\n> > return !(*this == other);\n> > }\n> >\n> > -private:\n> > +protected:\n> > + bool rw_ : true;\n> > ControlValue min_;\n> > ControlValue max_;\n> > ControlValue def_;\n> > std::vector values_;\n> > };\n> >\n> > +class ROControlInfo : public ControlInfo\n> > +{\n> > + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n> > +\n> > + explicit ROControlInfo(const ControlValue &min = 0,\n> > + const ControlValue &max = 0,\n> > + const ControlValue &def = 0)\n> > + : ControlInfo(min, max, def)\n> > + {\n> > + rw_ = false;\n> > + }\n> > +\n> > + explicit ROControlInfo(Span values,\n> > + const ControlValue &def = {})\n> > + : ControlInfo(values, def)\n> > + {\n> > + rw_ = false;\n> > + }\n> > +\n> > + explicit ROControlInfo(bool value)\n> > + {\n> > + min_ = max_ = def_ = value;\n> > + rw_ = false;\n> > + }\n> > +};\n> > +\n> >\n> > I'm sure it could be done in a smarter way than this, I'm just wonder\n> > if it's worth it and IPA/ph should be in charge of deciding what type\n> > of ControlInfo to construct. For the generator, once we have a\n> > read_only flag, this should be trivial instead.\n> >\n> > > Now that I've started the brainstorming, I'll let you unleash your brain\n> > > power to shoot this down or improve it :-)\n> > >\n> > > > > mind, I guess we could now live with this ?\n> > > > >\n> > > > > > + *\n> > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > > > + * default value is \\a def.\n> > > > > > + */\n> > > > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > > > +{\n> > > > > > +\tassert(values.count(def) && values.size() == 2);\n> > > > >\n> > > > > Be aware this will accept ControlInfo({true, true}, false);\n> > > >\n> > > > It's a std::set so if you do { true, true } then values.size()\n> > > > should be 1.\n> > > >\n> > > > At least that was my reasoning. Is it wrong?\n> > > >\n> > > > > > +}\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > > > + * \\param[in] value The control valid boolean value\n> > > > > > + *\n> > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > > > + */\n> > > > > > +ControlInfo::ControlInfo(bool value)\n> > > > > > +\t: min_(value), max_(value), def_(value)\n> > > > > > +{\n> > > > > > +\tvalues_ = { value };\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\fn ControlInfo::min()\n> > > > > > * \\brief Retrieve the minimum value of the control\n> > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > > > index 1e05e131..2827473b 100644\n> > > > > > --- a/test/controls/control_info.cpp\n> > > > > > +++ b/test/controls/control_info.cpp\n> > > > > > @@ -44,6 +44,39 @@ protected:\n> > > > > > \t\t\treturn TestFail;\n> > > > > > \t\t}\n> > > > > >\n> > > > > > +\t\t/*\n> > > > > > +\t\t * Test information retrieval from a control with boolean\n> > > > > > +\t\t * values.\n> > > > > > +\t\t */\n> > > > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > > > +\n> > > > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > > > +\t\t aeEnable.def().get() != false ||\n> > > > > > +\t\t aeEnable.max().get() != true) {\n> > > > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > > > +\t\t\treturn TestFail;\n> > > > > > +\t\t}\n> > > > > > +\n> > > > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > > > +\t\t\treturn TestFail;\n> > > > > > +\t\t}\n> > > > > > +\n> > > > > > +\t\tControlInfo awbEnable(true);\n> > > > > > +\n> > > > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > > > +\t\t awbEnable.def().get() != true ||\n> > > > > > +\t\t awbEnable.max().get() != true) {\n> > > > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > > > +\t\t\treturn TestFail;\n> > > > > > +\t\t}\n> > > > > > +\n> > > > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > > > +\t\t\treturn TestFail;\n> > > > > > +\t\t}\n> > > > > > +\n> > > > >\n> > > > > As said, not in love with this, but I don't have anything better to\n> > > > > suggest :)\n> > > > >\n> > > > > Reviewed-by: Jacopo Mondi \n> > > > >\n> > > > > > \t\treturn TestPass;\n> > > > > > \t}\n> > > > > > };\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 455A4C3230\n\tfor ;\n\tWed, 28 Jul 2021 16:31:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C70B2687BE;\n\tWed, 28 Jul 2021 18:31:14 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B93E160506\n\tfor ;\n\tWed, 28 Jul 2021 18:31:13 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id EFA0620003;\n\tWed, 28 Jul 2021 16:31:12 +0000 (UTC)"],"Date":"Wed, 28 Jul 2021 18:31:59 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210728163159.ote6eckfegk3xe76@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t\n\t<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18411,"web_url":"https://patchwork.libcamera.org/comment/18411/","msgid":"","date":"2021-07-28T16:39:23","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> \n> [snip]\n> \n> > > > The following would be one way to add a bool-specific constructor that\n> > > > will have no risk of clashing with anything in the future:\n> > > >\n> > > > \tenum Mutable {\n> > > > \t\tReadOnly,\n> > > > \t\tReadWrite,\n> > > > \t};\n> > > >\n> > > > \tControlInfo(bool default, Mutable mutable);\n> > > >\n> > > > (the Mutable, ReadOnly and ReadWrite names can most probably be\n> > > > improved). One would use this, mapping to the four cases above, as\n> > > >\n> > > > \tControlInfo(false, ControlInfo::ReadWrite);\n> > > > \tControlInfo(true, ControlInfo::ReadWrite);\n> > > > \tControlInfo(false, ControlInfo::ReadOnly);\n> > > > \tControlInfo(true, ControlInfo::ReadOnly);\n> > > >\n> > > > Maybe writing it differently would be more explicit:\n> > > >\n> > > > \tControlInfo(ControlInfo::MutableBool, false);\n> > > > \tControlInfo(ControlInfo::MutableBool, true);\n> > > > \tControlInfo(ControlInfo::ImmutableBool, false);\n> > > > \tControlInfo(ControlInfo::ImmutableBool, true);\n> > >\n> > > Ah, this could solve the concerns I expressed above.\n> > >\n> > > We have the general need to express when a Control is RO or RW, don't\n> > > we ? We could add an rw_ flag to the ControlInfo class for that and\n> > > derive a read-only variant maybe ?\n> >\n> > Thinking more about this, the read-only concept bothers me a bit. A\n> > read-only control is essentially either a property or a metadata in the\n> > general case (depending on whether the value is static or dynamic). What\n> > we're dealing with here is a control that is meant to be writable, but\n> > happens to only have a single supported value.\n> \n> This has bothered me all the time I tried to find a good term to\n> identify those \"booleans with a single value\". I proposed 'identity'\n> but that's not really up to the point. The fact I can't find a term\n> for that makes me think I'm missing something and we should probably\n> be doing something different. Hence the proposal to have a dedicated\n> constructor, but yes, more constructors are ambiguos.\n> \n> >\n> > Taking one step back, what are our use cases for boolean controls that\n> > accept a single value ?\n> >\n> \n> I think the original intent was to be able to express things like: the\n> IPA only supports AE_ENABLE, so that you register a\n> \n> {controls::AeEnable, ControlInfo(true) }\n> \n> While if you can enable/disable it, and it's enabled by default:\n> \n> {controls::AeEnable, ControlInfo({true, false}, true) }\n> \n> I would now suggest to simply not register the control at all if it's\n> not mutable, but I'm sure right now I'm missing why this was not\n> possible in first place. Maybe it's just a matter of defining a policy\n> like this one: if a control (not a property, nor a metadata) cannot be\n> changed, just don't register it as part of the Camera::controls\" ?\n\nIn that case applications wouldn't be able to differentiate a device\nthat doesn't support auto-exposure and a device that doesn't support\nmanual exposure. OK, not entirely true, applications could check for the\npresence of a manual exposure time control, but in general, we could\nhave other boolean controls that would suffer from this issue.\n\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 1bc958a43b22..5ef84a92f219 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -272,11 +272,16 @@ public:\n> > > const ControlValue &def = 0);\n> > > explicit ControlInfo(Span values,\n> > > const ControlValue &def = {});\n> > > + explicit ControlInfo(bool value)\n> > > + : min_(false), max_(true), def_(value)\n> > > + {\n> > > + }\n> > >\n> > > const ControlValue &min() const { return min_; }\n> > > const ControlValue &max() const { return max_; }\n> > > const ControlValue &def() const { return def_; }\n> > > const std::vector &values() const { return values_; }\n> > > + bool rw() const { return rw_; }\n> > >\n> > > std::string toString() const;\n> > >\n> > > @@ -290,13 +295,41 @@ public:\n> > > return !(*this == other);\n> > > }\n> > >\n> > > -private:\n> > > +protected:\n> > > + bool rw_ : true;\n> > > ControlValue min_;\n> > > ControlValue max_;\n> > > ControlValue def_;\n> > > std::vector values_;\n> > > };\n> > >\n> > > +class ROControlInfo : public ControlInfo\n> > > +{\n> > > + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n> > > +\n> > > + explicit ROControlInfo(const ControlValue &min = 0,\n> > > + const ControlValue &max = 0,\n> > > + const ControlValue &def = 0)\n> > > + : ControlInfo(min, max, def)\n> > > + {\n> > > + rw_ = false;\n> > > + }\n> > > +\n> > > + explicit ROControlInfo(Span values,\n> > > + const ControlValue &def = {})\n> > > + : ControlInfo(values, def)\n> > > + {\n> > > + rw_ = false;\n> > > + }\n> > > +\n> > > + explicit ROControlInfo(bool value)\n> > > + {\n> > > + min_ = max_ = def_ = value;\n> > > + rw_ = false;\n> > > + }\n> > > +};\n> > > +\n> > >\n> > > I'm sure it could be done in a smarter way than this, I'm just wonder\n> > > if it's worth it and IPA/ph should be in charge of deciding what type\n> > > of ControlInfo to construct. For the generator, once we have a\n> > > read_only flag, this should be trivial instead.\n> > >\n> > > > Now that I've started the brainstorming, I'll let you unleash your brain\n> > > > power to shoot this down or improve it :-)\n> > > >\n> > > > > > mind, I guess we could now live with this ?\n> > > > > >\n> > > > > > > + *\n> > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > > > > + * default value is \\a def.\n> > > > > > > + */\n> > > > > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > > > > +{\n> > > > > > > +\tassert(values.count(def) && values.size() == 2);\n> > > > > >\n> > > > > > Be aware this will accept ControlInfo({true, true}, false);\n> > > > >\n> > > > > It's a std::set so if you do { true, true } then values.size()\n> > > > > should be 1.\n> > > > >\n> > > > > At least that was my reasoning. Is it wrong?\n> > > > >\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > > > > + * \\param[in] value The control valid boolean value\n> > > > > > > + *\n> > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > > > > + */\n> > > > > > > +ControlInfo::ControlInfo(bool value)\n> > > > > > > +\t: min_(value), max_(value), def_(value)\n> > > > > > > +{\n> > > > > > > +\tvalues_ = { value };\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\fn ControlInfo::min()\n> > > > > > > * \\brief Retrieve the minimum value of the control\n> > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > > > > index 1e05e131..2827473b 100644\n> > > > > > > --- a/test/controls/control_info.cpp\n> > > > > > > +++ b/test/controls/control_info.cpp\n> > > > > > > @@ -44,6 +44,39 @@ protected:\n> > > > > > > \t\t\treturn TestFail;\n> > > > > > > \t\t}\n> > > > > > >\n> > > > > > > +\t\t/*\n> > > > > > > +\t\t * Test information retrieval from a control with boolean\n> > > > > > > +\t\t * values.\n> > > > > > > +\t\t */\n> > > > > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > > > > +\n> > > > > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > > > > +\t\t aeEnable.def().get() != false ||\n> > > > > > > +\t\t aeEnable.max().get() != true) {\n> > > > > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > > > > +\t\t\treturn TestFail;\n> > > > > > > +\t\t}\n> > > > > > > +\n> > > > > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > > > > +\t\t\treturn TestFail;\n> > > > > > > +\t\t}\n> > > > > > > +\n> > > > > > > +\t\tControlInfo awbEnable(true);\n> > > > > > > +\n> > > > > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > > > > +\t\t awbEnable.def().get() != true ||\n> > > > > > > +\t\t awbEnable.max().get() != true) {\n> > > > > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > > > > +\t\t\treturn TestFail;\n> > > > > > > +\t\t}\n> > > > > > > +\n> > > > > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > > > > +\t\t\treturn TestFail;\n> > > > > > > +\t\t}\n> > > > > > > +\n> > > > > >\n> > > > > > As said, not in love with this, but I don't have anything better to\n> > > > > > suggest :)\n> > > > > >\n> > > > > > Reviewed-by: Jacopo Mondi \n> > > > > >\n> > > > > > > \t\treturn TestPass;\n> > > > > > > \t}\n> > > > > > > };\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart","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 2C7AFC322E\n\tfor ;\n\tWed, 28 Jul 2021 16:39:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BF2D687BE;\n\tWed, 28 Jul 2021 18:39:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 097E760506\n\tfor ;\n\tWed, 28 Jul 2021 18:39:30 +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 6C5E23F;\n\tWed, 28 Jul 2021 18:39:29 +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=\"FdBhwKCU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627490369;\n\tbh=s5SWAEDVY+elZMDH2XU+nVK+Tw8SXZNblr7XQJCwOhU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FdBhwKCUcvUAeDQfeaYcHO1goa6MYz7fre9LeYjMmunybVgTeb+OJf6ShIlEn2vI/\n\tlgm1CNmO9X0J/4bHNHEVYhyt4XL2xxH9faIoIfNTrbAmrGB+JFMwmjvXrAk3ycd6tF\n\tZwZSenKUBGnmGR4JGOF727XKSGDYZgTRZeOIXHiI=","Date":"Wed, 28 Jul 2021 19:39:23 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t\n\t<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>\n\t\n\t<20210728163159.ote6eckfegk3xe76@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210728163159.ote6eckfegk3xe76@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18419,"web_url":"https://patchwork.libcamera.org/comment/18419/","msgid":"<20210729103437.GL2167@pyrite.rasen.tech>","date":"2021-07-29T10:34:37","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo and Laurent,\n\nOn Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:\n> > On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > \n> > [snip]\n> > \n> > > > > The following would be one way to add a bool-specific constructor that\n> > > > > will have no risk of clashing with anything in the future:\n> > > > >\n> > > > > \tenum Mutable {\n> > > > > \t\tReadOnly,\n> > > > > \t\tReadWrite,\n> > > > > \t};\n> > > > >\n> > > > > \tControlInfo(bool default, Mutable mutable);\n> > > > >\n> > > > > (the Mutable, ReadOnly and ReadWrite names can most probably be\n> > > > > improved). One would use this, mapping to the four cases above, as\n> > > > >\n> > > > > \tControlInfo(false, ControlInfo::ReadWrite);\n> > > > > \tControlInfo(true, ControlInfo::ReadWrite);\n> > > > > \tControlInfo(false, ControlInfo::ReadOnly);\n> > > > > \tControlInfo(true, ControlInfo::ReadOnly);\n> > > > >\n> > > > > Maybe writing it differently would be more explicit:\n> > > > >\n> > > > > \tControlInfo(ControlInfo::MutableBool, false);\n> > > > > \tControlInfo(ControlInfo::MutableBool, true);\n> > > > > \tControlInfo(ControlInfo::ImmutableBool, false);\n> > > > > \tControlInfo(ControlInfo::ImmutableBool, true);\n> > > >\n> > > > Ah, this could solve the concerns I expressed above.\n> > > >\n> > > > We have the general need to express when a Control is RO or RW, don't\n> > > > we ? We could add an rw_ flag to the ControlInfo class for that and\n> > > > derive a read-only variant maybe ?\n> > >\n> > > Thinking more about this, the read-only concept bothers me a bit. A\n> > > read-only control is essentially either a property or a metadata in the\n> > > general case (depending on whether the value is static or dynamic). What\n> > > we're dealing with here is a control that is meant to be writable, but\n> > > happens to only have a single supported value.\n> > \n> > This has bothered me all the time I tried to find a good term to\n> > identify those \"booleans with a single value\". I proposed 'identity'\n> > but that's not really up to the point. The fact I can't find a term\n> > for that makes me think I'm missing something and we should probably\n> > be doing something different. Hence the proposal to have a dedicated\n> > constructor, but yes, more constructors are ambiguos.\n> > \n> > >\n> > > Taking one step back, what are our use cases for boolean controls that\n> > > accept a single value ?\n> > >\n> > \n> > I think the original intent was to be able to express things like: the\n> > IPA only supports AE_ENABLE, so that you register a\n> > \n> > {controls::AeEnable, ControlInfo(true) }\n> > \n> > While if you can enable/disable it, and it's enabled by default:\n> > \n> > {controls::AeEnable, ControlInfo({true, false}, true) }\n> > \n> > I would now suggest to simply not register the control at all if it's\n> > not mutable, but I'm sure right now I'm missing why this was not\n> > possible in first place. Maybe it's just a matter of defining a policy\n> > like this one: if a control (not a property, nor a metadata) cannot be\n> > changed, just don't register it as part of the Camera::controls\" ?\n\nOh huh, I see. The reason we're having this issue is because we want to\nbe able to report that \"we support AE\" and \"we don't support turning off\nAE\" (or the reverse, \"we don't support AE\"). In that case maybe\nproperties like \"AeAvailable\" and \"AeTurnoffable\" would serve the\nunchangable control?\n\nBut then we have properties that just mirror controls... Is using\ncontrols not the best way to report capabilities? They have attached\nControlInfos showing the valid values so I thought they were good.\n\nThat's why imo I think that ControlInfo({ true, false }, true) is fine.\nSure it's redundant information, but it's clear: \"I want a boolean\ncontrol that supports both true and false, and the default value is\ntrue\". We have assertions to protect against dumb/malicious users that\ntry weird stuff like ControlInfo({ true, true }, false). And then if you\nthe user goes \"I want a boolean control that supports only true\", then\nthe default value is implied, and ControlInfo(true) is sufficient.\n\nSorry it's just kinda rambling :p\n\n\nPaul\n\n> \n> In that case applications wouldn't be able to differentiate a device\n> that doesn't support auto-exposure and a device that doesn't support\n> manual exposure. OK, not entirely true, applications could check for the\n> presence of a manual exposure time control, but in general, we could\n> have other boolean controls that would suffer from this issue.\n> \n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index 1bc958a43b22..5ef84a92f219 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -272,11 +272,16 @@ public:\n> > > > const ControlValue &def = 0);\n> > > > explicit ControlInfo(Span values,\n> > > > const ControlValue &def = {});\n> > > > + explicit ControlInfo(bool value)\n> > > > + : min_(false), max_(true), def_(value)\n> > > > + {\n> > > > + }\n> > > >\n> > > > const ControlValue &min() const { return min_; }\n> > > > const ControlValue &max() const { return max_; }\n> > > > const ControlValue &def() const { return def_; }\n> > > > const std::vector &values() const { return values_; }\n> > > > + bool rw() const { return rw_; }\n> > > >\n> > > > std::string toString() const;\n> > > >\n> > > > @@ -290,13 +295,41 @@ public:\n> > > > return !(*this == other);\n> > > > }\n> > > >\n> > > > -private:\n> > > > +protected:\n> > > > + bool rw_ : true;\n> > > > ControlValue min_;\n> > > > ControlValue max_;\n> > > > ControlValue def_;\n> > > > std::vector values_;\n> > > > };\n> > > >\n> > > > +class ROControlInfo : public ControlInfo\n> > > > +{\n> > > > + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n> > > > +\n> > > > + explicit ROControlInfo(const ControlValue &min = 0,\n> > > > + const ControlValue &max = 0,\n> > > > + const ControlValue &def = 0)\n> > > > + : ControlInfo(min, max, def)\n> > > > + {\n> > > > + rw_ = false;\n> > > > + }\n> > > > +\n> > > > + explicit ROControlInfo(Span values,\n> > > > + const ControlValue &def = {})\n> > > > + : ControlInfo(values, def)\n> > > > + {\n> > > > + rw_ = false;\n> > > > + }\n> > > > +\n> > > > + explicit ROControlInfo(bool value)\n> > > > + {\n> > > > + min_ = max_ = def_ = value;\n> > > > + rw_ = false;\n> > > > + }\n> > > > +};\n> > > > +\n> > > >\n> > > > I'm sure it could be done in a smarter way than this, I'm just wonder\n> > > > if it's worth it and IPA/ph should be in charge of deciding what type\n> > > > of ControlInfo to construct. For the generator, once we have a\n> > > > read_only flag, this should be trivial instead.\n> > > >\n> > > > > Now that I've started the brainstorming, I'll let you unleash your brain\n> > > > > power to shoot this down or improve it :-)\n> > > > >\n> > > > > > > mind, I guess we could now live with this ?\n> > > > > > >\n> > > > > > > > + *\n> > > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > > > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > > > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > > > > > + * default value is \\a def.\n> > > > > > > > + */\n> > > > > > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > > > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > > > > > +{\n> > > > > > > > +\tassert(values.count(def) && values.size() == 2);\n> > > > > > >\n> > > > > > > Be aware this will accept ControlInfo({true, true}, false);\n> > > > > >\n> > > > > > It's a std::set so if you do { true, true } then values.size()\n> > > > > > should be 1.\n> > > > > >\n> > > > > > At least that was my reasoning. Is it wrong?\n> > > > > >\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > > > > > + * \\param[in] value The control valid boolean value\n> > > > > > > > + *\n> > > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > > > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > > > > > + */\n> > > > > > > > +ControlInfo::ControlInfo(bool value)\n> > > > > > > > +\t: min_(value), max_(value), def_(value)\n> > > > > > > > +{\n> > > > > > > > +\tvalues_ = { value };\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > /**\n> > > > > > > > * \\fn ControlInfo::min()\n> > > > > > > > * \\brief Retrieve the minimum value of the control\n> > > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > > > > > index 1e05e131..2827473b 100644\n> > > > > > > > --- a/test/controls/control_info.cpp\n> > > > > > > > +++ b/test/controls/control_info.cpp\n> > > > > > > > @@ -44,6 +44,39 @@ protected:\n> > > > > > > > \t\t\treturn TestFail;\n> > > > > > > > \t\t}\n> > > > > > > >\n> > > > > > > > +\t\t/*\n> > > > > > > > +\t\t * Test information retrieval from a control with boolean\n> > > > > > > > +\t\t * values.\n> > > > > > > > +\t\t */\n> > > > > > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > > > > > +\n> > > > > > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > > > > > +\t\t aeEnable.def().get() != false ||\n> > > > > > > > +\t\t aeEnable.max().get() != true) {\n> > > > > > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > +\t\t}\n> > > > > > > > +\n> > > > > > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > > > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > > > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > +\t\t}\n> > > > > > > > +\n> > > > > > > > +\t\tControlInfo awbEnable(true);\n> > > > > > > > +\n> > > > > > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > > > > > +\t\t awbEnable.def().get() != true ||\n> > > > > > > > +\t\t awbEnable.max().get() != true) {\n> > > > > > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > +\t\t}\n> > > > > > > > +\n> > > > > > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > > > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > +\t\t}\n> > > > > > > > +\n> > > > > > >\n> > > > > > > As said, not in love with this, but I don't have anything better to\n> > > > > > > suggest :)\n> > > > > > >\n> > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > >\n> > > > > > > > \t\treturn TestPass;\n> > > > > > > > \t}\n> > > > > > > > };\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 ABD3EC322E\n\tfor ;\n\tThu, 29 Jul 2021 10:34:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15C3C687BF;\n\tThu, 29 Jul 2021 12:34: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 9FFE2687BA\n\tfor ;\n\tThu, 29 Jul 2021 12:34:45 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C3C809FB;\n\tThu, 29 Jul 2021 12:34:43 +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=\"n8Ntqvx+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627554885;\n\tbh=Y9BLnBvpudj6gzjE0DefDk9YOgIEYDgvO00XjvTmEXA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n8Ntqvx+zwNFuFEJQU7YRnKnGvDLSxMc74Oa9T0gteaXSrz+5la+t2vae9DQYcayf\n\tUJbIbdlvzqoxSzUmXtbauL8DR+mGR4CylBBFuezzkQJ0p+xQ3a4jzkSGSmdT8PyonV\n\tHotdxK383MctOh2awCw7eL+4KDIgPgs5mVDffh4w=","Date":"Thu, 29 Jul 2021 19:34:37 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210729103437.GL2167@pyrite.rasen.tech>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t\n\t<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>\n\t\n\t<20210728163159.ote6eckfegk3xe76@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18472,"web_url":"https://patchwork.libcamera.org/comment/18472/","msgid":"","date":"2021-08-02T01:08:19","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Thu, Jul 29, 2021 at 07:34:37PM +0900, paul.elder@ideasonboard.com wrote:\n> On Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:\n> > On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:\n> > > On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:\n> > > > Hi Jacopo,\n> > > \n> > > [snip]\n> > > \n> > > > > > The following would be one way to add a bool-specific constructor that\n> > > > > > will have no risk of clashing with anything in the future:\n> > > > > >\n> > > > > > \tenum Mutable {\n> > > > > > \t\tReadOnly,\n> > > > > > \t\tReadWrite,\n> > > > > > \t};\n> > > > > >\n> > > > > > \tControlInfo(bool default, Mutable mutable);\n> > > > > >\n> > > > > > (the Mutable, ReadOnly and ReadWrite names can most probably be\n> > > > > > improved). One would use this, mapping to the four cases above, as\n> > > > > >\n> > > > > > \tControlInfo(false, ControlInfo::ReadWrite);\n> > > > > > \tControlInfo(true, ControlInfo::ReadWrite);\n> > > > > > \tControlInfo(false, ControlInfo::ReadOnly);\n> > > > > > \tControlInfo(true, ControlInfo::ReadOnly);\n> > > > > >\n> > > > > > Maybe writing it differently would be more explicit:\n> > > > > >\n> > > > > > \tControlInfo(ControlInfo::MutableBool, false);\n> > > > > > \tControlInfo(ControlInfo::MutableBool, true);\n> > > > > > \tControlInfo(ControlInfo::ImmutableBool, false);\n> > > > > > \tControlInfo(ControlInfo::ImmutableBool, true);\n> > > > >\n> > > > > Ah, this could solve the concerns I expressed above.\n> > > > >\n> > > > > We have the general need to express when a Control is RO or RW, don't\n> > > > > we ? We could add an rw_ flag to the ControlInfo class for that and\n> > > > > derive a read-only variant maybe ?\n> > > >\n> > > > Thinking more about this, the read-only concept bothers me a bit. A\n> > > > read-only control is essentially either a property or a metadata in the\n> > > > general case (depending on whether the value is static or dynamic). What\n> > > > we're dealing with here is a control that is meant to be writable, but\n> > > > happens to only have a single supported value.\n> > > \n> > > This has bothered me all the time I tried to find a good term to\n> > > identify those \"booleans with a single value\". I proposed 'identity'\n> > > but that's not really up to the point. The fact I can't find a term\n> > > for that makes me think I'm missing something and we should probably\n> > > be doing something different. Hence the proposal to have a dedicated\n> > > constructor, but yes, more constructors are ambiguos.\n> > > \n> > > > Taking one step back, what are our use cases for boolean controls that\n> > > > accept a single value ?\n> > > \n> > > I think the original intent was to be able to express things like: the\n> > > IPA only supports AE_ENABLE, so that you register a\n> > > \n> > > {controls::AeEnable, ControlInfo(true) }\n> > > \n> > > While if you can enable/disable it, and it's enabled by default:\n> > > \n> > > {controls::AeEnable, ControlInfo({true, false}, true) }\n> > > \n> > > I would now suggest to simply not register the control at all if it's\n> > > not mutable, but I'm sure right now I'm missing why this was not\n> > > possible in first place. Maybe it's just a matter of defining a policy\n> > > like this one: if a control (not a property, nor a metadata) cannot be\n> > > changed, just don't register it as part of the Camera::controls\" ?\n> \n> Oh huh, I see. The reason we're having this issue is because we want to\n> be able to report that \"we support AE\" and \"we don't support turning off\n> AE\" (or the reverse, \"we don't support AE\"). In that case maybe\n> properties like \"AeAvailable\" and \"AeTurnoffable\" would serve the\n> unchangable control?\n> \n> But then we have properties that just mirror controls... Is using\n> controls not the best way to report capabilities? They have attached\n> ControlInfos showing the valid values so I thought they were good.\n\nThis is an area where we depart from the Android camera HAL API. In\nAndroid, meta-information about controls (does this make it camera\nmeta-meta-data ? :-)) is reported as static metadata, while in\nlibcamera, we report them through ControlInfo. I like ControlInfo as it\ncarries a clear association with controls, while in Android, one has to\nknow which static metadata corresponds to which control metadata (or\npossibly dynamic metadata). Static information that are not related to\ncontrols are reported in libcamera through properties, which as\nequivalent to the Android static metadata. There's one shortcoming in\nour approach though, we have no way to report information about metadata\n(in the libcamera sense, as reported through requests, the equivalent of\nthe Android dynamic metadata). Maybe we would need Camera::metadata()\nfor that.\n\nAnyway, at this point I don't think we should switch to creating more\nproperties to report information about controls.\n\n> That's why imo I think that ControlInfo({ true, false }, true) is fine.\n> Sure it's redundant information, but it's clear: \"I want a boolean\n> control that supports both true and false, and the default value is\n> true\". We have assertions to protect against dumb/malicious users that\n> try weird stuff like ControlInfo({ true, true }, false). And then if you\n> the user goes \"I want a boolean control that supports only true\", then\n> the default value is implied, and ControlInfo(true) is sufficient.\n\nAt the risk of repeating myself, this bothers me for a few reasons:\n\n- We have a similar issue with enumerated controls (not having a nice\n syntax to create a ControlInfo with a set of enum values and a\n default), and bool controls can be considered as a special case of an\n enumeration with only two values, so it would be nice to have a single\n API (or at least single syntax, even if the constructors end up being\n different) to handle both. I'm not sure if this is possible.\n\n- There's room for getting it wrong by passing ({ true, true }, false).\n As you mentioned, we have asserts to catch this, but making errors\n impossible in the first place would create a nicer API.\n\n- ControlInfo(true) (or false) is really confusing to read, more, I\n believe, than ControlInfo({ true, true }, true). Furthermore, the more\n constructors we have, especially with common types, the more we'll\n risk running into a situation where we won't be able to add a new\n constructor due to ambiguous overload resolution. I think writing\n ControlInfo({ true }, true) would be better than ControlInfo(true)\n from that point of view.\n\nNone of those are showstoppers, but they make me think we can do better.\n\n> Sorry it's just kinda rambling :p\n\nThat's what I've been doing as well so far, so no need to apologize :-)\n\n> > In that case applications wouldn't be able to differentiate a device\n> > that doesn't support auto-exposure and a device that doesn't support\n> > manual exposure. OK, not entirely true, applications could check for the\n> > presence of a manual exposure time control, but in general, we could\n> > have other boolean controls that would suffer from this issue.\n> > \n> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > index 1bc958a43b22..5ef84a92f219 100644\n> > > > > --- a/include/libcamera/controls.h\n> > > > > +++ b/include/libcamera/controls.h\n> > > > > @@ -272,11 +272,16 @@ public:\n> > > > > const ControlValue &def = 0);\n> > > > > explicit ControlInfo(Span values,\n> > > > > const ControlValue &def = {});\n> > > > > + explicit ControlInfo(bool value)\n> > > > > + : min_(false), max_(true), def_(value)\n> > > > > + {\n> > > > > + }\n> > > > >\n> > > > > const ControlValue &min() const { return min_; }\n> > > > > const ControlValue &max() const { return max_; }\n> > > > > const ControlValue &def() const { return def_; }\n> > > > > const std::vector &values() const { return values_; }\n> > > > > + bool rw() const { return rw_; }\n> > > > >\n> > > > > std::string toString() const;\n> > > > >\n> > > > > @@ -290,13 +295,41 @@ public:\n> > > > > return !(*this == other);\n> > > > > }\n> > > > >\n> > > > > -private:\n> > > > > +protected:\n> > > > > + bool rw_ : true;\n> > > > > ControlValue min_;\n> > > > > ControlValue max_;\n> > > > > ControlValue def_;\n> > > > > std::vector values_;\n> > > > > };\n> > > > >\n> > > > > +class ROControlInfo : public ControlInfo\n> > > > > +{\n> > > > > + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n> > > > > +\n> > > > > + explicit ROControlInfo(const ControlValue &min = 0,\n> > > > > + const ControlValue &max = 0,\n> > > > > + const ControlValue &def = 0)\n> > > > > + : ControlInfo(min, max, def)\n> > > > > + {\n> > > > > + rw_ = false;\n> > > > > + }\n> > > > > +\n> > > > > + explicit ROControlInfo(Span values,\n> > > > > + const ControlValue &def = {})\n> > > > > + : ControlInfo(values, def)\n> > > > > + {\n> > > > > + rw_ = false;\n> > > > > + }\n> > > > > +\n> > > > > + explicit ROControlInfo(bool value)\n> > > > > + {\n> > > > > + min_ = max_ = def_ = value;\n> > > > > + rw_ = false;\n> > > > > + }\n> > > > > +};\n> > > > > +\n> > > > >\n> > > > > I'm sure it could be done in a smarter way than this, I'm just wonder\n> > > > > if it's worth it and IPA/ph should be in charge of deciding what type\n> > > > > of ControlInfo to construct. For the generator, once we have a\n> > > > > read_only flag, this should be trivial instead.\n> > > > >\n> > > > > > Now that I've started the brainstorming, I'll let you unleash your brain\n> > > > > > power to shoot this down or improve it :-)\n> > > > > >\n> > > > > > > > mind, I guess we could now live with this ?\n> > > > > > > >\n> > > > > > > > > + *\n> > > > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > > > > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > > > > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > > > > > > + * default value is \\a def.\n> > > > > > > > > + */\n> > > > > > > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > > > > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > > > > > > +{\n> > > > > > > > > +\tassert(values.count(def) && values.size() == 2);\n> > > > > > > >\n> > > > > > > > Be aware this will accept ControlInfo({true, true}, false);\n> > > > > > >\n> > > > > > > It's a std::set so if you do { true, true } then values.size()\n> > > > > > > should be 1.\n> > > > > > >\n> > > > > > > At least that was my reasoning. Is it wrong?\n> > > > > > >\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > +/**\n> > > > > > > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > > > > > > + * \\param[in] value The control valid boolean value\n> > > > > > > > > + *\n> > > > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > > > > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > > > > > > + */\n> > > > > > > > > +ControlInfo::ControlInfo(bool value)\n> > > > > > > > > +\t: min_(value), max_(value), def_(value)\n> > > > > > > > > +{\n> > > > > > > > > +\tvalues_ = { value };\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > /**\n> > > > > > > > > * \\fn ControlInfo::min()\n> > > > > > > > > * \\brief Retrieve the minimum value of the control\n> > > > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > > > > > > index 1e05e131..2827473b 100644\n> > > > > > > > > --- a/test/controls/control_info.cpp\n> > > > > > > > > +++ b/test/controls/control_info.cpp\n> > > > > > > > > @@ -44,6 +44,39 @@ protected:\n> > > > > > > > > \t\t\treturn TestFail;\n> > > > > > > > > \t\t}\n> > > > > > > > >\n> > > > > > > > > +\t\t/*\n> > > > > > > > > +\t\t * Test information retrieval from a control with boolean\n> > > > > > > > > +\t\t * values.\n> > > > > > > > > +\t\t */\n> > > > > > > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > > > > > > +\n> > > > > > > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > > > > > > +\t\t aeEnable.def().get() != false ||\n> > > > > > > > > +\t\t aeEnable.max().get() != true) {\n> > > > > > > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > +\t\t}\n> > > > > > > > > +\n> > > > > > > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > > > > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > > > > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > +\t\t}\n> > > > > > > > > +\n> > > > > > > > > +\t\tControlInfo awbEnable(true);\n> > > > > > > > > +\n> > > > > > > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > > > > > > +\t\t awbEnable.def().get() != true ||\n> > > > > > > > > +\t\t awbEnable.max().get() != true) {\n> > > > > > > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > +\t\t}\n> > > > > > > > > +\n> > > > > > > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > > > > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > +\t\t}\n> > > > > > > > > +\n> > > > > > > >\n> > > > > > > > As said, not in love with this, but I don't have anything better to\n> > > > > > > > suggest :)\n> > > > > > > >\n> > > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > >\n> > > > > > > > > \t\treturn TestPass;\n> > > > > > > > > \t}\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 13582BD878\n\tfor ;\n\tMon, 2 Aug 2021 01:08:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 923FA687C5;\n\tMon, 2 Aug 2021 03:08:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E2D94687B6\n\tfor ;\n\tMon, 2 Aug 2021 03:08:30 +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 504B44A3;\n\tMon, 2 Aug 2021 03:08:30 +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=\"K+ICs9lE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627866510;\n\tbh=YWS76D8LfOWf7Mf+isYML4gwQ4lymQhbGU042RceVDs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K+ICs9lE/bDD6Wo8nPZLVt01GhK+9i+9OuuajSgQ7XmG8rVGjRPGTTXptuN98OI5M\n\tBvemHLmNABR/sm50z7BmWClLW9YD0f0sXihS/CPhlytiHxqaZgMzWpseoGoYb8E7rU\n\tcmzJIWWYzp6KNYSMnLH8xZPNcXgZLYaGbiBO6k4Y=","Date":"Mon, 2 Aug 2021 04:08:19 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t\n\t<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>\n\t\n\t<20210728163159.ote6eckfegk3xe76@uno.localdomain>\n\t\n\t<20210729103437.GL2167@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210729103437.GL2167@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18745,"web_url":"https://patchwork.libcamera.org/comment/18745/","msgid":"","date":"2021-08-12T20:42:56","subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for ControlInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Mon, Aug 02, 2021 at 04:08:22AM +0300, Laurent Pinchart wrote:\n> On Thu, Jul 29, 2021 at 07:34:37PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:\n> > > On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:\n> > > > On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:\n> > > > > Hi Jacopo,\n> > > > \n> > > > [snip]\n> > > > \n> > > > > > > The following would be one way to add a bool-specific constructor that\n> > > > > > > will have no risk of clashing with anything in the future:\n> > > > > > >\n> > > > > > > \tenum Mutable {\n> > > > > > > \t\tReadOnly,\n> > > > > > > \t\tReadWrite,\n> > > > > > > \t};\n> > > > > > >\n> > > > > > > \tControlInfo(bool default, Mutable mutable);\n> > > > > > >\n> > > > > > > (the Mutable, ReadOnly and ReadWrite names can most probably be\n> > > > > > > improved). One would use this, mapping to the four cases above, as\n> > > > > > >\n> > > > > > > \tControlInfo(false, ControlInfo::ReadWrite);\n> > > > > > > \tControlInfo(true, ControlInfo::ReadWrite);\n> > > > > > > \tControlInfo(false, ControlInfo::ReadOnly);\n> > > > > > > \tControlInfo(true, ControlInfo::ReadOnly);\n> > > > > > >\n> > > > > > > Maybe writing it differently would be more explicit:\n> > > > > > >\n> > > > > > > \tControlInfo(ControlInfo::MutableBool, false);\n> > > > > > > \tControlInfo(ControlInfo::MutableBool, true);\n> > > > > > > \tControlInfo(ControlInfo::ImmutableBool, false);\n> > > > > > > \tControlInfo(ControlInfo::ImmutableBool, true);\n> > > > > >\n> > > > > > Ah, this could solve the concerns I expressed above.\n> > > > > >\n> > > > > > We have the general need to express when a Control is RO or RW, don't\n> > > > > > we ? We could add an rw_ flag to the ControlInfo class for that and\n> > > > > > derive a read-only variant maybe ?\n> > > > >\n> > > > > Thinking more about this, the read-only concept bothers me a bit. A\n> > > > > read-only control is essentially either a property or a metadata in the\n> > > > > general case (depending on whether the value is static or dynamic). What\n> > > > > we're dealing with here is a control that is meant to be writable, but\n> > > > > happens to only have a single supported value.\n> > > > \n> > > > This has bothered me all the time I tried to find a good term to\n> > > > identify those \"booleans with a single value\". I proposed 'identity'\n> > > > but that's not really up to the point. The fact I can't find a term\n> > > > for that makes me think I'm missing something and we should probably\n> > > > be doing something different. Hence the proposal to have a dedicated\n> > > > constructor, but yes, more constructors are ambiguos.\n> > > > \n> > > > > Taking one step back, what are our use cases for boolean controls that\n> > > > > accept a single value ?\n> > > > \n> > > > I think the original intent was to be able to express things like: the\n> > > > IPA only supports AE_ENABLE, so that you register a\n> > > > \n> > > > {controls::AeEnable, ControlInfo(true) }\n> > > > \n> > > > While if you can enable/disable it, and it's enabled by default:\n> > > > \n> > > > {controls::AeEnable, ControlInfo({true, false}, true) }\n> > > > \n> > > > I would now suggest to simply not register the control at all if it's\n> > > > not mutable, but I'm sure right now I'm missing why this was not\n> > > > possible in first place. Maybe it's just a matter of defining a policy\n> > > > like this one: if a control (not a property, nor a metadata) cannot be\n> > > > changed, just don't register it as part of the Camera::controls\" ?\n> > \n> > Oh huh, I see. The reason we're having this issue is because we want to\n> > be able to report that \"we support AE\" and \"we don't support turning off\n> > AE\" (or the reverse, \"we don't support AE\"). In that case maybe\n> > properties like \"AeAvailable\" and \"AeTurnoffable\" would serve the\n> > unchangable control?\n> > \n> > But then we have properties that just mirror controls... Is using\n> > controls not the best way to report capabilities? They have attached\n> > ControlInfos showing the valid values so I thought they were good.\n> \n> This is an area where we depart from the Android camera HAL API. In\n> Android, meta-information about controls (does this make it camera\n> meta-meta-data ? :-)) is reported as static metadata, while in\n> libcamera, we report them through ControlInfo. I like ControlInfo as it\n> carries a clear association with controls, while in Android, one has to\n> know which static metadata corresponds to which control metadata (or\n> possibly dynamic metadata). Static information that are not related to\n> controls are reported in libcamera through properties, which as\n> equivalent to the Android static metadata. There's one shortcoming in\n> our approach though, we have no way to report information about metadata\n> (in the libcamera sense, as reported through requests, the equivalent of\n> the Android dynamic metadata). Maybe we would need Camera::metadata()\n> for that.\n> \n> Anyway, at this point I don't think we should switch to creating more\n> properties to report information about controls.\n> \n> > That's why imo I think that ControlInfo({ true, false }, true) is fine.\n> > Sure it's redundant information, but it's clear: \"I want a boolean\n> > control that supports both true and false, and the default value is\n> > true\". We have assertions to protect against dumb/malicious users that\n> > try weird stuff like ControlInfo({ true, true }, false). And then if you\n> > the user goes \"I want a boolean control that supports only true\", then\n> > the default value is implied, and ControlInfo(true) is sufficient.\n> \n> At the risk of repeating myself, this bothers me for a few reasons:\n> \n> - We have a similar issue with enumerated controls (not having a nice\n> syntax to create a ControlInfo with a set of enum values and a\n> default), and bool controls can be considered as a special case of an\n> enumeration with only two values, so it would be nice to have a single\n> API (or at least single syntax, even if the constructors end up being\n> different) to handle both. I'm not sure if this is possible.\n> \n> - There's room for getting it wrong by passing ({ true, true }, false).\n> As you mentioned, we have asserts to catch this, but making errors\n> impossible in the first place would create a nicer API.\n> \n> - ControlInfo(true) (or false) is really confusing to read, more, I\n> believe, than ControlInfo({ true, true }, true). Furthermore, the more\n> constructors we have, especially with common types, the more we'll\n> risk running into a situation where we won't be able to add a new\n> constructor due to ambiguous overload resolution. I think writing\n> ControlInfo({ true }, true) would be better than ControlInfo(true)\n> from that point of view.\n> \n> None of those are showstoppers, but they make me think we can do better.\n\nI notice this got merged, even though the discussion hasn't come to a\nconclusion. Not really an issue as such, but I'd like to revive this\nmail thread. Any opinion on the three points above ?\n\n> > Sorry it's just kinda rambling :p\n> \n> That's what I've been doing as well so far, so no need to apologize :-)\n> \n> > > In that case applications wouldn't be able to differentiate a device\n> > > that doesn't support auto-exposure and a device that doesn't support\n> > > manual exposure. OK, not entirely true, applications could check for the\n> > > presence of a manual exposure time control, but in general, we could\n> > > have other boolean controls that would suffer from this issue.\n> > > \n> > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > > index 1bc958a43b22..5ef84a92f219 100644\n> > > > > > --- a/include/libcamera/controls.h\n> > > > > > +++ b/include/libcamera/controls.h\n> > > > > > @@ -272,11 +272,16 @@ public:\n> > > > > > const ControlValue &def = 0);\n> > > > > > explicit ControlInfo(Span values,\n> > > > > > const ControlValue &def = {});\n> > > > > > + explicit ControlInfo(bool value)\n> > > > > > + : min_(false), max_(true), def_(value)\n> > > > > > + {\n> > > > > > + }\n> > > > > >\n> > > > > > const ControlValue &min() const { return min_; }\n> > > > > > const ControlValue &max() const { return max_; }\n> > > > > > const ControlValue &def() const { return def_; }\n> > > > > > const std::vector &values() const { return values_; }\n> > > > > > + bool rw() const { return rw_; }\n> > > > > >\n> > > > > > std::string toString() const;\n> > > > > >\n> > > > > > @@ -290,13 +295,41 @@ public:\n> > > > > > return !(*this == other);\n> > > > > > }\n> > > > > >\n> > > > > > -private:\n> > > > > > +protected:\n> > > > > > + bool rw_ : true;\n> > > > > > ControlValue min_;\n> > > > > > ControlValue max_;\n> > > > > > ControlValue def_;\n> > > > > > std::vector values_;\n> > > > > > };\n> > > > > >\n> > > > > > +class ROControlInfo : public ControlInfo\n> > > > > > +{\n> > > > > > + LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);\n> > > > > > +\n> > > > > > + explicit ROControlInfo(const ControlValue &min = 0,\n> > > > > > + const ControlValue &max = 0,\n> > > > > > + const ControlValue &def = 0)\n> > > > > > + : ControlInfo(min, max, def)\n> > > > > > + {\n> > > > > > + rw_ = false;\n> > > > > > + }\n> > > > > > +\n> > > > > > + explicit ROControlInfo(Span values,\n> > > > > > + const ControlValue &def = {})\n> > > > > > + : ControlInfo(values, def)\n> > > > > > + {\n> > > > > > + rw_ = false;\n> > > > > > + }\n> > > > > > +\n> > > > > > + explicit ROControlInfo(bool value)\n> > > > > > + {\n> > > > > > + min_ = max_ = def_ = value;\n> > > > > > + rw_ = false;\n> > > > > > + }\n> > > > > > +};\n> > > > > > +\n> > > > > >\n> > > > > > I'm sure it could be done in a smarter way than this, I'm just wonder\n> > > > > > if it's worth it and IPA/ph should be in charge of deciding what type\n> > > > > > of ControlInfo to construct. For the generator, once we have a\n> > > > > > read_only flag, this should be trivial instead.\n> > > > > >\n> > > > > > > Now that I've started the brainstorming, I'll let you unleash your brain\n> > > > > > > power to shoot this down or improve it :-)\n> > > > > > >\n> > > > > > > > > mind, I guess we could now live with this ?\n> > > > > > > > >\n> > > > > > > > > > + *\n> > > > > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are\n> > > > > > > > > > + * valid values. \\a values must be { false, true } (the order is irrelevant).\n> > > > > > > > > > + * The minimum value will always be false, and the maximum always true. The\n> > > > > > > > > > + * default value is \\a def.\n> > > > > > > > > > + */\n> > > > > > > > > > +ControlInfo::ControlInfo(std::set values, bool def)\n> > > > > > > > > > +\t: min_(false), max_(true), def_(def), values_({ false, true })\n> > > > > > > > > > +{\n> > > > > > > > > > +\tassert(values.count(def) && values.size() == 2);\n> > > > > > > > >\n> > > > > > > > > Be aware this will accept ControlInfo({true, true}, false);\n> > > > > > > >\n> > > > > > > > It's a std::set so if you do { true, true } then values.size()\n> > > > > > > > should be 1.\n> > > > > > > >\n> > > > > > > > At least that was my reasoning. Is it wrong?\n> > > > > > > >\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > +/**\n> > > > > > > > > > + * \\brief Construct a boolean ControlInfo with only one valid value\n> > > > > > > > > > + * \\param[in] value The control valid boolean value\n> > > > > > > > > > + *\n> > > > > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid\n> > > > > > > > > > + * value. The minimum, maximum, and default values will all be \\a value.\n> > > > > > > > > > + */\n> > > > > > > > > > +ControlInfo::ControlInfo(bool value)\n> > > > > > > > > > +\t: min_(value), max_(value), def_(value)\n> > > > > > > > > > +{\n> > > > > > > > > > +\tvalues_ = { value };\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > /**\n> > > > > > > > > > * \\fn ControlInfo::min()\n> > > > > > > > > > * \\brief Retrieve the minimum value of the control\n> > > > > > > > > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > > > > > > > > index 1e05e131..2827473b 100644\n> > > > > > > > > > --- a/test/controls/control_info.cpp\n> > > > > > > > > > +++ b/test/controls/control_info.cpp\n> > > > > > > > > > @@ -44,6 +44,39 @@ protected:\n> > > > > > > > > > \t\t\treturn TestFail;\n> > > > > > > > > > \t\t}\n> > > > > > > > > >\n> > > > > > > > > > +\t\t/*\n> > > > > > > > > > +\t\t * Test information retrieval from a control with boolean\n> > > > > > > > > > +\t\t * values.\n> > > > > > > > > > +\t\t */\n> > > > > > > > > > +\t\tControlInfo aeEnable({ false, true }, false);\n> > > > > > > > > > +\n> > > > > > > > > > +\t\tif (aeEnable.min().get() != false ||\n> > > > > > > > > > +\t\t aeEnable.def().get() != false ||\n> > > > > > > > > > +\t\t aeEnable.max().get() != true) {\n> > > > > > > > > > +\t\t\tcout << \"Invalid control range for AeEnable\" << endl;\n> > > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > > +\t\t}\n> > > > > > > > > > +\n> > > > > > > > > > +\t\tif (aeEnable.values()[0].get() != false ||\n> > > > > > > > > > +\t\t aeEnable.values()[1].get() != true) {\n> > > > > > > > > > +\t\t\tcout << \"Invalid control values for AeEnable\" << endl;\n> > > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > > +\t\t}\n> > > > > > > > > > +\n> > > > > > > > > > +\t\tControlInfo awbEnable(true);\n> > > > > > > > > > +\n> > > > > > > > > > +\t\tif (awbEnable.min().get() != true ||\n> > > > > > > > > > +\t\t awbEnable.def().get() != true ||\n> > > > > > > > > > +\t\t awbEnable.max().get() != true) {\n> > > > > > > > > > +\t\t\tcout << \"Invalid control range for AwbEnable\" << endl;\n> > > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > > +\t\t}\n> > > > > > > > > > +\n> > > > > > > > > > +\t\tif (awbEnable.values()[0].get() != true) {\n> > > > > > > > > > +\t\t\tcout << \"Invalid control values for AwbEnable\" << endl;\n> > > > > > > > > > +\t\t\treturn TestFail;\n> > > > > > > > > > +\t\t}\n> > > > > > > > > > +\n> > > > > > > > >\n> > > > > > > > > As said, not in love with this, but I don't have anything better to\n> > > > > > > > > suggest :)\n> > > > > > > > >\n> > > > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > > > > >\n> > > > > > > > > > \t\treturn TestPass;\n> > > > > > > > > > \t}\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 44A39C3240\n\tfor ;\n\tThu, 12 Aug 2021 20:43:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97B0168888;\n\tThu, 12 Aug 2021 22:43:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E38E068823\n\tfor ;\n\tThu, 12 Aug 2021 22:43:00 +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 38EDCEE;\n\tThu, 12 Aug 2021 22:43:00 +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=\"GpamBeQT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628800980;\n\tbh=g9w4R/KkYqH0EaRFISBnLoG9GtIv8bO79qp4eB8wXcU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GpamBeQTVtQTdBF4nY5E6O9n41n6EU8NGd5/j7BVW/+3YzQURxMyocsh6r+v1C041\n\tyrUneNYaI1l6qgHNLYaDBE3ju1VEXL7nNng2Xxznnq3ZQ7RPXsNfyrvwmplJy0C+mN\n\tZhDaXxfLBYyjFG3yG6D0U86Hh5VPjjiJFuB1sO2E=","Date":"Thu, 12 Aug 2021 23:42:56 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210720101307.26010-2-paul.elder@ideasonboard.com>\n\t<20210722130609.ih4igh37s4lm33vl@uno.localdomain>\n\t<20210723054552.GA63622@pyrite.rasen.tech>\n\t\n\t<20210726140807.v25z5ljeapv7a5ws@uno.localdomain>\n\t\n\t<20210728163159.ote6eckfegk3xe76@uno.localdomain>\n\t\n\t<20210729103437.GL2167@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 1/9] controls: Add boolean\n\tconstructors for 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]