[libcamera-devel] [PATCH v2 04/13] libcamera: controls: Construct from values list

[Kieran Bingham]

Hi Jacopo,

On 21/10/2020 14:31, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Wed, Oct 21, 2020 at 01:50:12PM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>>
>> On 20/10/2020 19:05, Jacopo Mondi wrote:
>>> Add a constructor to the ControlInfo class that allows creating
>>> a class instance from the list of the control supported values.
>>>
>>
>> I see.
>>
>> please ignore the request in my previous patch ;-) hehe
>>
>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>  include/libcamera/controls.h |  1 +
>>>  src/libcamera/controls.cpp   | 17 +++++++++++++++++
>>>  2 files changed, 18 insertions(+)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index d1f6d4490c35..d4fdf5807f1c 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -269,6 +269,7 @@ public:
>>>  			     const ControlValue &max = 0,
>>>  			     const ControlValue &def = 0,
>>>  			     const std::vector<ControlValue> &values = {});
>>> +	explicit ControlInfo(const std::vector<ControlValue> &values);
>>>
>>>  	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 61feee37a1b8..e80f6116a684 100644
>>> --- a/src/libcamera/controls.cpp
>>> +++ b/src/libcamera/controls.cpp
>>> @@ -493,6 +493,23 @@ ControlInfo::ControlInfo(const ControlValue &min,
>>>  {
>>>  }
>>>
>>> +/**
>>> + * \brief Construct a ControlInfo from the list of supported values
>>> + * \param[in] values The control supported values
>>> + *
>>> + * Construct a ControlInfo from a list of supported values. The ControlInfo
>>> + * minimum and maximum values are assigned to the first and last members of
>>> + * the values list respectively. The ControlInfo default value is set to be
>>> + * equal to the minimum one.
>>> + */
>>> +ControlInfo::ControlInfo(const std::vector<ControlValue> &values)
>>> +	: values_(values)
>>> +{
>>> +	min_ = values_.front();
>>> +	max_ = values_.back();
>>
>> That feels like a bit of an assumption that the list of values is sorted.
>>
>> Maybe there should be at least some extra checks here?
>>
>> Or just start with a values.sort() ?
> 
> Well, enums are defined in the .yaml file, we're in control of them,
> aren't we ?

Hrm ... we are indeed putting them in the yaml ... I'm still a bit weary
- but indeed that allays my initial fear.

I would have suggested we could go one step further and make the
generator script give the enums their values ... but there might be
cases were we want to give explicit values ... so that gets complicated.

Ok, so leave this as is for now. If we ever hit an unsorted/unordered
list I'll try really hard not to say I told you so ;-)

(but you're right, I don't think that should happen) hehe

>>
>>
>>> +	def_ = min_;
>>
>> And I wonder if this should be (optionally?) supplied in the constructor
>> arguments?
> 
> This might be worth it, I agree.
> 
>>
>> Anyway, with or without those comments:
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>> +}
>>> +
>>>  /**
>>>   * \fn ControlInfo::min()
>>>   * \brief Retrieve the minimum value of the control
>>>
>>
>> --
>> Regards
>> --
>> Kieran

-- 
Regards
--
Kieran