[libcamera-devel] [PATCH v3 04/14] libcamera: controls: Construct from values list

[Jacopo Mondi]

Hi Laurent,

On Thu, Oct 22, 2020 at 05:12:14AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Oct 21, 2020 at 04:36:25PM +0200, Jacopo Mondi wrote:
> > Add two constructors to the ControlInfo class that allows creating
>
> s/allows/allow/
>
> > a class instance from the list of the control supported values with
>
> I'd talk about "valid values" instead of "supported values" here and in
> several other places. An enumerated control will have all supported
> values defined in the yaml file, and ControlInfo will list the valid
> values for a particular pipeline handler (or maybe 'valid' and
> 'supported' could be switched actually). The point is to explain that
> there's a difference between what the API defines and what a particular
> camera supports. This would be best explained, I think, when documenting
> the concept of enumerated controls as mentioned in the review of 03/14.
>
> > an optional default value.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/controls.h |  3 +++
> >  src/libcamera/controls.cpp   | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index d1f6d4490c35..0099b6329026 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -269,6 +269,9 @@ public:
> >  			     const ControlValue &max = 0,
> >  			     const ControlValue &def = 0,
> >  			     const std::vector<ControlValue> &values = {});
> > +	explicit ControlInfo(const std::vector<ControlValue> &values,
> > +			     const ControlValue &def);
> > +	explicit ControlInfo(const std::vector<ControlValue> &values);
> >
> >  	const ControlValue &min() const { return min_; }
> >  	const ControlValue &max() const { return max_; }
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 61feee37a1b8..389ecd5c519c 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -493,6 +493,37 @@ ControlInfo::ControlInfo(const ControlValue &min,
> >  {
> >  }
> >
> > +/**
> > + * \brief Construct a ControlInfo from the list of supported values and a default
> > + * \param[in] values The control supported values
> > + * \param[in] def The control default value
> > + *
> > + * Construct a ControlInfo from a list of supported values. The ControlInfo
> > + * minimum and maximum values are assigned to the first and last members of
>
> s/assigned/set/ (otherwise it means the opposite, .front() = min)
> s/members/entries/
>
> Same comments for the next function.
>
> I think you should mention that the values must be sorted in increasing
> order. Is it worth adding an assertion if that's not the case ? You can
> use std::is_sorted. This is a bit of a rabbit hole, as it requires
> defining operator< for ControlValue, but we can restrict the
> implementation for now to the int32 type (as that's what we use for
> enumerated controls), so it may not be too complicated.
>

As said to Kieran, we're in control of the definition of the enumerate
controls. Currently, I don't see a use case for out-of-order
enumerated values, and I would rather std::sort them if we want to be
extra paranoid, since it requires operator< anyway.o


> > + * the values list respectively.
> > + */
> > +ControlInfo::ControlInfo(const std::vector<ControlValue> &values,
> > +			 const ControlValue &def)
> > +	: def_(def), values_(values)
> > +{
> > +	min_ = values_.front();
> > +	max_ = values_.back();
> > +}
> > +
> > +/**
> > + * \brief Construct a ControlInfo from the list of supported values
> > + * \param[in] values The control supported values
> > + *
> > + * Construct a ControlInfo from a list of supported values. The ControlInfo
> > + * minimum and maximum values are assigned to the first and last members of
> > + * the values list respectively. The ControlInfo default value is set to be
> > + * equal to the minimum one.
> > + */
> > +ControlInfo::ControlInfo(const std::vector<ControlValue> &values)
> > +	: ControlInfo(values, values.front())
> > +{
> > +}
> > +
> >  /**
> >   * \fn ControlInfo::min()
> >   * \brief Retrieve the minimum value of the control
>
> --
> Regards,
>
> Laurent Pinchart