[libcamera-devel,v3,04/14] libcamera: controls: Introduce control-related data types

1546 diff mbox series

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Add a set of data types to support controls:

- ControlValue stores a control type and value in a generic way
- ControlId enumerates all the control identifies
- ControlIdentier declares the types of a control and map their names
- ControlInfo stores runtime information for controls
- ControlList contains a set of control info and value pairs

The control definitions map is generated from the controls documentation
to ensure that the two will always be synchronised.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Squashed "Provide ControlValue class"
- Renamed Value to ControlValue
- Removed operator<<()
- Added control table generation
- Moved control definitions to control_definitions.h
- Renamed ControlTypes to controlsTypes and make it const
- Moved the initial controls list to a separate patch
- Renamed control_definitions.h to control_ids.h and
  control_definitions.cpp to control_types.cpp to match the contained
  enum and variable name respectively
- Indexed ControlList by ControlInfo pointer instead of value
- Replaced ControlInfoHash with std::hash specialisation
- Added automatic conversion between 32- and 64-bit integer values

The automatic conversion between integer types was prompted by an
assertion failure due to the use of getInt() on the min() and max()
value of an Integer control. The min and max ControlValue instances are
create as Integer64, due to the V4L2ControlInfo class returning the
range as int64_t. This may need to be reworked.
---
 Documentation/Doxyfile.in       |   3 +-
 include/libcamera/control_ids.h |  35 +++
 include/libcamera/controls.h    | 134 ++++++++++
 include/libcamera/meson.build   |   2 +
 src/libcamera/controls.cpp      | 428 ++++++++++++++++++++++++++++++++
 src/libcamera/gen-controls.awk  | 106 ++++++++
 src/libcamera/meson.build       |  11 +
 7 files changed, 718 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/control_ids.h
 create mode 100644 include/libcamera/controls.h
 create mode 100644 src/libcamera/controls.cpp
 create mode 100755 src/libcamera/gen-controls.awk

Hi Laurent,

On 01/07/2019 00:38, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Add a set of data types to support controls:
> 
> - ControlValue stores a control type and value in a generic way
> - ControlId enumerates all the control identifies

s/identifies/identities/ ?

> - ControlIdentier declares the types of a control and map their names

s/map/maps/

> - ControlInfo stores runtime information for controls
> - ControlList contains a set of control info and value pairs
> 
> The control definitions map is generated from the controls documentation
> to ensure that the two will always be synchronised.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Some minor review comments sprinkled throughout, but otherwise:

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



> ---
> Changes since v2:
> 
> - Squashed "Provide ControlValue class"
> - Renamed Value to ControlValue
> - Removed operator<<()

:-( Do you dislike this? or is this just to simplify things.

I had considered going round the rest of the codebase and adding
operator<<() where we had toString() ... but I guess I won't do that if
you're nacking it here.


> - Added control table generation
> - Moved control definitions to control_definitions.h
> - Renamed ControlTypes to controlsTypes and make it const

Presumably you mean controlTypes there (no extra s).

> - Moved the initial controls list to a separate patch
> - Renamed control_definitions.h to control_ids.h and
>   control_definitions.cpp to control_types.cpp to match the contained
>   enum and variable name respectively
> - Indexed ControlList by ControlInfo pointer instead of value
> - Replaced ControlInfoHash with std::hash specialisation

But this affects the std namespace right? I chose to put a function
object in specifically to keep libcamera code in the libcamera namespace.

<edit: Never mind:
https://en.cppreference.com/w/cpp/language/extending_std states that
specialisations of std::hash are allowed>


> - Added automatic conversion between 32- and 64-bit integer values
> 
> The automatic conversion between integer types was prompted by an
> assertion failure due to the use of getInt() on the min() and max()
> value of an Integer control. The min and max ControlValue instances are
> create as Integer64, due to the V4L2ControlInfo class returning the
> range as int64_t. This may need to be reworked.



> ---
>  Documentation/Doxyfile.in       |   3 +-
>  include/libcamera/control_ids.h |  35 +++
>  include/libcamera/controls.h    | 134 ++++++++++
>  include/libcamera/meson.build   |   2 +
>  src/libcamera/controls.cpp      | 428 ++++++++++++++++++++++++++++++++
>  src/libcamera/gen-controls.awk  | 106 ++++++++
>  src/libcamera/meson.build       |  11 +
>  7 files changed, 718 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/control_ids.h
>  create mode 100644 include/libcamera/controls.h
>  create mode 100644 src/libcamera/controls.cpp
>  create mode 100755 src/libcamera/gen-controls.awk
> 
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index c58631200dd5..9ca32241b895 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -868,7 +868,8 @@ EXCLUDE_SYMBOLS        = libcamera::SignalBase \
>                           libcamera::SlotArgs \
>                           libcamera::SlotBase \
>                           libcamera::SlotMember \
> -                         libcamera::SlotStatic
> +                         libcamera::SlotStatic \
> +                         std::*
>  
>  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
>  # that contain example code fragments that are included (see the \include
> diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> new file mode 100644
> index 000000000000..d0e700da9844
> --- /dev/null
> +++ b/include/libcamera/control_ids.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * control_ids.h : Control ID list
> + */
> +
> +#ifndef __LIBCAMERA_CONTROL_IDS_H__
> +#define __LIBCAMERA_CONTROL_IDS_H__
> +
> +#include <functional>
> +
> +namespace libcamera {
> +
> +enum ControlId {
> +};
> +
> +} /* namespace libcamera */
> +
> +namespace std {
> +
> +template<>
> +struct hash<libcamera::ControlId> {
> +	using argument_type = libcamera::ControlId;
> +	using result_type = std::size_t;
> +
> +	result_type operator()(const argument_type &key) const noexcept
> +	{
> +		return std::hash<std::underlying_type<argument_type>::type>()(key);
> +	}
> +};
> +
> +} /* namespace std */
> +
> +#endif // __LIBCAMERA_CONTROL_IDS_H__
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> new file mode 100644
> index 000000000000..ad2d49d522c5
> --- /dev/null
> +++ b/include/libcamera/controls.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * controls.h - Control handling
> + */
> +
> +#ifndef __LIBCAMERA_CONTROLS_H__
> +#define __LIBCAMERA_CONTROLS_H__
> +
> +#include <stdint.h>
> +#include <string>
> +#include <unordered_map>
> +
> +#include <libcamera/control_ids.h>
> +
> +namespace libcamera {
> +
> +class Camera;
> +
> +enum ControlValueType {
> +	ControlValueNone,
> +	ControlValueBool,
> +	ControlValueInteger,
> +	ControlValueInteger64,
> +};
> +
> +class ControlValue
> +{
> +public:
> +	ControlValue();
> +	ControlValue(bool value);
> +	ControlValue(int value);
> +	ControlValue(int64_t value);
> +
> +	ControlValueType type() const { return type_; };
> +	bool isNone() const { return type_ == ControlValueNone; };
> +
> +	void set(bool value);
> +	void set(int value);
> +	void set(int64_t value);
> +
> +	bool getBool() const;
> +	int getInt() const;
> +	int64_t getInt64() const;
> +
> +	std::string toString() const;
> +
> +private:
> +	ControlValueType type_;
> +
> +	union {
> +		bool bool_;
> +		int integer_;
> +		int64_t integer64_;
> +	};
> +};
> +
> +struct ControlIdentifier {
> +	ControlId id;
> +	const char *name;
> +	ControlValueType type;
> +};
> +
> +class ControlInfo
> +{
> +public:
> +	explicit ControlInfo(ControlId id, const ControlValue &min = 0,
> +			     const ControlValue &max = 0);
> +
> +	ControlId id() const { return ident_->id; }
> +	const char *name() const { return ident_->name; }
> +	ControlValueType type() const { return ident_->type; }
> +
> +	const ControlValue &min() const { return min_; }
> +	const ControlValue &max() const { return max_; }
> +
> +	std::string toString() const;
> +
> +private:
> +	const struct ControlIdentifier *ident_;
> +	ControlValue min_;
> +	ControlValue max_;
> +};
> +
> +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);
> +bool operator==(const ControlId &lhs, const ControlInfo &rhs);
> +bool operator==(const ControlInfo &lhs, const ControlId &rhs);
> +static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +
> +class ControlList
> +{
> +private:
> +	using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;
> +
> +public:
> +	ControlList(Camera *camera);
> +
> +	using iterator = ControlListMap::iterator;
> +	using const_iterator = ControlListMap::const_iterator;
> +
> +	iterator begin() { return controls_.begin(); }
> +	iterator end() { return controls_.end(); }
> +	const_iterator begin() const { return controls_.begin(); }
> +	const_iterator end() const { return controls_.end(); }
> +
> +	bool contains(const ControlInfo *info) const { return controls_.count(info); };

I thought I had already updated this to use .find() ... perhaps it got
lost in the merge/handover.

Though I see you've later implemented a contains(ControlId id) and left
this as .count(). Was it intentional or just time constraints?

Because contains() has changed type this looks somewhat intentional ...

Also - extraneous semi-colon on the end:
   s/; };/; }/


> +	bool empty() const { return controls_.empty(); }
> +	std::size_t size() const { return controls_.size(); }
> +	void clear() { controls_.clear(); }
> +
> +	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
> +
> +	void update(const ControlList &list);
> +
> +private:
> +	Camera *camera_;
> +	ControlListMap controls_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CONTROLS_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 15484724df01..3067120a1598 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -2,6 +2,8 @@ libcamera_api = files([
>      'buffer.h',
>      'camera.h',
>      'camera_manager.h',
> +    'control_ids.h',
> +    'controls.h',
>      'event_dispatcher.h',
>      'event_notifier.h',
>      'geometry.h',
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> new file mode 100644
> index 000000000000..22db2b93eff2
> --- /dev/null
> +++ b/src/libcamera/controls.cpp
> @@ -0,0 +1,428 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * controls.cpp - Control handling
> + */
> +
> +#include <libcamera/controls.h>
> +
> +#include <sstream>
> +#include <string>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +/**
> + * \file controls.h
> + * \brief Describes control framework and controls supported by a camera
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Controls)
> +
> +/**
> + * \enum ControlValueType
> + * Determines the type of value represented by a ControlValue
> + * \var ControlValueNone
> + * Identifies an unset control value
> + * \var ControlValueBool
> + * Identifies controls storing a boolean value
> + * \var ControlValueInteger
> + * Identifies controls storing an integer value
> + * \var ControlValueInteger64
> + * Identifies controls storing a 64-bit integer value
> + */
> +
> +/**
> + * \class ControlValue
> + * \brief Abstract type for values in a control
> + */
> +
> +/**
> + * \brief Construct an empty ControlValue.
> + */
> +ControlValue::ControlValue()
> +	: type_(ControlValueNone)
> +{
> +}
> +
> +/**
> + * \brief Construct a Boolean ControlValue
> + * \param[in] value Boolean value to store
> + */
> +ControlValue::ControlValue(bool value)
> +	: type_(ControlValueBool), bool_(value)
> +{
> +}
> +
> +/**
> + * \brief Construct an integer ControlValue
> + * \param[in] value Integer value to store
> + */
> +ControlValue::ControlValue(int value)
> +	: type_(ControlValueInteger), integer_(value)
> +{
> +}
> +
> +/**
> + * \brief Construct a 64 bit integer ControlValue
> + * \param[in] value String representation to store
> + */
> +ControlValue::ControlValue(int64_t value)
> +	: type_(ControlValueInteger64), integer64_(value)
> +{
> +}
> +
> +/**
> + * \fn ControlValue::type
> + * \brief Return the type of value represented by the object
> + */
> +
> +/**
> + * \fn ControlValue::isNone
> + * \brief Determine if the value is initialised
> + * \return True if the value type is ControlValueNone, false otherwise
> + */
> +
> +/**
> + * \brief Set the value with a boolean
> + * \param[in] value Boolean value to store
> + */
> +void ControlValue::set(bool value)
> +{
> +	type_ = ControlValueBool;
> +	bool_ = value;
> +}
> +
> +/**
> + * \brief Set the value with an integer
> + * \param[in] value Integer value to store
> + */
> +void ControlValue::set(int value)
> +{
> +	type_ = ControlValueInteger;
> +	integer_ = value;
> +}
> +
> +/**
> + * \brief Set the value with a 64 bit integer
> + * \param[in] value 64 bit integer value to store
> + */
> +void ControlValue::set(int64_t value)
> +{
> +	type_ = ControlValueInteger64;
> +	integer64_ = value;
> +}
> +
> +/**
> + * \brief Get the boolean value.
> + *
> + * The value type must be Boolean.
> + */
> +bool ControlValue::getBool() const
> +{
> +	ASSERT(type_ == ControlValueBool);
> +
> +	return bool_;
> +}
> +
> +/**
> + * \brief Get the integer value.
> + *
> + * The value type must be Integer or Integer64
> + */
> +int ControlValue::getInt() const
> +{
> +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> +
> +	return integer_;
> +}
> +
> +/**
> + * \brief Get the 64 bit integer value.
> + *
> + * The value type must be Integer or Integer64
> + */
> +int64_t ControlValue::getInt64() const
> +{
> +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> +
> +	return integer64_;
> +}
> +
> +/**
> + * \brief Assemble and return a string describing the value
> + * \return A string describing the ControlValue
> + */
> +std::string ControlValue::toString() const
> +{
> +	switch (type_) {
> +	case ControlValueNone:
> +		return "<None>";
> +	case ControlValueBool:
> +		return bool_ ? "True" : "False";
> +	case ControlValueInteger:
> +		return std::to_string(integer_);
> +	case ControlValueInteger64:
> +		return std::to_string(integer64_);
> +	}
> +
> +	return "<ValueType Error>";
> +}
> +
> +/**
> + * \enum ControlId
> + * Control Identifiers
> + */
> +
> +/**
> + * \struct ControlIdentifier
> + * \brief Describes a ControlId with control specific constant meta-data.
> + *
> + * Defines a Control with a unique ID, a name, and a type.
> + * This structure is used as static part of the autogenerated control

s/autogenerated/auto-generated/

> + * definitions, which are generated from the ControlId documentation.
> + *
> + * \var ControlIdentifier::id
> + * The unique ID for a control
> + * \var ControlIdentifier::name
> + * The string representation of the control
> + * \var ControlIdentifier::type
> + * The ValueType required to represent the control value
> + */
> +
> +/*
> + * The controlTypes are automatically generated to produce a control_types.cpp
> + * output. This file is not for public use, and so no suitable header exists
> + * for this sole usage of the controlTypes reference. As such the extern is
> + * only defined here for use during the ControlInfo constructor and should not
> + * be referenced directly elsewhere.
> + */
> +extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> +
> +/**
> + * \class ControlInfo
> + * \brief Describe the information and capabilities of a Control
> + *
> + * The ControlInfo represents control specific meta-data which is constant on a
> + * per camera basis. ControlInfo classes are constructed by pipeline handlers
> + * to expose the controls they support and the metadata needed to utilise those
> + * controls.
> + */
> +
> +/**
> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> + */
> +ControlInfo::ControlInfo(ControlId id, const ControlValue &min,
> +			 const ControlValue &max)
> +	: min_(min), max_(max)
> +{
> +	auto iter = controlTypes.find(id);
> +	if (iter == controlTypes.end()) {
> +		LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> +		return;
> +	}
> +
> +	ident_ = &iter->second;
> +}
> +
> +/**
> + * \fn ControlInfo::id()
> + * \brief Retrieve the ID of the control information descriptor
> + * \return the ControlId
> + */
> +
> +/**
> + * \fn ControlInfo::name()
> + * \brief Retrieve the string name of the control information descriptor
> + * \return A string name for the Control
> + */
> +
> +/**
> + * \fn ControlInfo::type()
> + * \brief Retrieve the ValueType of the control information descriptor
> + * \return The control type
> + */
> +
> +/**
> + * \fn ControlInfo::min()
> + * \brief Reports the minimum value of the control
> + * \return a COntrolValue with the minimum setting for the control

s/COntrolValue/ControlValue/

> + */
> +
> +/**
> + * \fn ControlInfo::max()
> + * \brief Reports the maximum value of the control
> + * \return a ControlValue with the maximum setting for the control
> + */
> +
> +/**
> + * \brief Provide a string representation of the ControlInfo
> + */
> +std::string ControlInfo::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]";
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \brief Compare control information for equality
> + * \param[in] lhs Left-hand side control information
> + * \param[in] rhs Right-hand side control information
> + *
> + * Control information are compared based on their ID only, as a camera may not

information is uncountable:

"Control information is compared based on the ID only, ..."

