[libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls: Construct from a list of ids

[Niklas Söderlund]

On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:
> > > Add a constructor for the V4L2ControlList class that accepts a list of
> > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList
> > > instance to be used for reading controls only.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/include/v4l2_controls.h |  3 +++
> > >  src/libcamera/v4l2_controls.cpp       | 17 +++++++++++++++++
> > >  2 files changed, 20 insertions(+)
> > >
> > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > > index 10b726504951..86c84e06d741 100644
> > > --- a/src/libcamera/include/v4l2_controls.h
> > > +++ b/src/libcamera/include/v4l2_controls.h
> > > @@ -65,6 +65,9 @@ public:
> > >  	using iterator = std::vector<V4L2Control>::iterator;
> > >  	using const_iterator = std::vector<V4L2Control>::const_iterator;
> > >
> > > +	V4L2ControlList() {}
> > > +	V4L2ControlList(std::vector<unsigned int> ids);
> > > +
> > >  	iterator begin() { return controls_.begin(); }
> > >  	const_iterator begin() const { return controls_.begin(); }
> > >  	iterator end() { return controls_.end(); }
> > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > > index 84258d9954d0..eeb325cbfff9 100644
> > > --- a/src/libcamera/v4l2_controls.cpp
> > > +++ b/src/libcamera/v4l2_controls.cpp
> > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > >   * and prepare to be re-used for a new control write/read sequence.
> > >   */
> > >
> > > +/**
> > > + * \brief Construct a V4L2ControlList from a list of V4L2 controls identifiers
> > > + * \param ids A list of V4L2 control identifiers (V4L2_CID_*)
> > > + *
> > > + * Construct a V4L2ControlList from a list of control identifiers without any
> > > + * value associated. This constructor is particularly useful to create a
> > > + * V4L2ControlList that is used to read the values of all controls in the
> > > + * \a ids list. The created V4L2ControlList should not be used to write control
> > > + * values unless it is cleared first and then controls with an associated value
> > > + * are manually added to it.
> > > + */
> >
> > Do you think it would be worth it to add some code to enforce that no
> > controls can be written if it's initialized with this constructor?
> >
> 
> How would you do so? The most naive implementation that comes to mind
> it's to add a 'writable' flag to initialize to true only if the
> control has a value assigned and check for the flag
> V4L2Device::setControls()

I'm not sure how to best implement it, but the constraint is so specific 
it looked like something that could be described in code. Now that I dig 
thru the code with fresh eyes I can't find the rational for the 
V4L2ControlList needs to be cleared before it's attempted to be used for 
writing.

The control values are initialized to 0 using this new constructor and 
if used for writing V4L2Device::setControls() will validate that the 
control exists on the device and bail before writing anything if one the 
controls do not exist.

What reason do you think there is for the need to manually add controls 
with a specific value before writing? This is just a specialisation 
where all values are initialized to 0, at least in my mind I might be 
missing something ;-)

> 
> > > +V4L2ControlList::V4L2ControlList(std::vector<unsigned int> ids)
> > > +{
> > > +	for (auto id : ids)
> > > +		add(id);
> > > +}
> > > +
> > >  /**
> > >   * \typedef V4L2ControlList::iterator
> > >   * \brief Iterator on the V4L2 controls contained in the instance
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund