[libcamera-devel,v5,1/9] controls: Add boolean constructors for ControlInfo

13050 diff mbox series

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 new constructors to ControlInfo that takes a set of booleans (if
both booleans are valid values) plus a default, and another that takes
only one boolean (if only one boolean is a valid value).

Update the ControlInfo test accordingly.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v5:
- break away the single-value one to a different constructor

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   |  3 +++
 src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
 test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)

Hi Paul,

On Tue, Jul 20, 2021 at 07:12:59PM +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 new constructors to ControlInfo that takes a set of booleans (if
> both booleans are valid values) plus a default, and another that takes
> only one boolean (if only one boolean is a valid value).
>
> Update the ControlInfo test accordingly.

Thank you for considering my suggestion!

>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
> ---
> Changes in v5:
> - break away the single-value one to a different constructor
>
> 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   |  3 +++
>  src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
>  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1bc958a4..de733bd8 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,8 @@ public:
>  			     const ControlValue &def = 0);
>  	explicit ControlInfo(Span<const ControlValue> values,
>  			     const ControlValue &def = {});
> +	explicit ControlInfo(std::set<bool> values, bool def);

No need for explicit if the constructor accepts two arguments

> +	explicit ControlInfo(bool value);
>
>  	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 78109f41..283472c5 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
>  		values_.push_back(value);
>  }
>
> +/**
> + * \brief Construct a boolean ControlInfo with both boolean values
> + * \param[in] values The control valid boolean values (both true and false)
> + * \param[in] def The control default boolean value

I'm still not super happy with the outcome, as the 'valid values' can
be true/false and nothing else, so passing them in makes not much
sense. But as said in the previous review that's how we could
disambiguate between a ControlInfo that supports both values and one
that only supports true or false. Unless something smarter comes to
mind, I guess we could now live with this ?

> + *
> + * Construct a ControlInfo for a boolean control, where both true and false are
> + * valid values. \a values must be { false, true } (the order is irrelevant).
> + * The minimum value will always be false, and the maximum always true. The
> + * default value is \a def.
> + */
> +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> +	: min_(false), max_(true), def_(def), values_({ false, true })
> +{
> +	assert(values.count(def) && values.size() == 2);

Be aware this will accept ControlInfo({true, true}, false);

> +}
> +
> +/**
> + * \brief Construct a boolean ControlInfo with only one valid value
> + * \param[in] value The control valid boolean value
> + *
> + * Construct a ControlInfo for a boolean control, where there is only valid
> + * value. The minimum, maximum, and default values will all be \a value.
> + */
> +ControlInfo::ControlInfo(bool value)
> +	: min_(value), max_(value), def_(value)
> +{
> +	values_ = { value };
> +}
> +
>  /**
>   * \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..2827473b 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);
> +
> +		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;
> +		}
> +

As said, not in love with this, but I don't have anything better to
suggest :)

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

>  		return TestPass;
>  	}
>  };
> --
> 2.27.0
>

Hi Jacopo,

On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Tue, Jul 20, 2021 at 07:12:59PM +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 new constructors to ControlInfo that takes a set of booleans (if
> > both booleans are valid values) plus a default, and another that takes
> > only one boolean (if only one boolean is a valid value).
> >
> > Update the ControlInfo test accordingly.
> 
> Thank you for considering my suggestion!
> 
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> >
> > ---
> > Changes in v5:
> > - break away the single-value one to a different constructor
> >
> > 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   |  3 +++
> >  src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
> >  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
> >  3 files changed, 65 insertions(+)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 1bc958a4..de733bd8 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,8 @@ public:
> >  			     const ControlValue &def = 0);
> >  	explicit ControlInfo(Span<const ControlValue> values,
> >  			     const ControlValue &def = {});
> > +	explicit ControlInfo(std::set<bool> values, bool def);
> 
> No need for explicit if the constructor accepts two arguments
> 
> > +	explicit ControlInfo(bool value);
> >
> >  	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 78109f41..283472c5 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> >  		values_.push_back(value);
> >  }
> >
> > +/**
> > + * \brief Construct a boolean ControlInfo with both boolean values
> > + * \param[in] values The control valid boolean values (both true and false)
> > + * \param[in] def The control default boolean value
> 
> I'm still not super happy with the outcome, as the 'valid values' can
> be true/false and nothing else, so passing them in makes not much
> sense. But as said in the previous review that's how we could
> disambiguate between a ControlInfo that supports both values and one
> that only supports true or false. Unless something smarter comes to

Yeah that was the idea. It's unnecessary information but it makes the
distinction clear :/

> mind, I guess we could now live with this ?
> 
> > + *
> > + * Construct a ControlInfo for a boolean control, where both true and false are
> > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > + * The minimum value will always be false, and the maximum always true. The
> > + * default value is \a def.
> > + */
> > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > +{
> > +	assert(values.count(def) && values.size() == 2);
> 
> Be aware this will accept ControlInfo({true, true}, false);

It's a std::set<bool> so if you do { true, true } then values.size()
should be 1.

At least that was my reasoning. Is it wrong?

> 
> > +}
> > +
> > +/**
> > + * \brief Construct a boolean ControlInfo with only one valid value
> > + * \param[in] value The control valid boolean value
> > + *
> > + * Construct a ControlInfo for a boolean control, where there is only valid
> > + * value. The minimum, maximum, and default values will all be \a value.
> > + */
> > +ControlInfo::ControlInfo(bool value)
> > +	: min_(value), max_(value), def_(value)
> > +{
> > +	values_ = { value };
> > +}
> > +
> >  /**
> >   * \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..2827473b 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);
> > +
> > +		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;
> > +		}
> > +
> 
> As said, not in love with this, but I don't have anything better to
> suggest :)
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Thanks,

Paul

> 
> >  		return TestPass;
> >  	}
> >  };
> > --
> > 2.27.0
> >

Hi Paul,

On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Jacopo,
>
> On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:
> > Hi Paul,
> >
> > On Tue, Jul 20, 2021 at 07:12:59PM +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 new constructors to ControlInfo that takes a set of booleans (if
> > > both booleans are valid values) plus a default, and another that takes
> > > only one boolean (if only one boolean is a valid value).
> > >
> > > Update the ControlInfo test accordingly.
> >
> > Thank you for considering my suggestion!
> >
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > Changes in v5:
> > > - break away the single-value one to a different constructor
> > >
> > > 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   |  3 +++
> > >  src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
> > >  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
> > >  3 files changed, 65 insertions(+)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 1bc958a4..de733bd8 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,8 @@ public:
> > >  			     const ControlValue &def = 0);
> > >  	explicit ControlInfo(Span<const ControlValue> values,
> > >  			     const ControlValue &def = {});
> > > +	explicit ControlInfo(std::set<bool> values, bool def);
> >
> > No need for explicit if the constructor accepts two arguments
> >
> > > +	explicit ControlInfo(bool value);
> > >
> > >  	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 78109f41..283472c5 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> > >  		values_.push_back(value);
> > >  }
> > >
> > > +/**
> > > + * \brief Construct a boolean ControlInfo with both boolean values
> > > + * \param[in] values The control valid boolean values (both true and false)
> > > + * \param[in] def The control default boolean value
> >
> > I'm still not super happy with the outcome, as the 'valid values' can
> > be true/false and nothing else, so passing them in makes not much
> > sense. But as said in the previous review that's how we could
> > disambiguate between a ControlInfo that supports both values and one
> > that only supports true or false. Unless something smarter comes to
>
> Yeah that was the idea. It's unnecessary information but it makes the
> distinction clear :/
>
> > mind, I guess we could now live with this ?
> >
> > > + *
> > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > + * The minimum value will always be false, and the maximum always true. The
> > > + * default value is \a def.
> > > + */
> > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > +{
> > > +	assert(values.count(def) && values.size() == 2);
> >
> > Be aware this will accept ControlInfo({true, true}, false);
>
> It's a std::set<bool> so if you do { true, true } then values.size()
> should be 1.
>
> At least that was my reasoning. Is it wrong?
>

Oh you're right, I've overlooked the container type! Thanks for
the clarification!


> >
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > + * \param[in] value The control valid boolean value
> > > + *
> > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > + * value. The minimum, maximum, and default values will all be \a value.
> > > + */
> > > +ControlInfo::ControlInfo(bool value)
> > > +	: min_(value), max_(value), def_(value)
> > > +{
> > > +	values_ = { value };
> > > +}
> > > +
> > >  /**
> > >   * \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..2827473b 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);
> > > +
> > > +		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;
> > > +		}
> > > +
> >
> > As said, not in love with this, but I don't have anything better to
> > suggest :)
> >
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
>
> Thanks,
>
> Paul
>
> >
> > >  		return TestPass;
> > >  	}
> > >  };
> > > --
> > > 2.27.0
> > >

Hi Paul,

Thank you for the patch.

On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:
> On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:
> > On Tue, Jul 20, 2021 at 07:12:59PM +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 new constructors to ControlInfo that takes a set of booleans (if
> > > both booleans are valid values) plus a default, and another that takes
> > > only one boolean (if only one boolean is a valid value).
> > >
> > > Update the ControlInfo test accordingly.
> > 
> > Thank you for considering my suggestion!
> > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > ---
> > > Changes in v5:
> > > - break away the single-value one to a different constructor
> > >
> > > 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   |  3 +++
> > >  src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
> > >  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
> > >  3 files changed, 65 insertions(+)
> > >
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 1bc958a4..de733bd8 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,8 @@ public:
> > >  			     const ControlValue &def = 0);
> > >  	explicit ControlInfo(Span<const ControlValue> values,
> > >  			     const ControlValue &def = {});
> > > +	explicit ControlInfo(std::set<bool> values, bool def);
> > 
> > No need for explicit if the constructor accepts two arguments
> > 
> > > +	explicit ControlInfo(bool value);
> > >
> > >  	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 78109f41..283472c5 100644
> > > --- a/src/libcamera/controls.cpp
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> > >  		values_.push_back(value);
> > >  }
> > >
> > > +/**
> > > + * \brief Construct a boolean ControlInfo with both boolean values
> > > + * \param[in] values The control valid boolean values (both true and false)
> > > + * \param[in] def The control default boolean value
> > 
> > I'm still not super happy with the outcome, as the 'valid values' can
> > be true/false and nothing else, so passing them in makes not much
> > sense. But as said in the previous review that's how we could
> > disambiguate between a ControlInfo that supports both values and one
> > that only supports true or false. Unless something smarter comes to
> 
> Yeah that was the idea. It's unnecessary information but it makes the
> distinction clear :/

So we all agree it's not great. The question is how it could be better
:-)

Turning brainstorming mode on.

We have two types of integer-based controls, numerical and enumerated.
The numerical controls have a minimum, maximum and default, while the
enumerated controls have a list of possible values and a default.
Constructing a ControlInfo for enumerated controls requires providing a
span that contains all the possible values.

When generating control_ids.h from control_ids.yaml, the python script
creates, for each enumerated control, an array containing all possible
values. This can be used as an argument to the ControlInfo constructor,
as a span can be implicitly constructed from an std::array:

	ControlInfo info{ controls::AeExposureModeValues };

If we were to create a ControlInfo that only supports a subset of those
modes, however, the following syntax, would result in a compilation
error:

	ControlInfo info{ {
		controls::ExposureNormal,
		controls::ExposureShort,
		controls::ExposureLong,
	} };

To make it compile, we would need to write

	ControlInfo info{ std::array<ControlValue, 4>{
		static_cast<int32_t>(controls::ExposureNormal),
		static_cast<int32_t>(controls::ExposureShort),
		static_cast<int32_t>(controls::ExposureLong),
	} };

which isn't exactly great.

The bools fall in the same category,

	ControlInfo info{ { false, true }, true };

won't compile, while

	ControlInfo info{ std::array<ControlValue, 2>{ false, true }, true };

would. Arguably not the greatest either.

If we want to improve this, the first question we should ask ourselves
is if a nicer syntax common to all enumerated controls is achievable.
For instance, we could perhaps define the following constructor:

	template<typename T>
	ControlInfo(std::initializer_list<T> values, T def);

With appropriate implementations for the types we want to support. This
should work for bools, allowing us to write

	ControlInfo info{ { false, true }, true };

but won't work nicely for AeExposureMode. Due to the values being
enumerated, the compiler will want and implementation for

	ControlInfo::ControlInfo<controls::AeExposureModeEnum>(std::initializer_list<controls::AeExposureModeEnum>,
							       controls::AeExposureModeEnum)

and not

	ControlInfo::ControlInfo<int32_t(std::initializer_list<int32_t>, int32_t)

Maybe we could work around that by creating an inline constructor that
will only match enumerated types (using std::enable_if) and casting to
int32_t. I'm not sure if

	static_cast<std::initializer_list<int32_t>>(values)

would work though, I have a feeling it won't be that easy.

I'm sure other options may be possible too.

Maybe a nice solution for this doesn't exist, in which case we could
focus on bools only. In that case, I see four different cases for a
ControlInfo related to bools:

- values = { false, true }, default = false
- values = { false, true }, default = true
- values = { false }, default = false
- values = { true }, default = true

Anything else is invalid. Could we improve this patch by catching more
invalid combinations are compile time ? One of the things that bother me
is the increase in the number of constructors, which could create
ambiguous constructs due to the implicit constructors for ControlValue.

The following would be one way to add a bool-specific constructor that
will have no risk of clashing with anything in the future:

	enum Mutable {
		ReadOnly,
		ReadWrite,
	};

	ControlInfo(bool default, Mutable mutable);

(the Mutable, ReadOnly and ReadWrite names can most probably be
improved). One would use this, mapping to the four cases above, as

	ControlInfo(false, ControlInfo::ReadWrite);
	ControlInfo(true, ControlInfo::ReadWrite);
	ControlInfo(false, ControlInfo::ReadOnly);
	ControlInfo(true, ControlInfo::ReadOnly);

Maybe writing it differently would be more explicit:

	ControlInfo(ControlInfo::MutableBool, false);
	ControlInfo(ControlInfo::MutableBool, true);
	ControlInfo(ControlInfo::ImmutableBool, false);
	ControlInfo(ControlInfo::ImmutableBool, true);

Now that I've started the brainstorming, I'll let you unleash your brain
power to shoot this down or improve it :-)

> > mind, I guess we could now live with this ?
> > 
> > > + *
> > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > + * The minimum value will always be false, and the maximum always true. The
> > > + * default value is \a def.
> > > + */
> > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > +{
> > > +	assert(values.count(def) && values.size() == 2);
> > 
> > Be aware this will accept ControlInfo({true, true}, false);
> 
> It's a std::set<bool> so if you do { true, true } then values.size()
> should be 1.
> 
> At least that was my reasoning. Is it wrong?
> 
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > + * \param[in] value The control valid boolean value
> > > + *
> > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > + * value. The minimum, maximum, and default values will all be \a value.
> > > + */
> > > +ControlInfo::ControlInfo(bool value)
> > > +	: min_(value), max_(value), def_(value)
> > > +{
> > > +	values_ = { value };
> > > +}
> > > +
> > >  /**
> > >   * \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..2827473b 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);
> > > +
> > > +		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;
> > > +		}
> > > +
> > 
> > As said, not in love with this, but I don't have anything better to
> > suggest :)
> > 
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > >  		return TestPass;
> > >  	}
> > >  };

Hi Laurent,

On Sun, Jul 25, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:
> > On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:
> > > On Tue, Jul 20, 2021 at 07:12:59PM +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 new constructors to ControlInfo that takes a set of booleans (if
> > > > both booleans are valid values) plus a default, and another that takes
> > > > only one boolean (if only one boolean is a valid value).
> > > >
> > > > Update the ControlInfo test accordingly.
> > >
> > > Thank you for considering my suggestion!
> > >
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > >
> > > > ---
> > > > Changes in v5:
> > > > - break away the single-value one to a different constructor
> > > >
> > > > 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   |  3 +++
> > > >  src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
> > > >  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
> > > >  3 files changed, 65 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > index 1bc958a4..de733bd8 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,8 @@ public:
> > > >  			     const ControlValue &def = 0);
> > > >  	explicit ControlInfo(Span<const ControlValue> values,
> > > >  			     const ControlValue &def = {});
> > > > +	explicit ControlInfo(std::set<bool> values, bool def);
> > >
> > > No need for explicit if the constructor accepts two arguments
> > >
> > > > +	explicit ControlInfo(bool value);
> > > >
> > > >  	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 78109f41..283472c5 100644
> > > > --- a/src/libcamera/controls.cpp
> > > > +++ b/src/libcamera/controls.cpp
> > > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> > > >  		values_.push_back(value);
> > > >  }
> > > >
> > > > +/**
> > > > + * \brief Construct a boolean ControlInfo with both boolean values
> > > > + * \param[in] values The control valid boolean values (both true and false)
> > > > + * \param[in] def The control default boolean value
> > >
> > > I'm still not super happy with the outcome, as the 'valid values' can
> > > be true/false and nothing else, so passing them in makes not much
> > > sense. But as said in the previous review that's how we could
> > > disambiguate between a ControlInfo that supports both values and one
> > > that only supports true or false. Unless something smarter comes to
> >
> > Yeah that was the idea. It's unnecessary information but it makes the
> > distinction clear :/
>
> So we all agree it's not great. The question is how it could be better
> :-)
>
> Turning brainstorming mode on.
>
> We have two types of integer-based controls, numerical and enumerated.
> The numerical controls have a minimum, maximum and default, while the
> enumerated controls have a list of possible values and a default.
> Constructing a ControlInfo for enumerated controls requires providing a
> span that contains all the possible values.
>
> When generating control_ids.h from control_ids.yaml, the python script
> creates, for each enumerated control, an array containing all possible
> values. This can be used as an argument to the ControlInfo constructor,
> as a span can be implicitly constructed from an std::array:
>
> 	ControlInfo info{ controls::AeExposureModeValues };
>
> If we were to create a ControlInfo that only supports a subset of those
> modes, however, the following syntax, would result in a compilation
> error:
>
> 	ControlInfo info{ {
> 		controls::ExposureNormal,
> 		controls::ExposureShort,
> 		controls::ExposureLong,
> 	} };
>
> To make it compile, we would need to write
>
> 	ControlInfo info{ std::array<ControlValue, 4>{
> 		static_cast<int32_t>(controls::ExposureNormal),
> 		static_cast<int32_t>(controls::ExposureShort),
> 		static_cast<int32_t>(controls::ExposureLong),
> 	} };
>
> which isn't exactly great.
>
> The bools fall in the same category,
>
> 	ControlInfo info{ { false, true }, true };

I appreciate the thoughtful analysis of the issue, but my concern was
different and could be expressed as:
 - For a boolean control info, does it make sense to pass in
   {true, false} as possible values as
   - The order is not relevant
   - The only values a boolean control info can support are, by
     definition, true or false

All of this assuming a boolean control info that only support true OR
false is constructed by passing in the single supported value.

Even if we make the constructors nicer, my issue was conceptually on
the requirement to pass {false, true} in, even if that's not required
by definition


>
> won't compile, while
>
> 	ControlInfo info{ std::array<ControlValue, 2>{ false, true }, true };
>
> would. Arguably not the greatest either.
>
> If we want to improve this, the first question we should ask ourselves
> is if a nicer syntax common to all enumerated controls is achievable.
> For instance, we could perhaps define the following constructor:
>
> 	template<typename T>
> 	ControlInfo(std::initializer_list<T> values, T def);
>
> With appropriate implementations for the types we want to support. This
> should work for bools, allowing us to write
>
> 	ControlInfo info{ { false, true }, true };

This is possible today with the simple constructor added by Paul, but
does not solve the issue with the two values being passed in

>
> but won't work nicely for AeExposureMode. Due to the values being
> enumerated, the compiler will want and implementation for
>
> 	ControlInfo::ControlInfo<controls::AeExposureModeEnum>(std::initializer_list<controls::AeExposureModeEnum>,
> 							       controls::AeExposureModeEnum)
>
> and not
>
> 	ControlInfo::ControlInfo<int32_t(std::initializer_list<int32_t>, int32_t)
>
> Maybe we could work around that by creating an inline constructor that
> will only match enumerated types (using std::enable_if) and casting to
> int32_t. I'm not sure if
>
> 	static_cast<std::initializer_list<int32_t>>(values)
>
> would work though, I have a feeling it won't be that easy.
>
> I'm sure other options may be possible too.
>
> Maybe a nice solution for this doesn't exist, in which case we could
> focus on bools only. In that case, I see four different cases for a
> ControlInfo related to bools:
>
> - values = { false, true }, default = false
> - values = { false, true }, default = true
> - values = { false }, default = false
> - values = { true }, default = true
>
> Anything else is invalid. Could we improve this patch by catching more
> invalid combinations are compile time ? One of the things that bother me

I think this version only accepts the 4 of them, according to Paul's
reply to my comment that ({false, false}, true) was possible.

It currently goes through two constructors, one for 'mutable' booleans
control info:
        ControlInfo::ControlInfo(std::set<bool> values, bool def)

As this is an std::set, {false, false} or {true, true} will be refused
by the assertion
        assert(values.count(def) && values.size() == 2);

 (note: asserting for values.size() == 2 is probably enough)

And one for 'non mutable' ones
        ControlInfo::ControlInfo(bool value)

Which was suggested as it reduces the possibility of mis-use as
        ControlInfo( {false}, true)
is not possible with this constructor

All in all I think this version is safe, I'm just bothered by the fact
{true, false} have to passed in when those are implicitly the only
values supported by a boolean control info

> is the increase in the number of constructors, which could create
> ambiguous constructs due to the implicit constructors for ControlValue.

That's a reasonable concern

>
> The following would be one way to add a bool-specific constructor that
> will have no risk of clashing with anything in the future:
>
> 	enum Mutable {
> 		ReadOnly,
> 		ReadWrite,
> 	};
>
> 	ControlInfo(bool default, Mutable mutable);
>
> (the Mutable, ReadOnly and ReadWrite names can most probably be
> improved). One would use this, mapping to the four cases above, as
>
> 	ControlInfo(false, ControlInfo::ReadWrite);
> 	ControlInfo(true, ControlInfo::ReadWrite);
> 	ControlInfo(false, ControlInfo::ReadOnly);
> 	ControlInfo(true, ControlInfo::ReadOnly);
>
> Maybe writing it differently would be more explicit:
>
> 	ControlInfo(ControlInfo::MutableBool, false);
> 	ControlInfo(ControlInfo::MutableBool, true);
> 	ControlInfo(ControlInfo::ImmutableBool, false);
> 	ControlInfo(ControlInfo::ImmutableBool, true);

Ah, this could solve the concerns I expressed above.

We have the general need to express when a Control is RO or RW, don't
we ? We could add an rw_ flag to the ControlInfo class for that and
derive a read-only variant maybe ?

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index 1bc958a43b22..5ef84a92f219 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -272,11 +272,16 @@ public:
                             const ControlValue &def = 0);
        explicit ControlInfo(Span<const ControlValue> values,
                             const ControlValue &def = {});
+       explicit ControlInfo(bool value)
+               : min_(false), max_(true), def_(value)
+       {
+       }

        const ControlValue &min() const { return min_; }
        const ControlValue &max() const { return max_; }
        const ControlValue &def() const { return def_; }
        const std::vector<ControlValue> &values() const { return values_; }
+       bool rw() const { return rw_; }

        std::string toString() const;

@@ -290,13 +295,41 @@ public:
                return !(*this == other);
        }

-private:
+protected:
+       bool rw_ : true;
        ControlValue min_;
        ControlValue max_;
        ControlValue def_;
        std::vector<ControlValue> values_;
 };

+class ROControlInfo : public ControlInfo
+{
+       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
+
+       explicit ROControlInfo(const ControlValue &min = 0,
+                              const ControlValue &max = 0,
+                              const ControlValue &def = 0)
+               : ControlInfo(min, max, def)
+       {
+               rw_ = false;
+       }
+
+       explicit ROControlInfo(Span<const ControlValue> values,
+                              const ControlValue &def = {})
+               : ControlInfo(values, def)
+       {
+               rw_ = false;
+       }
+
+       explicit ROControlInfo(bool value)
+       {
+               min_ = max_ = def_ = value;
+               rw_ = false;
+       }
+};
+

I'm sure it could be done in a smarter way than this, I'm just wonder
if it's worth it and IPA/ph should be in charge of deciding what type
of ControlInfo to construct. For the generator, once we have a
read_only flag, this should be trivial instead.

>
> Now that I've started the brainstorming, I'll let you unleash your brain
> power to shoot this down or improve it :-)
>
> > > mind, I guess we could now live with this ?
> > >
> > > > + *
> > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > + * The minimum value will always be false, and the maximum always true. The
> > > > + * default value is \a def.
> > > > + */
> > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > +{
> > > > +	assert(values.count(def) && values.size() == 2);
> > >
> > > Be aware this will accept ControlInfo({true, true}, false);
> >
> > It's a std::set<bool> so if you do { true, true } then values.size()
> > should be 1.
> >
> > At least that was my reasoning. Is it wrong?
> >
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > + * \param[in] value The control valid boolean value
> > > > + *
> > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > + */
> > > > +ControlInfo::ControlInfo(bool value)
> > > > +	: min_(value), max_(value), def_(value)
> > > > +{
> > > > +	values_ = { value };
> > > > +}
> > > > +
> > > >  /**
> > > >   * \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..2827473b 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);
> > > > +
> > > > +		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;
> > > > +		}
> > > > +
> > >
> > > As said, not in love with this, but I don't have anything better to
> > > suggest :)
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > >
> > > >  		return TestPass;
> > > >  	}
> > > >  };
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Mon, Jul 26, 2021 at 04:08:07PM +0200, Jacopo Mondi wrote:
> On Sun, Jul 25, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote:
> > On Fri, Jul 23, 2021 at 02:45:52PM +0900, paul.elder@ideasonboard.com wrote:
> > > On Thu, Jul 22, 2021 at 03:06:09PM +0200, Jacopo Mondi wrote:
> > > > On Tue, Jul 20, 2021 at 07:12:59PM +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 new constructors to ControlInfo that takes a set of booleans (if
> > > > > both booleans are valid values) plus a default, and another that takes
> > > > > only one boolean (if only one boolean is a valid value).
> > > > >
> > > > > Update the ControlInfo test accordingly.
> > > >
> > > > Thank you for considering my suggestion!
> > > >
> > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > >
> > > > > ---
> > > > > Changes in v5:
> > > > > - break away the single-value one to a different constructor
> > > > >
> > > > > 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   |  3 +++
> > > > >  src/libcamera/controls.cpp     | 29 +++++++++++++++++++++++++++++
> > > > >  test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 65 insertions(+)
> > > > >
> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > > index 1bc958a4..de733bd8 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,8 @@ public:
> > > > >  			     const ControlValue &def = 0);
> > > > >  	explicit ControlInfo(Span<const ControlValue> values,
> > > > >  			     const ControlValue &def = {});
> > > > > +	explicit ControlInfo(std::set<bool> values, bool def);
> > > >
> > > > No need for explicit if the constructor accepts two arguments
> > > >
> > > > > +	explicit ControlInfo(bool value);
> > > > >
> > > > >  	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 78109f41..283472c5 100644
> > > > > --- a/src/libcamera/controls.cpp
> > > > > +++ b/src/libcamera/controls.cpp
> > > > > @@ -514,6 +514,35 @@ ControlInfo::ControlInfo(Span<const ControlValue> values,
> > > > >  		values_.push_back(value);
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * \brief Construct a boolean ControlInfo with both boolean values
> > > > > + * \param[in] values The control valid boolean values (both true and false)
> > > > > + * \param[in] def The control default boolean value
> > > >
> > > > I'm still not super happy with the outcome, as the 'valid values' can
> > > > be true/false and nothing else, so passing them in makes not much
> > > > sense. But as said in the previous review that's how we could
> > > > disambiguate between a ControlInfo that supports both values and one
> > > > that only supports true or false. Unless something smarter comes to
> > >
> > > Yeah that was the idea. It's unnecessary information but it makes the
> > > distinction clear :/
> >
> > So we all agree it's not great. The question is how it could be better
> > :-)
> >
> > Turning brainstorming mode on.
> >
> > We have two types of integer-based controls, numerical and enumerated.
> > The numerical controls have a minimum, maximum and default, while the
> > enumerated controls have a list of possible values and a default.
> > Constructing a ControlInfo for enumerated controls requires providing a
> > span that contains all the possible values.
> >
> > When generating control_ids.h from control_ids.yaml, the python script
> > creates, for each enumerated control, an array containing all possible
> > values. This can be used as an argument to the ControlInfo constructor,
> > as a span can be implicitly constructed from an std::array:
> >
> > 	ControlInfo info{ controls::AeExposureModeValues };
> >
> > If we were to create a ControlInfo that only supports a subset of those
> > modes, however, the following syntax, would result in a compilation
> > error:
> >
> > 	ControlInfo info{ {
> > 		controls::ExposureNormal,
> > 		controls::ExposureShort,
> > 		controls::ExposureLong,
> > 	} };
> >
> > To make it compile, we would need to write
> >
> > 	ControlInfo info{ std::array<ControlValue, 4>{
> > 		static_cast<int32_t>(controls::ExposureNormal),
> > 		static_cast<int32_t>(controls::ExposureShort),
> > 		static_cast<int32_t>(controls::ExposureLong),
> > 	} };
> >
> > which isn't exactly great.
> >
> > The bools fall in the same category,
> >
> > 	ControlInfo info{ { false, true }, true };
> 
> I appreciate the thoughtful analysis of the issue, but my concern was
> different and could be expressed as:
>  - For a boolean control info, does it make sense to pass in
>    {true, false} as possible values as
>    - The order is not relevant
>    - The only values a boolean control info can support are, by
>      definition, true or false

Agreed, hence the alternative, bool-only proposal below :-) The downside
is that it won't help us with enum-based controls, but maybe that can be
addressed separately.

> All of this assuming a boolean control info that only support true OR
> false is constructed by passing in the single supported value.
> 
> Even if we make the constructors nicer, my issue was conceptually on
> the requirement to pass {false, true} in, even if that's not required
> by definition
> 
> > won't compile, while
> >
> > 	ControlInfo info{ std::array<ControlValue, 2>{ false, true }, true };
> >
> > would. Arguably not the greatest either.
> >
> > If we want to improve this, the first question we should ask ourselves
> > is if a nicer syntax common to all enumerated controls is achievable.
> > For instance, we could perhaps define the following constructor:
> >
> > 	template<typename T>
> > 	ControlInfo(std::initializer_list<T> values, T def);
> >
> > With appropriate implementations for the types we want to support. This
> > should work for bools, allowing us to write
> >
> > 	ControlInfo info{ { false, true }, true };
> 
> This is possible today with the simple constructor added by Paul, but
> does not solve the issue with the two values being passed in
> 
> > but won't work nicely for AeExposureMode. Due to the values being
> > enumerated, the compiler will want and implementation for
> >
> > 	ControlInfo::ControlInfo<controls::AeExposureModeEnum>(std::initializer_list<controls::AeExposureModeEnum>,
> > 							       controls::AeExposureModeEnum)
> >
> > and not
> >
> > 	ControlInfo::ControlInfo<int32_t(std::initializer_list<int32_t>, int32_t)
> >
> > Maybe we could work around that by creating an inline constructor that
> > will only match enumerated types (using std::enable_if) and casting to
> > int32_t. I'm not sure if
> >
> > 	static_cast<std::initializer_list<int32_t>>(values)
> >
> > would work though, I have a feeling it won't be that easy.
> >
> > I'm sure other options may be possible too.

If someone has a great idea that would address both the boolean and
enumerated controls, I'm still all ears :-)

> > Maybe a nice solution for this doesn't exist, in which case we could
> > focus on bools only. In that case, I see four different cases for a
> > ControlInfo related to bools:
> >
> > - values = { false, true }, default = false
> > - values = { false, true }, default = true
> > - values = { false }, default = false
> > - values = { true }, default = true
> >
> > Anything else is invalid. Could we improve this patch by catching more
> > invalid combinations are compile time ? One of the things that bother me
> 
> I think this version only accepts the 4 of them, according to Paul's
> reply to my comment that ({false, false}, true) was possible.
> 
> It currently goes through two constructors, one for 'mutable' booleans
> control info:
>         ControlInfo::ControlInfo(std::set<bool> values, bool def)
> 
> As this is an std::set, {false, false} or {true, true} will be refused
> by the assertion
>         assert(values.count(def) && values.size() == 2);
> 
>  (note: asserting for values.size() == 2 is probably enough)
> 
> And one for 'non mutable' ones
>         ControlInfo::ControlInfo(bool value)
> 
> Which was suggested as it reduces the possibility of mis-use as
>         ControlInfo( {false}, true)
> is not possible with this constructor
> 
> All in all I think this version is safe, I'm just bothered by the fact
> {true, false} have to passed in when those are implicitly the only
> values supported by a boolean control info
> 
> > is the increase in the number of constructors, which could create
> > ambiguous constructs due to the implicit constructors for ControlValue.
> 
> That's a reasonable concern
> 
> > The following would be one way to add a bool-specific constructor that
> > will have no risk of clashing with anything in the future:
> >
> > 	enum Mutable {
> > 		ReadOnly,
> > 		ReadWrite,
> > 	};
> >
> > 	ControlInfo(bool default, Mutable mutable);
> >
> > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > improved). One would use this, mapping to the four cases above, as
> >
> > 	ControlInfo(false, ControlInfo::ReadWrite);
> > 	ControlInfo(true, ControlInfo::ReadWrite);
> > 	ControlInfo(false, ControlInfo::ReadOnly);
> > 	ControlInfo(true, ControlInfo::ReadOnly);
> >
> > Maybe writing it differently would be more explicit:
> >
> > 	ControlInfo(ControlInfo::MutableBool, false);
> > 	ControlInfo(ControlInfo::MutableBool, true);
> > 	ControlInfo(ControlInfo::ImmutableBool, false);
> > 	ControlInfo(ControlInfo::ImmutableBool, true);
> 
> Ah, this could solve the concerns I expressed above.
> 
> We have the general need to express when a Control is RO or RW, don't
> we ? We could add an rw_ flag to the ControlInfo class for that and
> derive a read-only variant maybe ?

Thinking more about this, the read-only concept bothers me a bit. A
read-only control is essentially either a property or a metadata in the
general case (depending on whether the value is static or dynamic). What
we're dealing with here is a control that is meant to be writable, but
happens to only have a single supported value.

Taking one step back, what are our use cases for boolean controls that
accept a single value ?

> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 1bc958a43b22..5ef84a92f219 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -272,11 +272,16 @@ public:
>                              const ControlValue &def = 0);
>         explicit ControlInfo(Span<const ControlValue> values,
>                              const ControlValue &def = {});
> +       explicit ControlInfo(bool value)
> +               : min_(false), max_(true), def_(value)
> +       {
> +       }
> 
>         const ControlValue &min() const { return min_; }
>         const ControlValue &max() const { return max_; }
>         const ControlValue &def() const { return def_; }
>         const std::vector<ControlValue> &values() const { return values_; }
> +       bool rw() const { return rw_; }
> 
>         std::string toString() const;
> 
> @@ -290,13 +295,41 @@ public:
>                 return !(*this == other);
>         }
> 
> -private:
> +protected:
> +       bool rw_ : true;
>         ControlValue min_;
>         ControlValue max_;
>         ControlValue def_;
>         std::vector<ControlValue> values_;
>  };
> 
> +class ROControlInfo : public ControlInfo
> +{
> +       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> +
> +       explicit ROControlInfo(const ControlValue &min = 0,
> +                              const ControlValue &max = 0,
> +                              const ControlValue &def = 0)
> +               : ControlInfo(min, max, def)
> +       {
> +               rw_ = false;
> +       }
> +
> +       explicit ROControlInfo(Span<const ControlValue> values,
> +                              const ControlValue &def = {})
> +               : ControlInfo(values, def)
> +       {
> +               rw_ = false;
> +       }
> +
> +       explicit ROControlInfo(bool value)
> +       {
> +               min_ = max_ = def_ = value;
> +               rw_ = false;
> +       }
> +};
> +
> 
> I'm sure it could be done in a smarter way than this, I'm just wonder
> if it's worth it and IPA/ph should be in charge of deciding what type
> of ControlInfo to construct. For the generator, once we have a
> read_only flag, this should be trivial instead.
> 
> > Now that I've started the brainstorming, I'll let you unleash your brain
> > power to shoot this down or improve it :-)
> >
> > > > mind, I guess we could now live with this ?
> > > >
> > > > > + *
> > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > + * default value is \a def.
> > > > > + */
> > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > > +{
> > > > > +	assert(values.count(def) && values.size() == 2);
> > > >
> > > > Be aware this will accept ControlInfo({true, true}, false);
> > >
> > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > should be 1.
> > >
> > > At least that was my reasoning. Is it wrong?
> > >
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > + * \param[in] value The control valid boolean value
> > > > > + *
> > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > + */
> > > > > +ControlInfo::ControlInfo(bool value)
> > > > > +	: min_(value), max_(value), def_(value)
> > > > > +{
> > > > > +	values_ = { value };
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * \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..2827473b 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);
> > > > > +
> > > > > +		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;
> > > > > +		}
> > > > > +
> > > >
> > > > As said, not in love with this, but I don't have anything better to
> > > > suggest :)
> > > >
> > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >
> > > > >  		return TestPass;
> > > > >  	}
> > > > >  };

