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

[Kieran Bingham]

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 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,
>>> +};
>>> +
>>> +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',
> 

-- 
Regards
--
Kieran