[libcamera-devel] [PATCH v2 03/13] libcamera: controls: Add supported values to ControlInfo

[Kieran Bingham]

On 21/10/2020 13:47, Kieran Bingham wrote:
> Hi Jacopo,
> 
> On 20/10/2020 19:05, Jacopo Mondi wrote:
>> Add to the ControlInfo class a list of supported values that can be
>> provided at construction time and retrieved through an accessor method.
>>
>> This is meant to support controls that have an enumerated list of
>> supported values.
>>
>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>> ---
>>  include/libcamera/controls.h |  5 ++++-
>>  src/libcamera/controls.cpp   | 22 +++++++++++++++++++---
>>  2 files changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index 80944efc133a..d1f6d4490c35 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -267,11 +267,13 @@ class ControlInfo
>>  public:
>>  	explicit ControlInfo(const ControlValue &min = 0,
>>  			     const ControlValue &max = 0,
>> -			     const ControlValue &def = 0);
>> +			     const ControlValue &def = 0,
>> +			     const std::vector<ControlValue> &values = {});
>>  
>>  	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_; }
>>  
>>  	std::string toString() const;
>>  
>> @@ -289,6 +291,7 @@ private:
>>  	ControlValue min_;
>>  	ControlValue max_;
>>  	ControlValue def_;
>> +	std::vector<ControlValue> values_;
>>  };
>>  
>>  using ControlIdMap = std::unordered_map<unsigned int, const ControlId *>;
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> index dca782667d88..61feee37a1b8 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -479,15 +479,17 @@ void ControlValue::reserve(ControlType type, bool isArray, std::size_t numElemen
>>   */
>>  
>>  /**
>> - * \brief Construct a ControlInfo with minimum and maximum range parameters
>> + * \brief Construct a ControlInfo with parameters
>>   * \param[in] min The control minimum value
>>   * \param[in] max The control maximum value
>>   * \param[in] def The control default value
>> + * \param[in] values The control supported values
> 
> How does this interact with min/max? I presume they should still get set
> by the min/max of the values in the vector?
> 
> 
> Perhaps there should be another Constructor for ControlInfo which takes
> a set of values, and a default instead?
> 
> And then, the min/max would be set to the min/max of the values
> provided...? or perhaps ignored?
> 
> I assume if there is a list of values given, then the min/max is a
> little irrelevant?

If only I could see the future, then old me here, would know that new me
now would find the suggestion already completed in the next patch.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> 
>>   */
>>  ControlInfo::ControlInfo(const ControlValue &min,
>>  			 const ControlValue &max,
>> -			 const ControlValue &def)
>> -	: min_(min), max_(max), def_(def)
>> +			 const ControlValue &def,
>> +			 const std::vector<ControlValue> &values)
>> +	: min_(min), max_(max), def_(def), values_(values)
>>  {
>>  }
>>  
>> @@ -519,6 +521,20 @@ ControlInfo::ControlInfo(const ControlValue &min,
>>   * \return A ControlValue with the default value for the control
>>   */
>>  
>> +/**
>> + * \fn ControlInfo::values()
>> + * \brief Retrieve the values supported by the control
>> + *
>> + * For controls that support a pre-defined number of values, the enumeration of
>> + * those is reported through a vector of ControlValue instances accessible with
>> + * this method.
>> + *
>> + * If the control reports a list of supported values, setting values outside
>> + * of the reported ones results in undefined behaviour.
>> + *
>> + * \return A vector of ControlValue instances with the supported values
>> + */
>> +
>>  /**
>>   * \brief Provide a string representation of the ControlInfo
>>   */
>>
> 

-- 
Regards
--
Kieran