[libcamera-devel,v3,03/14] libcamera: controls: Add supported values to ControlInfo

10164 diff mbox series

Add to the ControlInfo class a list of supported values that can be
provided at construction time and retrieved through an accessor method.

This is meant to support controls that have an enumerated list of
supported values.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h |  5 ++++-
 src/libcamera/controls.cpp   | 22 +++++++++++++++++++---
 2 files changed, 23 insertions(+), 4 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Wed, Oct 21, 2020 at 04:36:24PM +0200, Jacopo Mondi wrote:
> Add to the ControlInfo class a list of supported values that can be
> provided at construction time and retrieved through an accessor method.
> 
> This is meant to support controls that have an enumerated list of
> supported values.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/controls.h |  5 ++++-
>  src/libcamera/controls.cpp   | 22 +++++++++++++++++++---
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 80944efc133a..d1f6d4490c35 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -267,11 +267,13 @@ class ControlInfo
>  public:
>  	explicit ControlInfo(const ControlValue &min = 0,
>  			     const ControlValue &max = 0,
> -			     const ControlValue &def = 0);
> +			     const ControlValue &def = 0,
> +			     const std::vector<ControlValue> &values = {});

I don't think we should add a list of values to this constructor.
Enumerated control types have their minimum and maximum implicitly
defined by the supported values. Patch 04/14 adds new constructors which
look right to me, I don't really see the use case for this one here.

And dropping this, I think you can squash 03/14 and 04/14 together.

>  
>  	const ControlValue &min() const { return min_; }
>  	const ControlValue &max() const { return max_; }
>  	const ControlValue &def() const { return def_; }
> +	const std::vector<ControlValue> &values() const { return values_; }
>  
>  	std::string toString() const;
>  
> @@ -289,6 +291,7 @@ private:
>  	ControlValue min_;
>  	ControlValue max_;
>  	ControlValue def_;
> +	std::vector<ControlValue> values_;
>  };
>  
>  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index dca782667d88..61feee37a1b8 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -479,15 +479,17 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>   */
>  
>  /**
> - * \brief Construct a ControlInfo with minimum and maximum range parameters
> + * \brief Construct a ControlInfo with parameters
>   * \param[in] min The control minimum value
>   * \param[in] max The control maximum value
>   * \param[in] def The control default value
> + * \param[in] values The control supported values
>   */
>  ControlInfo::ControlInfo(const ControlValue &min,
>  			 const ControlValue &max,
> -			 const ControlValue &def)
> -	: min_(min), max_(max), def_(def)
> +			 const ControlValue &def,
> +			 const std::vector<ControlValue> &values)
> +	: min_(min), max_(max), def_(def), values_(values)
>  {
>  }
>  
> @@ -519,6 +521,20 @@ ControlInfo::ControlInfo(const ControlValue &min,
>   * \return A ControlValue with the default value for the control
>   */
>  
> +/**
> + * \fn ControlInfo::values()
> + * \brief Retrieve the values supported by the control
> + *
> + * For controls that support a pre-defined number of values, the enumeration of
> + * those is reported through a vector of ControlValue instances accessible with
> + * this method.

Should we explicitly define a concept of enumerated controls in the
documentation ? I'm thinking it would be useful to add a flag passed to
constructors of both Control and ControlId, and recorded in ControlId,
to tell that the control is an enum.

If we define such a concept in the Control class, then this function can
just refer to it by saying it reports valid values for enumerated
controls. I think that would be better as it would expose the concept of
enumerated controls in a more visible place instead of having it more
hidden here.

I can help write documentation if needed.

> + *
> + * If the control reports a list of supported values, setting values outside
> + * of the reported ones results in undefined behaviour.

I think this belongs to Control::set().

> + *
> + * \return A vector of ControlValue instances with the supported values
> + */
> +
>  /**
>   * \brief Provide a string representation of the ControlInfo
>   */

Hi Jacopo,

Another small comment.

On Thu, Oct 22, 2020 at 04:51:46AM +0300, Laurent Pinchart wrote:
> On Wed, Oct 21, 2020 at 04:36:24PM +0200, Jacopo Mondi wrote:
> > Add to the ControlInfo class a list of supported values that can be
> > provided at construction time and retrieved through an accessor method.
> > 
> > This is meant to support controls that have an enumerated list of
> > supported values.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/controls.h |  5 ++++-
> >  src/libcamera/controls.cpp   | 22 +++++++++++++++++++---
> >  2 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 80944efc133a..d1f6d4490c35 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -267,11 +267,13 @@ class ControlInfo
> >  public:
> >  	explicit ControlInfo(const ControlValue &min = 0,
> >  			     const ControlValue &max = 0,
> > -			     const ControlValue &def = 0);
> > +			     const ControlValue &def = 0,
> > +			     const std::vector<ControlValue> &values = {});