Hi Laurent,

On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,

[snip]

> > > The following would be one way to add a bool-specific constructor that
> > > will have no risk of clashing with anything in the future:
> > >
> > > 	enum Mutable {
> > > 		ReadOnly,
> > > 		ReadWrite,
> > > 	};
> > >
> > > 	ControlInfo(bool default, Mutable mutable);
> > >
> > > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > > improved). One would use this, mapping to the four cases above, as
> > >
> > > 	ControlInfo(false, ControlInfo::ReadWrite);
> > > 	ControlInfo(true, ControlInfo::ReadWrite);
> > > 	ControlInfo(false, ControlInfo::ReadOnly);
> > > 	ControlInfo(true, ControlInfo::ReadOnly);
> > >
> > > Maybe writing it differently would be more explicit:
> > >
> > > 	ControlInfo(ControlInfo::MutableBool, false);
> > > 	ControlInfo(ControlInfo::MutableBool, true);
> > > 	ControlInfo(ControlInfo::ImmutableBool, false);
> > > 	ControlInfo(ControlInfo::ImmutableBool, true);
> >
> > Ah, this could solve the concerns I expressed above.
> >
> > We have the general need to express when a Control is RO or RW, don't
> > we ? We could add an rw_ flag to the ControlInfo class for that and
> > derive a read-only variant maybe ?
>
> Thinking more about this, the read-only concept bothers me a bit. A
> read-only control is essentially either a property or a metadata in the
> general case (depending on whether the value is static or dynamic). What
> we're dealing with here is a control that is meant to be writable, but
> happens to only have a single supported value.

