[{"id":34243,"web_url":"https://patchwork.libcamera.org/comment/34243/","msgid":"<174731432844.329449.10966170717807072478@calcite>","date":"2025-05-15T13:05:28","subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Barnabás,\n\nThanks for the patch.\n\nQuoting Barnabás Pőcze (2025-05-15 14:30:39)\n> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n> \n> The contstructors introduced by that commit are not used anywhere,\n> and they do not match the existing practice for boolean controls.\n> \n> Specifically, every single boolean control is described by calling\n> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`\n> constructor. Crucially, that constructor does not set `values_`,\n> while the two removed contructors do. And whether or not `values_`\n> has any elements is currently used as an implicit sign to decide\n> whether or not the control is \"enum-like\", and those are assumed\n> to have type `int32_t`.\n\nI remember adding these constructors because if values_ is not populated then\napplications can't enumerate the possible values of the control. (I suppose it\nwas an oversight that those constructors didn't get selected properly, though).\n\nThere might be debate whether or not boolean controls needs to be iterated, but\nI had imagined that some platforms might want to report a control to be\nsupported, but only one boolean value is supported. Something like a uvc camera\ndeclaring that it doesn't have ae but it supports manual controls. (Even though\nwe might be moving away from boolean enable flags to enum mode flags)\n\nAs for determining whether or not a control is enum-like, I remember adding\nControlId::isArray(). Although that acts on ControlId as opposed to\nControlInfo... Is that insufficient?\n\n\nThanks,\n\nPaul\n\n> \n> For example, any boolean control described using any of the two\n> removed constructors would cause an assertion in failure in\n> `CameraSession::listControls()` when calling `value.get()`.\n> \n> Signed-off-by: Barnabás Pőcze \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 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 2ae4ec3d4..32fb31f78 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -10,7 +10,6 @@\n> #include \n> #include \n> #include \n> -#include \n> #include \n> #include \n> #include \n> @@ -334,8 +333,6 @@ public:\n> const ControlValue &def = {});\n> explicit ControlInfo(Span values,\n> const ControlValue &def = {});\n> - explicit ControlInfo(std::set values, bool def);\n> - explicit ControlInfo(bool value);\n> \n> const ControlValue &min() const { return min_; }\n> const ControlValue &max() const { return max_; }\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 70f6f6092..eaa34e70b 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n> values_.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> - * 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> - : min_(false), max_(true), def_(def), values_({ false, true })\n> -{\n> - ASSERT(values.count(def) && values.size() == 2);\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> - : min_(value), max_(value), def_(value)\n> -{\n> - values_ = { 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 e1bb43f0e..09c17ae63 100644\n> --- a/test/controls/control_info.cpp\n> +++ b/test/controls/control_info.cpp\n> @@ -46,39 +46,6 @@ protected:\n> return TestFail;\n> }\n> \n> - /*\n> - * Test information retrieval from a control with boolean\n> - * values.\n> - */\n> - ControlInfo aeEnable({ false, true }, false);\n> -\n> - if (aeEnable.min().get() != false ||\n> - aeEnable.def().get() != false ||\n> - aeEnable.max().get() != true) {\n> - cout << \"Invalid control range for AeEnable\" << endl;\n> - return TestFail;\n> - }\n> -\n> - if (aeEnable.values()[0].get() != false ||\n> - aeEnable.values()[1].get() != true) {\n> - cout << \"Invalid control values for AeEnable\" << endl;\n> - return TestFail;\n> - }\n> -\n> - ControlInfo awbEnable(true);\n> -\n> - if (awbEnable.min().get() != true ||\n> - awbEnable.def().get() != true ||\n> - awbEnable.max().get() != true) {\n> - cout << \"Invalid control range for AwbEnable\" << endl;\n> - return TestFail;\n> - }\n> -\n> - if (awbEnable.values()[0].get() != true) {\n> - cout << \"Invalid control values for AwbEnable\" << endl;\n> - return TestFail;\n> - }\n> -\n> return TestPass;\n> }\n> };\n> -- \n> 2.49.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 66548C3220\n\tfor ;\n\tThu, 15 May 2025 13:05:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3346568C92;\n\tThu, 15 May 2025 15:05:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF4F468B4F\n\tfor ;\n\tThu, 15 May 2025 15:05:31 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2a01:cb16:204c:4644:2597:989f:e013:9296])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9F7C5886;\n\tThu, 15 May 2025 15:05:14 +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=\"lOSMixmI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747314314;\n\tbh=9CqKfmX6/22tVdXtOVO3kg0VWZ2BV6OluoekdiWQRds=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=lOSMixmIFcDcwVZpT4ulSaSFtwxa8Khx0HYKyVD07aIK2VNR+2H+dYtZJ7U06jgWJ\n\tEPUIOFJCp9gI4d5L+YfN7DpNi4KfizfnlojOtJIXcgf4DXpTzVSHnaQFHJR/K3T+yl\n\twYhtMi1hZoLn0lFh/X9WdrIUOJnQF2440ZUTqvIM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>","References":"<20250515123039.3134682-1-barnabas.pocze@ideasonboard.com>\n\t<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 15 May 2025 15:05:28 +0200","Message-ID":"<174731432844.329449.10966170717807072478@calcite>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34247,"web_url":"https://patchwork.libcamera.org/comment/34247/","msgid":"<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>","date":"2025-05-15T13:27:02","subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 05. 15. 15:05 keltezéssel, Paul Elder írta:\n> Hi Barnabás,\n> \n> Thanks for the patch.\n> \n> Quoting Barnabás Pőcze (2025-05-15 14:30:39)\n>> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n>>\n>> The contstructors introduced by that commit are not used anywhere,\n>> and they do not match the existing practice for boolean controls.\n>>\n>> Specifically, every single boolean control is described by calling\n>> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`\n>> constructor. Crucially, that constructor does not set `values_`,\n>> while the two removed contructors do. And whether or not `values_`\n>> has any elements is currently used as an implicit sign to decide\n>> whether or not the control is \"enum-like\", and those are assumed\n>> to have type `int32_t`.\n> \n> I remember adding these constructors because if values_ is not populated then\n> applications can't enumerate the possible values of the control. (I suppose it\n> was an oversight that those constructors didn't get selected properly, though).\n\nOne could argue that for boolean controls min()/max() can be sufficient...\n\n\n> \n> There might be debate whether or not boolean controls needs to be iterated, but\n> I had imagined that some platforms might want to report a control to be\n> supported, but only one boolean value is supported. Something like a uvc camera\n> declaring that it doesn't have ae but it supports manual controls. (Even though\n> we might be moving away from boolean enable flags to enum mode flags)\n\n... min()/max() can be the same if only a single boolean value is actually allowed.\n\n\n> \n> As for determining whether or not a control is enum-like, I remember adding\n> ControlId::isArray(). Although that acts on ControlId as opposed to\n> ControlInfo... Is that insufficient?\n\nI'm not sure if `ControlId::isArray()` does that. Maybe you meant\n`ControlId::enumerators()`? I think that could work, but e.g.\n`CameraSession::listControls()` does not use it at the moment.\n\nIn any case, I'm just saying that the current situation is not ideal,\nand something should be changed. And this seemed like the least intrusive\nfirst step. (Although admittedly I do not have further steps in mind.)\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> \n> Thanks,\n> \n> Paul\n> \n>>\n>> For example, any boolean control described using any of the two\n>> removed constructors would cause an assertion in failure in\n>> `CameraSession::listControls()` when calling `value.get()`.\n>>\n>> Signed-off-by: Barnabás Pőcze \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 deletions(-)\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> index 2ae4ec3d4..32fb31f78 100644\n>> --- a/include/libcamera/controls.h\n>> +++ b/include/libcamera/controls.h\n>> @@ -10,7 +10,6 @@\n>> #include \n>> #include \n>> #include \n>> -#include \n>> #include \n>> #include \n>> #include \n>> @@ -334,8 +333,6 @@ public:\n>> const ControlValue &def = {});\n>> explicit ControlInfo(Span values,\n>> const ControlValue &def = {});\n>> - explicit ControlInfo(std::set values, bool def);\n>> - explicit ControlInfo(bool value);\n>> \n>> const ControlValue &min() const { return min_; }\n>> const ControlValue &max() const { return max_; }\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> index 70f6f6092..eaa34e70b 100644\n>> --- a/src/libcamera/controls.cpp\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n>> values_.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>> - * 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>> - : min_(false), max_(true), def_(def), values_({ false, true })\n>> -{\n>> - ASSERT(values.count(def) && values.size() == 2);\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>> - : min_(value), max_(value), def_(value)\n>> -{\n>> - values_ = { 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 e1bb43f0e..09c17ae63 100644\n>> --- a/test/controls/control_info.cpp\n>> +++ b/test/controls/control_info.cpp\n>> @@ -46,39 +46,6 @@ protected:\n>> return TestFail;\n>> }\n>> \n>> - /*\n>> - * Test information retrieval from a control with boolean\n>> - * values.\n>> - */\n>> - ControlInfo aeEnable({ false, true }, false);\n>> -\n>> - if (aeEnable.min().get() != false ||\n>> - aeEnable.def().get() != false ||\n>> - aeEnable.max().get() != true) {\n>> - cout << \"Invalid control range for AeEnable\" << endl;\n>> - return TestFail;\n>> - }\n>> -\n>> - if (aeEnable.values()[0].get() != false ||\n>> - aeEnable.values()[1].get() != true) {\n>> - cout << \"Invalid control values for AeEnable\" << endl;\n>> - return TestFail;\n>> - }\n>> -\n>> - ControlInfo awbEnable(true);\n>> -\n>> - if (awbEnable.min().get() != true ||\n>> - awbEnable.def().get() != true ||\n>> - awbEnable.max().get() != true) {\n>> - cout << \"Invalid control range for AwbEnable\" << endl;\n>> - return TestFail;\n>> - }\n>> -\n>> - if (awbEnable.values()[0].get() != true) {\n>> - cout << \"Invalid control values for AwbEnable\" << endl;\n>> - return TestFail;\n>> - }\n>> -\n>> return TestPass;\n>> }\n>> };\n>> -- \n>> 2.49.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 E67D3C3220\n\tfor ;\n\tThu, 15 May 2025 13:27:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8850368B70;\n\tThu, 15 May 2025 15:27:13 +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 030B968B4F\n\tfor ;\n\tThu, 15 May 2025 15:27:11 +0200 (CEST)","from [192.168.33.2] (185.221.142.248.nat.pool.zt.hu\n\t[185.221.142.248])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AA7F0886;\n\tThu, 15 May 2025 15:26:54 +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=\"kojlEf6k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747315614;\n\tbh=s4Ijjzjq9166jEDSpyFU10WbELBcHUJpO03leu8+S8w=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=kojlEf6kWQdUn/HCE7IBul9x0veBTHjVmNPk1Qx8M5m1QJ9rQIZEQc0DyRPwK9MDx\n\togIyGqI15aTTAFRemNMH3ciCHc7cBLSN1KjyNMlOci9seuapEJn8lquhIgDJ3275sj\n\tX5mANjw6HFrSIfPPbjNCUqp4Ho1a0CzzgNre7p3I=","Message-ID":"<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>","Date":"Thu, 15 May 2025 15:27:02 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250515123039.3134682-1-barnabas.pocze@ideasonboard.com>\n\t<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>\n\t<174731432844.329449.10966170717807072478@calcite>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174731432844.329449.10966170717807072478@calcite>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34254,"web_url":"https://patchwork.libcamera.org/comment/34254/","msgid":"<174739791984.530918.14871462540325529681@calcite>","date":"2025-05-16T12:18:39","subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-05-15 15:27:02)\n> Hi\n> \n> 2025. 05. 15. 15:05 keltezéssel, Paul Elder írta:\n> > Hi Barnabás,\n> > \n> > Thanks for the patch.\n> > \n> > Quoting Barnabás Pőcze (2025-05-15 14:30:39)\n> >> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n> >>\n> >> The contstructors introduced by that commit are not used anywhere,\n> >> and they do not match the existing practice for boolean controls.\n> >>\n> >> Specifically, every single boolean control is described by calling\n> >> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`\n> >> constructor. Crucially, that constructor does not set `values_`,\n> >> while the two removed contructors do. And whether or not `values_`\n> >> has any elements is currently used as an implicit sign to decide\n> >> whether or not the control is \"enum-like\", and those are assumed\n> >> to have type `int32_t`.\n> > \n> > I remember adding these constructors because if values_ is not populated then\n> > applications can't enumerate the possible values of the control. (I suppose it\n> > was an oversight that those constructors didn't get selected properly, though).\n> \n> One could argue that for boolean controls min()/max() can be sufficient...\n\nHm, good point.\n\n> \n> \n> > \n> > There might be debate whether or not boolean controls needs to be iterated, but\n> > I had imagined that some platforms might want to report a control to be\n> > supported, but only one boolean value is supported. Something like a uvc camera\n> > declaring that it doesn't have ae but it supports manual controls. (Even though\n> > we might be moving away from boolean enable flags to enum mode flags)\n> \n> ... min()/max() can be the same if only a single boolean value is actually allowed.\n\nRight.\n\n> \n> \n> > \n> > As for determining whether or not a control is enum-like, I remember adding\n> > ControlId::isArray(). Although that acts on ControlId as opposed to\n> > ControlInfo... Is that insufficient?\n> \n> I'm not sure if `ControlId::isArray()` does that. Maybe you meant\n> `ControlId::enumerators()`? I think that could work, but e.g.\n\nOops yeah that's what I meant.\n\n> `CameraSession::listControls()` does not use it at the moment.\n\nHm it indeed does not.\n\nimo it probably should, but now I see where you're going with this. I guess it\ndoesn't really make sense either to enumerate possible values of a boolen,and\nmin/max is indeed enough.\n\n> In any case, I'm just saying that the current situation is not ideal,\n> and something should be changed. And this seemed like the least intrusive\n> first step. (Although admittedly I do not have further steps in mind.)\n\nYes, that's true. I think maybe an upgrade to the documentation about checking\ntypes of controls and how to retrieve the possible values of a ControlInfo\nmight be good, plus fixing how our apps do these. But we can do this on top.\n\nReviewed-by: Paul Elder \n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> > \n> > \n> > Thanks,\n> > \n> > Paul\n> > \n> >>\n> >> For example, any boolean control described using any of the two\n> >> removed constructors would cause an assertion in failure in\n> >> `CameraSession::listControls()` when calling `value.get()`.\n> >>\n> >> Signed-off-by: Barnabás Pőcze \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 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >> index 2ae4ec3d4..32fb31f78 100644\n> >> --- a/include/libcamera/controls.h\n> >> +++ b/include/libcamera/controls.h\n> >> @@ -10,7 +10,6 @@\n> >> #include \n> >> #include \n> >> #include \n> >> -#include \n> >> #include \n> >> #include \n> >> #include \n> >> @@ -334,8 +333,6 @@ public:\n> >> const ControlValue &def = {});\n> >> explicit ControlInfo(Span values,\n> >> const ControlValue &def = {});\n> >> - explicit ControlInfo(std::set values, bool def);\n> >> - explicit ControlInfo(bool value);\n> >> \n> >> const ControlValue &min() const { return min_; }\n> >> const ControlValue &max() const { return max_; }\n> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >> index 70f6f6092..eaa34e70b 100644\n> >> --- a/src/libcamera/controls.cpp\n> >> +++ b/src/libcamera/controls.cpp\n> >> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n> >> values_.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> >> - * 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> >> - : min_(false), max_(true), def_(def), values_({ false, true })\n> >> -{\n> >> - ASSERT(values.count(def) && values.size() == 2);\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> >> - : min_(value), max_(value), def_(value)\n> >> -{\n> >> - values_ = { 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 e1bb43f0e..09c17ae63 100644\n> >> --- a/test/controls/control_info.cpp\n> >> +++ b/test/controls/control_info.cpp\n> >> @@ -46,39 +46,6 @@ protected:\n> >> return TestFail;\n> >> }\n> >> \n> >> - /*\n> >> - * Test information retrieval from a control with boolean\n> >> - * values.\n> >> - */\n> >> - ControlInfo aeEnable({ false, true }, false);\n> >> -\n> >> - if (aeEnable.min().get() != false ||\n> >> - aeEnable.def().get() != false ||\n> >> - aeEnable.max().get() != true) {\n> >> - cout << \"Invalid control range for AeEnable\" << endl;\n> >> - return TestFail;\n> >> - }\n> >> -\n> >> - if (aeEnable.values()[0].get() != false ||\n> >> - aeEnable.values()[1].get() != true) {\n> >> - cout << \"Invalid control values for AeEnable\" << endl;\n> >> - return TestFail;\n> >> - }\n> >> -\n> >> - ControlInfo awbEnable(true);\n> >> -\n> >> - if (awbEnable.min().get() != true ||\n> >> - awbEnable.def().get() != true ||\n> >> - awbEnable.max().get() != true) {\n> >> - cout << \"Invalid control range for AwbEnable\" << endl;\n> >> - return TestFail;\n> >> - }\n> >> -\n> >> - if (awbEnable.values()[0].get() != true) {\n> >> - cout << \"Invalid control values for AwbEnable\" << endl;\n> >> - return TestFail;\n> >> - }\n> >> -\n> >> return TestPass;\n> >> }\n> >> };\n> >> -- \n> >> 2.49.0\n> >>\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 81807BD78E\n\tfor ;\n\tFri, 16 May 2025 12:18:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F15CE68C92;\n\tFri, 16 May 2025 14:18:45 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B5B868B66\n\tfor ;\n\tFri, 16 May 2025 14:18:44 +0200 (CEST)","from pyrite.rasen.tech\n\t(2a01cb09d07af96689d02a40c946803c.ipv6.abo.wanadoo.fr\n\t[IPv6:2a01:cb09:d07a:f966:89d0:2a40:c946:803c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 391841D8;\n\tFri, 16 May 2025 14:18:26 +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=\"g4BmRZB7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747397906;\n\tbh=4rMZJQkjDuFMepwBxfNkGwwDlUWwFO8jCVlMRRGrLxA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=g4BmRZB7+M1BGLlN1BTg+cEzc5UXCk+1PIBMRyq6jdAOACPWqs3kaRIvKM5Y4WokX\n\tvCnKEUumtMnH8TTvQ07UTF+37rbsTFZgIN3263HkyKO25yhZWUY/+0e5bdaQh6vTJW\n\tCD1SR60aq3I6OO1hkqkuSDeCtBaocTzM+wFq+2sc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>","References":"<20250515123039.3134682-1-barnabas.pocze@ideasonboard.com>\n\t<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>\n\t<174731432844.329449.10966170717807072478@calcite>\n\t<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>","Subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 16 May 2025 14:18:39 +0200","Message-ID":"<174739791984.530918.14871462540325529681@calcite>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34255,"web_url":"https://patchwork.libcamera.org/comment/34255/","msgid":"<174739810007.530918.13639892985831093802@calcite>","date":"2025-05-16T12:21:40","subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Paul Elder (2025-05-16 14:18:39)\n> Quoting Barnabás Pőcze (2025-05-15 15:27:02)\n> > Hi\n> > \n> > 2025. 05. 15. 15:05 keltezéssel, Paul Elder írta:\n> > > Hi Barnabás,\n> > > \n> > > Thanks for the patch.\n> > > \n> > > Quoting Barnabás Pőcze (2025-05-15 14:30:39)\n> > >> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n> > >>\n> > >> The contstructors introduced by that commit are not used anywhere,\n> > >> and they do not match the existing practice for boolean controls.\n> > >>\n> > >> Specifically, every single boolean control is described by calling\n> > >> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`\n> > >> constructor. Crucially, that constructor does not set `values_`,\n> > >> while the two removed contructors do. And whether or not `values_`\n> > >> has any elements is currently used as an implicit sign to decide\n> > >> whether or not the control is \"enum-like\", and those are assumed\n> > >> to have type `int32_t`.\n> > > \n> > > I remember adding these constructors because if values_ is not populated then\n> > > applications can't enumerate the possible values of the control. (I suppose it\n> > > was an oversight that those constructors didn't get selected properly, though).\n> > \n> > One could argue that for boolean controls min()/max() can be sufficient...\n> \n> Hm, good point.\n> \n> > \n> > \n> > > \n> > > There might be debate whether or not boolean controls needs to be iterated, but\n> > > I had imagined that some platforms might want to report a control to be\n> > > supported, but only one boolean value is supported. Something like a uvc camera\n> > > declaring that it doesn't have ae but it supports manual controls. (Even though\n> > > we might be moving away from boolean enable flags to enum mode flags)\n> > \n> > ... min()/max() can be the same if only a single boolean value is actually allowed.\n> \n> Right.\n> \n> > \n> > \n> > > \n> > > As for determining whether or not a control is enum-like, I remember adding\n> > > ControlId::isArray(). Although that acts on ControlId as opposed to\n> > > ControlInfo... Is that insufficient?\n> > \n> > I'm not sure if `ControlId::isArray()` does that. Maybe you meant\n> > `ControlId::enumerators()`? I think that could work, but e.g.\n> \n> Oops yeah that's what I meant.\n> \n> > `CameraSession::listControls()` does not use it at the moment.\n> \n> Hm it indeed does not.\n> \n> imo it probably should, but now I see where you're going with this. I guess it\n> doesn't really make sense either to enumerate possible values of a boolen,and\n> min/max is indeed enough.\n> \n> > In any case, I'm just saying that the current situation is not ideal,\n> > and something should be changed. And this seemed like the least intrusive\n> > first step. (Although admittedly I do not have further steps in mind.)\n> \n> Yes, that's true. I think maybe an upgrade to the documentation about checking\n> types of controls and how to retrieve the possible values of a ControlInfo\n> might be good, plus fixing how our apps do these. But we can do this on top.\n> \n> Reviewed-by: Paul Elder \n> \n> > \n> > \n> > Regards,\n> > Barnabás Pőcze\n> > \n> > > \n> > > \n> > > Thanks,\n> > > \n> > > Paul\n> > > \n> > >>\n> > >> For example, any boolean control described using any of the two\n> > >> removed constructors would cause an assertion in failure in\n> > >> `CameraSession::listControls()` when calling `value.get()`.\n> > >>\n> > >> Signed-off-by: Barnabás Pőcze \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 deletions(-)\n> > >>\n> > >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > >> index 2ae4ec3d4..32fb31f78 100644\n> > >> --- a/include/libcamera/controls.h\n> > >> +++ b/include/libcamera/controls.h\n> > >> @@ -10,7 +10,6 @@\n> > >> #include \n> > >> #include \n> > >> #include \n> > >> -#include \n> > >> #include \n> > >> #include \n> > >> #include \n> > >> @@ -334,8 +333,6 @@ public:\n> > >> const ControlValue &def = {});\n> > >> explicit ControlInfo(Span values,\n> > >> const ControlValue &def = {});\n> > >> - explicit ControlInfo(std::set values, bool def);\n> > >> - explicit ControlInfo(bool value);\n> > >> \n> > >> const ControlValue &min() const { return min_; }\n> > >> const ControlValue &max() const { return max_; }\n> > >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > >> index 70f6f6092..eaa34e70b 100644\n> > >> --- a/src/libcamera/controls.cpp\n> > >> +++ b/src/libcamera/controls.cpp\n> > >> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n> > >> values_.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> > >> - * 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> > >> - : min_(false), max_(true), def_(def), values_({ false, true })\n> > >> -{\n> > >> - ASSERT(values.count(def) && values.size() == 2);\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> > >> - : min_(value), max_(value), def_(value)\n> > >> -{\n> > >> - values_ = { 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 e1bb43f0e..09c17ae63 100644\n> > >> --- a/test/controls/control_info.cpp\n> > >> +++ b/test/controls/control_info.cpp\n> > >> @@ -46,39 +46,6 @@ protected:\n> > >> return TestFail;\n> > >> }\n> > >> \n> > >> - /*\n> > >> - * Test information retrieval from a control with boolean\n> > >> - * values.\n> > >> - */\n> > >> - ControlInfo aeEnable({ false, true }, false);\n> > >> -\n> > >> - if (aeEnable.min().get() != false ||\n> > >> - aeEnable.def().get() != false ||\n> > >> - aeEnable.max().get() != true) {\n> > >> - cout << \"Invalid control range for AeEnable\" << endl;\n> > >> - return TestFail;\n> > >> - }\n\nWait, can't we keep this test?\n\n> > >> -\n> > >> - if (aeEnable.values()[0].get() != false ||\n> > >> - aeEnable.values()[1].get() != true) {\n> > >> - cout << \"Invalid control values for AeEnable\" << endl;\n> > >> - return TestFail;\n> > >> - }\n\nig we can drop this one.\n\n> > >> -\n> > >> - ControlInfo awbEnable(true);\n> > >> -\n> > >> - if (awbEnable.min().get() != true ||\n> > >> - awbEnable.def().get() != true ||\n> > >> - awbEnable.max().get() != true) {\n> > >> - cout << \"Invalid control range for AwbEnable\" << endl;\n> > >> - return TestFail;\n> > >> - }\n\nI also want to keep this one.\n\n> > >> -\n> > >> - if (awbEnable.values()[0].get() != true) {\n> > >> - cout << \"Invalid control values for AwbEnable\" << endl;\n> > >> - return TestFail;\n> > >> - }\n\nAnd this one can be dropped too ig.\n\n\nPaul\n\n> > >> -\n> > >> return TestPass;\n> > >> }\n> > >> };\n> > >> -- \n> > >> 2.49.0\n> > >>\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 3FBC1C3200\n\tfor ;\n\tFri, 16 May 2025 12:21:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1117D68C92;\n\tFri, 16 May 2025 14:21:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A444768B66\n\tfor ;\n\tFri, 16 May 2025 14:21:43 +0200 (CEST)","from pyrite.rasen.tech\n\t(2a01cb09d07af96689d02a40c946803c.ipv6.abo.wanadoo.fr\n\t[IPv6:2a01:cb09:d07a:f966:89d0:2a40:c946:803c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A34D51D8;\n\tFri, 16 May 2025 14:21:25 +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=\"Cf4+TKf0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747398085;\n\tbh=PutjDb7mXlkS59rzTJXLb0qnqvXcjMrHm25pKkIS6Ww=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Cf4+TKf0adGOLE8FRttZ4g99R+q+9k6QWhCjhgSojP1MI9pFE5cBHBsvpTdN6e3o3\n\tg7SlFJr1kqV+DJBZ1ej3TpZV/4sqVLL7pnuBAgJBYMlxsSK9TvW246xzVmtB1cs9wL\n\tIIc/XkS9QQd3sfTpWjvt/Wv57WnsN20A/Upt1Kgo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<174739791984.530918.14871462540325529681@calcite>","References":"<20250515123039.3134682-1-barnabas.pocze@ideasonboard.com>\n\t<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>\n\t<174731432844.329449.10966170717807072478@calcite>\n\t<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>\n\t<174739791984.530918.14871462540325529681@calcite>","Subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 16 May 2025 14:21:40 +0200","Message-ID":"<174739810007.530918.13639892985831093802@calcite>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34263,"web_url":"https://patchwork.libcamera.org/comment/34263/","msgid":"<33078417-ddc1-42a0-89a0-d16727820ccd@ideasonboard.com>","date":"2025-05-16T15:32:22","subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 05. 16. 14:21 keltezéssel, Paul Elder írta:\n> Quoting Paul Elder (2025-05-16 14:18:39)\n>> Quoting Barnabás Pőcze (2025-05-15 15:27:02)\n>>> Hi\n>>>\n>>> 2025. 05. 15. 15:05 keltezéssel, Paul Elder írta:\n>>>> Hi Barnabás,\n>>>>\n>>>> Thanks for the patch.\n>>>>\n>>>> Quoting Barnabás Pőcze (2025-05-15 14:30:39)\n>>>>> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n>>>>>\n>>>>> The contstructors introduced by that commit are not used anywhere,\n>>>>> and they do not match the existing practice for boolean controls.\n>>>>>\n>>>>> Specifically, every single boolean control is described by calling\n>>>>> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`\n>>>>> constructor. Crucially, that constructor does not set `values_`,\n>>>>> while the two removed contructors do. And whether or not `values_`\n>>>>> has any elements is currently used as an implicit sign to decide\n>>>>> whether or not the control is \"enum-like\", and those are assumed\n>>>>> to have type `int32_t`.\n>>>>\n>>>> I remember adding these constructors because if values_ is not populated then\n>>>> applications can't enumerate the possible values of the control. (I suppose it\n>>>> was an oversight that those constructors didn't get selected properly, though).\n>>>\n>>> One could argue that for boolean controls min()/max() can be sufficient...\n>>\n>> Hm, good point.\n>>\n>>>\n>>>\n>>>>\n>>>> There might be debate whether or not boolean controls needs to be iterated, but\n>>>> I had imagined that some platforms might want to report a control to be\n>>>> supported, but only one boolean value is supported. Something like a uvc camera\n>>>> declaring that it doesn't have ae but it supports manual controls. (Even though\n>>>> we might be moving away from boolean enable flags to enum mode flags)\n>>>\n>>> ... min()/max() can be the same if only a single boolean value is actually allowed.\n>>\n>> Right.\n>>\n>>>\n>>>\n>>>>\n>>>> As for determining whether or not a control is enum-like, I remember adding\n>>>> ControlId::isArray(). Although that acts on ControlId as opposed to\n>>>> ControlInfo... Is that insufficient?\n>>>\n>>> I'm not sure if `ControlId::isArray()` does that. Maybe you meant\n>>> `ControlId::enumerators()`? I think that could work, but e.g.\n>>\n>> Oops yeah that's what I meant.\n>>\n>>> `CameraSession::listControls()` does not use it at the moment.\n>>\n>> Hm it indeed does not.\n>>\n>> imo it probably should, but now I see where you're going with this. I guess it\n>> doesn't really make sense either to enumerate possible values of a boolen,and\n>> min/max is indeed enough.\n>>\n>>> In any case, I'm just saying that the current situation is not ideal,\n>>> and something should be changed. And this seemed like the least intrusive\n>>> first step. (Although admittedly I do not have further steps in mind.)\n>>\n>> Yes, that's true. I think maybe an upgrade to the documentation about checking\n>> types of controls and how to retrieve the possible values of a ControlInfo\n>> might be good, plus fixing how our apps do these. But we can do this on top.\n>>\n>> Reviewed-by: Paul Elder \n>>\n>>>\n>>>\n>>> Regards,\n>>> Barnabás Pőcze\n>>>\n>>>>\n>>>>\n>>>> Thanks,\n>>>>\n>>>> Paul\n>>>>\n>>>>>\n>>>>> For example, any boolean control described using any of the two\n>>>>> removed constructors would cause an assertion in failure in\n>>>>> `CameraSession::listControls()` when calling `value.get()`.\n>>>>>\n>>>>> Signed-off-by: Barnabás Pőcze \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 deletions(-)\n>>>>>\n>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>>>> index 2ae4ec3d4..32fb31f78 100644\n>>>>> --- a/include/libcamera/controls.h\n>>>>> +++ b/include/libcamera/controls.h\n>>>>> @@ -10,7 +10,6 @@\n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> -#include \n>>>>> #include \n>>>>> #include \n>>>>> #include \n>>>>> @@ -334,8 +333,6 @@ public:\n>>>>> const ControlValue &def = {});\n>>>>> explicit ControlInfo(Span values,\n>>>>> const ControlValue &def = {});\n>>>>> - explicit ControlInfo(std::set values, bool def);\n>>>>> - explicit ControlInfo(bool value);\n>>>>> \n>>>>> const ControlValue &min() const { return min_; }\n>>>>> const ControlValue &max() const { return max_; }\n>>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>>>> index 70f6f6092..eaa34e70b 100644\n>>>>> --- a/src/libcamera/controls.cpp\n>>>>> +++ b/src/libcamera/controls.cpp\n>>>>> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n>>>>> values_.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>>>>> - * 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>>>>> - : min_(false), max_(true), def_(def), values_({ false, true })\n>>>>> -{\n>>>>> - ASSERT(values.count(def) && values.size() == 2);\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>>>>> - : min_(value), max_(value), def_(value)\n>>>>> -{\n>>>>> - values_ = { 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 e1bb43f0e..09c17ae63 100644\n>>>>> --- a/test/controls/control_info.cpp\n>>>>> +++ b/test/controls/control_info.cpp\n>>>>> @@ -46,39 +46,6 @@ protected:\n>>>>> return TestFail;\n>>>>> }\n>>>>> \n>>>>> - /*\n>>>>> - * Test information retrieval from a control with boolean\n>>>>> - * values.\n>>>>> - */\n>>>>> - ControlInfo aeEnable({ false, true }, false);\n>>>>> -\n>>>>> - if (aeEnable.min().get() != false ||\n>>>>> - aeEnable.def().get() != false ||\n>>>>> - aeEnable.max().get() != true) {\n>>>>> - cout << \"Invalid control range for AeEnable\" << endl;\n>>>>> - return TestFail;\n>>>>> - }\n> \n> Wait, can't we keep this test?\n\nThe constructor needs to be modified as follows:\n\n ControlInfo aeEnable(false, true, false);\n\n\n> \n>>>>> -\n>>>>> - if (aeEnable.values()[0].get() != false ||\n>>>>> - aeEnable.values()[1].get() != true) {\n>>>>> - cout << \"Invalid control values for AeEnable\" << endl;\n>>>>> - return TestFail;\n>>>>> - }\n> \n> ig we can drop this one.\n> \n>>>>> -\n>>>>> - ControlInfo awbEnable(true);\n>>>>> -\n>>>>> - if (awbEnable.min().get() != true ||\n>>>>> - awbEnable.def().get() != true ||\n>>>>> - awbEnable.max().get() != true) {\n>>>>> - cout << \"Invalid control range for AwbEnable\" << endl;\n>>>>> - return TestFail;\n>>>>> - }\n> \n> I also want to keep this one.\n\nThis constructor also needs to be modified:\n\n ControlInfo aeEnable(true, true, true);\n\n\nSo should I make these changes?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>>>>> -\n>>>>> - if (awbEnable.values()[0].get() != true) {\n>>>>> - cout << \"Invalid control values for AwbEnable\" << endl;\n>>>>> - return TestFail;\n>>>>> - }\n> \n> And this one can be dropped too ig.\n> \n> \n> Paul\n> \n>>>>> -\n>>>>> return TestPass;\n>>>>> }\n>>>>> };\n>>>>> -- \n>>>>> 2.49.0\n>>>>>\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 0424CBD78E\n\tfor ;\n\tFri, 16 May 2025 15:32:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3F00368B70;\n\tFri, 16 May 2025 17:32:27 +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 ABED168B66\n\tfor ;\n\tFri, 16 May 2025 17:32:25 +0200 (CEST)","from [192.168.33.17] (185.221.142.248.nat.pool.zt.hu\n\t[185.221.142.248])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 88A464AD;\n\tFri, 16 May 2025 17:32:07 +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=\"cNgn50S6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747409527;\n\tbh=6frsuWYAwwZzbE18PNlPQd8CK1nm2FE4/qI7Xfl8xcI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=cNgn50S6DvWGxcD/GmHw9QDiT/Ch5Mk9QrvNgCNVCDCV4mndquIzmfqkV2eQEi6n7\n\t5WbAJyNXHpfaDeZHhvWI9DsqBJeL3NlwJmFoWRwJGxBeE41qNtOPJCdtZmyJPl186n\n\tQ2Vl+kGhslq+fqQ9WlzzPMd4ZgtunQYAPIFz3hI8=","Message-ID":"<33078417-ddc1-42a0-89a0-d16727820ccd@ideasonboard.com>","Date":"Fri, 16 May 2025 17:32:22 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250515123039.3134682-1-barnabas.pocze@ideasonboard.com>\n\t<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>\n\t<174731432844.329449.10966170717807072478@calcite>\n\t<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>\n\t<174739791984.530918.14871462540325529681@calcite>\n\t<174739810007.530918.13639892985831093802@calcite>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174739810007.530918.13639892985831093802@calcite>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34265,"web_url":"https://patchwork.libcamera.org/comment/34265/","msgid":"<174743194880.1455723.8393354387031579457@calcite>","date":"2025-05-16T21:45:48","subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-05-16 17:32:22)\n> Hi\n> \n> \n> 2025. 05. 16. 14:21 keltezéssel, Paul Elder írta:\n> > Quoting Paul Elder (2025-05-16 14:18:39)\n> >> Quoting Barnabás Pőcze (2025-05-15 15:27:02)\n> >>> Hi\n> >>>\n> >>> 2025. 05. 15. 15:05 keltezéssel, Paul Elder írta:\n> >>>> Hi Barnabás,\n> >>>>\n> >>>> Thanks for the patch.\n> >>>>\n> >>>> Quoting Barnabás Pőcze (2025-05-15 14:30:39)\n> >>>>> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.\n> >>>>>\n> >>>>> The contstructors introduced by that commit are not used anywhere,\n> >>>>> and they do not match the existing practice for boolean controls.\n> >>>>>\n> >>>>> Specifically, every single boolean control is described by calling\n> >>>>> the `ControlInfo(ControlValue, ControlValue, ControlValue = {})`\n> >>>>> constructor. Crucially, that constructor does not set `values_`,\n> >>>>> while the two removed contructors do. And whether or not `values_`\n> >>>>> has any elements is currently used as an implicit sign to decide\n> >>>>> whether or not the control is \"enum-like\", and those are assumed\n> >>>>> to have type `int32_t`.\n> >>>>\n> >>>> I remember adding these constructors because if values_ is not populated then\n> >>>> applications can't enumerate the possible values of the control. (I suppose it\n> >>>> was an oversight that those constructors didn't get selected properly, though).\n> >>>\n> >>> One could argue that for boolean controls min()/max() can be sufficient...\n> >>\n> >> Hm, good point.\n> >>\n> >>>\n> >>>\n> >>>>\n> >>>> There might be debate whether or not boolean controls needs to be iterated, but\n> >>>> I had imagined that some platforms might want to report a control to be\n> >>>> supported, but only one boolean value is supported. Something like a uvc camera\n> >>>> declaring that it doesn't have ae but it supports manual controls. (Even though\n> >>>> we might be moving away from boolean enable flags to enum mode flags)\n> >>>\n> >>> ... min()/max() can be the same if only a single boolean value is actually allowed.\n> >>\n> >> Right.\n> >>\n> >>>\n> >>>\n> >>>>\n> >>>> As for determining whether or not a control is enum-like, I remember adding\n> >>>> ControlId::isArray(). Although that acts on ControlId as opposed to\n> >>>> ControlInfo... Is that insufficient?\n> >>>\n> >>> I'm not sure if `ControlId::isArray()` does that. Maybe you meant\n> >>> `ControlId::enumerators()`? I think that could work, but e.g.\n> >>\n> >> Oops yeah that's what I meant.\n> >>\n> >>> `CameraSession::listControls()` does not use it at the moment.\n> >>\n> >> Hm it indeed does not.\n> >>\n> >> imo it probably should, but now I see where you're going with this. I guess it\n> >> doesn't really make sense either to enumerate possible values of a boolen,and\n> >> min/max is indeed enough.\n> >>\n> >>> In any case, I'm just saying that the current situation is not ideal,\n> >>> and something should be changed. And this seemed like the least intrusive\n> >>> first step. (Although admittedly I do not have further steps in mind.)\n> >>\n> >> Yes, that's true. I think maybe an upgrade to the documentation about checking\n> >> types of controls and how to retrieve the possible values of a ControlInfo\n> >> might be good, plus fixing how our apps do these. But we can do this on top.\n> >>\n> >> Reviewed-by: Paul Elder \n> >>\n> >>>\n> >>>\n> >>> Regards,\n> >>> Barnabás Pőcze\n> >>>\n> >>>>\n> >>>>\n> >>>> Thanks,\n> >>>>\n> >>>> Paul\n> >>>>\n> >>>>>\n> >>>>> For example, any boolean control described using any of the two\n> >>>>> removed constructors would cause an assertion in failure in\n> >>>>> `CameraSession::listControls()` when calling `value.get()`.\n> >>>>>\n> >>>>> Signed-off-by: Barnabás Pőcze \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 deletions(-)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >>>>> index 2ae4ec3d4..32fb31f78 100644\n> >>>>> --- a/include/libcamera/controls.h\n> >>>>> +++ b/include/libcamera/controls.h\n> >>>>> @@ -10,7 +10,6 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> #include \n> >>>>> -#include \n> >>>>> #include \n> >>>>> #include \n> >>>>> #include \n> >>>>> @@ -334,8 +333,6 @@ public:\n> >>>>> const ControlValue &def = {});\n> >>>>> explicit ControlInfo(Span values,\n> >>>>> const ControlValue &def = {});\n> >>>>> - explicit ControlInfo(std::set values, bool def);\n> >>>>> - explicit ControlInfo(bool value);\n> >>>>> \n> >>>>> const ControlValue &min() const { return min_; }\n> >>>>> const ControlValue &max() const { return max_; }\n> >>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >>>>> index 70f6f6092..eaa34e70b 100644\n> >>>>> --- a/src/libcamera/controls.cpp\n> >>>>> +++ b/src/libcamera/controls.cpp\n> >>>>> @@ -625,35 +625,6 @@ ControlInfo::ControlInfo(Span values,\n> >>>>> values_.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> >>>>> - * 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> >>>>> - : min_(false), max_(true), def_(def), values_({ false, true })\n> >>>>> -{\n> >>>>> - ASSERT(values.count(def) && values.size() == 2);\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> >>>>> - : min_(value), max_(value), def_(value)\n> >>>>> -{\n> >>>>> - values_ = { 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 e1bb43f0e..09c17ae63 100644\n> >>>>> --- a/test/controls/control_info.cpp\n> >>>>> +++ b/test/controls/control_info.cpp\n> >>>>> @@ -46,39 +46,6 @@ protected:\n> >>>>> return TestFail;\n> >>>>> }\n> >>>>> \n> >>>>> - /*\n> >>>>> - * Test information retrieval from a control with boolean\n> >>>>> - * values.\n> >>>>> - */\n> >>>>> - ControlInfo aeEnable({ false, true }, false);\n> >>>>> -\n> >>>>> - if (aeEnable.min().get() != false ||\n> >>>>> - aeEnable.def().get() != false ||\n> >>>>> - aeEnable.max().get() != true) {\n> >>>>> - cout << \"Invalid control range for AeEnable\" << endl;\n> >>>>> - return TestFail;\n> >>>>> - }\n> > \n> > Wait, can't we keep this test?\n> \n> The constructor needs to be modified as follows:\n> \n> ControlInfo aeEnable(false, true, false);\n> \n> \n> > \n> >>>>> -\n> >>>>> - if (aeEnable.values()[0].get() != false ||\n> >>>>> - aeEnable.values()[1].get() != true) {\n> >>>>> - cout << \"Invalid control values for AeEnable\" << endl;\n> >>>>> - return TestFail;\n> >>>>> - }\n> > \n> > ig we can drop this one.\n> > \n> >>>>> -\n> >>>>> - ControlInfo awbEnable(true);\n> >>>>> -\n> >>>>> - if (awbEnable.min().get() != true ||\n> >>>>> - awbEnable.def().get() != true ||\n> >>>>> - awbEnable.max().get() != true) {\n> >>>>> - cout << \"Invalid control range for AwbEnable\" << endl;\n> >>>>> - return TestFail;\n> >>>>> - }\n> > \n> > I also want to keep this one.\n> \n> This constructor also needs to be modified:\n> \n> ControlInfo aeEnable(true, true, true);\n\nAh indeed, you're right.\n\n> \n> \n> So should I make these changes?\n\nYes, please.\n\n\nThanks,\n\nPaul\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> > \n> >>>>> -\n> >>>>> - if (awbEnable.values()[0].get() != true) {\n> >>>>> - cout << \"Invalid control values for AwbEnable\" << endl;\n> >>>>> - return TestFail;\n> >>>>> - }\n> > \n> > And this one can be dropped too ig.\n> > \n> > \n> > Paul\n> > \n> >>>>> -\n> >>>>> return TestPass;\n> >>>>> }\n> >>>>> };\n> >>>>> -- \n> >>>>> 2.49.0\n> >>>>>\n> >>>\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 DBA8DC3200\n\tfor ;\n\tFri, 16 May 2025 21:45:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 042B768B70;\n\tFri, 16 May 2025 23:45: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 ADDD668B66\n\tfor ;\n\tFri, 16 May 2025 23:45:54 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2a01:cb1e:7a:51ab:ed9d:6fa3:bc49:4dd6])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5686A5B3;\n\tFri, 16 May 2025 23:45:36 +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=\"atN9fwFs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1747431936;\n\tbh=rxcEG6BYam64f4qBa/FfkGQGiRKxX5Shwg0e/dtwJzA=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=atN9fwFsHpy8DX1HNoj6UnTo3VswpQZ/3cAvV/dJV44Z+PsvDlCQUsompa72ysLIA\n\ttcI0VGRXdu9G0vO7pufkkx2iPCQaVF5sn6ySVLJcJqN0Vu29QOgHyUN14OPKSOUl6+\n\t3qo0EIttLFcJzpVRug9BnZv77iRb2HD/fufEBQkw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<33078417-ddc1-42a0-89a0-d16727820ccd@ideasonboard.com>","References":"<20250515123039.3134682-1-barnabas.pocze@ideasonboard.com>\n\t<20250515123039.3134682-2-barnabas.pocze@ideasonboard.com>\n\t<174731432844.329449.10966170717807072478@calcite>\n\t<3ca1f2c6-3af0-454a-82b2-effde99a029e@ideasonboard.com>\n\t<174739791984.530918.14871462540325529681@calcite>\n\t<174739810007.530918.13639892985831093802@calcite>\n\t<33078417-ddc1-42a0-89a0-d16727820ccd@ideasonboard.com>","Subject":"Re: [PATCH v1 2/2] Revert \"controls: Add boolean constructors for\n\tControlInfo\"","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 16 May 2025 23:45:48 +0200","Message-ID":"<174743194880.1455723.8393354387031579457@calcite>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]