[libcamera-devel,RFC,v2,3/9] libcamera: controls: Introduce Control structures

1496 diff mbox series

ControlIdentifiers declare the types of a control, and map their names,
and the ControlInfo class allows runtime state to be represented.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/controls.h  |  58 ++++++++++++
 include/libcamera/meson.build |   1 +
 src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
 src/libcamera/meson.build     |   1 +
 4 files changed, 220 insertions(+)
 create mode 100644 include/libcamera/controls.h
 create mode 100644 src/libcamera/controls.cpp

Hi Kieran,

Thanks for your work.

On 2019-06-21 17:13:55 +0100, Kieran Bingham wrote:
> ControlIdentifiers declare the types of a control, and map their names,
> and the ControlInfo class allows runtime state to be represented.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/controls.h  |  58 ++++++++++++
>  include/libcamera/meson.build |   1 +
>  src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  4 files changed, 220 insertions(+)
>  create mode 100644 include/libcamera/controls.h
>  create mode 100644 src/libcamera/controls.cpp
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> new file mode 100644
> index 000000000000..95198d41c4cf
> --- /dev/null
> +++ b/include/libcamera/controls.h
> @@ -0,0 +1,58 @@
> +/* 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 <libcamera/value.h>
> +
> +namespace libcamera {
> +
> +enum ControlId : uint32_t {
> +	/* IPA Controls */
> +	IpaAwbEnable,
> +
> +	/* Manual Controls */
> +	ManualBrightness,
> +	ManualExposure,
> +	ManualGain,
> +};
> +
> +struct ControlIdentifier {
> +	ControlId id;
> +	const char *name;
> +	ValueType type;
> +};
> +
> +class ControlInfo
> +{
> +public:
> +	ControlInfo(ControlId id, Value min = 0, Value max = 0);
> +
> +	ControlId id() const { return ident_->id; }
> +	const char *name() const { return ident_->name; }
> +	ValueType type() const { return ident_->type; }
> +
> +	const Value &min() { return min_; }
> +	const Value &max() { return min_; }

These can be const functions.

> +
> +	std::string toString() const;
> +
> +	bool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }
> +	bool operator==(const ControlId rhs) const { return id() == rhs; }
> +
> +private:
> +	struct ControlIdentifier const *ident_;
> +	Value min_;
> +	Value max_;
> +
> +};
> +
> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CONTROLS_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index eb2211ae1fc3..06e3feebd23d 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_api = files([
>      'buffer.h',
> +    'controls.h',

Not inserted in alphabetical order ;-)

>      'camera.h',
>      'camera_manager.h',
>      'event_dispatcher.h',
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> new file mode 100644
> index 000000000000..b1be46ddb55e
> --- /dev/null
> +++ b/src/libcamera/controls.cpp
> @@ -0,0 +1,160 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * controls.cpp - Control handling
> + */
> +
> +#include <limits.h>
> +#include <sstream>
> +#include <string>
> +
> +#include <libcamera/controls.h>

You should have this as the first include statement in the file.

> +
> +#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 ControlId
> + * Control Identifiers
> + * \var IpaAwbEnable
> + * bool: Enables or disables the AWB
> + * \var ManualExposure
> + * Manually control the exposure time in milli-seconds
> + * \var ManualGain
> + * Controls the value of the manual gain setting
> + */

ManualBrightness missing.

> +
> +/**
> + * \struct ControlIdentifier
> + * Defines a Control with a unique ID, a name, and a type.
> + * \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
> + */
> +
> +/*
> + * Two sets of tables are generated for the static control definitions.
> + *
> + * An enum to declare the ID (in controls.h), and a type to establish its
> + * representation.
> + *
> + * Todo: Automate the generation of both tables from a single input table.
> + * Todo: Consider if this should be a static std::map created at init instead.

I think a static std::map (or maybe unordered_map) is a good idea.

> + */
> +static const struct ControlIdentifier ControlTypes[] = {
> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }
> +
> +	CONTROL_TYPE(IpaAwbEnable,		ValueBool),
> +	CONTROL_TYPE(ManualBrightness,		ValueInteger),
> +	CONTROL_TYPE(ManualExposure,		ValueInteger),
> +	CONTROL_TYPE(ManualGain,		ValueInteger),
> +
> +#undef CONTROL_TYPE
> +};
> +
> +static struct ControlIdentifier const *FindControlType(ControlId id)
> +{
> +	struct ControlIdentifier const *ident;
> +
> +	for (ident = ControlTypes;
> +	     ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];
> +	     ++ident) {
> +		if (ident->id == id)
> +			return ident;
> +	}
> +
> +	LOG(Controls, Fatal) << "Failed to find a ControlType.";
> +
> +	/* Unreachable. */
> +	return nullptr;
> +}
> +
> +/**
> + * \class ControlInfo
> + * \brief Describes the information and capabilities of a Control
> + */
> +
> +/**
> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> + */
> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)
> +	: ident_(FindControlType(id)), min_(min), max_(max)
> +{
> +}
> +
> +/**
> + * \fn ControlInfo::id()
> + * \brief Return the ID of the control information descriptor
> + * \return the ControlId
> + */
> +
> +/**
> + * \fn ControlInfo::name()
> + * \brief Return the string name of the control information descriptor
> + * \return A string name for the Control
> + */
> +
> +/**
> + * \fn ControlInfo::type()
> + * \brief Return the ValueType of the control information descriptor
> + * \return the ControlId
> + */
> +
> +/**
> + * \fn ControlInfo::min()
> + * \brief Reports the minimum value of the control
> + * \return a Value with the minimum setting for the control
> + */
> +
> +/**
> + * \fn ControlInfo::max()
> + * \brief Reports the maximum value of the control
> + * \return a Value with the maximum setting for the control
> + */
> +
> +/**
> + * \brief Provide a string representation of the ControlInfo
> + */
> +std::string ControlInfo::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss << "Control: " << name()
> +	   << " : Min(" << min_ << ") Max(" << max_ << ")";
> +
> +	return ss.str();
> +}
> +
> +/**
> + * \fn ControlInfo::operator==(const ControlInfo &rhs) const
> + * \brief Establish equivalence of ControlInfo. Only the IDs are considered.
> + */
> +
> +/**
> + * \fn ControlInfo::operator==(const ControlId rhs) const
> + * \brief Establish equivalence of ControlInfo, against a ControlID.
> + */
> +
> +/**
> + * \brief Provide a string stream representation of the ControlInfo \a info to
> + * the \a stream.
> + */
> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
> +{
> +	return stream << info.toString();
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 8e68373118df..e2c07d79bfb5 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',
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

Thank you for the patch.

On Fri, Jun 21, 2019 at 05:13:55PM +0100, Kieran Bingham wrote:
> ControlIdentifiers declare the types of a control, and map their names,
> and the ControlInfo class allows runtime state to be represented.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/controls.h  |  58 ++++++++++++
>  include/libcamera/meson.build |   1 +
>  src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
>  src/libcamera/meson.build     |   1 +
>  4 files changed, 220 insertions(+)
>  create mode 100644 include/libcamera/controls.h
>  create mode 100644 src/libcamera/controls.cpp
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> new file mode 100644
> index 000000000000..95198d41c4cf
> --- /dev/null
> +++ b/include/libcamera/controls.h
> @@ -0,0 +1,58 @@
> +/* 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 <libcamera/value.h>
> +
> +namespace libcamera {
> +
> +enum ControlId : uint32_t {
> +	/* IPA Controls */
> +	IpaAwbEnable,
> +
> +	/* Manual Controls */
> +	ManualBrightness,
> +	ManualExposure,
> +	ManualGain,

There will be lots of nitpicking about the names, we'll revisit this
later. For the time being I would still remove the IPA prefix and avoid
mentioning IPA at all, as I think it should remain an internal concept.

> +};
> +
> +struct ControlIdentifier {
> +	ControlId id;
> +	const char *name;
> +	ValueType type;
> +};
> +
> +class ControlInfo
> +{
> +public:
> +	ControlInfo(ControlId id, Value min = 0, Value max = 0);

Should you define the constructor as explicit as it's callable with a
single argument ? Or does the compiler refuse that as there's more than
one argument, even if all but the first have default values ?

> +
> +	ControlId id() const { return ident_->id; }
> +	const char *name() const { return ident_->name; }
> +	ValueType type() const { return ident_->type; }
> +
> +	const Value &min() { return min_; }
> +	const Value &max() { return min_; }
> +
> +	std::string toString() const;
> +
> +	bool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }
> +	bool operator==(const ControlId rhs) const { return id() == rhs; }

