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

21272 diff mbox series

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.

There already exists a map from name -> value in generated code. Reuse
this and pass it to the ControlId constructor, which in turn generates
the reverse map. The reverse map is then exposed to applications.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

---
Changes in v4:
- improve commit message
- remove forward map, as it won't be used
- replace enumToString() with simply exposing the reverse map (as a
  const map reference)
  - simplify the code accordingly

Changes in v3:
- s/nameValueMap/enumStrMap/
- s/enumName/enumToString/

No change in v2
---
 include/libcamera/controls.h     | 14 ++++++++------
 src/libcamera/control_ids.cpp.in |  4 +++-
 src/libcamera/controls.cpp       | 15 +++++++++++++++
 3 files changed, 26 insertions(+), 7 deletions(-)

Quoting Paul Elder (2024-09-16 00:24:18)
> 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.
> 
> There already exists a map from name -> value in generated code. Reuse
> this and pass it to the ControlId constructor, which in turn generates
> the reverse map. The reverse map is then exposed to applications.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> 


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
> Changes in v4:
> - improve commit message
> - remove forward map, as it won't be used
> - replace enumToString() with simply exposing the reverse map (as a
>   const map reference)
>   - simplify the code accordingly
> 
> Changes in v3:
> - s/nameValueMap/enumStrMap/
> - s/enumName/enumToString/
> 
> No change in v2
> ---
>  include/libcamera/controls.h     | 14 ++++++++------
>  src/libcamera/control_ids.cpp.in |  4 +++-
>  src/libcamera/controls.cpp       | 15 +++++++++++++++
>  3 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 7c2bb2872..96a774ccd 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,13 @@ 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> &enumStrMap = {});
>  
>         unsigned int id() const { return id_; }
>         const std::string &name() const { return name_; }
>         ControlType type() const { return type_; }
> +       const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
>  
>  private:
>         LIBCAMERA_DISABLE_COPY_AND_MOVE(ControlId)
> @@ -228,6 +228,8 @@ private:
>         unsigned int id_;
>         std::string name_;
>         ControlType type_;
> +       std::map<std::string, int32_t> enumStrMap_;
> +       std::map<int32_t, std::string> reverseMap_;
>  };
>  
>  static inline bool operator==(unsigned int lhs, const ControlId &rhs)
> @@ -256,8 +258,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> &enumStrMap = {})
> +               : ControlId(id, name, details::control_type<std::remove_cv_t<T>>::value, enumStrMap)
>         {
>         }
>  
> 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..a46c431a1 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -389,7 +389,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] enumStrMap 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> &enumStrMap)
> +       : id_(id), name_(name), type_(type), enumStrMap_(enumStrMap)
> +{
> +       for (const auto &pair : enumStrMap_)
> +               reverseMap_[pair.second] = pair.first;
> +}
>  
>  /**
>   * \fn unsigned int ControlId::id() const
> @@ -409,6 +417,12 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>   * \return The control data type
>   */
>  
> +/**
> + * \fn const std::map<int32_t, std::string> &ControlId::enumerators() const
> + * \brief Retrieve the map of enum values to enum names
> + * \return The map of enum values to enum names
> + */
> +
>  /**
>   * \fn bool operator==(unsigned int lhs, const ControlId &rhs)
>   * \brief Compare a ControlId with a control numerical ID
> @@ -459,6 +473,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] enumStrMap The map from enum names to values (optional)
>   *
>   * The control data type is automatically deduced from the template type T.
>   */
> -- 
> 2.39.2
>

Hi Paul,

Thank you for the patch.

On Mon, Sep 16, 2024 at 01:24:18AM +0200, 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.
> 
> There already exists a map from name -> value in generated code. Reuse
> this and pass it to the ControlId constructor, which in turn generates
> the reverse map. The reverse map is then exposed to applications.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 
> ---
> Changes in v4:
> - improve commit message
> - remove forward map, as it won't be used
> - replace enumToString() with simply exposing the reverse map (as a
>   const map reference)
>   - simplify the code accordingly
> 
> Changes in v3:
> - s/nameValueMap/enumStrMap/
> - s/enumName/enumToString/
> 
> No change in v2
> ---
>  include/libcamera/controls.h     | 14 ++++++++------
>  src/libcamera/control_ids.cpp.in |  4 +++-
>  src/libcamera/controls.cpp       | 15 +++++++++++++++
>  3 files changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 7c2bb2872..96a774ccd 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,13 @@ 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> &enumStrMap = {});
>  
>  	unsigned int id() const { return id_; }
>  	const std::string &name() const { return name_; }
>  	ControlType type() const { return type_; }
> +	const std::map<int32_t, std::string> &enumerators() const { return reverseMap_; }
>  
>  private:
>  	LIBCAMERA_DISABLE_COPY_AND_MOVE(ControlId)
> @@ -228,6 +228,8 @@ private:
>  	unsigned int id_;
>  	std::string name_;
>  	ControlType type_;
> +	std::map<std::string, int32_t> enumStrMap_;
> +	std::map<int32_t, std::string> reverseMap_;
>  };
>  
>  static inline bool operator==(unsigned int lhs, const ControlId &rhs)
> @@ -256,8 +258,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> &enumStrMap = {})
> +		: ControlId(id, name, details::control_type<std::remove_cv_t<T>>::value, enumStrMap)
>  	{
>  	}
>  
> 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..a46c431a1 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -389,7 +389,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] enumStrMap 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> &enumStrMap)
> +	: id_(id), name_(name), type_(type), enumStrMap_(enumStrMap)
> +{
> +	for (const auto &pair : enumStrMap_)
> +		reverseMap_[pair.second] = pair.first;
> +}
>  
>  /**
>   * \fn unsigned int ControlId::id() const
> @@ -409,6 +417,12 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>   * \return The control data type
>   */
>  
> +/**
> + * \fn const std::map<int32_t, std::string> &ControlId::enumerators() const
> + * \brief Retrieve the map of enum values to enum names
> + * \return The map of enum values to enum names
> + */
> +
>  /**
>   * \fn bool operator==(unsigned int lhs, const ControlId &rhs)
>   * \brief Compare a ControlId with a control numerical ID
> @@ -459,6 +473,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] enumStrMap The map from enum names to values (optional)
>   *
>   * The control data type is automatically deduced from the template type T.
>   */