> + * have two separate controls with the same ID.
> + *
> + * \return True if \a lhs and \a rhs are equal, false otherwise
> + */
> +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)
> +{
> +	return lhs.id() == rhs.id();
> +}
> +
> +/**
> + * \brief Compare control ID and information for equality
> + * \param[in] lhs Left-hand side control identifier
> + * \param[in] rhs Right-hand side control information
> + *
> + * Control information are compared based on their ID only, as a camera may not

Same as above.

> + * have two separate controls with the same ID.
> + *
> + * \return True if \a lhs and \a rhs are equal, false otherwise
> + */
> +bool operator==(const ControlId &lhs, const ControlInfo &rhs)
> +{
> +	return lhs == rhs.id();
> +}
> +
> +/**
> + * \brief Compare control information and ID for equality
> + * \param[in] lhs Left-hand side control information
> + * \param[in] rhs Right-hand side control identifier
> + *
> + * Control information are compared based on their ID only, as a camera may not

Same as above.

> + * have two separate controls with the same ID.
> + *
> + * \return True if \a lhs and \a rhs are equal, false otherwise
> + */
> +bool operator==(const ControlInfo &lhs, const ControlId &rhs)
> +{
> +	return lhs.id() == rhs;
> +}
> +
> +/**
> + * \class ControlList
> + * \brief Associates a list of ControlIds with their values for a Camera.
> + *
> + * A ControlList specifies a map of ControlIds and Values and associated
> + * validation against the ControlInfo for the related Camera device.
> + */
> +
> +/**
> + * \brief Construct a ControlList with a reference to the Camera it applies on
> + */
> +ControlList::ControlList(Camera *camera)
> +	: camera_(camera)
> +{
> +}
> +
> +/**
> + * \typedef ControlList::iterator
> + * \brief Iterator for the controls contained within the list.
> + */
> +
> +/**
> + * \typedef ControlList::const_iterator
> + * \brief Const iterator for the controls contained within the list.
> + */
> +
> +/**
> + * \fn iterator ControlList::begin()
> + * \brief Retrieve an iterator to the first Control in the list
> + * \return An iterator to the first Control in the list
> + */
> +
> +/**
> + * \fn iterator ControlList::end()
> + * \brief Retrieve an iterator to the next element after the last controls in
> + * the instance.
> + * \return An iterator to the element following the last control in the instance
> + */
> +
> +/**
> + * \fn const_iterator ControlList::begin() const
> + * \brief Retrieve a const_iterator to the first Control in the list
> + * \return A const_iterator to the first Control in the list
> + */
> +
> +/**
> + * \fn const_iterator ControlList::end() const
> + * \brief Retrieve a constant iterator pointing to an empty element after the
> + * \return A const iterator to the element following the last control in the
> + * instance
> + */
> +
> +/**
> + * \fn ControlList::contains(const ControlInfo *info) const
> + * \brief Check if the ist contains a control with the specified \a info
> + * \param[in] info The control info
> + * \return True if the list contains a matching control, false otherwise
> + */
> +
> +/**
> + * \fn ControlList::empty()
> + * \brief Identify if the list is empty
> + * \return True if the list does not contain any control, false otherwise
> + */
> +
> +/**
> + * \fn ControlList::size()
> + * \brief Retrieve the number of controls in the list
> + * \return The number of Control entries stored in the list
> + */
> +
> +/**
> + * \fn ControlList::clear()
> + * \brief Removes all controls from the list
> + */
> +
> +/**
> + * \fn ControlList::operator[](const ControlInfo *info)
> + * \brief Access or insert the control specified by \a info
> + * \param[in] info The control info
> + *
> + * This method returns a reference to the control identified by \a info,
> + * inserting it in the list if the info is not already present.
> + *
> + * \return A reference to the value of the control identified by \a info
> + */
> +
> +/**
> + * \brief Update all Control values with the value from the given \a list
> + * \param list The list of controls to update or append to this list
> + *
> + * Update all controls in the ControlList, by the values given by \a list
> + * If the list already contains a control of this ID then it will be overwritten
> + */
> +void ControlList::update(const ControlList &list)
> +{
> +	if (list.camera_ != camera_) {
> +		LOG(Controls, Error)
> +			<< "ControlLists can not be translated between cameras";
> +		return;
> +	}
> +
> +	for (auto it : list) {
> +		const ControlInfo *info = it.first;
> +		const ControlValue &value = it.second;
> +
> +		controls_[info] = value;
> +	}
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> new file mode 100755
> index 000000000000..a91529b575db
> --- /dev/null
> +++ b/src/libcamera/gen-controls.awk
> @@ -0,0 +1,106 @@
> +#!/usr/bin/awk -f
> +
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +
> +# Controls are documented using Doxygen in the main control.cpp source.

s/control.cpp/controls.cpp/

> +#
> +# Generate control tables directly from the documentation, creating enumerations
> +# to support the IDs and static type information regarding each control.
> +
> +BEGIN {
> +	id=0
> +	input=ARGV[1]
> +	mode=ARGV[2]
> +	output=ARGV[3]
> +}
> +
> +# Detect Doxygen style comment blocks and ignore other lines
> +/^\/\*\*$/ { in_doxygen=1; first_line=1; next }
> +// { if (!in_doxygen) next }
> +
> +# Entry point for the Control Documentation
> +/ * \\enum ControlId$/ { in_controls=1; first_line=0; next }
> +// { if (!in_controls) next }
> +
> +# Extract control information
> +/ \* \\var/ { names[++id]=$3; first_line=0; next }
> +/ \* ControlType:/ { types[id] = $3 }
> +
> +# End of comment blocks
> +/^ \*\// { in_doxygen=0 }
> +
> +# Identify the end of controls
> +/^ \* \\/ { if (first_line) exit }> +// { first_line=0 }

This first_line=0 appears to be redundant.

'first_line' doesn't seem like a great variable name choice either, but
if we may end up rewriting this at some point - I don't think we need to
dwell on it while it's functional.

> +
> +################################
> +# Support output file generation
> +
> +function basename(file) {
> +	sub(".*/", "", file)
> +	return file
> +}
> +
> +function Header(file, description) {
> +	print "/* SPDX-License-Identifier: LGPL-2.1-or-later */" > file
> +	print "/*" > file
> +	print " * Copyright (C) 2019, Google Inc." > file
> +	print " *" > file
> +	print " * " basename(file) " - " description > file
> +	print " *" > file
> +	print " * This file is autogenerated. Do not edit." > file

I would say s/autogenerated/auto-generated/ but I think perhaps
autogenerated might be fairly commonly used.


> +	print " */" > file
> +	print "" > file
> +}
> +
> +function EnterNameSpace(file) {
> +	print "namespace libcamera {" > file
> +	print "" > file
> +}
> +
> +function ExitNameSpace(file) {
> +	print "" > file
> +	print "} /* namespace libcamera */" > file
> +}
> +
> +function GenerateHeader(file) {
> +	Header(file, "Control ID list")
> +
> +	print "#ifndef __LIBCAMERA_CONTROL_IDS_H__" > file
> +	print "#define __LIBCAMERA_CONTROL_IDS_H__" > file
> +	print "" > file
> +
> +	EnterNameSpace(file)
> +	print "enum ControlId {" > file
> +	for (i=1; i <= id; ++i) {
> +		printf "\t%s,\n", names[i] > file
> +	}
> +	print "};" > file
> +	ExitNameSpace(file)
> +
> +	print "" > file
> +	print "#endif // __LIBCAMERA_CONTROL_IDS_H__" > file
> +}
> +
> +function GenerateTable(file) {
> +	Header(file, "Control types")
> +	print "#include <libcamera/controls.h>" > file
> +	print "" > file
> +
> +	EnterNameSpace(file)
> +
> +	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> +	print "controlTypes {" > file
> +	for (i=1; i <= id; ++i) {
> +		printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> +	}
> +	print "};" > file
> +	ExitNameSpace(file)
> +}
> +
> +END {
> +	if (mode == "--header")
> +		GenerateHeader(output)
> +	else
> +		GenerateTable(output)
> +}
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 985aa7e8ab0e..b1ee92735e41 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -3,6 +3,7 @@ libcamera_sources = files([
>      'camera.cpp',
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
> +    'controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
>      'event_dispatcher.cpp',
> @@ -66,6 +67,16 @@ if libudev.found()
>      ])
>  endif
>  
> +gen_controls = files('gen-controls.awk')
> +
> +control_types_cpp = custom_target('control_types_cpp',
> +                                  input : files('controls.cpp'),
> +                                  output : 'control_types.cpp',
> +                                  depend_files : gen_controls,
> +                                  command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> +
> +libcamera_sources += control_types_cpp
> +
>  libcamera_deps = [
>      cc.find_library('dl'),
>      libudev,
>

Hi Kieran,

On Mon, Jul 01, 2019 at 10:09:24AM +0100, Kieran Bingham wrote:
> On 01/07/2019 00:38, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Add a set of data types to support controls:
> > 
> > - ControlValue stores a control type and value in a generic way
> > - ControlId enumerates all the control identifies
> 
> s/identifies/identities/ ?
> 
> > - ControlIdentier declares the types of a control and map their names
> 
> s/map/maps/
> 
> > - ControlInfo stores runtime information for controls
> > - ControlList contains a set of control info and value pairs
> > 
> > The control definitions map is generated from the controls documentation
> > to ensure that the two will always be synchronised.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Some minor review comments sprinkled throughout, but otherwise:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> > Changes since v2:
> > 
> > - Squashed "Provide ControlValue class"
> > - Renamed Value to ControlValue
> > - Removed operator<<()
> 
> :-( Do you dislike this? or is this just to simplify things.
> 
> I had considered going round the rest of the codebase and adding
> operator<<() where we had toString() ... but I guess I won't do that if
> you're nacking it here.

I think it gets a bit overkill as we have toString(). If we decide to
add it, it should be a global decision as we don't define it anywhere.
That's why I've removed it from here, for now.

> > - Added control table generation
> > - Moved control definitions to control_definitions.h
> > - Renamed ControlTypes to controlsTypes and make it const
> 
> Presumably you mean controlTypes there (no extra s).

Yes, sorry.

> > - Moved the initial controls list to a separate patch
> > - Renamed control_definitions.h to control_ids.h and
> >   control_definitions.cpp to control_types.cpp to match the contained
> >   enum and variable name respectively
> > - Indexed ControlList by ControlInfo pointer instead of value
> > - Replaced ControlInfoHash with std::hash specialisation
> 
> But this affects the std namespace right? I chose to put a function
> object in specifically to keep libcamera code in the libcamera namespace.
> 
> <edit: Never mind:
> https://en.cppreference.com/w/cpp/language/extending_std states that
> specialisations of std::hash are allowed>

That's where I got the idea from :-) I also considered adding a template
specialisation that would cover all enums, but it may cause troubles
with some compiler versions, so I didn't go that route. Now that we only
index containers with ControlId and not with both ControlId and
ControlInfo I don't mind too much.

> > - Added automatic conversion between 32- and 64-bit integer values
> > 
> > The automatic conversion between integer types was prompted by an
> > assertion failure due to the use of getInt() on the min() and max()
> > value of an Integer control. The min and max ControlValue instances are
> > create as Integer64, due to the V4L2ControlInfo class returning the
> > range as int64_t. This may need to be reworked.
> >
> > ---
> >  Documentation/Doxyfile.in       |   3 +-
> >  include/libcamera/control_ids.h |  35 +++
> >  include/libcamera/controls.h    | 134 ++++++++++
> >  include/libcamera/meson.build   |   2 +
> >  src/libcamera/controls.cpp      | 428 ++++++++++++++++++++++++++++++++
> >  src/libcamera/gen-controls.awk  | 106 ++++++++
> >  src/libcamera/meson.build       |  11 +
> >  7 files changed, 718 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/control_ids.h
> >  create mode 100644 include/libcamera/controls.h
> >  create mode 100644 src/libcamera/controls.cpp
> >  create mode 100755 src/libcamera/gen-controls.awk
> > 
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index c58631200dd5..9ca32241b895 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -868,7 +868,8 @@ EXCLUDE_SYMBOLS        = libcamera::SignalBase \
> >                           libcamera::SlotArgs \
> >                           libcamera::SlotBase \
> >                           libcamera::SlotMember \
> > -                         libcamera::SlotStatic
> > +                         libcamera::SlotStatic \
> > +                         std::*
> >  
> >  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> >  # that contain example code fragments that are included (see the \include
> > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> > new file mode 100644
> > index 000000000000..d0e700da9844
> > --- /dev/null
> > +++ b/include/libcamera/control_ids.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * control_ids.h : Control ID list
> > + */
> > +
> > +#ifndef __LIBCAMERA_CONTROL_IDS_H__
> > +#define __LIBCAMERA_CONTROL_IDS_H__
> > +
> > +#include <functional>
> > +
> > +namespace libcamera {
> > +
> > +enum ControlId {
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +namespace std {
> > +
> > +template<>
> > +struct hash<libcamera::ControlId> {
> > +	using argument_type = libcamera::ControlId;
> > +	using result_type = std::size_t;
> > +
> > +	result_type operator()(const argument_type &key) const noexcept
> > +	{
> > +		return std::hash<std::underlying_type<argument_type>::type>()(key);
> > +	}
> > +};
> > +
> > +} /* namespace std */
> > +
> > +#endif // __LIBCAMERA_CONTROL_IDS_H__
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > new file mode 100644
> > index 000000000000..ad2d49d522c5
> > --- /dev/null
> > +++ b/include/libcamera/controls.h
> > @@ -0,0 +1,134 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.h - Control handling
> > + */
> > +
> > +#ifndef __LIBCAMERA_CONTROLS_H__
> > +#define __LIBCAMERA_CONTROLS_H__
> > +
> > +#include <stdint.h>
> > +#include <string>
> > +#include <unordered_map>
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +namespace libcamera {
> > +
> > +class Camera;
> > +
> > +enum ControlValueType {
> > +	ControlValueNone,
> > +	ControlValueBool,
> > +	ControlValueInteger,
> > +	ControlValueInteger64,
> > +};
> > +
> > +class ControlValue
> > +{
> > +public:
> > +	ControlValue();
> > +	ControlValue(bool value);
> > +	ControlValue(int value);
> > +	ControlValue(int64_t value);
> > +
> > +	ControlValueType type() const { return type_; };
> > +	bool isNone() const { return type_ == ControlValueNone; };
> > +
> > +	void set(bool value);
> > +	void set(int value);
> > +	void set(int64_t value);
> > +
> > +	bool getBool() const;
> > +	int getInt() const;
> > +	int64_t getInt64() const;
> > +
> > +	std::string toString() const;
> > +
> > +private:
> > +	ControlValueType type_;
> > +
> > +	union {
> > +		bool bool_;
> > +		int integer_;
> > +		int64_t integer64_;
> > +	};
> > +};
> > +
> > +struct ControlIdentifier {
> > +	ControlId id;
> > +	const char *name;
> > +	ControlValueType type;
> > +};
> > +
> > +class ControlInfo
> > +{
> > +public:
> > +	explicit ControlInfo(ControlId id, const ControlValue &min = 0,
> > +			     const ControlValue &max = 0);
> > +
> > +	ControlId id() const { return ident_->id; }
> > +	const char *name() const { return ident_->name; }
> > +	ControlValueType type() const { return ident_->type; }
> > +
> > +	const ControlValue &min() const { return min_; }
> > +	const ControlValue &max() const { return max_; }
> > +
> > +	std::string toString() const;
> > +
> > +private:
> > +	const struct ControlIdentifier *ident_;
> > +	ControlValue min_;
> > +	ControlValue max_;
> > +};
> > +
> > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);
> > +bool operator==(const ControlId &lhs, const ControlInfo &rhs);
> > +bool operator==(const ControlInfo &lhs, const ControlId &rhs);
> > +static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +
> > +class ControlList
> > +{
> > +private:
> > +	using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;
> > +
> > +public:
> > +	ControlList(Camera *camera);
> > +
> > +	using iterator = ControlListMap::iterator;
> > +	using const_iterator = ControlListMap::const_iterator;
> > +
> > +	iterator begin() { return controls_.begin(); }
> > +	iterator end() { return controls_.end(); }
> > +	const_iterator begin() const { return controls_.begin(); }
> > +	const_iterator end() const { return controls_.end(); }
> > +
> > +	bool contains(const ControlInfo *info) const { return controls_.count(info); };
> 
> I thought I had already updated this to use .find() ... perhaps it got
> lost in the merge/handover.
> 
> Though I see you've later implemented a contains(ControlId id) and left
> this as .count(). Was it intentional or just time constraints?
> 
> Because contains() has changed type this looks somewhat intentional ...
> 
> Also - extraneous semi-colon on the end:
>    s/; };/; }/

I agree that find() is better.

> > +	bool empty() const { return controls_.empty(); }
> > +	std::size_t size() const { return controls_.size(); }
> > +	void clear() { controls_.clear(); }
> > +
> > +	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
> > +
> > +	void update(const ControlList &list);
> > +
> > +private:
> > +	Camera *camera_;
> > +	ControlListMap controls_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CONTROLS_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 15484724df01..3067120a1598 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -2,6 +2,8 @@ libcamera_api = files([
> >      'buffer.h',
> >      'camera.h',
> >      'camera_manager.h',
> > +    'control_ids.h',
> > +    'controls.h',
> >      'event_dispatcher.h',
> >      'event_notifier.h',
> >      'geometry.h',
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > new file mode 100644
> > index 000000000000..22db2b93eff2
> > --- /dev/null
> > +++ b/src/libcamera/controls.cpp
> > @@ -0,0 +1,428 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.cpp - Control handling
> > + */
> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include <sstream>
> > +#include <string>
> > +
> > +#include "log.h"
> > +#include "utils.h"
> > +
> > +/**
> > + * \file controls.h
> > + * \brief Describes control framework and controls supported by a camera
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Controls)
> > +
> > +/**
> > + * \enum ControlValueType
> > + * Determines the type of value represented by a ControlValue
> > + * \var ControlValueNone
> > + * Identifies an unset control value
> > + * \var ControlValueBool
> > + * Identifies controls storing a boolean value
> > + * \var ControlValueInteger
> > + * Identifies controls storing an integer value
> > + * \var ControlValueInteger64
> > + * Identifies controls storing a 64-bit integer value
> > + */
> > +
> > +/**
> > + * \class ControlValue
> > + * \brief Abstract type for values in a control
> > + */
> > +
> > +/**
> > + * \brief Construct an empty ControlValue.
> > + */
> > +ControlValue::ControlValue()
> > +	: type_(ControlValueNone)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct a Boolean ControlValue
> > + * \param[in] value Boolean value to store
> > + */
> > +ControlValue::ControlValue(bool value)
> > +	: type_(ControlValueBool), bool_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct an integer ControlValue
> > + * \param[in] value Integer value to store
> > + */
> > +ControlValue::ControlValue(int value)
> > +	: type_(ControlValueInteger), integer_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct a 64 bit integer ControlValue
> > + * \param[in] value String representation to store
> > + */
> > +ControlValue::ControlValue(int64_t value)
> > +	: type_(ControlValueInteger64), integer64_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \fn ControlValue::type
> > + * \brief Return the type of value represented by the object
> > + */
> > +
> > +/**
> > + * \fn ControlValue::isNone
> > + * \brief Determine if the value is initialised
> > + * \return True if the value type is ControlValueNone, false otherwise
> > + */
> > +
> > +/**
> > + * \brief Set the value with a boolean
> > + * \param[in] value Boolean value to store
> > + */
> > +void ControlValue::set(bool value)
> > +{
> > +	type_ = ControlValueBool;
> > +	bool_ = value;
> > +}
> > +
> > +/**
> > + * \brief Set the value with an integer
> > + * \param[in] value Integer value to store
> > + */
> > +void ControlValue::set(int value)
> > +{
> > +	type_ = ControlValueInteger;
> > +	integer_ = value;
> > +}
> > +
> > +/**
> > + * \brief Set the value with a 64 bit integer
> > + * \param[in] value 64 bit integer value to store
> > + */
> > +void ControlValue::set(int64_t value)
> > +{
> > +	type_ = ControlValueInteger64;
> > +	integer64_ = value;
> > +}
> > +
> > +/**
> > + * \brief Get the boolean value.
> > + *
> > + * The value type must be Boolean.
> > + */
> > +bool ControlValue::getBool() const
> > +{
> > +	ASSERT(type_ == ControlValueBool);
> > +
> > +	return bool_;
> > +}
> > +
> > +/**
> > + * \brief Get the integer value.
> > + *
> > + * The value type must be Integer or Integer64
> > + */
> > +int ControlValue::getInt() const
> > +{
> > +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> > +
> > +	return integer_;
> > +}
> > +
> > +/**
> > + * \brief Get the 64 bit integer value.
> > + *
> > + * The value type must be Integer or Integer64
> > + */
> > +int64_t ControlValue::getInt64() const
> > +{
> > +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> > +
> > +	return integer64_;
> > +}
> > +
> > +/**
> > + * \brief Assemble and return a string describing the value
> > + * \return A string describing the ControlValue
> > + */
> > +std::string ControlValue::toString() const
> > +{
> > +	switch (type_) {
> > +	case ControlValueNone:
> > +		return "<None>";
> > +	case ControlValueBool:
> > +		return bool_ ? "True" : "False";
> > +	case ControlValueInteger:
> > +		return std::to_string(integer_);
> > +	case ControlValueInteger64:
> > +		return std::to_string(integer64_);
> > +	}
> > +
> > +	return "<ValueType Error>";
> > +}
> > +
> > +/**
> > + * \enum ControlId
> > + * Control Identifiers
> > + */
> > +
> > +/**
> > + * \struct ControlIdentifier
> > + * \brief Describes a ControlId with control specific constant meta-data.
> > + *
> > + * Defines a Control with a unique ID, a name, and a type.
> > + * This structure is used as static part of the autogenerated control
> 
> s/autogenerated/auto-generated/
> 
> > + * definitions, which are generated from the ControlId documentation.
> > + *
> > + * \var ControlIdentifier::id
> > + * The unique ID for a control
> > + * \var ControlIdentifier::name
> > + * The string representation of the control
> > + * \var ControlIdentifier::type
> > + * The ValueType required to represent the control value
> > + */
> > +
> > +/*
> > + * The controlTypes are automatically generated to produce a control_types.cpp
> > + * output. This file is not for public use, and so no suitable header exists
> > + * for this sole usage of the controlTypes reference. As such the extern is
> > + * only defined here for use during the ControlInfo constructor and should not
> > + * be referenced directly elsewhere.
> > + */
> > +extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> > +
> > +/**
> > + * \class ControlInfo
> > + * \brief Describe the information and capabilities of a Control
> > + *
> > + * The ControlInfo represents control specific meta-data which is constant on a
> > + * per camera basis. ControlInfo classes are constructed by pipeline handlers
> > + * to expose the controls they support and the metadata needed to utilise those
> > + * controls.
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> > + */
> > +ControlInfo::ControlInfo(ControlId id, const ControlValue &min,
> > +			 const ControlValue &max)
> > +	: min_(min), max_(max)
> > +{
> > +	auto iter = controlTypes.find(id);
> > +	if (iter == controlTypes.end()) {
> > +		LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> > +		return;
> > +	}
> > +
> > +	ident_ = &iter->second;
> > +}
> > +
> > +/**
> > + * \fn ControlInfo::id()
> > + * \brief Retrieve the ID of the control information descriptor
> > + * \return the ControlId
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::name()
> > + * \brief Retrieve the string name of the control information descriptor
> > + * \return A string name for the Control
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::type()
> > + * \brief Retrieve the ValueType of the control information descriptor
> > + * \return The control type
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::min()
> > + * \brief Reports the minimum value of the control
> > + * \return a COntrolValue with the minimum setting for the control
> 
> s/COntrolValue/ControlValue/
> 
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::max()
> > + * \brief Reports the maximum value of the control
> > + * \return a ControlValue with the maximum setting for the control
> > + */
> > +
> > +/**
> > + * \brief Provide a string representation of the ControlInfo
> > + */
> > +std::string ControlInfo::toString() const
> > +{
> > +	std::stringstream ss;
> > +
> > +	ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]";
> > +
> > +	return ss.str();
> > +}
> > +
> > +/**
> > + * \brief Compare control information for equality
> > + * \param[in] lhs Left-hand side control information
> > + * \param[in] rhs Right-hand side control information
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> 
> information is uncountable:
> 
> "Control information is compared based on the ID only, ..."
> 
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)
> > +{
> > +	return lhs.id() == rhs.id();
> > +}
> > +
> > +/**
> > + * \brief Compare control ID and information for equality
> > + * \param[in] lhs Left-hand side control identifier
> > + * \param[in] rhs Right-hand side control information
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> 
> Same as above.
> 
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlId &lhs, const ControlInfo &rhs)
> > +{
> > +	return lhs == rhs.id();
> > +}
> > +
> > +/**
> > + * \brief Compare control information and ID for equality
> > + * \param[in] lhs Left-hand side control information
> > + * \param[in] rhs Right-hand side control identifier
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> 
> Same as above.
> 
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlInfo &lhs, const ControlId &rhs)
> > +{
> > +	return lhs.id() == rhs;
> > +}
> > +
> > +/**
> > + * \class ControlList
> > + * \brief Associates a list of ControlIds with their values for a Camera.
> > + *
> > + * A ControlList specifies a map of ControlIds and Values and associated
> > + * validation against the ControlInfo for the related Camera device.
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlList with a reference to the Camera it applies on
> > + */
> > +ControlList::ControlList(Camera *camera)
> > +	: camera_(camera)
> > +{
> > +}
> > +
> > +/**
> > + * \typedef ControlList::iterator
> > + * \brief Iterator for the controls contained within the list.
> > + */
> > +
> > +/**
> > + * \typedef ControlList::const_iterator
> > + * \brief Const iterator for the controls contained within the list.
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::begin()
> > + * \brief Retrieve an iterator to the first Control in the list
> > + * \return An iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::end()
> > + * \brief Retrieve an iterator to the next element after the last controls in
> > + * the instance.
> > + * \return An iterator to the element following the last control in the instance
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::begin() const
> > + * \brief Retrieve a const_iterator to the first Control in the list
> > + * \return A const_iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::end() const
> > + * \brief Retrieve a constant iterator pointing to an empty element after the
> > + * \return A const iterator to the element following the last control in the
> > + * instance
> > + */
> > +
> > +/**
> > + * \fn ControlList::contains(const ControlInfo *info) const
> > + * \brief Check if the ist contains a control with the specified \a info
> > + * \param[in] info The control info
> > + * \return True if the list contains a matching control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::empty()
> > + * \brief Identify if the list is empty
> > + * \return True if the list does not contain any control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::size()
> > + * \brief Retrieve the number of controls in the list
> > + * \return The number of Control entries stored in the list
> > + */
> > +
> > +/**
> > + * \fn ControlList::clear()
> > + * \brief Removes all controls from the list
> > + */
> > +
> > +/**
> > + * \fn ControlList::operator[](const ControlInfo *info)
> > + * \brief Access or insert the control specified by \a info
> > + * \param[in] info The control info
> > + *
> > + * This method returns a reference to the control identified by \a info,
> > + * inserting it in the list if the info is not already present.
> > + *
> > + * \return A reference to the value of the control identified by \a info
> > + */
> > +
> > +/**
> > + * \brief Update all Control values with the value from the given \a list
> > + * \param list The list of controls to update or append to this list
> > + *
> > + * Update all controls in the ControlList, by the values given by \a list
> > + * If the list already contains a control of this ID then it will be overwritten
> > + */
> > +void ControlList::update(const ControlList &list)
> > +{
> > +	if (list.camera_ != camera_) {
> > +		LOG(Controls, Error)
> > +			<< "ControlLists can not be translated between cameras";
> > +		return;
> > +	}
> > +
> > +	for (auto it : list) {
> > +		const ControlInfo *info = it.first;
> > +		const ControlValue &value = it.second;
> > +
> > +		controls_[info] = value;
> > +	}
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> > new file mode 100755
> > index 000000000000..a91529b575db
> > --- /dev/null
> > +++ b/src/libcamera/gen-controls.awk
> > @@ -0,0 +1,106 @@
> > +#!/usr/bin/awk -f
> > +
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +
> > +# Controls are documented using Doxygen in the main control.cpp source.
> 
> s/control.cpp/controls.cpp/
> 
> > +#
> > +# Generate control tables directly from the documentation, creating enumerations
> > +# to support the IDs and static type information regarding each control.
> > +
> > +BEGIN {
> > +	id=0
> > +	input=ARGV[1]
> > +	mode=ARGV[2]
> > +	output=ARGV[3]
> > +}
> > +
> > +# Detect Doxygen style comment blocks and ignore other lines
> > +/^\/\*\*$/ { in_doxygen=1; first_line=1; next }
> > +// { if (!in_doxygen) next }
> > +
> > +# Entry point for the Control Documentation
> > +/ * \\enum ControlId$/ { in_controls=1; first_line=0; next }
> > +// { if (!in_controls) next }
> > +
> > +# Extract control information
> > +/ \* \\var/ { names[++id]=$3; first_line=0; next }
> > +/ \* ControlType:/ { types[id] = $3 }
> > +
> > +# End of comment blocks
> > +/^ \*\// { in_doxygen=0 }
> > +
> > +# Identify the end of controls
> > +/^ \* \\/ { if (first_line) exit }> +// { first_line=0 }
> 
> This first_line=0 appears to be redundant.
> 
> 'first_line' doesn't seem like a great variable name choice either, but
> if we may end up rewriting this at some point - I don't think we need to
> dwell on it while it's functional.

I don't like it much, so feel free to rewrite it :-) I had to modify the
awk script as it was picking up an unrelated enum.

> > +
> > +################################
> > +# Support output file generation
> > +
> > +function basename(file) {
> > +	sub(".*/", "", file)
> > +	return file
> > +}
> > +
> > +function Header(file, description) {
> > +	print "/* SPDX-License-Identifier: LGPL-2.1-or-later */" > file
> > +	print "/*" > file
> > +	print " * Copyright (C) 2019, Google Inc." > file
> > +	print " *" > file
> > +	print " * " basename(file) " - " description > file
> > +	print " *" > file
> > +	print " * This file is autogenerated. Do not edit." > file
> 
> I would say s/autogenerated/auto-generated/ but I think perhaps
> autogenerated might be fairly commonly used.

https://en.wiktionary.org/wiki/autogenerated
https://en.wiktionary.org/wiki/auto-generated

> > +	print " */" > file
> > +	print "" > file
> > +}
> > +
> > +function EnterNameSpace(file) {
> > +	print "namespace libcamera {" > file
> > +	print "" > file
> > +}
> > +
> > +function ExitNameSpace(file) {
> > +	print "" > file
> > +	print "} /* namespace libcamera */" > file
> > +}
> > +
> > +function GenerateHeader(file) {
> > +	Header(file, "Control ID list")
> > +
> > +	print "#ifndef __LIBCAMERA_CONTROL_IDS_H__" > file
> > +	print "#define __LIBCAMERA_CONTROL_IDS_H__" > file
> > +	print "" > file
> > +
> > +	EnterNameSpace(file)
> > +	print "enum ControlId {" > file
> > +	for (i=1; i <= id; ++i) {
> > +		printf "\t%s,\n", names[i] > file
> > +	}
> > +	print "};" > file
> > +	ExitNameSpace(file)
> > +
> > +	print "" > file
> > +	print "#endif // __LIBCAMERA_CONTROL_IDS_H__" > file
> > +}
> > +
> > +function GenerateTable(file) {
> > +	Header(file, "Control types")
> > +	print "#include <libcamera/controls.h>" > file
> > +	print "" > file
> > +
> > +	EnterNameSpace(file)
> > +
> > +	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> > +	print "controlTypes {" > file
> > +	for (i=1; i <= id; ++i) {
> > +		printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> > +	}
> > +	print "};" > file
> > +	ExitNameSpace(file)
> > +}
> > +
> > +END {
> > +	if (mode == "--header")
> > +		GenerateHeader(output)
> > +	else
> > +		GenerateTable(output)
> > +}
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 985aa7e8ab0e..b1ee92735e41 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -3,6 +3,7 @@ libcamera_sources = files([
> >      'camera.cpp',
> >      'camera_manager.cpp',
> >      'camera_sensor.cpp',
> > +    'controls.cpp',
> >      'device_enumerator.cpp',
> >      'device_enumerator_sysfs.cpp',
> >      'event_dispatcher.cpp',
> > @@ -66,6 +67,16 @@ if libudev.found()
> >      ])
> >  endif
> >  
> > +gen_controls = files('gen-controls.awk')
> > +
> > +control_types_cpp = custom_target('control_types_cpp',
> > +                                  input : files('controls.cpp'),
> > +                                  output : 'control_types.cpp',
> > +                                  depend_files : gen_controls,
> > +                                  command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> > +
> > +libcamera_sources += control_types_cpp
> > +
> >  libcamera_deps = [
> >      cc.find_library('dl'),
> >      libudev,

On Mon, Jul 01, 2019 at 02:38:07AM +0300, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Add a set of data types to support controls:
>
> - ControlValue stores a control type and value in a generic way
> - ControlId enumerates all the control identifies

identifiers

> - ControlIdentier declares the types of a control and map their names

ControlIdentifier

> - ControlInfo stores runtime information for controls
> - ControlList contains a set of control info and value pairs
>
> The control definitions map is generated from the controls documentation
> to ensure that the two will always be synchronised.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v2:
>
> - Squashed "Provide ControlValue class"
> - Renamed Value to ControlValue
> - Removed operator<<()
> - Added control table generation
> - Moved control definitions to control_definitions.h
> - Renamed ControlTypes to controlsTypes and make it const
> - Moved the initial controls list to a separate patch
> - Renamed control_definitions.h to control_ids.h and
>   control_definitions.cpp to control_types.cpp to match the contained
>   enum and variable name respectively
> - Indexed ControlList by ControlInfo pointer instead of value
> - Replaced ControlInfoHash with std::hash specialisation
> - Added automatic conversion between 32- and 64-bit integer values
>
> The automatic conversion between integer types was prompted by an
> assertion failure due to the use of getInt() on the min() and max()
> value of an Integer control. The min and max ControlValue instances are
> create as Integer64, due to the V4L2ControlInfo class returning the
> range as int64_t. This may need to be reworked.
> ---
>  Documentation/Doxyfile.in       |   3 +-
>  include/libcamera/control_ids.h |  35 +++
>  include/libcamera/controls.h    | 134 ++++++++++
>  include/libcamera/meson.build   |   2 +
>  src/libcamera/controls.cpp      | 428 ++++++++++++++++++++++++++++++++
>  src/libcamera/gen-controls.awk  | 106 ++++++++
>  src/libcamera/meson.build       |  11 +
>  7 files changed, 718 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/control_ids.h
>  create mode 100644 include/libcamera/controls.h
>  create mode 100644 src/libcamera/controls.cpp
>  create mode 100755 src/libcamera/gen-controls.awk
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index c58631200dd5..9ca32241b895 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -868,7 +868,8 @@ EXCLUDE_SYMBOLS        = libcamera::SignalBase \
>                           libcamera::SlotArgs \
>                           libcamera::SlotBase \
>                           libcamera::SlotMember \
> -                         libcamera::SlotStatic
> +                         libcamera::SlotStatic \
> +                         std::*
>
>  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
>  # that contain example code fragments that are included (see the \include
> diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> new file mode 100644
> index 000000000000..d0e700da9844
> --- /dev/null
> +++ b/include/libcamera/control_ids.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * control_ids.h : Control ID list
> + */
> +
> +#ifndef __LIBCAMERA_CONTROL_IDS_H__
> +#define __LIBCAMERA_CONTROL_IDS_H__
> +
> +#include <functional>
> +
> +namespace libcamera {
> +
> +enum ControlId {
> +};
> +
> +} /* namespace libcamera */
> +
> +namespace std {
> +
> +template<>
> +struct hash<libcamera::ControlId> {
> +	using argument_type = libcamera::ControlId;
> +	using result_type = std::size_t;
> +
> +	result_type operator()(const argument_type &key) const noexcept
> +	{
> +		return std::hash<std::underlying_type<argument_type>::type>()(key);
> +	}
> +};
> +
> +} /* namespace std */
> +
> +#endif // __LIBCAMERA_CONTROL_IDS_H__
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> new file mode 100644
> index 000000000000..ad2d49d522c5
> --- /dev/null
> +++ b/include/libcamera/controls.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * controls.h - Control handling
> + */
> +
> +#ifndef __LIBCAMERA_CONTROLS_H__
> +#define __LIBCAMERA_CONTROLS_H__
> +
> +#include <stdint.h>
> +#include <string>
> +#include <unordered_map>
> +
> +#include <libcamera/control_ids.h>
> +
> +namespace libcamera {
> +
> +class Camera;
> +
> +enum ControlValueType {
> +	ControlValueNone,
> +	ControlValueBool,
> +	ControlValueInteger,
> +	ControlValueInteger64,
> +};
> +
> +class ControlValue
> +{
> +public:
> +	ControlValue();
> +	ControlValue(bool value);
> +	ControlValue(int value);
> +	ControlValue(int64_t value);
> +
> +	ControlValueType type() const { return type_; };
> +	bool isNone() const { return type_ == ControlValueNone; };
> +
> +	void set(bool value);
> +	void set(int value);
> +	void set(int64_t value);
> +
> +	bool getBool() const;
> +	int getInt() const;
> +	int64_t getInt64() const;
> +
> +	std::string toString() const;
> +
> +private:
> +	ControlValueType type_;
> +
> +	union {
> +		bool bool_;
> +		int integer_;
> +		int64_t integer64_;
> +	};
> +};
> +
> +struct ControlIdentifier {
> +	ControlId id;
> +	const char *name;
> +	ControlValueType type;
> +};
> +
> +class ControlInfo
> +{
> +public:
> +	explicit ControlInfo(ControlId id, const ControlValue &min = 0,
> +			     const ControlValue &max = 0);
> +
> +	ControlId id() const { return ident_->id; }
> +	const char *name() const { return ident_->name; }
> +	ControlValueType type() const { return ident_->type; }
> +
> +	const ControlValue &min() const { return min_; }
> +	const ControlValue &max() const { return max_; }
> +
> +	std::string toString() const;
> +
> +private:
> +	const struct ControlIdentifier *ident_;
> +	ControlValue min_;
> +	ControlValue max_;

Do we plan to have a step too?

> +};
> +
> +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);
> +bool operator==(const ControlId &lhs, const ControlInfo &rhs);
> +bool operator==(const ControlInfo &lhs, const ControlId &rhs);
> +static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)
> +{
> +	return !(lhs == rhs);
> +}
> +
> +class ControlList
> +{
> +private:
> +	using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;

Does it make sense to make this private if it is then used in the
public API ?

> +
> +public:
> +	ControlList(Camera *camera);
> +
> +	using iterator = ControlListMap::iterator;
> +	using const_iterator = ControlListMap::const_iterator;
> +
> +	iterator begin() { return controls_.begin(); }
> +	iterator end() { return controls_.end(); }
> +	const_iterator begin() const { return controls_.begin(); }
> +	const_iterator end() const { return controls_.end(); }
> +
> +	bool contains(const ControlInfo *info) const { return controls_.count(info); };
> +	bool empty() const { return controls_.empty(); }
> +	std::size_t size() const { return controls_.size(); }
> +	void clear() { controls_.clear(); }
> +
> +	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
> +
> +	void update(const ControlList &list);
> +
> +private:
> +	Camera *camera_;
> +	ControlListMap controls_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CONTROLS_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 15484724df01..3067120a1598 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -2,6 +2,8 @@ libcamera_api = files([
>      'buffer.h',
>      'camera.h',
>      'camera_manager.h',
> +    'control_ids.h',
> +    'controls.h',
>      'event_dispatcher.h',
>      'event_notifier.h',
>      'geometry.h',
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> new file mode 100644
> index 000000000000..22db2b93eff2
> --- /dev/null
> +++ b/src/libcamera/controls.cpp
> @@ -0,0 +1,428 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * controls.cpp - Control handling
> + */
> +
> +#include <libcamera/controls.h>
> +
> +#include <sstream>
> +#include <string>
> +
> +#include "log.h"
> +#include "utils.h"
> +
> +/**
> + * \file controls.h
> + * \brief Describes control framework and controls supported by a camera
> + */
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(Controls)
> +
> +/**
> + * \enum ControlValueType
> + * Determines the type of value represented by a ControlValue

a/Determines/Defines ?
"type of a value" ?

> + * \var ControlValueNone
> + * Identifies an unset control value
> + * \var ControlValueBool
> + * Identifies controls storing a boolean value
> + * \var ControlValueInteger
> + * Identifies controls storing an integer value
> + * \var ControlValueInteger64
> + * Identifies controls storing a 64-bit integer value
> + */
> +
> +/**
> + * \class ControlValue
> + * \brief Abstract type for values in a control

Does the control 'contains' a value, or is the value part of the
control itself? I would say "Abstract type representing the value of a
control"

> + */
> +
> +/**
> + * \brief Construct an empty ControlValue.
> + */
> +ControlValue::ControlValue()
> +	: type_(ControlValueNone)
> +{
> +}
> +

Does this have any use?

> +/**
> + * \brief Construct a Boolean ControlValue
> + * \param[in] value Boolean value to store
> + */
> +ControlValue::ControlValue(bool value)
> +	: type_(ControlValueBool), bool_(value)
> +{
> +}
> +
> +/**
> + * \brief Construct an integer ControlValue
> + * \param[in] value Integer value to store
> + */
> +ControlValue::ControlValue(int value)
> +	: type_(ControlValueInteger), integer_(value)
> +{
> +}
> +
> +/**
> + * \brief Construct a 64 bit integer ControlValue
> + * \param[in] value String representation to store

Not a string but a 64-bit integer

> + */
> +ControlValue::ControlValue(int64_t value)
> +	: type_(ControlValueInteger64), integer64_(value)
> +{
> +}
> +
> +/**
> + * \fn ControlValue::type
> + * \brief Return the type of value represented by the object

s/represented by the object// ?

> + */
> +
> +/**
> + * \fn ControlValue::isNone
> + * \brief Determine if the value is initialised

As we check for isNone, the operation determines if the value is NOT
initialized :)

> + * \return True if the value type is ControlValueNone, false otherwise

Just "True if the value is not initialized... "

> + */
> +
> +/**
> + * \brief Set the value with a boolean
> + * \param[in] value Boolean value to store
> + */
> +void ControlValue::set(bool value)
> +{
> +	type_ = ControlValueBool;
> +	bool_ = value;
> +}

Ah yes, you can create None values then update. Is this a good idea?
Won't control always have a single type associated? Or we want to be
be able to re-use the same ControlValue with many controls ?

> +
> +/**
> + * \brief Set the value with an integer
> + * \param[in] value Integer value to store
> + */
> +void ControlValue::set(int value)
> +{
> +	type_ = ControlValueInteger;
> +	integer_ = value;
> +}
> +
> +/**
> + * \brief Set the value with a 64 bit integer
> + * \param[in] value 64 bit integer value to store
> + */
> +void ControlValue::set(int64_t value)
> +{
> +	type_ = ControlValueInteger64;
> +	integer64_ = value;
> +}
> +
> +/**
> + * \brief Get the boolean value.

We usually don't end briefs with full stops, here and below.

> + *
> + * The value type must be Boolean.

missing \return here and below.

> + */
> +bool ControlValue::getBool() const
> +{
> +	ASSERT(type_ == ControlValueBool);

I wonder how easy would that be to create a control value, set it to a
type, then to another, then read the original type and then crash the
library here.

> +
> +	return bool_;
> +}
> +
> +/**
> + * \brief Get the integer value.
> + *
> + * The value type must be Integer or Integer64
> + */
> +int ControlValue::getInt() const
> +{
> +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);

is this a good idea to access 64 bit integers as integers?
> +
> +	return integer_;
> +}
> +
> +/**
> + * \brief Get the 64 bit integer value.
> + *
> + * The value type must be Integer or Integer64
> + */
> +int64_t ControlValue::getInt64() const
> +{
> +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);

Accessing an integer as a 64 bit one is safer instead.

> +
> +	return integer64_;
> +}
> +
> +/**
> + * \brief Assemble and return a string describing the value
> + * \return A string describing the ControlValue
> + */
> +std::string ControlValue::toString() const
> +{
> +	switch (type_) {
> +	case ControlValueNone:
> +		return "<None>";
> +	case ControlValueBool:
> +		return bool_ ? "True" : "False";
> +	case ControlValueInteger:
> +		return std::to_string(integer_);
> +	case ControlValueInteger64:
> +		return std::to_string(integer64_);
> +	}
> +
> +	return "<ValueType Error>";
> +}
> +
> +/**
> + * \enum ControlId
> + * Control Identifiers

numerical identifiers? There are many "info" "id" etc etc associated
with controls here. Make it clear this is the numerical id with a bit
more of description?

> + */
> +
> +/**
> + * \struct ControlIdentifier

It's easy to confuse this Identifier with ControlId if you describe
control id as the control identifier.

> + * \brief Describes a ControlId with control specific constant meta-data.

full stop in brief, and we usually don't use the third person afaict

> + *
> + * Defines a Control with a unique ID, a name, and a type.
> + * This structure is used as static part of the autogenerated control
> + * definitions, which are generated from the ControlId documentation.
> + *
> + * \var ControlIdentifier::id
> + * The unique ID for a control
> + * \var ControlIdentifier::name
> + * The string representation of the control
> + * \var ControlIdentifier::type
> + * The ValueType required to represent the control value
> + */
> +
> +/*
> + * The controlTypes are automatically generated to produce a control_types.cpp
> + * output. This file is not for public use, and so no suitable header exists
> + * for this sole usage of the controlTypes reference. As such the extern is
> + * only defined here for use during the ControlInfo constructor and should not
> + * be referenced directly elsewhere.
> + */
> +extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> +
> +/**
> + * \class ControlInfo
> + * \brief Describe the information and capabilities of a Control
> + *
> + * The ControlInfo represents control specific meta-data which is constant on a
> + * per camera basis. ControlInfo classes are constructed by pipeline handlers
> + * to expose the controls they support and the metadata needed to utilise those
> + * controls.
> + */
> +
> +/**
> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> + */
> +ControlInfo::ControlInfo(ControlId id, const ControlValue &min,
> +			 const ControlValue &max)
> +	: min_(min), max_(max)
> +{
> +	auto iter = controlTypes.find(id);
> +	if (iter == controlTypes.end()) {
> +		LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> +		return;
> +	}
> +
> +	ident_ = &iter->second;
> +}
> +
> +/**
> + * \fn ControlInfo::id()
> + * \brief Retrieve the ID of the control information descriptor
> + * \return the ControlId
> + */
> +
> +/**
> + * \fn ControlInfo::name()
> + * \brief Retrieve the string name of the control information descriptor
> + * \return A string name for the Control
> + */
> +
> +/**
> + * \fn ControlInfo::type()
> + * \brief Retrieve the ValueType of the control information descriptor
> + * \return The control type
> + */

Isn't the 'information' in 'control information $SOMETHING' redundant?

> +
> +/**
> + * \fn ControlInfo::min()
> + * \brief Reports the minimum value of the control
> + * \return a COntrolValue with the minimum setting for the control

s/COntrol/control

and also "minimum setting" would not be better described as "minium
valid value" or similar ?

> + */
> +
> +/**
> + * \fn ControlInfo::max()
> + * \brief Reports the maximum value of the control

I know these are not purely getters, but we usually use "Retrieve"

> + * \return a ControlValue with the maximum setting for the control
> + */
> +
> +/**
> + * \brief Provide a string representation of the ControlInfo
> + */
> +std::string ControlInfo::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]";
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \brief Compare control information for equality

I would simply "Verify it two control information are equal" or
similar

> + * \param[in] lhs Left-hand side control information
> + * \param[in] rhs Right-hand side control information
> + *
> + * Control information are compared based on their ID only, as a camera may not
> + * have two separate controls with the same ID.
> + *
> + * \return True if \a lhs and \a rhs are equal, false otherwise
> + */
> +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)
> +{
> +	return lhs.id() == rhs.id();
> +}
> +
> +/**
> + * \brief Compare control ID and information for equality
> + * \param[in] lhs Left-hand side control identifier
> + * \param[in] rhs Right-hand side control information
> + *
> + * Control information are compared based on their ID only, as a camera may not
> + * have two separate controls with the same ID.
> + *
> + * \return True if \a lhs and \a rhs are equal, false otherwise
> + */
> +bool operator==(const ControlId &lhs, const ControlInfo &rhs)
> +{
> +	return lhs == rhs.id();
> +}
> +
> +/**
> + * \brief Compare control information and ID for equality
> + * \param[in] lhs Left-hand side control information
> + * \param[in] rhs Right-hand side control identifier
> + *
> + * Control information are compared based on their ID only, as a camera may not
> + * have two separate controls with the same ID.
> + *
> + * \return True if \a lhs and \a rhs are equal, false otherwise
> + */
> +bool operator==(const ControlInfo &lhs, const ControlId &rhs)
> +{
> +	return lhs.id() == rhs;
> +}
> +
> +/**
> + * \class ControlList
> + * \brief Associates a list of ControlIds with their values for a Camera.

Full stop and third person in \brief

> + *
> + * A ControlList specifies a map of ControlIds and Values and associated
> + * validation against the ControlInfo for the related Camera device.

Maybe I'm tired, but this is very not clear to me. Particularly the
fact that a map associates ControlIds and value and associated
validation.

Now that I wrote it I get what the meaning was, but I would re-phrase
this as

"A ControlList wraps (or other verbs) a map of ControlId to
ControlValue and provide validation operations against the controlInfo
provided (or other verbs) by the Camera device that created the
control"

> + */
> +
> +/**
> + * \brief Construct a ControlList with a reference to the Camera it applies on
> + */
> +ControlList::ControlList(Camera *camera)

At this point it is not clear to me what the lifetime of a ControlList
is, but in most part of our code (ok, not most, many) the camera is
passed around as a shared pointer. Is this needed here?

> +	: camera_(camera)
> +{
> +}
> +
> +/**
> + * \typedef ControlList::iterator
> + * \brief Iterator for the controls contained within the list.

full stop

> + */
> +
> +/**
> + * \typedef ControlList::const_iterator
> + * \brief Const iterator for the controls contained within the list.
> + */
> +
> +/**
> + * \fn iterator ControlList::begin()
> + * \brief Retrieve an iterator to the first Control in the list
> + * \return An iterator to the first Control in the list
> + */
> +
> +/**
> + * \fn iterator ControlList::end()
> + * \brief Retrieve an iterator to the next element after the last controls in
> + * the instance.
> + * \return An iterator to the element following the last control in the instance

You are using "Control in the list" for being and "control in the
instance" for end. Care to align them?

> + */
> +
> +/**
> + * \fn const_iterator ControlList::begin() const
> + * \brief Retrieve a const_iterator to the first Control in the list
> + * \return A const_iterator to the first Control in the list
> + */
> +
> +/**
> + * \fn const_iterator ControlList::end() const
> + * \brief Retrieve a constant iterator pointing to an empty element after the
> + * \return A const iterator to the element following the last control in the
> + * instance
> + */
> +
> +/**
> + * \fn ControlList::contains(const ControlInfo *info) const
> + * \brief Check if the ist contains a control with the specified \a info

a/ist/list
I would have used instance, but it's fine.

> + * \param[in] info The control info
> + * \return True if the list contains a matching control, false otherwise
> + */
> +
> +/**
> + * \fn ControlList::empty()
> + * \brief Identify if the list is empty
> + * \return True if the list does not contain any control, false otherwise
> + */
> +
> +/**
> + * \fn ControlList::size()
> + * \brief Retrieve the number of controls in the list
> + * \return The number of Control entries stored in the list
> + */
> +
> +/**
> + * \fn ControlList::clear()
> + * \brief Removes all controls from the list
> + */

Same comment for the use of list vs instance here. I think it's mostly
a matter of tastes.

> +
> +/**
> + * \fn ControlList::operator[](const ControlInfo *info)
> + * \brief Access or insert the control specified by \a info
> + * \param[in] info The control info
> + *
> + * This method returns a reference to the control identified by \a info,
> + * inserting it in the list if the info is not already present.
> + *
> + * \return A reference to the value of the control identified by \a info
> + */
> +
> +/**
> + * \brief Update all Control values with the value from the given \a list
> + * \param list The list of controls to update or append to this list
> + *
> + * Update all controls in the ControlList, by the values given by \a list

s/by the value/with the value/ ?

> + * If the list already contains a control of this ID then it will be overwritten

s/of this ID/with this ID/ ?
it will be overwritten or "its value will be updated" ?

> + */
> +void ControlList::update(const ControlList &list)
> +{
> +	if (list.camera_ != camera_) {
> +		LOG(Controls, Error)
> +			<< "ControlLists can not be translated between cameras";
> +		return;
> +	}
> +
> +	for (auto it : list) {

I'm not sure if "auto &it" makes any difference or not...

> +		const ControlInfo *info = it.first;
> +		const ControlValue &value = it.second;
> +
> +		controls_[info] = value;
> +	}
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> new file mode 100755
> index 000000000000..a91529b575db
> --- /dev/null
> +++ b/src/libcamera/gen-controls.awk

I trust your awk skills and assume everything is perfect here :)

> @@ -0,0 +1,106 @@
> +#!/usr/bin/awk -f
> +
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +
> +# Controls are documented using Doxygen in the main control.cpp source.
> +#
> +# Generate control tables directly from the documentation, creating enumerations
> +# to support the IDs and static type information regarding each control.
> +
> +BEGIN {
> +	id=0
> +	input=ARGV[1]
> +	mode=ARGV[2]
> +	output=ARGV[3]
> +}
> +
> +# Detect Doxygen style comment blocks and ignore other lines
> +/^\/\*\*$/ { in_doxygen=1; first_line=1; next }
> +// { if (!in_doxygen) next }
> +
> +# Entry point for the Control Documentation
> +/ * \\enum ControlId$/ { in_controls=1; first_line=0; next }
> +// { if (!in_controls) next }
> +
> +# Extract control information
> +/ \* \\var/ { names[++id]=$3; first_line=0; next }
> +/ \* ControlType:/ { types[id] = $3 }
> +
> +# End of comment blocks
> +/^ \*\// { in_doxygen=0 }
> +
> +# Identify the end of controls
> +/^ \* \\/ { if (first_line) exit }
> +// { first_line=0 }
> +
> +################################
> +# Support output file generation
> +
> +function basename(file) {
> +	sub(".*/", "", file)
> +	return file
> +}
> +
> +function Header(file, description) {
> +	print "/* SPDX-License-Identifier: LGPL-2.1-or-later */" > file
> +	print "/*" > file
> +	print " * Copyright (C) 2019, Google Inc." > file
> +	print " *" > file
> +	print " * " basename(file) " - " description > file
> +	print " *" > file
> +	print " * This file is autogenerated. Do not edit." > file
> +	print " */" > file
> +	print "" > file
> +}
> +
> +function EnterNameSpace(file) {
> +	print "namespace libcamera {" > file
> +	print "" > file
> +}
> +
> +function ExitNameSpace(file) {
> +	print "" > file
> +	print "} /* namespace libcamera */" > file
> +}
> +
> +function GenerateHeader(file) {
> +	Header(file, "Control ID list")
> +
> +	print "#ifndef __LIBCAMERA_CONTROL_IDS_H__" > file
> +	print "#define __LIBCAMERA_CONTROL_IDS_H__" > file
> +	print "" > file
> +
> +	EnterNameSpace(file)
> +	print "enum ControlId {" > file
> +	for (i=1; i <= id; ++i) {
> +		printf "\t%s,\n", names[i] > file
> +	}
> +	print "};" > file
> +	ExitNameSpace(file)
> +
> +	print "" > file
> +	print "#endif // __LIBCAMERA_CONTROL_IDS_H__" > file
> +}
> +
> +function GenerateTable(file) {
> +	Header(file, "Control types")
> +	print "#include <libcamera/controls.h>" > file
> +	print "" > file
> +
> +	EnterNameSpace(file)
> +
> +	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> +	print "controlTypes {" > file
> +	for (i=1; i <= id; ++i) {
> +		printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> +	}
> +	print "};" > file
> +	ExitNameSpace(file)
> +}
> +
> +END {
> +	if (mode == "--header")
> +		GenerateHeader(output)
> +	else
> +		GenerateTable(output)
> +}
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 985aa7e8ab0e..b1ee92735e41 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -3,6 +3,7 @@ libcamera_sources = files([
>      'camera.cpp',
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
> +    'controls.cpp',
>      'device_enumerator.cpp',
>      'device_enumerator_sysfs.cpp',
>      'event_dispatcher.cpp',
> @@ -66,6 +67,16 @@ if libudev.found()
>      ])
>  endif
>
> +gen_controls = files('gen-controls.awk')
> +
> +control_types_cpp = custom_target('control_types_cpp',
> +                                  input : files('controls.cpp'),
> +                                  output : 'control_types.cpp',
> +                                  depend_files : gen_controls,
> +                                  command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> +
> +libcamera_sources += control_types_cpp
> +

If both you and Kieran had a look here, I assume this is great

Thanks
   j

>  libcamera_deps = [
>      cc.find_library('dl'),
>      libudev,
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Mon, Jul 01, 2019 at 06:08:25PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 01, 2019 at 02:38:07AM +0300, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Add a set of data types to support controls:
> >
> > - ControlValue stores a control type and value in a generic way
> > - ControlId enumerates all the control identifies
> 
> identifiers
> 
> > - ControlIdentier declares the types of a control and map their names
> 
> ControlIdentifier
> 
> > - ControlInfo stores runtime information for controls
> > - ControlList contains a set of control info and value pairs
> >
> > The control definitions map is generated from the controls documentation
> > to ensure that the two will always be synchronised.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Squashed "Provide ControlValue class"
> > - Renamed Value to ControlValue
> > - Removed operator<<()
> > - Added control table generation
> > - Moved control definitions to control_definitions.h
> > - Renamed ControlTypes to controlsTypes and make it const
> > - Moved the initial controls list to a separate patch
> > - Renamed control_definitions.h to control_ids.h and
> >   control_definitions.cpp to control_types.cpp to match the contained
> >   enum and variable name respectively
> > - Indexed ControlList by ControlInfo pointer instead of value
> > - Replaced ControlInfoHash with std::hash specialisation
> > - Added automatic conversion between 32- and 64-bit integer values
> >
> > The automatic conversion between integer types was prompted by an
> > assertion failure due to the use of getInt() on the min() and max()
> > value of an Integer control. The min and max ControlValue instances are
> > create as Integer64, due to the V4L2ControlInfo class returning the
> > range as int64_t. This may need to be reworked.
> > ---
> >  Documentation/Doxyfile.in       |   3 +-
> >  include/libcamera/control_ids.h |  35 +++
> >  include/libcamera/controls.h    | 134 ++++++++++
> >  include/libcamera/meson.build   |   2 +
> >  src/libcamera/controls.cpp      | 428 ++++++++++++++++++++++++++++++++
> >  src/libcamera/gen-controls.awk  | 106 ++++++++
> >  src/libcamera/meson.build       |  11 +
> >  7 files changed, 718 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/control_ids.h
> >  create mode 100644 include/libcamera/controls.h
> >  create mode 100644 src/libcamera/controls.cpp
> >  create mode 100755 src/libcamera/gen-controls.awk
> >
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index c58631200dd5..9ca32241b895 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -868,7 +868,8 @@ EXCLUDE_SYMBOLS        = libcamera::SignalBase \
> >                           libcamera::SlotArgs \
> >                           libcamera::SlotBase \
> >                           libcamera::SlotMember \
> > -                         libcamera::SlotStatic
> > +                         libcamera::SlotStatic \
> > +                         std::*
> >
> >  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> >  # that contain example code fragments that are included (see the \include
> > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> > new file mode 100644
> > index 000000000000..d0e700da9844
> > --- /dev/null
> > +++ b/include/libcamera/control_ids.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * control_ids.h : Control ID list
> > + */
> > +
> > +#ifndef __LIBCAMERA_CONTROL_IDS_H__
> > +#define __LIBCAMERA_CONTROL_IDS_H__
> > +
> > +#include <functional>
> > +
> > +namespace libcamera {
> > +
> > +enum ControlId {
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +namespace std {
> > +
> > +template<>
> > +struct hash<libcamera::ControlId> {
> > +	using argument_type = libcamera::ControlId;
> > +	using result_type = std::size_t;
> > +
> > +	result_type operator()(const argument_type &key) const noexcept
> > +	{
> > +		return std::hash<std::underlying_type<argument_type>::type>()(key);
> > +	}
> > +};
> > +
> > +} /* namespace std */
> > +
> > +#endif // __LIBCAMERA_CONTROL_IDS_H__
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > new file mode 100644
> > index 000000000000..ad2d49d522c5
> > --- /dev/null
> > +++ b/include/libcamera/controls.h
> > @@ -0,0 +1,134 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.h - Control handling
> > + */
> > +
> > +#ifndef __LIBCAMERA_CONTROLS_H__
> > +#define __LIBCAMERA_CONTROLS_H__
> > +
> > +#include <stdint.h>
> > +#include <string>
> > +#include <unordered_map>
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +namespace libcamera {
> > +
> > +class Camera;
> > +
> > +enum ControlValueType {
> > +	ControlValueNone,
> > +	ControlValueBool,
> > +	ControlValueInteger,
> > +	ControlValueInteger64,
> > +};
> > +
> > +class ControlValue
> > +{
> > +public:
> > +	ControlValue();
> > +	ControlValue(bool value);
> > +	ControlValue(int value);
> > +	ControlValue(int64_t value);
> > +
> > +	ControlValueType type() const { return type_; };
> > +	bool isNone() const { return type_ == ControlValueNone; };
> > +
> > +	void set(bool value);
> > +	void set(int value);
> > +	void set(int64_t value);
> > +
> > +	bool getBool() const;
> > +	int getInt() const;
> > +	int64_t getInt64() const;
> > +
> > +	std::string toString() const;
> > +
> > +private:
> > +	ControlValueType type_;
> > +
> > +	union {
> > +		bool bool_;
> > +		int integer_;
> > +		int64_t integer64_;
> > +	};
> > +};
> > +
> > +struct ControlIdentifier {
> > +	ControlId id;
> > +	const char *name;
> > +	ControlValueType type;
> > +};
> > +
> > +class ControlInfo
> > +{
> > +public:
> > +	explicit ControlInfo(ControlId id, const ControlValue &min = 0,
> > +			     const ControlValue &max = 0);
> > +
> > +	ControlId id() const { return ident_->id; }
> > +	const char *name() const { return ident_->name; }
> > +	ControlValueType type() const { return ident_->type; }
> > +
> > +	const ControlValue &min() const { return min_; }
> > +	const ControlValue &max() const { return max_; }
> > +
> > +	std::string toString() const;
> > +
> > +private:
> > +	const struct ControlIdentifier *ident_;
> > +	ControlValue min_;
> > +	ControlValue max_;
> 
> Do we plan to have a step too?

And possibly a default, and possibly other information. I don't know yet
:-)

> > +};
> > +
> > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);
> > +bool operator==(const ControlId &lhs, const ControlInfo &rhs);
> > +bool operator==(const ControlInfo &lhs, const ControlId &rhs);
> > +static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +
> > +class ControlList
> > +{
> > +private:
> > +	using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;
> 
> Does it make sense to make this private if it is then used in the
> public API ?

It's only instantiated by this class for the controls_ field, so I don't
think it needs to be public.

> > +
> > +public:
> > +	ControlList(Camera *camera);
> > +
> > +	using iterator = ControlListMap::iterator;
> > +	using const_iterator = ControlListMap::const_iterator;
> > +
> > +	iterator begin() { return controls_.begin(); }
> > +	iterator end() { return controls_.end(); }
> > +	const_iterator begin() const { return controls_.begin(); }
> > +	const_iterator end() const { return controls_.end(); }
> > +
> > +	bool contains(const ControlInfo *info) const { return controls_.count(info); };
> > +	bool empty() const { return controls_.empty(); }
> > +	std::size_t size() const { return controls_.size(); }
> > +	void clear() { controls_.clear(); }
> > +
> > +	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
> > +
> > +	void update(const ControlList &list);
> > +
> > +private:
> > +	Camera *camera_;
> > +	ControlListMap controls_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CONTROLS_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 15484724df01..3067120a1598 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -2,6 +2,8 @@ libcamera_api = files([
> >      'buffer.h',
> >      'camera.h',
> >      'camera_manager.h',
> > +    'control_ids.h',
> > +    'controls.h',
> >      'event_dispatcher.h',
> >      'event_notifier.h',
> >      'geometry.h',
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > new file mode 100644
> > index 000000000000..22db2b93eff2
> > --- /dev/null
> > +++ b/src/libcamera/controls.cpp
> > @@ -0,0 +1,428 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.cpp - Control handling
> > + */
> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include <sstream>
> > +#include <string>
> > +
> > +#include "log.h"
> > +#include "utils.h"
> > +
> > +/**
> > + * \file controls.h
> > + * \brief Describes control framework and controls supported by a camera
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Controls)
> > +
> > +/**
> > + * \enum ControlValueType
> > + * Determines the type of value represented by a ControlValue
> 
> a/Determines/Defines ?
> "type of a value" ?

I'll replace this with

 \brief Define the data type of a value represented by a ControlValue

> > + * \var ControlValueNone
> > + * Identifies an unset control value
> > + * \var ControlValueBool
> > + * Identifies controls storing a boolean value
> > + * \var ControlValueInteger
> > + * Identifies controls storing an integer value
> > + * \var ControlValueInteger64
> > + * Identifies controls storing a 64-bit integer value
> > + */
> > +
> > +/**
> > + * \class ControlValue
> > + * \brief Abstract type for values in a control
> 
> Does the control 'contains' a value, or is the value part of the
> control itself? I would say "Abstract type representing the value of a
> control"

I agree, I will use your text.

> > + */
> > +
> > +/**
> > + * \brief Construct an empty ControlValue.
> > + */
> > +ControlValue::ControlValue()
> > +	: type_(ControlValueNone)
> > +{
> > +}
> > +
> 
> Does this have any use?

It can be used to represent a value that hasn't been set, as opposed to
a value set to a default value.

> > +/**
> > + * \brief Construct a Boolean ControlValue
> > + * \param[in] value Boolean value to store
> > + */
> > +ControlValue::ControlValue(bool value)
> > +	: type_(ControlValueBool), bool_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct an integer ControlValue
> > + * \param[in] value Integer value to store
> > + */
> > +ControlValue::ControlValue(int value)
> > +	: type_(ControlValueInteger), integer_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct a 64 bit integer ControlValue
> > + * \param[in] value String representation to store
> 
> Not a string but a 64-bit integer
> 
> > + */
> > +ControlValue::ControlValue(int64_t value)
> > +	: type_(ControlValueInteger64), integer64_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \fn ControlValue::type
> > + * \brief Return the type of value represented by the object
> 
> s/represented by the object// ?

I will use "Retrieve data type of the value"

> > + */
> > +
> > +/**
> > + * \fn ControlValue::isNone
> > + * \brief Determine if the value is initialised
> 
> As we check for isNone, the operation determines if the value is NOT
> initialized :)
> 
> > + * \return True if the value type is ControlValueNone, false otherwise
> 
> Just "True if the value is not initialized... "

I think explicitly saying how we check that the value is uninitialised
is useful.

> > + */
> > +
> > +/**
> > + * \brief Set the value with a boolean
> > + * \param[in] value Boolean value to store
> > + */
> > +void ControlValue::set(bool value)
> > +{
> > +	type_ = ControlValueBool;
> > +	bool_ = value;
> > +}
> 
> Ah yes, you can create None values then update. Is this a good idea?
> Won't control always have a single type associated? Or we want to be
> be able to re-use the same ControlValue with many controls ?

ControlValue instances are stored in a map, so when we do

	controls[Brightness].set(30)

if the controls container doesn't contain a ControlValue associated with
Brightness, one will be created with the default constructor, and then
its value will be overwritten. Note that we can also write

	controls[Brightness] = 30;

> > +
> > +/**
> > + * \brief Set the value with an integer
> > + * \param[in] value Integer value to store
> > + */
> > +void ControlValue::set(int value)
> > +{
> > +	type_ = ControlValueInteger;
> > +	integer_ = value;
> > +}
> > +
> > +/**
> > + * \brief Set the value with a 64 bit integer
> > + * \param[in] value 64 bit integer value to store
> > + */
> > +void ControlValue::set(int64_t value)
> > +{
> > +	type_ = ControlValueInteger64;
> > +	integer64_ = value;
> > +}
> > +
> > +/**
> > + * \brief Get the boolean value.
> 
> We usually don't end briefs with full stops, here and below.
> 
> > + *
> > + * The value type must be Boolean.
> 
> missing \return here and below.
> 
> > + */
> > +bool ControlValue::getBool() const
> > +{
> > +	ASSERT(type_ == ControlValueBool);
> 
> I wonder how easy would that be to create a control value, set it to a
> type, then to another, then read the original type and then crash the
> library here.

Pretty easy, but only in debug builds. In release builds the returned
value will just be undefined. This is something I'm open to discuss.

> > +
> > +	return bool_;
> > +}
> > +
> > +/**
> > + * \brief Get the integer value.
> > + *
> > + * The value type must be Integer or Integer64
> > + */
> > +int ControlValue::getInt() const
> > +{
> > +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> 
> is this a good idea to access 64 bit integers as integers?

It will certainly return an incorrect value if the value is larger than
1 << 32.

I did this because of a crash in a debug build :-) As you may notice in
later patches, the UVC and VIMC pipeline handler create instances of
ControlInfo for controls enumerated from V4L2. As the V4L2 controls API
return minimum and maximum values as int64_t, the ControlValue used to
store the minimum and maximum are of type ControlValueInteger64, even
for 32-bit controls. A call to info.min().getInt() the crashed.

I think this is a bit fragile, and I would like to refactor this part of
the API, but I'm not sure how yet. If you have a good idea we can
discuss it now :-) Otherwise I think it can be done on top of this
series.

> > +
> > +	return integer_;
> > +}
> > +
> > +/**
> > + * \brief Get the 64 bit integer value.
> > + *
> > + * The value type must be Integer or Integer64
> > + */
> > +int64_t ControlValue::getInt64() const
> > +{
> > +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> 
> Accessing an integer as a 64 bit one is safer instead.
> 
> > +
> > +	return integer64_;
> > +}
> > +
> > +/**
> > + * \brief Assemble and return a string describing the value
> > + * \return A string describing the ControlValue
> > + */
> > +std::string ControlValue::toString() const
> > +{
> > +	switch (type_) {
> > +	case ControlValueNone:
> > +		return "<None>";
> > +	case ControlValueBool:
> > +		return bool_ ? "True" : "False";
> > +	case ControlValueInteger:
> > +		return std::to_string(integer_);
> > +	case ControlValueInteger64:
> > +		return std::to_string(integer64_);
> > +	}
> > +
> > +	return "<ValueType Error>";
> > +}
> > +
> > +/**
> > + * \enum ControlId
> > + * Control Identifiers
> 
> numerical identifiers? There are many "info" "id" etc etc associated
> with controls here. Make it clear this is the numerical id with a bit
> more of description?

How about "Numerical control ID" ?

> > + */
> > +
> > +/**
> > + * \struct ControlIdentifier
> 
> It's easy to confuse this Identifier with ControlId if you describe
> control id as the control identifier.

I agree, and I'm not very fond of the names ControlId vs.
ControlIdentifier. If you think of better names, please feel free to
propose them. I think ControlId is probably fine, ControlIdentifier
could be renamed. ControlInfo is already taken though, and I don't think
ControlStaticInfo and ControlDynamicInfo would be good names (even if
that's essentially what they are).

> > + * \brief Describes a ControlId with control specific constant meta-data.
> 
> full stop in brief, and we usually don't use the third person afaict
> 
> > + *
> > + * Defines a Control with a unique ID, a name, and a type.
> > + * This structure is used as static part of the autogenerated control
> > + * definitions, which are generated from the ControlId documentation.
> > + *
> > + * \var ControlIdentifier::id
> > + * The unique ID for a control
> > + * \var ControlIdentifier::name
> > + * The string representation of the control
> > + * \var ControlIdentifier::type
> > + * The ValueType required to represent the control value
> > + */
> > +
> > +/*
> > + * The controlTypes are automatically generated to produce a control_types.cpp
> > + * output. This file is not for public use, and so no suitable header exists
> > + * for this sole usage of the controlTypes reference. As such the extern is
> > + * only defined here for use during the ControlInfo constructor and should not
> > + * be referenced directly elsewhere.
> > + */
> > +extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> > +
> > +/**
> > + * \class ControlInfo
> > + * \brief Describe the information and capabilities of a Control
> > + *
> > + * The ControlInfo represents control specific meta-data which is constant on a
> > + * per camera basis. ControlInfo classes are constructed by pipeline handlers
> > + * to expose the controls they support and the metadata needed to utilise those
> > + * controls.
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> > + */
> > +ControlInfo::ControlInfo(ControlId id, const ControlValue &min,
> > +			 const ControlValue &max)
> > +	: min_(min), max_(max)
> > +{
> > +	auto iter = controlTypes.find(id);
> > +	if (iter == controlTypes.end()) {
> > +		LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> > +		return;
> > +	}
> > +
> > +	ident_ = &iter->second;
> > +}
> > +
> > +/**
> > + * \fn ControlInfo::id()
> > + * \brief Retrieve the ID of the control information descriptor
> > + * \return the ControlId
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::name()
> > + * \brief Retrieve the string name of the control information descriptor
> > + * \return A string name for the Control
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::type()
> > + * \brief Retrieve the ValueType of the control information descriptor
> > + * \return The control type
> > + */
> 
> Isn't the 'information' in 'control information $SOMETHING' redundant?

Yes, I think that's a bit too long. I will write "Retrieve the control
data type" here, and change the above two functions accordingly.

> > +
> > +/**
> > + * \fn ControlInfo::min()
> > + * \brief Reports the minimum value of the control
> > + * \return a COntrolValue with the minimum setting for the control
> 
> s/COntrol/control
> 
> and also "minimum setting" would not be better described as "minium
> valid value" or similar ?
> 
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::max()
> > + * \brief Reports the maximum value of the control
> 
> I know these are not purely getters, but we usually use "Retrieve"
> 
> > + * \return a ControlValue with the maximum setting for the control
> > + */
> > +
> > +/**
> > + * \brief Provide a string representation of the ControlInfo
> > + */
> > +std::string ControlInfo::toString() const
> > +{
> > +	std::stringstream ss;
> > +
> > +	ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]";
> > +
> > +	return ss.str();
> > +}
> > +
> > +/**
> > + * \brief Compare control information for equality
> 
> I would simply "Verify it two control information are equal" or
> similar

Or just "Compare control information" ? I think we use a "for equality"
construct somewhere else, so I would probably leave this one as-is.

> > + * \param[in] lhs Left-hand side control information
> > + * \param[in] rhs Right-hand side control information
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)
> > +{
> > +	return lhs.id() == rhs.id();
> > +}
> > +
> > +/**
> > + * \brief Compare control ID and information for equality
> > + * \param[in] lhs Left-hand side control identifier
> > + * \param[in] rhs Right-hand side control information
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlId &lhs, const ControlInfo &rhs)
> > +{
> > +	return lhs == rhs.id();
> > +}
> > +
> > +/**
> > + * \brief Compare control information and ID for equality
> > + * \param[in] lhs Left-hand side control information
> > + * \param[in] rhs Right-hand side control identifier
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlInfo &lhs, const ControlId &rhs)
> > +{
> > +	return lhs.id() == rhs;
> > +}
> > +
> > +/**
> > + * \class ControlList
> > + * \brief Associates a list of ControlIds with their values for a Camera.
> 
> Full stop and third person in \brief
> 
> > + *
> > + * A ControlList specifies a map of ControlIds and Values and associated
> > + * validation against the ControlInfo for the related Camera device.
> 
> Maybe I'm tired, but this is very not clear to me. Particularly the
> fact that a map associates ControlIds and value and associated
> validation.
> 
> Now that I wrote it I get what the meaning was, but I would re-phrase
> this as
> 
> "A ControlList wraps (or other verbs) a map of ControlId to
> ControlValue and provide validation operations against the controlInfo
> provided (or other verbs) by the Camera device that created the
> control"

I fully agree this isn't very clear, I will write

 * A ControlList wraps a map of ControlId to ControlValue and provide
 * additional validation against the control information exposed by a Camera.

> > + */
> > +
> > +/**
> > + * \brief Construct a ControlList with a reference to the Camera it applies on
> > + */
> > +ControlList::ControlList(Camera *camera)
> 
> At this point it is not clear to me what the lifetime of a ControlList
> is, but in most part of our code (ok, not most, many) the camera is
> passed around as a shared pointer. Is this needed here?

It's a good question. On one hand we could, with an additional cost. On
the other hand ControlList instances will in practice be create by a
Request, or returned by a Camera function for control templates. The
requests are only valid for as long as the camera is valid, and
applications should not keep control lists around after the camera is
destroyed. I don't think we need a shared pointer here, but I will add a
sentence to explain this.

> > +	: camera_(camera)
> > +{
> > +}
> > +
> > +/**
> > + * \typedef ControlList::iterator
> > + * \brief Iterator for the controls contained within the list.
> 
> full stop
> 
> > + */
> > +
> > +/**
> > + * \typedef ControlList::const_iterator
> > + * \brief Const iterator for the controls contained within the list.
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::begin()
> > + * \brief Retrieve an iterator to the first Control in the list
> > + * \return An iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::end()
> > + * \brief Retrieve an iterator to the next element after the last controls in
> > + * the instance.
> > + * \return An iterator to the element following the last control in the instance
> 
> You are using "Control in the list" for being and "control in the
> instance" for end. Care to align them?
> 
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::begin() const
> > + * \brief Retrieve a const_iterator to the first Control in the list
> > + * \return A const_iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::end() const
> > + * \brief Retrieve a constant iterator pointing to an empty element after the
> > + * \return A const iterator to the element following the last control in the
> > + * instance
> > + */
> > +
> > +/**
> > + * \fn ControlList::contains(const ControlInfo *info) const
> > + * \brief Check if the ist contains a control with the specified \a info
> 
> a/ist/list
> I would have used instance, but it's fine.
> 
> > + * \param[in] info The control info
> > + * \return True if the list contains a matching control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::empty()
> > + * \brief Identify if the list is empty
> > + * \return True if the list does not contain any control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::size()
> > + * \brief Retrieve the number of controls in the list
> > + * \return The number of Control entries stored in the list
> > + */
> > +
> > +/**
> > + * \fn ControlList::clear()
> > + * \brief Removes all controls from the list
> > + */
> 
> Same comment for the use of list vs instance here. I think it's mostly
> a matter of tastes.
> 
> > +
> > +/**
> > + * \fn ControlList::operator[](const ControlInfo *info)
> > + * \brief Access or insert the control specified by \a info
> > + * \param[in] info The control info
> > + *
> > + * This method returns a reference to the control identified by \a info,
> > + * inserting it in the list if the info is not already present.
> > + *
> > + * \return A reference to the value of the control identified by \a info
> > + */
> > +
> > +/**
> > + * \brief Update all Control values with the value from the given \a list
> > + * \param list The list of controls to update or append to this list
> > + *
> > + * Update all controls in the ControlList, by the values given by \a list
> 
> s/by the value/with the value/ ?
> 
> > + * If the list already contains a control of this ID then it will be overwritten
> 
> s/of this ID/with this ID/ ?
> it will be overwritten or "its value will be updated" ?

The documentation is unclear, I will rewrite it. I'm also wondering if
we should rename this function to merge(), but that could imply that
that other list would get stripped out from its elements, which isn't
the case.

> > + */
> > +void ControlList::update(const ControlList &list)
> > +{
> > +	if (list.camera_ != camera_) {
> > +		LOG(Controls, Error)
> > +			<< "ControlLists can not be translated between cameras";
> > +		return;
> > +	}
> > +
> > +	for (auto it : list) {
> 
> I'm not sure if "auto &it" makes any difference or not...

I think the compiler will make it a reference automatically. I could be
wrong though.

> > +		const ControlInfo *info = it.first;
> > +		const ControlValue &value = it.second;
> > +
> > +		controls_[info] = value;
> > +	}
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> > new file mode 100755
> > index 000000000000..a91529b575db
> > --- /dev/null
> > +++ b/src/libcamera/gen-controls.awk
> 
> I trust your awk skills and assume everything is perfect here :)

I'll blame Kieran for any problem ;-)

> > @@ -0,0 +1,106 @@
> > +#!/usr/bin/awk -f
> > +
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +
> > +# Controls are documented using Doxygen in the main control.cpp source.
> > +#
> > +# Generate control tables directly from the documentation, creating enumerations
> > +# to support the IDs and static type information regarding each control.
> > +
> > +BEGIN {
> > +	id=0
> > +	input=ARGV[1]
> > +	mode=ARGV[2]
> > +	output=ARGV[3]
> > +}
> > +
> > +# Detect Doxygen style comment blocks and ignore other lines
> > +/^\/\*\*$/ { in_doxygen=1; first_line=1; next }
> > +// { if (!in_doxygen) next }
> > +
> > +# Entry point for the Control Documentation
> > +/ * \\enum ControlId$/ { in_controls=1; first_line=0; next }
> > +// { if (!in_controls) next }
> > +
> > +# Extract control information
> > +/ \* \\var/ { names[++id]=$3; first_line=0; next }
> > +/ \* ControlType:/ { types[id] = $3 }
> > +
> > +# End of comment blocks
> > +/^ \*\// { in_doxygen=0 }
> > +
> > +# Identify the end of controls
> > +/^ \* \\/ { if (first_line) exit }
> > +// { first_line=0 }
> > +
> > +################################
> > +# Support output file generation
> > +
> > +function basename(file) {
> > +	sub(".*/", "", file)
> > +	return file
> > +}
> > +
> > +function Header(file, description) {
> > +	print "/* SPDX-License-Identifier: LGPL-2.1-or-later */" > file
> > +	print "/*" > file
> > +	print " * Copyright (C) 2019, Google Inc." > file
> > +	print " *" > file
> > +	print " * " basename(file) " - " description > file
> > +	print " *" > file
> > +	print " * This file is autogenerated. Do not edit." > file
> > +	print " */" > file
> > +	print "" > file
> > +}
> > +
> > +function EnterNameSpace(file) {
> > +	print "namespace libcamera {" > file
> > +	print "" > file
> > +}
> > +
> > +function ExitNameSpace(file) {
> > +	print "" > file
> > +	print "} /* namespace libcamera */" > file
> > +}
> > +
> > +function GenerateHeader(file) {
> > +	Header(file, "Control ID list")
> > +
> > +	print "#ifndef __LIBCAMERA_CONTROL_IDS_H__" > file
> > +	print "#define __LIBCAMERA_CONTROL_IDS_H__" > file
> > +	print "" > file
> > +
> > +	EnterNameSpace(file)
> > +	print "enum ControlId {" > file
> > +	for (i=1; i <= id; ++i) {
> > +		printf "\t%s,\n", names[i] > file
> > +	}
> > +	print "};" > file
> > +	ExitNameSpace(file)
> > +
> > +	print "" > file
> > +	print "#endif // __LIBCAMERA_CONTROL_IDS_H__" > file
> > +}
> > +
> > +function GenerateTable(file) {
> > +	Header(file, "Control types")
> > +	print "#include <libcamera/controls.h>" > file
> > +	print "" > file
> > +
> > +	EnterNameSpace(file)
> > +
> > +	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> > +	print "controlTypes {" > file
> > +	for (i=1; i <= id; ++i) {
> > +		printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> > +	}
> > +	print "};" > file
> > +	ExitNameSpace(file)
> > +}
> > +
> > +END {
> > +	if (mode == "--header")
> > +		GenerateHeader(output)
> > +	else
> > +		GenerateTable(output)
> > +}
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 985aa7e8ab0e..b1ee92735e41 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -3,6 +3,7 @@ libcamera_sources = files([
> >      'camera.cpp',
> >      'camera_manager.cpp',
> >      'camera_sensor.cpp',
> > +    'controls.cpp',
> >      'device_enumerator.cpp',
> >      'device_enumerator_sysfs.cpp',
> >      'event_dispatcher.cpp',
> > @@ -66,6 +67,16 @@ if libudev.found()
> >      ])
> >  endif
> >
> > +gen_controls = files('gen-controls.awk')
> > +
> > +control_types_cpp = custom_target('control_types_cpp',
> > +                                  input : files('controls.cpp'),
> > +                                  output : 'control_types.cpp',
> > +                                  depend_files : gen_controls,
> > +                                  command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> > +
> > +libcamera_sources += control_types_cpp
> > +
> 
> If both you and Kieran had a look here, I assume this is great
> 
> >  libcamera_deps = [
> >      cc.find_library('dl'),
> >      libudev,

HI Laurent,

On Mon, Jul 01, 2019 at 08:04:46PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Jul 01, 2019 at 06:08:25PM +0200, Jacopo Mondi wrote:
> > On Mon, Jul 01, 2019 at 02:38:07AM +0300, Laurent Pinchart wrote:
> > > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > Add a set of data types to support controls:
> > >
> > > - ControlValue stores a control type and value in a generic way
> > > - ControlId enumerates all the control identifies
> >
> > identifiers
> >
> > > - ControlIdentier declares the types of a control and map their names
> >
> > ControlIdentifier
> >
> > > - ControlInfo stores runtime information for controls
> > > - ControlList contains a set of control info and value pairs
> > >
> > > The control definitions map is generated from the controls documentation
> > > to ensure that the two will always be synchronised.
> > >
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v2:
> > >
> > > - Squashed "Provide ControlValue class"
> > > - Renamed Value to ControlValue
> > > - Removed operator<<()
> > > - Added control table generation
> > > - Moved control definitions to control_definitions.h
> > > - Renamed ControlTypes to controlsTypes and make it const
> > > - Moved the initial controls list to a separate patch
> > > - Renamed control_definitions.h to control_ids.h and
> > >   control_definitions.cpp to control_types.cpp to match the contained
> > >   enum and variable name respectively
> > > - Indexed ControlList by ControlInfo pointer instead of value
> > > - Replaced ControlInfoHash with std::hash specialisation
> > > - Added automatic conversion between 32- and 64-bit integer values
> > >
> > > The automatic conversion between integer types was prompted by an
> > > assertion failure due to the use of getInt() on the min() and max()
> > > value of an Integer control. The min and max ControlValue instances are
> > > create as Integer64, due to the V4L2ControlInfo class returning the
> > > range as int64_t. This may need to be reworked.
> > > ---
> > >  Documentation/Doxyfile.in       |   3 +-
> > >  include/libcamera/control_ids.h |  35 +++
> > >  include/libcamera/controls.h    | 134 ++++++++++
> > >  include/libcamera/meson.build   |   2 +
> > >  src/libcamera/controls.cpp      | 428 ++++++++++++++++++++++++++++++++
> > >  src/libcamera/gen-controls.awk  | 106 ++++++++
> > >  src/libcamera/meson.build       |  11 +
> > >  7 files changed, 718 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/libcamera/control_ids.h
> > >  create mode 100644 include/libcamera/controls.h
> > >  create mode 100644 src/libcamera/controls.cpp
> > >  create mode 100755 src/libcamera/gen-controls.awk
> > >
> > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > > index c58631200dd5..9ca32241b895 100644
> > > --- a/Documentation/Doxyfile.in
> > > +++ b/Documentation/Doxyfile.in
> > > @@ -868,7 +868,8 @@ EXCLUDE_SYMBOLS        = libcamera::SignalBase \
> > >                           libcamera::SlotArgs \
> > >                           libcamera::SlotBase \
> > >                           libcamera::SlotMember \
> > > -                         libcamera::SlotStatic
> > > +                         libcamera::SlotStatic \
> > > +                         std::*
> > >
> > >  # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> > >  # that contain example code fragments that are included (see the \include
> > > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> > > new file mode 100644
> > > index 000000000000..d0e700da9844
> > > --- /dev/null
> > > +++ b/include/libcamera/control_ids.h
> > > @@ -0,0 +1,35 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * control_ids.h : Control ID list
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_CONTROL_IDS_H__
> > > +#define __LIBCAMERA_CONTROL_IDS_H__
> > > +
> > > +#include <functional>
> > > +
> > > +namespace libcamera {
> > > +
> > > +enum ControlId {
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +namespace std {
> > > +
> > > +template<>
> > > +struct hash<libcamera::ControlId> {
> > > +	using argument_type = libcamera::ControlId;
> > > +	using result_type = std::size_t;
> > > +
> > > +	result_type operator()(const argument_type &key) const noexcept
> > > +	{
> > > +		return std::hash<std::underlying_type<argument_type>::type>()(key);
> > > +	}
> > > +};
> > > +
> > > +} /* namespace std */
> > > +
> > > +#endif // __LIBCAMERA_CONTROL_IDS_H__
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > > new file mode 100644
> > > index 000000000000..ad2d49d522c5
> > > --- /dev/null
> > > +++ b/include/libcamera/controls.h
> > > @@ -0,0 +1,134 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * controls.h - Control handling
> > > + */
> > > +
> > > +#ifndef __LIBCAMERA_CONTROLS_H__
> > > +#define __LIBCAMERA_CONTROLS_H__
> > > +
> > > +#include <stdint.h>
> > > +#include <string>
> > > +#include <unordered_map>
> > > +
> > > +#include <libcamera/control_ids.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +class Camera;
> > > +
> > > +enum ControlValueType {
> > > +	ControlValueNone,
> > > +	ControlValueBool,
> > > +	ControlValueInteger,
> > > +	ControlValueInteger64,
> > > +};
> > > +
> > > +class ControlValue
> > > +{
> > > +public:
> > > +	ControlValue();
> > > +	ControlValue(bool value);
> > > +	ControlValue(int value);
> > > +	ControlValue(int64_t value);
> > > +
> > > +	ControlValueType type() const { return type_; };
> > > +	bool isNone() const { return type_ == ControlValueNone; };
> > > +
> > > +	void set(bool value);
> > > +	void set(int value);
> > > +	void set(int64_t value);
> > > +
> > > +	bool getBool() const;
> > > +	int getInt() const;
> > > +	int64_t getInt64() const;
> > > +
> > > +	std::string toString() const;
> > > +
> > > +private:
> > > +	ControlValueType type_;
> > > +
> > > +	union {
> > > +		bool bool_;
> > > +		int integer_;
> > > +		int64_t integer64_;
> > > +	};
> > > +};
> > > +
> > > +struct ControlIdentifier {
> > > +	ControlId id;
> > > +	const char *name;
> > > +	ControlValueType type;
> > > +};
> > > +
> > > +class ControlInfo
> > > +{
> > > +public:
> > > +	explicit ControlInfo(ControlId id, const ControlValue &min = 0,
> > > +			     const ControlValue &max = 0);
> > > +
> > > +	ControlId id() const { return ident_->id; }
> > > +	const char *name() const { return ident_->name; }
> > > +	ControlValueType type() const { return ident_->type; }
> > > +
> > > +	const ControlValue &min() const { return min_; }
> > > +	const ControlValue &max() const { return max_; }
> > > +
> > > +	std::string toString() const;
> > > +
> > > +private:
> > > +	const struct ControlIdentifier *ident_;
> > > +	ControlValue min_;
> > > +	ControlValue max_;
> >
> > Do we plan to have a step too?
>
> And possibly a default, and possibly other information. I don't know yet
> :-)
>
> > > +};
> > > +
> > > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);
> > > +bool operator==(const ControlId &lhs, const ControlInfo &rhs);
> > > +bool operator==(const ControlInfo &lhs, const ControlId &rhs);
> > > +static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)
> > > +{
> > > +	return !(lhs == rhs);
> > > +}
> > > +static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)
> > > +{
> > > +	return !(lhs == rhs);
> > > +}
> > > +static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)
> > > +{
> > > +	return !(lhs == rhs);
> > > +}
> > > +
> > > +class ControlList
> > > +{
> > > +private:
> > > +	using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;
> >
> > Does it make sense to make this private if it is then used in the
> > public API ?
>
> It's only instantiated by this class for the controls_ field, so I don't
> think it needs to be public.
>
> > > +
> > > +public:
> > > +	ControlList(Camera *camera);
> > > +
> > > +	using iterator = ControlListMap::iterator;
> > > +	using const_iterator = ControlListMap::const_iterator;
> > > +
> > > +	iterator begin() { return controls_.begin(); }
> > > +	iterator end() { return controls_.end(); }
> > > +	const_iterator begin() const { return controls_.begin(); }
> > > +	const_iterator end() const { return controls_.end(); }
> > > +
> > > +	bool contains(const ControlInfo *info) const { return controls_.count(info); };
> > > +	bool empty() const { return controls_.empty(); }
> > > +	std::size_t size() const { return controls_.size(); }
> > > +	void clear() { controls_.clear(); }
> > > +
> > > +	ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
> > > +
> > > +	void update(const ControlList &list);
> > > +
> > > +private:
> > > +	Camera *camera_;
> > > +	ControlListMap controls_;
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +
> > > +#endif /* __LIBCAMERA_CONTROLS_H__ */
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index 15484724df01..3067120a1598 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -2,6 +2,8 @@ libcamera_api = files([
> > >      'buffer.h',
> > >      'camera.h',
> > >      'camera_manager.h',
> > > +    'control_ids.h',
> > > +    'controls.h',
> > >      'event_dispatcher.h',
> > >      'event_notifier.h',
> > >      'geometry.h',
> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > > new file mode 100644
> > > index 000000000000..22db2b93eff2
> > > --- /dev/null
> > > +++ b/src/libcamera/controls.cpp
> > > @@ -0,0 +1,428 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2019, Google Inc.
> > > + *
> > > + * controls.cpp - Control handling
> > > + */
> > > +
> > > +#include <libcamera/controls.h>
> > > +
> > > +#include <sstream>
> > > +#include <string>
> > > +
> > > +#include "log.h"
> > > +#include "utils.h"
> > > +
> > > +/**
> > > + * \file controls.h
> > > + * \brief Describes control framework and controls supported by a camera
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +LOG_DEFINE_CATEGORY(Controls)
> > > +
> > > +/**
> > > + * \enum ControlValueType
> > > + * Determines the type of value represented by a ControlValue
> >
> > a/Determines/Defines ?
> > "type of a value" ?
>
> I'll replace this with
>
>  \brief Define the data type of a value represented by a ControlValue
>
> > > + * \var ControlValueNone
> > > + * Identifies an unset control value
> > > + * \var ControlValueBool
> > > + * Identifies controls storing a boolean value
> > > + * \var ControlValueInteger
> > > + * Identifies controls storing an integer value
> > > + * \var ControlValueInteger64
> > > + * Identifies controls storing a 64-bit integer value
> > > + */
> > > +
> > > +/**
> > > + * \class ControlValue
> > > + * \brief Abstract type for values in a control
> >
> > Does the control 'contains' a value, or is the value part of the
> > control itself? I would say "Abstract type representing the value of a
> > control"
>
> I agree, I will use your text.
>
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct an empty ControlValue.
> > > + */
> > > +ControlValue::ControlValue()
> > > +	: type_(ControlValueNone)
> > > +{
> > > +}
> > > +
> >
> > Does this have any use?
>
> It can be used to represent a value that hasn't been set, as opposed to
> a value set to a default value.
>
> > > +/**
> > > + * \brief Construct a Boolean ControlValue
> > > + * \param[in] value Boolean value to store
> > > + */
> > > +ControlValue::ControlValue(bool value)
> > > +	: type_(ControlValueBool), bool_(value)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct an integer ControlValue
> > > + * \param[in] value Integer value to store
> > > + */
> > > +ControlValue::ControlValue(int value)
> > > +	: type_(ControlValueInteger), integer_(value)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct a 64 bit integer ControlValue
> > > + * \param[in] value String representation to store
> >
> > Not a string but a 64-bit integer
> >
> > > + */
> > > +ControlValue::ControlValue(int64_t value)
> > > +	: type_(ControlValueInteger64), integer64_(value)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \fn ControlValue::type
> > > + * \brief Return the type of value represented by the object
> >
> > s/represented by the object// ?
>
> I will use "Retrieve data type of the value"
>
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlValue::isNone
> > > + * \brief Determine if the value is initialised
> >
> > As we check for isNone, the operation determines if the value is NOT
> > initialized :)
> >
> > > + * \return True if the value type is ControlValueNone, false otherwise
> >
> > Just "True if the value is not initialized... "
>
> I think explicitly saying how we check that the value is uninitialised
> is useful.
>
> > > + */
> > > +
> > > +/**
> > > + * \brief Set the value with a boolean
> > > + * \param[in] value Boolean value to store
> > > + */
> > > +void ControlValue::set(bool value)
> > > +{
> > > +	type_ = ControlValueBool;
> > > +	bool_ = value;
> > > +}
> >
> > Ah yes, you can create None values then update. Is this a good idea?
> > Won't control always have a single type associated? Or we want to be
> > be able to re-use the same ControlValue with many controls ?
>
> ControlValue instances are stored in a map, so when we do
>
> 	controls[Brightness].set(30)
>
> if the controls container doesn't contain a ControlValue associated with
> Brightness, one will be created with the default constructor, and then
> its value will be overwritten. Note that we can also write
>
> 	controls[Brightness] = 30;
>
> > > +
> > > +/**
> > > + * \brief Set the value with an integer
> > > + * \param[in] value Integer value to store
> > > + */
> > > +void ControlValue::set(int value)
> > > +{
> > > +	type_ = ControlValueInteger;
> > > +	integer_ = value;
> > > +}
> > > +
> > > +/**
> > > + * \brief Set the value with a 64 bit integer
> > > + * \param[in] value 64 bit integer value to store
> > > + */
> > > +void ControlValue::set(int64_t value)
> > > +{
> > > +	type_ = ControlValueInteger64;
> > > +	integer64_ = value;
> > > +}
> > > +
> > > +/**
> > > + * \brief Get the boolean value.
> >
> > We usually don't end briefs with full stops, here and below.
> >
> > > + *
> > > + * The value type must be Boolean.
> >
> > missing \return here and below.
> >
> > > + */
> > > +bool ControlValue::getBool() const
> > > +{
> > > +	ASSERT(type_ == ControlValueBool);
> >
> > I wonder how easy would that be to create a control value, set it to a
> > type, then to another, then read the original type and then crash the
> > library here.
>
> Pretty easy, but only in debug builds. In release builds the returned
> value will just be undefined. This is something I'm open to discuss.
>

We should record this with a todo entry maybe ?

> > > +
> > > +	return bool_;
> > > +}
> > > +
> > > +/**
> > > + * \brief Get the integer value.
> > > + *
> > > + * The value type must be Integer or Integer64
> > > + */
> > > +int ControlValue::getInt() const
> > > +{
> > > +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> >
> > is this a good idea to access 64 bit integers as integers?
>
> It will certainly return an incorrect value if the value is larger than
> 1 << 32.
>
> I did this because of a crash in a debug build :-) As you may notice in
> later patches, the UVC and VIMC pipeline handler create instances of
> ControlInfo for controls enumerated from V4L2. As the V4L2 controls API
> return minimum and maximum values as int64_t, the ControlValue used to
> store the minimum and maximum are of type ControlValueInteger64, even
> for 32-bit controls. A call to info.min().getInt() the crashed.

I see. Yes, its a bit fragile indeed, but I think we can fix on top.
record this with a todo as well?

>
> I think this is a bit fragile, and I would like to refactor this part of
> the API, but I'm not sure how yet. If you have a good idea we can
> discuss it now :-) Otherwise I think it can be done on top of this
> series.
>
> > > +
> > > +	return integer_;
> > > +}
> > > +
> > > +/**
> > > + * \brief Get the 64 bit integer value.
> > > + *
> > > + * The value type must be Integer or Integer64
> > > + */
> > > +int64_t ControlValue::getInt64() const
> > > +{
> > > +	ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
> >
> > Accessing an integer as a 64 bit one is safer instead.
> >
> > > +
> > > +	return integer64_;
> > > +}
> > > +
> > > +/**
> > > + * \brief Assemble and return a string describing the value
> > > + * \return A string describing the ControlValue
> > > + */
> > > +std::string ControlValue::toString() const
> > > +{
> > > +	switch (type_) {
> > > +	case ControlValueNone:
> > > +		return "<None>";
> > > +	case ControlValueBool:
> > > +		return bool_ ? "True" : "False";
> > > +	case ControlValueInteger:
> > > +		return std::to_string(integer_);
> > > +	case ControlValueInteger64:
> > > +		return std::to_string(integer64_);
> > > +	}
> > > +
> > > +	return "<ValueType Error>";
> > > +}
> > > +
> > > +/**
> > > + * \enum ControlId
> > > + * Control Identifiers
> >
> > numerical identifiers? There are many "info" "id" etc etc associated
> > with controls here. Make it clear this is the numerical id with a bit
> > more of description?
>
> How about "Numerical control ID" ?
>
> > > + */
> > > +
> > > +/**
> > > + * \struct ControlIdentifier
> >
> > It's easy to confuse this Identifier with ControlId if you describe
> > control id as the control identifier.
>
> I agree, and I'm not very fond of the names ControlId vs.
> ControlIdentifier. If you think of better names, please feel free to
> propose them. I think ControlId is probably fine, ControlIdentifier
> could be renamed. ControlInfo is already taken though, and I don't think
> ControlStaticInfo and ControlDynamicInfo would be good names (even if
> that's essentially what they are).
>

Naming is not easy here indeed. I would propose ControlDesc for static
information but I'm not sure it's any better.

I think for the moment is fine..

> > > + * \brief Describes a ControlId with control specific constant meta-data.
> >
> > full stop in brief, and we usually don't use the third person afaict
> >
> > > + *
> > > + * Defines a Control with a unique ID, a name, and a type.
> > > + * This structure is used as static part of the autogenerated control
> > > + * definitions, which are generated from the ControlId documentation.
> > > + *
> > > + * \var ControlIdentifier::id
> > > + * The unique ID for a control
> > > + * \var ControlIdentifier::name
> > > + * The string representation of the control
> > > + * \var ControlIdentifier::type
> > > + * The ValueType required to represent the control value
> > > + */
> > > +
> > > +/*
> > > + * The controlTypes are automatically generated to produce a control_types.cpp
> > > + * output. This file is not for public use, and so no suitable header exists
> > > + * for this sole usage of the controlTypes reference. As such the extern is
> > > + * only defined here for use during the ControlInfo constructor and should not
> > > + * be referenced directly elsewhere.
> > > + */
> > > +extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> > > +
> > > +/**
> > > + * \class ControlInfo
> > > + * \brief Describe the information and capabilities of a Control
> > > + *
> > > + * The ControlInfo represents control specific meta-data which is constant on a
> > > + * per camera basis. ControlInfo classes are constructed by pipeline handlers
> > > + * to expose the controls they support and the metadata needed to utilise those
> > > + * controls.
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> > > + */
> > > +ControlInfo::ControlInfo(ControlId id, const ControlValue &min,
> > > +			 const ControlValue &max)
> > > +	: min_(min), max_(max)
> > > +{
> > > +	auto iter = controlTypes.find(id);
> > > +	if (iter == controlTypes.end()) {
> > > +		LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> > > +		return;
> > > +	}
> > > +
> > > +	ident_ = &iter->second;
> > > +}
> > > +
> > > +/**
> > > + * \fn ControlInfo::id()
> > > + * \brief Retrieve the ID of the control information descriptor
> > > + * \return the ControlId
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlInfo::name()
> > > + * \brief Retrieve the string name of the control information descriptor
> > > + * \return A string name for the Control
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlInfo::type()
> > > + * \brief Retrieve the ValueType of the control information descriptor
> > > + * \return The control type
> > > + */
> >
> > Isn't the 'information' in 'control information $SOMETHING' redundant?
>
> Yes, I think that's a bit too long. I will write "Retrieve the control
> data type" here, and change the above two functions accordingly.
>
> > > +
> > > +/**
> > > + * \fn ControlInfo::min()
> > > + * \brief Reports the minimum value of the control
> > > + * \return a COntrolValue with the minimum setting for the control
> >
> > s/COntrol/control
> >
> > and also "minimum setting" would not be better described as "minium
> > valid value" or similar ?
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlInfo::max()
> > > + * \brief Reports the maximum value of the control
> >
> > I know these are not purely getters, but we usually use "Retrieve"
> >
> > > + * \return a ControlValue with the maximum setting for the control
> > > + */
> > > +
> > > +/**
> > > + * \brief Provide a string representation of the ControlInfo
> > > + */
> > > +std::string ControlInfo::toString() const
> > > +{
> > > +	std::stringstream ss;
> > > +
> > > +	ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]";
> > > +
> > > +	return ss.str();
> > > +}
> > > +
> > > +/**
> > > + * \brief Compare control information for equality
> >
> > I would simply "Verify it two control information are equal" or
> > similar
>
> Or just "Compare control information" ? I think we use a "for equality"
> construct somewhere else, so I would probably leave this one as-is.
>
> > > + * \param[in] lhs Left-hand side control information
> > > + * \param[in] rhs Right-hand side control information
> > > + *
> > > + * Control information are compared based on their ID only, as a camera may not
> > > + * have two separate controls with the same ID.
> > > + *
> > > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > > + */
> > > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)
> > > +{
> > > +	return lhs.id() == rhs.id();
> > > +}
> > > +
> > > +/**
> > > + * \brief Compare control ID and information for equality
> > > + * \param[in] lhs Left-hand side control identifier
> > > + * \param[in] rhs Right-hand side control information
> > > + *
> > > + * Control information are compared based on their ID only, as a camera may not
> > > + * have two separate controls with the same ID.
> > > + *
> > > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > > + */
> > > +bool operator==(const ControlId &lhs, const ControlInfo &rhs)
> > > +{
> > > +	return lhs == rhs.id();
> > > +}
> > > +
> > > +/**
> > > + * \brief Compare control information and ID for equality
> > > + * \param[in] lhs Left-hand side control information
> > > + * \param[in] rhs Right-hand side control identifier
> > > + *
> > > + * Control information are compared based on their ID only, as a camera may not
> > > + * have two separate controls with the same ID.
> > > + *
> > > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > > + */
> > > +bool operator==(const ControlInfo &lhs, const ControlId &rhs)
> > > +{
> > > +	return lhs.id() == rhs;
> > > +}
> > > +
> > > +/**
> > > + * \class ControlList
> > > + * \brief Associates a list of ControlIds with their values for a Camera.
> >
> > Full stop and third person in \brief
> >
> > > + *
> > > + * A ControlList specifies a map of ControlIds and Values and associated
> > > + * validation against the ControlInfo for the related Camera device.
> >
> > Maybe I'm tired, but this is very not clear to me. Particularly the
> > fact that a map associates ControlIds and value and associated
> > validation.
> >
> > Now that I wrote it I get what the meaning was, but I would re-phrase
> > this as
> >
> > "A ControlList wraps (or other verbs) a map of ControlId to
> > ControlValue and provide validation operations against the controlInfo
> > provided (or other verbs) by the Camera device that created the
> > control"
>
> I fully agree this isn't very clear, I will write
>
>  * A ControlList wraps a map of ControlId to ControlValue and provide
>  * additional validation against the control information exposed by a Camera.
>
> > > + */
> > > +
> > > +/**
> > > + * \brief Construct a ControlList with a reference to the Camera it applies on
> > > + */
> > > +ControlList::ControlList(Camera *camera)
> >
> > At this point it is not clear to me what the lifetime of a ControlList
> > is, but in most part of our code (ok, not most, many) the camera is
> > passed around as a shared pointer. Is this needed here?
>
> It's a good question. On one hand we could, with an additional cost. On
> the other hand ControlList instances will in practice be create by a
> Request, or returned by a Camera function for control templates. The
> requests are only valid for as long as the camera is valid, and
> applications should not keep control lists around after the camera is
> destroyed. I don't think we need a shared pointer here, but I will add a
> sentence to explain this.
>

Thanks for the explanation. I think we're fine then.

> > > +	: camera_(camera)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \typedef ControlList::iterator
> > > + * \brief Iterator for the controls contained within the list.
> >
> > full stop
> >
> > > + */
> > > +
> > > +/**
> > > + * \typedef ControlList::const_iterator
> > > + * \brief Const iterator for the controls contained within the list.
> > > + */
> > > +
> > > +/**
> > > + * \fn iterator ControlList::begin()
> > > + * \brief Retrieve an iterator to the first Control in the list
> > > + * \return An iterator to the first Control in the list
> > > + */
> > > +
> > > +/**
> > > + * \fn iterator ControlList::end()
> > > + * \brief Retrieve an iterator to the next element after the last controls in
> > > + * the instance.
> > > + * \return An iterator to the element following the last control in the instance
> >
> > You are using "Control in the list" for being and "control in the
> > instance" for end. Care to align them?
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn const_iterator ControlList::begin() const
> > > + * \brief Retrieve a const_iterator to the first Control in the list
> > > + * \return A const_iterator to the first Control in the list
> > > + */
> > > +
> > > +/**
> > > + * \fn const_iterator ControlList::end() const
> > > + * \brief Retrieve a constant iterator pointing to an empty element after the
> > > + * \return A const iterator to the element following the last control in the
> > > + * instance
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlList::contains(const ControlInfo *info) const
> > > + * \brief Check if the ist contains a control with the specified \a info
> >
> > a/ist/list
> > I would have used instance, but it's fine.
> >
> > > + * \param[in] info The control info
> > > + * \return True if the list contains a matching control, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlList::empty()
> > > + * \brief Identify if the list is empty
> > > + * \return True if the list does not contain any control, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlList::size()
> > > + * \brief Retrieve the number of controls in the list
> > > + * \return The number of Control entries stored in the list
> > > + */
> > > +
> > > +/**
> > > + * \fn ControlList::clear()
> > > + * \brief Removes all controls from the list
> > > + */
> >
> > Same comment for the use of list vs instance here. I think it's mostly
> > a matter of tastes.
> >
> > > +
> > > +/**
> > > + * \fn ControlList::operator[](const ControlInfo *info)
> > > + * \brief Access or insert the control specified by \a info
> > > + * \param[in] info The control info
> > > + *
> > > + * This method returns a reference to the control identified by \a info,
> > > + * inserting it in the list if the info is not already present.
> > > + *
> > > + * \return A reference to the value of the control identified by \a info
> > > + */
> > > +
> > > +/**
> > > + * \brief Update all Control values with the value from the given \a list
> > > + * \param list The list of controls to update or append to this list
> > > + *
> > > + * Update all controls in the ControlList, by the values given by \a list
> >
> > s/by the value/with the value/ ?
> >
> > > + * If the list already contains a control of this ID then it will be overwritten
> >
> > s/of this ID/with this ID/ ?
> > it will be overwritten or "its value will be updated" ?
>
> The documentation is unclear, I will rewrite it. I'm also wondering if
> we should rename this function to merge(), but that could imply that
> that other list would get stripped out from its elements, which isn't
> the case.
>

Naming is hard here as well. I'm not too un-happy with update() and
not in love with merge(). What about just set() ?

> > > + */
> > > +void ControlList::update(const ControlList &list)
> > > +{
> > > +	if (list.camera_ != camera_) {
> > > +		LOG(Controls, Error)
> > > +			<< "ControlLists can not be translated between cameras";
> > > +		return;
> > > +	}
> > > +
> > > +	for (auto it : list) {
> >
> > I'm not sure if "auto &it" makes any difference or not...
>
> I think the compiler will make it a reference automatically. I could be
> wrong though.
>
> > > +		const ControlInfo *info = it.first;
> > > +		const ControlValue &value = it.second;
> > > +
> > > +		controls_[info] = value;
> > > +	}
> > > +}
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> > > new file mode 100755
> > > index 000000000000..a91529b575db
> > > --- /dev/null
> > > +++ b/src/libcamera/gen-controls.awk
> >
> > I trust your awk skills and assume everything is perfect here :)
>
> I'll blame Kieran for any problem ;-)
>

He could then blame me for not having reviewed this :)

Thanks
   j

> > > @@ -0,0 +1,106 @@
> > > +#!/usr/bin/awk -f
> > > +
> > > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > > +
> > > +# Controls are documented using Doxygen in the main control.cpp source.
> > > +#
> > > +# Generate control tables directly from the documentation, creating enumerations
> > > +# to support the IDs and static type information regarding each control.
> > > +
> > > +BEGIN {
> > > +	id=0
> > > +	input=ARGV[1]
> > > +	mode=ARGV[2]
> > > +	output=ARGV[3]
> > > +}
> > > +
> > > +# Detect Doxygen style comment blocks and ignore other lines
> > > +/^\/\*\*$/ { in_doxygen=1; first_line=1; next }
> > > +// { if (!in_doxygen) next }
> > > +
> > > +# Entry point for the Control Documentation
> > > +/ * \\enum ControlId$/ { in_controls=1; first_line=0; next }
> > > +// { if (!in_controls) next }
> > > +
> > > +# Extract control information
> > > +/ \* \\var/ { names[++id]=$3; first_line=0; next }
> > > +/ \* ControlType:/ { types[id] = $3 }
> > > +
> > > +# End of comment blocks
> > > +/^ \*\// { in_doxygen=0 }
> > > +
> > > +# Identify the end of controls
> > > +/^ \* \\/ { if (first_line) exit }
> > > +// { first_line=0 }
> > > +
> > > +################################
> > > +# Support output file generation
> > > +
> > > +function basename(file) {
> > > +	sub(".*/", "", file)
> > > +	return file
> > > +}
> > > +
> > > +function Header(file, description) {
> > > +	print "/* SPDX-License-Identifier: LGPL-2.1-or-later */" > file
> > > +	print "/*" > file
> > > +	print " * Copyright (C) 2019, Google Inc." > file
> > > +	print " *" > file
> > > +	print " * " basename(file) " - " description > file
> > > +	print " *" > file
> > > +	print " * This file is autogenerated. Do not edit." > file
> > > +	print " */" > file
> > > +	print "" > file
> > > +}
> > > +
> > > +function EnterNameSpace(file) {
> > > +	print "namespace libcamera {" > file
> > > +	print "" > file
> > > +}
> > > +
> > > +function ExitNameSpace(file) {
> > > +	print "" > file
> > > +	print "} /* namespace libcamera */" > file
> > > +}
> > > +
> > > +function GenerateHeader(file) {
> > > +	Header(file, "Control ID list")
> > > +
> > > +	print "#ifndef __LIBCAMERA_CONTROL_IDS_H__" > file
> > > +	print "#define __LIBCAMERA_CONTROL_IDS_H__" > file
> > > +	print "" > file
> > > +
> > > +	EnterNameSpace(file)
> > > +	print "enum ControlId {" > file
> > > +	for (i=1; i <= id; ++i) {
> > > +		printf "\t%s,\n", names[i] > file
> > > +	}
> > > +	print "};" > file
> > > +	ExitNameSpace(file)
> > > +
> > > +	print "" > file
> > > +	print "#endif // __LIBCAMERA_CONTROL_IDS_H__" > file
> > > +}
> > > +
> > > +function GenerateTable(file) {
> > > +	Header(file, "Control types")
> > > +	print "#include <libcamera/controls.h>" > file
> > > +	print "" > file
> > > +
> > > +	EnterNameSpace(file)
> > > +
> > > +	print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> > > +	print "controlTypes {" > file
> > > +	for (i=1; i <= id; ++i) {
> > > +		printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> > > +	}
> > > +	print "};" > file
> > > +	ExitNameSpace(file)
> > > +}
> > > +
> > > +END {
> > > +	if (mode == "--header")
> > > +		GenerateHeader(output)
> > > +	else
> > > +		GenerateTable(output)
> > > +}
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index 985aa7e8ab0e..b1ee92735e41 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -3,6 +3,7 @@ libcamera_sources = files([
> > >      'camera.cpp',
> > >      'camera_manager.cpp',
> > >      'camera_sensor.cpp',
> > > +    'controls.cpp',
> > >      'device_enumerator.cpp',
> > >      'device_enumerator_sysfs.cpp',
> > >      'event_dispatcher.cpp',
> > > @@ -66,6 +67,16 @@ if libudev.found()
> > >      ])
> > >  endif
> > >
> > > +gen_controls = files('gen-controls.awk')
> > > +
> > > +control_types_cpp = custom_target('control_types_cpp',
> > > +                                  input : files('controls.cpp'),
> > > +                                  output : 'control_types.cpp',
> > > +                                  depend_files : gen_controls,
> > > +                                  command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> > > +
> > > +libcamera_sources += control_types_cpp
> > > +
> >
> > If both you and Kieran had a look here, I assume this is great
> >
> > >  libcamera_deps = [
> > >      cc.find_library('dl'),
> > >      libudev,
>
> --
> Regards,
>
> Laurent Pinchart