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

[Kieran Bingham]

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() ?


> +	def_ = min_;

And I wonder if this should be (optionally?) supplied in the constructor
arguments?

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