[{"id":31158,"web_url":"https://patchwork.libcamera.org/comment/31158/","msgid":"<6ca6d0f6-e848-411e-92f0-8ae5853158a6@ideasonboard.com>","date":"2024-09-11T04:34:24","subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul\n\nOn 10/09/24 7:01 pm, Paul Elder wrote:\n> Add a test for enum controls to test that the ranges and values are set\n> properly.\n>\n> Signed-off-by: Paul Elder \n> ---\n> test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 39 insertions(+)\n>\n> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> index e1bb43f0e..460aaa345 100644\n> --- a/test/controls/control_info.cpp\n> +++ b/test/controls/control_info.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -79,6 +80,44 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\t/*\n> +\t\t * Test information retrieval from an enum control.\n> +\t\t */\n> +\t\tControlInfo awbMode(static_cast(controls::AwbTungsten),\n> +\t\t\t\t static_cast(controls::AwbDaylight));\n> +\t\tif (awbMode.min().get() != controls::AwbTungsten ||\n> +\t\t awbMode.max().get() != controls::AwbDaylight) {\n> +\t\t\tcout << \"Invalid control range for AwbMode\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tstd::vector modes = {\n> +\t\t\tstatic_cast(controls::AwbTungsten),\n> +\t\t\tstatic_cast(controls::AwbFluorescent),\n> +\t\t\tstatic_cast(controls::AwbDaylight),\n> +\t\t};\n> +\t\tControlInfo awbModes(Span{ modes });\n> +\n> +\t\tif (awbModes.min() != modes.front() ||\n> +\t\t awbModes.def() != modes.front() ||\n> +\t\t awbModes.max() != modes.back()) {\n> +\t\t\tcout << \"Invalid control range for AwbModes\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tif (awbModes.values().size() != modes.size()) {\n\nAre we really comparing the awbModes with the modes vector ?\n\nShouldn't we be comparing awbModes with awbMode ?\n> +\t\t\tcout << \"Invalid size for AwbModes\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const auto &value : awbModes.values()) {\n> +\t\t\tif (value != modes.at(i++)) {\n\nSimilar comment here:\n\nShouldn't we be comparing enum values :\n\nControlInfo awbModes\n\nvs\n\nControlInfo awbMode\n\nMy idea of the test would be to equate the values of the range vs values \nfrom modes vector. Does it make sense?\n\n> +\t\t\t\tcout << \"Invalid control values for AwbModes\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\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 9F820C324C\n\tfor ;\n\tWed, 11 Sep 2024 04:34:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5943D634FC;\n\tWed, 11 Sep 2024 06:34:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3851A618F1\n\tfor ;\n\tWed, 11 Sep 2024 06:34:29 +0200 (CEST)","from [IPV6:2405:201:2015:f873:c173:4b:4a04:3a21] (unknown\n\t[IPv6:2405:201:2015:f873:c173:4b:4a04:3a21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53D025B3;\n\tWed, 11 Sep 2024 06:33:11 +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=\"F9k4tZWG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726029191;\n\tbh=C6FyyRBC66D0/fHJ7I2aCoRi3eLLWh7eDXKWsy3WisI=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=F9k4tZWG5yg6NIxsJfSe4W6SGf+cTR3sTVNM/vrcqnQqj0JRt1c0y0qnI9IYkVGfN\n\tiTiZxO2bsXsvbqa+JbDkv8Cdqk4S5oBRFPoDmn2bJRKdexF6Enxl/ZKJCqP8ZymVt+\n\tiwpuS5EfveTl+NcRLJqvuxaKTE4ZAZXm+n3hUgFA=","Message-ID":"<6ca6d0f6-e848-411e-92f0-8ae5853158a6@ideasonboard.com>","Date":"Wed, 11 Sep 2024 10:04:24 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240910133130.1374459-1-paul.elder@ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240910133130.1374459-1-paul.elder@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":31164,"web_url":"https://patchwork.libcamera.org/comment/31164/","msgid":"","date":"2024-09-11T09:06:16","subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Sep 11, 2024 at 10:04:24AM +0530, Umang Jain wrote:\n> Hi Paul\n> \n> On 10/09/24 7:01 pm, Paul Elder wrote:\n> > Add a test for enum controls to test that the ranges and values are set\n> > properly.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++\n> > 1 file changed, 39 insertions(+)\n> > \n> > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > index e1bb43f0e..460aaa345 100644\n> > --- a/test/controls/control_info.cpp\n> > +++ b/test/controls/control_info.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> > #include \n> > +#include \n> > #include \n> > #include \n> > @@ -79,6 +80,44 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > +\t\t/*\n> > +\t\t * Test information retrieval from an enum control.\n> > +\t\t */\n> > +\t\tControlInfo awbMode(static_cast(controls::AwbTungsten),\n> > +\t\t\t\t static_cast(controls::AwbDaylight));\n> > +\t\tif (awbMode.min().get() != controls::AwbTungsten ||\n> > +\t\t awbMode.max().get() != controls::AwbDaylight) {\n> > +\t\t\tcout << \"Invalid control range for AwbMode\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tstd::vector modes = {\n> > +\t\t\tstatic_cast(controls::AwbTungsten),\n> > +\t\t\tstatic_cast(controls::AwbFluorescent),\n> > +\t\t\tstatic_cast(controls::AwbDaylight),\n> > +\t\t};\n> > +\t\tControlInfo awbModes(Span{ modes });\n> > +\n> > +\t\tif (awbModes.min() != modes.front() ||\n> > +\t\t awbModes.def() != modes.front() ||\n> > +\t\t awbModes.max() != modes.back()) {\n> > +\t\t\tcout << \"Invalid control range for AwbModes\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tif (awbModes.values().size() != modes.size()) {\n> \n> Are we really comparing the awbModes with the modes vector ?\n> \n> Shouldn't we be comparing awbModes with awbMode ?\n\nOk I think this is caused by a mismatch of what you imagine should be\ntested and what I wanted to test.\n\nI wanted to test/confirm that if you create an enum ControlInfo from a\nlist of values that it would store those values properly (which was\nconfusing because ControlInfo::toString() only shows \"[0..2]\" so it\nlooks like 0, 1, and 2 are supported even if you created the ControlInfo\nwith only 0 and 2).\n\nAnd in fact, we shouldn't actually construct enum ControlInfo like\nawbMode is, as we've decided (so far) that the discriminator between an\nenum control and a non-enum control is if ControlInfo::values() is\nnon-null. So by that definition awbMode wouldn't actually be treated as\nan enum ControlInfo. But enum ControlInfos shouldn't be created like\nthat anyway so maybe I should get rid of that actually?\n\n> > +\t\t\tcout << \"Invalid size for AwbModes\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\tunsigned int i = 0;\n> > +\t\tfor (const auto &value : awbModes.values()) {\n> > +\t\t\tif (value != modes.at(i++)) {\n> \n> Similar comment here:\n> \n> Shouldn't we be comparing enum values :\n> \n> ControlInfo awbModes\n> \n> vs\n> \n> ControlInfo awbMode\n> \n> My idea of the test would be to equate the values of the range vs values\n> from modes vector. Does it make sense?\n\nI suppose we could extend ControlInfo to populate values_ when an enum\nControlInfo is constructed from a range? Now that enum information is\nstored in ControlId we could do that.\n\nBut imo it doesn't make sense to construct an enum ControlInfo from a\nrange; you should be declaring which specific values you support from\nthe enum. That's the point of enum controls right?\n\nMaybe I should just delete the awbMode test.\n\n\nThanks,\n\nPaul\n\n> > +\t\t\t\tcout << \"Invalid control values for AwbModes\" << endl;\n> > +\t\t\t\treturn TestFail;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > \t\treturn TestPass;\n> > \t}\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 05B9BBF415\n\tfor ;\n\tWed, 11 Sep 2024 09:06:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 82196634FC;\n\tWed, 11 Sep 2024 11:06:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9D268634E3\n\tfor ;\n\tWed, 11 Sep 2024 11:06:21 +0200 (CEST)","from pyrite.rasen.tech (213-229-8-243.static.upcbusiness.at\n\t[213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D133411D6;\n\tWed, 11 Sep 2024 11:05:03 +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=\"rSUeL4Zs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726045503;\n\tbh=QfAyOoLlrXacKlqfzpmrIr62moFFimf7U7cslSh7Bcg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rSUeL4ZsJmB/3EwWgGxGZa1bHxKaJJkdx31fGufvyPZinBiCEX08qiH+a25V8LuF+\n\t2t3uzdxbDyYE3gk6nqzoPvWWO4C0WqtamE//06puunxghn4QjIG2AmcNLjpdq7YkCY\n\tULjJG7x4IC1PETEGyClYQer7NZGanxyBy3wd5yxY=","Date":"Wed, 11 Sep 2024 11:06:16 +0200","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","Message-ID":"","References":"<20240910133130.1374459-1-paul.elder@ideasonboard.com>\n\t<6ca6d0f6-e848-411e-92f0-8ae5853158a6@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<6ca6d0f6-e848-411e-92f0-8ae5853158a6@ideasonboard.com>","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":31174,"web_url":"https://patchwork.libcamera.org/comment/31174/","msgid":"<172607114399.1037309.15159809398463131504@ping.linuxembedded.co.uk>","date":"2024-09-11T16:12:23","subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-09-11 10:06:16)\n> Hi Umang,\n> \n> On Wed, Sep 11, 2024 at 10:04:24AM +0530, Umang Jain wrote:\n> > Hi Paul\n> > \n> > On 10/09/24 7:01 pm, Paul Elder wrote:\n> > > Add a test for enum controls to test that the ranges and values are set\n> > > properly.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++\n> > > 1 file changed, 39 insertions(+)\n> > > \n> > > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > > index e1bb43f0e..460aaa345 100644\n> > > --- a/test/controls/control_info.cpp\n> > > +++ b/test/controls/control_info.cpp\n> > > @@ -6,6 +6,7 @@\n> > > */\n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > @@ -79,6 +80,44 @@ protected:\n> > > return TestFail;\n> > > }\n> > > + /*\n> > > + * Test information retrieval from an enum control.\n> > > + */\n> > > + ControlInfo awbMode(static_cast(controls::AwbTungsten),\n> > > + static_cast(controls::AwbDaylight));\n> > > + if (awbMode.min().get() != controls::AwbTungsten ||\n> > > + awbMode.max().get() != controls::AwbDaylight) {\n> > > + cout << \"Invalid control range for AwbMode\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + std::vector modes = {\n> > > + static_cast(controls::AwbTungsten),\n> > > + static_cast(controls::AwbFluorescent),\n> > > + static_cast(controls::AwbDaylight),\n> > > + };\n> > > + ControlInfo awbModes(Span{ modes });\n> > > +\n> > > + if (awbModes.min() != modes.front() ||\n> > > + awbModes.def() != modes.front() ||\n> > > + awbModes.max() != modes.back()) {\n> > > + cout << \"Invalid control range for AwbModes\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + if (awbModes.values().size() != modes.size()) {\n> > \n> > Are we really comparing the awbModes with the modes vector ?\n> > \n> > Shouldn't we be comparing awbModes with awbMode ?\n> \n> Ok I think this is caused by a mismatch of what you imagine should be\n> tested and what I wanted to test.\n> \n> I wanted to test/confirm that if you create an enum ControlInfo from a\n> list of values that it would store those values properly (which was\n> confusing because ControlInfo::toString() only shows \"[0..2]\" so it\n> looks like 0, 1, and 2 are supported even if you created the ControlInfo\n> with only 0 and 2).\n> \n> And in fact, we shouldn't actually construct enum ControlInfo like\n> awbMode is, as we've decided (so far) that the discriminator between an\n> enum control and a non-enum control is if ControlInfo::values() is\n> non-null. So by that definition awbMode wouldn't actually be treated as\n> an enum ControlInfo. But enum ControlInfos shouldn't be created like\n> that anyway so maybe I should get rid of that actually?\n> \n> > > + cout << \"Invalid size for AwbModes\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + unsigned int i = 0;\n> > > + for (const auto &value : awbModes.values()) {\n> > > + if (value != modes.at(i++)) {\n> > \n> > Similar comment here:\n> > \n> > Shouldn't we be comparing enum values :\n> > \n> > ControlInfo awbModes\n> > \n> > vs\n> > \n> > ControlInfo awbMode\n> > \n> > My idea of the test would be to equate the values of the range vs values\n> > from modes vector. Does it make sense?\n> \n> I suppose we could extend ControlInfo to populate values_ when an enum\n> ControlInfo is constructed from a range? Now that enum information is\n> stored in ControlId we could do that.\n> \n> But imo it doesn't make sense to construct an enum ControlInfo from a\n> range; you should be declaring which specific values you support from\n> the enum. That's the point of enum controls right?\n\nWould we expect any shortcuts for adding a control that supports 'all'\navailable enum 's ? or would they have to be explicitly mentioned when\nbeing registered?\n\n--\nKieran\n\n> \n> Maybe I should just delete the awbMode test.\n> \n> \n> Thanks,\n> \n> Paul\n> \n> > > + cout << \"Invalid control values for AwbModes\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > + }\n> > > +\n> > > return TestPass;\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 F3501C324C\n\tfor ;\n\tWed, 11 Sep 2024 16:12:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45C9B634FC;\n\tWed, 11 Sep 2024 18:12:28 +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 57905618FB\n\tfor ;\n\tWed, 11 Sep 2024 18:12:27 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5845BBEB;\n\tWed, 11 Sep 2024 18:11:09 +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=\"H0vRi6nx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726071069;\n\tbh=2Z4xsnBelR4LAQSNgFm9D4y70NsrU9kmuojep5gBT3s=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=H0vRi6nxdkBBZFGHW7SCNAbQ/mtq59tGf/gyx4Do8AXA8buSG1xvk7qIVkImIjPyv\n\tZKwtXvjiugK3x86CT/xeZ7wxlNIEynNw4dlSA7j71MwPQlHL6KGTcoCIicM+hzX1W8\n\tt1yxgkiFb9+0RcwMhj62YeGi0vtAOwD/b/VS8UVo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240910133130.1374459-1-paul.elder@ideasonboard.com>\n\t<6ca6d0f6-e848-411e-92f0-8ae5853158a6@ideasonboard.com>\n\t","Subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Paul Elder ,\n\tUmang Jain ","Date":"Wed, 11 Sep 2024 17:12:23 +0100","Message-ID":"<172607114399.1037309.15159809398463131504@ping.linuxembedded.co.uk>","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":31182,"web_url":"https://patchwork.libcamera.org/comment/31182/","msgid":"","date":"2024-09-12T03:31:13","subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Paul,\n\nOn 11/09/24 2:36 pm, Paul Elder wrote:\n> Hi Umang,\n>\n> On Wed, Sep 11, 2024 at 10:04:24AM +0530, Umang Jain wrote:\n>> Hi Paul\n>>\n>> On 10/09/24 7:01 pm, Paul Elder wrote:\n>>> Add a test for enum controls to test that the ranges and values are set\n>>> properly.\n>>>\n>>> Signed-off-by: Paul Elder \n>>> ---\n>>> test/controls/control_info.cpp | 39 ++++++++++++++++++++++++++++++++++\n>>> 1 file changed, 39 insertions(+)\n>>>\n>>> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n>>> index e1bb43f0e..460aaa345 100644\n>>> --- a/test/controls/control_info.cpp\n>>> +++ b/test/controls/control_info.cpp\n>>> @@ -6,6 +6,7 @@\n>>> */\n>>> #include \n>>> +#include \n>>> #include \n>>> #include \n>>> @@ -79,6 +80,44 @@ protected:\n>>> \t\t\treturn TestFail;\n>>> \t\t}\n>>> +\t\t/*\n>>> +\t\t * Test information retrieval from an enum control.\n>>> +\t\t */\n>>> +\t\tControlInfo awbMode(static_cast(controls::AwbTungsten),\n>>> +\t\t\t\t static_cast(controls::AwbDaylight));\n>>> +\t\tif (awbMode.min().get() != controls::AwbTungsten ||\n>>> +\t\t awbMode.max().get() != controls::AwbDaylight) {\n>>> +\t\t\tcout << \"Invalid control range for AwbMode\" << endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\tstd::vector modes = {\n>>> +\t\t\tstatic_cast(controls::AwbTungsten),\n>>> +\t\t\tstatic_cast(controls::AwbFluorescent),\n>>> +\t\t\tstatic_cast(controls::AwbDaylight),\n>>> +\t\t};\n>>> +\t\tControlInfo awbModes(Span{ modes });\n>>> +\n>>> +\t\tif (awbModes.min() != modes.front() ||\n>>> +\t\t awbModes.def() != modes.front() ||\n>>> +\t\t awbModes.max() != modes.back()) {\n>>> +\t\t\tcout << \"Invalid control range for AwbModes\" << endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\tif (awbModes.values().size() != modes.size()) {\n>> Are we really comparing the awbModes with the modes vector ?\n>>\n>> Shouldn't we be comparing awbModes with awbMode ?\n> Ok I think this is caused by a mismatch of what you imagine should be\n> tested and what I wanted to test.\n>\n> I wanted to test/confirm that if you create an enum ControlInfo from a\n> list of values that it would store those values properly (which was\n> confusing because ControlInfo::toString() only shows \"[0..2]\" so it\n> looks like 0, 1, and 2 are supported even if you created the ControlInfo\n> with only 0 and 2).\n\nThat seems fine to test.\n>\n> And in fact, we shouldn't actually construct enum ControlInfo like\n> awbMode is, as we've decided (so far) that the discriminator between an\n> enum control and a non-enum control is if ControlInfo::values() is\n> non-null. So by that definition awbMode wouldn't actually be treated as\n> an enum ControlInfo. But enum ControlInfos shouldn't be created like\n> that anyway so maybe I should get rid of that actually?\n\nYes, if its not expected to be a range, then we should get rid of 'awbMode'\n\nFurthermore possibility, we can decide and go one step further, to \nprevent enum ControlInfos being defined as ranges, in controls handling \nlogic.\n\n\n>\n>>> +\t\t\tcout << \"Invalid size for AwbModes\" << endl;\n>>> +\t\t\treturn TestFail;\n>>> +\t\t}\n>>> +\n>>> +\t\tunsigned int i = 0;\n>>> +\t\tfor (const auto &value : awbModes.values()) {\n>>> +\t\t\tif (value != modes.at(i++)) {\n>> Similar comment here:\n>>\n>> Shouldn't we be comparing enum values :\n>>\n>> ControlInfo awbModes\n>>\n>> vs\n>>\n>> ControlInfo awbMode\n>>\n>> My idea of the test would be to equate the values of the range vs values\n>> from modes vector. Does it make sense?\n> I suppose we could extend ControlInfo to populate values_ when an enum\n> ControlInfo is constructed from a range? Now that enum information is\n> stored in ControlId we could do that.\n>\n> But imo it doesn't make sense to construct an enum ControlInfo from a\n> range; you should be declaring which specific values you support from\n> the enum. That's the point of enum controls right?\n>\n> Maybe I should just delete the awbMode test.\n>\n>\n> Thanks,\n>\n> Paul\n>\n>>> +\t\t\t\tcout << \"Invalid control values for AwbModes\" << endl;\n>>> +\t\t\t\treturn TestFail;\n>>> +\t\t\t}\n>>> +\t\t}\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 A25AABF415\n\tfor ;\n\tThu, 12 Sep 2024 03:31:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 36BC9634F9;\n\tThu, 12 Sep 2024 05:31:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 26A86618F0\n\tfor ;\n\tThu, 12 Sep 2024 05:31:17 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A7DD76AF;\n\tThu, 12 Sep 2024 05:29: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=\"PPErlRLs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726111799;\n\tbh=7KVqWzR6guAwoYyXPCv0iNnNV6kzH4HaT08r2us7VAo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=PPErlRLsqy36ihM3vH/oM+dxGUt5EvHvDDMEbd6axAEDX8o5QKREV7WuaFif+oz/E\n\tfEuLelmiMT4sxj2Jg7En3mwSKU6tLKMbpaLs3w+IEr5qbtj+j9pBL/xjXG6wzCRNoG\n\tbySlgwcU5DuLy83woC+nLoxWnyngB1PSEoLtX2R8=","Message-ID":"","Date":"Thu, 12 Sep 2024 09:01:13 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] test: controls: control_info: Add test for enum controls","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240910133130.1374459-1-paul.elder@ideasonboard.com>\n\t<6ca6d0f6-e848-411e-92f0-8ae5853158a6@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]