Message ID | 20210716105631.158153-2-paul.elder@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Paul, On Fri, Jul 16, 2021 at 07:56:11PM +0900, Paul Elder wrote: > It would be convenient to be able to iterate over available boolean > values, for example for controls that designate if some function can be > enabled/disabled. The current min/max/def constructor is insufficient, > as .values() is empty, so the values cannot be easily iterated over, and > creating a Span of booleans does not work for the values constructor. > > Add a new constructor to ControlInfo that takes a set of booleans (to > allow specifiying one or two available values), and a default. The > default value is not optional, as it doesn't make sense to have a silent > default for boolean values. > > Update the ControlInfo test accordingly. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v2: > - use set instead of span of bools > - add assertion to make sure that the default is a valid value > - update the test > --- > include/libcamera/controls.h | 2 ++ > src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++ > test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++ > 3 files changed, 63 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 1bc958a4..707dc335 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -9,6 +9,7 @@ > #define __LIBCAMERA_CONTROLS_H__ > > #include <assert.h> > +#include <set> > #include <stdint.h> > #include <string> > #include <unordered_map> > @@ -272,6 +273,7 @@ public: > const ControlValue &def = 0); > explicit ControlInfo(Span<const ControlValue> values, > const ControlValue &def = {}); > + explicit ControlInfo(std::set<bool> values, bool def); > > const ControlValue &min() const { return min_; } > const ControlValue &max() const { return max_; } > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 34317fa0..f6351c01 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -514,6 +514,34 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > values_.push_back(value); > } > > +/** > + * \brief Construct a ControlInfo from a list of valid boolean values > + * \param[in] values The control valid boolean vaalues s/vaalues/values/ > + * \param[in] def The control default boolean value > + * > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo > + * minimum and maximum values are set to the first and last members of the > + * values list respectively. The default value is set to \a def. std::set is sorted, the order in which elements are listed is irrelevant as far as I can tell (and honestly I don't know if true < or > false :) I would remove the ambiguities and specify that if the ControlInfo supports both {true, false} then min = false, max = true and default is what is passed as paramter. If the set of possible values only contains true OR false, then min = max = def. I think getting the terminology right would really simplify this part. How would you call a ControlInfo that supports both {true, false} and how would you call one that only supported {true} or {false}. binary vs identity ? Something like: Construct a ControlInfo for a boolean control. The ControlInfo can represent a binary (??) control that allows both true and false to be selected, or it can represent an identity (??) control, that only allows a single value. ControlInfo for binary controls are constructed with {true, false} as first parameter (the order is not relevant) \a values and the selected default value as provided in \a def. The minimum value will always be false, and the maximum one will always be true. ControlInfo for identity controls are constructed with a set containing the single supported value as first parameter \a values which will also match their minimum, maximum, and default values. I know wonder if, for indentities, it wouldn't be better a constructor as ControlInfo(bool) > + */ > +ControlInfo::ControlInfo(std::set<bool> values, bool def) > + : def_(def) > +{ > + if (values.size() == 1) { > + min_ = max_ = *values.begin(); Make sure def matches values[0] You can make def default=false and if size() == 1 set it to the single supported value, but that would still allow to write ({true}, false) I would make two constructors to be honest. > + } else { > + min_ = false; > + max_ = true; > + } Ah, see, the order is not relevant! > + > + def_ = def; Isn't this already assigned through the constructor initialization list ? > + > + assert(values.count(def)); > + > + values_.reserve(2); > + for (const bool &value : values) > + values_.push_back(value); values_ = {false, true} ? I would ControlInfo::ControlInfo(std::set<bool> values, bool def) { min_ = false; max_ = true; def_ = def; values_ = { false, true }; return 0; } ControlInfo::ControlInfo(bool value) { min_ = max_ = def_ = value; values_ = { value }; return 0; } With the first version being a syntactic sugar as 'values' is not really required, but serves to disambiguate the two contructors and makes it nicer to read at the expense of typing in {false, true}. > +} > + > /** > * \fn ControlInfo::min() > * \brief Retrieve the minimum value of the control > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp > index 1e05e131..b3d2bd54 100644 > --- a/test/controls/control_info.cpp > +++ b/test/controls/control_info.cpp > @@ -44,6 +44,39 @@ protected: > return TestFail; > } > > + /* > + * Test information retrieval from a control with boolean > + * values. > + */ > + ControlInfo aeEnable({ false, true }, false); > + > + if (aeEnable.min().get<bool>() != false || > + aeEnable.def().get<bool>() != false || > + aeEnable.max().get<bool>() != true) { > + cout << "Invalid control range for AeEnable" << endl; > + return TestFail; > + } > + > + if (aeEnable.values()[0].get<bool>() != false || > + aeEnable.values()[1].get<bool>() != true) { > + cout << "Invalid control values for AeEnable" << endl; > + return TestFail; > + } > + > + ControlInfo awbEnable({ true }, true); > + > + if (awbEnable.min().get<bool>() != true || > + awbEnable.def().get<bool>() != true || > + awbEnable.max().get<bool>() != true) { > + cout << "Invalid control range for AwbEnable" << endl; > + return TestFail; > + } > + > + if (awbEnable.values()[0].get<bool>() != true) { > + cout << "Invalid control values for AwbEnable" << endl; > + return TestFail; > + } > + > return TestPass; > } > }; > -- > 2.27.0 >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 1bc958a4..707dc335 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -9,6 +9,7 @@ #define __LIBCAMERA_CONTROLS_H__ #include <assert.h> +#include <set> #include <stdint.h> #include <string> #include <unordered_map> @@ -272,6 +273,7 @@ public: const ControlValue &def = 0); explicit ControlInfo(Span<const ControlValue> values, const ControlValue &def = {}); + explicit ControlInfo(std::set<bool> values, bool def); const ControlValue &min() const { return min_; } const ControlValue &max() const { return max_; } diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 34317fa0..f6351c01 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -514,6 +514,34 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, values_.push_back(value); } +/** + * \brief Construct a ControlInfo from a list of valid boolean values + * \param[in] values The control valid boolean vaalues + * \param[in] def The control default boolean value + * + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo + * minimum and maximum values are set to the first and last members of the + * values list respectively. The default value is set to \a def. + */ +ControlInfo::ControlInfo(std::set<bool> values, bool def) + : def_(def) +{ + if (values.size() == 1) { + min_ = max_ = *values.begin(); + } else { + min_ = false; + max_ = true; + } + + def_ = def; + + assert(values.count(def)); + + values_.reserve(2); + for (const bool &value : values) + values_.push_back(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..b3d2bd54 100644 --- a/test/controls/control_info.cpp +++ b/test/controls/control_info.cpp @@ -44,6 +44,39 @@ protected: return TestFail; } + /* + * Test information retrieval from a control with boolean + * values. + */ + ControlInfo aeEnable({ false, true }, false); + + if (aeEnable.min().get<bool>() != false || + aeEnable.def().get<bool>() != false || + aeEnable.max().get<bool>() != true) { + cout << "Invalid control range for AeEnable" << endl; + return TestFail; + } + + if (aeEnable.values()[0].get<bool>() != false || + aeEnable.values()[1].get<bool>() != true) { + cout << "Invalid control values for AeEnable" << endl; + return TestFail; + } + + ControlInfo awbEnable({ true }, true); + + if (awbEnable.min().get<bool>() != true || + awbEnable.def().get<bool>() != true || + awbEnable.max().get<bool>() != true) { + cout << "Invalid control range for AwbEnable" << endl; + return TestFail; + } + + if (awbEnable.values()[0].get<bool>() != true) { + cout << "Invalid control values for AwbEnable" << endl; + return TestFail; + } + return TestPass; } };
It would be convenient to be able to iterate over available boolean values, for example for controls that designate if some function can be enabled/disabled. The current min/max/def constructor is insufficient, as .values() is empty, so the values cannot be easily iterated over, and creating a Span of booleans does not work for the values constructor. Add a new constructor to ControlInfo that takes a set of booleans (to allow specifiying one or two available values), and a default. The default value is not optional, as it doesn't make sense to have a silent default for boolean values. Update the ControlInfo test accordingly. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v2: - use set instead of span of bools - add assertion to make sure that the default is a valid value - update the test --- include/libcamera/controls.h | 2 ++ src/libcamera/controls.cpp | 28 ++++++++++++++++++++++++++++ test/controls/control_info.cpp | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+)