I wonder if these methods should be defined as non-members, as a bool
operator==(const ControlId lhs, const ControlInfo &rhs) should also be
defined for symmetry. Or maybe we should drop the second method, and
always write info.id() == id explicitly ? It's not much longer, and
should be more explicit.

You should also define operator!= as !(lhs == rhs).

> +
> +private:
> +	struct ControlIdentifier const *ident_;
> +	Value min_;
> +	Value max_;
> +
> +};
> +
> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);

Don't you need to #include <ostream> ?

> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_CONTROLS_H__ */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index eb2211ae1fc3..06e3feebd23d 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -1,5 +1,6 @@
>  libcamera_api = files([
>      'buffer.h',
> +    'controls.h',
>      'camera.h',
>      'camera_manager.h',
>      'event_dispatcher.h',
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> new file mode 100644
> index 000000000000..b1be46ddb55e
> --- /dev/null
> +++ b/src/libcamera/controls.cpp
> @@ -0,0 +1,160 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * controls.cpp - Control handling
> + */
> +
> +#include <limits.h>
> +#include <sstream>
> +#include <string>
> +
> +#include <libcamera/controls.h>

Please include this header first, for the same reason as in patch 1/9.

> +
> +#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 ControlId
> + * Control Identifiers
> + * \var IpaAwbEnable
> + * bool: Enables or disables the AWB
> + * \var ManualExposure
> + * Manually control the exposure time in milli-seconds
> + * \var ManualGain
> + * Controls the value of the manual gain setting
> + */

This is an area where I want libcamera to have amazing documentation.
Not only will each control need to be documented in details, but I also
want interactions between controls to be documented. In the meantime,
could you already try to format the documentation block appropriately ?
One block per control would be ideal.

> +
> +/**
> + * \struct ControlIdentifier
> + * Defines a Control with a unique ID, a name, and a type.

\brief ?

I think a more elaborate description would be useful, as so far I'm not
sure why you have split control information in two parts (although I
suspect the reason, but an explicit description would be better :-)).

> + * \var ControlIdentifier::id

Is the ControlIdentifier:: prefix needed ?

> + * 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
> + */
> +
> +/*
> + * Two sets of tables are generated for the static control definitions.
> + *

s/.\n \*\n \* An/:\r * an/

Is that readable ? :-)

> + * An enum to declare the ID (in controls.h), and a type to establish its
> + * representation.

The enum is actually not a table, it doesn't generate data as such.

> + *
> + * Todo: Automate the generation of both tables from a single input table.

s/Todo: /\\todo /

Would it be possible to generate them both from a documentation file ?

> + * Todo: Consider if this should be a static std::map created at init instead.

Any reason why it shouldn't be ? :-) Lookup would be more efficient.

> + */
> +static const struct ControlIdentifier ControlTypes[] = {
> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }
> +
> +	CONTROL_TYPE(IpaAwbEnable,		ValueBool),
> +	CONTROL_TYPE(ManualBrightness,		ValueInteger),
> +	CONTROL_TYPE(ManualExposure,		ValueInteger),
> +	CONTROL_TYPE(ManualGain,		ValueInteger),
> +
> +#undef CONTROL_TYPE
> +};
> +
> +static struct ControlIdentifier const *FindControlType(ControlId id)

This is a function and should thus start with a lowercase letter.

> +{
> +	struct ControlIdentifier const *ident;
> +
> +	for (ident = ControlTypes;
> +	     ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];
> +	     ++ident) {
> +		if (ident->id == id)
> +			return ident;
> +	}
> +
> +	LOG(Controls, Fatal) << "Failed to find a ControlType.";
> +
> +	/* Unreachable. */
> +	return nullptr;
> +}
> +
> +/**
> + * \class ControlInfo
> + * \brief Describes the information and capabilities of a Control

s/Describes/Describe/

Again a bit more documentation would help.

> + */
> +
> +/**
> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> + */
> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)
> +	: ident_(FindControlType(id)), min_(min), max_(max)
> +{
> +}
> +
> +/**
> + * \fn ControlInfo::id()
> + * \brief Return the ID of the control information descriptor

s/Return/Retrieve/

This is a bit confusing as nothing documents what a "control information
descriptor" is. I think the documentation will need to be revisited.

> + * \return the ControlId

The control ID

> + */
> +
> +/**
> + * \fn ControlInfo::name()
> + * \brief Return the string name of the control information descriptor

s/Return/Retrieve/

and below too.

> + * \return A string name for the Control
> + */
> +
> +/**
> + * \fn ControlInfo::type()
> + * \brief Return the ValueType of the control information descriptor
> + * \return the ControlId

The control type

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

s/a/A/

How does that apply to non-integer controls ?

> + */
> +
> +/**
> + * \fn ControlInfo::max()
> + * \brief Reports the maximum value of the control
> + * \return a Value with the maximum setting for the control
> + */
> +
> +/**
> + * \brief Provide a string representation of the ControlInfo
> + */
> +std::string ControlInfo::toString() const
> +{
> +	std::stringstream ss;
> +
> +	ss << "Control: " << name()
> +	   << " : Min(" << min_ << ") Max(" << max_ << ")";

I still need to look at how you use this, but in general it's best to
shorten the representations as much as possible. I may remove the
"Control:" prefix, maybe with

	ss << name() << "[" << min_ << "," << max_ << "]";

This would print

	ManualExposure[1,1000]

We could replace the comma with ..

	ManualExposure[1..1000]

> +
> +	return ss.str();
> +}
> +
> +/**
> + * \fn ControlInfo::operator==(const ControlInfo &rhs) const
> + * \brief Establish equivalence of ControlInfo. Only the IDs are considered.
> + */
> +
> +/**
> + * \fn ControlInfo::operator==(const ControlId rhs) const
> + * \brief Establish equivalence of ControlInfo, against a ControlID.
> + */
> +
> +/**
> + * \brief Provide a string stream representation of the ControlInfo \a info to
> + * the \a stream.
> + */
> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
> +{
> +	return stream << info.toString();
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 8e68373118df..e2c07d79bfb5 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',

On Sun, Jun 23, 2019 at 05:22:03PM +0200, Niklas Söderlund wrote:
> On 2019-06-21 17:13:55 +0100, Kieran Bingham wrote:
> > ControlIdentifiers declare the types of a control, and map their names,
> > and the ControlInfo class allows runtime state to be represented.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/controls.h  |  58 ++++++++++++
> >  include/libcamera/meson.build |   1 +
> >  src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
> >  src/libcamera/meson.build     |   1 +
> >  4 files changed, 220 insertions(+)
> >  create mode 100644 include/libcamera/controls.h
> >  create mode 100644 src/libcamera/controls.cpp
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > new file mode 100644
> > index 000000000000..95198d41c4cf
> > --- /dev/null
> > +++ b/include/libcamera/controls.h
> > @@ -0,0 +1,58 @@
> > +/* 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 <libcamera/value.h>
> > +
> > +namespace libcamera {
> > +
> > +enum ControlId : uint32_t {
> > +	/* IPA Controls */
> > +	IpaAwbEnable,
> > +
> > +	/* Manual Controls */
> > +	ManualBrightness,
> > +	ManualExposure,
> > +	ManualGain,
> > +};
> > +
> > +struct ControlIdentifier {
> > +	ControlId id;
> > +	const char *name;
> > +	ValueType type;
> > +};
> > +
> > +class ControlInfo
> > +{
> > +public:
> > +	ControlInfo(ControlId id, Value min = 0, Value max = 0);
> > +
> > +	ControlId id() const { return ident_->id; }
> > +	const char *name() const { return ident_->name; }
> > +	ValueType type() const { return ident_->type; }
> > +
> > +	const Value &min() { return min_; }
> > +	const Value &max() { return min_; }
> 
> These can be const functions.

And the second one should return max_.

> > +
> > +	std::string toString() const;
> > +
> > +	bool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }
> > +	bool operator==(const ControlId rhs) const { return id() == rhs; }
> > +
> > +private:
> > +	struct ControlIdentifier const *ident_;
> > +	Value min_;
> > +	Value max_;
> > +
> > +};
> > +
> > +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CONTROLS_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index eb2211ae1fc3..06e3feebd23d 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -1,5 +1,6 @@
> >  libcamera_api = files([
> >      'buffer.h',
> > +    'controls.h',
> 
> Not inserted in alphabetical order ;-)

How did *I* miss that one ? :-)

