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

[Jacopo Mondi]

Hi Paul,

On Fri, Jul 16, 2021 at 07:56:11PM +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 set 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.
>
> Update the ControlInfo test accordingly.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v2:
> - use set instead of span of bools
> - add assertion to make sure that the default is a valid value
> - update the test
> ---
>  include/libcamera/controls.h   |  2 ++
>  src/libcamera/controls.cpp     | 28 ++++++++++++++++++++++++++++
>  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1bc958a4..707dc335 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -9,6 +9,7 @@
>  #define __LIBCAMERA_CONTROLS_H__
>
>  #include <assert.h>
> +#include <set>
>  #include <stdint.h>
>  #include <string>
>  #include <unordered_map>
> @@ -272,6 +273,7 @@ public:
>  			     const ControlValue &def = 0);
>  	explicit ControlInfo(Span<const ControlValue> values,
>  			     const ControlValue &def = {});
> +	explicit ControlInfo(std::set<bool> values, bool def);
>
>  	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..f6351c01 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -514,6 +514,34 @@ 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/

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

std::set is sorted, the order in which elements are listed is
irrelevant as far as I can tell (and honestly I don't know if true <
or > false :)

I would remove the ambiguities and specify that if the ControlInfo
supports both {true, false} then min = false, max = true and default
is what is passed as paramter. If the set of possible values only
contains true OR false, then min = max = def.

I think getting the terminology right would really simplify this part.
How would you call a ControlInfo that supports both {true, false} and
how would you call one that only supported {true} or {false}. binary vs
identity ?

Something like:

  Construct a ControlInfo for a boolean control. The ControlInfo can
  represent a binary (??) control that allows both true and false to be
  selected, or it can represent an identity (??) control, that only
  allows a single value.

  ControlInfo for binary controls are constructed with {true, false}
  as first parameter (the order is not relevant) \a values and the
  selected default value as provided in \a def. The minimum value will
  always be false, and the maximum one will always be true.

  ControlInfo for identity controls are constructed with a set
  containing the single supported value as first parameter \a values
  which will also match their minimum, maximum, and default values.

I know wonder if, for indentities, it wouldn't be better a constructor
as
        ControlInfo(bool)

> + */
> +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> +	: def_(def)
> +{
> +	if (values.size() == 1) {
> +		min_ = max_ = *values.begin();

Make sure def matches values[0]
You can make def default=false and if size() == 1 set it to the single
supported value, but that would still allow to write ({true}, false)
I would make two constructors to be honest.

> +	} else {
> +		min_ = false;
> +		max_ = true;
> +	}

Ah, see, the order is not relevant!

> +
> +	def_ = def;

Isn't this already assigned through the constructor initialization list ?

> +
> +	assert(values.count(def));
> +
> +	values_.reserve(2);
> +	for (const bool &value : values)
> +		values_.push_back(value);

values_ = {false, true} ?

I would

        ControlInfo::ControlInfo(std::set<bool> values, bool def)
        {
                min_ = false;
                max_ = true;
                def_ = def;
                values_ = { false, true };

                return 0;
        }

        ControlInfo::ControlInfo(bool value)
        {
                min_ = max_ = def_ = value;
                values_ = { value };

                return 0;
        }

With the first version being a syntactic sugar as 'values' is not
really required, but serves to disambiguate the two contructors and
makes it nicer to read at the expense of typing in {false, true}.

> +}
> +
>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp
> index 1e05e131..b3d2bd54 100644
> --- a/test/controls/control_info.cpp
> +++ b/test/controls/control_info.cpp
> @@ -44,6 +44,39 @@ protected:
>  			return TestFail;
>  		}
>
> +		/*
> +		 * Test information retrieval from a control with boolean
> +		 * values.
> +		 */
> +		ControlInfo aeEnable({ false, true }, false);
> +
> +		if (aeEnable.min().get<bool>() != false ||
> +		    aeEnable.def().get<bool>() != false ||
> +		    aeEnable.max().get<bool>() != true) {
> +			cout << "Invalid control range for AeEnable" << endl;
> +			return TestFail;
> +		}
> +
> +		if (aeEnable.values()[0].get<bool>() != false ||
> +		    aeEnable.values()[1].get<bool>() != true) {
> +			cout << "Invalid control values for AeEnable" << endl;
> +			return TestFail;
> +		}
> +
> +		ControlInfo awbEnable({ true }, true);
> +
> +		if (awbEnable.min().get<bool>() != true ||
> +		    awbEnable.def().get<bool>() != true ||
> +		    awbEnable.max().get<bool>() != true) {
> +			cout << "Invalid control range for AwbEnable" << endl;
> +			return TestFail;
> +		}
> +
> +		if (awbEnable.values()[0].get<bool>() != true) {
> +			cout << "Invalid control values for AwbEnable" << endl;
> +			return TestFail;
> +		}
> +
>  		return TestPass;
>  	}
>  };
> --
> 2.27.0
>