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

[Kieran Bingham]

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 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.


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

-- 
Regards
--
Kieran