> >      'camera.h',
> >      'camera_manager.h',
> >      'event_dispatcher.h',
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > new file mode 100644
> > index 000000000000..b1be46ddb55e
> > --- /dev/null
> > +++ b/src/libcamera/controls.cpp
> > @@ -0,0 +1,160 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.cpp - Control handling
> > + */
> > +
> > +#include <limits.h>
> > +#include <sstream>
> > +#include <string>
> > +
> > +#include <libcamera/controls.h>
> 
> You should have this as the first include statement in the file.
> 
> > +
> > +#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 ControlId
> > + * Control Identifiers
> > + * \var IpaAwbEnable
> > + * bool: Enables or disables the AWB
> > + * \var ManualExposure
> > + * Manually control the exposure time in milli-seconds
> > + * \var ManualGain
> > + * Controls the value of the manual gain setting
> > + */
> 
> ManualBrightness missing.
> 
> > +
> > +/**
> > + * \struct ControlIdentifier
> > + * Defines a Control with a unique ID, a name, and a type.
> > + * \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
> > + */
> > +
> > +/*
> > + * Two sets of tables are generated for the static control definitions.
> > + *
> > + * An enum to declare the ID (in controls.h), and a type to establish its
> > + * representation.
> > + *
> > + * Todo: Automate the generation of both tables from a single input table.
> > + * Todo: Consider if this should be a static std::map created at init instead.
> 
> I think a static std::map (or maybe unordered_map) is a good idea.
> 
> > + */
> > +static const struct ControlIdentifier ControlTypes[] = {
> > +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }
> > +
> > +	CONTROL_TYPE(IpaAwbEnable,		ValueBool),
> > +	CONTROL_TYPE(ManualBrightness,		ValueInteger),
> > +	CONTROL_TYPE(ManualExposure,		ValueInteger),
> > +	CONTROL_TYPE(ManualGain,		ValueInteger),
> > +
> > +#undef CONTROL_TYPE
> > +};
> > +
> > +static struct ControlIdentifier const *FindControlType(ControlId id)
> > +{
> > +	struct ControlIdentifier const *ident;
> > +
> > +	for (ident = ControlTypes;
> > +	     ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];
> > +	     ++ident) {
> > +		if (ident->id == id)
> > +			return ident;
> > +	}
> > +
> > +	LOG(Controls, Fatal) << "Failed to find a ControlType.";
> > +
> > +	/* Unreachable. */
> > +	return nullptr;
> > +}
> > +
> > +/**
> > + * \class ControlInfo
> > + * \brief Describes the information and capabilities of a Control
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> > + */
> > +ControlInfo::ControlInfo(ControlId id, Value min, Value max)
> > +	: ident_(FindControlType(id)), min_(min), max_(max)
> > +{
> > +}
> > +
> > +/**
> > + * \fn ControlInfo::id()
> > + * \brief Return the ID of the control information descriptor
> > + * \return the ControlId
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::name()
> > + * \brief Return the string name of the control information descriptor
> > + * \return A string name for the Control
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::type()
> > + * \brief Return the ValueType of the control information descriptor
> > + * \return the ControlId
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::min()
> > + * \brief Reports the minimum value of the control
> > + * \return a Value with the minimum setting for the control
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::max()
> > + * \brief Reports the maximum value of the control
> > + * \return a Value with the maximum setting for the control
> > + */
> > +
> > +/**
> > + * \brief Provide a string representation of the ControlInfo
> > + */
> > +std::string ControlInfo::toString() const
> > +{
> > +	std::stringstream ss;
> > +
> > +	ss << "Control: " << name()
> > +	   << " : Min(" << min_ << ") Max(" << max_ << ")";
> > +
> > +	return ss.str();
> > +}
> > +
> > +/**
> > + * \fn ControlInfo::operator==(const ControlInfo &rhs) const
> > + * \brief Establish equivalence of ControlInfo. Only the IDs are considered.
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::operator==(const ControlId rhs) const
> > + * \brief Establish equivalence of ControlInfo, against a ControlID.
> > + */
> > +
> > +/**
> > + * \brief Provide a string stream representation of the ControlInfo \a info to
> > + * the \a stream.
> > + */
> > +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
> > +{
> > +	return stream << info.toString();
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 8e68373118df..e2c07d79bfb5 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',

Hi Laurent,

On 23/06/2019 22:52, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Jun 21, 2019 at 05:13:55PM +0100, Kieran Bingham wrote:
>> ControlIdentifiers declare the types of a control, and map their names,
>> and the ControlInfo class allows runtime state to be represented.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  include/libcamera/controls.h  |  58 ++++++++++++
>>  include/libcamera/meson.build |   1 +
>>  src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
>>  src/libcamera/meson.build     |   1 +
>>  4 files changed, 220 insertions(+)
>>  create mode 100644 include/libcamera/controls.h
>>  create mode 100644 src/libcamera/controls.cpp
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> new file mode 100644
>> index 000000000000..95198d41c4cf
>> --- /dev/null
>> +++ b/include/libcamera/controls.h
>> @@ -0,0 +1,58 @@
>> +/* 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 <libcamera/value.h>
>> +
>> +namespace libcamera {
>> +
>> +enum ControlId : uint32_t {
>> +	/* IPA Controls */
>> +	IpaAwbEnable,
>> +
>> +	/* Manual Controls */
>> +	ManualBrightness,
>> +	ManualExposure,
>> +	ManualGain,
> 
> There will be lots of nitpicking about the names, we'll revisit this
> later. For the time being I would still remove the IPA prefix and avoid
> mentioning IPA at all, as I think it should remain an internal concept.


These are essentially dummy controls at the moment. I picked a few
arbitrary initial control names. We can change them as you like. Are you
still planning to provide a list of expected controls that we should
support?

But at the moment I expect the controls that are supported to grow
dynamically as we develop them. I don't think we're going to get them
all in, in one hit.

I'll drop the Ipa anyway.


> 
>> +};
>> +
>> +struct ControlIdentifier {
>> +	ControlId id;
>> +	const char *name;
>> +	ValueType type;
>> +};
>> +
>> +class ControlInfo
>> +{
>> +public:
>> +	ControlInfo(ControlId id, Value min = 0, Value max = 0);
> 
> Should you define the constructor as explicit as it's callable with a
> single argument ? Or does the compiler refuse that as there's more than
> one argument, even if all but the first have default values ?


That breaks things currently. I'll look into this later, depending on
what happens with the changes to the ControlList class and map construction.



> 
>> +
>> +	ControlId id() const { return ident_->id; }
>> +	const char *name() const { return ident_->name; }
>> +	ValueType type() const { return ident_->type; }
>> +
>> +	const Value &min() { return min_; }
>> +	const Value &max() { return min_; }
>> +
>> +	std::string toString() const;
>> +
>> +	bool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }
>> +	bool operator==(const ControlId rhs) const { return id() == rhs; }
> 
> I wonder if these methods should be defined as non-members, as a bool
> operator==(const ControlId lhs, const ControlInfo &rhs) should also be
> defined for symmetry. Or maybe we should drop the second method, and
> always write info.id() == id explicitly ? It's not much longer, and
> should be more explicit.

