[libcamera-devel] [PATCH v3 04/14] libcamera: controls: Introduce control-related data types
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 1 19:04:46 CEST 2019
Hi Jacopo,
On Mon, Jul 01, 2019 at 06:08:25PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 01, 2019 at 02:38:07AM +0300, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Add a set of data types to support controls:
> >
> > - ControlValue stores a control type and value in a generic way
> > - ControlId enumerates all the control identifies
>
> identifiers
>
> > - ControlIdentier declares the types of a control and map their names
>
> ControlIdentifier
>
> > - ControlInfo stores runtime information for controls
> > - ControlList contains a set of control info and value pairs
> >
> > The control definitions map is generated from the controls documentation
> > to ensure that the two will always be synchronised.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > Changes since v2:
> >
> > - Squashed "Provide ControlValue class"
> > - Renamed Value to ControlValue
> > - Removed operator<<()
> > - Added control table generation
> > - Moved control definitions to control_definitions.h
> > - Renamed ControlTypes to controlsTypes and make it const
> > - Moved the initial controls list to a separate patch
> > - Renamed control_definitions.h to control_ids.h and
> > control_definitions.cpp to control_types.cpp to match the contained
> > enum and variable name respectively
> > - Indexed ControlList by ControlInfo pointer instead of value
> > - Replaced ControlInfoHash with std::hash specialisation
> > - Added automatic conversion between 32- and 64-bit integer values
> >
> > The automatic conversion between integer types was prompted by an
> > assertion failure due to the use of getInt() on the min() and max()
> > value of an Integer control. The min and max ControlValue instances are
> > create as Integer64, due to the V4L2ControlInfo class returning the
> > range as int64_t. This may need to be reworked.
> > ---
> > Documentation/Doxyfile.in | 3 +-
> > include/libcamera/control_ids.h | 35 +++
> > include/libcamera/controls.h | 134 ++++++++++
> > include/libcamera/meson.build | 2 +
> > src/libcamera/controls.cpp | 428 ++++++++++++++++++++++++++++++++
> > src/libcamera/gen-controls.awk | 106 ++++++++
> > src/libcamera/meson.build | 11 +
> > 7 files changed, 718 insertions(+), 1 deletion(-)
> > create mode 100644 include/libcamera/control_ids.h
> > create mode 100644 include/libcamera/controls.h
> > create mode 100644 src/libcamera/controls.cpp
> > create mode 100755 src/libcamera/gen-controls.awk
> >
> > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> > index c58631200dd5..9ca32241b895 100644
> > --- a/Documentation/Doxyfile.in
> > +++ b/Documentation/Doxyfile.in
> > @@ -868,7 +868,8 @@ EXCLUDE_SYMBOLS = libcamera::SignalBase \
> > libcamera::SlotArgs \
> > libcamera::SlotBase \
> > libcamera::SlotMember \
> > - libcamera::SlotStatic
> > + libcamera::SlotStatic \
> > + std::*
> >
> > # The EXAMPLE_PATH tag can be used to specify one or more files or directories
> > # that contain example code fragments that are included (see the \include
> > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h
> > new file mode 100644
> > index 000000000000..d0e700da9844
> > --- /dev/null
> > +++ b/include/libcamera/control_ids.h
> > @@ -0,0 +1,35 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * control_ids.h : Control ID list
> > + */
> > +
> > +#ifndef __LIBCAMERA_CONTROL_IDS_H__
> > +#define __LIBCAMERA_CONTROL_IDS_H__
> > +
> > +#include <functional>
> > +
> > +namespace libcamera {
> > +
> > +enum ControlId {
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +namespace std {
> > +
> > +template<>
> > +struct hash<libcamera::ControlId> {
> > + using argument_type = libcamera::ControlId;
> > + using result_type = std::size_t;
> > +
> > + result_type operator()(const argument_type &key) const noexcept
> > + {
> > + return std::hash<std::underlying_type<argument_type>::type>()(key);
> > + }
> > +};
> > +
> > +} /* namespace std */
> > +
> > +#endif // __LIBCAMERA_CONTROL_IDS_H__
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > new file mode 100644
> > index 000000000000..ad2d49d522c5
> > --- /dev/null
> > +++ b/include/libcamera/controls.h
> > @@ -0,0 +1,134 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.h - Control handling
> > + */
> > +
> > +#ifndef __LIBCAMERA_CONTROLS_H__
> > +#define __LIBCAMERA_CONTROLS_H__
> > +
> > +#include <stdint.h>
> > +#include <string>
> > +#include <unordered_map>
> > +
> > +#include <libcamera/control_ids.h>
> > +
> > +namespace libcamera {
> > +
> > +class Camera;
> > +
> > +enum ControlValueType {
> > + ControlValueNone,
> > + ControlValueBool,
> > + ControlValueInteger,
> > + ControlValueInteger64,
> > +};
> > +
> > +class ControlValue
> > +{
> > +public:
> > + ControlValue();
> > + ControlValue(bool value);
> > + ControlValue(int value);
> > + ControlValue(int64_t value);
> > +
> > + ControlValueType type() const { return type_; };
> > + bool isNone() const { return type_ == ControlValueNone; };
> > +
> > + void set(bool value);
> > + void set(int value);
> > + void set(int64_t value);
> > +
> > + bool getBool() const;
> > + int getInt() const;
> > + int64_t getInt64() const;
> > +
> > + std::string toString() const;
> > +
> > +private:
> > + ControlValueType type_;
> > +
> > + union {
> > + bool bool_;
> > + int integer_;
> > + int64_t integer64_;
> > + };
> > +};
> > +
> > +struct ControlIdentifier {
> > + ControlId id;
> > + const char *name;
> > + ControlValueType type;
> > +};
> > +
> > +class ControlInfo
> > +{
> > +public:
> > + explicit ControlInfo(ControlId id, const ControlValue &min = 0,
> > + const ControlValue &max = 0);
> > +
> > + ControlId id() const { return ident_->id; }
> > + const char *name() const { return ident_->name; }
> > + ControlValueType type() const { return ident_->type; }
> > +
> > + const ControlValue &min() const { return min_; }
> > + const ControlValue &max() const { return max_; }
> > +
> > + std::string toString() const;
> > +
> > +private:
> > + const struct ControlIdentifier *ident_;
> > + ControlValue min_;
> > + ControlValue max_;
>
> Do we plan to have a step too?
And possibly a default, and possibly other information. I don't know yet
:-)
> > +};
> > +
> > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);
> > +bool operator==(const ControlId &lhs, const ControlInfo &rhs);
> > +bool operator==(const ControlInfo &lhs, const ControlId &rhs);
> > +static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)
> > +{
> > + return !(lhs == rhs);
> > +}
> > +static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)
> > +{
> > + return !(lhs == rhs);
> > +}
> > +static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)
> > +{
> > + return !(lhs == rhs);
> > +}
> > +
> > +class ControlList
> > +{
> > +private:
> > + using ControlListMap = std::unordered_map<const ControlInfo *, ControlValue>;
>
> Does it make sense to make this private if it is then used in the
> public API ?
It's only instantiated by this class for the controls_ field, so I don't
think it needs to be public.
> > +
> > +public:
> > + ControlList(Camera *camera);
> > +
> > + using iterator = ControlListMap::iterator;
> > + using const_iterator = ControlListMap::const_iterator;
> > +
> > + iterator begin() { return controls_.begin(); }
> > + iterator end() { return controls_.end(); }
> > + const_iterator begin() const { return controls_.begin(); }
> > + const_iterator end() const { return controls_.end(); }
> > +
> > + bool contains(const ControlInfo *info) const { return controls_.count(info); };
> > + bool empty() const { return controls_.empty(); }
> > + std::size_t size() const { return controls_.size(); }
> > + void clear() { controls_.clear(); }
> > +
> > + ControlValue &operator[](const ControlInfo *info) { return controls_[info]; }
> > +
> > + void update(const ControlList &list);
> > +
> > +private:
> > + Camera *camera_;
> > + ControlListMap controls_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_CONTROLS_H__ */
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 15484724df01..3067120a1598 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -2,6 +2,8 @@ libcamera_api = files([
> > 'buffer.h',
> > 'camera.h',
> > 'camera_manager.h',
> > + 'control_ids.h',
> > + 'controls.h',
> > 'event_dispatcher.h',
> > 'event_notifier.h',
> > 'geometry.h',
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > new file mode 100644
> > index 000000000000..22db2b93eff2
> > --- /dev/null
> > +++ b/src/libcamera/controls.cpp
> > @@ -0,0 +1,428 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * controls.cpp - Control handling
> > + */
> > +
> > +#include <libcamera/controls.h>
> > +
> > +#include <sstream>
> > +#include <string>
> > +
> > +#include "log.h"
> > +#include "utils.h"
> > +
> > +/**
> > + * \file controls.h
> > + * \brief Describes control framework and controls supported by a camera
> > + */
> > +
> > +namespace libcamera {
> > +
> > +LOG_DEFINE_CATEGORY(Controls)
> > +
> > +/**
> > + * \enum ControlValueType
> > + * Determines the type of value represented by a ControlValue
>
> a/Determines/Defines ?
> "type of a value" ?
I'll replace this with
\brief Define the data type of a value represented by a ControlValue
> > + * \var ControlValueNone
> > + * Identifies an unset control value
> > + * \var ControlValueBool
> > + * Identifies controls storing a boolean value
> > + * \var ControlValueInteger
> > + * Identifies controls storing an integer value
> > + * \var ControlValueInteger64
> > + * Identifies controls storing a 64-bit integer value
> > + */
> > +
> > +/**
> > + * \class ControlValue
> > + * \brief Abstract type for values in a control
>
> Does the control 'contains' a value, or is the value part of the
> control itself? I would say "Abstract type representing the value of a
> control"
I agree, I will use your text.
> > + */
> > +
> > +/**
> > + * \brief Construct an empty ControlValue.
> > + */
> > +ControlValue::ControlValue()
> > + : type_(ControlValueNone)
> > +{
> > +}
> > +
>
> Does this have any use?
It can be used to represent a value that hasn't been set, as opposed to
a value set to a default value.
> > +/**
> > + * \brief Construct a Boolean ControlValue
> > + * \param[in] value Boolean value to store
> > + */
> > +ControlValue::ControlValue(bool value)
> > + : type_(ControlValueBool), bool_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct an integer ControlValue
> > + * \param[in] value Integer value to store
> > + */
> > +ControlValue::ControlValue(int value)
> > + : type_(ControlValueInteger), integer_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Construct a 64 bit integer ControlValue
> > + * \param[in] value String representation to store
>
> Not a string but a 64-bit integer
>
> > + */
> > +ControlValue::ControlValue(int64_t value)
> > + : type_(ControlValueInteger64), integer64_(value)
> > +{
> > +}
> > +
> > +/**
> > + * \fn ControlValue::type
> > + * \brief Return the type of value represented by the object
>
> s/represented by the object// ?
I will use "Retrieve data type of the value"
> > + */
> > +
> > +/**
> > + * \fn ControlValue::isNone
> > + * \brief Determine if the value is initialised
>
> As we check for isNone, the operation determines if the value is NOT
> initialized :)
>
> > + * \return True if the value type is ControlValueNone, false otherwise
>
> Just "True if the value is not initialized... "
I think explicitly saying how we check that the value is uninitialised
is useful.
> > + */
> > +
> > +/**
> > + * \brief Set the value with a boolean
> > + * \param[in] value Boolean value to store
> > + */
> > +void ControlValue::set(bool value)
> > +{
> > + type_ = ControlValueBool;
> > + bool_ = value;
> > +}
>
> Ah yes, you can create None values then update. Is this a good idea?
> Won't control always have a single type associated? Or we want to be
> be able to re-use the same ControlValue with many controls ?
ControlValue instances are stored in a map, so when we do
controls[Brightness].set(30)
if the controls container doesn't contain a ControlValue associated with
Brightness, one will be created with the default constructor, and then
its value will be overwritten. Note that we can also write
controls[Brightness] = 30;
> > +
> > +/**
> > + * \brief Set the value with an integer
> > + * \param[in] value Integer value to store
> > + */
> > +void ControlValue::set(int value)
> > +{
> > + type_ = ControlValueInteger;
> > + integer_ = value;
> > +}
> > +
> > +/**
> > + * \brief Set the value with a 64 bit integer
> > + * \param[in] value 64 bit integer value to store
> > + */
> > +void ControlValue::set(int64_t value)
> > +{
> > + type_ = ControlValueInteger64;
> > + integer64_ = value;
> > +}
> > +
> > +/**
> > + * \brief Get the boolean value.
>
> We usually don't end briefs with full stops, here and below.
>
> > + *
> > + * The value type must be Boolean.
>
> missing \return here and below.
>
> > + */
> > +bool ControlValue::getBool() const
> > +{
> > + ASSERT(type_ == ControlValueBool);
>
> I wonder how easy would that be to create a control value, set it to a
> type, then to another, then read the original type and then crash the
> library here.
Pretty easy, but only in debug builds. In release builds the returned
value will just be undefined. This is something I'm open to discuss.
> > +
> > + return bool_;
> > +}
> > +
> > +/**
> > + * \brief Get the integer value.
> > + *
> > + * The value type must be Integer or Integer64
> > + */
> > +int ControlValue::getInt() const
> > +{
> > + ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
>
> is this a good idea to access 64 bit integers as integers?
It will certainly return an incorrect value if the value is larger than
1 << 32.
I did this because of a crash in a debug build :-) As you may notice in
later patches, the UVC and VIMC pipeline handler create instances of
ControlInfo for controls enumerated from V4L2. As the V4L2 controls API
return minimum and maximum values as int64_t, the ControlValue used to
store the minimum and maximum are of type ControlValueInteger64, even
for 32-bit controls. A call to info.min().getInt() the crashed.
I think this is a bit fragile, and I would like to refactor this part of
the API, but I'm not sure how yet. If you have a good idea we can
discuss it now :-) Otherwise I think it can be done on top of this
series.
> > +
> > + return integer_;
> > +}
> > +
> > +/**
> > + * \brief Get the 64 bit integer value.
> > + *
> > + * The value type must be Integer or Integer64
> > + */
> > +int64_t ControlValue::getInt64() const
> > +{
> > + ASSERT(type_ == ControlValueInteger || type_ == ControlValueInteger64);
>
> Accessing an integer as a 64 bit one is safer instead.
>
> > +
> > + return integer64_;
> > +}
> > +
> > +/**
> > + * \brief Assemble and return a string describing the value
> > + * \return A string describing the ControlValue
> > + */
> > +std::string ControlValue::toString() const
> > +{
> > + switch (type_) {
> > + case ControlValueNone:
> > + return "<None>";
> > + case ControlValueBool:
> > + return bool_ ? "True" : "False";
> > + case ControlValueInteger:
> > + return std::to_string(integer_);
> > + case ControlValueInteger64:
> > + return std::to_string(integer64_);
> > + }
> > +
> > + return "<ValueType Error>";
> > +}
> > +
> > +/**
> > + * \enum ControlId
> > + * Control Identifiers
>
> numerical identifiers? There are many "info" "id" etc etc associated
> with controls here. Make it clear this is the numerical id with a bit
> more of description?
How about "Numerical control ID" ?
> > + */
> > +
> > +/**
> > + * \struct ControlIdentifier
>
> It's easy to confuse this Identifier with ControlId if you describe
> control id as the control identifier.
I agree, and I'm not very fond of the names ControlId vs.
ControlIdentifier. If you think of better names, please feel free to
propose them. I think ControlId is probably fine, ControlIdentifier
could be renamed. ControlInfo is already taken though, and I don't think
ControlStaticInfo and ControlDynamicInfo would be good names (even if
that's essentially what they are).
> > + * \brief Describes a ControlId with control specific constant meta-data.
>
> full stop in brief, and we usually don't use the third person afaict
>
> > + *
> > + * Defines a Control with a unique ID, a name, and a type.
> > + * This structure is used as static part of the autogenerated control
> > + * definitions, which are generated from the ControlId documentation.
> > + *
> > + * \var ControlIdentifier::id
> > + * The unique ID for a control
> > + * \var ControlIdentifier::name
> > + * The string representation of the control
> > + * \var ControlIdentifier::type
> > + * The ValueType required to represent the control value
> > + */
> > +
> > +/*
> > + * The controlTypes are automatically generated to produce a control_types.cpp
> > + * output. This file is not for public use, and so no suitable header exists
> > + * for this sole usage of the controlTypes reference. As such the extern is
> > + * only defined here for use during the ControlInfo constructor and should not
> > + * be referenced directly elsewhere.
> > + */
> > +extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes;
> > +
> > +/**
> > + * \class ControlInfo
> > + * \brief Describe the information and capabilities of a Control
> > + *
> > + * The ControlInfo represents control specific meta-data which is constant on a
> > + * per camera basis. ControlInfo classes are constructed by pipeline handlers
> > + * to expose the controls they support and the metadata needed to utilise those
> > + * controls.
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlInfo with minimum and maximum range parameters.
> > + */
> > +ControlInfo::ControlInfo(ControlId id, const ControlValue &min,
> > + const ControlValue &max)
> > + : min_(min), max_(max)
> > +{
> > + auto iter = controlTypes.find(id);
> > + if (iter == controlTypes.end()) {
> > + LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo";
> > + return;
> > + }
> > +
> > + ident_ = &iter->second;
> > +}
> > +
> > +/**
> > + * \fn ControlInfo::id()
> > + * \brief Retrieve the ID of the control information descriptor
> > + * \return the ControlId
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::name()
> > + * \brief Retrieve the string name of the control information descriptor
> > + * \return A string name for the Control
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::type()
> > + * \brief Retrieve the ValueType of the control information descriptor
> > + * \return The control type
> > + */
>
> Isn't the 'information' in 'control information $SOMETHING' redundant?
Yes, I think that's a bit too long. I will write "Retrieve the control
data type" here, and change the above two functions accordingly.
> > +
> > +/**
> > + * \fn ControlInfo::min()
> > + * \brief Reports the minimum value of the control
> > + * \return a COntrolValue with the minimum setting for the control
>
> s/COntrol/control
>
> and also "minimum setting" would not be better described as "minium
> valid value" or similar ?
>
> > + */
> > +
> > +/**
> > + * \fn ControlInfo::max()
> > + * \brief Reports the maximum value of the control
>
> I know these are not purely getters, but we usually use "Retrieve"
>
> > + * \return a ControlValue with the maximum setting for the control
> > + */
> > +
> > +/**
> > + * \brief Provide a string representation of the ControlInfo
> > + */
> > +std::string ControlInfo::toString() const
> > +{
> > + std::stringstream ss;
> > +
> > + ss << name() << "[" << min_.toString() << ".." << max_.toString() << "]";
> > +
> > + return ss.str();
> > +}
> > +
> > +/**
> > + * \brief Compare control information for equality
>
> I would simply "Verify it two control information are equal" or
> similar
Or just "Compare control information" ? I think we use a "for equality"
construct somewhere else, so I would probably leave this one as-is.
> > + * \param[in] lhs Left-hand side control information
> > + * \param[in] rhs Right-hand side control information
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)
> > +{
> > + return lhs.id() == rhs.id();
> > +}
> > +
> > +/**
> > + * \brief Compare control ID and information for equality
> > + * \param[in] lhs Left-hand side control identifier
> > + * \param[in] rhs Right-hand side control information
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlId &lhs, const ControlInfo &rhs)
> > +{
> > + return lhs == rhs.id();
> > +}
> > +
> > +/**
> > + * \brief Compare control information and ID for equality
> > + * \param[in] lhs Left-hand side control information
> > + * \param[in] rhs Right-hand side control identifier
> > + *
> > + * Control information are compared based on their ID only, as a camera may not
> > + * have two separate controls with the same ID.
> > + *
> > + * \return True if \a lhs and \a rhs are equal, false otherwise
> > + */
> > +bool operator==(const ControlInfo &lhs, const ControlId &rhs)
> > +{
> > + return lhs.id() == rhs;
> > +}
> > +
> > +/**
> > + * \class ControlList
> > + * \brief Associates a list of ControlIds with their values for a Camera.
>
> Full stop and third person in \brief
>
> > + *
> > + * A ControlList specifies a map of ControlIds and Values and associated
> > + * validation against the ControlInfo for the related Camera device.
>
> Maybe I'm tired, but this is very not clear to me. Particularly the
> fact that a map associates ControlIds and value and associated
> validation.
>
> Now that I wrote it I get what the meaning was, but I would re-phrase
> this as
>
> "A ControlList wraps (or other verbs) a map of ControlId to
> ControlValue and provide validation operations against the controlInfo
> provided (or other verbs) by the Camera device that created the
> control"
I fully agree this isn't very clear, I will write
* A ControlList wraps a map of ControlId to ControlValue and provide
* additional validation against the control information exposed by a Camera.
> > + */
> > +
> > +/**
> > + * \brief Construct a ControlList with a reference to the Camera it applies on
> > + */
> > +ControlList::ControlList(Camera *camera)
>
> At this point it is not clear to me what the lifetime of a ControlList
> is, but in most part of our code (ok, not most, many) the camera is
> passed around as a shared pointer. Is this needed here?
It's a good question. On one hand we could, with an additional cost. On
the other hand ControlList instances will in practice be create by a
Request, or returned by a Camera function for control templates. The
requests are only valid for as long as the camera is valid, and
applications should not keep control lists around after the camera is
destroyed. I don't think we need a shared pointer here, but I will add a
sentence to explain this.
> > + : camera_(camera)
> > +{
> > +}
> > +
> > +/**
> > + * \typedef ControlList::iterator
> > + * \brief Iterator for the controls contained within the list.
>
> full stop
>
> > + */
> > +
> > +/**
> > + * \typedef ControlList::const_iterator
> > + * \brief Const iterator for the controls contained within the list.
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::begin()
> > + * \brief Retrieve an iterator to the first Control in the list
> > + * \return An iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn iterator ControlList::end()
> > + * \brief Retrieve an iterator to the next element after the last controls in
> > + * the instance.
> > + * \return An iterator to the element following the last control in the instance
>
> You are using "Control in the list" for being and "control in the
> instance" for end. Care to align them?
>
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::begin() const
> > + * \brief Retrieve a const_iterator to the first Control in the list
> > + * \return A const_iterator to the first Control in the list
> > + */
> > +
> > +/**
> > + * \fn const_iterator ControlList::end() const
> > + * \brief Retrieve a constant iterator pointing to an empty element after the
> > + * \return A const iterator to the element following the last control in the
> > + * instance
> > + */
> > +
> > +/**
> > + * \fn ControlList::contains(const ControlInfo *info) const
> > + * \brief Check if the ist contains a control with the specified \a info
>
> a/ist/list
> I would have used instance, but it's fine.
>
> > + * \param[in] info The control info
> > + * \return True if the list contains a matching control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::empty()
> > + * \brief Identify if the list is empty
> > + * \return True if the list does not contain any control, false otherwise
> > + */
> > +
> > +/**
> > + * \fn ControlList::size()
> > + * \brief Retrieve the number of controls in the list
> > + * \return The number of Control entries stored in the list
> > + */
> > +
> > +/**
> > + * \fn ControlList::clear()
> > + * \brief Removes all controls from the list
> > + */
>
> Same comment for the use of list vs instance here. I think it's mostly
> a matter of tastes.
>
> > +
> > +/**
> > + * \fn ControlList::operator[](const ControlInfo *info)
> > + * \brief Access or insert the control specified by \a info
> > + * \param[in] info The control info
> > + *
> > + * This method returns a reference to the control identified by \a info,
> > + * inserting it in the list if the info is not already present.
> > + *
> > + * \return A reference to the value of the control identified by \a info
> > + */
> > +
> > +/**
> > + * \brief Update all Control values with the value from the given \a list
> > + * \param list The list of controls to update or append to this list
> > + *
> > + * Update all controls in the ControlList, by the values given by \a list
>
> s/by the value/with the value/ ?
>
> > + * If the list already contains a control of this ID then it will be overwritten
>
> s/of this ID/with this ID/ ?
> it will be overwritten or "its value will be updated" ?
The documentation is unclear, I will rewrite it. I'm also wondering if
we should rename this function to merge(), but that could imply that
that other list would get stripped out from its elements, which isn't
the case.
> > + */
> > +void ControlList::update(const ControlList &list)
> > +{
> > + if (list.camera_ != camera_) {
> > + LOG(Controls, Error)
> > + << "ControlLists can not be translated between cameras";
> > + return;
> > + }
> > +
> > + for (auto it : list) {
>
> I'm not sure if "auto &it" makes any difference or not...
I think the compiler will make it a reference automatically. I could be
wrong though.
> > + const ControlInfo *info = it.first;
> > + const ControlValue &value = it.second;
> > +
> > + controls_[info] = value;
> > + }
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk
> > new file mode 100755
> > index 000000000000..a91529b575db
> > --- /dev/null
> > +++ b/src/libcamera/gen-controls.awk
>
> I trust your awk skills and assume everything is perfect here :)
I'll blame Kieran for any problem ;-)
> > @@ -0,0 +1,106 @@
> > +#!/usr/bin/awk -f
> > +
> > +# SPDX-License-Identifier: LGPL-2.1-or-later
> > +
> > +# Controls are documented using Doxygen in the main control.cpp source.
> > +#
> > +# Generate control tables directly from the documentation, creating enumerations
> > +# to support the IDs and static type information regarding each control.
> > +
> > +BEGIN {
> > + id=0
> > + input=ARGV[1]
> > + mode=ARGV[2]
> > + output=ARGV[3]
> > +}
> > +
> > +# Detect Doxygen style comment blocks and ignore other lines
> > +/^\/\*\*$/ { in_doxygen=1; first_line=1; next }
> > +// { if (!in_doxygen) next }
> > +
> > +# Entry point for the Control Documentation
> > +/ * \\enum ControlId$/ { in_controls=1; first_line=0; next }
> > +// { if (!in_controls) next }
> > +
> > +# Extract control information
> > +/ \* \\var/ { names[++id]=$3; first_line=0; next }
> > +/ \* ControlType:/ { types[id] = $3 }
> > +
> > +# End of comment blocks
> > +/^ \*\// { in_doxygen=0 }
> > +
> > +# Identify the end of controls
> > +/^ \* \\/ { if (first_line) exit }
> > +// { first_line=0 }
> > +
> > +################################
> > +# Support output file generation
> > +
> > +function basename(file) {
> > + sub(".*/", "", file)
> > + return file
> > +}
> > +
> > +function Header(file, description) {
> > + print "/* SPDX-License-Identifier: LGPL-2.1-or-later */" > file
> > + print "/*" > file
> > + print " * Copyright (C) 2019, Google Inc." > file
> > + print " *" > file
> > + print " * " basename(file) " - " description > file
> > + print " *" > file
> > + print " * This file is autogenerated. Do not edit." > file
> > + print " */" > file
> > + print "" > file
> > +}
> > +
> > +function EnterNameSpace(file) {
> > + print "namespace libcamera {" > file
> > + print "" > file
> > +}
> > +
> > +function ExitNameSpace(file) {
> > + print "" > file
> > + print "} /* namespace libcamera */" > file
> > +}
> > +
> > +function GenerateHeader(file) {
> > + Header(file, "Control ID list")
> > +
> > + print "#ifndef __LIBCAMERA_CONTROL_IDS_H__" > file
> > + print "#define __LIBCAMERA_CONTROL_IDS_H__" > file
> > + print "" > file
> > +
> > + EnterNameSpace(file)
> > + print "enum ControlId {" > file
> > + for (i=1; i <= id; ++i) {
> > + printf "\t%s,\n", names[i] > file
> > + }
> > + print "};" > file
> > + ExitNameSpace(file)
> > +
> > + print "" > file
> > + print "#endif // __LIBCAMERA_CONTROL_IDS_H__" > file
> > +}
> > +
> > +function GenerateTable(file) {
> > + Header(file, "Control types")
> > + print "#include <libcamera/controls.h>" > file
> > + print "" > file
> > +
> > + EnterNameSpace(file)
> > +
> > + print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file
> > + print "controlTypes {" > file
> > + for (i=1; i <= id; ++i) {
> > + printf "\t{ %s, { %s, \"%s\", ControlValue%s } },\n", names[i], names[i], names[i], types[i] > file
> > + }
> > + print "};" > file
> > + ExitNameSpace(file)
> > +}
> > +
> > +END {
> > + if (mode == "--header")
> > + GenerateHeader(output)
> > + else
> > + GenerateTable(output)
> > +}
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 985aa7e8ab0e..b1ee92735e41 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -3,6 +3,7 @@ libcamera_sources = files([
> > 'camera.cpp',
> > 'camera_manager.cpp',
> > 'camera_sensor.cpp',
> > + 'controls.cpp',
> > 'device_enumerator.cpp',
> > 'device_enumerator_sysfs.cpp',
> > 'event_dispatcher.cpp',
> > @@ -66,6 +67,16 @@ if libudev.found()
> > ])
> > endif
> >
> > +gen_controls = files('gen-controls.awk')
> > +
> > +control_types_cpp = custom_target('control_types_cpp',
> > + input : files('controls.cpp'),
> > + output : 'control_types.cpp',
> > + depend_files : gen_controls,
> > + command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])
> > +
> > +libcamera_sources += control_types_cpp
> > +
>
> If both you and Kieran had a look here, I assume this is great
>
> > libcamera_deps = [
> > cc.find_library('dl'),
> > libudev,
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list