[{"id":2722,"web_url":"https://patchwork.libcamera.org/comment/2722/","msgid":"<20190929100035.bfqtwew72jg5m3rt@uno.localdomain>","date":"2019-09-29T10:00:35","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: controls: Improve\n\tthe API towards applications","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Sep 28, 2019 at 06:22:30PM +0300, Laurent Pinchart wrote:\n> Rework the control-related classes to improve the API towards\n> applications. The goal is to enable writing code similar to\n>\n> \tRequest *req = ...;\n> \tControlList &controls = req->controls();\n> \tcontrols->set(controls::AwbEnable, false);\n> \tcontrols->set(controls::ManualExposure, 1000);\n>\n> \t...\n>\n> \tint32_t exposure = controls->get(controls::ManualExposure);\n>\n\nI very much like this API!\n\n> with the get and set operations ensuring type safety for the control\n> values. This is achieved by creating the following classes:\n>\n> - Control defines controls and is the main way to reference a control.\n> It is a template class to allow methods using it to refer to the\n> control type.\n>\n> - ControlId is the base class of Control. It stores the control ID, name\n> and type, and can be used in contexts where a control needs to be\n> referenced regardless of its type (for instance in lists of controls).\n> This class replaces ControlIdentifier.\n>\n> - ControlValue is kept as-is.\n>\n> The ControlList class now exposes two template get() and set() methods\n> that replace the operator[]. They ensure type safety by infering the\n> value type from the Control reference that they receive.\n>\n> The main way to refer to a control is now through the Control class, and\n> optionally through its base ControlId class. The ControlId enumeration\n> is removed, replaced by a list of global Control instances. Numerical\n> control IDs are turned into macros, and are still exposed as they are\n> required to communicate with IPAs (especially to deserialise control\n> lists). They should however not be used by applications.\n>\n> Auto-generation of header and source files is removed for now to keep\n> the change simple. It will be added back in the future in a more\n> elaborate form.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/control_ids.h | 45 ++--\n> include/libcamera/controls.h | 108 +++++++---\n> src/libcamera/control_ids.cpp | 72 +++++++\n> src/libcamera/controls.cpp | 317 ++++++++++++++--------------\n> src/libcamera/gen-controls.awk | 106 ----------\n> src/libcamera/meson.build | 11 +-\n> src/libcamera/pipeline/uvcvideo.cpp | 40 ++--\n> src/libcamera/pipeline/vimc.cpp | 28 +--\n> test/controls/control_info.cpp | 25 +--\n> test/controls/control_list.cpp | 66 +++---\n> 10 files changed, 391 insertions(+), 427 deletions(-)\n> create mode 100644 src/libcamera/control_ids.cpp\n> delete mode 100755 src/libcamera/gen-controls.awk\n>\n> diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h\n> index 75b6a2d5cafe..81e3e2193767 100644\n> --- a/include/libcamera/control_ids.h\n> +++ b/include/libcamera/control_ids.h\n> @@ -8,34 +8,31 @@\n> #ifndef __LIBCAMERA_CONTROL_IDS_H__\n> #define __LIBCAMERA_CONTROL_IDS_H__\n>\n> -#include \n> +#include \n\nC++ has if I'm not mistaken\n\n> +\n> +#include \n>\n> namespace libcamera {\n>\n> -enum ControlId {\n> -\tAwbEnable,\n> -\tBrightness,\n> -\tContrast,\n> -\tSaturation,\n> -\tManualExposure,\n> -\tManualGain,\n> -};\n> +namespace controls {\n> +\n> +#define AWB_ENABLE\t\t1\n> +#define BRIGHTNESS\t\t2\n> +#define CONTRAST\t\t3\n> +#define SATURATION\t\t4\n> +#define MANUAL_EXPOSURE\t\t5\n> +#define MANUAL_GAIN\t\t6\n> +\n> +extern const Control None;\n> +extern const Control AwbEnable;\n> +extern const Control Brightness;\n> +extern const Control Contrast;\n> +extern const Control Saturation;\n> +extern const Control ManualExposure;\n> +extern const Control ManualGain;\n> +\n> +} /* namespace controls */\n>\n> } /* namespace libcamera */\n>\n> -namespace std {\n> -\n> -template<>\n> -struct hash {\n> -\tusing argument_type = libcamera::ControlId;\n> -\tusing result_type = std::size_t;\n> -\n> -\tresult_type operator()(const argument_type &key) const noexcept\n> -\t{\n> -\t\treturn std::hash::type>()(key);\n> -\t}\n> -};\n> -\n> -} /* namespace std */\n> -\n> #endif // __LIBCAMERA_CONTROL_IDS_H__\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index a3089064c4b5..9698bd1dd383 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -8,12 +8,9 @@\n> #ifndef __LIBCAMERA_CONTROLS_H__\n> #define __LIBCAMERA_CONTROLS_H__\n>\n> -#include \n> #include \n> #include \n>\n> -#include \n> -\n> namespace libcamera {\n>\n> class Camera;\n> @@ -53,55 +50,75 @@ private:\n> \t};\n> };\n>\n> -struct ControlIdentifier {\n> -\tControlId id;\n> -\tconst char *name;\n> -\tControlType type;\n> +class ControlId\n> +{\n> +public:\n> +\tunsigned int id() const { return id_; }\n> +\tconst char *name() const { return name_; }\n> +\tControlType type() const { return type_; }\n> +\n> +protected:\n> +\tControlId(unsigned int id, const char *name, ControlType type)\n> +\t\t: id_(id), name_(name), type_(type)\n> +\t{\n> +\t}\n> +\n> +private:\n> +\tControlId(const ControlId &) = delete;\n> +\tControlId &operator=(const ControlId &) = delete;\n> +\n> +\tunsigned int id_;\n> +\tconst char *name_;\n\nThinking about serialization, is is worth transporting the name ?\n\nAs we discussed, probably re-introducing a global map of \nwould allow to re-construct the Control just by using the id. Of course\nthat would require linking against the file that defines the control\nmap also for closed source IPA. Could they live without the control\nname otherwise ?\n\n> +\tControlType type_;\n> +};\n> +\n> +static inline bool operator==(const ControlId &lhs, const ControlId &rhs)\n> +{\n> +\treturn lhs.id() == rhs.id();\n> +}\n> +\n> +static inline bool operator!=(const ControlId &lhs, const ControlId &rhs)\n> +{\n> +\treturn !(lhs == rhs);\n> +}\n> +\n> +template\n> +class Control : public ControlId\n> +{\n> +public:\n> +\tusing type = T;\n\nThis does not seems to be used.\n\n> +\n> +\tControl(unsigned int id, const char *name);\n> +\n> +private:\n\nWhy private ?\n\n> +\tControl(const Control &) = delete;\n> +\tControl &operator=(const Control &) = delete;\n> };\n>\n> class ControlInfo\n> {\n> public:\n> -\texplicit ControlInfo(ControlId id, const ControlValue &min = 0,\n> +\texplicit ControlInfo(const ControlId &id, const ControlValue &min = 0,\n> \t\t\t const ControlValue &max = 0);\n>\n> -\tControlId id() const { return ident_->id; }\n> -\tconst char *name() const { return ident_->name; }\n> -\tControlType type() const { return ident_->type; }\n> -\n> +\tconst ControlId &id() const { return id_; }\n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n>\n> \tstd::string toString() const;\n>\n> private:\n> -\tconst struct ControlIdentifier *ident_;\n> +\tconst ControlId &id_;\n> \tControlValue min_;\n> \tControlValue max_;\n> };\n>\n> -bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);\n> -bool operator==(const ControlId &lhs, const ControlInfo &rhs);\n> -bool operator==(const ControlInfo &lhs, const ControlId &rhs);\n> -static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)\n> -{\n> -\treturn !(lhs == rhs);\n> -}\n> -static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)\n> -{\n> -\treturn !(lhs == rhs);\n> -}\n> -static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)\n> -{\n> -\treturn !(lhs == rhs);\n> -}\n> -\n> -using ControlInfoMap = std::unordered_map;\n> +using ControlInfoMap = std::unordered_map;\n>\n> class ControlList\n> {\n> private:\n> -\tusing ControlListMap = std::unordered_map;\n> +\tusing ControlListMap = std::unordered_map;\n\nI like both maps are indexed by the same key\n\n>\n> public:\n> \tControlList(Camera *camera);\n> @@ -114,18 +131,39 @@ public:\n> \tconst_iterator begin() const { return controls_.begin(); }\n> \tconst_iterator end() const { return controls_.end(); }\n>\n> -\tbool contains(ControlId id) const;\n> -\tbool contains(const ControlInfo *info) const;\n> +\tbool contains(const ControlId &id) const;\n> \tbool empty() const { return controls_.empty(); }\n> \tstd::size_t size() const { return controls_.size(); }\n> \tvoid clear() { controls_.clear(); }\n>\n> -\tControlValue &operator[](ControlId id);\n> -\tControlValue &operator[](const ControlInfo *info) { return controls_[info]; }\n> +\ttemplate\n> +\tconst T &get(const Control &ctrl) const\n> +\t{\n> +\t\tconst ControlValue *val = find(ctrl);\n> +\t\tif (!val) {\n> +\t\t\tstatic T t(0);\n> +\t\t\treturn t;\n> +\t\t}\n> +\n> +\t\treturn val->get();\n> +\t}\n> +\n> +\ttemplate\n> +\tvoid set(const Control &ctrl, const T &value)\n> +\t{\n> +\t\tControlValue *val = find(ctrl);\n> +\t\tif (!val)\n> +\t\t\treturn;\n> +\n> +\t\tval->set(value);\n> +\t}\n\nIn terms of generated code this does not make any difference, but as\nthese functions are inlinded in the class definition, I would\n\n\tconst T &get(const Control &ctrl) const\n\t{\n\t\tconst ControlValue *val = find(ctrl);\n\t\treturn val ? static T t(0) :\n\t\t val->get();\n\t}\n\n\ttemplate\n\tvoid set(const Control &ctrl, const T &value)\n\t{\n\t\tControlValue *val = find(ctrl);\n\t\tif (val)\n\t\t val->set(value);\n\t}\n\nTo shorten them by a few lines (be aware, not compiled)\n\n>\n> \tvoid update(const ControlList &list);\n>\n> private:\n> +\tconst ControlValue *find(const ControlId &id) const;\n> +\tControlValue *find(const ControlId &id);\n> +\n> \tCamera *camera_;\n> \tControlListMap controls_;\n> };\n> diff --git a/src/libcamera/control_ids.cpp b/src/libcamera/control_ids.cpp\n> new file mode 100644\n> index 000000000000..7e4e38b572f0\n> --- /dev/null\n> +++ b/src/libcamera/control_ids.cpp\n> @@ -0,0 +1,72 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * control_ids.cpp : Control ID list\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file control_ids.h\n> + * \\brief Camera control identifiers\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace controls {\n> +\n> +/**\n> + * \\def AWB_ENABLE\n> + * \\brief Numerical value of the AwbEnable control\n> + * \\def BRIGHTNESS\n> + * \\brief Numerical value of the Brightness control\n> + * \\def CONTRAST\n> + * \\brief Numerical value of the Contrast control\n> + * \\def SATURATION\n> + * \\brief Numerical value of the Saturation control\n> + * \\def MANUAL_EXPOSURE\n> + * \\brief Numerical value of the ManualExposure control\n> + * \\def MANUAL_GAIN\n> + * \\brief Numerical value of the ManualGain control\n> + */\n> +\n> +/**\n> + * \\brief Invalid control\n> + */\n> +extern const Control None(0, \"none\");\n> +\n> +/**\n> + * \\brief Enables or disables the AWB.\n> + * \\sa ManualGain\n> + */\n> +extern const Control AwbEnable(AWB_ENABLE, \"AwbEnable\");\n> +\n> +/**\n> + * \\brief Specify a fixed brightness parameter.\n> + */\n> +extern const Control Brightness(BRIGHTNESS, \"Brightness\");\n> +\n> +/**\n> + * \\brief Specify a fixed contrast parameter.\n> + */\n> +extern const Control Contrast(CONTRAST, \"Contrast\");\n> +\n> +/**\n> + * \\brief Specify a fixed saturation parameter.\n> + */\n> +extern const Control Saturation(SATURATION, \"Saturation\");\n> +\n> +/**\n> + * \\brief Specify a fixed exposure time in milli-seconds\n> + */\n> +extern const Control ManualExposure(MANUAL_EXPOSURE, \"ManualExposure\");\n> +\n> +/**\n> + * \\brief Specify a fixed gain parameter\n> + */\n> +extern const Control ManualGain(MANUAL_GAIN, \"ManualGain\");\n> +\n> +} /* namespace controls */\n> +\n> +} /* namespace libcamera */\n\nWe'll really need to expand this documentation soon :(\n\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 295ccd55a622..8886660b7617 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -18,6 +18,29 @@\n> /**\n> * \\file controls.h\n> * \\brief Describes control framework and controls supported by a camera\n> + *\n> + * A control is a mean to govern or influence the operation of a camera. Every\n> + * control is defined by a unique numerical ID, a name string and the data type\n> + * of the value it stores. The libcamera API defines a set of standard controls\n> + * in the libcamera::controls namespace, as a set of instances of the Control\n> + * class.\n> + *\n> + * The main way for applications to interact with controls is through the\n> + * ControlList stored in the Request class:\n> + *\n> + * \\code{.cpp}\n> + * Request *req = ...;\n> + * ControlList &controls = req->controls();\n> + * controls->set(controls::AwbEnable, false);\n> + * controls->set(controls::ManualExposure, 1000);\n> + *\n> + * ...\n> + *\n> + * int32_t exposure = controls->get(controls::ManualExposure);\n> + * \\endcode\n> + *\n> + * The ControlList::get() and ControlList::set() methods automatically deduce\n> + * the data type based on the control.\n> */\n>\n> namespace libcamera {\n> @@ -173,76 +196,120 @@ std::string ControlValue::toString() const\n> }\n>\n> /**\n> - * \\enum ControlId\n> - * \\brief Numerical control ID\n> + * \\class ControlId\n> + * \\brief Control static metadata\n> + *\n> + * The ControlId class stores a control ID, name and data type. It provides\n> + * unique identification of a control, but without support for compile-time\n> + * type deduction that the derived template Control class supports. See the\n> + * Control class for more information.\n> */\n>\n> /**\n> - * \\var AwbEnable\n> - * ControlType: Bool\n> - *\n> - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> + * \\fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)\n> + * \\brief Construct a ControlId instance\n> + * \\param[in] id The control numerical ID\n> + * \\param[in] name The control name\n> + * \\param[in] type The control data type\n> */\n>\n> /**\n> - * \\var Brightness\n> - * ControlType: Integer32\n> - *\n> - * Specify a fixed brightness parameter.\n> + * \\fn unsigned int ControlId::id() const\n> + * \\brief Retrieve the control numerical ID\n> + * \\return The control numerical ID\n> */\n>\n> /**\n> - * \\var Contrast\n> - * ControlType: Integer32\n> - *\n> - * Specify a fixed contrast parameter.\n> + * \\fn const char *ControlId::name() const\n> + * \\brief Retrieve the control name\n> + * \\return The control name\n> */\n>\n> /**\n> - * \\var Saturation\n> - * ControlType: Integer32\n> - *\n> - * Specify a fixed saturation parameter.\n> + * \\fn ControlType ControlId::type() const\n> + * \\brief Retrieve the control data type\n> + * \\return The control data type\n> */\n>\n> /**\n> - * \\var ManualExposure\n> - * ControlType: Integer32\n> + * \\fn bool operator==(const ControlId &lhs, const ControlId &rhs)\n> + * \\brief Compare two ControlId instances for equality\n> + * \\param[in] lhs Left-hand side ControlId\n> + * \\param[in] rhs Left-hand side ControlId\n\nRight-hand\n\n> + *\n> + * ControlId instances are compared based on the numerical ControlId::id()\n> + * only, as an object may not have two separate controls with the same\n> + * numerical ID.\n> *\n> - * Specify a fixed exposure time in milli-seconds\n> + * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n\n \\return True if \\a lhs and \\a rhs have the same control Id, false\n otherwise\n\n?\n\n> */\n>\n> /**\n> - * \\var ManualGain\n> - * ControlType: Integer32\n> + * \\class Control\n> + * \\brief Describe a control and its intrinsic properties\n> + *\n> + * The Control class models a control exposed by a camera. Its template type\n> + * name T refers to the control data type, and allows methods that operate on\n> + * control values to be defined as template methods using the same type T for\n> + * the control value. See for instance how the ControlList::get() method\n> + * returns a value corresponding to the type of the requested control.\n> + *\n> + * While this class is the main mean to refer to a control, the control\n> + * identifying information are stored in the non-template base ControlId class.\n> + * This allows code that operates on a set of controls of different types to\n> + * reference those controls through a ControlId instead of a Control. For\n> + * instance, the list of controls supported by a camera is exposed as ControlId\n> + * instead of Control.\n> *\n> - * Specify a fixed gain parameter\n> + * While controls of any type can be defined through template specialisation,\n> + * libcamera only supports the bool, int32_t and int64_t types natively (this\n> + * includes types that are equivalent to the supported types, such as int and\n> + * long int).\n\nThis is just temporary, would you like to document this?\n\n> + *\n> + * Controls IDs shall be unique. While nothing prevents multiple instances of\n> + * the Control class to be created with the same ID, this may lead to undefined\n> + * behaviour.\n> */\n>\n> /**\n> - * \\struct ControlIdentifier\n> - * \\brief Describe a ControlId with control specific constant meta-data\n> - *\n> - * Defines a Control with a unique ID, a name, and a type.\n> - * This structure is used as static part of the auto-generated control\n> - * definitions, which are generated from the ControlId documentation.\n> + * \\fn Control::Control(unsigned int id, const char *name)\n> + * \\brief Construct a Control instance\n> + * \\param[in] id The control numerical ID\n> + * \\param[in] name The control name\n> *\n> - * \\var ControlIdentifier::id\n> - * The unique ID for a control\n> - * \\var ControlIdentifier::name\n> - * The string representation of the control\n> - * \\var ControlIdentifier::type\n> - * The ValueType required to represent the control value\n> + * The control data type is automatically deduced from the template type T.\n> */\n>\n> -/*\n> - * The controlTypes are automatically generated to produce a control_types.cpp\n> - * output. This file is not for public use, and so no suitable header exists\n> - * for this sole usage of the controlTypes reference. As such the extern is\n> - * only defined here for use during the ControlInfo constructor and should not\n> - * be referenced directly elsewhere.\n> +/**\n> + * \\typedef Control::type\n> + * \\brief The Control template type T\n> */\n> -extern const std::unordered_map controlTypes;\n> +\n> +#ifndef __DOXYGEN__\n> +template<>\n> +Control::Control(unsigned int id, const char *name)\n> +\t: ControlId(id, name, ControlTypeNone)\n> +{\n> +}\n> +\n> +template<>\n> +Control::Control(unsigned int id, const char *name)\n> +\t: ControlId(id, name, ControlTypeBool)\n> +{\n> +}\n> +\n> +template<>\n> +Control::Control(unsigned int id, const char *name)\n> +\t: ControlId(id, name, ControlTypeInteger32)\n> +{\n> +}\n> +\n> +template<>\n> +Control::Control(unsigned int id, const char *name)\n> +\t: ControlId(id, name, ControlTypeInteger64)\n> +{\n> +}\n> +#endif /* __DOXYGEN__ */\n>\n> /**\n> * \\class ControlInfo\n> @@ -260,17 +327,10 @@ extern const std::unordered_map controlTypes;\n> * \\param[in] min The control minimum value\n> * \\param[in] max The control maximum value\n> */\n> -ControlInfo::ControlInfo(ControlId id, const ControlValue &min,\n> +ControlInfo::ControlInfo(const ControlId &id, const ControlValue &min,\n> \t\t\t const ControlValue &max)\n> -\t: min_(min), max_(max)\n> +\t: id_(id), min_(min), max_(max)\n> {\n> -\tauto iter = controlTypes.find(id);\n> -\tif (iter == controlTypes.end()) {\n> -\t\tLOG(Controls, Fatal) << \"Attempt to create invalid ControlInfo\";\n> -\t\treturn;\n> -\t}\n> -\n> -\tident_ = &iter->second;\n> }\n>\n> /**\n> @@ -279,18 +339,6 @@ ControlInfo::ControlInfo(ControlId id, const ControlValue &min,\n> * \\return The control ID\n> */\n>\n> -/**\n> - * \\fn ControlInfo::name()\n> - * \\brief Retrieve the control name string\n> - * \\return The control name string\n> - */\n> -\n> -/**\n> - * \\fn ControlInfo::type()\n> - * \\brief Retrieve the control data type\n> - * \\return The control data type\n> - */\n> -\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control\n> @@ -310,56 +358,11 @@ std::string ControlInfo::toString() const\n> {\n> \tstd::stringstream ss;\n>\n> -\tss << name() << \"[\" << min_.toString() << \"..\" << max_.toString() << \"]\";\n> +\tss << id_.name() << \"[\" << min_.toString() << \"..\" << max_.toString() << \"]\";\n>\n> \treturn ss.str();\n> }\n>\n> -/**\n> - * \\brief Compare control information for equality\n> - * \\param[in] lhs Left-hand side control information\n> - * \\param[in] rhs Right-hand side control information\n> - *\n> - * Control information is compared based on the ID only, as a camera may not\n> - * have two separate controls with the same ID.\n> - *\n> - * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> - */\n> -bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)\n> -{\n> -\treturn lhs.id() == rhs.id();\n> -}\n> -\n> -/**\n> - * \\brief Compare control ID and information for equality\n> - * \\param[in] lhs Left-hand side control identifier\n> - * \\param[in] rhs Right-hand side control information\n> - *\n> - * Control information is compared based on the ID only, as a camera may not\n> - * have two separate controls with the same ID.\n> - *\n> - * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> - */\n> -bool operator==(const ControlId &lhs, const ControlInfo &rhs)\n> -{\n> -\treturn lhs == rhs.id();\n> -}\n> -\n> -/**\n> - * \\brief Compare control information and ID for equality\n> - * \\param[in] lhs Left-hand side control information\n> - * \\param[in] rhs Right-hand side control identifier\n> - *\n> - * Control information is compared based on the ID only, as a camera may not\n> - * have two separate controls with the same ID.\n> - *\n> - * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> - */\n> -bool operator==(const ControlInfo &lhs, const ControlId &rhs)\n> -{\n> -\treturn lhs.id() == rhs;\n> -}\n> -\n> /**\n> * \\typedef ControlInfoMap\n> * \\brief A map of ControlId to ControlInfo\n> @@ -431,29 +434,9 @@ ControlList::ControlList(Camera *camera)\n> *\n> * \\return True if the list contains a matching control, false otherwise\n> */\n> -bool ControlList::contains(ControlId id) const\n> +bool ControlList::contains(const ControlId &id) const\n> {\n> -\tconst ControlInfoMap &controls = camera_->controls();\n> -\tconst auto iter = controls.find(id);\n> -\tif (iter == controls.end()) {\n> -\t\tLOG(Controls, Error)\n> -\t\t\t<< \"Camera \" << camera_->name()\n> -\t\t\t<< \" does not support control \" << id;\n> -\n> -\t\treturn false;\n> -\t}\n> -\n> -\treturn controls_.find(&iter->second) != controls_.end();\n> -}\n> -\n> -/**\n> - * \\brief Check if the list contains a control with the specified \\a info\n> - * \\param[in] info The control info\n> - * \\return True if the list contains a matching control, false otherwise\n> - */\n> -bool ControlList::contains(const ControlInfo *info) const\n> -{\n> -\treturn controls_.find(info) != controls_.end();\n> +\treturn controls_.find(&id) != controls_.end();\n> }\n>\n> /**\n> @@ -474,44 +457,60 @@ bool ControlList::contains(const ControlInfo *info) const\n> */\n>\n> /**\n> - * \\brief Access or insert the control specified by \\a id\n> - * \\param[in] id The control ID\n> + * \\fn template const T &ControlList::get() const\n> + * \\brief Get the value of a control\n> + * \\param[in] ctrl The control\n> *\n> - * This method returns a reference to the control identified by \\a id, inserting\n> - * it in the list if the ID is not already present.\n> + * The behaviour is undefined if the control \\a ctrl is not present in the\n> + * list. Use ControlList::contains() to test for the presence of a control in\n> + * the list before retrieving its value.\n\nIs it undefined? An empty control is returned if I read things right\n\n> *\n> - * The behaviour is undefined if the control \\a id is not supported by the\n> - * camera that the ControlList refers to.\n> + * The control value type shall match the type T, otherwise the behaviour is\n> + * undefined.\n> *\n> - * \\return A reference to the value of the control identified by \\a id\n> + * \\return The control value\n> */\n> -ControlValue &ControlList::operator[](ControlId id)\n> +\n> +/**\n> + * \\fn template void ControlList::set()\n> + * \\brief Set the control value to \\a value\n> + * \\param[in] ctrl The control\n> + * \\param[in] value The control value\n> + *\n> + * This method sets the value of a control in the control list. If the control\n> + * is already present in the list, its value is updated.\n\nWhat about clearly stating the control is added if not present?\n\n> + *\n> + * The behaviour is undefined if the control \\a ctrl is not supported by the\n> + * camera that the list refers to.\n\nIt seems to me it just gets ignored\n\n> + */\n> +\n> +const ControlValue *ControlList::find(const ControlId &id) const\n> +{\n> +\tconst auto iter = controls_.find(&id);\n> +\tif (iter == controls_.end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< \"Control \" << id.name() << \" not found\";\n> +\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\treturn &iter->second;\n> +}\n> +\n> +ControlValue *ControlList::find(const ControlId &id)\n> {\n> \tconst ControlInfoMap &controls = camera_->controls();\n> -\tconst auto iter = controls.find(id);\n> +\tconst auto iter = controls.find(&id);\n> \tif (iter == controls.end()) {\n> \t\tLOG(Controls, Error)\n> \t\t\t<< \"Camera \" << camera_->name()\n> -\t\t\t<< \" does not support control \" << id;\n> -\n> -\t\tstatic ControlValue empty;\n> -\t\treturn empty;\n> +\t\t\t<< \" does not support control \" << id.name();\n> +\t\treturn nullptr;\n> \t}\n>\n> -\treturn controls_[&iter->second];\n> +\treturn &controls_[&id];\n\nDouble lookup ? Couldn't you re-use iter ?\n\n> }\n>\n> -/**\n> - * \\fn ControlList::operator[](const ControlInfo *info)\n> - * \\brief Access or insert the control specified by \\a info\n> - * \\param[in] info The control info\n> - *\n> - * This method returns a reference to the control identified by \\a info,\n> - * inserting it in the list if the info is not already present.\n> - *\n> - * \\return A reference to the value of the control identified by \\a info\n> - */\n> -\n> /**\n> * \\brief Update the list with a union of itself and \\a other\n> * \\param other The other list\n> @@ -533,10 +532,10 @@ void ControlList::update(const ControlList &other)\n> \t}\n>\n> \tfor (auto it : other) {\n> -\t\tconst ControlInfo *info = it.first;\n> +\t\tconst ControlId *id = it.first;\n> \t\tconst ControlValue &value = it.second;\n>\n> -\t\tcontrols_[info] = value;\n> +\t\tcontrols_[id] = value;\n> \t}\n> }\n>\n> diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk\n> deleted file mode 100755\n> index a3f291e7071c..000000000000\n> --- a/src/libcamera/gen-controls.awk\n> +++ /dev/null\n> @@ -1,106 +0,0 @@\n> -#!/usr/bin/awk -f\n> -\n> -# SPDX-License-Identifier: LGPL-2.1-or-later\n> -\n> -# Controls are documented using Doxygen in the main controls.cpp source.\n> -#\n> -# Generate control tables directly from the documentation, creating enumerations\n> -# to support the IDs and static type information regarding each control.\n> -\n> -BEGIN {\n> -\tid=0\n> -\tinput=ARGV[1]\n> -\tmode=ARGV[2]\n> -\toutput=ARGV[3]\n> -}\n> -\n> -# Detect Doxygen style comment blocks and ignore other lines\n> -/^\\/\\*\\*$/ { in_doxygen=1; first_line=1; next }\n> -// { if (!in_doxygen) next }\n> -\n> -# Entry point for the Control Documentation\n> -/ * \\\\enum ControlId$/ { in_controls=1; first_line=0; next }\n> -// { if (!in_controls) next }\n> -\n> -# Extract control information\n> -/ \\* \\\\var/ { names[++id]=$3; first_line=0; next }\n> -/ \\* ControlType:/ { types[id] = $3 }\n> -\n> -# End of comment blocks\n> -/^ \\*\\// { in_doxygen=0 }\n> -\n> -# Identify the end of controls\n> -/^ \\* \\\\/ { if (first_line) exit }\n> -// { first_line=0 }\n> -\n> -################################\n> -# Support output file generation\n> -\n> -function basename(file) {\n> -\tsub(\".*/\", \"\", file)\n> -\treturn file\n> -}\n> -\n> -function Header(file, description) {\n> -\tprint \"/* SPDX-License-Identifier: LGPL-2.1-or-later */\" > file\n> -\tprint \"/*\" > file\n> -\tprint \" * Copyright (C) 2019, Google Inc.\" > file\n> -\tprint \" *\" > file\n> -\tprint \" * \" basename(file) \" - \" description > file\n> -\tprint \" *\" > file\n> -\tprint \" * This file is auto-generated. Do not edit.\" > file\n> -\tprint \" */\" > file\n> -\tprint \"\" > file\n> -}\n> -\n> -function EnterNameSpace(file) {\n> -\tprint \"namespace libcamera {\" > file\n> -\tprint \"\" > file\n> -}\n> -\n> -function ExitNameSpace(file) {\n> -\tprint \"\" > file\n> -\tprint \"} /* namespace libcamera */\" > file\n> -}\n> -\n> -function GenerateHeader(file) {\n> -\tHeader(file, \"Control ID list\")\n> -\n> -\tprint \"#ifndef __LIBCAMERA_CONTROL_IDS_H__\" > file\n> -\tprint \"#define __LIBCAMERA_CONTROL_IDS_H__\" > file\n> -\tprint \"\" > file\n> -\n> -\tEnterNameSpace(file)\n> -\tprint \"enum ControlId {\" > file\n> -\tfor (i=1; i <= id; ++i) {\n> -\t\tprintf \"\\t%s,\\n\", names[i] > file\n> -\t}\n> -\tprint \"};\" > file\n> -\tExitNameSpace(file)\n> -\n> -\tprint \"\" > file\n> -\tprint \"#endif // __LIBCAMERA_CONTROL_IDS_H__\" > file\n> -}\n> -\n> -function GenerateTable(file) {\n> -\tHeader(file, \"Control types\")\n> -\tprint \"#include \" > file\n> -\tprint \"\" > file\n> -\n> -\tEnterNameSpace(file)\n> -\n> -\tprint \"extern const std::unordered_map\" > file\n> -\tprint \"controlTypes {\" > file\n> -\tfor (i=1; i <= id; ++i) {\n> -\t\tprintf \"\\t{ %s, { %s, \\\"%s\\\", ControlType%s } },\\n\", names[i], names[i], names[i], types[i] > file\n> -\t}\n> -\tprint \"};\" > file\n> -\tExitNameSpace(file)\n> -}\n> -\n> -END {\n> -\tif (mode == \"--header\")\n> -\t\tGenerateHeader(output)\n> -\telse\n> -\t\tGenerateTable(output)\n> -}\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 755149c55c7b..8123d1d5bee9 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -5,6 +5,7 @@ libcamera_sources = files([\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> 'controls.cpp',\n> + 'control_ids.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n> @@ -57,16 +58,6 @@ if libudev.found()\n> ])\n> endif\n>\n> -gen_controls = files('gen-controls.awk')\n> -\n> -control_types_cpp = custom_target('control_types_cpp',\n> - input : files('controls.cpp'),\n> - output : 'control_types.cpp',\n> - depend_files : gen_controls,\n> - command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])\n> -\n> -libcamera_sources += control_types_cpp\n> -\n> gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n>\n> version_cpp = vcs_tag(command : [gen_version, meson.build_root()],\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 0d56758e3e1d..44e19f40f1b9 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -10,6 +10,7 @@\n> #include \n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -230,33 +231,20 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \tV4L2ControlList controls;\n>\n> \tfor (auto it : request->controls()) {\n> -\t\tconst ControlInfo *ci = it.first;\n> +\t\tconst ControlId &id = *it.first;\n> \t\tControlValue &value = it.second;\n>\n> -\t\tswitch (ci->id()) {\n> -\t\tcase Brightness:\n> +\t\tif (id == controls::Brightness) {\n> \t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tcase Contrast:\n> +\t\t} else if (id == controls::Contrast) {\n> \t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tcase Saturation:\n> +\t\t} else if (id == controls::Saturation) {\n> \t\t\tcontrols.add(V4L2_CID_SATURATION, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tcase ManualExposure:\n> +\t\t} else if (id == controls::ManualExposure) {\n> \t\t\tcontrols.add(V4L2_CID_EXPOSURE_AUTO, 1);\n> \t\t\tcontrols.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tcase ManualGain:\n> +\t\t} else if (id == controls::ManualGain) {\n> \t\t\tcontrols.add(V4L2_CID_GAIN, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tdefault:\n> -\t\t\tbreak;\n> \t\t}\n> \t}\n>\n> @@ -352,23 +340,23 @@ int UVCCameraData::init(MediaEntity *entity)\n> \tfor (const auto &ctrl : controls) {\n> \t\tunsigned int v4l2Id = ctrl.first;\n> \t\tconst V4L2ControlInfo &info = ctrl.second;\n> -\t\tControlId id;\n> +\t\tconst ControlId *id = &controls::None;\n>\n> \t\tswitch (v4l2Id) {\n> \t\tcase V4L2_CID_BRIGHTNESS:\n> -\t\t\tid = Brightness;\n> +\t\t\tid = &controls::Brightness;\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_CONTRAST:\n> -\t\t\tid = Contrast;\n> +\t\t\tid = &controls::Contrast;\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_SATURATION:\n> -\t\t\tid = Saturation;\n> +\t\t\tid = &controls::Saturation;\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> -\t\t\tid = ManualExposure;\n> +\t\t\tid = &controls::ManualExposure;\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_GAIN:\n> -\t\t\tid = ManualGain;\n> +\t\t\tid = &controls::ManualGain;\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tcontinue;\n> @@ -376,7 +364,7 @@ int UVCCameraData::init(MediaEntity *entity)\n>\n> \t\tcontrolInfo_.emplace(std::piecewise_construct,\n> \t\t\t\t std::forward_as_tuple(id),\n> -\t\t\t\t std::forward_as_tuple(id, info.min(), info.max()));\n> +\t\t\t\t std::forward_as_tuple(*id, info.min(), info.max()));\n> \t}\n>\n> \treturn 0;\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index e549dc64b996..a48febf2172e 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -283,24 +284,15 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> \tV4L2ControlList controls;\n>\n> \tfor (auto it : request->controls()) {\n> -\t\tconst ControlInfo *ci = it.first;\n> +\t\tconst ControlId &id = *it.first;\n> \t\tControlValue &value = it.second;\n>\n> -\t\tswitch (ci->id()) {\n> -\t\tcase Brightness:\n> +\t\tif (id == controls::Brightness) {\n> \t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tcase Contrast:\n> +\t\t} else if (id == controls::Contrast) {\n> \t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tcase Saturation:\n> +\t\t} else if (id == controls::Saturation) {\n> \t\t\tcontrols.add(V4L2_CID_SATURATION, value.get());\n> -\t\t\tbreak;\n> -\n> -\t\tdefault:\n> -\t\t\tbreak;\n> \t\t}\n> \t}\n>\n> @@ -427,17 +419,17 @@ int VimcCameraData::init(MediaDevice *media)\n> \tfor (const auto &ctrl : controls) {\n> \t\tunsigned int v4l2Id = ctrl.first;\n> \t\tconst V4L2ControlInfo &info = ctrl.second;\n> -\t\tControlId id;\n> +\t\tconst ControlId *id = &controls::None;\n>\n> \t\tswitch (v4l2Id) {\n> \t\tcase V4L2_CID_BRIGHTNESS:\n> -\t\t\tid = Brightness;\n> +\t\t\tid = &controls::Brightness;\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_CONTRAST:\n> -\t\t\tid = Contrast;\n> +\t\t\tid = &controls::Contrast;\n> \t\t\tbreak;\n> \t\tcase V4L2_CID_SATURATION:\n> -\t\t\tid = Saturation;\n> +\t\t\tid = &controls::Saturation;\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tcontinue;\n> @@ -445,7 +437,7 @@ int VimcCameraData::init(MediaDevice *media)\n>\n> \t\tcontrolInfo_.emplace(std::piecewise_construct,\n> \t\t\t\t std::forward_as_tuple(id),\n> -\t\t\t\t std::forward_as_tuple(id, info.min(), info.max()));\n> +\t\t\t\t std::forward_as_tuple(*id, info.min(), info.max()));\n> \t}\n>\n> \treturn 0;\n> diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> index 2aba568a302e..dbc43df8e3d3 100644\n> --- a/test/controls/control_info.cpp\n> +++ b/test/controls/control_info.cpp\n> @@ -7,6 +7,7 @@\n>\n> #include \n>\n> +#include \n> #include \n>\n> #include \"test.h\"\n> @@ -23,17 +24,17 @@ protected:\n> \t\t * Test information retrieval from a control with no minimum\n> \t\t * and maximum.\n> \t\t */\n> -\t\tControlInfo info(Brightness);\n> +\t\tControlInfo brightness(controls::Brightness);\n>\n> -\t\tif (info.id() != Brightness ||\n> -\t\t info.type() != ControlTypeInteger32 ||\n> -\t\t info.name() != std::string(\"Brightness\")) {\n> +\t\tif (brightness.id() != controls::Brightness ||\n> +\t\t brightness.id().type() != ControlTypeInteger32 ||\n> +\t\t brightness.id().name() != std::string(\"Brightness\")) {\n> \t\t\tcout << \"Invalid control identification for Brightness\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (info.min().get() != 0 ||\n> -\t\t info.max().get() != 0) {\n> +\t\tif (brightness.min().get() != 0 ||\n> +\t\t brightness.max().get() != 0) {\n> \t\t\tcout << \"Invalid control range for Brightness\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -42,17 +43,17 @@ protected:\n> \t\t * Test information retrieval from a control with a minimum and\n> \t\t * a maximum value.\n> \t\t */\n> -\t\tinfo = ControlInfo(Contrast, 10, 200);\n> +\t\tControlInfo contrast(controls::Contrast, 10, 200);\n>\n> -\t\tif (info.id() != Contrast ||\n> -\t\t info.type() != ControlTypeInteger32 ||\n> -\t\t info.name() != std::string(\"Contrast\")) {\n> +\t\tif (contrast.id() != controls::Contrast ||\n> +\t\t contrast.id().type() != ControlTypeInteger32 ||\n> +\t\t contrast.id().name() != std::string(\"Contrast\")) {\n> \t\t\tcout << \"Invalid control identification for Contrast\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (info.min().get() != 10 ||\n> -\t\t info.max().get() != 200) {\n> +\t\tif (contrast.min().get() != 10 ||\n> +\t\t contrast.max().get() != 200) {\n> \t\t\tcout << \"Invalid control range for Contrast\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index 5215840b1c4e..053696814b67 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -9,6 +9,7 @@\n>\n> #include \n> #include \n> +#include \n> #include \n>\n> #include \"test.h\"\n> @@ -52,7 +53,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (list.contains(Brightness)) {\n> +\t\tif (list.contains(controls::Brightness)) {\n> \t\t\tcout << \"List should not contain Brightness control\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -70,7 +71,7 @@ protected:\n> \t\t * Set a control, and verify that the list now contains it, and\n> \t\t * nothing else.\n> \t\t */\n> -\t\tlist[Brightness] = 255;\n> +\t\tlist.set(controls::Brightness, 255);\n>\n> \t\tif (list.empty()) {\n> \t\t\tcout << \"List should not be empty\" << endl;\n> @@ -82,7 +83,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (!list.contains(Brightness)) {\n> +\t\tif (!list.contains(controls::Brightness)) {\n> \t\t\tcout << \"List should contain Brightness control\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> @@ -96,54 +97,45 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (list[Brightness].get() != 255) {\n> +\t\tif (list.get(controls::Brightness) != 255) {\n> \t\t\tcout << \"Incorrest Brightness control value\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (list.contains(Contrast)) {\n> +\t\tif (list.contains(controls::Contrast)) {\n> \t\t\tcout << \"List should not contain Contract control\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\t/*\n> -\t\t * Set a second control through ControlInfo and retrieve it\n> -\t\t * through both controlId and ControlInfo.\n> -\t\t */\n> -\t\tconst ControlInfoMap &controls = camera_->controls();\n> -\t\tconst ControlInfo *brightness = &controls.find(Brightness)->second;\n> -\t\tconst ControlInfo *contrast = &controls.find(Contrast)->second;\n> +\t\t/* Update the first control and set a second one. */\n> +\t\tlist.set(controls::Brightness, 64);\n> +\t\tlist.set(controls::Contrast, 128);\n>\n> -\t\tlist[brightness] = 64;\n> -\t\tlist[contrast] = 128;\n> -\n> -\t\tif (!list.contains(Contrast) || !list.contains(contrast)) {\n> +\t\tif (!list.contains(controls::Contrast) ||\n> +\t\t !list.contains(controls::Contrast)) {\n> \t\t\tcout << \"List should contain Contrast control\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\t/*\n> -\t\t * Test control value retrieval and update through ControlInfo.\n> -\t\t */\n> -\t\tif (list[brightness].get() != 64 ||\n> -\t\t list[contrast].get() != 128) {\n> +\t\tif (list.get(controls::Brightness) != 64 ||\n> +\t\t list.get(controls::Contrast) != 128) {\n> \t\t\tcout << \"Failed to retrieve control value\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tlist[brightness] = 10;\n> -\t\tlist[contrast] = 20;\n> +\t\t/*\n> +\t\t * Update both controls and verify that the container doesn't\n> +\t\t * grow.\n> +\t\t */\n> +\t\tlist.set(controls::Brightness, 10);\n> +\t\tlist.set(controls::Contrast, 20);\n>\n> -\t\tif (list[brightness].get() != 10 ||\n> -\t\t list[contrast].get() != 20) {\n> +\t\tif (list.get(controls::Brightness) != 10 ||\n> +\t\t list.get(controls::Contrast) != 20) {\n> \t\t\tcout << \"Failed to update control value\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\t/*\n> -\t\t * Assert that the container has not grown with the control\n> -\t\t * updated.\n> -\t\t */\n> \t\tif (list.size() != 2) {\n> \t\t\tcout << \"List should contain two elements\" << endl;\n> \t\t\treturn TestFail;\n> @@ -157,8 +149,8 @@ protected:\n> \t\t */\n> \t\tControlList newList(camera_.get());\n>\n> -\t\tnewList[Brightness] = 128;\n> -\t\tnewList[Saturation] = 255;\n> +\t\tnewList.set(controls::Brightness, 128);\n> +\t\tnewList.set(controls::Saturation, 255);\n> \t\tnewList.update(list);\n>\n> \t\tlist.clear();\n> @@ -178,16 +170,16 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (!newList.contains(Brightness) ||\n> -\t\t !newList.contains(Contrast) ||\n> -\t\t !newList.contains(Saturation)) {\n> +\t\tif (!newList.contains(controls::Brightness) ||\n> +\t\t !newList.contains(controls::Contrast) ||\n> +\t\t !newList.contains(controls::Saturation)) {\n> \t\t\tcout << \"New list contains incorrect items\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tif (newList[Brightness].get() != 10 ||\n> -\t\t newList[Contrast].get() != 20 ||\n> -\t\t newList[Saturation].get() != 255) {\n> +\t\tif (newList.get(controls::Brightness) != 10 ||\n> +\t\t newList.get(controls::Contrast) != 20 ||\n> +\t\t newList.get(controls::Saturation) != 255) {\n> \t\t\tcout << \"New list contains incorrect values\" << endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC89F60BE8\n\tfor ;\n\tSun, 29 Sep 2019 11:58:55 +0200 (CEST)","from uno.localdomain\n\t(host7-199-dynamic.171-212-r.retail.telecomitalia.it [212.171.199.7])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 51783C0002;\n\tSun, 29 Sep 2019 09:58:53 +0000 (UTC)"],"X-Originating-IP":"212.171.199.7","Date":"Sun, 29 Sep 2019 12:00:35 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190929100035.bfqtwew72jg5m3rt@uno.localdomain>","References":"<20190928152238.23752-1-laurent.pinchart@ideasonboard.com>\n\t<20190928152238.23752-5-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6degigcg4mrj6j37\"","Content-Disposition":"inline","In-Reply-To":"<20190928152238.23752-5-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: controls: Improve\n\tthe API towards applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 29 Sep 2019 09:58:56 -0000"}},{"id":2732,"web_url":"https://patchwork.libcamera.org/comment/2732/","msgid":"<20190929124524.GD4827@pendragon.ideasonboard.com>","date":"2019-09-29T12:45:24","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: controls: Improve\n\tthe API towards applications","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Sep 29, 2019 at 12:00:35PM +0200, Jacopo Mondi wrote:\n> On Sat, Sep 28, 2019 at 06:22:30PM +0300, Laurent Pinchart wrote:\n> > Rework the control-related classes to improve the API towards\n> > applications. The goal is to enable writing code similar to\n> >\n> > \tRequest *req = ...;\n> > \tControlList &controls = req->controls();\n> > \tcontrols->set(controls::AwbEnable, false);\n> > \tcontrols->set(controls::ManualExposure, 1000);\n> >\n> > \t...\n> >\n> > \tint32_t exposure = controls->get(controls::ManualExposure);\n> >\n> \n> I very much like this API!\n\nThank you :-)\n\n> > with the get and set operations ensuring type safety for the control\n> > values. This is achieved by creating the following classes:\n> >\n> > - Control defines controls and is the main way to reference a control.\n> > It is a template class to allow methods using it to refer to the\n> > control type.\n> >\n> > - ControlId is the base class of Control. It stores the control ID, name\n> > and type, and can be used in contexts where a control needs to be\n> > referenced regardless of its type (for instance in lists of controls).\n> > This class replaces ControlIdentifier.\n> >\n> > - ControlValue is kept as-is.\n> >\n> > The ControlList class now exposes two template get() and set() methods\n> > that replace the operator[]. They ensure type safety by infering the\n> > value type from the Control reference that they receive.\n> >\n> > The main way to refer to a control is now through the Control class, and\n> > optionally through its base ControlId class. The ControlId enumeration\n> > is removed, replaced by a list of global Control instances. Numerical\n> > control IDs are turned into macros, and are still exposed as they are\n> > required to communicate with IPAs (especially to deserialise control\n> > lists). They should however not be used by applications.\n> >\n> > Auto-generation of header and source files is removed for now to keep\n> > the change simple. It will be added back in the future in a more\n> > elaborate form.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/control_ids.h | 45 ++--\n> > include/libcamera/controls.h | 108 +++++++---\n> > src/libcamera/control_ids.cpp | 72 +++++++\n> > src/libcamera/controls.cpp | 317 ++++++++++++++--------------\n> > src/libcamera/gen-controls.awk | 106 ----------\n> > src/libcamera/meson.build | 11 +-\n> > src/libcamera/pipeline/uvcvideo.cpp | 40 ++--\n> > src/libcamera/pipeline/vimc.cpp | 28 +--\n> > test/controls/control_info.cpp | 25 +--\n> > test/controls/control_list.cpp | 66 +++---\n> > 10 files changed, 391 insertions(+), 427 deletions(-)\n> > create mode 100644 src/libcamera/control_ids.cpp\n> > delete mode 100755 src/libcamera/gen-controls.awk\n> >\n> > diff --git a/include/libcamera/control_ids.h b/include/libcamera/control_ids.h\n> > index 75b6a2d5cafe..81e3e2193767 100644\n> > --- a/include/libcamera/control_ids.h\n> > +++ b/include/libcamera/control_ids.h\n> > @@ -8,34 +8,31 @@\n> > #ifndef __LIBCAMERA_CONTROL_IDS_H__\n> > #define __LIBCAMERA_CONTROL_IDS_H__\n> >\n> > -#include \n> > +#include \n> \n> C++ has if I'm not mistaken\n\nIt does, and it has headers for pretty much all the standard C\nheaders. We mix and match the C and C++ headers in libcamera, which\nisn't very nice, and we should decide which ones to use. I have read an\narticle a while ago explaining while was a bad idea, but I can't\nfind it anymore, and I don't remember what the rationale was :-)\n\nIn any case I think we should decide on this globally. Would you happen\nto have any information on the subject ? I'm not against switching to\nthe C++ headers, but I think we should then use the std:: namespace\nexplicitly: this means std::printf, but also std::int32_t :-S\n\n> > +\n> > +#include \n> >\n> > namespace libcamera {\n> >\n> > -enum ControlId {\n> > -\tAwbEnable,\n> > -\tBrightness,\n> > -\tContrast,\n> > -\tSaturation,\n> > -\tManualExposure,\n> > -\tManualGain,\n> > -};\n> > +namespace controls {\n> > +\n> > +#define AWB_ENABLE\t\t1\n> > +#define BRIGHTNESS\t\t2\n> > +#define CONTRAST\t\t3\n> > +#define SATURATION\t\t4\n> > +#define MANUAL_EXPOSURE\t\t5\n> > +#define MANUAL_GAIN\t\t6\n> > +\n> > +extern const Control None;\n> > +extern const Control AwbEnable;\n> > +extern const Control Brightness;\n> > +extern const Control Contrast;\n> > +extern const Control Saturation;\n> > +extern const Control ManualExposure;\n> > +extern const Control ManualGain;\n> > +\n> > +} /* namespace controls */\n> >\n> > } /* namespace libcamera */\n> >\n> > -namespace std {\n> > -\n> > -template<>\n> > -struct hash {\n> > -\tusing argument_type = libcamera::ControlId;\n> > -\tusing result_type = std::size_t;\n> > -\n> > -\tresult_type operator()(const argument_type &key) const noexcept\n> > -\t{\n> > -\t\treturn std::hash::type>()(key);\n> > -\t}\n> > -};\n> > -\n> > -} /* namespace std */\n> > -\n> > #endif // __LIBCAMERA_CONTROL_IDS_H__\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index a3089064c4b5..9698bd1dd383 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -8,12 +8,9 @@\n> > #ifndef __LIBCAMERA_CONTROLS_H__\n> > #define __LIBCAMERA_CONTROLS_H__\n> >\n> > -#include \n> > #include \n> > #include \n> >\n> > -#include \n> > -\n> > namespace libcamera {\n> >\n> > class Camera;\n> > @@ -53,55 +50,75 @@ private:\n> > \t};\n> > };\n> >\n> > -struct ControlIdentifier {\n> > -\tControlId id;\n> > -\tconst char *name;\n> > -\tControlType type;\n> > +class ControlId\n> > +{\n> > +public:\n> > +\tunsigned int id() const { return id_; }\n> > +\tconst char *name() const { return name_; }\n> > +\tControlType type() const { return type_; }\n> > +\n> > +protected:\n> > +\tControlId(unsigned int id, const char *name, ControlType type)\n> > +\t\t: id_(id), name_(name), type_(type)\n> > +\t{\n> > +\t}\n> > +\n> > +private:\n> > +\tControlId(const ControlId &) = delete;\n> > +\tControlId &operator=(const ControlId &) = delete;\n> > +\n> > +\tunsigned int id_;\n> > +\tconst char *name_;\n> \n> Thinking about serialization, is is worth transporting the name ?\n> \n> As we discussed, probably re-introducing a global map of \n> would allow to re-construct the Control just by using the id. Of course\n> that would require linking against the file that defines the control\n> map also for closed source IPA. Could they live without the control\n> name otherwise ?\n\nThe name should only be used for debugging purpose, so we could do\nwithout it. We could even drop it completely and go for numerical IDs,\nat the cost of not so nice debugging messages. I'd rather keep the name\nfor now. IPAs that link against libcamera will benefit from it, IPAs\nthat don't will handle their own logging anyway, so nothing in them will\ndepend on us providing the name.\n\n> > +\tControlType type_;\n> > +};\n> > +\n> > +static inline bool operator==(const ControlId &lhs, const ControlId &rhs)\n> > +{\n> > +\treturn lhs.id() == rhs.id();\n> > +}\n> > +\n> > +static inline bool operator!=(const ControlId &lhs, const ControlId &rhs)\n> > +{\n> > +\treturn !(lhs == rhs);\n> > +}\n> > +\n> > +template\n> > +class Control : public ControlId\n> > +{\n> > +public:\n> > +\tusing type = T;\n> \n> This does not seems to be used.\n\nIt allows writing code such as \n\n\tBrightness::type value = 42;\n\n\tif (foo)\n\t\tvalue += 10;\n\n\tcontrols.set(Brightness, value);\n\nIt may be used in pipeline handlers and IPAs, but it's mostly for\napplications.\n\n> > +\n> > +\tControl(unsigned int id, const char *name);\n> > +\n> > +private:\n> \n> Why private ?\n\nNo reason :-) We're mixing = delete in public and private. Before C++11\nbrought = delete, the customary way to delete implicitly created\nconstructors and operators was to explicitly define them as private, so\nI suppose this is where I got the private from. Once again I don't mind\nmuch, but this should be a library-wide decision I think. What's your\npreference ?\n\n> > +\tControl(const Control &) = delete;\n> > +\tControl &operator=(const Control &) = delete;\n> > };\n> >\n> > class ControlInfo\n> > {\n> > public:\n> > -\texplicit ControlInfo(ControlId id, const ControlValue &min = 0,\n> > +\texplicit ControlInfo(const ControlId &id, const ControlValue &min = 0,\n> > \t\t\t const ControlValue &max = 0);\n> >\n> > -\tControlId id() const { return ident_->id; }\n> > -\tconst char *name() const { return ident_->name; }\n> > -\tControlType type() const { return ident_->type; }\n> > -\n> > +\tconst ControlId &id() const { return id_; }\n> > \tconst ControlValue &min() const { return min_; }\n> > \tconst ControlValue &max() const { return max_; }\n> >\n> > \tstd::string toString() const;\n> >\n> > private:\n> > -\tconst struct ControlIdentifier *ident_;\n> > +\tconst ControlId &id_;\n> > \tControlValue min_;\n> > \tControlValue max_;\n> > };\n> >\n> > -bool operator==(const ControlInfo &lhs, const ControlInfo &rhs);\n> > -bool operator==(const ControlId &lhs, const ControlInfo &rhs);\n> > -bool operator==(const ControlInfo &lhs, const ControlId &rhs);\n> > -static inline bool operator!=(const ControlInfo &lhs, const ControlInfo &rhs)\n> > -{\n> > -\treturn !(lhs == rhs);\n> > -}\n> > -static inline bool operator!=(const ControlId &lhs, const ControlInfo &rhs)\n> > -{\n> > -\treturn !(lhs == rhs);\n> > -}\n> > -static inline bool operator!=(const ControlInfo &lhs, const ControlId &rhs)\n> > -{\n> > -\treturn !(lhs == rhs);\n> > -}\n> > -\n> > -using ControlInfoMap = std::unordered_map;\n> > +using ControlInfoMap = std::unordered_map;\n> >\n> > class ControlList\n> > {\n> > private:\n> > -\tusing ControlListMap = std::unordered_map;\n> > +\tusing ControlListMap = std::unordered_map;\n> \n> I like both maps are indexed by the same key\n> \n> >\n> > public:\n> > \tControlList(Camera *camera);\n> > @@ -114,18 +131,39 @@ public:\n> > \tconst_iterator begin() const { return controls_.begin(); }\n> > \tconst_iterator end() const { return controls_.end(); }\n> >\n> > -\tbool contains(ControlId id) const;\n> > -\tbool contains(const ControlInfo *info) const;\n> > +\tbool contains(const ControlId &id) const;\n> > \tbool empty() const { return controls_.empty(); }\n> > \tstd::size_t size() const { return controls_.size(); }\n> > \tvoid clear() { controls_.clear(); }\n> >\n> > -\tControlValue &operator[](ControlId id);\n> > -\tControlValue &operator[](const ControlInfo *info) { return controls_[info]; }\n> > +\ttemplate\n> > +\tconst T &get(const Control &ctrl) const\n> > +\t{\n> > +\t\tconst ControlValue *val = find(ctrl);\n> > +\t\tif (!val) {\n> > +\t\t\tstatic T t(0);\n> > +\t\t\treturn t;\n> > +\t\t}\n> > +\n> > +\t\treturn val->get();\n> > +\t}\n> > +\n> > +\ttemplate\n> > +\tvoid set(const Control &ctrl, const T &value)\n> > +\t{\n> > +\t\tControlValue *val = find(ctrl);\n> > +\t\tif (!val)\n> > +\t\t\treturn;\n> > +\n> > +\t\tval->set(value);\n> > +\t}\n> \n> In terms of generated code this does not make any difference, but as\n> these functions are inlinded in the class definition, I would\n> \n> \tconst T &get(const Control &ctrl) const\n> \t{\n> \t\tconst ControlValue *val = find(ctrl);\n> \t\treturn val ? static T t(0) :\n> \t\t val->get();\n> \t}\n> \n> \ttemplate\n> \tvoid set(const Control &ctrl, const T &value)\n> \t{\n> \t\tControlValue *val = find(ctrl);\n> \t\tif (val)\n> \t\t val->set(value);\n> \t}\n> \n> To shorten them by a few lines (be aware, not compiled)\n\nI don't mind the second one, but the first one doesn't look very nice\n:-S\n\n> >\n> > \tvoid update(const ControlList &list);\n> >\n> > private:\n> > +\tconst ControlValue *find(const ControlId &id) const;\n> > +\tControlValue *find(const ControlId &id);\n> > +\n> > \tCamera *camera_;\n> > \tControlListMap controls_;\n> > };\n> > diff --git a/src/libcamera/control_ids.cpp b/src/libcamera/control_ids.cpp\n> > new file mode 100644\n> > index 000000000000..7e4e38b572f0\n> > --- /dev/null\n> > +++ b/src/libcamera/control_ids.cpp\n> > @@ -0,0 +1,72 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * control_ids.cpp : Control ID list\n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file control_ids.h\n> > + * \\brief Camera control identifiers\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace controls {\n> > +\n> > +/**\n> > + * \\def AWB_ENABLE\n> > + * \\brief Numerical value of the AwbEnable control\n> > + * \\def BRIGHTNESS\n> > + * \\brief Numerical value of the Brightness control\n> > + * \\def CONTRAST\n> > + * \\brief Numerical value of the Contrast control\n> > + * \\def SATURATION\n> > + * \\brief Numerical value of the Saturation control\n> > + * \\def MANUAL_EXPOSURE\n> > + * \\brief Numerical value of the ManualExposure control\n> > + * \\def MANUAL_GAIN\n> > + * \\brief Numerical value of the ManualGain control\n> > + */\n> > +\n> > +/**\n> > + * \\brief Invalid control\n> > + */\n> > +extern const Control None(0, \"none\");\n> > +\n> > +/**\n> > + * \\brief Enables or disables the AWB.\n> > + * \\sa ManualGain\n> > + */\n> > +extern const Control AwbEnable(AWB_ENABLE, \"AwbEnable\");\n> > +\n> > +/**\n> > + * \\brief Specify a fixed brightness parameter.\n> > + */\n> > +extern const Control Brightness(BRIGHTNESS, \"Brightness\");\n> > +\n> > +/**\n> > + * \\brief Specify a fixed contrast parameter.\n> > + */\n> > +extern const Control Contrast(CONTRAST, \"Contrast\");\n> > +\n> > +/**\n> > + * \\brief Specify a fixed saturation parameter.\n> > + */\n> > +extern const Control Saturation(SATURATION, \"Saturation\");\n> > +\n> > +/**\n> > + * \\brief Specify a fixed exposure time in milli-seconds\n> > + */\n> > +extern const Control ManualExposure(MANUAL_EXPOSURE, \"ManualExposure\");\n> > +\n> > +/**\n> > + * \\brief Specify a fixed gain parameter\n> > + */\n> > +extern const Control ManualGain(MANUAL_GAIN, \"ManualGain\");\n> > +\n> > +} /* namespace controls */\n> > +\n> > +} /* namespace libcamera */\n> \n> We'll really need to expand this documentation soon :(\n\nAbsolutely, and I think we need to generate control_ids.h and\ncontrol_ids.cpp from a single source. I'm toying with the idea of\ndescribing all controls using yaml.\n\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 295ccd55a622..8886660b7617 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -18,6 +18,29 @@\n> > /**\n> > * \\file controls.h\n> > * \\brief Describes control framework and controls supported by a camera\n> > + *\n> > + * A control is a mean to govern or influence the operation of a camera. Every\n> > + * control is defined by a unique numerical ID, a name string and the data type\n> > + * of the value it stores. The libcamera API defines a set of standard controls\n> > + * in the libcamera::controls namespace, as a set of instances of the Control\n> > + * class.\n> > + *\n> > + * The main way for applications to interact with controls is through the\n> > + * ControlList stored in the Request class:\n> > + *\n> > + * \\code{.cpp}\n> > + * Request *req = ...;\n> > + * ControlList &controls = req->controls();\n> > + * controls->set(controls::AwbEnable, false);\n> > + * controls->set(controls::ManualExposure, 1000);\n> > + *\n> > + * ...\n> > + *\n> > + * int32_t exposure = controls->get(controls::ManualExposure);\n> > + * \\endcode\n> > + *\n> > + * The ControlList::get() and ControlList::set() methods automatically deduce\n> > + * the data type based on the control.\n> > */\n> >\n> > namespace libcamera {\n> > @@ -173,76 +196,120 @@ std::string ControlValue::toString() const\n> > }\n> >\n> > /**\n> > - * \\enum ControlId\n> > - * \\brief Numerical control ID\n> > + * \\class ControlId\n> > + * \\brief Control static metadata\n> > + *\n> > + * The ControlId class stores a control ID, name and data type. It provides\n> > + * unique identification of a control, but without support for compile-time\n> > + * type deduction that the derived template Control class supports. See the\n> > + * Control class for more information.\n> > */\n> >\n> > /**\n> > - * \\var AwbEnable\n> > - * ControlType: Bool\n> > - *\n> > - * Enables or disables the AWB. See also \\a libcamera::ControlId::ManualGain\n> > + * \\fn ControlId::ControlId(unsigned int id, const char *name, ControlType type)\n> > + * \\brief Construct a ControlId instance\n> > + * \\param[in] id The control numerical ID\n> > + * \\param[in] name The control name\n> > + * \\param[in] type The control data type\n> > */\n> >\n> > /**\n> > - * \\var Brightness\n> > - * ControlType: Integer32\n> > - *\n> > - * Specify a fixed brightness parameter.\n> > + * \\fn unsigned int ControlId::id() const\n> > + * \\brief Retrieve the control numerical ID\n> > + * \\return The control numerical ID\n> > */\n> >\n> > /**\n> > - * \\var Contrast\n> > - * ControlType: Integer32\n> > - *\n> > - * Specify a fixed contrast parameter.\n> > + * \\fn const char *ControlId::name() const\n> > + * \\brief Retrieve the control name\n> > + * \\return The control name\n> > */\n> >\n> > /**\n> > - * \\var Saturation\n> > - * ControlType: Integer32\n> > - *\n> > - * Specify a fixed saturation parameter.\n> > + * \\fn ControlType ControlId::type() const\n> > + * \\brief Retrieve the control data type\n> > + * \\return The control data type\n> > */\n> >\n> > /**\n> > - * \\var ManualExposure\n> > - * ControlType: Integer32\n> > + * \\fn bool operator==(const ControlId &lhs, const ControlId &rhs)\n> > + * \\brief Compare two ControlId instances for equality\n> > + * \\param[in] lhs Left-hand side ControlId\n> > + * \\param[in] rhs Left-hand side ControlId\n> \n> Right-hand\n\nGood catch :-)\n\n> > + *\n> > + * ControlId instances are compared based on the numerical ControlId::id()\n> > + * only, as an object may not have two separate controls with the same\n> > + * numerical ID.\n> > *\n> > - * Specify a fixed exposure time in milli-seconds\n> > + * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> \n> \\return True if \\a lhs and \\a rhs have the same control Id, false\n> otherwise\n> \n> ?\n\nOK\n\n> > */\n> >\n> > /**\n> > - * \\var ManualGain\n> > - * ControlType: Integer32\n> > + * \\class Control\n> > + * \\brief Describe a control and its intrinsic properties\n> > + *\n> > + * The Control class models a control exposed by a camera. Its template type\n> > + * name T refers to the control data type, and allows methods that operate on\n> > + * control values to be defined as template methods using the same type T for\n> > + * the control value. See for instance how the ControlList::get() method\n> > + * returns a value corresponding to the type of the requested control.\n> > + *\n> > + * While this class is the main mean to refer to a control, the control\n> > + * identifying information are stored in the non-template base ControlId class.\n> > + * This allows code that operates on a set of controls of different types to\n> > + * reference those controls through a ControlId instead of a Control. For\n> > + * instance, the list of controls supported by a camera is exposed as ControlId\n> > + * instead of Control.\n> > *\n> > - * Specify a fixed gain parameter\n> > + * While controls of any type can be defined through template specialisation,\n> > + * libcamera only supports the bool, int32_t and int64_t types natively (this\n> > + * includes types that are equivalent to the supported types, such as int and\n> > + * long int).\n> \n> This is just temporary, would you like to document this?\n\nI thought it was useful to mention the equivalence between int32_t and\nint, and int64_t and long int somewhere. What do you mean by \"is just\ntemporary\" ?\n\n> > + *\n> > + * Controls IDs shall be unique. While nothing prevents multiple instances of\n> > + * the Control class to be created with the same ID, this may lead to undefined\n> > + * behaviour.\n> > */\n> >\n> > /**\n> > - * \\struct ControlIdentifier\n> > - * \\brief Describe a ControlId with control specific constant meta-data\n> > - *\n> > - * Defines a Control with a unique ID, a name, and a type.\n> > - * This structure is used as static part of the auto-generated control\n> > - * definitions, which are generated from the ControlId documentation.\n> > + * \\fn Control::Control(unsigned int id, const char *name)\n> > + * \\brief Construct a Control instance\n> > + * \\param[in] id The control numerical ID\n> > + * \\param[in] name The control name\n> > *\n> > - * \\var ControlIdentifier::id\n> > - * The unique ID for a control\n> > - * \\var ControlIdentifier::name\n> > - * The string representation of the control\n> > - * \\var ControlIdentifier::type\n> > - * The ValueType required to represent the control value\n> > + * The control data type is automatically deduced from the template type T.\n> > */\n> >\n> > -/*\n> > - * The controlTypes are automatically generated to produce a control_types.cpp\n> > - * output. This file is not for public use, and so no suitable header exists\n> > - * for this sole usage of the controlTypes reference. As such the extern is\n> > - * only defined here for use during the ControlInfo constructor and should not\n> > - * be referenced directly elsewhere.\n> > +/**\n> > + * \\typedef Control::type\n> > + * \\brief The Control template type T\n> > */\n> > -extern const std::unordered_map controlTypes;\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template<>\n> > +Control::Control(unsigned int id, const char *name)\n> > +\t: ControlId(id, name, ControlTypeNone)\n> > +{\n> > +}\n> > +\n> > +template<>\n> > +Control::Control(unsigned int id, const char *name)\n> > +\t: ControlId(id, name, ControlTypeBool)\n> > +{\n> > +}\n> > +\n> > +template<>\n> > +Control::Control(unsigned int id, const char *name)\n> > +\t: ControlId(id, name, ControlTypeInteger32)\n> > +{\n> > +}\n> > +\n> > +template<>\n> > +Control::Control(unsigned int id, const char *name)\n> > +\t: ControlId(id, name, ControlTypeInteger64)\n> > +{\n> > +}\n> > +#endif /* __DOXYGEN__ */\n> >\n> > /**\n> > * \\class ControlInfo\n> > @@ -260,17 +327,10 @@ extern const std::unordered_map controlTypes;\n> > * \\param[in] min The control minimum value\n> > * \\param[in] max The control maximum value\n> > */\n> > -ControlInfo::ControlInfo(ControlId id, const ControlValue &min,\n> > +ControlInfo::ControlInfo(const ControlId &id, const ControlValue &min,\n> > \t\t\t const ControlValue &max)\n> > -\t: min_(min), max_(max)\n> > +\t: id_(id), min_(min), max_(max)\n> > {\n> > -\tauto iter = controlTypes.find(id);\n> > -\tif (iter == controlTypes.end()) {\n> > -\t\tLOG(Controls, Fatal) << \"Attempt to create invalid ControlInfo\";\n> > -\t\treturn;\n> > -\t}\n> > -\n> > -\tident_ = &iter->second;\n> > }\n> >\n> > /**\n> > @@ -279,18 +339,6 @@ ControlInfo::ControlInfo(ControlId id, const ControlValue &min,\n> > * \\return The control ID\n> > */\n> >\n> > -/**\n> > - * \\fn ControlInfo::name()\n> > - * \\brief Retrieve the control name string\n> > - * \\return The control name string\n> > - */\n> > -\n> > -/**\n> > - * \\fn ControlInfo::type()\n> > - * \\brief Retrieve the control data type\n> > - * \\return The control data type\n> > - */\n> > -\n> > /**\n> > * \\fn ControlInfo::min()\n> > * \\brief Retrieve the minimum value of the control\n> > @@ -310,56 +358,11 @@ std::string ControlInfo::toString() const\n> > {\n> > \tstd::stringstream ss;\n> >\n> > -\tss << name() << \"[\" << min_.toString() << \"..\" << max_.toString() << \"]\";\n> > +\tss << id_.name() << \"[\" << min_.toString() << \"..\" << max_.toString() << \"]\";\n> >\n> > \treturn ss.str();\n> > }\n> >\n> > -/**\n> > - * \\brief Compare control information for equality\n> > - * \\param[in] lhs Left-hand side control information\n> > - * \\param[in] rhs Right-hand side control information\n> > - *\n> > - * Control information is compared based on the ID only, as a camera may not\n> > - * have two separate controls with the same ID.\n> > - *\n> > - * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> > - */\n> > -bool operator==(const ControlInfo &lhs, const ControlInfo &rhs)\n> > -{\n> > -\treturn lhs.id() == rhs.id();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Compare control ID and information for equality\n> > - * \\param[in] lhs Left-hand side control identifier\n> > - * \\param[in] rhs Right-hand side control information\n> > - *\n> > - * Control information is compared based on the ID only, as a camera may not\n> > - * have two separate controls with the same ID.\n> > - *\n> > - * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> > - */\n> > -bool operator==(const ControlId &lhs, const ControlInfo &rhs)\n> > -{\n> > -\treturn lhs == rhs.id();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Compare control information and ID for equality\n> > - * \\param[in] lhs Left-hand side control information\n> > - * \\param[in] rhs Right-hand side control identifier\n> > - *\n> > - * Control information is compared based on the ID only, as a camera may not\n> > - * have two separate controls with the same ID.\n> > - *\n> > - * \\return True if \\a lhs and \\a rhs are equal, false otherwise\n> > - */\n> > -bool operator==(const ControlInfo &lhs, const ControlId &rhs)\n> > -{\n> > -\treturn lhs.id() == rhs;\n> > -}\n> > -\n> > /**\n> > * \\typedef ControlInfoMap\n> > * \\brief A map of ControlId to ControlInfo\n> > @@ -431,29 +434,9 @@ ControlList::ControlList(Camera *camera)\n> > *\n> > * \\return True if the list contains a matching control, false otherwise\n> > */\n> > -bool ControlList::contains(ControlId id) const\n> > +bool ControlList::contains(const ControlId &id) const\n> > {\n> > -\tconst ControlInfoMap &controls = camera_->controls();\n> > -\tconst auto iter = controls.find(id);\n> > -\tif (iter == controls.end()) {\n> > -\t\tLOG(Controls, Error)\n> > -\t\t\t<< \"Camera \" << camera_->name()\n> > -\t\t\t<< \" does not support control \" << id;\n> > -\n> > -\t\treturn false;\n> > -\t}\n> > -\n> > -\treturn controls_.find(&iter->second) != controls_.end();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Check if the list contains a control with the specified \\a info\n> > - * \\param[in] info The control info\n> > - * \\return True if the list contains a matching control, false otherwise\n> > - */\n> > -bool ControlList::contains(const ControlInfo *info) const\n> > -{\n> > -\treturn controls_.find(info) != controls_.end();\n> > +\treturn controls_.find(&id) != controls_.end();\n> > }\n> >\n> > /**\n> > @@ -474,44 +457,60 @@ bool ControlList::contains(const ControlInfo *info) const\n> > */\n> >\n> > /**\n> > - * \\brief Access or insert the control specified by \\a id\n> > - * \\param[in] id The control ID\n> > + * \\fn template const T &ControlList::get() const\n> > + * \\brief Get the value of a control\n> > + * \\param[in] ctrl The control\n> > *\n> > - * This method returns a reference to the control identified by \\a id, inserting\n> > - * it in the list if the ID is not already present.\n> > + * The behaviour is undefined if the control \\a ctrl is not present in the\n> > + * list. Use ControlList::contains() to test for the presence of a control in\n> > + * the list before retrieving its value.\n> \n> Is it undefined? An empty control is returned if I read things right\n\nUndefined behaviour means that applications can't rely on the behaviour,\nnot that it's random :-) We could replace the return with an assert(),\nand applications won't be allowed to complain.\n\nhttps://en.wikipedia.org/wiki/Undefined_behavior\n\n> > *\n> > - * The behaviour is undefined if the control \\a id is not supported by the\n> > - * camera that the ControlList refers to.\n> > + * The control value type shall match the type T, otherwise the behaviour is\n> > + * undefined.\n> > *\n> > - * \\return A reference to the value of the control identified by \\a id\n> > + * \\return The control value\n> > */\n> > -ControlValue &ControlList::operator[](ControlId id)\n> > +\n> > +/**\n> > + * \\fn template void ControlList::set()\n> > + * \\brief Set the control value to \\a value\n> > + * \\param[in] ctrl The control\n> > + * \\param[in] value The control value\n> > + *\n> > + * This method sets the value of a control in the control list. If the control\n> > + * is already present in the list, its value is updated.\n> \n> What about clearly stating the control is added if not present?\n\nI'll reword this.\n\n> > + *\n> > + * The behaviour is undefined if the control \\a ctrl is not supported by the\n> > + * camera that the list refers to.\n> \n> It seems to me it just gets ignored\n\nSame explanation as above, it's still undefined.\n\n> > + */\n> > +\n> > +const ControlValue *ControlList::find(const ControlId &id) const\n> > +{\n> > +\tconst auto iter = controls_.find(&id);\n> > +\tif (iter == controls_.end()) {\n> > +\t\tLOG(Controls, Error)\n> > +\t\t\t<< \"Control \" << id.name() << \" not found\";\n> > +\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\treturn &iter->second;\n> > +}\n> > +\n> > +ControlValue *ControlList::find(const ControlId &id)\n> > {\n> > \tconst ControlInfoMap &controls = camera_->controls();\n> > -\tconst auto iter = controls.find(id);\n> > +\tconst auto iter = controls.find(&id);\n> > \tif (iter == controls.end()) {\n> > \t\tLOG(Controls, Error)\n> > \t\t\t<< \"Camera \" << camera_->name()\n> > -\t\t\t<< \" does not support control \" << id;\n> > -\n> > -\t\tstatic ControlValue empty;\n> > -\t\treturn empty;\n> > +\t\t\t<< \" does not support control \" << id.name();\n> > +\t\treturn nullptr;\n> > \t}\n> >\n> > -\treturn controls_[&iter->second];\n> > +\treturn &controls_[&id];\n> \n> Double lookup ? Couldn't you re-use iter ?\n\niter is for controls, this is controls_.\n\n> > }\n> >\n> > -/**\n> > - * \\fn ControlList::operator[](const ControlInfo *info)\n> > - * \\brief Access or insert the control specified by \\a info\n> > - * \\param[in] info The control info\n> > - *\n> > - * This method returns a reference to the control identified by \\a info,\n> > - * inserting it in the list if the info is not already present.\n> > - *\n> > - * \\return A reference to the value of the control identified by \\a info\n> > - */\n> > -\n> > /**\n> > * \\brief Update the list with a union of itself and \\a other\n> > * \\param other The other list\n> > @@ -533,10 +532,10 @@ void ControlList::update(const ControlList &other)\n> > \t}\n> >\n> > \tfor (auto it : other) {\n> > -\t\tconst ControlInfo *info = it.first;\n> > +\t\tconst ControlId *id = it.first;\n> > \t\tconst ControlValue &value = it.second;\n> >\n> > -\t\tcontrols_[info] = value;\n> > +\t\tcontrols_[id] = value;\n> > \t}\n> > }\n> >\n> > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk\n> > deleted file mode 100755\n> > index a3f291e7071c..000000000000\n> > --- a/src/libcamera/gen-controls.awk\n> > +++ /dev/null\n> > @@ -1,106 +0,0 @@\n> > -#!/usr/bin/awk -f\n> > -\n> > -# SPDX-License-Identifier: LGPL-2.1-or-later\n> > -\n> > -# Controls are documented using Doxygen in the main controls.cpp source.\n> > -#\n> > -# Generate control tables directly from the documentation, creating enumerations\n> > -# to support the IDs and static type information regarding each control.\n> > -\n> > -BEGIN {\n> > -\tid=0\n> > -\tinput=ARGV[1]\n> > -\tmode=ARGV[2]\n> > -\toutput=ARGV[3]\n> > -}\n> > -\n> > -# Detect Doxygen style comment blocks and ignore other lines\n> > -/^\\/\\*\\*$/ { in_doxygen=1; first_line=1; next }\n> > -// { if (!in_doxygen) next }\n> > -\n> > -# Entry point for the Control Documentation\n> > -/ * \\\\enum ControlId$/ { in_controls=1; first_line=0; next }\n> > -// { if (!in_controls) next }\n> > -\n> > -# Extract control information\n> > -/ \\* \\\\var/ { names[++id]=$3; first_line=0; next }\n> > -/ \\* ControlType:/ { types[id] = $3 }\n> > -\n> > -# End of comment blocks\n> > -/^ \\*\\// { in_doxygen=0 }\n> > -\n> > -# Identify the end of controls\n> > -/^ \\* \\\\/ { if (first_line) exit }\n> > -// { first_line=0 }\n> > -\n> > -################################\n> > -# Support output file generation\n> > -\n> > -function basename(file) {\n> > -\tsub(\".*/\", \"\", file)\n> > -\treturn file\n> > -}\n> > -\n> > -function Header(file, description) {\n> > -\tprint \"/* SPDX-License-Identifier: LGPL-2.1-or-later */\" > file\n> > -\tprint \"/*\" > file\n> > -\tprint \" * Copyright (C) 2019, Google Inc.\" > file\n> > -\tprint \" *\" > file\n> > -\tprint \" * \" basename(file) \" - \" description > file\n> > -\tprint \" *\" > file\n> > -\tprint \" * This file is auto-generated. Do not edit.\" > file\n> > -\tprint \" */\" > file\n> > -\tprint \"\" > file\n> > -}\n> > -\n> > -function EnterNameSpace(file) {\n> > -\tprint \"namespace libcamera {\" > file\n> > -\tprint \"\" > file\n> > -}\n> > -\n> > -function ExitNameSpace(file) {\n> > -\tprint \"\" > file\n> > -\tprint \"} /* namespace libcamera */\" > file\n> > -}\n> > -\n> > -function GenerateHeader(file) {\n> > -\tHeader(file, \"Control ID list\")\n> > -\n> > -\tprint \"#ifndef __LIBCAMERA_CONTROL_IDS_H__\" > file\n> > -\tprint \"#define __LIBCAMERA_CONTROL_IDS_H__\" > file\n> > -\tprint \"\" > file\n> > -\n> > -\tEnterNameSpace(file)\n> > -\tprint \"enum ControlId {\" > file\n> > -\tfor (i=1; i <= id; ++i) {\n> > -\t\tprintf \"\\t%s,\\n\", names[i] > file\n> > -\t}\n> > -\tprint \"};\" > file\n> > -\tExitNameSpace(file)\n> > -\n> > -\tprint \"\" > file\n> > -\tprint \"#endif // __LIBCAMERA_CONTROL_IDS_H__\" > file\n> > -}\n> > -\n> > -function GenerateTable(file) {\n> > -\tHeader(file, \"Control types\")\n> > -\tprint \"#include \" > file\n> > -\tprint \"\" > file\n> > -\n> > -\tEnterNameSpace(file)\n> > -\n> > -\tprint \"extern const std::unordered_map\" > file\n> > -\tprint \"controlTypes {\" > file\n> > -\tfor (i=1; i <= id; ++i) {\n> > -\t\tprintf \"\\t{ %s, { %s, \\\"%s\\\", ControlType%s } },\\n\", names[i], names[i], names[i], types[i] > file\n> > -\t}\n> > -\tprint \"};\" > file\n> > -\tExitNameSpace(file)\n> > -}\n> > -\n> > -END {\n> > -\tif (mode == \"--header\")\n> > -\t\tGenerateHeader(output)\n> > -\telse\n> > -\t\tGenerateTable(output)\n> > -}\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 755149c55c7b..8123d1d5bee9 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -5,6 +5,7 @@ libcamera_sources = files([\n> > 'camera_manager.cpp',\n> > 'camera_sensor.cpp',\n> > 'controls.cpp',\n> > + 'control_ids.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'event_dispatcher.cpp',\n> > @@ -57,16 +58,6 @@ if libudev.found()\n> > ])\n> > endif\n> >\n> > -gen_controls = files('gen-controls.awk')\n> > -\n> > -control_types_cpp = custom_target('control_types_cpp',\n> > - input : files('controls.cpp'),\n> > - output : 'control_types.cpp',\n> > - depend_files : gen_controls,\n> > - command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@'])\n> > -\n> > -libcamera_sources += control_types_cpp\n> > -\n> > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')\n> >\n> > version_cpp = vcs_tag(command : [gen_version, meson.build_root()],\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index 0d56758e3e1d..44e19f40f1b9 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -10,6 +10,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -230,33 +231,20 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \tV4L2ControlList controls;\n> >\n> > \tfor (auto it : request->controls()) {\n> > -\t\tconst ControlInfo *ci = it.first;\n> > +\t\tconst ControlId &id = *it.first;\n> > \t\tControlValue &value = it.second;\n> >\n> > -\t\tswitch (ci->id()) {\n> > -\t\tcase Brightness:\n> > +\t\tif (id == controls::Brightness) {\n> > \t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase Contrast:\n> > +\t\t} else if (id == controls::Contrast) {\n> > \t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase Saturation:\n> > +\t\t} else if (id == controls::Saturation) {\n> > \t\t\tcontrols.add(V4L2_CID_SATURATION, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase ManualExposure:\n> > +\t\t} else if (id == controls::ManualExposure) {\n> > \t\t\tcontrols.add(V4L2_CID_EXPOSURE_AUTO, 1);\n> > \t\t\tcontrols.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase ManualGain:\n> > +\t\t} else if (id == controls::ManualGain) {\n> > \t\t\tcontrols.add(V4L2_CID_GAIN, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tdefault:\n> > -\t\t\tbreak;\n> > \t\t}\n> > \t}\n> >\n> > @@ -352,23 +340,23 @@ int UVCCameraData::init(MediaEntity *entity)\n> > \tfor (const auto &ctrl : controls) {\n> > \t\tunsigned int v4l2Id = ctrl.first;\n> > \t\tconst V4L2ControlInfo &info = ctrl.second;\n> > -\t\tControlId id;\n> > +\t\tconst ControlId *id = &controls::None;\n> >\n> > \t\tswitch (v4l2Id) {\n> > \t\tcase V4L2_CID_BRIGHTNESS:\n> > -\t\t\tid = Brightness;\n> > +\t\t\tid = &controls::Brightness;\n> > \t\t\tbreak;\n> > \t\tcase V4L2_CID_CONTRAST:\n> > -\t\t\tid = Contrast;\n> > +\t\t\tid = &controls::Contrast;\n> > \t\t\tbreak;\n> > \t\tcase V4L2_CID_SATURATION:\n> > -\t\t\tid = Saturation;\n> > +\t\t\tid = &controls::Saturation;\n> > \t\t\tbreak;\n> > \t\tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > -\t\t\tid = ManualExposure;\n> > +\t\t\tid = &controls::ManualExposure;\n> > \t\t\tbreak;\n> > \t\tcase V4L2_CID_GAIN:\n> > -\t\t\tid = ManualGain;\n> > +\t\t\tid = &controls::ManualGain;\n> > \t\t\tbreak;\n> > \t\tdefault:\n> > \t\t\tcontinue;\n> > @@ -376,7 +364,7 @@ int UVCCameraData::init(MediaEntity *entity)\n> >\n> > \t\tcontrolInfo_.emplace(std::piecewise_construct,\n> > \t\t\t\t std::forward_as_tuple(id),\n> > -\t\t\t\t std::forward_as_tuple(id, info.min(), info.max()));\n> > +\t\t\t\t std::forward_as_tuple(*id, info.min(), info.max()));\n> > \t}\n> >\n> > \treturn 0;\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index e549dc64b996..a48febf2172e 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -283,24 +284,15 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\n> > \tV4L2ControlList controls;\n> >\n> > \tfor (auto it : request->controls()) {\n> > -\t\tconst ControlInfo *ci = it.first;\n> > +\t\tconst ControlId &id = *it.first;\n> > \t\tControlValue &value = it.second;\n> >\n> > -\t\tswitch (ci->id()) {\n> > -\t\tcase Brightness:\n> > +\t\tif (id == controls::Brightness) {\n> > \t\t\tcontrols.add(V4L2_CID_BRIGHTNESS, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase Contrast:\n> > +\t\t} else if (id == controls::Contrast) {\n> > \t\t\tcontrols.add(V4L2_CID_CONTRAST, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tcase Saturation:\n> > +\t\t} else if (id == controls::Saturation) {\n> > \t\t\tcontrols.add(V4L2_CID_SATURATION, value.get());\n> > -\t\t\tbreak;\n> > -\n> > -\t\tdefault:\n> > -\t\t\tbreak;\n> > \t\t}\n> > \t}\n> >\n> > @@ -427,17 +419,17 @@ int VimcCameraData::init(MediaDevice *media)\n> > \tfor (const auto &ctrl : controls) {\n> > \t\tunsigned int v4l2Id = ctrl.first;\n> > \t\tconst V4L2ControlInfo &info = ctrl.second;\n> > -\t\tControlId id;\n> > +\t\tconst ControlId *id = &controls::None;\n> >\n> > \t\tswitch (v4l2Id) {\n> > \t\tcase V4L2_CID_BRIGHTNESS:\n> > -\t\t\tid = Brightness;\n> > +\t\t\tid = &controls::Brightness;\n> > \t\t\tbreak;\n> > \t\tcase V4L2_CID_CONTRAST:\n> > -\t\t\tid = Contrast;\n> > +\t\t\tid = &controls::Contrast;\n> > \t\t\tbreak;\n> > \t\tcase V4L2_CID_SATURATION:\n> > -\t\t\tid = Saturation;\n> > +\t\t\tid = &controls::Saturation;\n> > \t\t\tbreak;\n> > \t\tdefault:\n> > \t\t\tcontinue;\n> > @@ -445,7 +437,7 @@ int VimcCameraData::init(MediaDevice *media)\n> >\n> > \t\tcontrolInfo_.emplace(std::piecewise_construct,\n> > \t\t\t\t std::forward_as_tuple(id),\n> > -\t\t\t\t std::forward_as_tuple(id, info.min(), info.max()));\n> > +\t\t\t\t std::forward_as_tuple(*id, info.min(), info.max()));\n> > \t}\n> >\n> > \treturn 0;\n> > diff --git a/test/controls/control_info.cpp b/test/controls/control_info.cpp\n> > index 2aba568a302e..dbc43df8e3d3 100644\n> > --- a/test/controls/control_info.cpp\n> > +++ b/test/controls/control_info.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \"test.h\"\n> > @@ -23,17 +24,17 @@ protected:\n> > \t\t * Test information retrieval from a control with no minimum\n> > \t\t * and maximum.\n> > \t\t */\n> > -\t\tControlInfo info(Brightness);\n> > +\t\tControlInfo brightness(controls::Brightness);\n> >\n> > -\t\tif (info.id() != Brightness ||\n> > -\t\t info.type() != ControlTypeInteger32 ||\n> > -\t\t info.name() != std::string(\"Brightness\")) {\n> > +\t\tif (brightness.id() != controls::Brightness ||\n> > +\t\t brightness.id().type() != ControlTypeInteger32 ||\n> > +\t\t brightness.id().name() != std::string(\"Brightness\")) {\n> > \t\t\tcout << \"Invalid control identification for Brightness\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (info.min().get() != 0 ||\n> > -\t\t info.max().get() != 0) {\n> > +\t\tif (brightness.min().get() != 0 ||\n> > +\t\t brightness.max().get() != 0) {\n> > \t\t\tcout << \"Invalid control range for Brightness\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > @@ -42,17 +43,17 @@ protected:\n> > \t\t * Test information retrieval from a control with a minimum and\n> > \t\t * a maximum value.\n> > \t\t */\n> > -\t\tinfo = ControlInfo(Contrast, 10, 200);\n> > +\t\tControlInfo contrast(controls::Contrast, 10, 200);\n> >\n> > -\t\tif (info.id() != Contrast ||\n> > -\t\t info.type() != ControlTypeInteger32 ||\n> > -\t\t info.name() != std::string(\"Contrast\")) {\n> > +\t\tif (contrast.id() != controls::Contrast ||\n> > +\t\t contrast.id().type() != ControlTypeInteger32 ||\n> > +\t\t contrast.id().name() != std::string(\"Contrast\")) {\n> > \t\t\tcout << \"Invalid control identification for Contrast\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (info.min().get() != 10 ||\n> > -\t\t info.max().get() != 200) {\n> > +\t\tif (contrast.min().get() != 10 ||\n> > +\t\t contrast.max().get() != 200) {\n> > \t\t\tcout << \"Invalid control range for Contrast\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > index 5215840b1c4e..053696814b67 100644\n> > --- a/test/controls/control_list.cpp\n> > +++ b/test/controls/control_list.cpp\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"test.h\"\n> > @@ -52,7 +53,7 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (list.contains(Brightness)) {\n> > +\t\tif (list.contains(controls::Brightness)) {\n> > \t\t\tcout << \"List should not contain Brightness control\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > @@ -70,7 +71,7 @@ protected:\n> > \t\t * Set a control, and verify that the list now contains it, and\n> > \t\t * nothing else.\n> > \t\t */\n> > -\t\tlist[Brightness] = 255;\n> > +\t\tlist.set(controls::Brightness, 255);\n> >\n> > \t\tif (list.empty()) {\n> > \t\t\tcout << \"List should not be empty\" << endl;\n> > @@ -82,7 +83,7 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (!list.contains(Brightness)) {\n> > +\t\tif (!list.contains(controls::Brightness)) {\n> > \t\t\tcout << \"List should contain Brightness control\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > @@ -96,54 +97,45 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (list[Brightness].get() != 255) {\n> > +\t\tif (list.get(controls::Brightness) != 255) {\n> > \t\t\tcout << \"Incorrest Brightness control value\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (list.contains(Contrast)) {\n> > +\t\tif (list.contains(controls::Contrast)) {\n> > \t\t\tcout << \"List should not contain Contract control\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\t/*\n> > -\t\t * Set a second control through ControlInfo and retrieve it\n> > -\t\t * through both controlId and ControlInfo.\n> > -\t\t */\n> > -\t\tconst ControlInfoMap &controls = camera_->controls();\n> > -\t\tconst ControlInfo *brightness = &controls.find(Brightness)->second;\n> > -\t\tconst ControlInfo *contrast = &controls.find(Contrast)->second;\n> > +\t\t/* Update the first control and set a second one. */\n> > +\t\tlist.set(controls::Brightness, 64);\n> > +\t\tlist.set(controls::Contrast, 128);\n> >\n> > -\t\tlist[brightness] = 64;\n> > -\t\tlist[contrast] = 128;\n> > -\n> > -\t\tif (!list.contains(Contrast) || !list.contains(contrast)) {\n> > +\t\tif (!list.contains(controls::Contrast) ||\n> > +\t\t !list.contains(controls::Contrast)) {\n> > \t\t\tcout << \"List should contain Contrast control\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\t/*\n> > -\t\t * Test control value retrieval and update through ControlInfo.\n> > -\t\t */\n> > -\t\tif (list[brightness].get() != 64 ||\n> > -\t\t list[contrast].get() != 128) {\n> > +\t\tif (list.get(controls::Brightness) != 64 ||\n> > +\t\t list.get(controls::Contrast) != 128) {\n> > \t\t\tcout << \"Failed to retrieve control value\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tlist[brightness] = 10;\n> > -\t\tlist[contrast] = 20;\n> > +\t\t/*\n> > +\t\t * Update both controls and verify that the container doesn't\n> > +\t\t * grow.\n> > +\t\t */\n> > +\t\tlist.set(controls::Brightness, 10);\n> > +\t\tlist.set(controls::Contrast, 20);\n> >\n> > -\t\tif (list[brightness].get() != 10 ||\n> > -\t\t list[contrast].get() != 20) {\n> > +\t\tif (list.get(controls::Brightness) != 10 ||\n> > +\t\t list.get(controls::Contrast) != 20) {\n> > \t\t\tcout << \"Failed to update control value\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\t/*\n> > -\t\t * Assert that the container has not grown with the control\n> > -\t\t * updated.\n> > -\t\t */\n> > \t\tif (list.size() != 2) {\n> > \t\t\tcout << \"List should contain two elements\" << endl;\n> > \t\t\treturn TestFail;\n> > @@ -157,8 +149,8 @@ protected:\n> > \t\t */\n> > \t\tControlList newList(camera_.get());\n> >\n> > -\t\tnewList[Brightness] = 128;\n> > -\t\tnewList[Saturation] = 255;\n> > +\t\tnewList.set(controls::Brightness, 128);\n> > +\t\tnewList.set(controls::Saturation, 255);\n> > \t\tnewList.update(list);\n> >\n> > \t\tlist.clear();\n> > @@ -178,16 +170,16 @@ protected:\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (!newList.contains(Brightness) ||\n> > -\t\t !newList.contains(Contrast) ||\n> > -\t\t !newList.contains(Saturation)) {\n> > +\t\tif (!newList.contains(controls::Brightness) ||\n> > +\t\t !newList.contains(controls::Contrast) ||\n> > +\t\t !newList.contains(controls::Saturation)) {\n> > \t\t\tcout << \"New list contains incorrect items\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> >\n> > -\t\tif (newList[Brightness].get() != 10 ||\n> > -\t\t newList[Contrast].get() != 20 ||\n> > -\t\t newList[Saturation].get() != 255) {\n> > +\t\tif (newList.get(controls::Brightness) != 10 ||\n> > +\t\t newList.get(controls::Contrast) != 20 ||\n> > +\t\t newList.get(controls::Saturation) != 255) {\n> > \t\t\tcout << \"New list contains incorrect values\" << endl;\n> > \t\t\treturn TestFail;\n> > \t\t}\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B235B61654\n\tfor ;\n\tSun, 29 Sep 2019 14:45:35 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E284320;\n\tSun, 29 Sep 2019 14:45:34 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1569761135;\n\tbh=qO57jMTuZyaN1WQdlgFT5hy81b+6HOF7dp1UyOv3+HU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Lgfin7trS3HmmN2pE/ttunxEhQUIpNdP7BRijwt1vYcFLxxOzV2WwxlDO6ZYmys9M\n\tdH/r7LPbCwl73ThIn9lXtQq0t8kkl8zczFV5sXVAPkydgi9++ksAF3x0yPPBRMFGGX\n\tUvFLWKgWLIGjYniLZgfEwxjQqwgrBtaEN0XNGMcc=","Date":"Sun, 29 Sep 2019 15:45:24 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190929124524.GD4827@pendragon.ideasonboard.com>","References":"<20190928152238.23752-1-laurent.pinchart@ideasonboard.com>\n\t<20190928152238.23752-5-laurent.pinchart@ideasonboard.com>\n\t<20190929100035.bfqtwew72jg5m3rt@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190929100035.bfqtwew72jg5m3rt@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: controls: Improve\n\tthe API towards applications","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 29 Sep 2019 12:45:35 -0000"}}]