The comparisons are only really for the STL container (ControlList). But
as long as they are implemented. I don't think it matters if they are in
or out of the class.

Perhaps simpler to inline them in the class - but that could still be
done outside.


> You should also define operator!= as !(lhs == rhs).

I don't think this is used by the std::unordered_map ... And I'm not
sure if I expect users to be doing comparisons ... still it's an easy
implementation so it perhaps doesn't hurt.



>> +
>> +private:
>> +	struct ControlIdentifier const *ident_;
>> +	Value min_;
>> +	Value max_;
>> +
>> +};
>> +
>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
> 
> Don't you need to #include <ostream> ?

Sure :) - Added.

> 
>> +
>> +} /* namespace libcamera */
>> +
>> +#endif /* __LIBCAMERA_CONTROLS_H__ */
>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>> index eb2211ae1fc3..06e3feebd23d 100644
>> --- a/include/libcamera/meson.build
>> +++ b/include/libcamera/meson.build
>> @@ -1,5 +1,6 @@
>>  libcamera_api = files([
>>      'buffer.h',
>> +    'controls.h',
>>      'camera.h',
>>      'camera_manager.h',
>>      'event_dispatcher.h',
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> new file mode 100644
>> index 000000000000..b1be46ddb55e
>> --- /dev/null
>> +++ b/src/libcamera/controls.cpp
>> @@ -0,0 +1,160 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2019, Google Inc.
>> + *
>> + * controls.cpp - Control handling
>> + */
>> +
>> +#include <limits.h>
>> +#include <sstream>
>> +#include <string>
>> +
>> +#include <libcamera/controls.h>
> 
> Please include this header first, for the same reason as in patch 1/9.

Fixed.

> 
>> +
>> +#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 ControlId
>> + * Control Identifiers
>> + * \var IpaAwbEnable
>> + * bool: Enables or disables the AWB
>> + * \var ManualExposure
>> + * Manually control the exposure time in milli-seconds
>> + * \var ManualGain
>> + * Controls the value of the manual gain setting
>> + */
> 
> This is an area where I want libcamera to have amazing documentation.
> Not only will each control need to be documented in details, but I also
> want interactions between controls to be documented. In the meantime,
> could you already try to format the documentation block appropriately ?
> One block per control would be ideal.

Ok - but I am not by any means considering these controls as 'final'
yet. These are just examples to get the framework in.

Each of those is now given their own section.

If we can generate all the

>> +
>> +/**
>> + * \struct ControlIdentifier
>> + * Defines a Control with a unique ID, a name, and a type.
> 
> \brief ?
> 
> I think a more elaborate description would be useful, as so far I'm not
> sure why you have split control information in two parts (although I
> suspect the reason, but an explicit description would be better :-)).

I've updated this a bit - but it still needs more refinement as the
implementation grows. And it depends on how the controls really get
constructed.


> 
>> + * \var ControlIdentifier::id
> 
> Is the ControlIdentifier:: prefix needed ?


It seems that way.

Without the prefix - doxygen doesn't seem to know the following var's
are part of the \struct ControlIdentifier...

> 
>> + * 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
>> + */
>> +
>> +/*
>> + * Two sets of tables are generated for the static control definitions.
>> + *
> 
> s/.\n \*\n \* An/:\r * an/
> 
> Is that readable ? :-)

It's a good job I read regex. ... :D (for a *very* limited subset of regex)


>> + * An enum to declare the ID (in controls.h), and a type to establish its
>> + * representation.
> 
> The enum is actually not a table, it doesn't generate data as such.

My point is - I don't want to have to list an enum once to create the
enum, and then a second table to create definitions. I'd like to find a
way such that creating a control is just an addition to a single table.


>> + *
>> + * Todo: Automate the generation of both tables from a single input table.
> 
> s/Todo: /\\todo /
> 
> Would it be possible to generate them both from a documentation file ?

^ Equally - there's a third place that would have to be updated. (the docs).

There's definitely scope here that things should be autogenerated from a
single source.


> 
>> + * Todo: Consider if this should be a static std::map created at init instead.
> 
> Any reason why it shouldn't be ? :-) Lookup would be more efficient.

Because \todo - and trying to figure out the details...


This is the point where RFC means 'Help out with suggestions on todo's :-D


> 
>> + */
>> +static const struct ControlIdentifier ControlTypes[] = {
>> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }
>> +
>> +	CONTROL_TYPE(IpaAwbEnable,		ValueBool),
>> +	CONTROL_TYPE(ManualBrightness,		ValueInteger),
>> +	CONTROL_TYPE(ManualExposure,		ValueInteger),
>> +	CONTROL_TYPE(ManualGain,		ValueInteger),
>> +
>> +#undef CONTROL_TYPE
>> +};
>> +
>> +static struct ControlIdentifier const *FindControlType(ControlId id)
> 
> This is a function and should thus start with a lowercase letter.

Ack.

> 
>> +{
>> +	struct ControlIdentifier const *ident;
>> +
>> +	for (ident = ControlTypes;
>> +	     ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];
>> +	     ++ident) {
>> +		if (ident->id == id)
>> +			return ident;
>> +	}
>> +
>> +	LOG(Controls, Fatal) << "Failed to find a ControlType.";
>> +
>> +	/* Unreachable. */
>> +	return nullptr;
>> +}
>> +
>> +/**
>> + * \class ControlInfo
>> + * \brief Describes the information and capabilities of a Control
> 
> s/Describes/Describe/
> 
> Again a bit more documentation would help.
> 
>> + */
>> +
>> +/**
>> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
>> + */
>> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)
>> +	: ident_(FindControlType(id)), min_(min), max_(max)
>> +{
>> +}
>> +
>> +/**
>> + * \fn ControlInfo::id()
>> + * \brief Return the ID of the control information descriptor
> 
> s/Return/Retrieve/
> 
> This is a bit confusing as nothing documents what a "control information
> descriptor" is. I think the documentation will need to be revisited.


>> + * \return the ControlId
> 
> The control ID
> 
>> + */
>> +
>> +/**
>> + * \fn ControlInfo::name()
>> + * \brief Return the string name of the control information descriptor
> 
> s/Return/Retrieve/
> 
> and below too.
> 
>> + * \return A string name for the Control
>> + */
>> +
>> +/**
>> + * \fn ControlInfo::type()
>> + * \brief Return the ValueType of the control information descriptor
>> + * \return the ControlId
> 
> The control type
> 
>> + */
>> +
>> +/**
>> + * \fn ControlInfo::min()
>> + * \brief Reports the minimum value of the control
>> + * \return a Value with the minimum setting for the control
> 
> s/a/A/
> 
> How does that apply to non-integer controls ?

I have no idea yet. "RFC" : What are your thoughts?

min/max are not even yet populated with anything.

Perhaps it only applies to integer controls.

> 
>> + */
>> +
>> +/**
>> + * \fn ControlInfo::max()
>> + * \brief Reports the maximum value of the control
>> + * \return a Value with the maximum setting for the control
>> + */
>> +
>> +/**
>> + * \brief Provide a string representation of the ControlInfo
>> + */
>> +std::string ControlInfo::toString() const
>> +{
>> +	std::stringstream ss;
>> +
>> +	ss << "Control: " << name()
>> +	   << " : Min(" << min_ << ") Max(" << max_ << ")";
> 
> I still need to look at how you use this, but in general it's best to
> shorten the representations as much as possible. I may remove the
> "Control:" prefix, maybe with
> 
> 	ss << name() << "[" << min_ << "," << max_ << "]";
> 
> This would print
> 
> 	ManualExposure[1,1000]
> 
> We could replace the comma with ..
> 
> 	ManualExposure[1..1000]

I like that !

Done.


> 
>> +
>> +	return ss.str();
>> +}
>> +
>> +/**
>> + * \fn ControlInfo::operator==(const ControlInfo &rhs) const
>> + * \brief Establish equivalence of ControlInfo. Only the IDs are considered.
>> + */
>> +
>> +/**
>> + * \fn ControlInfo::operator==(const ControlId rhs) const
>> + * \brief Establish equivalence of ControlInfo, against a ControlID.
>> + */
>> +
>> +/**
>> + * \brief Provide a string stream representation of the ControlInfo \a info to
>> + * the \a stream.
>> + */
>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
>> +{
>> +	return stream << info.toString();
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 8e68373118df..e2c07d79bfb5 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',
>

Hi Laurent, Niklas,

On 23/06/2019 22:54, Laurent Pinchart wrote:
> On Sun, Jun 23, 2019 at 05:22:03PM +0200, Niklas Söderlund wrote:
>> On 2019-06-21 17:13:55 +0100, Kieran Bingham wrote:
>>> ControlIdentifiers declare the types of a control, and map their names,
>>> and the ControlInfo class allows runtime state to be represented.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>  include/libcamera/controls.h  |  58 ++++++++++++
>>>  include/libcamera/meson.build |   1 +
>>>  src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
>>>  src/libcamera/meson.build     |   1 +
>>>  4 files changed, 220 insertions(+)
>>>  create mode 100644 include/libcamera/controls.h
>>>  create mode 100644 src/libcamera/controls.cpp
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> new file mode 100644
>>> index 000000000000..95198d41c4cf
>>> --- /dev/null
>>> +++ b/include/libcamera/controls.h
>>> @@ -0,0 +1,58 @@
>>> +/* 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 <libcamera/value.h>
>>> +
>>> +namespace libcamera {
>>> +
>>> +enum ControlId : uint32_t {
>>> +	/* IPA Controls */
>>> +	IpaAwbEnable,
>>> +
>>> +	/* Manual Controls */
>>> +	ManualBrightness,
>>> +	ManualExposure,
>>> +	ManualGain,
>>> +};
>>> +
>>> +struct ControlIdentifier {
>>> +	ControlId id;
>>> +	const char *name;
>>> +	ValueType type;
>>> +};
>>> +
>>> +class ControlInfo
>>> +{
>>> +public:
>>> +	ControlInfo(ControlId id, Value min = 0, Value max = 0);
>>> +
>>> +	ControlId id() const { return ident_->id; }
>>> +	const char *name() const { return ident_->name; }
>>> +	ValueType type() const { return ident_->type; }
>>> +
>>> +	const Value &min() { return min_; }
>>> +	const Value &max() { return min_; }
>>
>> These can be const functions.

Done.

> 
> And the second one should return max_.


Done ... eeep  :-(


> 
>>> +
>>> +	std::string toString() const;
>>> +
>>> +	bool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }
>>> +	bool operator==(const ControlId rhs) const { return id() == rhs; }
>>> +
>>> +private:
>>> +	struct ControlIdentifier const *ident_;
>>> +	Value min_;
>>> +	Value max_;
>>> +
>>> +};
>>> +
>>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
>>> +
>>> +} /* namespace libcamera */
>>> +
>>> +#endif /* __LIBCAMERA_CONTROLS_H__ */
>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
>>> index eb2211ae1fc3..06e3feebd23d 100644
>>> --- a/include/libcamera/meson.build
>>> +++ b/include/libcamera/meson.build
>>> @@ -1,5 +1,6 @@
>>>  libcamera_api = files([
>>>      'buffer.h',
>>> +    'controls.h',
>>
>> Not inserted in alphabetical order ;-)
> 
> How did *I* miss that one ? :-)

