[libcamera-devel] [RFC PATCH v3 01/16] controls: Add boolean constructor for ControlInfo

[Jacopo Mondi]

Hi Paul,

On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote:
> It would be convenient to be able to iterate over available boolean
> values, for example for controls that designate if some function can be
> enabled/disabled. The current min/max/def constructor is insufficient,
> as .values() is empty, so the values cannot be easily iterated over, and
> creating a Span of booleans does not work for the values constructor.
>
> Add a new constructor to ControlInfo that takes a Span of booleans (to
> allow specifiying one or two available values), and a default. The
> default value is not optional, as it doesn't make sense to have a silent
> default for boolean values.

So this is intended to be used as
        ControlInfo({true}, true);
        ControlInfo({false}, false);
        ControlInfo({true, true}, true);
        ControlInfo({true, true}, false);

?

What is the purpose of having to specify a default if only one value
is available ? Does it even make sense to have a ControlInfo that only
represents true or false ?

Can you make sure
        ControlInfo({true}, false);
doesn't happen ?

>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> New in v3
> ---
>  include/libcamera/controls.h |  1 +
>  src/libcamera/controls.cpp   | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1bc958a4..2dd147c8 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -272,6 +272,7 @@ public:
>  			     const ControlValue &def = 0);
>  	explicit ControlInfo(Span<const ControlValue> values,
>  			     const ControlValue &def = {});
> +	explicit ControlInfo(Span<const bool> values, bool def);

The explicit keyword is used to disallow implicit conversion and copy
construction. It only applies to constructors with a single parameter
afaict, so it is not required here.

>
>  	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 34317fa0..e32e22e2 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
>  		values_.push_back(value);
>  }
>
> +/**
> + * \brief Construct a ControlInfo from a list of valid boolean values
> + * \param[in] values The control valid boolean vaalues

s/vaalues/values

The values can be just {true, false}, right ?

> + * \param[in] def The control default boolean value
> + *
> + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo
> + * minimum and maximum values are set to the first and last members of the
> + * values list respectively. The default value is set to \a def.

So ControlInfo({true, false}) != ControlInfo({false, true}) ?

> + */
> +ControlInfo::ControlInfo(Span<const bool> values, bool def)
> +	: def_(def)
> +{
> +	min_ = values.front();
> +	max_ = values.back();
> +
> +	values_.reserve(2);
> +	for (const bool &value : values)
> +		values_.push_back(value);
> +}
> +

Could you point me to where this constructor is used, as it seems
suspicious to me :)


>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
> --
> 2.27.0
>