You need to include vector at the beginning of this file. clang
complains about it.

> I don't think we should add a list of values to this constructor.
> Enumerated control types have their minimum and maximum implicitly
> defined by the supported values. Patch 04/14 adds new constructors which
> look right to me, I don't really see the use case for this one here.
> 
> And dropping this, I think you can squash 03/14 and 04/14 together.
> 
> >  
> >  	const ControlValue &min() const { return min_; }
> >  	const ControlValue &max() const { return max_; }
> >  	const ControlValue &def() const { return def_; }
> > +	const std::vector<ControlValue> &values() const { return values_; }
> >  
> >  	std::string toString() const;
> >  
> > @@ -289,6 +291,7 @@ private:
> >  	ControlValue min_;
> >  	ControlValue max_;
> >  	ControlValue def_;
> > +	std::vector<ControlValue> values_;
> >  };
> >  
> >  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index dca782667d88..61feee37a1b8 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -479,15 +479,17 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
> >   */
> >  
> >  /**
> > - * \brief Construct a ControlInfo with minimum and maximum range parameters
> > + * \brief Construct a ControlInfo with parameters
> >   * \param[in] min The control minimum value
> >   * \param[in] max The control maximum value
> >   * \param[in] def The control default value
> > + * \param[in] values The control supported values
> >   */
> >  ControlInfo::ControlInfo(const ControlValue &min,
> >  			 const ControlValue &max,
> > -			 const ControlValue &def)
> > -	: min_(min), max_(max), def_(def)
> > +			 const ControlValue &def,
> > +			 const std::vector<ControlValue> &values)
> > +	: min_(min), max_(max), def_(def), values_(values)
> >  {
> >  }
> >  
> > @@ -519,6 +521,20 @@ ControlInfo::ControlInfo(const ControlValue &min,
> >   * \return A ControlValue with the default value for the control
> >   */
> >  
> > +/**
> > + * \fn ControlInfo::values()
> > + * \brief Retrieve the values supported by the control
> > + *
> > + * For controls that support a pre-defined number of values, the enumeration of
> > + * those is reported through a vector of ControlValue instances accessible with
> > + * this method.
> 
> Should we explicitly define a concept of enumerated controls in the
> documentation ? I'm thinking it would be useful to add a flag passed to
> constructors of both Control and ControlId, and recorded in ControlId,
> to tell that the control is an enum.
> 
> If we define such a concept in the Control class, then this function can
> just refer to it by saying it reports valid values for enumerated
> controls. I think that would be better as it would expose the concept of
> enumerated controls in a more visible place instead of having it more
> hidden here.
> 
> I can help write documentation if needed.
> 
> > + *
> > + * If the control reports a list of supported values, setting values outside
> > + * of the reported ones results in undefined behaviour.
> 
> I think this belongs to Control::set().
> 
> > + *
> > + * \return A vector of ControlValue instances with the supported values
> > + */
> > +
> >  /**
> >   * \brief Provide a string representation of the ControlInfo
> >   */