[{"id":18206,"web_url":"https://patchwork.libcamera.org/comment/18206/","msgid":"<20210717093320.k2wa6zz6hom6bqay@uno.localdomain>","date":"2021-07-17T09:33:20","subject":"Re: [libcamera-devel] [RFC PATCH v4 01/21] controls: Add boolean\n\tconstructor 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 16, 2021 at 07:56:11PM +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 a new constructor to ControlInfo that takes a set of booleans (to\n> allow specifiying one or two available values), and a default. The\n> default value is not optional, as it doesn't make sense to have a silent\n> default for boolean values.\n>\n> Update the ControlInfo test accordingly.\n>\n> Signed-off-by: Paul Elder \n>\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 | 2 ++\n> src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++\n> test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++\n> 3 files changed, 63 insertions(+)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 1bc958a4..707dc335 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,7 @@ 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> \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 34317fa0..f6351c01 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -514,6 +514,34 @@ ControlInfo::ControlInfo(Span values,\n> \t\tvalues_.push_back(value);\n> }\n>\n> +/**\n> + * \\brief Construct a ControlInfo from a list of valid boolean values\n> + * \\param[in] values The control valid boolean vaalues\n\ns/vaalues/values/\n\n> + * \\param[in] def The control default boolean value\n> + *\n> + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo\n> + * minimum and maximum values are set to the first and last members of the\n> + * values list respectively. The default value is set to \\a def.\n\nstd::set is sorted, the order in which elements are listed is\nirrelevant as far as I can tell (and honestly I don't know if true <\nor > false :)\n\nI would remove the ambiguities and specify that if the ControlInfo\nsupports both {true, false} then min = false, max = true and default\nis what is passed as paramter. If the set of possible values only\ncontains true OR false, then min = max = def.\n\nI think getting the terminology right would really simplify this part.\nHow would you call a ControlInfo that supports both {true, false} and\nhow would you call one that only supported {true} or {false}. binary vs\nidentity ?\n\nSomething like:\n\n Construct a ControlInfo for a boolean control. The ControlInfo can\n represent a binary (??) control that allows both true and false to be\n selected, or it can represent an identity (??) control, that only\n allows a single value.\n\n ControlInfo for binary controls are constructed with {true, false}\n as first parameter (the order is not relevant) \\a values and the\n selected default value as provided in \\a def. The minimum value will\n always be false, and the maximum one will always be true.\n\n ControlInfo for identity controls are constructed with a set\n containing the single supported value as first parameter \\a values\n which will also match their minimum, maximum, and default values.\n\nI know wonder if, for indentities, it wouldn't be better a constructor\nas\n ControlInfo(bool)\n\n> + */\n> +ControlInfo::ControlInfo(std::set values, bool def)\n> +\t: def_(def)\n> +{\n> +\tif (values.size() == 1) {\n> +\t\tmin_ = max_ = *values.begin();\n\nMake sure def matches values[0]\nYou can make def default=false and if size() == 1 set it to the single\nsupported value, but that would still allow to write ({true}, false)\nI would make two constructors to be honest.\n\n> +\t} else {\n> +\t\tmin_ = false;\n> +\t\tmax_ = true;\n> +\t}\n\nAh, see, the order is not relevant!\n\n> +\n> +\tdef_ = def;\n\nIsn't this already assigned through the constructor initialization list ?\n\n> +\n> +\tassert(values.count(def));\n> +\n> +\tvalues_.reserve(2);\n> +\tfor (const bool &value : values)\n> +\t\tvalues_.push_back(value);\n\nvalues_ = {false, true} ?\n\nI would\n\n ControlInfo::ControlInfo(std::set values, bool def)\n {\n min_ = false;\n max_ = true;\n def_ = def;\n values_ = { false, true };\n\n return 0;\n }\n\n ControlInfo::ControlInfo(bool value)\n {\n min_ = max_ = def_ = value;\n values_ = { value };\n\n return 0;\n }\n\nWith the first version being a syntactic sugar as 'values' is not\nreally required, but serves to disambiguate the two contructors and\nmakes it nicer to read at the expense of typing in {false, true}.\n\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..b3d2bd54 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 }, 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> \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 CF2C4C0109\n\tfor ;\n\tSat, 17 Jul 2021 09:32:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF0A06853D;\n\tSat, 17 Jul 2021 11:32:49 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0301D6027F\n\tfor ;\n\tSat, 17 Jul 2021 11:32:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 722C560005;\n\tSat, 17 Jul 2021 09:32:33 +0000 (UTC)"],"Date":"Sat, 17 Jul 2021 11:33:20 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210717093320.k2wa6zz6hom6bqay@uno.localdomain>","References":"<20210716105631.158153-1-paul.elder@ideasonboard.com>\n\t<20210716105631.158153-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210716105631.158153-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v4 01/21] controls: Add boolean\n\tconstructor 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\" "}}]