[libcamera-devel,v2,04/13] libcamera: controls: Construct from values list
diff mbox series

Message ID 20201020180534.36855-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Introduce draft controls
Related show

Commit Message

Jacopo Mondi Oct. 20, 2020, 6:05 p.m. UTC
Add a constructor to the ControlInfo class that allows creating
a class instance from the list of the control supported values.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h |  1 +
 src/libcamera/controls.cpp   | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

Comments

Kieran Bingham Oct. 21, 2020, 12:50 p.m. UTC | #1
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@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() ?


> +	def_ = min_;

And I wonder if this should be (optionally?) supplied in the constructor
arguments?

Anyway, with or without those comments:

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

> +}
> +
>  /**
>   * \fn ControlInfo::min()
>   * \brief Retrieve the minimum value of the control
>
Jacopo Mondi Oct. 21, 2020, 1:31 p.m. UTC | #2
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@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 ?
>
>
> > +	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@ideasonboard.com>
>
> > +}
> > +
> >  /**
> >   * \fn ControlInfo::min()
> >   * \brief Retrieve the minimum value of the control
> >
>
> --
> Regards
> --
> Kieran
Kieran Bingham Oct. 21, 2020, 1:37 p.m. UTC | #3
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@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@ideasonboard.com>
>>
>>> +}
>>> +
>>>  /**
>>>   * \fn ControlInfo::min()
>>>   * \brief Retrieve the minimum value of the control
>>>
>>
>> --
>> Regards
>> --
>> Kieran

Patch
diff mbox series

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();
+	def_ = min_;
+}
+
 /**
  * \fn ControlInfo::min()
  * \brief Retrieve the minimum value of the control