[libcamera-devel] [PATCH v3 04/14] libcamera: controls: Introduce control-related data types
Jacopo Mondi
jacopo at jmondi.org
Mon Jul 1 19:21:11 CEST 2019
HI Laurent,
On Mon, Jul 01, 2019 at 08:04:46PM +0300, Laurent Pinchart wrote:
> 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.
>
We should record this with a todo entry maybe ?
> > > +
> > > + 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 see. Yes, its a bit fragile indeed, but I think we can fix on top.
record this with a todo as well?
>
> 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).
>
Naming is not easy here indeed. I would propose ControlDesc for static
information but I'm not sure it's any better.
I think for the moment is fine..
> > > + * \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.
>
Thanks for the explanation. I think we're fine then.
> > > + : 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.
>
Naming is hard here as well. I'm not too un-happy with update() and
not in love with merge(). What about just set() ?
> > > + */
> > > +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 ;-)
>
He could then blame me for not having reviewed this :)
Thanks
j
> > > @@ -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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190701/74d63558/attachment-0001.sig>
More information about the libcamera-devel
mailing list