This has bothered me all the time I tried to find a good term to
identify those "booleans with a single value". I proposed 'identity'
but that's not really up to the point. The fact I can't find a term
for that makes me think I'm missing something and we should probably
be doing something different. Hence the proposal to have a dedicated
constructor, but yes, more constructors are ambiguos.

>
> Taking one step back, what are our use cases for boolean controls that
> accept a single value ?
>

I think the original intent was to be able to express things like: the
IPA only supports AE_ENABLE, so that you register a

        {controls::AeEnable, ControlInfo(true) }

While if you can enable/disable it, and it's enabled by default:

        {controls::AeEnable, ControlInfo({true, false}, true) }

I would now suggest to simply not register the control at all if it's
not mutable, but I'm sure right now I'm missing why this was not
possible in first place. Maybe it's just a matter of defining a policy
like this one: if a control (not a property, nor a metadata) cannot be
changed, just don't register it as part of the Camera::controls" ?

Thanks
  j


> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 1bc958a43b22..5ef84a92f219 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -272,11 +272,16 @@ public:
> >                              const ControlValue &def = 0);
> >         explicit ControlInfo(Span<const ControlValue> values,
> >                              const ControlValue &def = {});
> > +       explicit ControlInfo(bool value)
> > +               : min_(false), max_(true), def_(value)
> > +       {
> > +       }
> >
> >         const ControlValue &min() const { return min_; }
> >         const ControlValue &max() const { return max_; }
> >         const ControlValue &def() const { return def_; }
> >         const std::vector<ControlValue> &values() const { return values_; }
> > +       bool rw() const { return rw_; }
> >
> >         std::string toString() const;
> >
> > @@ -290,13 +295,41 @@ public:
> >                 return !(*this == other);
> >         }
> >
> > -private:
> > +protected:
> > +       bool rw_ : true;
> >         ControlValue min_;
> >         ControlValue max_;
> >         ControlValue def_;
> >         std::vector<ControlValue> values_;
> >  };
> >
> > +class ROControlInfo : public ControlInfo
> > +{
> > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> > +
> > +       explicit ROControlInfo(const ControlValue &min = 0,
> > +                              const ControlValue &max = 0,
> > +                              const ControlValue &def = 0)
> > +               : ControlInfo(min, max, def)
> > +       {
> > +               rw_ = false;
> > +       }
> > +
> > +       explicit ROControlInfo(Span<const ControlValue> values,
> > +                              const ControlValue &def = {})
> > +               : ControlInfo(values, def)
> > +       {
> > +               rw_ = false;
> > +       }
> > +
> > +       explicit ROControlInfo(bool value)
> > +       {
> > +               min_ = max_ = def_ = value;
> > +               rw_ = false;
> > +       }
> > +};
> > +
> >
> > I'm sure it could be done in a smarter way than this, I'm just wonder
> > if it's worth it and IPA/ph should be in charge of deciding what type
> > of ControlInfo to construct. For the generator, once we have a
> > read_only flag, this should be trivial instead.
> >
> > > Now that I've started the brainstorming, I'll let you unleash your brain
> > > power to shoot this down or improve it :-)
> > >
> > > > > mind, I guess we could now live with this ?
> > > > >
> > > > > > + *
> > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > > + * default value is \a def.
> > > > > > + */
> > > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > > > +{
> > > > > > +	assert(values.count(def) && values.size() == 2);
> > > > >
> > > > > Be aware this will accept ControlInfo({true, true}, false);
> > > >
> > > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > > should be 1.
> > > >
> > > > At least that was my reasoning. Is it wrong?
> > > >
> > > > > > +}
> > > > > > +
> > > > > > +/**
> > > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > > + * \param[in] value The control valid boolean value
> > > > > > + *
> > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > > + */
> > > > > > +ControlInfo::ControlInfo(bool value)
> > > > > > +	: min_(value), max_(value), def_(value)
> > > > > > +{
> > > > > > +	values_ = { value };
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * \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..2827473b 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);
> > > > > > +
> > > > > > +		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;
> > > > > > +		}
> > > > > > +
> > > > >
> > > > > As said, not in love with this, but I don't have anything better to
> > > > > suggest :)
> > > > >
> > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > >
> > > > > >  		return TestPass;
> > > > > >  	}
> > > > > >  };
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> 
> [snip]
> 
> > > > The following would be one way to add a bool-specific constructor that
> > > > will have no risk of clashing with anything in the future:
> > > >
> > > > 	enum Mutable {
> > > > 		ReadOnly,
> > > > 		ReadWrite,
> > > > 	};
> > > >
> > > > 	ControlInfo(bool default, Mutable mutable);
> > > >
> > > > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > > > improved). One would use this, mapping to the four cases above, as
> > > >
> > > > 	ControlInfo(false, ControlInfo::ReadWrite);
> > > > 	ControlInfo(true, ControlInfo::ReadWrite);
> > > > 	ControlInfo(false, ControlInfo::ReadOnly);
> > > > 	ControlInfo(true, ControlInfo::ReadOnly);
> > > >
> > > > Maybe writing it differently would be more explicit:
> > > >
> > > > 	ControlInfo(ControlInfo::MutableBool, false);
> > > > 	ControlInfo(ControlInfo::MutableBool, true);
> > > > 	ControlInfo(ControlInfo::ImmutableBool, false);
> > > > 	ControlInfo(ControlInfo::ImmutableBool, true);
> > >
> > > Ah, this could solve the concerns I expressed above.
> > >
> > > We have the general need to express when a Control is RO or RW, don't
> > > we ? We could add an rw_ flag to the ControlInfo class for that and
> > > derive a read-only variant maybe ?
> >
> > Thinking more about this, the read-only concept bothers me a bit. A
> > read-only control is essentially either a property or a metadata in the
> > general case (depending on whether the value is static or dynamic). What
> > we're dealing with here is a control that is meant to be writable, but
> > happens to only have a single supported value.
> 
> This has bothered me all the time I tried to find a good term to
> identify those "booleans with a single value". I proposed 'identity'
> but that's not really up to the point. The fact I can't find a term
> for that makes me think I'm missing something and we should probably
> be doing something different. Hence the proposal to have a dedicated
> constructor, but yes, more constructors are ambiguos.
> 
> >
> > Taking one step back, what are our use cases for boolean controls that
> > accept a single value ?
> >
> 
> I think the original intent was to be able to express things like: the
> IPA only supports AE_ENABLE, so that you register a
> 
>         {controls::AeEnable, ControlInfo(true) }
> 
> While if you can enable/disable it, and it's enabled by default:
> 
>         {controls::AeEnable, ControlInfo({true, false}, true) }
> 
> I would now suggest to simply not register the control at all if it's
> not mutable, but I'm sure right now I'm missing why this was not
> possible in first place. Maybe it's just a matter of defining a policy
> like this one: if a control (not a property, nor a metadata) cannot be
> changed, just don't register it as part of the Camera::controls" ?