Now sorted correctly.

> 
>>>      'camera.h',
>>>      'camera_manager.h',
>>>      'event_dispatcher.h',
>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>>> new file mode 100644
>>> index 000000000000..b1be46ddb55e
>>> --- /dev/null
>>> +++ b/src/libcamera/controls.cpp
>>> @@ -0,0 +1,160 @@
>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>> +/*
>>> + * Copyright (C) 2019, Google Inc.
>>> + *
>>> + * controls.cpp - Control handling
>>> + */
>>> +
>>> +#include <limits.h>
>>> +#include <sstream>
>>> +#include <string>
>>> +
>>> +#include <libcamera/controls.h>
>>
>> You should have this as the first include statement in the file.

Fixed.

>>
>>> +
>>> +#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 ControlId
>>> + * Control Identifiers
>>> + * \var IpaAwbEnable
>>> + * bool: Enables or disables the AWB
>>> + * \var ManualExposure
>>> + * Manually control the exposure time in milli-seconds
>>> + * \var ManualGain
>>> + * Controls the value of the manual gain setting
>>> + */
>>
>> ManualBrightness missing.

Added.

>>
>>> +
>>> +/**
>>> + * \struct ControlIdentifier
>>> + * Defines a Control with a unique ID, a name, and a type.
>>> + * \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
>>> + */
>>> +
>>> +/*
>>> + * Two sets of tables are generated for the static control definitions.
>>> + *
>>> + * An enum to declare the ID (in controls.h), and a type to establish its
>>> + * representation.
>>> + *
>>> + * Todo: Automate the generation of both tables from a single input table.
>>> + * Todo: Consider if this should be a static std::map created at init instead.
>>
>> I think a static std::map (or maybe unordered_map) is a good idea.

Yes, but I think I still need to work out how to automatically generate
the tables from a single data source, and correctly populate them at
camera instantiation.


>>
>>> + */
>>> +static const struct ControlIdentifier ControlTypes[] = {
>>> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }
>>> +
>>> +	CONTROL_TYPE(IpaAwbEnable,		ValueBool),
>>> +	CONTROL_TYPE(ManualBrightness,		ValueInteger),
>>> +	CONTROL_TYPE(ManualExposure,		ValueInteger),
>>> +	CONTROL_TYPE(ManualGain,		ValueInteger),
>>> +
>>> +#undef CONTROL_TYPE
>>> +};
>>> +
>>> +static struct ControlIdentifier const *FindControlType(ControlId id)
>>> +{
>>> +	struct ControlIdentifier const *ident;
>>> +
>>> +	for (ident = ControlTypes;
>>> +	     ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];
>>> +	     ++ident) {
>>> +		if (ident->id == id)
>>> +			return ident;
>>> +	}
>>> +
>>> +	LOG(Controls, Fatal) << "Failed to find a ControlType.";
>>> +
>>> +	/* Unreachable. */
>>> +	return nullptr;
>>> +}
>>> +
>>> +/**
>>> + * \class ControlInfo
>>> + * \brief Describes the information and capabilities of a Control
>>> + */
>>> +
>>> +/**
>>> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
>>> + */
>>> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)
>>> +	: ident_(FindControlType(id)), min_(min), max_(max)
>>> +{
>>> +}
>>> +
>>> +/**
>>> + * \fn ControlInfo::id()
>>> + * \brief Return the ID of the control information descriptor
>>> + * \return the ControlId
>>> + */
>>> +
>>> +/**
>>> + * \fn ControlInfo::name()
>>> + * \brief Return the string name of the control information descriptor
>>> + * \return A string name for the Control
>>> + */
>>> +
>>> +/**
>>> + * \fn ControlInfo::type()
>>> + * \brief Return the ValueType of the control information descriptor
>>> + * \return the ControlId
>>> + */
>>> +
>>> +/**
>>> + * \fn ControlInfo::min()
>>> + * \brief Reports the minimum value of the control
>>> + * \return a Value with the minimum setting for the control
>>> + */
>>> +
>>> +/**
>>> + * \fn ControlInfo::max()
>>> + * \brief Reports the maximum value of the control
>>> + * \return a Value with the maximum setting for the control
>>> + */
>>> +
>>> +/**
>>> + * \brief Provide a string representation of the ControlInfo
>>> + */
>>> +std::string ControlInfo::toString() const
>>> +{
>>> +	std::stringstream ss;
>>> +
>>> +	ss << "Control: " << name()
>>> +	   << " : Min(" << min_ << ") Max(" << max_ << ")";
>>> +
>>> +	return ss.str();
>>> +}
>>> +
>>> +/**
>>> + * \fn ControlInfo::operator==(const ControlInfo &rhs) const
>>> + * \brief Establish equivalence of ControlInfo. Only the IDs are considered.
>>> + */
>>> +
>>> +/**
>>> + * \fn ControlInfo::operator==(const ControlId rhs) const
>>> + * \brief Establish equivalence of ControlInfo, against a ControlID.
>>> + */
>>> +
>>> +/**
>>> + * \brief Provide a string stream representation of the ControlInfo \a info to
>>> + * the \a stream.
>>> + */
>>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
>>> +{
>>> +	return stream << info.toString();
>>> +}
>>> +
>>> +} /* namespace libcamera */
>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>> index 8e68373118df..e2c07d79bfb5 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',
>

