[libcamera-devel,v5,1/6] libcamera: Add V4L2Controls

1484 diff mbox series

Add Libcamera V4L2 control support, implemented using the V4L2 Extended
Control APIs. This patch defines the types used to define and manage controls.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/v4l2_controls.h |  86 ++++++++
 src/libcamera/meson.build             |   1 +
 src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
 3 files changed, 369 insertions(+)
 create mode 100644 src/libcamera/include/v4l2_controls.h
 create mode 100644 src/libcamera/v4l2_controls.cpp

Hi Jacopo,

Thank you for the patch.

On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> Add Libcamera V4L2 control support, implemented using the V4L2 Extended

s/Libcamera/libcamera/ :-)

> Control APIs. This patch defines the types used to define and manage controls.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/v4l2_controls.h |  86 ++++++++
>  src/libcamera/meson.build             |   1 +
>  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
>  3 files changed, 369 insertions(+)
>  create mode 100644 src/libcamera/include/v4l2_controls.h
>  create mode 100644 src/libcamera/v4l2_controls.cpp
> 
> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> new file mode 100644
> index 000000000000..acd23dabd831
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_controls.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.h - V4L2 Controls Support
> + */
> +
> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> +
> +#include <string>
> +#include <vector>
> +
> +#include <stdint.h>
> +
> +#include <linux/v4l2-controls.h>
> +#include <linux/videodev2.h>
> +
> +namespace libcamera {
> +
> +class V4L2ControlInfo
> +{
> +public:
> +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> +
> +	unsigned int id() const { return id_; }
> +	unsigned int type() const { return type_; }
> +	size_t size() const { return size_; }
> +	const std::string &name() const { return name_; }
> +
> +private:
> +	unsigned int type_;
> +	std::string name_;
> +	unsigned int id_;
> +	size_t size_;

As requested by Niklas,

	unsigned int id_;
	unsigned int type_;
	size_t size_;
	std::string name_;

?

> +};
> +
> +class V4L2Control
> +{
> +public:
> +	V4L2Control(unsigned int id, int value = 0)
> +		: id_(id), value_(value) {}
> +
> +	int64_t value() const { return value_; }
> +	void setValue(int64_t value) { value_ = value; }
> +
> +	unsigned int id() const { return id_; }
> +
> +private:
> +	unsigned int id_;
> +	int64_t value_;
> +};
> +
> +class V4L2Controls

I was wondering if we should rename this to V4L2ControlList, as
V4L2Controls is very close to V4L2Control and easy to mistake.

> +{
> +public:
> +	V4L2Controls &operator=(const V4L2Controls &) = delete;

As commented before, I think you should either delete both the
assignement operator and the copy constructor, or neither of them.

> +
> +	using iterator = std::vector<V4L2Control>::iterator;
> +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> +
> +	iterator begin() { return controls_.begin(); }
> +	const_iterator begin() const { return controls_.begin(); }
> +	iterator end() { return controls_.end(); }
> +	const_iterator end() const { return controls_.end(); }
> +
> +	bool empty() const { return controls_.empty(); }
> +	size_t size() const { return controls_.size(); }

std::size_t to match std::vector ?

> +
> +	void reset() { controls_.clear(); }
> +	void add(unsigned int id, int64_t value = 0);
> +
> +	V4L2Control *operator[](unsigned int index)
> +	{
> +		return &controls_[index];
> +	}

Do we need to access a control by index for any other purpose than
iterating over the controls, which the iterator already gives us ?
Couldn't operator[] take the control ID, and getControl() be removed ?

> +
> +	V4L2Control *getControl(unsigned int id);
> +
> +private:
> +	std::vector<V4L2Control> controls_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index f26ad5b2dc57..985aa7e8ab0e 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -23,6 +23,7 @@ libcamera_sources = files([
>      'stream.cpp',
>      'timer.cpp',
>      'utils.cpp',
> +    'v4l2_controls.cpp',
>      'v4l2_device.cpp',
>      'v4l2_subdevice.cpp',
>      'v4l2_videodevice.cpp',
> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> new file mode 100644
> index 000000000000..abf596e7f1ef
> --- /dev/null
> +++ b/src/libcamera/v4l2_controls.cpp
> @@ -0,0 +1,282 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * v4l2_controls.cpp - V4L2 Controls Support
> + */
> +
> +#include "v4l2_controls.h"
> +
> +/**
> + * \file v4l2_controls.h
> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.

s/\.$//

> + *
> + * The V4L2 defined "Control API" allows application to inspect and modify set

"The V4L2 Control API allows applications ..."

s/set/sets/

> + * of configurable parameters on the video device or subdevice of interest. The

s/on the video device or subdevice of interest/on a video device or subdevice/

> + * nature of the parameters an application could modify using the control

s/could/can/

> + * framework depends on what the driver implements support for, and on the
> + * characteristics of the underlying hardware platform. Generally controls are
> + * used to modify user visible settings, such as the image brightness and
> + * exposure time, or non-standard parameters which cannot be controlled through
> + * the V4L2 format negotiation API.

The last two sentences describe more the V4L2 API than the V4L2Control*
classes API. I expect users of this code to know about V4L2 already, so
they could be dropped, but I also don't mind if you keep them. Just
don't invest too much time in writing document about V4L2 itself, as
that's a bit out of scope.

> + *
> + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> + * and have an associated type. Each control has a value, which is the data
> + * that can be modified with V4L2Device::setControls() or retrieved with
> + * V4L2Device::getControls().
> + *
> + * The control's type along with the control's flags defines the type of the

s/defines/define/

> + * control's value content. Controls might transport a single data value

s/might/can/

> + * stored in variable inside the control, or they might as well deal with more

s/might/can/

> + * complex data types, such as arrays of matrices, stored in a contiguous
> + * memory locations associated with the control and called 'the payload'. Such
> + * controls are called 'compound controls' and are currently not supported by
> + * the libcamera V4L2 control framework.

Here too you could simply have said that we don't support compound
controls yet, but I don't mind keeping the more detailed text as-is.

> + *
> + * libcamera implements support for controls using the V4L2 'Extended Control'
> + * API, which allows future handling of controls with payloads of arbitrary

It's not the controls that are extended, it's the API. It's the extended
V4L2 control API, not the V4L2 API for extended controls. I would thus
remove the quotes.

> + * sizes.
> + *
> + * The libcamera V4L2 Controls framework operates on lists of controls,
> + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.

Calling it V4L2ControlList would match the description nicely :-)

> + * The interface to set and get control is implemented by the V4L2Device
> + * class, and this file only provides the data type definitions.
> + *
> + * \todo Add support for compound controls
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \class V4L2ControlInfo
> + * \brief Information on a V4L2 control
> + *
> + * The V4L2ControlInfo class represent all the information related to

s/represent/represents/

> + * a V4L2 control, such as its ID, its type, its user-readable name and the
> + * expected size of its value data.
> + *
> + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> + *
> + * This class does not contain the control value, but only static information
> + * on the control, which shall be cached by the caller at initialisation time
> + * or the first time the control information is accessed.
> + */
> +
> +/**
> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> + */
> +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> +{
> +	id_ = ctrl.id;
> +	type_ = ctrl.type;
> +	name_ = static_cast<const char *>(ctrl.name);
> +	size_ = ctrl.elem_size * ctrl.elems;
> +}
> +
> +/**
> + * \fn V4L2ControlInfo::id()
> + * \brief Retrieve the control ID
> + * \return The V4L2 control ID
> + */
> +
> +/**
> + * \fn V4L2ControlInfo::type()
> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> + * \return The V4L2 control type
> + */
> +
> +/**
> + * \fn V4L2ControlInfo::size()
> + * \brief Retrieve the control value data size (in bytes)
> + * \return The V4L2 control value data size
> + */
> +
> +/**
> + * \fn V4L2ControlInfo::name()
> + * \brief Retrieve the control user readable name
> + * \return The V4L2 control user readable name
> + */
> +
> +/**
> + * \class V4L2Control
> + * \brief A V4L2 control value
> + *
> + * The V4L2Control class represent the value of a V4L2 control.
> + * The class stores values that have been read from or will be applied to
> + * a V4L2 device.
> + *
> + * The value stored in the class instances does not reflect what is actually
> + * applied to the hardware but is a pure software cache optionally initialized
> + * at control creation time and only modified by a control read operation.

With setControls() updating the value, this isn't true anymore.

> + *
> + * The write and read controls the V4L2Control class instances are not meant
> + * to be directly used but are instead intended to be grouped in
> + * V4L2Controls instances, which are then passed as parameters to
> + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> + */
> +
> +/**
> + * \fn V4L2Control::V4L2Control
> + * \brief Construct a V4L2 control with ID \a id and value \a value

"with an \a id and a \a value" ? Or "Construct an instance of control \a
ID with a \a value" ?

> + * \param id The V4L2 control ID
> + * \param value The control value
> + */
> +
> +/**
> + * \fn V4L2Control::value()
> + * \brief Retrieve the value of the control
> + *
> + * It is important to notice that the returned value is not read from the

s/notice/note/

> + * hardware at the time this operation is called, but it's the cached value
> + * obtained the most recent time the control has been read with
> + * V4L2Device::getControls() or the value of the control has been set to by the
> + * user with the add() operation.

You should mention setControls() here. How about

This method returns the cached control value, initially set by
V4L2Controls::add() and then updated when the controls are read or
written with V4L2Device::getControls() and V4L2Device::setControls().

> + *
> + * \return The V4L2 control value. The value might be 0 if the control has
> + * never been read from the device before this operation is called.

I would drop the second sentence, it's redundant with the above
paragraph, and a bit confusing too as the value may also be 0 after
being read.

> + */
> +
> +/**
> + * \fn V4L2Control::setValue()
> + * \brief Set the value of the control
> + * \param value The new V4L2 control value
> + *
> + * It is important to notice that the value is not applied to the
> + * hardware at the time this operation is called, but it will only
> + * be actually applied by calling V4L2Device::setControls().

Let's simplify it a bit, by explaining what the method does, not what it
does not do :-)

"This method stores the control value, which will be applied to the
device when calling V4L2Device::setControls()."

> + */
> +
> +/**
> + * \fn V4L2Control::id()
> + * \brief Retrieve the control ID this instance refers to
> + * \return The V4L2Control ID
> + */
> +
> +/**
> + * \class V4L2Controls
> + * \brief Container of V4L2Control instances
> + *
> + * The V4L2Controls class works as a container for a list of V4L2Control
> + * instances. The class provides operations to add a new control to the list,
> + * get back a control value, and reset the list of controls it contains.
> + *
> + * In order to set and get controls, user of the libcamera V4L2 control
> + * framework should operate on instances of the V4L2Controls class, and use
> + * them as argument for the V4L2Device::setControls() and
> + * V4L2Device::getControls() operations, which write and read a list of
> + * controls from or to a V4L2 device (a video device or a subdevice).

"to and from" (as it's "write and read")

> + *
> + * Controls are added to a V4L2Controls instance with the add() method, with
> + * or without an initial value.
> + *
> + * To write controls to a device, the controls of interest shall be added with
> + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t

s/an initial value/a value/

> + * value) to prepare for a write operation. Once the values of all controls of
> + * interest have been added, the V4L2Controls instance is passed to the
> + * V4L2Device::setControls(), which sets the controls on the device.
> + *
> + * To read controls from a device, the desired controls are added with
> + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> + * reads the controls from the device and updates the values stored in
> + * V4L2Controls.
> + *
> + * V4L2Controls instances can be reset to remove all controls they contain and
> + * prepare to be re-used for a new control write/read sequence.
> + */
> +
> +/**
> + * \typedef V4L2Controls::iterator
> + * \brief Iterator on the V4L2 controls contained in the instance
> + */
> +
> +/**
> + * \typedef V4L2Controls::const_iterator
> + * \brief Const iterator on the V4L2 controls contained in the instance
> + */
> +
> +/**
> + * \fn iterator V4L2Controls::begin()
> + * \brief Retrieve an iterator to the first V4L2Control in the instance
> + * \return An iterator to the first V4L2 control
> + */
> +
> +/**
> + * \fn const_iterator V4L2Controls::begin() const
> + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> + * \return A constant iterator to the first V4L2 control
> + */
> +
> +/**
> + * \fn iterator V4L2Controls::end()
> + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> + * instance
> + * \return An iterator to the element following the last V4L2 control in the
> + * instance
> + */
> +
> +/**
> + * \fn const_iterator V4L2Controls::end() const
> + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> + * in the instance
> + * \return A constant iterator to the element following the last V4L2 control
> + * in the instance
> + */
> +
> +/**
> + * \fn V4L2Controls::empty()
> + * \brief Verify if the instance does not contain any control
> + * \return True if the instance does not contain any control, false otherwise
> + */
> +
> +/**
> + * \fn V4L2Controls::size()
> + * \brief Retrieve the number on controls in the instance
> + * \return The number of V4L2Control stored in the instance
> + */
> +
> +/**
> + * \fn V4L2Controls::reset()
> + * \brief Removes all controls in the instance

s/Removes/Remove/

> + */
> +
> +/**
> + * \brief Add control with ID \a id and optional \a value to the instance
> + * \param id The V4L2 control ID (V4L2_CID_*)
> + * \param value The V4L2 control value
> + *
> + * V4L2Control can be added with an explicit value, or without one. In that
> + * case the control's value is defaulted to 0.

s/is defaulted/defaults to/

But I think we can skip the sentence, the doxygen documentation should
show the = 0.

> + */
> +void V4L2Controls::add(unsigned int id, int64_t value)
> +{
> +	controls_.emplace_back(id, value);
> +}
> +
> +/**
> + * \fn V4L2Controls::operator[](unsigned int index)
> + * \brief Access the control at index \a index
> + * \param index The index to access
> + * \return The V4L2 control at index \a index
> + */
> +
> +/**
> + * \brief Get a pointer the control with ID \a id
> + * \param id The V4L2 control ID (V4L2_CID_xx)
> + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> + * control ID is not part of this instance.
> + */
> +V4L2Control *V4L2Controls::getControl(unsigned int id)
> +{
> +	for (V4L2Control &ctrl : controls_) {
> +		if (ctrl.id() == id)
> +			return &ctrl;
> +	}
> +
> +	return nullptr;
> +}
> +
> +} /* namespace libcamera */

Hi Laurent,

On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> > Add Libcamera V4L2 control support, implemented using the V4L2 Extended
>
> s/Libcamera/libcamera/ :-)
>
> > Control APIs. This patch defines the types used to define and manage controls.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/v4l2_controls.h |  86 ++++++++
> >  src/libcamera/meson.build             |   1 +
> >  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
> >  3 files changed, 369 insertions(+)
> >  create mode 100644 src/libcamera/include/v4l2_controls.h
> >  create mode 100644 src/libcamera/v4l2_controls.cpp
> >
> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> > new file mode 100644
> > index 000000000000..acd23dabd831
> > --- /dev/null
> > +++ b/src/libcamera/include/v4l2_controls.h
> > @@ -0,0 +1,86 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.h - V4L2 Controls Support
> > + */
> > +
> > +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> > +#define __LIBCAMERA_V4L2_CONTROLS_H__
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <stdint.h>
> > +
> > +#include <linux/v4l2-controls.h>
> > +#include <linux/videodev2.h>
> > +
> > +namespace libcamera {
> > +
> > +class V4L2ControlInfo
> > +{
> > +public:
> > +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> > +
> > +	unsigned int id() const { return id_; }
> > +	unsigned int type() const { return type_; }
> > +	size_t size() const { return size_; }
> > +	const std::string &name() const { return name_; }
> > +
> > +private:
> > +	unsigned int type_;
> > +	std::string name_;
> > +	unsigned int id_;
> > +	size_t size_;
>
> As requested by Niklas,
>
> 	unsigned int id_;
> 	unsigned int type_;
> 	size_t size_;
> 	std::string name_;
>
> ?
>
> > +};
> > +
> > +class V4L2Control
> > +{
> > +public:
> > +	V4L2Control(unsigned int id, int value = 0)
> > +		: id_(id), value_(value) {}
> > +
> > +	int64_t value() const { return value_; }
> > +	void setValue(int64_t value) { value_ = value; }
> > +
> > +	unsigned int id() const { return id_; }
> > +
> > +private:
> > +	unsigned int id_;
> > +	int64_t value_;
> > +};
> > +
> > +class V4L2Controls
>
> I was wondering if we should rename this to V4L2ControlList, as
> V4L2Controls is very close to V4L2Control and easy to mistake.
>
> > +{
> > +public:
> > +	V4L2Controls &operator=(const V4L2Controls &) = delete;
>
> As commented before, I think you should either delete both the
> assignement operator and the copy constructor, or neither of them.
>
> > +
> > +	using iterator = std::vector<V4L2Control>::iterator;
> > +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> > +
> > +	iterator begin() { return controls_.begin(); }
> > +	const_iterator begin() const { return controls_.begin(); }
> > +	iterator end() { return controls_.end(); }
> > +	const_iterator end() const { return controls_.end(); }
> > +
> > +	bool empty() const { return controls_.empty(); }
> > +	size_t size() const { return controls_.size(); }
>
> std::size_t to match std::vector ?
>
> > +
> > +	void reset() { controls_.clear(); }
> > +	void add(unsigned int id, int64_t value = 0);
> > +
> > +	V4L2Control *operator[](unsigned int index)
> > +	{
> > +		return &controls_[index];
> > +	}
>
> Do we need to access a control by index for any other purpose than
> iterating over the controls, which the iterator already gives us ?
> Couldn't operator[] take the control ID, and getControl() be removed ?
>

I really don't like this.
operator[] for vector works on indexes, I would now use the same
method but with a different semantic. It would be confusing for
everyone. I can drop this operation if you like to.

> > +
> > +	V4L2Control *getControl(unsigned int id);
> > +
> > +private:
> > +	std::vector<V4L2Control> controls_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f26ad5b2dc57..985aa7e8ab0e 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -23,6 +23,7 @@ libcamera_sources = files([
> >      'stream.cpp',
> >      'timer.cpp',
> >      'utils.cpp',
> > +    'v4l2_controls.cpp',
> >      'v4l2_device.cpp',
> >      'v4l2_subdevice.cpp',
> >      'v4l2_videodevice.cpp',
> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> > new file mode 100644
> > index 000000000000..abf596e7f1ef
> > --- /dev/null
> > +++ b/src/libcamera/v4l2_controls.cpp
> > @@ -0,0 +1,282 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * v4l2_controls.cpp - V4L2 Controls Support
> > + */
> > +
> > +#include "v4l2_controls.h"
> > +
> > +/**
> > + * \file v4l2_controls.h
> > + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
>
> s/\.$//
>
> > + *
> > + * The V4L2 defined "Control API" allows application to inspect and modify set
>
> "The V4L2 Control API allows applications ..."
>
> s/set/sets/
>
> > + * of configurable parameters on the video device or subdevice of interest. The
>
> s/on the video device or subdevice of interest/on a video device or subdevice/
>
> > + * nature of the parameters an application could modify using the control
>
> s/could/can/
>
> > + * framework depends on what the driver implements support for, and on the
> > + * characteristics of the underlying hardware platform. Generally controls are
> > + * used to modify user visible settings, such as the image brightness and
> > + * exposure time, or non-standard parameters which cannot be controlled through
> > + * the V4L2 format negotiation API.
>
> The last two sentences describe more the V4L2 API than the V4L2Control*
> classes API. I expect users of this code to know about V4L2 already, so
> they could be dropped, but I also don't mind if you keep them. Just
> don't invest too much time in writing document about V4L2 itself, as
> that's a bit out of scope.
>

I feel it doesn't hurt, as it doesn't go in great lenght describing
controls but I'll drop.

> > + *
> > + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> > + * and have an associated type. Each control has a value, which is the data
> > + * that can be modified with V4L2Device::setControls() or retrieved with
> > + * V4L2Device::getControls().
> > + *
> > + * The control's type along with the control's flags defines the type of the
>
> s/defines/define/
>
> > + * control's value content. Controls might transport a single data value
>
> s/might/can/
>
> > + * stored in variable inside the control, or they might as well deal with more
>
> s/might/can/
>
> > + * complex data types, such as arrays of matrices, stored in a contiguous
> > + * memory locations associated with the control and called 'the payload'. Such
> > + * controls are called 'compound controls' and are currently not supported by
> > + * the libcamera V4L2 control framework.
>
> Here too you could simply have said that we don't support compound
> controls yet, but I don't mind keeping the more detailed text as-is.
>
> > + *
> > + * libcamera implements support for controls using the V4L2 'Extended Control'
> > + * API, which allows future handling of controls with payloads of arbitrary
>
> It's not the controls that are extended, it's the API. It's the extended
> V4L2 control API, not the V4L2 API for extended controls. I would thus
> remove the quotes.
>

I see. I should have moved the ending quote to include the "API" part.

> > + * sizes.
> > + *
> > + * The libcamera V4L2 Controls framework operates on lists of controls,
> > + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
>
> Calling it V4L2ControlList would match the description nicely :-)
>

Ack

> > + * The interface to set and get control is implemented by the V4L2Device
> > + * class, and this file only provides the data type definitions.
> > + *
> > + * \todo Add support for compound controls
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class V4L2ControlInfo
> > + * \brief Information on a V4L2 control
> > + *
> > + * The V4L2ControlInfo class represent all the information related to
>
> s/represent/represents/
>
> > + * a V4L2 control, such as its ID, its type, its user-readable name and the
> > + * expected size of its value data.
> > + *
> > + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> > + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> > + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> > + *
> > + * This class does not contain the control value, but only static information
> > + * on the control, which shall be cached by the caller at initialisation time
> > + * or the first time the control information is accessed.
> > + */
> > +
> > +/**
> > + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> > + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> > + */
> > +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> > +{
> > +	id_ = ctrl.id;
> > +	type_ = ctrl.type;
> > +	name_ = static_cast<const char *>(ctrl.name);
> > +	size_ = ctrl.elem_size * ctrl.elems;
> > +}
> > +
> > +/**
> > + * \fn V4L2ControlInfo::id()
> > + * \brief Retrieve the control ID
> > + * \return The V4L2 control ID
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::type()
> > + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> > + * \return The V4L2 control type
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::size()
> > + * \brief Retrieve the control value data size (in bytes)
> > + * \return The V4L2 control value data size
> > + */
> > +
> > +/**
> > + * \fn V4L2ControlInfo::name()
> > + * \brief Retrieve the control user readable name
> > + * \return The V4L2 control user readable name
> > + */
> > +
> > +/**
> > + * \class V4L2Control
> > + * \brief A V4L2 control value
> > + *
> > + * The V4L2Control class represent the value of a V4L2 control.
> > + * The class stores values that have been read from or will be applied to
> > + * a V4L2 device.
> > + *
> > + * The value stored in the class instances does not reflect what is actually
> > + * applied to the hardware but is a pure software cache optionally initialized
> > + * at control creation time and only modified by a control read operation.
>
> With setControls() updating the value, this isn't true anymore.
>

True. Nice catch.

> > + *
> > + * The write and read controls the V4L2Control class instances are not meant
> > + * to be directly used but are instead intended to be grouped in
> > + * V4L2Controls instances, which are then passed as parameters to
> > + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::V4L2Control
> > + * \brief Construct a V4L2 control with ID \a id and value \a value
>
> "with an \a id and a \a value" ? Or "Construct an instance of control \a
> ID with a \a value" ?
>

I'm always uncertain on how to handle these situation, where the name
of the parameter matches the generic name.

> > + * \param id The V4L2 control ID
> > + * \param value The control value
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::value()
> > + * \brief Retrieve the value of the control
> > + *
> > + * It is important to notice that the returned value is not read from the
>
> s/notice/note/
>
> > + * hardware at the time this operation is called, but it's the cached value
> > + * obtained the most recent time the control has been read with
> > + * V4L2Device::getControls() or the value of the control has been set to by the
> > + * user with the add() operation.
>
> You should mention setControls() here. How about
>
> This method returns the cached control value, initially set by
> V4L2Controls::add() and then updated when the controls are read or
> written with V4L2Device::getControls() and V4L2Device::setControls().
>
> > + *
> > + * \return The V4L2 control value. The value might be 0 if the control has
> > + * never been read from the device before this operation is called.
>
> I would drop the second sentence, it's redundant with the above
> paragraph, and a bit confusing too as the value may also be 0 after
> being read.
>

Ack.

> > + */
> > +
> > +/**
> > + * \fn V4L2Control::setValue()
> > + * \brief Set the value of the control
> > + * \param value The new V4L2 control value
> > + *
> > + * It is important to notice that the value is not applied to the
> > + * hardware at the time this operation is called, but it will only
> > + * be actually applied by calling V4L2Device::setControls().
>
> Let's simplify it a bit, by explaining what the method does, not what it
> does not do :-)
>
> "This method stores the control value, which will be applied to the
> device when calling V4L2Device::setControls()."
>
> > + */
> > +
> > +/**
> > + * \fn V4L2Control::id()
> > + * \brief Retrieve the control ID this instance refers to
> > + * \return The V4L2Control ID
> > + */
> > +
> > +/**
> > + * \class V4L2Controls
> > + * \brief Container of V4L2Control instances
> > + *
> > + * The V4L2Controls class works as a container for a list of V4L2Control
> > + * instances. The class provides operations to add a new control to the list,
> > + * get back a control value, and reset the list of controls it contains.
> > + *
> > + * In order to set and get controls, user of the libcamera V4L2 control
> > + * framework should operate on instances of the V4L2Controls class, and use
> > + * them as argument for the V4L2Device::setControls() and
> > + * V4L2Device::getControls() operations, which write and read a list of
> > + * controls from or to a V4L2 device (a video device or a subdevice).
>
> "to and from" (as it's "write and read")
>
> > + *
> > + * Controls are added to a V4L2Controls instance with the add() method, with
> > + * or without an initial value.
> > + *
> > + * To write controls to a device, the controls of interest shall be added with
> > + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
>
> s/an initial value/a value/
>
> > + * value) to prepare for a write operation. Once the values of all controls of
> > + * interest have been added, the V4L2Controls instance is passed to the
> > + * V4L2Device::setControls(), which sets the controls on the device.
> > + *
> > + * To read controls from a device, the desired controls are added with
> > + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> > + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> > + * reads the controls from the device and updates the values stored in
> > + * V4L2Controls.
> > + *
> > + * V4L2Controls instances can be reset to remove all controls they contain and
> > + * prepare to be re-used for a new control write/read sequence.
> > + */
> > +
> > +/**
> > + * \typedef V4L2Controls::iterator
> > + * \brief Iterator on the V4L2 controls contained in the instance
> > + */
> > +
> > +/**
> > + * \typedef V4L2Controls::const_iterator
> > + * \brief Const iterator on the V4L2 controls contained in the instance
> > + */
> > +
> > +/**
> > + * \fn iterator V4L2Controls::begin()
> > + * \brief Retrieve an iterator to the first V4L2Control in the instance
> > + * \return An iterator to the first V4L2 control
> > + */
> > +
> > +/**
> > + * \fn const_iterator V4L2Controls::begin() const
> > + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> > + * \return A constant iterator to the first V4L2 control
> > + */
> > +
> > +/**
> > + * \fn iterator V4L2Controls::end()
> > + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> > + * instance
> > + * \return An iterator to the element following the last V4L2 control in the
> > + * instance
> > + */
> > +
> > +/**
> > + * \fn const_iterator V4L2Controls::end() const
> > + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> > + * in the instance
> > + * \return A constant iterator to the element following the last V4L2 control
> > + * in the instance
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::empty()
> > + * \brief Verify if the instance does not contain any control
> > + * \return True if the instance does not contain any control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::size()
> > + * \brief Retrieve the number on controls in the instance
> > + * \return The number of V4L2Control stored in the instance
> > + */
> > +
> > +/**
> > + * \fn V4L2Controls::reset()
> > + * \brief Removes all controls in the instance
>
> s/Removes/Remove/
>
> > + */
> > +
> > +/**
> > + * \brief Add control with ID \a id and optional \a value to the instance
> > + * \param id The V4L2 control ID (V4L2_CID_*)
> > + * \param value The V4L2 control value
> > + *
> > + * V4L2Control can be added with an explicit value, or without one. In that
> > + * case the control's value is defaulted to 0.
>
> s/is defaulted/defaults to/
>
> But I think we can skip the sentence, the doxygen documentation should
> show the = 0.

Ack

Thanks
   j

>
> > + */
> > +void V4L2Controls::add(unsigned int id, int64_t value)
> > +{
> > +	controls_.emplace_back(id, value);
> > +}
> > +
> > +/**
> > + * \fn V4L2Controls::operator[](unsigned int index)
> > + * \brief Access the control at index \a index
> > + * \param index The index to access
> > + * \return The V4L2 control at index \a index
> > + */
> > +
> > +/**
> > + * \brief Get a pointer the control with ID \a id
> > + * \param id The V4L2 control ID (V4L2_CID_xx)
> > + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> > + * control ID is not part of this instance.
> > + */
> > +V4L2Control *V4L2Controls::getControl(unsigned int id)
> > +{
> > +	for (V4L2Control &ctrl : controls_) {
> > +		if (ctrl.id() == id)
> > +			return &ctrl;
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On 24/06/2019 09:15, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> Thank you for the patch.
>>
>> On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
>>> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
>>
>> s/Libcamera/libcamera/ :-)
>>
>>> Control APIs. This patch defines the types used to define and manage controls.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  src/libcamera/include/v4l2_controls.h |  86 ++++++++
>>>  src/libcamera/meson.build             |   1 +
>>>  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
>>>  3 files changed, 369 insertions(+)
>>>  create mode 100644 src/libcamera/include/v4l2_controls.h
>>>  create mode 100644 src/libcamera/v4l2_controls.cpp
>>>
>>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
>>> new file mode 100644
>>> index 000000000000..acd23dabd831
>>> --- /dev/null
>>> +++ b/src/libcamera/include/v4l2_controls.h
>>> @@ -0,0 +1,86 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>> + *
>>> + * v4l2_controls.h - V4L2 Controls Support
>>> + */
>>> +
>>> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
>>> +#define __LIBCAMERA_V4L2_CONTROLS_H__
>>> +
>>> +#include <string>
>>> +#include <vector>
>>> +
>>> +#include <stdint.h>
>>> +
>>> +#include <linux/v4l2-controls.h>
>>> +#include <linux/videodev2.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +class V4L2ControlInfo
>>> +{
>>> +public:
>>> +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
>>> +
>>> +	unsigned int id() const { return id_; }
>>> +	unsigned int type() const { return type_; }
>>> +	size_t size() const { return size_; }
>>> +	const std::string &name() const { return name_; }
>>> +
>>> +private:
>>> +	unsigned int type_;
>>> +	std::string name_;
>>> +	unsigned int id_;
>>> +	size_t size_;
>>
>> As requested by Niklas,
>>
>> 	unsigned int id_;
>> 	unsigned int type_;
>> 	size_t size_;
>> 	std::string name_;
>>
>> ?
>>
>>> +};
>>> +
>>> +class V4L2Control
>>> +{
>>> +public:
>>> +	V4L2Control(unsigned int id, int value = 0)
>>> +		: id_(id), value_(value) {}
>>> +
>>> +	int64_t value() const { return value_; }
>>> +	void setValue(int64_t value) { value_ = value; }
>>> +
>>> +	unsigned int id() const { return id_; }
>>> +
>>> +private:
>>> +	unsigned int id_;
>>> +	int64_t value_;
>>> +};
>>> +
>>> +class V4L2Controls
>>
>> I was wondering if we should rename this to V4L2ControlList, as
>> V4L2Controls is very close to V4L2Control and easy to mistake.
>>
>>> +{
>>> +public:
>>> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
>>
>> As commented before, I think you should either delete both the
>> assignement operator and the copy constructor, or neither of them.
>>
>>> +
>>> +	using iterator = std::vector<V4L2Control>::iterator;
>>> +	using const_iterator = std::vector<V4L2Control>::const_iterator;
>>> +
>>> +	iterator begin() { return controls_.begin(); }
>>> +	const_iterator begin() const { return controls_.begin(); }
>>> +	iterator end() { return controls_.end(); }
>>> +	const_iterator end() const { return controls_.end(); }
>>> +
>>> +	bool empty() const { return controls_.empty(); }
>>> +	size_t size() const { return controls_.size(); }
>>
>> std::size_t to match std::vector ?
>>
>>> +
>>> +	void reset() { controls_.clear(); }
>>> +	void add(unsigned int id, int64_t value = 0);
>>> +
>>> +	V4L2Control *operator[](unsigned int index)
>>> +	{
>>> +		return &controls_[index];
>>> +	}
>>
>> Do we need to access a control by index for any other purpose than
>> iterating over the controls, which the iterator already gives us ?
>> Couldn't operator[] take the control ID, and getControl() be removed ?
>>
> 
> I really don't like this.
> operator[] for vector works on indexes, I would now use the same
> method but with a different semantic. It would be confusing for
> everyone. I can drop this operation if you like to.

A user of a V4L2ControlList shouldn't know about the internal storage
mechanism, and indeed it could be changed without affecting the users.

That does add one concern about using a vector - Can two controls be
created with the same ID?

What happens then?
Is that a valid use case?


> 
>>> +
>>> +	V4L2Control *getControl(unsigned int id);
>>> +
>>> +private:
>>> +	std::vector<V4L2Control> controls_;
>>> +};
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index f26ad5b2dc57..985aa7e8ab0e 100644
>>> --- a/src/libcamera/meson.build
>>> +++ b/src/libcamera/meson.build
>>> @@ -23,6 +23,7 @@ libcamera_sources = files([
>>>      'stream.cpp',
>>>      'timer.cpp',
>>>      'utils.cpp',
>>> +    'v4l2_controls.cpp',
>>>      'v4l2_device.cpp',
>>>      'v4l2_subdevice.cpp',
>>>      'v4l2_videodevice.cpp',
>>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
>>> new file mode 100644
>>> index 000000000000..abf596e7f1ef
>>> --- /dev/null
>>> +++ b/src/libcamera/v4l2_controls.cpp
>>> @@ -0,0 +1,282 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>> + *
>>> + * v4l2_controls.cpp - V4L2 Controls Support
>>> + */
>>> +
>>> +#include "v4l2_controls.h"
>>> +
>>> +/**
>>> + * \file v4l2_controls.h
>>> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
>>
>> s/\.$//
>>
>>> + *
>>> + * The V4L2 defined "Control API" allows application to inspect and modify set
>>
>> "The V4L2 Control API allows applications ..."
>>
>> s/set/sets/
>>
>>> + * of configurable parameters on the video device or subdevice of interest. The
>>
>> s/on the video device or subdevice of interest/on a video device or subdevice/
>>
>>> + * nature of the parameters an application could modify using the control
>>
>> s/could/can/
>>
>>> + * framework depends on what the driver implements support for, and on the
>>> + * characteristics of the underlying hardware platform. Generally controls are
>>> + * used to modify user visible settings, such as the image brightness and
>>> + * exposure time, or non-standard parameters which cannot be controlled through
>>> + * the V4L2 format negotiation API.
>>
>> The last two sentences describe more the V4L2 API than the V4L2Control*
>> classes API. I expect users of this code to know about V4L2 already, so
>> they could be dropped, but I also don't mind if you keep them. Just
>> don't invest too much time in writing document about V4L2 itself, as
>> that's a bit out of scope.
>>
> 
> I feel it doesn't hurt, as it doesn't go in great lenght describing
> controls but I'll drop.
> 
>>> + *
>>> + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
>>> + * and have an associated type. Each control has a value, which is the data
>>> + * that can be modified with V4L2Device::setControls() or retrieved with
>>> + * V4L2Device::getControls().
>>> + *
>>> + * The control's type along with the control's flags defines the type of the
>>
>> s/defines/define/
>>
>>> + * control's value content. Controls might transport a single data value
>>
>> s/might/can/
>>
>>> + * stored in variable inside the control, or they might as well deal with more
>>
>> s/might/can/
>>
>>> + * complex data types, such as arrays of matrices, stored in a contiguous
>>> + * memory locations associated with the control and called 'the payload'. Such
>>> + * controls are called 'compound controls' and are currently not supported by
>>> + * the libcamera V4L2 control framework.
>>
>> Here too you could simply have said that we don't support compound
>> controls yet, but I don't mind keeping the more detailed text as-is.
>>
>>> + *
>>> + * libcamera implements support for controls using the V4L2 'Extended Control'
>>> + * API, which allows future handling of controls with payloads of arbitrary
>>
>> It's not the controls that are extended, it's the API. It's the extended
>> V4L2 control API, not the V4L2 API for extended controls. I would thus
>> remove the quotes.
>>
> 
> I see. I should have moved the ending quote to include the "API" part.
> 
>>> + * sizes.
>>> + *
>>> + * The libcamera V4L2 Controls framework operates on lists of controls,
>>> + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
>>
>> Calling it V4L2ControlList would match the description nicely :-)
>>
> 
> Ack
> 
>>> + * The interface to set and get control is implemented by the V4L2Device
>>> + * class, and this file only provides the data type definitions.
>>> + *
>>> + * \todo Add support for compound controls
>>> + */
>>> +
>>> +namespace libcamera {
>>> +
>>> +/**
>>> + * \class V4L2ControlInfo
>>> + * \brief Information on a V4L2 control
>>> + *
>>> + * The V4L2ControlInfo class represent all the information related to
>>
>> s/represent/represents/
>>
>>> + * a V4L2 control, such as its ID, its type, its user-readable name and the
>>> + * expected size of its value data.
>>> + *
>>> + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
>>> + * v4l2_query_ext_ctrl structure, after it has been filled by the device
>>> + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
>>> + *
>>> + * This class does not contain the control value, but only static information
>>> + * on the control, which shall be cached by the caller at initialisation time
>>> + * or the first time the control information is accessed.
>>> + */
>>> +
>>> +/**
>>> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
>>> + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
>>> + */
>>> +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
>>> +{
>>> +	id_ = ctrl.id;
>>> +	type_ = ctrl.type;
>>> +	name_ = static_cast<const char *>(ctrl.name);
>>> +	size_ = ctrl.elem_size * ctrl.elems;
>>> +}
>>> +
>>> +/**
>>> + * \fn V4L2ControlInfo::id()
>>> + * \brief Retrieve the control ID
>>> + * \return The V4L2 control ID
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ControlInfo::type()
>>> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
>>> + * \return The V4L2 control type
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ControlInfo::size()
>>> + * \brief Retrieve the control value data size (in bytes)
>>> + * \return The V4L2 control value data size
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2ControlInfo::name()
>>> + * \brief Retrieve the control user readable name
>>> + * \return The V4L2 control user readable name
>>> + */
>>> +
>>> +/**
>>> + * \class V4L2Control
>>> + * \brief A V4L2 control value
>>> + *
>>> + * The V4L2Control class represent the value of a V4L2 control.
>>> + * The class stores values that have been read from or will be applied to
>>> + * a V4L2 device.
>>> + *
>>> + * The value stored in the class instances does not reflect what is actually
>>> + * applied to the hardware but is a pure software cache optionally initialized
>>> + * at control creation time and only modified by a control read operation.
>>
>> With setControls() updating the value, this isn't true anymore.
>>
> 
> True. Nice catch.
> 
>>> + *
>>> + * The write and read controls the V4L2Control class instances are not meant
>>> + * to be directly used but are instead intended to be grouped in
>>> + * V4L2Controls instances, which are then passed as parameters to
>>> + * V4L2Device::setControls() and V4L2Device::getControls() operations.
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Control::V4L2Control
>>> + * \brief Construct a V4L2 control with ID \a id and value \a value
>>
>> "with an \a id and a \a value" ? Or "Construct an instance of control \a
>> ID with a \a value" ?
>>
> 
> I'm always uncertain on how to handle these situation, where the name
> of the parameter matches the generic name.
> 
>>> + * \param id The V4L2 control ID
>>> + * \param value The control value
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Control::value()
>>> + * \brief Retrieve the value of the control
>>> + *
>>> + * It is important to notice that the returned value is not read from the
>>
>> s/notice/note/
>>
>>> + * hardware at the time this operation is called, but it's the cached value
>>> + * obtained the most recent time the control has been read with
>>> + * V4L2Device::getControls() or the value of the control has been set to by the
>>> + * user with the add() operation.
>>
>> You should mention setControls() here. How about
>>
>> This method returns the cached control value, initially set by
>> V4L2Controls::add() and then updated when the controls are read or
>> written with V4L2Device::getControls() and V4L2Device::setControls().
>>
>>> + *
>>> + * \return The V4L2 control value. The value might be 0 if the control has
>>> + * never been read from the device before this operation is called.
>>
>> I would drop the second sentence, it's redundant with the above
>> paragraph, and a bit confusing too as the value may also be 0 after
>> being read.
>>
> 
> Ack.
> 
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Control::setValue()
>>> + * \brief Set the value of the control
>>> + * \param value The new V4L2 control value
>>> + *
>>> + * It is important to notice that the value is not applied to the
>>> + * hardware at the time this operation is called, but it will only
>>> + * be actually applied by calling V4L2Device::setControls().
>>
>> Let's simplify it a bit, by explaining what the method does, not what it
>> does not do :-)
>>
>> "This method stores the control value, which will be applied to the
>> device when calling V4L2Device::setControls()."
>>
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Control::id()
>>> + * \brief Retrieve the control ID this instance refers to
>>> + * \return The V4L2Control ID
>>> + */
>>> +
>>> +/**
>>> + * \class V4L2Controls
>>> + * \brief Container of V4L2Control instances
>>> + *
>>> + * The V4L2Controls class works as a container for a list of V4L2Control
>>> + * instances. The class provides operations to add a new control to the list,
>>> + * get back a control value, and reset the list of controls it contains.
>>> + *
>>> + * In order to set and get controls, user of the libcamera V4L2 control
>>> + * framework should operate on instances of the V4L2Controls class, and use
>>> + * them as argument for the V4L2Device::setControls() and
>>> + * V4L2Device::getControls() operations, which write and read a list of
>>> + * controls from or to a V4L2 device (a video device or a subdevice).
>>
>> "to and from" (as it's "write and read")
>>
>>> + *
>>> + * Controls are added to a V4L2Controls instance with the add() method, with
>>> + * or without an initial value.
>>> + *
>>> + * To write controls to a device, the controls of interest shall be added with
>>> + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
>>
>> s/an initial value/a value/
>>
>>> + * value) to prepare for a write operation. Once the values of all controls of
>>> + * interest have been added, the V4L2Controls instance is passed to the
>>> + * V4L2Device::setControls(), which sets the controls on the device.
>>> + *
>>> + * To read controls from a device, the desired controls are added with
>>> + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
>>> + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
>>> + * reads the controls from the device and updates the values stored in
>>> + * V4L2Controls.
>>> + *
>>> + * V4L2Controls instances can be reset to remove all controls they contain and
>>> + * prepare to be re-used for a new control write/read sequence.
>>> + */
>>> +
>>> +/**
>>> + * \typedef V4L2Controls::iterator
>>> + * \brief Iterator on the V4L2 controls contained in the instance
>>> + */
>>> +
>>> +/**
>>> + * \typedef V4L2Controls::const_iterator
>>> + * \brief Const iterator on the V4L2 controls contained in the instance
>>> + */
>>> +
>>> +/**
>>> + * \fn iterator V4L2Controls::begin()
>>> + * \brief Retrieve an iterator to the first V4L2Control in the instance
>>> + * \return An iterator to the first V4L2 control
>>> + */
>>> +
>>> +/**
>>> + * \fn const_iterator V4L2Controls::begin() const
>>> + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
>>> + * \return A constant iterator to the first V4L2 control
>>> + */
>>> +
>>> +/**
>>> + * \fn iterator V4L2Controls::end()
>>> + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
>>> + * instance
>>> + * \return An iterator to the element following the last V4L2 control in the
>>> + * instance
>>> + */
>>> +
>>> +/**
>>> + * \fn const_iterator V4L2Controls::end() const
>>> + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
>>> + * in the instance
>>> + * \return A constant iterator to the element following the last V4L2 control
>>> + * in the instance
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Controls::empty()
>>> + * \brief Verify if the instance does not contain any control
>>> + * \return True if the instance does not contain any control, false otherwise
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Controls::size()
>>> + * \brief Retrieve the number on controls in the instance
>>> + * \return The number of V4L2Control stored in the instance
>>> + */
>>> +
>>> +/**
>>> + * \fn V4L2Controls::reset()
>>> + * \brief Removes all controls in the instance
>>
>> s/Removes/Remove/
>>
>>> + */
>>> +
>>> +/**
>>> + * \brief Add control with ID \a id and optional \a value to the instance
>>> + * \param id The V4L2 control ID (V4L2_CID_*)
>>> + * \param value The V4L2 control value
>>> + *
>>> + * V4L2Control can be added with an explicit value, or without one. In that
>>> + * case the control's value is defaulted to 0.
>>
>> s/is defaulted/defaults to/
>>
>> But I think we can skip the sentence, the doxygen documentation should
>> show the = 0.
> 
> Ack
> 
> Thanks
>    j
> 
>>
>>> + */
>>> +void V4L2Controls::add(unsigned int id, int64_t value)
>>> +{
>>> +	controls_.emplace_back(id, value);
>>> +}
>>> +
>>> +/**
>>> + * \fn V4L2Controls::operator[](unsigned int index)
>>> + * \brief Access the control at index \a index
>>> + * \param index The index to access
>>> + * \return The V4L2 control at index \a index
>>> + */
>>> +
>>> +/**
>>> + * \brief Get a pointer the control with ID \a id
>>> + * \param id The V4L2 control ID (V4L2_CID_xx)
>>> + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
>>> + * control ID is not part of this instance.
>>> + */
>>> +V4L2Control *V4L2Controls::getControl(unsigned int id)
>>> +{
>>> +	for (V4L2Control &ctrl : controls_) {
>>> +		if (ctrl.id() == id)
>>> +			return &ctrl;
>>> +	}
>>> +
>>> +	return nullptr;
>>> +}
>>> +
>>> +} /* namespace libcamera */
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

On Mon, Jun 24, 2019 at 09:36:25AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 24/06/2019 09:15, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
> >> Hi Jacopo,
> >>
> >> Thank you for the patch.
> >>
> >> On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> >>> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> >>
> >> s/Libcamera/libcamera/ :-)
> >>
> >>> Control APIs. This patch defines the types used to define and manage controls.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  src/libcamera/include/v4l2_controls.h |  86 ++++++++
> >>>  src/libcamera/meson.build             |   1 +
> >>>  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
> >>>  3 files changed, 369 insertions(+)
> >>>  create mode 100644 src/libcamera/include/v4l2_controls.h
> >>>  create mode 100644 src/libcamera/v4l2_controls.cpp
> >>>
> >>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> >>> new file mode 100644
> >>> index 000000000000..acd23dabd831
> >>> --- /dev/null
> >>> +++ b/src/libcamera/include/v4l2_controls.h
> >>> @@ -0,0 +1,86 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * v4l2_controls.h - V4L2 Controls Support
> >>> + */
> >>> +
> >>> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> >>> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> >>> +
> >>> +#include <string>
> >>> +#include <vector>
> >>> +
> >>> +#include <stdint.h>
> >>> +
> >>> +#include <linux/v4l2-controls.h>
> >>> +#include <linux/videodev2.h>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +class V4L2ControlInfo
> >>> +{
> >>> +public:
> >>> +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >>> +
> >>> +	unsigned int id() const { return id_; }
> >>> +	unsigned int type() const { return type_; }
> >>> +	size_t size() const { return size_; }
> >>> +	const std::string &name() const { return name_; }
> >>> +
> >>> +private:
> >>> +	unsigned int type_;
> >>> +	std::string name_;
> >>> +	unsigned int id_;
> >>> +	size_t size_;
> >>
> >> As requested by Niklas,
> >>
> >> 	unsigned int id_;
> >> 	unsigned int type_;
> >> 	size_t size_;
> >> 	std::string name_;
> >>
> >> ?
> >>
> >>> +};
> >>> +
> >>> +class V4L2Control
> >>> +{
> >>> +public:
> >>> +	V4L2Control(unsigned int id, int value = 0)
> >>> +		: id_(id), value_(value) {}
> >>> +
> >>> +	int64_t value() const { return value_; }
> >>> +	void setValue(int64_t value) { value_ = value; }
> >>> +
> >>> +	unsigned int id() const { return id_; }
> >>> +
> >>> +private:
> >>> +	unsigned int id_;
> >>> +	int64_t value_;
> >>> +};
> >>> +
> >>> +class V4L2Controls
> >>
> >> I was wondering if we should rename this to V4L2ControlList, as
> >> V4L2Controls is very close to V4L2Control and easy to mistake.
> >>
> >>> +{
> >>> +public:
> >>> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> >>
> >> As commented before, I think you should either delete both the
> >> assignement operator and the copy constructor, or neither of them.
> >>
> >>> +
> >>> +	using iterator = std::vector<V4L2Control>::iterator;
> >>> +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> >>> +
> >>> +	iterator begin() { return controls_.begin(); }
> >>> +	const_iterator begin() const { return controls_.begin(); }
> >>> +	iterator end() { return controls_.end(); }
> >>> +	const_iterator end() const { return controls_.end(); }
> >>> +
> >>> +	bool empty() const { return controls_.empty(); }
> >>> +	size_t size() const { return controls_.size(); }
> >>
> >> std::size_t to match std::vector ?
> >>
> >>> +
> >>> +	void reset() { controls_.clear(); }
> >>> +	void add(unsigned int id, int64_t value = 0);
> >>> +
> >>> +	V4L2Control *operator[](unsigned int index)
> >>> +	{
> >>> +		return &controls_[index];
> >>> +	}
> >>
> >> Do we need to access a control by index for any other purpose than
> >> iterating over the controls, which the iterator already gives us ?
> >> Couldn't operator[] take the control ID, and getControl() be removed ?
> >>
> >
> > I really don't like this.
> > operator[] for vector works on indexes, I would now use the same
> > method but with a different semantic. It would be confusing for
> > everyone. I can drop this operation if you like to.
>
> A user of a V4L2ControlList shouldn't know about the internal storage
> mechanism, and indeed it could be changed without affecting the users.
>

Sorry but I don't agree. This class "quacks" like a vector, and mimics
its interface. We could extend it of course, but diverging seems a
very bad idea. Furthermore, 99% of usages of the [] operator I can
think of work on indexes, I don't see a reason to change this and ask
people to go and look at the documentation to know how operator[]
works (for no clear gain, in my opinion)

> That does add one concern about using a vector - Can two controls be
> created with the same ID?
>
> What happens then?
> Is that a valid use case?
>

That's a real concern instead. Nothing in the V4L2 extended control
APIs prescribes to have unique entries, so if that happens here, this
won't be an issue at the V4L2 control level, but it might be confusing
for application and lead to some subtile bug when the same
V4L2ControlList is manipulated by different methods.

So, it's not a bug, but it might be desirable to make sure that
"add()" only adds unique entries, but at the same time this is not an
API for applications, and pipeline handlers should know better. I'm
debated, slightly leaning to leave it the way it is now.

>
> >
> >>> +
> >>> +	V4L2Control *getControl(unsigned int id);
> >>> +
> >>> +private:
> >>> +	std::vector<V4L2Control> controls_;
> >>> +};
> >>> +
> >>> +} /* namespace libcamera */
> >>> +
> >>> +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> >>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >>> index f26ad5b2dc57..985aa7e8ab0e 100644
> >>> --- a/src/libcamera/meson.build
> >>> +++ b/src/libcamera/meson.build
> >>> @@ -23,6 +23,7 @@ libcamera_sources = files([
> >>>      'stream.cpp',
> >>>      'timer.cpp',
> >>>      'utils.cpp',
> >>> +    'v4l2_controls.cpp',
> >>>      'v4l2_device.cpp',
> >>>      'v4l2_subdevice.cpp',
> >>>      'v4l2_videodevice.cpp',
> >>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> >>> new file mode 100644
> >>> index 000000000000..abf596e7f1ef
> >>> --- /dev/null
> >>> +++ b/src/libcamera/v4l2_controls.cpp
> >>> @@ -0,0 +1,282 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2019, Google Inc.
> >>> + *
> >>> + * v4l2_controls.cpp - V4L2 Controls Support
> >>> + */
> >>> +
> >>> +#include "v4l2_controls.h"
> >>> +
> >>> +/**
> >>> + * \file v4l2_controls.h
> >>> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> >>
> >> s/\.$//
> >>
> >>> + *
> >>> + * The V4L2 defined "Control API" allows application to inspect and modify set
> >>
> >> "The V4L2 Control API allows applications ..."
> >>
> >> s/set/sets/
> >>
> >>> + * of configurable parameters on the video device or subdevice of interest. The
> >>
> >> s/on the video device or subdevice of interest/on a video device or subdevice/
> >>
> >>> + * nature of the parameters an application could modify using the control
> >>
> >> s/could/can/
> >>
> >>> + * framework depends on what the driver implements support for, and on the
> >>> + * characteristics of the underlying hardware platform. Generally controls are
> >>> + * used to modify user visible settings, such as the image brightness and
> >>> + * exposure time, or non-standard parameters which cannot be controlled through
> >>> + * the V4L2 format negotiation API.
> >>
> >> The last two sentences describe more the V4L2 API than the V4L2Control*
> >> classes API. I expect users of this code to know about V4L2 already, so
> >> they could be dropped, but I also don't mind if you keep them. Just
> >> don't invest too much time in writing document about V4L2 itself, as
> >> that's a bit out of scope.
> >>
> >
> > I feel it doesn't hurt, as it doesn't go in great lenght describing
> > controls but I'll drop.
> >
> >>> + *
> >>> + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> >>> + * and have an associated type. Each control has a value, which is the data
> >>> + * that can be modified with V4L2Device::setControls() or retrieved with
> >>> + * V4L2Device::getControls().
> >>> + *
> >>> + * The control's type along with the control's flags defines the type of the
> >>
> >> s/defines/define/
> >>
> >>> + * control's value content. Controls might transport a single data value
> >>
> >> s/might/can/
> >>
> >>> + * stored in variable inside the control, or they might as well deal with more
> >>
> >> s/might/can/
> >>
> >>> + * complex data types, such as arrays of matrices, stored in a contiguous
> >>> + * memory locations associated with the control and called 'the payload'. Such
> >>> + * controls are called 'compound controls' and are currently not supported by
> >>> + * the libcamera V4L2 control framework.
> >>
> >> Here too you could simply have said that we don't support compound
> >> controls yet, but I don't mind keeping the more detailed text as-is.
> >>
> >>> + *
> >>> + * libcamera implements support for controls using the V4L2 'Extended Control'
> >>> + * API, which allows future handling of controls with payloads of arbitrary
> >>
> >> It's not the controls that are extended, it's the API. It's the extended
> >> V4L2 control API, not the V4L2 API for extended controls. I would thus
> >> remove the quotes.
> >>
> >
> > I see. I should have moved the ending quote to include the "API" part.
> >
> >>> + * sizes.
> >>> + *
> >>> + * The libcamera V4L2 Controls framework operates on lists of controls,
> >>> + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
> >>
> >> Calling it V4L2ControlList would match the description nicely :-)
> >>
> >
> > Ack
> >
> >>> + * The interface to set and get control is implemented by the V4L2Device
> >>> + * class, and this file only provides the data type definitions.
> >>> + *
> >>> + * \todo Add support for compound controls
> >>> + */
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +/**
> >>> + * \class V4L2ControlInfo
> >>> + * \brief Information on a V4L2 control
> >>> + *
> >>> + * The V4L2ControlInfo class represent all the information related to
> >>
> >> s/represent/represents/
> >>
> >>> + * a V4L2 control, such as its ID, its type, its user-readable name and the
> >>> + * expected size of its value data.
> >>> + *
> >>> + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> >>> + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> >>> + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> >>> + *
> >>> + * This class does not contain the control value, but only static information
> >>> + * on the control, which shall be cached by the caller at initialisation time
> >>> + * or the first time the control information is accessed.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> >>> + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >>> + */
> >>> +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >>> +{
> >>> +	id_ = ctrl.id;
> >>> +	type_ = ctrl.type;
> >>> +	name_ = static_cast<const char *>(ctrl.name);
> >>> +	size_ = ctrl.elem_size * ctrl.elems;
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlInfo::id()
> >>> + * \brief Retrieve the control ID
> >>> + * \return The V4L2 control ID
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlInfo::type()
> >>> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> >>> + * \return The V4L2 control type
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlInfo::size()
> >>> + * \brief Retrieve the control value data size (in bytes)
> >>> + * \return The V4L2 control value data size
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2ControlInfo::name()
> >>> + * \brief Retrieve the control user readable name
> >>> + * \return The V4L2 control user readable name
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2Control
> >>> + * \brief A V4L2 control value
> >>> + *
> >>> + * The V4L2Control class represent the value of a V4L2 control.
> >>> + * The class stores values that have been read from or will be applied to
> >>> + * a V4L2 device.
> >>> + *
> >>> + * The value stored in the class instances does not reflect what is actually
> >>> + * applied to the hardware but is a pure software cache optionally initialized
> >>> + * at control creation time and only modified by a control read operation.
> >>
> >> With setControls() updating the value, this isn't true anymore.
> >>
> >
> > True. Nice catch.
> >
> >>> + *
> >>> + * The write and read controls the V4L2Control class instances are not meant
> >>> + * to be directly used but are instead intended to be grouped in
> >>> + * V4L2Controls instances, which are then passed as parameters to
> >>> + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::V4L2Control
> >>> + * \brief Construct a V4L2 control with ID \a id and value \a value
> >>
> >> "with an \a id and a \a value" ? Or "Construct an instance of control \a
> >> ID with a \a value" ?
> >>
> >
> > I'm always uncertain on how to handle these situation, where the name
> > of the parameter matches the generic name.
> >
> >>> + * \param id The V4L2 control ID
> >>> + * \param value The control value
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::value()
> >>> + * \brief Retrieve the value of the control
> >>> + *
> >>> + * It is important to notice that the returned value is not read from the
> >>
> >> s/notice/note/
> >>
> >>> + * hardware at the time this operation is called, but it's the cached value
> >>> + * obtained the most recent time the control has been read with
> >>> + * V4L2Device::getControls() or the value of the control has been set to by the
> >>> + * user with the add() operation.
> >>
> >> You should mention setControls() here. How about
> >>
> >> This method returns the cached control value, initially set by
> >> V4L2Controls::add() and then updated when the controls are read or
> >> written with V4L2Device::getControls() and V4L2Device::setControls().
> >>
> >>> + *
> >>> + * \return The V4L2 control value. The value might be 0 if the control has
> >>> + * never been read from the device before this operation is called.
> >>
> >> I would drop the second sentence, it's redundant with the above
> >> paragraph, and a bit confusing too as the value may also be 0 after
> >> being read.
> >>
> >
> > Ack.
> >
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::setValue()
> >>> + * \brief Set the value of the control
> >>> + * \param value The new V4L2 control value
> >>> + *
> >>> + * It is important to notice that the value is not applied to the
> >>> + * hardware at the time this operation is called, but it will only
> >>> + * be actually applied by calling V4L2Device::setControls().
> >>
> >> Let's simplify it a bit, by explaining what the method does, not what it
> >> does not do :-)
> >>
> >> "This method stores the control value, which will be applied to the
> >> device when calling V4L2Device::setControls()."
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Control::id()
> >>> + * \brief Retrieve the control ID this instance refers to
> >>> + * \return The V4L2Control ID
> >>> + */
> >>> +
> >>> +/**
> >>> + * \class V4L2Controls
> >>> + * \brief Container of V4L2Control instances
> >>> + *
> >>> + * The V4L2Controls class works as a container for a list of V4L2Control
> >>> + * instances. The class provides operations to add a new control to the list,
> >>> + * get back a control value, and reset the list of controls it contains.
> >>> + *
> >>> + * In order to set and get controls, user of the libcamera V4L2 control
> >>> + * framework should operate on instances of the V4L2Controls class, and use
> >>> + * them as argument for the V4L2Device::setControls() and
> >>> + * V4L2Device::getControls() operations, which write and read a list of
> >>> + * controls from or to a V4L2 device (a video device or a subdevice).
> >>
> >> "to and from" (as it's "write and read")
> >>
> >>> + *
> >>> + * Controls are added to a V4L2Controls instance with the add() method, with
> >>> + * or without an initial value.
> >>> + *
> >>> + * To write controls to a device, the controls of interest shall be added with
> >>> + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
> >>
> >> s/an initial value/a value/
> >>
> >>> + * value) to prepare for a write operation. Once the values of all controls of
> >>> + * interest have been added, the V4L2Controls instance is passed to the
> >>> + * V4L2Device::setControls(), which sets the controls on the device.
> >>> + *
> >>> + * To read controls from a device, the desired controls are added with
> >>> + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> >>> + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> >>> + * reads the controls from the device and updates the values stored in
> >>> + * V4L2Controls.
> >>> + *
> >>> + * V4L2Controls instances can be reset to remove all controls they contain and
> >>> + * prepare to be re-used for a new control write/read sequence.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \typedef V4L2Controls::iterator
> >>> + * \brief Iterator on the V4L2 controls contained in the instance
> >>> + */
> >>> +
> >>> +/**
> >>> + * \typedef V4L2Controls::const_iterator
> >>> + * \brief Const iterator on the V4L2 controls contained in the instance
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn iterator V4L2Controls::begin()
> >>> + * \brief Retrieve an iterator to the first V4L2Control in the instance
> >>> + * \return An iterator to the first V4L2 control
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn const_iterator V4L2Controls::begin() const
> >>> + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> >>> + * \return A constant iterator to the first V4L2 control
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn iterator V4L2Controls::end()
> >>> + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> >>> + * instance
> >>> + * \return An iterator to the element following the last V4L2 control in the
> >>> + * instance
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn const_iterator V4L2Controls::end() const
> >>> + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> >>> + * in the instance
> >>> + * \return A constant iterator to the element following the last V4L2 control
> >>> + * in the instance
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Controls::empty()
> >>> + * \brief Verify if the instance does not contain any control
> >>> + * \return True if the instance does not contain any control, false otherwise
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Controls::size()
> >>> + * \brief Retrieve the number on controls in the instance
> >>> + * \return The number of V4L2Control stored in the instance
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Controls::reset()
> >>> + * \brief Removes all controls in the instance
> >>
> >> s/Removes/Remove/
> >>
> >>> + */
> >>> +
> >>> +/**
> >>> + * \brief Add control with ID \a id and optional \a value to the instance
> >>> + * \param id The V4L2 control ID (V4L2_CID_*)
> >>> + * \param value The V4L2 control value
> >>> + *
> >>> + * V4L2Control can be added with an explicit value, or without one. In that
> >>> + * case the control's value is defaulted to 0.
> >>
> >> s/is defaulted/defaults to/
> >>
> >> But I think we can skip the sentence, the doxygen documentation should
> >> show the = 0.
> >
> > Ack
> >
> > Thanks
> >    j
> >
> >>
> >>> + */
> >>> +void V4L2Controls::add(unsigned int id, int64_t value)
> >>> +{
> >>> +	controls_.emplace_back(id, value);
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Controls::operator[](unsigned int index)
> >>> + * \brief Access the control at index \a index
> >>> + * \param index The index to access
> >>> + * \return The V4L2 control at index \a index
> >>> + */
> >>> +
> >>> +/**
> >>> + * \brief Get a pointer the control with ID \a id
> >>> + * \param id The V4L2 control ID (V4L2_CID_xx)
> >>> + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> >>> + * control ID is not part of this instance.
> >>> + */
> >>> +V4L2Control *V4L2Controls::getControl(unsigned int id)
> >>> +{
> >>> +	for (V4L2Control &ctrl : controls_) {
> >>> +		if (ctrl.id() == id)
> >>> +			return &ctrl;
> >>> +	}
> >>> +
> >>> +	return nullptr;
> >>> +}
> >>> +
> >>> +} /* namespace libcamera */
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran

Hi Jacopo,

On Mon, Jun 24, 2019 at 10:15:05AM +0200, Jacopo Mondi wrote:
> On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> >> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> >
> > s/Libcamera/libcamera/ :-)
> >
> >> Control APIs. This patch defines the types used to define and manage controls.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/libcamera/include/v4l2_controls.h |  86 ++++++++
> >>  src/libcamera/meson.build             |   1 +
> >>  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
> >>  3 files changed, 369 insertions(+)
> >>  create mode 100644 src/libcamera/include/v4l2_controls.h
> >>  create mode 100644 src/libcamera/v4l2_controls.cpp
> >>
> >> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> >> new file mode 100644
> >> index 000000000000..acd23dabd831
> >> --- /dev/null
> >> +++ b/src/libcamera/include/v4l2_controls.h
> >> @@ -0,0 +1,86 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * v4l2_controls.h - V4L2 Controls Support
> >> + */
> >> +
> >> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> >> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> >> +
> >> +#include <string>
> >> +#include <vector>
> >> +
> >> +#include <stdint.h>
> >> +
> >> +#include <linux/v4l2-controls.h>
> >> +#include <linux/videodev2.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +class V4L2ControlInfo
> >> +{
> >> +public:
> >> +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >> +
> >> +	unsigned int id() const { return id_; }
> >> +	unsigned int type() const { return type_; }
> >> +	size_t size() const { return size_; }
> >> +	const std::string &name() const { return name_; }
> >> +
> >> +private:
> >> +	unsigned int type_;
> >> +	std::string name_;
> >> +	unsigned int id_;
> >> +	size_t size_;
> >
> > As requested by Niklas,
> >
> > 	unsigned int id_;
> > 	unsigned int type_;
> > 	size_t size_;
> > 	std::string name_;
> >
> > ?
> >
> >> +};
> >> +
> >> +class V4L2Control
> >> +{
> >> +public:
> >> +	V4L2Control(unsigned int id, int value = 0)
> >> +		: id_(id), value_(value) {}
> >> +
> >> +	int64_t value() const { return value_; }
> >> +	void setValue(int64_t value) { value_ = value; }
> >> +
> >> +	unsigned int id() const { return id_; }
> >> +
> >> +private:
> >> +	unsigned int id_;
> >> +	int64_t value_;
> >> +};
> >> +
> >> +class V4L2Controls
> >
> > I was wondering if we should rename this to V4L2ControlList, as
> > V4L2Controls is very close to V4L2Control and easy to mistake.
> >
> >> +{
> >> +public:
> >> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> >
> > As commented before, I think you should either delete both the
> > assignement operator and the copy constructor, or neither of them.
> >
> >> +
> >> +	using iterator = std::vector<V4L2Control>::iterator;
> >> +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> >> +
> >> +	iterator begin() { return controls_.begin(); }
> >> +	const_iterator begin() const { return controls_.begin(); }
> >> +	iterator end() { return controls_.end(); }
> >> +	const_iterator end() const { return controls_.end(); }
> >> +
> >> +	bool empty() const { return controls_.empty(); }
> >> +	size_t size() const { return controls_.size(); }
> >
> > std::size_t to match std::vector ?
> >
> >> +
> >> +	void reset() { controls_.clear(); }
> >> +	void add(unsigned int id, int64_t value = 0);
> >> +
> >> +	V4L2Control *operator[](unsigned int index)
> >> +	{
> >> +		return &controls_[index];
> >> +	}
> >
> > Do we need to access a control by index for any other purpose than
> > iterating over the controls, which the iterator already gives us ?
> > Couldn't operator[] take the control ID, and getControl() be removed ?
> 
> I really don't like this.
> operator[] for vector works on indexes, I would now use the same
> method but with a different semantic. It would be confusing for
> everyone. I can drop this operation if you like to.

Note that this class is not a vector by itself, even if it uses a vector
internally. It's essentially a map (associates keys with values) sorted
by insertion order. Or it can be described as a list with key lookup.
It's a hybrid, and we're thus allowed to create any API we want, as long
as it's consistent, properly documented, and easy and intuitive to use.

If you want to keep the getControl() method I would indeed drop the
operator[] as we don't really need it.

> >> +
> >> +	V4L2Control *getControl(unsigned int id);
> >> +
> >> +private:
> >> +	std::vector<V4L2Control> controls_;
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index f26ad5b2dc57..985aa7e8ab0e 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -23,6 +23,7 @@ libcamera_sources = files([
> >>      'stream.cpp',
> >>      'timer.cpp',
> >>      'utils.cpp',
> >> +    'v4l2_controls.cpp',
> >>      'v4l2_device.cpp',
> >>      'v4l2_subdevice.cpp',
> >>      'v4l2_videodevice.cpp',
> >> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> >> new file mode 100644
> >> index 000000000000..abf596e7f1ef
> >> --- /dev/null
> >> +++ b/src/libcamera/v4l2_controls.cpp
> >> @@ -0,0 +1,282 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * v4l2_controls.cpp - V4L2 Controls Support
> >> + */
> >> +
> >> +#include "v4l2_controls.h"
> >> +
> >> +/**
> >> + * \file v4l2_controls.h
> >> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> >
> > s/\.$//
> >
> >> + *
> >> + * The V4L2 defined "Control API" allows application to inspect and modify set
> >
> > "The V4L2 Control API allows applications ..."
> >
> > s/set/sets/
> >
> >> + * of configurable parameters on the video device or subdevice of interest. The
> >
> > s/on the video device or subdevice of interest/on a video device or subdevice/
> >
> >> + * nature of the parameters an application could modify using the control
> >
> > s/could/can/
> >
> >> + * framework depends on what the driver implements support for, and on the
> >> + * characteristics of the underlying hardware platform. Generally controls are
> >> + * used to modify user visible settings, such as the image brightness and
> >> + * exposure time, or non-standard parameters which cannot be controlled through
> >> + * the V4L2 format negotiation API.
> >
> > The last two sentences describe more the V4L2 API than the V4L2Control*
> > classes API. I expect users of this code to know about V4L2 already, so
> > they could be dropped, but I also don't mind if you keep them. Just
> > don't invest too much time in writing document about V4L2 itself, as
> > that's a bit out of scope.
> 
> I feel it doesn't hurt, as it doesn't go in great lenght describing
> controls but I'll drop.
> 
> >> + *
> >> + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> >> + * and have an associated type. Each control has a value, which is the data
> >> + * that can be modified with V4L2Device::setControls() or retrieved with
> >> + * V4L2Device::getControls().
> >> + *
> >> + * The control's type along with the control's flags defines the type of the
> >
> > s/defines/define/
> >
> >> + * control's value content. Controls might transport a single data value
> >
> > s/might/can/
> >
> >> + * stored in variable inside the control, or they might as well deal with more
> >
> > s/might/can/
> >
> >> + * complex data types, such as arrays of matrices, stored in a contiguous
> >> + * memory locations associated with the control and called 'the payload'. Such
> >> + * controls are called 'compound controls' and are currently not supported by
> >> + * the libcamera V4L2 control framework.
> >
> > Here too you could simply have said that we don't support compound
> > controls yet, but I don't mind keeping the more detailed text as-is.
> >
> >> + *
> >> + * libcamera implements support for controls using the V4L2 'Extended Control'
> >> + * API, which allows future handling of controls with payloads of arbitrary
> >
> > It's not the controls that are extended, it's the API. It's the extended
> > V4L2 control API, not the V4L2 API for extended controls. I would thus
> > remove the quotes.
> 
> I see. I should have moved the ending quote to include the "API" part.
> 
> >> + * sizes.
> >> + *
> >> + * The libcamera V4L2 Controls framework operates on lists of controls,
> >> + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
> >
> > Calling it V4L2ControlList would match the description nicely :-)
> 
> Ack
> 
> >> + * The interface to set and get control is implemented by the V4L2Device
> >> + * class, and this file only provides the data type definitions.
> >> + *
> >> + * \todo Add support for compound controls
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +/**
> >> + * \class V4L2ControlInfo
> >> + * \brief Information on a V4L2 control
> >> + *
> >> + * The V4L2ControlInfo class represent all the information related to
> >
> > s/represent/represents/
> >
> >> + * a V4L2 control, such as its ID, its type, its user-readable name and the
> >> + * expected size of its value data.
> >> + *
> >> + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> >> + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> >> + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> >> + *
> >> + * This class does not contain the control value, but only static information
> >> + * on the control, which shall be cached by the caller at initialisation time
> >> + * or the first time the control information is accessed.
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> >> + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >> + */
> >> +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >> +{
> >> +	id_ = ctrl.id;
> >> +	type_ = ctrl.type;
> >> +	name_ = static_cast<const char *>(ctrl.name);
> >> +	size_ = ctrl.elem_size * ctrl.elems;
> >> +}
> >> +
> >> +/**
> >> + * \fn V4L2ControlInfo::id()
> >> + * \brief Retrieve the control ID
> >> + * \return The V4L2 control ID
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2ControlInfo::type()
> >> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> >> + * \return The V4L2 control type
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2ControlInfo::size()
> >> + * \brief Retrieve the control value data size (in bytes)
> >> + * \return The V4L2 control value data size
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2ControlInfo::name()
> >> + * \brief Retrieve the control user readable name
> >> + * \return The V4L2 control user readable name
> >> + */
> >> +
> >> +/**
> >> + * \class V4L2Control
> >> + * \brief A V4L2 control value
> >> + *
> >> + * The V4L2Control class represent the value of a V4L2 control.
> >> + * The class stores values that have been read from or will be applied to
> >> + * a V4L2 device.
> >> + *
> >> + * The value stored in the class instances does not reflect what is actually
> >> + * applied to the hardware but is a pure software cache optionally initialized
> >> + * at control creation time and only modified by a control read operation.
> >
> > With setControls() updating the value, this isn't true anymore.
> 
> True. Nice catch.
> 
> >> + *
> >> + * The write and read controls the V4L2Control class instances are not meant
> >> + * to be directly used but are instead intended to be grouped in
> >> + * V4L2Controls instances, which are then passed as parameters to
> >> + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Control::V4L2Control
> >> + * \brief Construct a V4L2 control with ID \a id and value \a value
> >
> > "with an \a id and a \a value" ? Or "Construct an instance of control \a
> > ID with a \a value" ?
> 
> I'm always uncertain on how to handle these situation, where the name
> of the parameter matches the generic name.

I personally try to make the output documentation as easy to read as
possible, and for me that means avoiding heavy construct such as "the id
\a id". Have a look at the HTML, and see what is the easiest to read for
you :-)

> >> + * \param id The V4L2 control ID
> >> + * \param value The control value
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Control::value()
> >> + * \brief Retrieve the value of the control
> >> + *
> >> + * It is important to notice that the returned value is not read from the
> >
> > s/notice/note/
> >
> >> + * hardware at the time this operation is called, but it's the cached value
> >> + * obtained the most recent time the control has been read with
> >> + * V4L2Device::getControls() or the value of the control has been set to by the
> >> + * user with the add() operation.
> >
> > You should mention setControls() here. How about
> >
> > This method returns the cached control value, initially set by
> > V4L2Controls::add() and then updated when the controls are read or
> > written with V4L2Device::getControls() and V4L2Device::setControls().
> >
> >> + *
> >> + * \return The V4L2 control value. The value might be 0 if the control has
> >> + * never been read from the device before this operation is called.
> >
> > I would drop the second sentence, it's redundant with the above
> > paragraph, and a bit confusing too as the value may also be 0 after
> > being read.
> 
> Ack.
> 
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Control::setValue()
> >> + * \brief Set the value of the control
> >> + * \param value The new V4L2 control value
> >> + *
> >> + * It is important to notice that the value is not applied to the
> >> + * hardware at the time this operation is called, but it will only
> >> + * be actually applied by calling V4L2Device::setControls().
> >
> > Let's simplify it a bit, by explaining what the method does, not what it
> > does not do :-)
> >
> > "This method stores the control value, which will be applied to the
> > device when calling V4L2Device::setControls()."
> >
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Control::id()
> >> + * \brief Retrieve the control ID this instance refers to
> >> + * \return The V4L2Control ID
> >> + */
> >> +
> >> +/**
> >> + * \class V4L2Controls
> >> + * \brief Container of V4L2Control instances
> >> + *
> >> + * The V4L2Controls class works as a container for a list of V4L2Control
> >> + * instances. The class provides operations to add a new control to the list,
> >> + * get back a control value, and reset the list of controls it contains.
> >> + *
> >> + * In order to set and get controls, user of the libcamera V4L2 control
> >> + * framework should operate on instances of the V4L2Controls class, and use
> >> + * them as argument for the V4L2Device::setControls() and
> >> + * V4L2Device::getControls() operations, which write and read a list of
> >> + * controls from or to a V4L2 device (a video device or a subdevice).
> >
> > "to and from" (as it's "write and read")
> >
> >> + *
> >> + * Controls are added to a V4L2Controls instance with the add() method, with
> >> + * or without an initial value.
> >> + *
> >> + * To write controls to a device, the controls of interest shall be added with
> >> + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
> >
> > s/an initial value/a value/
> >
> >> + * value) to prepare for a write operation. Once the values of all controls of
> >> + * interest have been added, the V4L2Controls instance is passed to the
> >> + * V4L2Device::setControls(), which sets the controls on the device.
> >> + *
> >> + * To read controls from a device, the desired controls are added with
> >> + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> >> + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> >> + * reads the controls from the device and updates the values stored in
> >> + * V4L2Controls.
> >> + *
> >> + * V4L2Controls instances can be reset to remove all controls they contain and
> >> + * prepare to be re-used for a new control write/read sequence.
> >> + */
> >> +
> >> +/**
> >> + * \typedef V4L2Controls::iterator
> >> + * \brief Iterator on the V4L2 controls contained in the instance
> >> + */
> >> +
> >> +/**
> >> + * \typedef V4L2Controls::const_iterator
> >> + * \brief Const iterator on the V4L2 controls contained in the instance
> >> + */
> >> +
> >> +/**
> >> + * \fn iterator V4L2Controls::begin()
> >> + * \brief Retrieve an iterator to the first V4L2Control in the instance
> >> + * \return An iterator to the first V4L2 control
> >> + */
> >> +
> >> +/**
> >> + * \fn const_iterator V4L2Controls::begin() const
> >> + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> >> + * \return A constant iterator to the first V4L2 control
> >> + */
> >> +
> >> +/**
> >> + * \fn iterator V4L2Controls::end()
> >> + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> >> + * instance
> >> + * \return An iterator to the element following the last V4L2 control in the
> >> + * instance
> >> + */
> >> +
> >> +/**
> >> + * \fn const_iterator V4L2Controls::end() const
> >> + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> >> + * in the instance
> >> + * \return A constant iterator to the element following the last V4L2 control
> >> + * in the instance
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Controls::empty()
> >> + * \brief Verify if the instance does not contain any control
> >> + * \return True if the instance does not contain any control, false otherwise
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Controls::size()
> >> + * \brief Retrieve the number on controls in the instance
> >> + * \return The number of V4L2Control stored in the instance
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Controls::reset()
> >> + * \brief Removes all controls in the instance
> >
> > s/Removes/Remove/
> >
> >> + */
> >> +
> >> +/**
> >> + * \brief Add control with ID \a id and optional \a value to the instance
> >> + * \param id The V4L2 control ID (V4L2_CID_*)
> >> + * \param value The V4L2 control value
> >> + *
> >> + * V4L2Control can be added with an explicit value, or without one. In that
> >> + * case the control's value is defaulted to 0.
> >
> > s/is defaulted/defaults to/
> >
> > But I think we can skip the sentence, the doxygen documentation should
> > show the = 0.
> 
> Ack
> 
> >> + */
> >> +void V4L2Controls::add(unsigned int id, int64_t value)
> >> +{
> >> +	controls_.emplace_back(id, value);
> >> +}
> >> +
> >> +/**
> >> + * \fn V4L2Controls::operator[](unsigned int index)
> >> + * \brief Access the control at index \a index
> >> + * \param index The index to access
> >> + * \return The V4L2 control at index \a index
> >> + */
> >> +
> >> +/**
> >> + * \brief Get a pointer the control with ID \a id
> >> + * \param id The V4L2 control ID (V4L2_CID_xx)
> >> + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> >> + * control ID is not part of this instance.
> >> + */
> >> +V4L2Control *V4L2Controls::getControl(unsigned int id)
> >> +{
> >> +	for (V4L2Control &ctrl : controls_) {
> >> +		if (ctrl.id() == id)
> >> +			return &ctrl;
> >> +	}
> >> +
> >> +	return nullptr;
> >> +}
> >> +
> >> +} /* namespace libcamera */

Hi Jacopo,

On Mon, Jun 24, 2019 at 10:51:15AM +0200, Jacopo Mondi wrote:
> On Mon, Jun 24, 2019 at 09:36:25AM +0100, Kieran Bingham wrote:
> > On 24/06/2019 09:15, Jacopo Mondi wrote:
> >> On Sat, Jun 22, 2019 at 08:09:06PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Jun 20, 2019 at 05:31:39PM +0200, Jacopo Mondi wrote:
> >>>> Add Libcamera V4L2 control support, implemented using the V4L2 Extended
> >>>
> >>> s/Libcamera/libcamera/ :-)
> >>>
> >>>> Control APIs. This patch defines the types used to define and manage controls.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  src/libcamera/include/v4l2_controls.h |  86 ++++++++
> >>>>  src/libcamera/meson.build             |   1 +
> >>>>  src/libcamera/v4l2_controls.cpp       | 282 ++++++++++++++++++++++++++
> >>>>  3 files changed, 369 insertions(+)
> >>>>  create mode 100644 src/libcamera/include/v4l2_controls.h
> >>>>  create mode 100644 src/libcamera/v4l2_controls.cpp
> >>>>
> >>>> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h
> >>>> new file mode 100644
> >>>> index 000000000000..acd23dabd831
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera/include/v4l2_controls.h
> >>>> @@ -0,0 +1,86 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2019, Google Inc.
> >>>> + *
> >>>> + * v4l2_controls.h - V4L2 Controls Support
> >>>> + */
> >>>> +
> >>>> +#ifndef __LIBCAMERA_V4L2_CONTROLS_H__
> >>>> +#define __LIBCAMERA_V4L2_CONTROLS_H__
> >>>> +
> >>>> +#include <string>
> >>>> +#include <vector>
> >>>> +
> >>>> +#include <stdint.h>
> >>>> +
> >>>> +#include <linux/v4l2-controls.h>
> >>>> +#include <linux/videodev2.h>
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +class V4L2ControlInfo
> >>>> +{
> >>>> +public:
> >>>> +	V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);
> >>>> +
> >>>> +	unsigned int id() const { return id_; }
> >>>> +	unsigned int type() const { return type_; }
> >>>> +	size_t size() const { return size_; }
> >>>> +	const std::string &name() const { return name_; }
> >>>> +
> >>>> +private:
> >>>> +	unsigned int type_;
> >>>> +	std::string name_;
> >>>> +	unsigned int id_;
> >>>> +	size_t size_;
> >>>
> >>> As requested by Niklas,
> >>>
> >>> 	unsigned int id_;
> >>> 	unsigned int type_;
> >>> 	size_t size_;
> >>> 	std::string name_;
> >>>
> >>> ?
> >>>
> >>>> +};
> >>>> +
> >>>> +class V4L2Control
> >>>> +{
> >>>> +public:
> >>>> +	V4L2Control(unsigned int id, int value = 0)
> >>>> +		: id_(id), value_(value) {}
> >>>> +
> >>>> +	int64_t value() const { return value_; }
> >>>> +	void setValue(int64_t value) { value_ = value; }
> >>>> +
> >>>> +	unsigned int id() const { return id_; }
> >>>> +
> >>>> +private:
> >>>> +	unsigned int id_;
> >>>> +	int64_t value_;
> >>>> +};
> >>>> +
> >>>> +class V4L2Controls
> >>>
> >>> I was wondering if we should rename this to V4L2ControlList, as
> >>> V4L2Controls is very close to V4L2Control and easy to mistake.
> >>>
> >>>> +{
> >>>> +public:
> >>>> +	V4L2Controls &operator=(const V4L2Controls &) = delete;
> >>>
> >>> As commented before, I think you should either delete both the
> >>> assignement operator and the copy constructor, or neither of them.
> >>>
> >>>> +
> >>>> +	using iterator = std::vector<V4L2Control>::iterator;
> >>>> +	using const_iterator = std::vector<V4L2Control>::const_iterator;
> >>>> +
> >>>> +	iterator begin() { return controls_.begin(); }
> >>>> +	const_iterator begin() const { return controls_.begin(); }
> >>>> +	iterator end() { return controls_.end(); }
> >>>> +	const_iterator end() const { return controls_.end(); }
> >>>> +
> >>>> +	bool empty() const { return controls_.empty(); }
> >>>> +	size_t size() const { return controls_.size(); }
> >>>
> >>> std::size_t to match std::vector ?
> >>>
> >>>> +
> >>>> +	void reset() { controls_.clear(); }
> >>>> +	void add(unsigned int id, int64_t value = 0);
> >>>> +
> >>>> +	V4L2Control *operator[](unsigned int index)
> >>>> +	{
> >>>> +		return &controls_[index];
> >>>> +	}
> >>>
> >>> Do we need to access a control by index for any other purpose than
> >>> iterating over the controls, which the iterator already gives us ?
> >>> Couldn't operator[] take the control ID, and getControl() be removed ?
> >>>
> >>
> >> I really don't like this.
> >> operator[] for vector works on indexes, I would now use the same
> >> method but with a different semantic. It would be confusing for
> >> everyone. I can drop this operation if you like to.
> >
> > A user of a V4L2ControlList shouldn't know about the internal storage
> > mechanism, and indeed it could be changed without affecting the users.
> 
> Sorry but I don't agree. This class "quacks" like a vector, and mimics
> its interface. We could extend it of course, but diverging seems a
> very bad idea. Furthermore, 99% of usages of the [] operator I can
> think of work on indexes, I don't see a reason to change this and ask
> people to go and look at the documentation to know how operator[]
> works (for no clear gain, in my opinion)

The class provides iterators, and a few methods such as empty(), size()
and reset() (which I think should be called clear() if you want to
mimick the STL API), which are all provided by both std::vector and
std::map. Furthermore operator[] is defined in different ways for
different types of containers, the fact that you use more vectors than
map doesn't mean that the operator mostly works on indices :-)

I'm not saying it has to be turned into a key-based lookup, but if we
don't need index-based lookups, I would remove it.

> > That does add one concern about using a vector - Can two controls be
> > created with the same ID?
> >
> > What happens then?
> > Is that a valid use case?
> 
> That's a real concern instead. Nothing in the V4L2 extended control
> APIs prescribes to have unique entries, so if that happens here, this
> won't be an issue at the V4L2 control level, but it might be confusing
> for application and lead to some subtile bug when the same
> V4L2ControlList is manipulated by different methods.
> 
> So, it's not a bug, but it might be desirable to make sure that
> "add()" only adds unique entries, but at the same time this is not an
> API for applications, and pipeline handlers should know better. I'm
> debated, slightly leaning to leave it the way it is now.

If we allow add() to create multiple entries with the same ID then
getControl() won't work anymore. I don't see a use case for this, so I
would make the ID unique.

> >>>> +
> >>>> +	V4L2Control *getControl(unsigned int id);
> >>>> +
> >>>> +private:
> >>>> +	std::vector<V4L2Control> controls_;
> >>>> +};
> >>>> +
> >>>> +} /* namespace libcamera */
> >>>> +
> >>>> +#endif /* __LIBCAMERA_V4L2_CONTROLS_H__ */
> >>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >>>> index f26ad5b2dc57..985aa7e8ab0e 100644
> >>>> --- a/src/libcamera/meson.build
> >>>> +++ b/src/libcamera/meson.build
> >>>> @@ -23,6 +23,7 @@ libcamera_sources = files([
> >>>>      'stream.cpp',
> >>>>      'timer.cpp',
> >>>>      'utils.cpp',
> >>>> +    'v4l2_controls.cpp',
> >>>>      'v4l2_device.cpp',
> >>>>      'v4l2_subdevice.cpp',
> >>>>      'v4l2_videodevice.cpp',
> >>>> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp
> >>>> new file mode 100644
> >>>> index 000000000000..abf596e7f1ef
> >>>> --- /dev/null
> >>>> +++ b/src/libcamera/v4l2_controls.cpp
> >>>> @@ -0,0 +1,282 @@
> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>> +/*
> >>>> + * Copyright (C) 2019, Google Inc.
> >>>> + *
> >>>> + * v4l2_controls.cpp - V4L2 Controls Support
> >>>> + */
> >>>> +
> >>>> +#include "v4l2_controls.h"
> >>>> +
> >>>> +/**
> >>>> + * \file v4l2_controls.h
> >>>> + * \brief Support for V4L2 Controls using the V4L2 Extended Controls APIs.
> >>>
> >>> s/\.$//
> >>>
> >>>> + *
> >>>> + * The V4L2 defined "Control API" allows application to inspect and modify set
> >>>
> >>> "The V4L2 Control API allows applications ..."
> >>>
> >>> s/set/sets/
> >>>
> >>>> + * of configurable parameters on the video device or subdevice of interest. The
> >>>
> >>> s/on the video device or subdevice of interest/on a video device or subdevice/
> >>>
> >>>> + * nature of the parameters an application could modify using the control
> >>>
> >>> s/could/can/
> >>>
> >>>> + * framework depends on what the driver implements support for, and on the
> >>>> + * characteristics of the underlying hardware platform. Generally controls are
> >>>> + * used to modify user visible settings, such as the image brightness and
> >>>> + * exposure time, or non-standard parameters which cannot be controlled through
> >>>> + * the V4L2 format negotiation API.
> >>>
> >>> The last two sentences describe more the V4L2 API than the V4L2Control*
> >>> classes API. I expect users of this code to know about V4L2 already, so
> >>> they could be dropped, but I also don't mind if you keep them. Just
> >>> don't invest too much time in writing document about V4L2 itself, as
> >>> that's a bit out of scope.
> >>>
> >>
> >> I feel it doesn't hurt, as it doesn't go in great lenght describing
> >> controls but I'll drop.
> >>
> >>>> + *
> >>>> + * Controls are identified by a numerical ID, defined by the V4L2 kernel headers
> >>>> + * and have an associated type. Each control has a value, which is the data
> >>>> + * that can be modified with V4L2Device::setControls() or retrieved with
> >>>> + * V4L2Device::getControls().
> >>>> + *
> >>>> + * The control's type along with the control's flags defines the type of the
> >>>
> >>> s/defines/define/
> >>>
> >>>> + * control's value content. Controls might transport a single data value
> >>>
> >>> s/might/can/
> >>>
> >>>> + * stored in variable inside the control, or they might as well deal with more
> >>>
> >>> s/might/can/
> >>>
> >>>> + * complex data types, such as arrays of matrices, stored in a contiguous
> >>>> + * memory locations associated with the control and called 'the payload'. Such
> >>>> + * controls are called 'compound controls' and are currently not supported by
> >>>> + * the libcamera V4L2 control framework.
> >>>
> >>> Here too you could simply have said that we don't support compound
> >>> controls yet, but I don't mind keeping the more detailed text as-is.
> >>>
> >>>> + *
> >>>> + * libcamera implements support for controls using the V4L2 'Extended Control'
> >>>> + * API, which allows future handling of controls with payloads of arbitrary
> >>>
> >>> It's not the controls that are extended, it's the API. It's the extended
> >>> V4L2 control API, not the V4L2 API for extended controls. I would thus
> >>> remove the quotes.
> >>>
> >>
> >> I see. I should have moved the ending quote to include the "API" part.
> >>
> >>>> + * sizes.
> >>>> + *
> >>>> + * The libcamera V4L2 Controls framework operates on lists of controls,
> >>>> + * wrapped by the V4L2Controls class, to match the V4L2 extended controls API.
> >>>
> >>> Calling it V4L2ControlList would match the description nicely :-)
> >>>
> >>
> >> Ack
> >>
> >>>> + * The interface to set and get control is implemented by the V4L2Device
> >>>> + * class, and this file only provides the data type definitions.
> >>>> + *
> >>>> + * \todo Add support for compound controls
> >>>> + */
> >>>> +
> >>>> +namespace libcamera {
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2ControlInfo
> >>>> + * \brief Information on a V4L2 control
> >>>> + *
> >>>> + * The V4L2ControlInfo class represent all the information related to
> >>>
> >>> s/represent/represents/
> >>>
> >>>> + * a V4L2 control, such as its ID, its type, its user-readable name and the
> >>>> + * expected size of its value data.
> >>>> + *
> >>>> + * V4L2ControlInfo instances are created by inspecting the fieldS of a struct
> >>>> + * v4l2_query_ext_ctrl structure, after it has been filled by the device
> >>>> + * driver as a consequence of a VIDIOC_QUERY_EXT_CTRL ioctl call.
> >>>> + *
> >>>> + * This class does not contain the control value, but only static information
> >>>> + * on the control, which shall be cached by the caller at initialisation time
> >>>> + * or the first time the control information is accessed.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Construct a V4L2ControlInfo from a struct v4l2_query_ext_ctrl
> >>>> + * \param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel
> >>>> + */
> >>>> +V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)
> >>>> +{
> >>>> +	id_ = ctrl.id;
> >>>> +	type_ = ctrl.type;
> >>>> +	name_ = static_cast<const char *>(ctrl.name);
> >>>> +	size_ = ctrl.elem_size * ctrl.elems;
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::id()
> >>>> + * \brief Retrieve the control ID
> >>>> + * \return The V4L2 control ID
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::type()
> >>>> + * \brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*
> >>>> + * \return The V4L2 control type
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::size()
> >>>> + * \brief Retrieve the control value data size (in bytes)
> >>>> + * \return The V4L2 control value data size
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2ControlInfo::name()
> >>>> + * \brief Retrieve the control user readable name
> >>>> + * \return The V4L2 control user readable name
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2Control
> >>>> + * \brief A V4L2 control value
> >>>> + *
> >>>> + * The V4L2Control class represent the value of a V4L2 control.
> >>>> + * The class stores values that have been read from or will be applied to
> >>>> + * a V4L2 device.
> >>>> + *
> >>>> + * The value stored in the class instances does not reflect what is actually
> >>>> + * applied to the hardware but is a pure software cache optionally initialized
> >>>> + * at control creation time and only modified by a control read operation.
> >>>
> >>> With setControls() updating the value, this isn't true anymore.
> >>>
> >>
> >> True. Nice catch.
> >>
> >>>> + *
> >>>> + * The write and read controls the V4L2Control class instances are not meant
> >>>> + * to be directly used but are instead intended to be grouped in
> >>>> + * V4L2Controls instances, which are then passed as parameters to
> >>>> + * V4L2Device::setControls() and V4L2Device::getControls() operations.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::V4L2Control
> >>>> + * \brief Construct a V4L2 control with ID \a id and value \a value
> >>>
> >>> "with an \a id and a \a value" ? Or "Construct an instance of control \a
> >>> ID with a \a value" ?
> >>>
> >>
> >> I'm always uncertain on how to handle these situation, where the name
> >> of the parameter matches the generic name.
> >>
> >>>> + * \param id The V4L2 control ID
> >>>> + * \param value The control value
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::value()
> >>>> + * \brief Retrieve the value of the control
> >>>> + *
> >>>> + * It is important to notice that the returned value is not read from the
> >>>
> >>> s/notice/note/
> >>>
> >>>> + * hardware at the time this operation is called, but it's the cached value
> >>>> + * obtained the most recent time the control has been read with
> >>>> + * V4L2Device::getControls() or the value of the control has been set to by the
> >>>> + * user with the add() operation.
> >>>
> >>> You should mention setControls() here. How about
> >>>
> >>> This method returns the cached control value, initially set by
> >>> V4L2Controls::add() and then updated when the controls are read or
> >>> written with V4L2Device::getControls() and V4L2Device::setControls().
> >>>
> >>>> + *
> >>>> + * \return The V4L2 control value. The value might be 0 if the control has
> >>>> + * never been read from the device before this operation is called.
> >>>
> >>> I would drop the second sentence, it's redundant with the above
> >>> paragraph, and a bit confusing too as the value may also be 0 after
> >>> being read.
> >>>
> >>
> >> Ack.
> >>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::setValue()
> >>>> + * \brief Set the value of the control
> >>>> + * \param value The new V4L2 control value
> >>>> + *
> >>>> + * It is important to notice that the value is not applied to the
> >>>> + * hardware at the time this operation is called, but it will only
> >>>> + * be actually applied by calling V4L2Device::setControls().
> >>>
> >>> Let's simplify it a bit, by explaining what the method does, not what it
> >>> does not do :-)
> >>>
> >>> "This method stores the control value, which will be applied to the
> >>> device when calling V4L2Device::setControls()."
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Control::id()
> >>>> + * \brief Retrieve the control ID this instance refers to
> >>>> + * \return The V4L2Control ID
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \class V4L2Controls
> >>>> + * \brief Container of V4L2Control instances
> >>>> + *
> >>>> + * The V4L2Controls class works as a container for a list of V4L2Control
> >>>> + * instances. The class provides operations to add a new control to the list,
> >>>> + * get back a control value, and reset the list of controls it contains.
> >>>> + *
> >>>> + * In order to set and get controls, user of the libcamera V4L2 control
> >>>> + * framework should operate on instances of the V4L2Controls class, and use
> >>>> + * them as argument for the V4L2Device::setControls() and
> >>>> + * V4L2Device::getControls() operations, which write and read a list of
> >>>> + * controls from or to a V4L2 device (a video device or a subdevice).
> >>>
> >>> "to and from" (as it's "write and read")
> >>>
> >>>> + *
> >>>> + * Controls are added to a V4L2Controls instance with the add() method, with
> >>>> + * or without an initial value.
> >>>> + *
> >>>> + * To write controls to a device, the controls of interest shall be added with
> >>>> + * an initial value by calling V4L2Controls::add(unsigned int id, int64_t
> >>>
> >>> s/an initial value/a value/
> >>>
> >>>> + * value) to prepare for a write operation. Once the values of all controls of
> >>>> + * interest have been added, the V4L2Controls instance is passed to the
> >>>> + * V4L2Device::setControls(), which sets the controls on the device.
> >>>> + *
> >>>> + * To read controls from a device, the desired controls are added with
> >>>> + * V4L2Controls::add(unsigned int id) to prepare for a read operation. The
> >>>> + * V4L2Controls instance is then passed to V4L2Device::getControls(), which
> >>>> + * reads the controls from the device and updates the values stored in
> >>>> + * V4L2Controls.
> >>>> + *
> >>>> + * V4L2Controls instances can be reset to remove all controls they contain and
> >>>> + * prepare to be re-used for a new control write/read sequence.
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \typedef V4L2Controls::iterator
> >>>> + * \brief Iterator on the V4L2 controls contained in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \typedef V4L2Controls::const_iterator
> >>>> + * \brief Const iterator on the V4L2 controls contained in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn iterator V4L2Controls::begin()
> >>>> + * \brief Retrieve an iterator to the first V4L2Control in the instance
> >>>> + * \return An iterator to the first V4L2 control
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn const_iterator V4L2Controls::begin() const
> >>>> + * \brief Retrieve a constant iterator to the first V4L2Control in the instance
> >>>> + * \return A constant iterator to the first V4L2 control
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn iterator V4L2Controls::end()
> >>>> + * \brief Retrieve an iterator pointing to the past-the-end V4L2Control in the
> >>>> + * instance
> >>>> + * \return An iterator to the element following the last V4L2 control in the
> >>>> + * instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn const_iterator V4L2Controls::end() const
> >>>> + * \brief Retrieve a constant iterator pointing to the past-the-end V4L2Control
> >>>> + * in the instance
> >>>> + * \return A constant iterator to the element following the last V4L2 control
> >>>> + * in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::empty()
> >>>> + * \brief Verify if the instance does not contain any control
> >>>> + * \return True if the instance does not contain any control, false otherwise
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::size()
> >>>> + * \brief Retrieve the number on controls in the instance
> >>>> + * \return The number of V4L2Control stored in the instance
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::reset()
> >>>> + * \brief Removes all controls in the instance
> >>>
> >>> s/Removes/Remove/
> >>>
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Add control with ID \a id and optional \a value to the instance
> >>>> + * \param id The V4L2 control ID (V4L2_CID_*)
> >>>> + * \param value The V4L2 control value
> >>>> + *
> >>>> + * V4L2Control can be added with an explicit value, or without one. In that
> >>>> + * case the control's value is defaulted to 0.
> >>>
> >>> s/is defaulted/defaults to/
> >>>
> >>> But I think we can skip the sentence, the doxygen documentation should
> >>> show the = 0.
> >>
> >> Ack
> >>
> >>>> + */
> >>>> +void V4L2Controls::add(unsigned int id, int64_t value)
> >>>> +{
> >>>> +	controls_.emplace_back(id, value);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * \fn V4L2Controls::operator[](unsigned int index)
> >>>> + * \brief Access the control at index \a index
> >>>> + * \param index The index to access
> >>>> + * \return The V4L2 control at index \a index
> >>>> + */
> >>>> +
> >>>> +/**
> >>>> + * \brief Get a pointer the control with ID \a id
> >>>> + * \param id The V4L2 control ID (V4L2_CID_xx)
> >>>> + * \return A pointer to the V4L2Control with ID \a id or nullptr if the
> >>>> + * control ID is not part of this instance.
> >>>> + */
> >>>> +V4L2Control *V4L2Controls::getControl(unsigned int id)
> >>>> +{
> >>>> +	for (V4L2Control &ctrl : controls_) {
> >>>> +		if (ctrl.id() == id)
> >>>> +			return &ctrl;
> >>>> +	}
> >>>> +
> >>>> +	return nullptr;
> >>>> +}
> >>>> +
> >>>> +} /* namespace libcamera */