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

[Laurent Pinchart]

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.

> + * 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