Hi Kieran,

On Mon, Jun 24, 2019 at 05:49:35PM +0100, Kieran Bingham wrote:
> On 23/06/2019 22:52, Laurent Pinchart wrote:
> > On Fri, Jun 21, 2019 at 05:13:55PM +0100, Kieran Bingham wrote:
> >> ControlIdentifiers declare the types of a control, and map their names,
> >> and the ControlInfo class allows runtime state to be represented.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  include/libcamera/controls.h  |  58 ++++++++++++
> >>  include/libcamera/meson.build |   1 +
> >>  src/libcamera/controls.cpp    | 160 ++++++++++++++++++++++++++++++++++
> >>  src/libcamera/meson.build     |   1 +
> >>  4 files changed, 220 insertions(+)
> >>  create mode 100644 include/libcamera/controls.h
> >>  create mode 100644 src/libcamera/controls.cpp
> >>
> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >> new file mode 100644
> >> index 000000000000..95198d41c4cf
> >> --- /dev/null
> >> +++ b/include/libcamera/controls.h
> >> @@ -0,0 +1,58 @@
> >> +/* 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 <libcamera/value.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +enum ControlId : uint32_t {
> >> +	/* IPA Controls */
> >> +	IpaAwbEnable,
> >> +
> >> +	/* Manual Controls */
> >> +	ManualBrightness,
> >> +	ManualExposure,
> >> +	ManualGain,
> > 
> > There will be lots of nitpicking about the names, we'll revisit this
> > later. For the time being I would still remove the IPA prefix and avoid
> > mentioning IPA at all, as I think it should remain an internal concept.
> 
> These are essentially dummy controls at the moment. I picked a few
> arbitrary initial control names. We can change them as you like. Are you
> still planning to provide a list of expected controls that we should
> support?

