| Message ID | 20210702103800.41291-2-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote: > It would be convenient to be able to iterate over available boolean > values, for example for controls that designate if some function can be > enabled/disabled. The current min/max/def constructor is insufficient, > as .values() is empty, so the values cannot be easily iterated over, and > creating a Span of booleans does not work for the values constructor. > > Add a new constructor to ControlInfo that takes a Span of booleans (to > allow specifiying one or two available values), and a default. The > default value is not optional, as it doesn't make sense to have a silent > default for boolean values. So this is intended to be used as ControlInfo({true}, true); ControlInfo({false}, false); ControlInfo({true, true}, true); ControlInfo({true, true}, false); ? What is the purpose of having to specify a default if only one value is available ? Does it even make sense to have a ControlInfo that only represents true or false ? Can you make sure ControlInfo({true}, false); doesn't happen ? > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v3 > --- > include/libcamera/controls.h | 1 + > src/libcamera/controls.cpp | 20 ++++++++++++++++++++ > 2 files changed, 21 insertions(+) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 1bc958a4..2dd147c8 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -272,6 +272,7 @@ public: > const ControlValue &def = 0); > explicit ControlInfo(Span<const ControlValue> values, > const ControlValue &def = {}); > + explicit ControlInfo(Span<const bool> values, bool def); The explicit keyword is used to disallow implicit conversion and copy construction. It only applies to constructors with a single parameter afaict, so it is not required here. > > const ControlValue &min() const { return min_; } > const ControlValue &max() const { return max_; } > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 34317fa0..e32e22e2 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > values_.push_back(value); > } > > +/** > + * \brief Construct a ControlInfo from a list of valid boolean values > + * \param[in] values The control valid boolean vaalues s/vaalues/values The values can be just {true, false}, right ? > + * \param[in] def The control default boolean value > + * > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo > + * minimum and maximum values are set to the first and last members of the > + * values list respectively. The default value is set to \a def. So ControlInfo({true, false}) != ControlInfo({false, true}) ? > + */ > +ControlInfo::ControlInfo(Span<const bool> values, bool def) > + : def_(def) > +{ > + min_ = values.front(); > + max_ = values.back(); > + > + values_.reserve(2); > + for (const bool &value : values) > + values_.push_back(value); > +} > + Could you point me to where this constructor is used, as it seems suspicious to me :) > /** > * \fn ControlInfo::min() > * \brief Retrieve the minimum value of the control > -- > 2.27.0 >
Hi Jacopo, On Mon, Jul 05, 2021 at 03:39:00PM +0200, Jacopo Mondi wrote: > Hi Paul, > > On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote: > > It would be convenient to be able to iterate over available boolean > > values, for example for controls that designate if some function can be > > enabled/disabled. The current min/max/def constructor is insufficient, > > as .values() is empty, so the values cannot be easily iterated over, and > > creating a Span of booleans does not work for the values constructor. > > > > Add a new constructor to ControlInfo that takes a Span of booleans (to > > allow specifiying one or two available values), and a default. The > > default value is not optional, as it doesn't make sense to have a silent > > default for boolean values. > > So this is intended to be used as > ControlInfo({true}, true); > ControlInfo({false}, false); > ControlInfo({true, true}, true); ControlInfo({false, true}, true) > ControlInfo({true, true}, false); ControlInfo({false, true}, false) > > ? Yeah, that was the idea. tbh I don't feel so well about it but I'm not sure how else to it it. Although, since we have to declare a Span when we use it anyway, maybe we can just use the exsiting Span constructor. Just have to make sure false comes first, otherwise min_ = values.front() will choose the wrong value. So yeah, I guess I'll just drop this patch. Thanks, Paul > > What is the purpose of having to specify a default if only one value > is available ? Does it even make sense to have a ControlInfo that only If we only have ControlInfo({true}) with no second parameter, then it'll use the ControlInfo(Span<const ControlValue> values, const ControlValue &def = {}) constructor. > represents true or false ? > > Can you make sure > ControlInfo({true}, false); > doesn't happen ? atm, not really. > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > New in v3 > > --- > > include/libcamera/controls.h | 1 + > > src/libcamera/controls.cpp | 20 ++++++++++++++++++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index 1bc958a4..2dd147c8 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -272,6 +272,7 @@ public: > > const ControlValue &def = 0); > > explicit ControlInfo(Span<const ControlValue> values, > > const ControlValue &def = {}); > > + explicit ControlInfo(Span<const bool> values, bool def); > > The explicit keyword is used to disallow implicit conversion and copy > construction. It only applies to constructors with a single parameter > afaict, so it is not required here. > > > > > const ControlValue &min() const { return min_; } > > const ControlValue &max() const { return max_; } > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 34317fa0..e32e22e2 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > > values_.push_back(value); > > } > > > > +/** > > + * \brief Construct a ControlInfo from a list of valid boolean values > > + * \param[in] values The control valid boolean vaalues > > s/vaalues/values > > The values can be just {true, false}, right ? > > > + * \param[in] def The control default boolean value > > + * > > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo > > + * minimum and maximum values are set to the first and last members of the > > + * values list respectively. The default value is set to \a def. > > So ControlInfo({true, false}) != ControlInfo({false, true}) ? > > > + */ > > +ControlInfo::ControlInfo(Span<const bool> values, bool def) > > + : def_(def) > > +{ > > + min_ = values.front(); > > + max_ = values.back(); > > + > > + values_.reserve(2); > > + for (const bool &value : values) > > + values_.push_back(value); > > +} > > + > > Could you point me to where this constructor is used, as it seems > suspicious to me :) The pipeline handler uses this constructor to specify the valid values for boolean controls, like AeEnable, AeLock, and AwbLock in this series. > > > > /** > > * \fn ControlInfo::min() > > * \brief Retrieve the minimum value of the control > > -- > > 2.27.0 > >
Hi Paul, Thank you for the patch. On Tue, Jul 06, 2021 at 07:22:55PM +0900, paul.elder@ideasonboard.com wrote: > On Mon, Jul 05, 2021 at 03:39:00PM +0200, Jacopo Mondi wrote: > > On Fri, Jul 02, 2021 at 07:37:45PM +0900, Paul Elder wrote: > > > It would be convenient to be able to iterate over available boolean > > > values, for example for controls that designate if some function can be > > > enabled/disabled. The current min/max/def constructor is insufficient, > > > as .values() is empty, so the values cannot be easily iterated over, and > > > creating a Span of booleans does not work for the values constructor. > > > > > > Add a new constructor to ControlInfo that takes a Span of booleans (to > > > allow specifiying one or two available values), and a default. The > > > default value is not optional, as it doesn't make sense to have a silent > > > default for boolean values. > > > > So this is intended to be used as > > ControlInfo({true}, true); > > ControlInfo({false}, false); > > ControlInfo({true, true}, true); > > ControlInfo({false, true}, true) > > > ControlInfo({true, true}, false); > > ControlInfo({false, true}, false) > > > > ? > > Yeah, that was the idea. tbh I don't feel so well about it but I'm not > sure how else to it it. > > Although, since we have to declare a Span when we use it anyway, maybe > we can just use the exsiting Span constructor. Just have to make sure > false comes first, otherwise min_ = values.front() will choose the wrong > value. You could have a special case in the constructor when the type is bool, and sort the values. I'm not sure if it's worth it though. Now that I think about it, for enumerated types, would it make sense to pass an std::set<ControlValue> to the ControlInfo constructor ? It would require adding a comparison operator to the ControlValue class though. > So yeah, I guess I'll just drop this patch. > > > What is the purpose of having to specify a default if only one value > > is available ? Does it even make sense to have a ControlInfo that only > > If we only have ControlInfo({true}) with no second parameter, then it'll > use the ControlInfo(Span<const ControlValue> values, const ControlValue > &def = {}) constructor. > > > represents true or false ? > > > > Can you make sure > > ControlInfo({true}, false); > > doesn't happen ? > > atm, not really. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > New in v3 > > > --- > > > include/libcamera/controls.h | 1 + > > > src/libcamera/controls.cpp | 20 ++++++++++++++++++++ > > > 2 files changed, 21 insertions(+) > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index 1bc958a4..2dd147c8 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -272,6 +272,7 @@ public: > > > const ControlValue &def = 0); > > > explicit ControlInfo(Span<const ControlValue> values, > > > const ControlValue &def = {}); > > > + explicit ControlInfo(Span<const bool> values, bool def); > > > > The explicit keyword is used to disallow implicit conversion and copy > > construction. It only applies to constructors with a single parameter > > afaict, so it is not required here. > > > > > > > > const ControlValue &min() const { return min_; } > > > const ControlValue &max() const { return max_; } > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 34317fa0..e32e22e2 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, > > > values_.push_back(value); > > > } > > > > > > +/** > > > + * \brief Construct a ControlInfo from a list of valid boolean values > > > + * \param[in] values The control valid boolean vaalues > > > > s/vaalues/values > > > > The values can be just {true, false}, right ? > > > > > + * \param[in] def The control default boolean value > > > + * > > > + * Construct a ControlInfo from a list of valid boolean values. The ControlInfo > > > + * minimum and maximum values are set to the first and last members of the > > > + * values list respectively. The default value is set to \a def. > > > > So ControlInfo({true, false}) != ControlInfo({false, true}) ? > > > > > + */ > > > +ControlInfo::ControlInfo(Span<const bool> values, bool def) > > > + : def_(def) > > > +{ > > > + min_ = values.front(); > > > + max_ = values.back(); > > > + > > > + values_.reserve(2); > > > + for (const bool &value : values) > > > + values_.push_back(value); > > > +} > > > + > > > > Could you point me to where this constructor is used, as it seems > > suspicious to me :) > > The pipeline handler uses this constructor to specify the valid values > for boolean controls, like AeEnable, AeLock, and AwbLock in this series. > > > > /** > > > * \fn ControlInfo::min() > > > * \brief Retrieve the minimum value of the control
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 1bc958a4..2dd147c8 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -272,6 +272,7 @@ public: const ControlValue &def = 0); explicit ControlInfo(Span<const ControlValue> values, const ControlValue &def = {}); + explicit ControlInfo(Span<const bool> values, bool def); const ControlValue &min() const { return min_; } const ControlValue &max() const { return max_; } diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 34317fa0..e32e22e2 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -514,6 +514,26 @@ ControlInfo::ControlInfo(Span<const ControlValue> values, values_.push_back(value); } +/** + * \brief Construct a ControlInfo from a list of valid boolean values + * \param[in] values The control valid boolean vaalues + * \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(Span<const bool> values, bool def) + : def_(def) +{ + min_ = values.front(); + max_ = values.back(); + + values_.reserve(2); + for (const bool &value : values) + values_.push_back(value); +} + /** * \fn ControlInfo::min() * \brief Retrieve the minimum value of the control
It would be convenient to be able to iterate over available boolean values, for example for controls that designate if some function can be enabled/disabled. The current min/max/def constructor is insufficient, as .values() is empty, so the values cannot be easily iterated over, and creating a Span of booleans does not work for the values constructor. Add a new constructor to ControlInfo that takes a Span of booleans (to allow specifiying one or two available values), and a default. The default value is not optional, as it doesn't make sense to have a silent default for boolean values. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- New in v3 --- include/libcamera/controls.h | 1 + src/libcamera/controls.cpp | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+)