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

[Laurent Pinchart]

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 at ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo at 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
> >   */

-- 
Regards,

Laurent Pinchart