The full list will likely be discussed during our next code camp. For
the initial list required to develop the IPA API, I'm sorry about not
sending a formal e-mail yet. I think we need

AutoExposure (bool)
ManualExpose (int)
ManualGain (int)

The first control may turn into an exposure mode control with more than
two values.

> But at the moment I expect the controls that are supported to grow
> dynamically as we develop them. I don't think we're going to get them
> all in, in one hit.
> 
> I'll drop the Ipa anyway.

Could you also split it into AutoExposure and AutoWhiteBalance, in order
to already support IPA development ?

> >> +};
> >> +
> >> +struct ControlIdentifier {
> >> +	ControlId id;
> >> +	const char *name;
> >> +	ValueType type;
> >> +};
> >> +
> >> +class ControlInfo
> >> +{
> >> +public:
> >> +	ControlInfo(ControlId id, Value min = 0, Value max = 0);
> > 
> > Should you define the constructor as explicit as it's callable with a
> > single argument ? Or does the compiler refuse that as there's more than
> > one argument, even if all but the first have default values ?
> 
> That breaks things currently. I'll look into this later, depending on
> what happens with the changes to the ControlList class and map construction.

I think it's a sign of an issue ;-)

> >> +
> >> +	ControlId id() const { return ident_->id; }
> >> +	const char *name() const { return ident_->name; }
> >> +	ValueType type() const { return ident_->type; }
> >> +
> >> +	const Value &min() { return min_; }
> >> +	const Value &max() { return min_; }
> >> +
> >> +	std::string toString() const;
> >> +
> >> +	bool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }
> >> +	bool operator==(const ControlId rhs) const { return id() == rhs; }
> > 
> > I wonder if these methods should be defined as non-members, as a bool
> > operator==(const ControlId lhs, const ControlInfo &rhs) should also be
> > defined for symmetry. Or maybe we should drop the second method, and
> > always write info.id() == id explicitly ? It's not much longer, and
> > should be more explicit.
> 
> The comparisons are only really for the STL container (ControlList). But
> as long as they are implemented. I don't think it matters if they are in
> or out of the class.
> 
> Perhaps simpler to inline them in the class - but that could still be
> done outside.
> 
> > You should also define operator!= as !(lhs == rhs).
> 
> I don't think this is used by the std::unordered_map ... And I'm not
> sure if I expect users to be doing comparisons ... still it's an easy
> implementation so it perhaps doesn't hurt.

I think it's a good practice to define != when == is defined, otherwise
things can become confusing.

