[{"id":31381,"web_url":"https://patchwork.libcamera.org/comment/31381/","msgid":"","date":"2024-09-26T09:13:44","subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:\n> When constructing a ControlValue from an enum value, an explicit cast to\n> int32_t is needed as we use int32_t as the underlying type for all\n> enumerated controls. This makes users of ControlValue more complex. To\n> simplify them, specialize the control_type template for enum types, to\n> support construction of ControlValue directly without a cast.\n>\n> Signed-off-by: Laurent Pinchart \n\nbrilliant, less things to type in!\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> include/libcamera/controls.h | 6 +++++-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> 2 files changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 25f67ed948a3..c5131870d402 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -39,7 +39,7 @@ enum ControlType {\n>\n> namespace details {\n>\n> -template\n> +template>\n> struct control_type {\n> };\n>\n> @@ -102,6 +102,10 @@ struct control_type> : public control_type> {\n> \tstatic constexpr std::size_t size = N;\n> };\n>\n> +template\n> +struct control_type>> : public control_type {\n> +};\n> +\n> } /* namespace details */\n>\n> class ControlValue\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 430aa902eec8..29d3c6c194c1 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n> \t\tvalues.reserve(testPatternModes.size());\n>\n> \t\tfor (auto pattern : testPatternModes)\n> -\t\t\tvalues.emplace_back(static_cast(pattern));\n> +\t\t\tvalues.emplace_back(pattern);\n>\n> \t\tcontrols[&controls::draft::TestPatternMode] = ControlInfo(values);\n> \t}\n>\n> base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824\n> --\n> Regards,\n>\n> Laurent Pinchart\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 DAA7FC3257\n\tfor ;\n\tThu, 26 Sep 2024 09:13:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E614634FB;\n\tThu, 26 Sep 2024 11:13:51 +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 ADC82634F2\n\tfor ;\n\tThu, 26 Sep 2024 11:13:48 +0200 (CEST)","from ideasonboard.com (mob-5-90-51-229.net.vodafone.it\n\t[5.90.51.229])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 805578D4;\n\tThu, 26 Sep 2024 11:12:20 +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=\"Gwzg27rZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727341940;\n\tbh=iw59eU+FoC5k4hIN6k7QBC/aSqcX+moyFPgpoM/oBsQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gwzg27rZeoyLpGp0xJyBZTFrNzQ252nXz95D8htcQRqu1Ze875bCoPS0gfgOfwytv\n\tjuJu79idO2gEoNgsqYFyU/KVuVYwM7/0gPRC/mqjKT0awfxLTPbkGHJRTyPTfECZaI\n\tgTRO+npjJHpz6pl5tTvDELotbAd+lA3FIWy5JYCU=","Date":"Thu, 26 Sep 2024 11:13:44 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?q?Cl=C3=A1udio?=\n\tPaulo ","Subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","Message-ID":"","References":"<20240926002427.27703-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240926002427.27703-1-laurent.pinchart@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":31393,"web_url":"https://patchwork.libcamera.org/comment/31393/","msgid":"<172734652190.3726802.12952160199447613347@ping.linuxembedded.co.uk>","date":"2024-09-26T10:28:41","subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-09-26 10:13:44)\n> Hi Laurent\n> \n> On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:\n> > When constructing a ControlValue from an enum value, an explicit cast to\n> > int32_t is needed as we use int32_t as the underlying type for all\n> > enumerated controls. This makes users of ControlValue more complex. To\n> > simplify them, specialize the control_type template for enum types, to\n> > support construction of ControlValue directly without a cast.\n> >\n> > Signed-off-by: Laurent Pinchart \n> \n> brilliant, less things to type in!\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n> > ---\n> > include/libcamera/controls.h | 6 +++++-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > 2 files changed, 6 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 25f67ed948a3..c5131870d402 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -39,7 +39,7 @@ enum ControlType {\n> >\n> > namespace details {\n> >\n> > -template\n> > +template>\n> > struct control_type {\n> > };\n> >\n> > @@ -102,6 +102,10 @@ struct control_type> : public control_type> {\n> > static constexpr std::size_t size = N;\n> > };\n> >\n> > +template\n> > +struct control_type>> : public control_type {\n> > +};\n> > +\n\nI have no idea how you get this black magic to work ... :-)\n\nBut if it works and is reducing boilerplate casts which impact readability\nthen \\o/\n\nSo I think this makes a new templated control_type of the given enum\ntype, which is derived from an int32_t ...\n\nReviewed-by: Kieran Bingham \n\n> > } /* namespace details */\n> >\n> > class ControlValue\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 430aa902eec8..29d3c6c194c1 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n> > values.reserve(testPatternModes.size());\n> >\n> > for (auto pattern : testPatternModes)\n> > - values.emplace_back(static_cast(pattern));\n> > + values.emplace_back(pattern);\n> >\n> > controls[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > }\n> >\n> > base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 BF002C3257\n\tfor ;\n\tThu, 26 Sep 2024 10:28:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D896163511;\n\tThu, 26 Sep 2024 12:28: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 1AD4263502\n\tfor ;\n\tThu, 26 Sep 2024 12:28:45 +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 D97C7169;\n\tThu, 26 Sep 2024 12:27:16 +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=\"AlzzDZLi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727346436;\n\tbh=ex23T7LnFnu1QIP15K7kRiJgWdLH//3k1x9ba5l5Js8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=AlzzDZLi2TwH3i4+6bBQ6CHk+x/5+014+Okch4egUb7KxI+v0SdUm5K6hz4nIAlAm\n\tu4khcBJFjqOOkQjHe4pog83Ij7tO2jGyiNSPd3z9F7lCSy4cNvKzZNGe/ud9AZXZfm\n\tNI9U5X9luN0qAv3FqSbYfKCwtYGzHNA754ngDrA4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240926002427.27703-1-laurent.pinchart@ideasonboard.com>\n\t","Subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, =?utf-8?q?Cl=C3=A1udio?=\n\tPaulo ","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","Date":"Thu, 26 Sep 2024 11:28:41 +0100","Message-ID":"<172734652190.3726802.12952160199447613347@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":31398,"web_url":"https://patchwork.libcamera.org/comment/31398/","msgid":"<20240926103658.GC21788@pendragon.ideasonboard.com>","date":"2024-09-26T10:36:58","subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2024-09-26 10:13:44)\n> > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:\n> > > When constructing a ControlValue from an enum value, an explicit cast to\n> > > int32_t is needed as we use int32_t as the underlying type for all\n> > > enumerated controls. This makes users of ControlValue more complex. To\n> > > simplify them, specialize the control_type template for enum types, to\n> > > support construction of ControlValue directly without a cast.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > \n> > brilliant, less things to type in!\n> > \n> > Reviewed-by: Jacopo Mondi \n> > \n> > Thanks\n> > j\n> > \n> > > ---\n> > > include/libcamera/controls.h | 6 +++++-\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > 2 files changed, 6 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index 25f67ed948a3..c5131870d402 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -39,7 +39,7 @@ enum ControlType {\n> > >\n> > > namespace details {\n> > >\n> > > -template\n> > > +template>\n> > > struct control_type {\n> > > };\n> > >\n> > > @@ -102,6 +102,10 @@ struct control_type> : public control_type> {\n> > > static constexpr std::size_t size = N;\n> > > };\n> > >\n> > > +template\n> > > +struct control_type>> : public control_type {\n> > > +};\n> > > +\n> \n> I have no idea how you get this black magic to work ... :-)\n\nSometimes I'm not sure myself :-)\n\n> But if it works and is reducing boilerplate casts which impact readability\n> then \\o/\n> \n> So I think this makes a new templated control_type of the given enum\n> type, which is derived from an int32_t ...\n\nIt introduces a new specialization for the control_type structure\ntemplate, which is enabled only when T is a enumeration type. The\nspecialization inherits from control_type to make it explicit\nthat we're storing the value as an int32_t. I could have copied\n\n\tstatic constexpr ControlType value = ControlTypeInteger32;\n\tstatic constexpr std::size_t size = 0;\n\nfrom struct control_type instead, but inheriting expresses the\ngoal better I think.\n\nI had to add a second template parameter to the base template, as I\nneeded a place to introduce the std::enable_if_t<> in the template\nspecialization. Writing\n\ntemplate>> * = nullptr>\nstruct control_type : public control_type {\n};\n\nis not valid, as it's not a specialization of the base template.\n\nAs the second template parameter of the base template has a default\nvalue, none of the other template specialization need to be modified.\n\n> Reviewed-by: Kieran Bingham \n> \n> > > } /* namespace details */\n> > >\n> > > class ControlValue\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 430aa902eec8..29d3c6c194c1 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -958,7 +958,7 @@ int PipelineHandlerIPU3::updateControls(IPU3CameraData *data)\n> > > values.reserve(testPatternModes.size());\n> > >\n> > > for (auto pattern : testPatternModes)\n> > > - values.emplace_back(static_cast(pattern));\n> > > + values.emplace_back(pattern);\n> > >\n> > > controls[&controls::draft::TestPatternMode] = ControlInfo(values);\n> > > }\n> > >\n> > > base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824","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 49C08C0F1B\n\tfor ;\n\tThu, 26 Sep 2024 10:37:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03AF863510;\n\tThu, 26 Sep 2024 12:37:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 173DF63502\n\tfor ;\n\tThu, 26 Sep 2024 12:37:01 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE49F169;\n\tThu, 26 Sep 2024 12:35:32 +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=\"pQmV18re\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727346933;\n\tbh=u6AFriC7a/omnm2mUSTbt8ANu+MyqNB+vSKRDVWypUo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pQmV18reD0vxxUfl79QPfUnD4g8z1HqFCqAikoio6gvxSdapS/+7MGNNK8qOlbbjf\n\tz2Aq+RR2QphRMWOC/ra1jwhsOO8tcGk+gBdYNdY2V6KZcw9YG6soOtcBgYUJxA3dmv\n\tB4afJh0zfXXmWzxJp/nGBz3yJAZKyCYgx2iEKpw8=","Date":"Thu, 26 Sep 2024 13:36:58 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org, =?utf-8?q?Cl=C3=A1udio?=\n\tPaulo ","Subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","Message-ID":"<20240926103658.GC21788@pendragon.ideasonboard.com>","References":"<20240926002427.27703-1-laurent.pinchart@ideasonboard.com>\n\t\n\t<172734652190.3726802.12952160199447613347@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172734652190.3726802.12952160199447613347@ping.linuxembedded.co.uk>","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":31410,"web_url":"https://patchwork.libcamera.org/comment/31410/","msgid":"","date":"2024-09-26T15:07:05","subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart írta:\n\n> On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2024-09-26 10:13:44)\n> > > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:\n> > > > When constructing a ControlValue from an enum value, an explicit cast to\n> > > > int32_t is needed as we use int32_t as the underlying type for all\n> > > > enumerated controls. This makes users of ControlValue more complex. To\n> > > > simplify them, specialize the control_type template for enum types, to\n> > > > support construction of ControlValue directly without a cast.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > >\n> > > brilliant, less things to type in!\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > Thanks\n> > > j\n> > >\n> > > > ---\n> > > > include/libcamera/controls.h | 6 +++++-\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > > 2 files changed, 6 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > index 25f67ed948a3..c5131870d402 100644\n> > > > --- a/include/libcamera/controls.h\n> > > > +++ b/include/libcamera/controls.h\n> > > > @@ -39,7 +39,7 @@ enum ControlType {\n> > > >\n> > > > namespace details {\n> > > >\n> > > > -template\n> > > > +template>\n> > > > struct control_type {\n> > > > };\n> > > >\n> > > > @@ -102,6 +102,10 @@ struct control_type> : public control_type> {\n> > > > static constexpr std::size_t size = N;\n> > > > };\n> > > >\n> > > > +template\n> > > > +struct control_type>> : public control_type {\n> > > > +};\n> > > > +\n> >\n> > I have no idea how you get this black magic to work ... :-)\n> \n> Sometimes I'm not sure myself :-)\n\nIn C++20 one could just say something along the lines of\n\ntemplate requires (std::is_enum_v)\nstruct control_type : control_type { };\n\nA lot less of SFINAE \"magic\"...\n\nBut more importantly, consider\n\n enum class thing : std::uint16_t { a };\n ControlValue { thing::a };\n\nOne would expect this to work, but it won't because the assertion in\n\n void ControlValue::set(ControlType type, bool isArray, const void *data,\n \t\t std::size_t numElements, std::size_t elementSize)\n {\n \tASSERT(elementSize == ControlValueSize[type]);\n\naborts the process since `elementSize == sizeof(std::uint16_t) != sizeof(std::int32_t)`.\n\nThis affects every enum type whose underlying type is not the same size as `std::int32_t`.\nThis can be addressed by restricting the specialization even more with something like\n\n std::is_enum_v && sizeof(T) == sizeof(int32_t)\n\nOr properly supporting every enum type. The patch below shows a quick prototype\ntrying to do that, note that it uses C++20 for brevity but equivalent results can\nbe achieved with sufficient SFINAE usage. Also note, that it still does not\nproperly handle arrays of enums.\n\n\ndiff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\nindex 25f67ed9..db3919d5 100644\n--- a/include/libcamera/controls.h\n+++ b/include/libcamera/controls.h\n@@ -53,30 +54,34 @@ template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeBool;\n \tstatic constexpr std::size_t size = 0;\n+\tusing value_type = bool;\n };\n \n template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeByte;\n-\tstatic constexpr std::size_t size = 0;\n+\tusing value_type = uint8_t;\n };\n \n template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeInteger32;\n \tstatic constexpr std::size_t size = 0;\n+\tusing value_type = int32_t;\n };\n \n template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeInteger64;\n \tstatic constexpr std::size_t size = 0;\n+\tusing value_type = int64_t;\n };\n \n template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeFloat;\n \tstatic constexpr std::size_t size = 0;\n+\tusing value_type = float;\n };\n \n template<>\n@@ -89,12 +94,14 @@ template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeRectangle;\n \tstatic constexpr std::size_t size = 0;\n+\tusing value_type = Rectangle;\n };\n \n template<>\n struct control_type {\n \tstatic constexpr ControlType value = ControlTypeSize;\n \tstatic constexpr std::size_t size = 0;\n+\tusing value_type = Size;\n };\n \n template\n@@ -102,38 +109,24 @@ struct control_type> : public control_type> {\n \tstatic constexpr std::size_t size = N;\n };\n \n+template\n+\trequires (std::is_enum_v && sizeof(T) <= sizeof(int32_t))\n+struct control_type : control_type {\n+\tstatic constexpr auto convert(T x) { return static_cast(x); }\n+};\n+\n+template\n+\trequires (std::is_enum_v && sizeof(int32_t) < sizeof(T) && sizeof(T) <= sizeof(int64_t))\n+struct control_type : control_type {\n+\tstatic constexpr auto convert(T x) { return static_cast(x); }\n+};\n+\n } /* namespace details */\n \n class ControlValue\n {\n public:\n \tControlValue();\n-\n-#ifndef __DOXYGEN__\n-\ttemplate::value &&\n-\t\t\t\t\t details::control_type::value &&\n-\t\t\t\t\t !std::is_same>::value,\n-\t\t\t\t\t std::nullptr_t> = nullptr>\n-\tControlValue(const T &value)\n-\t\t: type_(ControlTypeNone), numElements_(0)\n-\t{\n-\t\tset(details::control_type>::value, false,\n-\t\t &value, 1, sizeof(T));\n-\t}\n-\n-\ttemplate::value ||\n-\t\t\t\t\t std::is_same>::value,\n-\t\t\t\t\t std::nullptr_t> = nullptr>\n-#else\n-\ttemplate\n-#endif\n-\tControlValue(const T &value)\n-\t\t: type_(ControlTypeNone), numElements_(0)\n-\t{\n-\t\tset(details::control_type>::value, true,\n-\t\t value.data(), value.size(), sizeof(typename T::value_type));\n-\t}\n-\n \t~ControlValue();\n \n \tControlValue(const ControlValue &other);\n@@ -182,28 +175,46 @@ public:\n \t\treturn T{ value, numElements_ };\n \t}\n \n-#ifndef __DOXYGEN__\n-\ttemplate::value &&\n-\t\t\t\t\t !std::is_same>::value,\n-\t\t\t\t\t std::nullptr_t> = nullptr>\n+\ttemplate\n+\t\trequires (requires { details::control_type>::value; }\n+\t\t\t && !details::is_span>::value\n+\t\t\t && !std::is_same_v>)\n \tvoid set(const T &value)\n \t{\n-\t\tset(details::control_type>::value, false,\n-\t\t reinterpret_cast(&value), 1, sizeof(T));\n+\t\tusing CT = details::control_type>;\n+\n+\t\tif constexpr (requires {\n+\t\t\ttypename CT::value_type;\n+\t\t\trequires !std::is_same_v>;\n+\t\t}) {\n+\t\t\ttypename CT::value_type cvalue = CT::convert(value);\n+\n+\t\t\tset(CT::value, false,\n+\t\t\t reinterpret_cast(&cvalue), 1, sizeof(cvalue));\n+\t\t}\n+\t\telse {\n+\t\t\tset(CT::value, false,\n+\t\t\t reinterpret_cast(&value), 1, sizeof(value));\n+\t\t}\n \t}\n \n-\ttemplate::value ||\n-\t\t\t\t\t std::is_same>::value,\n-\t\t\t\t\t std::nullptr_t> = nullptr>\n-#else\n \ttemplate\n-#endif\n+\t\trequires (requires { details::control_type>::value; }\n+\t\t\t && (details::is_span::value || std::is_same_v>))\n \tvoid set(const T &value)\n \t{\n \t\tset(details::control_type>::value, true,\n \t\t value.data(), value.size(), sizeof(typename T::value_type));\n \t}\n \n+\ttemplate\n+\tControlValue(const T &value)\n+\t\trequires (requires { set(value); })\n+\t\t: type_(ControlTypeNone), numElements_(0)\n+\t{\n+\t\tset(value);\n+\t}\n+\n \tvoid reserve(ControlType type, bool isArray = false,\n \t\t std::size_t numElements = 1);\n \n\n\n> [...]\n\n\nRegards,\nBarnabás Pőcze","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 AC8C4C0F1B\n\tfor ;\n\tThu, 26 Sep 2024 15:07:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE05F6350F;\n\tThu, 26 Sep 2024 17:07:10 +0200 (CEST)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 974DE634F9\n\tfor ;\n\tThu, 26 Sep 2024 17:07:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"xhyMKLzC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1727363228; x=1727622428;\n\tbh=zM+HFU2YsIwI3WG4Dy6tRX0RRtsZaXAFJtxS3jo6kec=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=xhyMKLzC5ztUPZqt/nuIDatmmvPTtn8r6W/sgDJg7jW39lY1eNC9tDoo1pycBOYsm\n\tvkRMK9QsN8gQGhcIkHUJV+oU0/xza5ZfTDPkxpVN89uV6IMDnQ3Yt29IwbGbaPGtSL\n\tdfLuigd/ruA0005eIDKPemFSW4bQxd5WgaDcdPVaC89Giy5NdpDqrP+1w/8vPI9oKA\n\t/oWlp2q27rHCeZCObLm5B+9c4R4jvk/QffMNscz7KJz5TJ+8RCyjOqtVgxtbX1gKdb\n\tgToJhvzrzA3CXcRgwpRNhHZRC7yPXIMYMecvmKVzhxsorFC3qC4O5SEj3bgYXIxnEY\n\tq2WlENcOpxUzg==","Date":"Thu, 26 Sep 2024 15:07:05 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham , Jacopo Mondi\n\t, libcamera-devel@lists.libcamera.org,\n\t=?utf-8?q?Cl=C3=A1udio_Paulo?= ","Subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","Message-ID":"","In-Reply-To":"<20240926103658.GC21788@pendragon.ideasonboard.com>","References":"<20240926002427.27703-1-laurent.pinchart@ideasonboard.com>\n\t\n\t<172734652190.3726802.12952160199447613347@ping.linuxembedded.co.uk>\n\t<20240926103658.GC21788@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"718cbaf1a79505e0cc5c121e1e8a4f582c07b5de","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":31427,"web_url":"https://patchwork.libcamera.org/comment/31427/","msgid":"<20240926235850.GN21788@pendragon.ideasonboard.com>","date":"2024-09-26T23:58:50","subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Thu, Sep 26, 2024 at 03:07:05PM +0000, Barnabás Pőcze wrote:\n> 2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart írta:\n> > On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:\n> > > Quoting Jacopo Mondi (2024-09-26 10:13:44)\n> > > > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:\n> > > > > When constructing a ControlValue from an enum value, an explicit cast to\n> > > > > int32_t is needed as we use int32_t as the underlying type for all\n> > > > > enumerated controls. This makes users of ControlValue more complex. To\n> > > > > simplify them, specialize the control_type template for enum types, to\n> > > > > support construction of ControlValue directly without a cast.\n> > > > >\n> > > > > Signed-off-by: Laurent Pinchart \n> > > >\n> > > > brilliant, less things to type in!\n> > > >\n> > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > > ---\n> > > > > include/libcamera/controls.h | 6 +++++-\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > > > 2 files changed, 6 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > index 25f67ed948a3..c5131870d402 100644\n> > > > > --- a/include/libcamera/controls.h\n> > > > > +++ b/include/libcamera/controls.h\n> > > > > @@ -39,7 +39,7 @@ enum ControlType {\n> > > > >\n> > > > > namespace details {\n> > > > >\n> > > > > -template\n> > > > > +template>\n> > > > > struct control_type {\n> > > > > };\n> > > > >\n> > > > > @@ -102,6 +102,10 @@ struct control_type> : public control_type> {\n> > > > > static constexpr std::size_t size = N;\n> > > > > };\n> > > > >\n> > > > > +template\n> > > > > +struct control_type>> : public control_type {\n> > > > > +};\n> > > > > +\n> > >\n> > > I have no idea how you get this black magic to work ... :-)\n> > \n> > Sometimes I'm not sure myself :-)\n> \n> In C++20 one could just say something along the lines of\n> \n> template requires (std::is_enum_v)\n> struct control_type : control_type { };\n> \n> A lot less of SFINAE \"magic\"...\n\nThere are quite a few C++20 features I'd love to use in libcamera. We\ncould finally replace Span with std::span :-)\n\n> But more importantly, consider\n> \n> enum class thing : std::uint16_t { a };\n> ControlValue { thing::a };\n> \n> One would expect this to work, but it won't because the assertion in\n> \n> void ControlValue::set(ControlType type, bool isArray, const void *data,\n> \t\t std::size_t numElements, std::size_t elementSize)\n> {\n> \tASSERT(elementSize == ControlValueSize[type]);\n> \n> aborts the process since `elementSize == sizeof(std::uint16_t) != sizeof(std::int32_t)`.\n> \n> This affects every enum type whose underlying type is not the same size as `std::int32_t`.\n> This can be addressed by restricting the specialization even more with something like\n> \n> std::is_enum_v && sizeof(T) == sizeof(int32_t)\n> \n> Or properly supporting every enum type. The patch below shows a quick prototype\n> trying to do that, note that it uses C++20 for brevity but equivalent results can\n> be achieved with sufficient SFINAE usage. Also note, that it still does not\n> properly handle arrays of enums.\n\nCouldn't it be done simply by inheriting from\ncontrol_type> ? I've considered that, but\ngiven that we have standardized enums to use int32_t as the underlying\ntype of libcamera controls, I decided not to go that way.\n\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 25f67ed9..db3919d5 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -53,30 +54,34 @@ template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeBool;\n> \tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = bool;\n> };\n> \n> template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeByte;\n> -\tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = uint8_t;\n> };\n> \n> template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeInteger32;\n> \tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = int32_t;\n> };\n> \n> template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeInteger64;\n> \tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = int64_t;\n> };\n> \n> template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeFloat;\n> \tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = float;\n> };\n> \n> template<>\n> @@ -89,12 +94,14 @@ template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeRectangle;\n> \tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = Rectangle;\n> };\n> \n> template<>\n> struct control_type {\n> \tstatic constexpr ControlType value = ControlTypeSize;\n> \tstatic constexpr std::size_t size = 0;\n> +\tusing value_type = Size;\n> };\n> \n> template\n> @@ -102,38 +109,24 @@ struct control_type> : public control_type> {\n> \tstatic constexpr std::size_t size = N;\n> };\n> \n> +template\n> +\trequires (std::is_enum_v && sizeof(T) <= sizeof(int32_t))\n> +struct control_type : control_type {\n> +\tstatic constexpr auto convert(T x) { return static_cast(x); }\n> +};\n> +\n> +template\n> +\trequires (std::is_enum_v && sizeof(int32_t) < sizeof(T) && sizeof(T) <= sizeof(int64_t))\n> +struct control_type : control_type {\n> +\tstatic constexpr auto convert(T x) { return static_cast(x); }\n> +};\n> +\n> } /* namespace details */\n> \n> class ControlValue\n> {\n> public:\n> \tControlValue();\n> -\n> -#ifndef __DOXYGEN__\n> -\ttemplate::value &&\n> -\t\t\t\t\t details::control_type::value &&\n> -\t\t\t\t\t !std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -\tControlValue(const T &value)\n> -\t\t: type_(ControlTypeNone), numElements_(0)\n> -\t{\n> -\t\tset(details::control_type>::value, false,\n> -\t\t &value, 1, sizeof(T));\n> -\t}\n> -\n> -\ttemplate::value ||\n> -\t\t\t\t\t std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -#else\n> -\ttemplate\n> -#endif\n> -\tControlValue(const T &value)\n> -\t\t: type_(ControlTypeNone), numElements_(0)\n> -\t{\n> -\t\tset(details::control_type>::value, true,\n> -\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> -\t}\n> -\n> \t~ControlValue();\n> \n> \tControlValue(const ControlValue &other);\n> @@ -182,28 +175,46 @@ public:\n> \t\treturn T{ value, numElements_ };\n> \t}\n> \n> -#ifndef __DOXYGEN__\n> -\ttemplate::value &&\n> -\t\t\t\t\t !std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> +\ttemplate\n> +\t\trequires (requires { details::control_type>::value; }\n> +\t\t\t && !details::is_span>::value\n> +\t\t\t && !std::is_same_v>)\n> \tvoid set(const T &value)\n> \t{\n> -\t\tset(details::control_type>::value, false,\n> -\t\t reinterpret_cast(&value), 1, sizeof(T));\n> +\t\tusing CT = details::control_type>;\n> +\n> +\t\tif constexpr (requires {\n> +\t\t\ttypename CT::value_type;\n> +\t\t\trequires !std::is_same_v>;\n> +\t\t}) {\n> +\t\t\ttypename CT::value_type cvalue = CT::convert(value);\n> +\n> +\t\t\tset(CT::value, false,\n> +\t\t\t reinterpret_cast(&cvalue), 1, sizeof(cvalue));\n> +\t\t}\n> +\t\telse {\n> +\t\t\tset(CT::value, false,\n> +\t\t\t reinterpret_cast(&value), 1, sizeof(value));\n> +\t\t}\n> \t}\n> \n> -\ttemplate::value ||\n> -\t\t\t\t\t std::is_same>::value,\n> -\t\t\t\t\t std::nullptr_t> = nullptr>\n> -#else\n> \ttemplate\n> -#endif\n> +\t\trequires (requires { details::control_type>::value; }\n> +\t\t\t && (details::is_span::value || std::is_same_v>))\n> \tvoid set(const T &value)\n> \t{\n> \t\tset(details::control_type>::value, true,\n> \t\t value.data(), value.size(), sizeof(typename T::value_type));\n> \t}\n> \n> +\ttemplate\n> +\tControlValue(const T &value)\n> +\t\trequires (requires { set(value); })\n> +\t\t: type_(ControlTypeNone), numElements_(0)\n> +\t{\n> +\t\tset(value);\n> +\t}\n> +\n> \tvoid reserve(ControlType type, bool isArray = false,\n> \t\t std::size_t numElements = 1);\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 534F1C3257\n\tfor ;\n\tThu, 26 Sep 2024 23:58:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C04F6350F;\n\tFri, 27 Sep 2024 01:58:55 +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 95BE2634F4\n\tfor ;\n\tFri, 27 Sep 2024 01:58:53 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2D7322F;\n\tFri, 27 Sep 2024 01:57:24 +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=\"c+cXNnRv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727395044;\n\tbh=6jVzeBlMw00jyXVwLjW9ft2pvFu02pQVumlejdf9QD4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c+cXNnRvp/fWrt7n5mM4esEkVhiAu4F6K3n+w0Q5MV3sj9BtYf9YYtQ+Wp0iHQc9G\n\t++wikTkDWAs8EaQ8pSi3x+DaErMd8E5LNY26m9N8j2Plkf8uzQUQEMTv5YZYlxyGF9\n\tCfUTg248d4Kh8d983ENRe4aBsrQpf0omGQCJNBsI=","Date":"Fri, 27 Sep 2024 02:58:50 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham , Jacopo Mondi\n\t, libcamera-devel@lists.libcamera.org,\n\t=?utf-8?q?Cl=C3=A1udio?= Paulo ","Subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","Message-ID":"<20240926235850.GN21788@pendragon.ideasonboard.com>","References":"<20240926002427.27703-1-laurent.pinchart@ideasonboard.com>\n\t\n\t<172734652190.3726802.12952160199447613347@ping.linuxembedded.co.uk>\n\t<20240926103658.GC21788@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":31439,"web_url":"https://patchwork.libcamera.org/comment/31439/","msgid":"","date":"2024-09-27T10:27:01","subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2024. szeptember 27., péntek 1:58 keltezéssel, Laurent Pinchart írta:\n\n\n> Hi Barnabás,\n> \n> On Thu, Sep 26, 2024 at 03:07:05PM +0000, Barnabás Pőcze wrote:\n> > 2024. szeptember 26., csütörtök 12:36 keltezéssel, Laurent Pinchart írta:\n> > > On Thu, Sep 26, 2024 at 11:28:41AM +0100, Kieran Bingham wrote:\n> > > > Quoting Jacopo Mondi (2024-09-26 10:13:44)\n> > > > > On Thu, Sep 26, 2024 at 03:24:27AM GMT, Laurent Pinchart wrote:\n> > > > > > When constructing a ControlValue from an enum value, an explicit cast to\n> > > > > > int32_t is needed as we use int32_t as the underlying type for all\n> > > > > > enumerated controls. This makes users of ControlValue more complex. To\n> > > > > > simplify them, specialize the control_type template for enum types, to\n> > > > > > support construction of ControlValue directly without a cast.\n> > > > > >\n> > > > > > Signed-off-by: Laurent Pinchart \n> > > > >\n> > > > > brilliant, less things to type in!\n> > > > >\n> > > > > Reviewed-by: Jacopo Mondi \n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > > ---\n> > > > > > include/libcamera/controls.h | 6 +++++-\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > > > > > 2 files changed, 6 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > > > > index 25f67ed948a3..c5131870d402 100644\n> > > > > > --- a/include/libcamera/controls.h\n> > > > > > +++ b/include/libcamera/controls.h\n> > > > > > @@ -39,7 +39,7 @@ enum ControlType {\n> > > > > >\n> > > > > > namespace details {\n> > > > > >\n> > > > > > -template\n> > > > > > +template>\n> > > > > > struct control_type {\n> > > > > > };\n> > > > > >\n> > > > > > @@ -102,6 +102,10 @@ struct control_type> : public control_type> {\n> > > > > > static constexpr std::size_t size = N;\n> > > > > > };\n> > > > > >\n> > > > > > +template\n> > > > > > +struct control_type>> : public control_type {\n> > > > > > +};\n> > > > > > +\n> > > >\n> > > > I have no idea how you get this black magic to work ... :-)\n> > >\n> > > Sometimes I'm not sure myself :-)\n> >\n> > In C++20 one could just say something along the lines of\n> >\n> > template requires (std::is_enum_v)\n> > struct control_type : control_type { };\n> >\n> > A lot less of SFINAE \"magic\"...\n> \n> There are quite a few C++20 features I'd love to use in libcamera. We\n> could finally replace Span with std::span :-)\n> \n> > But more importantly, consider\n> >\n> > enum class thing : std::uint16_t { a };\n> > ControlValue { thing::a };\n> >\n> > One would expect this to work, but it won't because the assertion in\n> >\n> > void ControlValue::set(ControlType type, bool isArray, const void *data,\n> > \t\t std::size_t numElements, std::size_t elementSize)\n> > {\n> > \tASSERT(elementSize == ControlValueSize[type]);\n> >\n> > aborts the process since `elementSize == sizeof(std::uint16_t) != sizeof(std::int32_t)`.\n> >\n> > This affects every enum type whose underlying type is not the same size as `std::int32_t`.\n> > This can be addressed by restricting the specialization even more with something like\n> >\n> > std::is_enum_v && sizeof(T) == sizeof(int32_t)\n> >\n> > Or properly supporting every enum type. The patch below shows a quick prototype\n> > trying to do that, note that it uses C++20 for brevity but equivalent results can\n> > be achieved with sufficient SFINAE usage. Also note, that it still does not\n> > properly handle arrays of enums.\n> \n> Couldn't it be done simply by inheriting from\n> control_type> ? I've considered that, but\n> given that we have standardized enums to use int32_t as the underlying\n> type of libcamera controls, I decided not to go that way.\n\nI guess that should work. But only for enums which have int{32,64}_t as their\nunderlying type since those are the only two integer types supported. I would\nimagine uint{32,64}_t are also prime candidates for enums. But I suppose it\nshouldn't be too difficult to add (all) other fixed-width integer types as well.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 25f67ed9..db3919d5 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -53,30 +54,34 @@ template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeBool;\n> > \tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = bool;\n> > };\n> >\n> > template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeByte;\n> > -\tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = uint8_t;\n> > };\n> >\n> > template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeInteger32;\n> > \tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = int32_t;\n> > };\n> >\n> > template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeInteger64;\n> > \tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = int64_t;\n> > };\n> >\n> > template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeFloat;\n> > \tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = float;\n> > };\n> >\n> > template<>\n> > @@ -89,12 +94,14 @@ template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeRectangle;\n> > \tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = Rectangle;\n> > };\n> >\n> > template<>\n> > struct control_type {\n> > \tstatic constexpr ControlType value = ControlTypeSize;\n> > \tstatic constexpr std::size_t size = 0;\n> > +\tusing value_type = Size;\n> > };\n> >\n> > template\n> > @@ -102,38 +109,24 @@ struct control_type> : public control_type> {\n> > \tstatic constexpr std::size_t size = N;\n> > };\n> >\n> > +template\n> > +\trequires (std::is_enum_v && sizeof(T) <= sizeof(int32_t))\n> > +struct control_type : control_type {\n> > +\tstatic constexpr auto convert(T x) { return static_cast(x); }\n> > +};\n> > +\n> > +template\n> > +\trequires (std::is_enum_v && sizeof(int32_t) < sizeof(T) && sizeof(T) <= sizeof(int64_t))\n> > +struct control_type : control_type {\n> > +\tstatic constexpr auto convert(T x) { return static_cast(x); }\n> > +};\n> > +\n> > } /* namespace details */\n> >\n> > class ControlValue\n> > {\n> > public:\n> > \tControlValue();\n> > -\n> > -#ifndef __DOXYGEN__\n> > -\ttemplate::value &&\n> > -\t\t\t\t\t details::control_type::value &&\n> > -\t\t\t\t\t !std::is_same>::value,\n> > -\t\t\t\t\t std::nullptr_t> = nullptr>\n> > -\tControlValue(const T &value)\n> > -\t\t: type_(ControlTypeNone), numElements_(0)\n> > -\t{\n> > -\t\tset(details::control_type>::value, false,\n> > -\t\t &value, 1, sizeof(T));\n> > -\t}\n> > -\n> > -\ttemplate::value ||\n> > -\t\t\t\t\t std::is_same>::value,\n> > -\t\t\t\t\t std::nullptr_t> = nullptr>\n> > -#else\n> > -\ttemplate\n> > -#endif\n> > -\tControlValue(const T &value)\n> > -\t\t: type_(ControlTypeNone), numElements_(0)\n> > -\t{\n> > -\t\tset(details::control_type>::value, true,\n> > -\t\t value.data(), value.size(), sizeof(typename T::value_type));\n> > -\t}\n> > -\n> > \t~ControlValue();\n> >\n> > \tControlValue(const ControlValue &other);\n> > @@ -182,28 +175,46 @@ public:\n> > \t\treturn T{ value, numElements_ };\n> > \t}\n> >\n> > -#ifndef __DOXYGEN__\n> > -\ttemplate::value &&\n> > -\t\t\t\t\t !std::is_same>::value,\n> > -\t\t\t\t\t std::nullptr_t> = nullptr>\n> > +\ttemplate\n> > +\t\trequires (requires { details::control_type>::value; }\n> > +\t\t\t && !details::is_span>::value\n> > +\t\t\t && !std::is_same_v>)\n> > \tvoid set(const T &value)\n> > \t{\n> > -\t\tset(details::control_type>::value, false,\n> > -\t\t reinterpret_cast(&value), 1, sizeof(T));\n> > +\t\tusing CT = details::control_type>;\n> > +\n> > +\t\tif constexpr (requires {\n> > +\t\t\ttypename CT::value_type;\n> > +\t\t\trequires !std::is_same_v>;\n> > +\t\t}) {\n> > +\t\t\ttypename CT::value_type cvalue = CT::convert(value);\n> > +\n> > +\t\t\tset(CT::value, false,\n> > +\t\t\t reinterpret_cast(&cvalue), 1, sizeof(cvalue));\n> > +\t\t}\n> > +\t\telse {\n> > +\t\t\tset(CT::value, false,\n> > +\t\t\t reinterpret_cast(&value), 1, sizeof(value));\n> > +\t\t}\n> > \t}\n> >\n> > -\ttemplate::value ||\n> > -\t\t\t\t\t std::is_same>::value,\n> > -\t\t\t\t\t std::nullptr_t> = nullptr>\n> > -#else\n> > \ttemplate\n> > -#endif\n> > +\t\trequires (requires { details::control_type>::value; }\n> > +\t\t\t && (details::is_span::value || std::is_same_v>))\n> > \tvoid set(const T &value)\n> > \t{\n> > \t\tset(details::control_type>::value, true,\n> > \t\t value.data(), value.size(), sizeof(typename T::value_type));\n> > \t}\n> >\n> > +\ttemplate\n> > +\tControlValue(const T &value)\n> > +\t\trequires (requires { set(value); })\n> > +\t\t: type_(ControlTypeNone), numElements_(0)\n> > +\t{\n> > +\t\tset(value);\n> > +\t}\n> > +\n> > \tvoid reserve(ControlType type, bool isArray = false,\n> > \t\t std::size_t numElements = 1);\n> >\n> > > [...]\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\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 DF367C3257\n\tfor ;\n\tFri, 27 Sep 2024 10:27:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BD4BE6350F;\n\tFri, 27 Sep 2024 12:27:08 +0200 (CEST)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C47C634F2\n\tfor ;\n\tFri, 27 Sep 2024 12:27:07 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"lDqGeiJi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1727432826; x=1727692026;\n\tbh=Ot5E6bujkTxvqPE+5l2+abF5sqrSy/ITs40THyWsVbk=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=lDqGeiJidqlhFNnhFNL+0+VL7OWPRzHNNyFB0w0n7eYSOUlXbZ4RYtR8xTegj8QDY\n\t/RU8pYKuSpXV7Huc8oqBqQdigx8MiKADfP2RLlCJ8rVxsBRPfIlY86LTLSTGzF8Ckh\n\t9XT2kpe65Hfku6I5JlIY5zoEAuObaaNodQFAcfp1ZyBW3X7k/tDDFT//IXZuFiArjK\n\tk8kzIqY/u1S8vXsWkUnmY3yF5FJxcjrxA+IT/jzjWQEDgbYUjJAM7m5V/fnSi5yft1\n\tZ7rPWhUfnUPWFyW1wcI2RCI1LB2dl83vmZpMQW/dkSQRk4+ZYvwYMQazXRlzaq99Kq\n\tBdrE2zlZ9E7yw==","Date":"Fri, 27 Sep 2024 10:27:01 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham , Jacopo Mondi\n\t, libcamera-devel@lists.libcamera.org,\n\t=?utf-8?q?Cl=C3=A1udio_Paulo?= ","Subject":"Re: [PATCH] libcamera: controls: Handle enum values without a cast","Message-ID":"","In-Reply-To":"<20240926235850.GN21788@pendragon.ideasonboard.com>","References":"<20240926002427.27703-1-laurent.pinchart@ideasonboard.com>\n\t\n\t<172734652190.3726802.12952160199447613347@ping.linuxembedded.co.uk>\n\t<20240926103658.GC21788@pendragon.ideasonboard.com>\n\t\n\t<20240926235850.GN21788@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"293422d8aaf205f553a44a4e0e61eb30b4b80f06","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}}]