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

[Laurent Pinchart]

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 at 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',

-- 
Regards,

Laurent Pinchart