[{"id":31172,"web_url":"https://patchwork.libcamera.org/comment/31172/","msgid":"<20240911160652.GD4470@pendragon.ideasonboard.com>","date":"2024-09-11T16:06:52","subject":"Re: [PATCH v3 1/3] libcamera: controls: Add enum names and values\n\tmap to ControlId","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Sep 11, 2024 at 11:35:58AM +0200, Paul Elder wrote:\n> Add to ControlId information about the names and values of enum, in the\n> event that the ControlId is an enum type. This allows applications to\n> query the ControlId for the names of the enum values, so that they can\n> be displayed on a UI, for example. Without this, it was necessary to use\n> macros of NameOfControlNameValueMap, which is difficult to use and is\n> very inflexible.\n> \n> The main map is name -> value, as this was necessary for mapping names\n> from tuning files to enum values. We reuse this generated code to\n> reduce complexity, and generate a reverse map from it.\n\nThis is confusing, I read it as ControlId having a forward map already,\nbut it doesn't.\n\nIf I understand this correctly, the forward map exists in the generated\ncontrol_ids.h header, you're now passing it to the ControlId\nconstructor. It would be clearer to explain that the forward map exists\nin generated code, so you pass it to the ControlId constructor, which\nthen generates the reverse map.\n\n> Signed-off-by: Paul Elder \n> Reviewed-by: Umang Jain \n> \n> ---\n> Changes in v3:\n> - s/nameValueMap/enumStrMap/\n> - s/enumName/enumToString/\n> \n> No change in v2\n> ---\n> include/libcamera/controls.h | 15 +++++++++------\n> src/libcamera/control_ids.cpp.in | 4 +++-\n> src/libcamera/controls.cpp | 29 +++++++++++++++++++++++++++++\n> 3 files changed, 41 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 7c2bb2872..8959dfcc2 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -213,14 +214,14 @@ private:\n> class ControlId\n> {\n> public:\n> -\tControlId(unsigned int id, const std::string &name, ControlType type)\n> -\t\t: id_(id), name_(name), type_(type)\n> -\t{\n> -\t}\n> +\tControlId(unsigned int id, const std::string &name, ControlType type,\n> +\t\t const std::map &enumStrMap = {});\n> \n> \tunsigned int id() const { return id_; }\n> \tconst std::string &name() const { return name_; }\n> \tControlType type() const { return type_; }\n> +\tconst std::map &enumStrMap() const { return enumStrMap_; }\n\nThis function is never used. Unless you have a plan to use it, I'd drop\nit. You can then also drop enumStrMap_.\n\n> +\tconst std::string enumToString(int32_t value) const;\n\nHow about\n\n\tconst std::map& enumerators() const { return reverseMap_; }\n\n? You wouldn't need to return a magic \"UNKNOWN\" value.\n\n> \n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(ControlId)\n> @@ -228,6 +229,8 @@ private:\n> \tunsigned int id_;\n> \tstd::string name_;\n> \tControlType type_;\n> +\tstd::map enumStrMap_;\n> +\tstd::map reverseMap_;\n> };\n> \n> static inline bool operator==(unsigned int lhs, const ControlId &rhs)\n> @@ -256,8 +259,8 @@ class Control : public ControlId\n> public:\n> \tusing type = T;\n> \n> -\tControl(unsigned int id, const char *name)\n> -\t\t: ControlId(id, name, details::control_type>::value)\n> +\tControl(unsigned int id, const char *name, const std::map &enumStrMap = {})\n> +\t\t: ControlId(id, name, details::control_type>::value, enumStrMap)\n> \t{\n> \t}\n> \n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index 05c8fb385..3a2049311 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -89,8 +89,10 @@ extern const std::map {{ctrl.name}}NameValueMap = {\n> \t{ \"{{enum.name}}\", {{enum.name}} },\n> {%- endfor %}\n> };\n> -{% endif -%}\n> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\", {{ctrl.name}}NameValueMap);\n> +{% else -%}\n> extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, \"{{ctrl.name}}\");\n> +{% endif -%}\n> {%- endfor %}\n> \n> {% if vendor != 'libcamera' %}\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index dba744042..0f83139fb 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \n> \n> +#include \n\nIs this used below or a leftover ?\n\n> +#include \n\nYou can drop map as controls.h is guaranteed to provide it.\n\n> #include \n> #include \n> #include \n> @@ -389,7 +391,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * \\param[in] id The control numerical ID\n> * \\param[in] name The control name\n> * \\param[in] type The control data type\n> + * \\param[in] enumStrMap The map from enum names to values (optional)\n> */\n> +ControlId::ControlId(unsigned int id, const std::string &name, ControlType type,\n> +\t\t const std::map &enumStrMap)\n> +\t: id_(id), name_(name), type_(type), enumStrMap_(enumStrMap)\n> +{\n> +\tfor (const auto &pair : enumStrMap_)\n> +\t\treverseMap_[pair.second] = pair.first;\n> +}\n> \n> /**\n> * \\fn unsigned int ControlId::id() const\n> @@ -409,6 +419,24 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * \\return The control data type\n> */\n> \n> +/**\n> + * \\brief Retrieve the name of an enum value\n> + * \\return The name of the enum value\n> + */\n> +const std::string ControlId::enumToString(int32_t value) const\n> +{\n> +\tif (reverseMap_.find(value) != reverseMap_.end())\n> +\t\treturn reverseMap_.at(value);\n> +\n> +\treturn \"UNKNOWN\";\n> +}\n> +\n> +/**\n> + * \\fn std::map ControlId::enumStrMap() const\n> + * \\brief Retrieve the map from enum names to enum values (if applicable)\n> + * \\return The map from enum names to enum values\n> + */\n> +\n> /**\n> * \\fn bool operator==(unsigned int lhs, const ControlId &rhs)\n> * \\brief Compare a ControlId with a control numerical ID\n> @@ -459,6 +487,7 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen\n> * \\brief Construct a Control instance\n> * \\param[in] id The control numerical ID\n> * \\param[in] name The control name\n> + * \\param[in] enumStrMap The map from enum names to values (optional)\n> *\n> * The control data type is automatically deduced from the template type 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 973F7BF415\n\tfor ;\n\tWed, 11 Sep 2024 16:07:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2190618F2;\n\tWed, 11 Sep 2024 18:07: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 BCC4E618F2\n\tfor ;\n\tWed, 11 Sep 2024 18:07:26 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(213-229-8-243.static.upcbusiness.at [213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC76ABEB;\n\tWed, 11 Sep 2024 18:06:08 +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=\"sfHVqhLi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726070769;\n\tbh=vDyT4V6z8Fp1rBA5MUXPO9Nef7L4VQAN5AdTTClvKyc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sfHVqhLi2V9Qx7WeuKsBdAAQeUSqLpr9ojeuo+pDQUl+DZp02OjY3k3TZfGnwKcxY\n\t6V/t962SA0Ax+abrJBubrKCy8mLv97Sg2Vj+mZFzBxqU77CTI2xLHgmjJa3RQ1XtC7\n\tSl7yoDSikpL6H1Z9y2GG6BgfkCLQOne5LooqejXY=","Date":"Wed, 11 Sep 2024 19:06:52 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, stefan.klug@ideasonboard.com,\n\tUmang Jain ","Subject":"Re: [PATCH v3 1/3] libcamera: controls: Add enum names and values\n\tmap to ControlId","Message-ID":"<20240911160652.GD4470@pendragon.ideasonboard.com>","References":"<20240911093600.671979-1-paul.elder@ideasonboard.com>\n\t<20240911093600.671979-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240911093600.671979-2-paul.elder@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\" "}}]