[PATCH v2 2/2] Revert "controls: Add boolean constructors for ControlInfo"

[Stefan Klug]

Hi Barnabás,

Thank you for the patch.

Quoting Barnabás Pőcze (2025-05-19 11:12:07)
> This reverts commit 10cdc914dad282b4ca0ad11067d5c6d446af1fcc.
> 
> The contstructors introduced by that commit are not used anywhere,
> and they do not match the existing practice for boolean controls.
> 
> Specifically, every single boolean control is described by calling
> the `ControlInfo(ControlValue, ControlValue, ControlValue)`
> constructor. Crucially, that constructor does not set `values_`,
> while the two removed contructors do. And whether or not `values_`
> has any elements is currently used as an implicit sign to decide
> whether or not the control is "enum-like", and those are assumed
> to have type `int32_t`.
> 
> For example, any boolean control described using any of the two
> removed constructors would cause an assertion in failure in
> `CameraSession::listControls()` when calling `value.get<int32_t>()`.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

Reading the initial commit I don't really understand the original
reason. But as these constructors are not used anymore, the revert seems
reasonable.

Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 

Cheers,
Stefan

> ---
> changes in v2:
>   * keep tests
> ---
>  include/libcamera/controls.h   |  3 ---
>  src/libcamera/controls.cpp     | 29 -----------------------------
>  test/controls/control_info.cpp |  9 ++++-----
>  3 files changed, 4 insertions(+), 37 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index 2ae4ec3d4..32fb31f78 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -10,7 +10,6 @@
>  #include <assert.h>
>  #include <map>
>  #include <optional>
> -#include <set>
>  #include <stdint.h>
>  #include <string>
>  #include <unordered_map>
> @@ -334,8 +333,6 @@ public:
>                              const ControlValue &def = {});
>         explicit ControlInfo(Span<const ControlValue> values,
>                              const ControlValue &def = {});
> -       explicit ControlInfo(std::set<bool> values, bool def);
> -       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 70f6f6092..eaa34e70b 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -625,35 +625,6 @@ 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
> - *
> - * 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);
> -}
> -
> -/**
> - * \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 e1bb43f0e..905d92dd5 100644
> --- a/test/controls/control_info.cpp
> +++ b/test/controls/control_info.cpp
> @@ -50,7 +50,7 @@ protected:
>                  * Test information retrieval from a control with boolean
>                  * values.
>                  */
> -               ControlInfo aeEnable({ false, true }, false);
> +               ControlInfo aeEnable(false, true, false);
> 
>                 if (aeEnable.min().get<bool>() != false ||
>                     aeEnable.def().get<bool>() != false ||
> @@ -59,13 +59,12 @@ protected:
>                         return TestFail;
>                 }
> 
> -               if (aeEnable.values()[0].get<bool>() != false ||
> -                   aeEnable.values()[1].get<bool>() != true) {
> +               if (!aeEnable.values().empty()) {
>                         cout << "Invalid control values for AeEnable" << endl;
>                         return TestFail;
>                 }
> 
> -               ControlInfo awbEnable(true);
> +               ControlInfo awbEnable(true, true, true);
> 
>                 if (awbEnable.min().get<bool>() != true ||
>                     awbEnable.def().get<bool>() != true ||
> @@ -74,7 +73,7 @@ protected:
>                         return TestFail;
>                 }
> 
> -               if (awbEnable.values()[0].get<bool>() != true) {
> +               if (!awbEnable.values().empty()) {
>                         cout << "Invalid control values for AwbEnable" << endl;
>                         return TestFail;
>                 }
> --
> 2.49.0