In that case applications wouldn't be able to differentiate a device
that doesn't support auto-exposure and a device that doesn't support
manual exposure. OK, not entirely true, applications could check for the
presence of a manual exposure time control, but in general, we could
have other boolean controls that would suffer from this issue.

> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > index 1bc958a43b22..5ef84a92f219 100644
> > > --- a/include/libcamera/controls.h
> > > +++ b/include/libcamera/controls.h
> > > @@ -272,11 +272,16 @@ public:
> > >                              const ControlValue &def = 0);
> > >         explicit ControlInfo(Span<const ControlValue> values,
> > >                              const ControlValue &def = {});
> > > +       explicit ControlInfo(bool value)
> > > +               : min_(false), max_(true), def_(value)
> > > +       {
> > > +       }
> > >
> > >         const ControlValue &min() const { return min_; }
> > >         const ControlValue &max() const { return max_; }
> > >         const ControlValue &def() const { return def_; }
> > >         const std::vector<ControlValue> &values() const { return values_; }
> > > +       bool rw() const { return rw_; }
> > >
> > >         std::string toString() const;
> > >
> > > @@ -290,13 +295,41 @@ public:
> > >                 return !(*this == other);
> > >         }
> > >
> > > -private:
> > > +protected:
> > > +       bool rw_ : true;
> > >         ControlValue min_;
> > >         ControlValue max_;
> > >         ControlValue def_;
> > >         std::vector<ControlValue> values_;
> > >  };
> > >
> > > +class ROControlInfo : public ControlInfo
> > > +{
> > > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> > > +
> > > +       explicit ROControlInfo(const ControlValue &min = 0,
> > > +                              const ControlValue &max = 0,
> > > +                              const ControlValue &def = 0)
> > > +               : ControlInfo(min, max, def)
> > > +       {
> > > +               rw_ = false;
> > > +       }
> > > +
> > > +       explicit ROControlInfo(Span<const ControlValue> values,
> > > +                              const ControlValue &def = {})
> > > +               : ControlInfo(values, def)
> > > +       {
> > > +               rw_ = false;
> > > +       }
> > > +
> > > +       explicit ROControlInfo(bool value)
> > > +       {
> > > +               min_ = max_ = def_ = value;
> > > +               rw_ = false;
> > > +       }
> > > +};
> > > +
> > >
> > > I'm sure it could be done in a smarter way than this, I'm just wonder
> > > if it's worth it and IPA/ph should be in charge of deciding what type
> > > of ControlInfo to construct. For the generator, once we have a
> > > read_only flag, this should be trivial instead.
> > >
> > > > Now that I've started the brainstorming, I'll let you unleash your brain
> > > > power to shoot this down or improve it :-)
> > > >
> > > > > > mind, I guess we could now live with this ?
> > > > > >
> > > > > > > + *
> > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > > > + * default value is \a def.
> > > > > > > + */
> > > > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > > > > +{
> > > > > > > +	assert(values.count(def) && values.size() == 2);
> > > > > >
> > > > > > Be aware this will accept ControlInfo({true, true}, false);
> > > > >
> > > > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > > > should be 1.
> > > > >
> > > > > At least that was my reasoning. Is it wrong?
> > > > >
> > > > > > > +}
> > > > > > > +
> > > > > > > +/**
> > > > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > > > + * \param[in] value The control valid boolean value
> > > > > > > + *
> > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > > > + */
> > > > > > > +ControlInfo::ControlInfo(bool value)
> > > > > > > +	: min_(value), max_(value), def_(value)
> > > > > > > +{
> > > > > > > +	values_ = { value };
> > > > > > > +}
> > > > > > > +
> > > > > > >  /**
> > > > > > >   * \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..2827473b 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);
> > > > > > > +
> > > > > > > +		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;
> > > > > > > +		}
> > > > > > > +
> > > > > >
> > > > > > As said, not in love with this, but I don't have anything better to
> > > > > > suggest :)
> > > > > >
> > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > >
> > > > > > >  		return TestPass;
> > > > > > >  	}
> > > > > > >  };
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Hi Jacopo and Laurent,

On Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:
> > On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > 
> > [snip]
> > 
> > > > > The following would be one way to add a bool-specific constructor that
> > > > > will have no risk of clashing with anything in the future:
> > > > >
> > > > > 	enum Mutable {
> > > > > 		ReadOnly,
> > > > > 		ReadWrite,
> > > > > 	};
> > > > >
> > > > > 	ControlInfo(bool default, Mutable mutable);
> > > > >
> > > > > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > > > > improved). One would use this, mapping to the four cases above, as
> > > > >
> > > > > 	ControlInfo(false, ControlInfo::ReadWrite);
> > > > > 	ControlInfo(true, ControlInfo::ReadWrite);
> > > > > 	ControlInfo(false, ControlInfo::ReadOnly);
> > > > > 	ControlInfo(true, ControlInfo::ReadOnly);
> > > > >
> > > > > Maybe writing it differently would be more explicit:
> > > > >
> > > > > 	ControlInfo(ControlInfo::MutableBool, false);
> > > > > 	ControlInfo(ControlInfo::MutableBool, true);
> > > > > 	ControlInfo(ControlInfo::ImmutableBool, false);
> > > > > 	ControlInfo(ControlInfo::ImmutableBool, true);
> > > >
> > > > Ah, this could solve the concerns I expressed above.
> > > >
> > > > We have the general need to express when a Control is RO or RW, don't
> > > > we ? We could add an rw_ flag to the ControlInfo class for that and
> > > > derive a read-only variant maybe ?
> > >
> > > Thinking more about this, the read-only concept bothers me a bit. A
> > > read-only control is essentially either a property or a metadata in the
> > > general case (depending on whether the value is static or dynamic). What
> > > we're dealing with here is a control that is meant to be writable, but
> > > happens to only have a single supported value.
> > 
> > This has bothered me all the time I tried to find a good term to
> > identify those "booleans with a single value". I proposed 'identity'
> > but that's not really up to the point. The fact I can't find a term
> > for that makes me think I'm missing something and we should probably
> > be doing something different. Hence the proposal to have a dedicated
> > constructor, but yes, more constructors are ambiguos.
> > 
> > >
> > > Taking one step back, what are our use cases for boolean controls that
> > > accept a single value ?
> > >
> > 
> > I think the original intent was to be able to express things like: the
> > IPA only supports AE_ENABLE, so that you register a
> > 
> >         {controls::AeEnable, ControlInfo(true) }
> > 
> > While if you can enable/disable it, and it's enabled by default:
> > 
> >         {controls::AeEnable, ControlInfo({true, false}, true) }
> > 
> > I would now suggest to simply not register the control at all if it's
> > not mutable, but I'm sure right now I'm missing why this was not
> > possible in first place. Maybe it's just a matter of defining a policy
> > like this one: if a control (not a property, nor a metadata) cannot be
> > changed, just don't register it as part of the Camera::controls" ?

Oh huh, I see. The reason we're having this issue is because we want to
be able to report that "we support AE" and "we don't support turning off
AE" (or the reverse, "we don't support AE"). In that case maybe
properties like "AeAvailable" and "AeTurnoffable" would serve the
unchangable control?

But then we have properties that just mirror controls... Is using
controls not the best way to report capabilities? They have attached
ControlInfos showing the valid values so I thought they were good.

That's why imo I think that ControlInfo({ true, false }, true) is fine.
Sure it's redundant information, but it's clear: "I want a boolean
control that supports both true and false, and the default value is
true". We have assertions to protect against dumb/malicious users that
try weird stuff like ControlInfo({ true, true }, false). And then if you
the user goes "I want a boolean control that supports only true", then
the default value is implied, and ControlInfo(true) is sufficient.

Sorry it's just kinda rambling :p


Paul

> 
> In that case applications wouldn't be able to differentiate a device
> that doesn't support auto-exposure and a device that doesn't support
> manual exposure. OK, not entirely true, applications could check for the
> presence of a manual exposure time control, but in general, we could
> have other boolean controls that would suffer from this issue.
> 
> > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > index 1bc958a43b22..5ef84a92f219 100644
> > > > --- a/include/libcamera/controls.h
> > > > +++ b/include/libcamera/controls.h
> > > > @@ -272,11 +272,16 @@ public:
> > > >                              const ControlValue &def = 0);
> > > >         explicit ControlInfo(Span<const ControlValue> values,
> > > >                              const ControlValue &def = {});
> > > > +       explicit ControlInfo(bool value)
> > > > +               : min_(false), max_(true), def_(value)
> > > > +       {
> > > > +       }
> > > >
> > > >         const ControlValue &min() const { return min_; }
> > > >         const ControlValue &max() const { return max_; }
> > > >         const ControlValue &def() const { return def_; }
> > > >         const std::vector<ControlValue> &values() const { return values_; }
> > > > +       bool rw() const { return rw_; }
> > > >
> > > >         std::string toString() const;
> > > >
> > > > @@ -290,13 +295,41 @@ public:
> > > >                 return !(*this == other);
> > > >         }
> > > >
> > > > -private:
> > > > +protected:
> > > > +       bool rw_ : true;
> > > >         ControlValue min_;
> > > >         ControlValue max_;
> > > >         ControlValue def_;
> > > >         std::vector<ControlValue> values_;
> > > >  };
> > > >
> > > > +class ROControlInfo : public ControlInfo
> > > > +{
> > > > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> > > > +
> > > > +       explicit ROControlInfo(const ControlValue &min = 0,
> > > > +                              const ControlValue &max = 0,
> > > > +                              const ControlValue &def = 0)
> > > > +               : ControlInfo(min, max, def)
> > > > +       {
> > > > +               rw_ = false;
> > > > +       }
> > > > +
> > > > +       explicit ROControlInfo(Span<const ControlValue> values,
> > > > +                              const ControlValue &def = {})
> > > > +               : ControlInfo(values, def)
> > > > +       {
> > > > +               rw_ = false;
> > > > +       }
> > > > +
> > > > +       explicit ROControlInfo(bool value)
> > > > +       {
> > > > +               min_ = max_ = def_ = value;
> > > > +               rw_ = false;
> > > > +       }
> > > > +};
> > > > +
> > > >
> > > > I'm sure it could be done in a smarter way than this, I'm just wonder
> > > > if it's worth it and IPA/ph should be in charge of deciding what type
> > > > of ControlInfo to construct. For the generator, once we have a
> > > > read_only flag, this should be trivial instead.
> > > >
> > > > > Now that I've started the brainstorming, I'll let you unleash your brain
> > > > > power to shoot this down or improve it :-)
> > > > >
> > > > > > > mind, I guess we could now live with this ?
> > > > > > >
> > > > > > > > + *
> > > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > > > > + * default value is \a def.
> > > > > > > > + */
> > > > > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > > > > > +{
> > > > > > > > +	assert(values.count(def) && values.size() == 2);
> > > > > > >
> > > > > > > Be aware this will accept ControlInfo({true, true}, false);
> > > > > >
> > > > > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > > > > should be 1.
> > > > > >
> > > > > > At least that was my reasoning. Is it wrong?
> > > > > >
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > > > > + * \param[in] value The control valid boolean value
> > > > > > > > + *
> > > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > > > > + */
> > > > > > > > +ControlInfo::ControlInfo(bool value)
> > > > > > > > +	: min_(value), max_(value), def_(value)
> > > > > > > > +{
> > > > > > > > +	values_ = { value };
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  /**
> > > > > > > >   * \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..2827473b 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);
> > > > > > > > +
> > > > > > > > +		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;
> > > > > > > > +		}
> > > > > > > > +
> > > > > > >
> > > > > > > As said, not in love with this, but I don't have anything better to
> > > > > > > suggest :)
> > > > > > >
> > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > >
> > > > > > > >  		return TestPass;
> > > > > > > >  	}
> > > > > > > >  };
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Hi Paul,

On Thu, Jul 29, 2021 at 07:34:37PM +0900, paul.elder@ideasonboard.com wrote:
> On Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:
> > On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:
> > > On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:
> > > > Hi Jacopo,
> > > 
> > > [snip]
> > > 
> > > > > > The following would be one way to add a bool-specific constructor that
> > > > > > will have no risk of clashing with anything in the future:
> > > > > >
> > > > > > 	enum Mutable {
> > > > > > 		ReadOnly,
> > > > > > 		ReadWrite,
> > > > > > 	};
> > > > > >
> > > > > > 	ControlInfo(bool default, Mutable mutable);
> > > > > >
> > > > > > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > > > > > improved). One would use this, mapping to the four cases above, as
> > > > > >
> > > > > > 	ControlInfo(false, ControlInfo::ReadWrite);
> > > > > > 	ControlInfo(true, ControlInfo::ReadWrite);
> > > > > > 	ControlInfo(false, ControlInfo::ReadOnly);
> > > > > > 	ControlInfo(true, ControlInfo::ReadOnly);
> > > > > >
> > > > > > Maybe writing it differently would be more explicit:
> > > > > >
> > > > > > 	ControlInfo(ControlInfo::MutableBool, false);
> > > > > > 	ControlInfo(ControlInfo::MutableBool, true);
> > > > > > 	ControlInfo(ControlInfo::ImmutableBool, false);
> > > > > > 	ControlInfo(ControlInfo::ImmutableBool, true);
> > > > >
> > > > > Ah, this could solve the concerns I expressed above.
> > > > >
> > > > > We have the general need to express when a Control is RO or RW, don't
> > > > > we ? We could add an rw_ flag to the ControlInfo class for that and
> > > > > derive a read-only variant maybe ?
> > > >
> > > > Thinking more about this, the read-only concept bothers me a bit. A
> > > > read-only control is essentially either a property or a metadata in the
> > > > general case (depending on whether the value is static or dynamic). What
> > > > we're dealing with here is a control that is meant to be writable, but
> > > > happens to only have a single supported value.
> > > 
> > > This has bothered me all the time I tried to find a good term to
> > > identify those "booleans with a single value". I proposed 'identity'
> > > but that's not really up to the point. The fact I can't find a term
> > > for that makes me think I'm missing something and we should probably
> > > be doing something different. Hence the proposal to have a dedicated
> > > constructor, but yes, more constructors are ambiguos.
> > > 
> > > > Taking one step back, what are our use cases for boolean controls that
> > > > accept a single value ?
> > > 
> > > I think the original intent was to be able to express things like: the
> > > IPA only supports AE_ENABLE, so that you register a
> > > 
> > >         {controls::AeEnable, ControlInfo(true) }
> > > 
> > > While if you can enable/disable it, and it's enabled by default:
> > > 
> > >         {controls::AeEnable, ControlInfo({true, false}, true) }
> > > 
> > > I would now suggest to simply not register the control at all if it's
> > > not mutable, but I'm sure right now I'm missing why this was not
> > > possible in first place. Maybe it's just a matter of defining a policy
> > > like this one: if a control (not a property, nor a metadata) cannot be
> > > changed, just don't register it as part of the Camera::controls" ?
> 
> Oh huh, I see. The reason we're having this issue is because we want to
> be able to report that "we support AE" and "we don't support turning off
> AE" (or the reverse, "we don't support AE"). In that case maybe
> properties like "AeAvailable" and "AeTurnoffable" would serve the
> unchangable control?
> 
> But then we have properties that just mirror controls... Is using
> controls not the best way to report capabilities? They have attached
> ControlInfos showing the valid values so I thought they were good.

This is an area where we depart from the Android camera HAL API. In
Android, meta-information about controls (does this make it camera
meta-meta-data ? :-)) is reported as static metadata, while in
libcamera, we report them through ControlInfo. I like ControlInfo as it
carries a clear association with controls, while in Android, one has to
know which static metadata corresponds to which control metadata (or
possibly dynamic metadata). Static information that are not related to
controls are reported in libcamera through properties, which as
equivalent to the Android static metadata. There's one shortcoming in
our approach though, we have no way to report information about metadata
(in the libcamera sense, as reported through requests, the equivalent of
the Android dynamic metadata). Maybe we would need Camera::metadata()
for that.

Anyway, at this point I don't think we should switch to creating more
properties to report information about controls.

> That's why imo I think that ControlInfo({ true, false }, true) is fine.
> Sure it's redundant information, but it's clear: "I want a boolean
> control that supports both true and false, and the default value is
> true". We have assertions to protect against dumb/malicious users that
> try weird stuff like ControlInfo({ true, true }, false). And then if you
> the user goes "I want a boolean control that supports only true", then
> the default value is implied, and ControlInfo(true) is sufficient.

At the risk of repeating myself, this bothers me for a few reasons:

- We have a similar issue with enumerated controls (not having a nice
  syntax to create a ControlInfo with a set of enum values and a
  default), and bool controls can be considered as a special case of an
  enumeration with only two values, so it would be nice to have a single
  API (or at least single syntax, even if the constructors end up being
  different) to handle both. I'm not sure if this is possible.

- There's room for getting it wrong by passing ({ true, true }, false).
  As you mentioned, we have asserts to catch this, but making errors
  impossible in the first place would create a nicer API.

- ControlInfo(true) (or false) is really confusing to read, more, I
  believe, than ControlInfo({ true, true }, true). Furthermore, the more
  constructors we have, especially with common types, the more we'll
  risk running into a situation where we won't be able to add a new
  constructor due to ambiguous overload resolution. I think writing
  ControlInfo({ true }, true) would be better than ControlInfo(true)
  from that point of view.

None of those are showstoppers, but they make me think we can do better.

> Sorry it's just kinda rambling :p

That's what I've been doing as well so far, so no need to apologize :-)

> > In that case applications wouldn't be able to differentiate a device
> > that doesn't support auto-exposure and a device that doesn't support
> > manual exposure. OK, not entirely true, applications could check for the
> > presence of a manual exposure time control, but in general, we could
> > have other boolean controls that would suffer from this issue.
> > 
> > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > > index 1bc958a43b22..5ef84a92f219 100644
> > > > > --- a/include/libcamera/controls.h
> > > > > +++ b/include/libcamera/controls.h
> > > > > @@ -272,11 +272,16 @@ public:
> > > > >                              const ControlValue &def = 0);
> > > > >         explicit ControlInfo(Span<const ControlValue> values,
> > > > >                              const ControlValue &def = {});
> > > > > +       explicit ControlInfo(bool value)
> > > > > +               : min_(false), max_(true), def_(value)
> > > > > +       {
> > > > > +       }
> > > > >
> > > > >         const ControlValue &min() const { return min_; }
> > > > >         const ControlValue &max() const { return max_; }
> > > > >         const ControlValue &def() const { return def_; }
> > > > >         const std::vector<ControlValue> &values() const { return values_; }
> > > > > +       bool rw() const { return rw_; }
> > > > >
> > > > >         std::string toString() const;
> > > > >
> > > > > @@ -290,13 +295,41 @@ public:
> > > > >                 return !(*this == other);
> > > > >         }
> > > > >
> > > > > -private:
> > > > > +protected:
> > > > > +       bool rw_ : true;
> > > > >         ControlValue min_;
> > > > >         ControlValue max_;
> > > > >         ControlValue def_;
> > > > >         std::vector<ControlValue> values_;
> > > > >  };
> > > > >
> > > > > +class ROControlInfo : public ControlInfo
> > > > > +{
> > > > > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> > > > > +
> > > > > +       explicit ROControlInfo(const ControlValue &min = 0,
> > > > > +                              const ControlValue &max = 0,
> > > > > +                              const ControlValue &def = 0)
> > > > > +               : ControlInfo(min, max, def)
> > > > > +       {
> > > > > +               rw_ = false;
> > > > > +       }
> > > > > +
> > > > > +       explicit ROControlInfo(Span<const ControlValue> values,
> > > > > +                              const ControlValue &def = {})
> > > > > +               : ControlInfo(values, def)
> > > > > +       {
> > > > > +               rw_ = false;
> > > > > +       }
> > > > > +
> > > > > +       explicit ROControlInfo(bool value)
> > > > > +       {
> > > > > +               min_ = max_ = def_ = value;
> > > > > +               rw_ = false;
> > > > > +       }
> > > > > +};
> > > > > +
> > > > >
> > > > > I'm sure it could be done in a smarter way than this, I'm just wonder
> > > > > if it's worth it and IPA/ph should be in charge of deciding what type
> > > > > of ControlInfo to construct. For the generator, once we have a
> > > > > read_only flag, this should be trivial instead.
> > > > >
> > > > > > Now that I've started the brainstorming, I'll let you unleash your brain
> > > > > > power to shoot this down or improve it :-)
> > > > > >
> > > > > > > > mind, I guess we could now live with this ?
> > > > > > > >
> > > > > > > > > + *
> > > > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > > > > > + * default value is \a def.
> > > > > > > > > + */
> > > > > > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > > > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > > > > > > +{
> > > > > > > > > +	assert(values.count(def) && values.size() == 2);
> > > > > > > >
> > > > > > > > Be aware this will accept ControlInfo({true, true}, false);
> > > > > > >
> > > > > > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > > > > > should be 1.
> > > > > > >
> > > > > > > At least that was my reasoning. Is it wrong?
> > > > > > >
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/**
> > > > > > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > > > > > + * \param[in] value The control valid boolean value
> > > > > > > > > + *
> > > > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > > > > > + */
> > > > > > > > > +ControlInfo::ControlInfo(bool value)
> > > > > > > > > +	: min_(value), max_(value), def_(value)
> > > > > > > > > +{
> > > > > > > > > +	values_ = { value };
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  /**
> > > > > > > > >   * \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..2827473b 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);
> > > > > > > > > +
> > > > > > > > > +		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;
> > > > > > > > > +		}
> > > > > > > > > +
> > > > > > > >
> > > > > > > > As said, not in love with this, but I don't have anything better to
> > > > > > > > suggest :)
> > > > > > > >
> > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > > >
> > > > > > > > >  		return TestPass;
> > > > > > > > >  	}
> > > > > > > > >  };

Hi Paul,

On Mon, Aug 02, 2021 at 04:08:22AM +0300, Laurent Pinchart wrote:
> On Thu, Jul 29, 2021 at 07:34:37PM +0900, paul.elder@ideasonboard.com wrote:
> > On Wed, Jul 28, 2021 at 07:39:23PM +0300, Laurent Pinchart wrote:
> > > On Wed, Jul 28, 2021 at 06:31:59PM +0200, Jacopo Mondi wrote:
> > > > On Mon, Jul 26, 2021 at 07:36:54PM +0300, Laurent Pinchart wrote:
> > > > > Hi Jacopo,
> > > > 
> > > > [snip]
> > > > 
> > > > > > > The following would be one way to add a bool-specific constructor that
> > > > > > > will have no risk of clashing with anything in the future:
> > > > > > >
> > > > > > > 	enum Mutable {
> > > > > > > 		ReadOnly,
> > > > > > > 		ReadWrite,
> > > > > > > 	};
> > > > > > >
> > > > > > > 	ControlInfo(bool default, Mutable mutable);
> > > > > > >
> > > > > > > (the Mutable, ReadOnly and ReadWrite names can most probably be
> > > > > > > improved). One would use this, mapping to the four cases above, as
> > > > > > >
> > > > > > > 	ControlInfo(false, ControlInfo::ReadWrite);
> > > > > > > 	ControlInfo(true, ControlInfo::ReadWrite);
> > > > > > > 	ControlInfo(false, ControlInfo::ReadOnly);
> > > > > > > 	ControlInfo(true, ControlInfo::ReadOnly);
> > > > > > >
> > > > > > > Maybe writing it differently would be more explicit:
> > > > > > >
> > > > > > > 	ControlInfo(ControlInfo::MutableBool, false);
> > > > > > > 	ControlInfo(ControlInfo::MutableBool, true);
> > > > > > > 	ControlInfo(ControlInfo::ImmutableBool, false);
> > > > > > > 	ControlInfo(ControlInfo::ImmutableBool, true);
> > > > > >
> > > > > > Ah, this could solve the concerns I expressed above.
> > > > > >
> > > > > > We have the general need to express when a Control is RO or RW, don't
> > > > > > we ? We could add an rw_ flag to the ControlInfo class for that and
> > > > > > derive a read-only variant maybe ?
> > > > >
> > > > > Thinking more about this, the read-only concept bothers me a bit. A
> > > > > read-only control is essentially either a property or a metadata in the
> > > > > general case (depending on whether the value is static or dynamic). What
> > > > > we're dealing with here is a control that is meant to be writable, but
> > > > > happens to only have a single supported value.
> > > > 
> > > > This has bothered me all the time I tried to find a good term to
> > > > identify those "booleans with a single value". I proposed 'identity'
> > > > but that's not really up to the point. The fact I can't find a term
> > > > for that makes me think I'm missing something and we should probably
> > > > be doing something different. Hence the proposal to have a dedicated
> > > > constructor, but yes, more constructors are ambiguos.
> > > > 
> > > > > Taking one step back, what are our use cases for boolean controls that
> > > > > accept a single value ?
> > > > 
> > > > I think the original intent was to be able to express things like: the
> > > > IPA only supports AE_ENABLE, so that you register a
> > > > 
> > > >         {controls::AeEnable, ControlInfo(true) }
> > > > 
> > > > While if you can enable/disable it, and it's enabled by default:
> > > > 
> > > >         {controls::AeEnable, ControlInfo({true, false}, true) }
> > > > 
> > > > I would now suggest to simply not register the control at all if it's
> > > > not mutable, but I'm sure right now I'm missing why this was not
> > > > possible in first place. Maybe it's just a matter of defining a policy
> > > > like this one: if a control (not a property, nor a metadata) cannot be
> > > > changed, just don't register it as part of the Camera::controls" ?
> > 
> > Oh huh, I see. The reason we're having this issue is because we want to
> > be able to report that "we support AE" and "we don't support turning off
> > AE" (or the reverse, "we don't support AE"). In that case maybe
> > properties like "AeAvailable" and "AeTurnoffable" would serve the
> > unchangable control?
> > 
> > But then we have properties that just mirror controls... Is using
> > controls not the best way to report capabilities? They have attached
> > ControlInfos showing the valid values so I thought they were good.
> 
> This is an area where we depart from the Android camera HAL API. In
> Android, meta-information about controls (does this make it camera
> meta-meta-data ? :-)) is reported as static metadata, while in
> libcamera, we report them through ControlInfo. I like ControlInfo as it
> carries a clear association with controls, while in Android, one has to
> know which static metadata corresponds to which control metadata (or
> possibly dynamic metadata). Static information that are not related to
> controls are reported in libcamera through properties, which as
> equivalent to the Android static metadata. There's one shortcoming in
> our approach though, we have no way to report information about metadata
> (in the libcamera sense, as reported through requests, the equivalent of
> the Android dynamic metadata). Maybe we would need Camera::metadata()
> for that.
> 
> Anyway, at this point I don't think we should switch to creating more
> properties to report information about controls.
> 
> > That's why imo I think that ControlInfo({ true, false }, true) is fine.
> > Sure it's redundant information, but it's clear: "I want a boolean
> > control that supports both true and false, and the default value is
> > true". We have assertions to protect against dumb/malicious users that
> > try weird stuff like ControlInfo({ true, true }, false). And then if you
> > the user goes "I want a boolean control that supports only true", then
> > the default value is implied, and ControlInfo(true) is sufficient.
> 
> At the risk of repeating myself, this bothers me for a few reasons:
> 
> - We have a similar issue with enumerated controls (not having a nice
>   syntax to create a ControlInfo with a set of enum values and a
>   default), and bool controls can be considered as a special case of an
>   enumeration with only two values, so it would be nice to have a single
>   API (or at least single syntax, even if the constructors end up being
>   different) to handle both. I'm not sure if this is possible.
> 
> - There's room for getting it wrong by passing ({ true, true }, false).
>   As you mentioned, we have asserts to catch this, but making errors
>   impossible in the first place would create a nicer API.
> 
> - ControlInfo(true) (or false) is really confusing to read, more, I
>   believe, than ControlInfo({ true, true }, true). Furthermore, the more
>   constructors we have, especially with common types, the more we'll
>   risk running into a situation where we won't be able to add a new
>   constructor due to ambiguous overload resolution. I think writing
>   ControlInfo({ true }, true) would be better than ControlInfo(true)
>   from that point of view.
> 
> None of those are showstoppers, but they make me think we can do better.

I notice this got merged, even though the discussion hasn't come to a
conclusion. Not really an issue as such, but I'd like to revive this
mail thread. Any opinion on the three points above ?

> > Sorry it's just kinda rambling :p
> 
> That's what I've been doing as well so far, so no need to apologize :-)
> 
> > > In that case applications wouldn't be able to differentiate a device
> > > that doesn't support auto-exposure and a device that doesn't support
> > > manual exposure. OK, not entirely true, applications could check for the
> > > presence of a manual exposure time control, but in general, we could
> > > have other boolean controls that would suffer from this issue.
> > > 
> > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > > > > index 1bc958a43b22..5ef84a92f219 100644
> > > > > > --- a/include/libcamera/controls.h
> > > > > > +++ b/include/libcamera/controls.h
> > > > > > @@ -272,11 +272,16 @@ public:
> > > > > >                              const ControlValue &def = 0);
> > > > > >         explicit ControlInfo(Span<const ControlValue> values,
> > > > > >                              const ControlValue &def = {});
> > > > > > +       explicit ControlInfo(bool value)
> > > > > > +               : min_(false), max_(true), def_(value)
> > > > > > +       {
> > > > > > +       }
> > > > > >
> > > > > >         const ControlValue &min() const { return min_; }
> > > > > >         const ControlValue &max() const { return max_; }
> > > > > >         const ControlValue &def() const { return def_; }
> > > > > >         const std::vector<ControlValue> &values() const { return values_; }
> > > > > > +       bool rw() const { return rw_; }
> > > > > >
> > > > > >         std::string toString() const;
> > > > > >
> > > > > > @@ -290,13 +295,41 @@ public:
> > > > > >                 return !(*this == other);
> > > > > >         }
> > > > > >
> > > > > > -private:
> > > > > > +protected:
> > > > > > +       bool rw_ : true;
> > > > > >         ControlValue min_;
> > > > > >         ControlValue max_;
> > > > > >         ControlValue def_;
> > > > > >         std::vector<ControlValue> values_;
> > > > > >  };
> > > > > >
> > > > > > +class ROControlInfo : public ControlInfo
> > > > > > +{
> > > > > > +       LIBCAMERA_DISABLE_COPY_AND_MOVE(ROControlInfo);
> > > > > > +
> > > > > > +       explicit ROControlInfo(const ControlValue &min = 0,
> > > > > > +                              const ControlValue &max = 0,
> > > > > > +                              const ControlValue &def = 0)
> > > > > > +               : ControlInfo(min, max, def)
> > > > > > +       {
> > > > > > +               rw_ = false;
> > > > > > +       }
> > > > > > +
> > > > > > +       explicit ROControlInfo(Span<const ControlValue> values,
> > > > > > +                              const ControlValue &def = {})
> > > > > > +               : ControlInfo(values, def)
> > > > > > +       {
> > > > > > +               rw_ = false;
> > > > > > +       }
> > > > > > +
> > > > > > +       explicit ROControlInfo(bool value)
> > > > > > +       {
> > > > > > +               min_ = max_ = def_ = value;
> > > > > > +               rw_ = false;
> > > > > > +       }
> > > > > > +};
> > > > > > +
> > > > > >
> > > > > > I'm sure it could be done in a smarter way than this, I'm just wonder
> > > > > > if it's worth it and IPA/ph should be in charge of deciding what type
> > > > > > of ControlInfo to construct. For the generator, once we have a
> > > > > > read_only flag, this should be trivial instead.
> > > > > >
> > > > > > > Now that I've started the brainstorming, I'll let you unleash your brain
> > > > > > > power to shoot this down or improve it :-)
> > > > > > >
> > > > > > > > > mind, I guess we could now live with this ?
> > > > > > > > >
> > > > > > > > > > + *
> > > > > > > > > > + * Construct a ControlInfo for a boolean control, where both true and false are
> > > > > > > > > > + * valid values. \a values must be { false, true } (the order is irrelevant).
> > > > > > > > > > + * The minimum value will always be false, and the maximum always true. The
> > > > > > > > > > + * default value is \a def.
> > > > > > > > > > + */
> > > > > > > > > > +ControlInfo::ControlInfo(std::set<bool> values, bool def)
> > > > > > > > > > +	: min_(false), max_(true), def_(def), values_({ false, true })
> > > > > > > > > > +{
> > > > > > > > > > +	assert(values.count(def) && values.size() == 2);
> > > > > > > > >
> > > > > > > > > Be aware this will accept ControlInfo({true, true}, false);
> > > > > > > >
> > > > > > > > It's a std::set<bool> so if you do { true, true } then values.size()
> > > > > > > > should be 1.
> > > > > > > >
> > > > > > > > At least that was my reasoning. Is it wrong?
> > > > > > > >
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +/**
> > > > > > > > > > + * \brief Construct a boolean ControlInfo with only one valid value
> > > > > > > > > > + * \param[in] value The control valid boolean value
> > > > > > > > > > + *
> > > > > > > > > > + * Construct a ControlInfo for a boolean control, where there is only valid
> > > > > > > > > > + * value. The minimum, maximum, and default values will all be \a value.
> > > > > > > > > > + */
> > > > > > > > > > +ControlInfo::ControlInfo(bool value)
> > > > > > > > > > +	: min_(value), max_(value), def_(value)
> > > > > > > > > > +{
> > > > > > > > > > +	values_ = { value };
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  /**
> > > > > > > > > >   * \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..2827473b 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);
> > > > > > > > > > +
> > > > > > > > > > +		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;
> > > > > > > > > > +		}
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > As said, not in love with this, but I don't have anything better to
> > > > > > > > > suggest :)
> > > > > > > > >
> > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > > > > > >
> > > > > > > > > >  		return TestPass;
> > > > > > > > > >  	}
> > > > > > > > > >  };