[libcamera-devel] [PATCH v2 04/13] libcamera: controls: Construct from values list

[Jacopo Mondi]

Hi Kieran,

On Wed, Oct 21, 2020 at 01:50:12PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
>
> On 20/10/2020 19:05, Jacopo Mondi wrote:
> > Add a constructor to the ControlInfo class that allows creating
> > a class instance from the list of the control supported values.
> >
>
> I see.
>
> please ignore the request in my previous patch ;-) hehe
>
>
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/controls.h |  1 +
> >  src/libcamera/controls.cpp   | 17 +++++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index d1f6d4490c35..d4fdf5807f1c 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -269,6 +269,7 @@ public:
> >  			     const ControlValue &max = 0,
> >  			     const ControlValue &def = 0,
> >  			     const std::vector<ControlValue> &values = {});
> > +	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..e80f6116a684 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -493,6 +493,23 @@ ControlInfo::ControlInfo(const ControlValue &min,
> >  {
> >  }
> >
> > +/**
> > + * \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)
> > +	: values_(values)
> > +{
> > +	min_ = values_.front();
> > +	max_ = values_.back();
>
> That feels like a bit of an assumption that the list of values is sorted.
>
> Maybe there should be at least some extra checks here?
>
> Or just start with a values.sort() ?

Well, enums are defined in the .yaml file, we're in control of them,
aren't we ?
>
>
> > +	def_ = min_;
>
> And I wonder if this should be (optionally?) supplied in the constructor
> arguments?

This might be worth it, I agree.

>
> Anyway, with or without those comments:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > +}
> > +
> >  /**
> >   * \fn ControlInfo::min()
> >   * \brief Retrieve the minimum value of the control
> >
>
> --
> Regards
> --
> Kieran