> >> +
> >> +private:
> >> +	struct ControlIdentifier const *ident_;
> >> +	Value min_;
> >> +	Value max_;
> >> +
> >> +};
> >> +
> >> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);
> > 
> > Don't you need to #include <ostream> ?
> 
> Sure :) - Added.
> 
> >> +
> >> +} /* namespace libcamera */
> >> +
> >> +#endif /* __LIBCAMERA_CONTROLS_H__ */
> >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> >> index eb2211ae1fc3..06e3feebd23d 100644
> >> --- a/include/libcamera/meson.build
> >> +++ b/include/libcamera/meson.build
> >> @@ -1,5 +1,6 @@
> >>  libcamera_api = files([
> >>      'buffer.h',
> >> +    'controls.h',
> >>      'camera.h',
> >>      'camera_manager.h',
> >>      'event_dispatcher.h',
> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> >> new file mode 100644
> >> index 000000000000..b1be46ddb55e
> >> --- /dev/null
> >> +++ b/src/libcamera/controls.cpp
> >> @@ -0,0 +1,160 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2019, Google Inc.
> >> + *
> >> + * controls.cpp - Control handling
> >> + */
> >> +
> >> +#include <limits.h>
> >> +#include <sstream>
> >> +#include <string>
> >> +
> >> +#include <libcamera/controls.h>
> > 
> > Please include this header first, for the same reason as in patch 1/9.
> 
> Fixed.
> 
> >> +
> >> +#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 ControlId
> >> + * Control Identifiers
> >> + * \var IpaAwbEnable
> >> + * bool: Enables or disables the AWB
> >> + * \var ManualExposure
> >> + * Manually control the exposure time in milli-seconds
> >> + * \var ManualGain
> >> + * Controls the value of the manual gain setting
> >> + */
> > 
> > This is an area where I want libcamera to have amazing documentation.
> > Not only will each control need to be documented in details, but I also
> > want interactions between controls to be documented. In the meantime,
> > could you already try to format the documentation block appropriately ?
> > One block per control would be ideal.
> 
> Ok - but I am not by any means considering these controls as 'final'
> yet. These are just examples to get the framework in.
> 
> Each of those is now given their own section.
> 
> If we can generate all the

Yes, all the should be generated :-)

My point was that it would be nice if you could experiment with
splitting the control documentation with a doxygen documentation block
for each control, preferably with multiple paragraphs per control (or
for one control at least). This is likely what we will be generating, so
having the proper doxygen formatting here will help.

> >> +
> >> +/**
> >> + * \struct ControlIdentifier
> >> + * Defines a Control with a unique ID, a name, and a type.
> > 
> > \brief ?
> > 
> > I think a more elaborate description would be useful, as so far I'm not
> > sure why you have split control information in two parts (although I
> > suspect the reason, but an explicit description would be better :-)).
> 
> I've updated this a bit - but it still needs more refinement as the
> implementation grows. And it depends on how the controls really get
> constructed.
> 
> >> + * \var ControlIdentifier::id
> > 
> > Is the ControlIdentifier:: prefix needed ?
> 
> It seems that way.
> 
> Without the prefix - doxygen doesn't seem to know the following var's
> are part of the \struct ControlIdentifier...

OK.

> >> + * 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
> >> + */
> >> +
> >> +/*
> >> + * Two sets of tables are generated for the static control definitions.
> >> + *
> > 
> > s/.\n \*\n \* An/:\r * an/
> > 
> > Is that readable ? :-)
> 
> It's a good job I read regex. ... :D (for a *very* limited subset of regex)
> 
> >> + * An enum to declare the ID (in controls.h), and a type to establish its
> >> + * representation.
> > 
> > The enum is actually not a table, it doesn't generate data as such.
> 
> My point is - I don't want to have to list an enum once to create the
> enum, and then a second table to create definitions. I'd like to find a
> way such that creating a control is just an addition to a single table.

What format do you think we should use to document controls and generate
data ?

> >> + *
> >> + * Todo: Automate the generation of both tables from a single input table.
> > 
> > s/Todo: /\\todo /
> > 
> > Would it be possible to generate them both from a documentation file ?
> 
> ^ Equally - there's a third place that would have to be updated. (the docs).
> 
> There's definitely scope here that things should be autogenerated from a
> single source.

That's my point, having a single documentation file that generates both
the header and the table.

> >> + * Todo: Consider if this should be a static std::map created at init instead.
> > 
> > Any reason why it shouldn't be ? :-) Lookup would be more efficient.
> 
> Because \todo - and trying to figure out the details...
> 
> This is the point where RFC means 'Help out with suggestions on todo's :-D

Then, if it wasn't explicit enough, I think it should be a map :-)

> >> + */
> >> +static const struct ControlIdentifier ControlTypes[] = {
> >> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }
> >> +
> >> +	CONTROL_TYPE(IpaAwbEnable,		ValueBool),
> >> +	CONTROL_TYPE(ManualBrightness,		ValueInteger),
> >> +	CONTROL_TYPE(ManualExposure,		ValueInteger),
> >> +	CONTROL_TYPE(ManualGain,		ValueInteger),
> >> +
> >> +#undef CONTROL_TYPE
> >> +};
> >> +
> >> +static struct ControlIdentifier const *FindControlType(ControlId id)
> > 
> > This is a function and should thus start with a lowercase letter.
> 
> Ack.
> 
> >> +{
> >> +	struct ControlIdentifier const *ident;
> >> +
> >> +	for (ident = ControlTypes;
> >> +	     ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];
> >> +	     ++ident) {
> >> +		if (ident->id == id)
> >> +			return ident;
> >> +	}
> >> +
> >> +	LOG(Controls, Fatal) << "Failed to find a ControlType.";
> >> +
> >> +	/* Unreachable. */
> >> +	return nullptr;
> >> +}
> >> +
> >> +/**
> >> + * \class ControlInfo
> >> + * \brief Describes the information and capabilities of a Control
> > 
> > s/Describes/Describe/
> > 
> > Again a bit more documentation would help.
> > 
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> >> + */
> >> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)
> >> +	: ident_(FindControlType(id)), min_(min), max_(max)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \fn ControlInfo::id()
> >> + * \brief Return the ID of the control information descriptor
> > 
> > s/Return/Retrieve/
> > 
> > This is a bit confusing as nothing documents what a "control information
> > descriptor" is. I think the documentation will need to be revisited.
> > 
> >> + * \return the ControlId
> > 
> > The control ID
> > 
> >> + */
> >> +
> >> +/**
> >> + * \fn ControlInfo::name()
> >> + * \brief Return the string name of the control information descriptor
> > 
> > s/Return/Retrieve/
> > 
> > and below too.
> > 
> >> + * \return A string name for the Control
> >> + */
> >> +
> >> +/**
> >> + * \fn ControlInfo::type()
> >> + * \brief Return the ValueType of the control information descriptor
> >> + * \return the ControlId
> > 
> > The control type
> > 
> >> + */
> >> +
> >> +/**
> >> + * \fn ControlInfo::min()
> >> + * \brief Reports the minimum value of the control
> >> + * \return a Value with the minimum setting for the control
> > 
> > s/a/A/
> > 
> > How does that apply to non-integer controls ?
> 
> I have no idea yet. "RFC" : What are your thoughts?
> 
> min/max are not even yet populated with anything.
> 
> Perhaps it only applies to integer controls.

I think we should document that the value is undefined for other types
of controls, yes. Or would it make sense to also define it for boolean
controls ? A boolean can only take two values, but it may be that in
some circumstances a camera would only support one value. We could drop
the control in that case, but exposing it with the only value it
supports may also be useful.

> >> + */
> >> +
> >> +/**
> >> + * \fn ControlInfo::max()
> >> + * \brief Reports the maximum value of the control
> >> + * \return a Value with the maximum setting for the control
> >> + */
> >> +
> >> +/**
> >> + * \brief Provide a string representation of the ControlInfo
> >> + */
> >> +std::string ControlInfo::toString() const
> >> +{
> >> +	std::stringstream ss;
> >> +
> >> +	ss << "Control: " << name()
> >> +	   << " : Min(" << min_ << ") Max(" << max_ << ")";
> > 
> > I still need to look at how you use this, but in general it's best to
> > shorten the representations as much as possible. I may remove the
> > "Control:" prefix, maybe with
> > 
> > 	ss << name() << "[" << min_ << "," << max_ << "]";
> > 
> > This would print
> > 
> > 	ManualExposure[1,1000]
> > 
> > We could replace the comma with ..
> > 
> > 	ManualExposure[1..1000]
> 
> I like that !
> 
> Done.
> 
> >> +
> >> +	return ss.str();
> >> +}
> >> +
> >> +/**
> >> + * \fn ControlInfo::operator==(const ControlInfo &rhs) const
> >> + * \brief Establish equivalence of ControlInfo. Only the IDs are considered.
> >> + */
> >> +
> >> +/**
> >> + * \fn ControlInfo::operator==(const ControlId rhs) const
> >> + * \brief Establish equivalence of ControlInfo, against a ControlID.
> >> + */
> >> +
> >> +/**
> >> + * \brief Provide a string stream representation of the ControlInfo \a info to
> >> + * the \a stream.
> >> + */
> >> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)
> >> +{
> >> +	return stream << info.toString();
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 8e68373118df..e2c07d79bfb5 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',