[PATCH 1/3] libcamera: controls: Add enum names and values map to ControlId

[Umang Jain]

Hi Paul,

On 11/09/24 2:18 am, Paul Elder wrote:
> Add to ControlId information about the names and values of enum, in the
> event that the ControlId is an enum type. This allows applications to
> query the ControlId for the names of the enum values, so that they can
> be displayed on a UI, for example. Without this, it was necessary to use
> macros of NameOfControlNameValueMap, which is difficult to use and is
> very inflexible.
>
> The main map is name -> value, as this was necessary for mapping names
> from tuning files to enum values. We reuse this generated code to
> reduce complexity, and generate a reverse map from it.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> No change in v2
> ---
>   include/libcamera/controls.h     | 15 +++++++++------
>   src/libcamera/control_ids.cpp.in |  4 +++-
>   src/libcamera/controls.cpp       | 29 +++++++++++++++++++++++++++++
>   3 files changed, 41 insertions(+), 7 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 7c2bb2872..407a12256 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -8,6 +8,7 @@
>   #pragma once
>   
>   #include <assert.h>
> +#include <map>
>   #include <optional>
>   #include <set>
>   #include <stdint.h>
> @@ -213,14 +214,14 @@ private:
>   class ControlId
>   {
>   public:
> -	ControlId(unsigned int id, const std::string &name, ControlType type)
> -		: id_(id), name_(name), type_(type)
> -	{
> -	}
> +	ControlId(unsigned int id, const std::string &name, ControlType type,
> +		  const std::map<std::string, int32_t> &nameValueMap = {});
>   
>   	unsigned int id() const { return id_; }
>   	const std::string &name() const { return name_; }
>   	ControlType type() const { return type_; }
> +	const std::map<std::string, int32_t> &nameValueMap() const { return nameValueMap_; }

maybe enumStrMap_  and enumStrMap() ?

This is a suggestion throughout the patch, as 'name' sounds very generic 
and can be confused/associated with name_ in my opinion
> +	const std::string enumName(int32_t value) const;

maybe 'enumToString()'  ?

Other than this,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>   
>   private:
>   	LIBCAMERA_DISABLE_COPY_AND_MOVE(ControlId)
> @@ -228,6 +229,8 @@ private:
>   	unsigned int id_;
>   	std::string name_;
>   	ControlType type_;
> +	std::map<std::string, int32_t> nameValueMap_;
> +	std::map<int32_t, std::string> reverseMap_;
>   };
>   
>   static inline bool operator==(unsigned int lhs, const ControlId &rhs)
> @@ -256,8 +259,8 @@ class Control : public ControlId
>   public:
>   	using type = T;
>   
> -	Control(unsigned int id, const char *name)
> -		: ControlId(id, name, details::control_type<std::remove_cv_t<T>>::value)
> +	Control(unsigned int id, const char *name, const std::map<std::string, int32_t> &nameValueMap = {})
> +		: ControlId(id, name, details::control_type<std::remove_cv_t<T>>::value, nameValueMap)
>   	{
>   	}
>   
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index 05c8fb385..3a2049311 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -89,8 +89,10 @@ extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap = {
>   	{ "{{enum.name}}", {{enum.name}} },
>   {%- endfor %}
>   };
> -{% endif -%}
> +extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}", {{ctrl.name}}NameValueMap);
> +{% else -%}
>   extern const Control<{{ctrl.type}}> {{ctrl.name}}({{ctrl.name|snake_case|upper}}, "{{ctrl.name}}");
> +{% endif -%}
>   {%- endfor %}
>   
>   {% if vendor != 'libcamera' %}
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index dba744042..1c7e7fde1 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -7,6 +7,8 @@
>   
>   #include <libcamera/controls.h>
>   
> +#include <algorithm>
> +#include <map>
>   #include <sstream>
>   #include <string.h>
>   #include <string>
> @@ -389,7 +391,15 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>    * \param[in] id The control numerical ID
>    * \param[in] name The control name
>    * \param[in] type The control data type
> + * \param[in] nameValueMap The map from enum names to values (optional)
>    */
> +ControlId::ControlId(unsigned int id, const std::string &name, ControlType type,
> +		     const std::map<std::string, int32_t> &nameValueMap)
> +	: id_(id), name_(name), type_(type), nameValueMap_(nameValueMap)
> +{
> +	for (const auto &pair : nameValueMap_)
> +		reverseMap_[pair.second] = pair.first;
> +}
>   
>   /**
>    * \fn unsigned int ControlId::id() const
> @@ -409,6 +419,24 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>    * \return The control data type
>    */
>   
> +/**
> + * \brief Retrieve the name of an enum value
> + * \return The name of the enum value
> + */
> +const std::string ControlId::enumName(int32_t value) const
> +{
> +	if (reverseMap_.find(value) != reverseMap_.end())
> +		return reverseMap_.at(value);
> +
> +	return "UNKNOWN";
> +}
> +
> +/**
> + * \fn std::map<std::string, int32_t> ControlId::nameValueMap() const
> + * \brief Retrieve the map from enum names to enum values (if applicable)
> + * \return The map from enum names to enum values
> + */
> +
>   /**
>    * \fn bool operator==(unsigned int lhs, const ControlId &rhs)
>    * \brief Compare a ControlId with a control numerical ID
> @@ -459,6 +487,7 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>    * \brief Construct a Control instance
>    * \param[in] id The control numerical ID
>    * \param[in] name The control name
> + * \param[in] nameValueMap The map from enum names to values (optional)
>    *
>    * The control data type is automatically deduced from the template type T.
>    */