[{"id":1974,"web_url":"https://patchwork.libcamera.org/comment/1974/","msgid":"<20190623152203.GB32581@bigcity.dyn.berto.se>","date":"2019-06-23T15:22:03","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Kieran,\n\nThanks for your work.\n\nOn 2019-06-21 17:13:55 +0100, Kieran Bingham wrote:\n> ControlIdentifiers declare the types of a control, and map their names,\n> and the ControlInfo class allows runtime state to be represented.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/controls.h | 58 ++++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/controls.cpp | 160 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 220 insertions(+)\n> create mode 100644 include/libcamera/controls.h\n> create mode 100644 src/libcamera/controls.cpp\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> new file mode 100644\n> index 000000000000..95198d41c4cf\n> --- /dev/null\n> +++ b/include/libcamera/controls.h\n> @@ -0,0 +1,58 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * controls.h - Control handling\n> + */\n> +#ifndef __LIBCAMERA_CONTROLS_H__\n> +#define __LIBCAMERA_CONTROLS_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +enum ControlId : uint32_t {\n> +\t/* IPA Controls */\n> +\tIpaAwbEnable,\n> +\n> +\t/* Manual Controls */\n> +\tManualBrightness,\n> +\tManualExposure,\n> +\tManualGain,\n> +};\n> +\n> +struct ControlIdentifier {\n> +\tControlId id;\n> +\tconst char *name;\n> +\tValueType type;\n> +};\n> +\n> +class ControlInfo\n> +{\n> +public:\n> +\tControlInfo(ControlId id, Value min = 0, Value max = 0);\n> +\n> +\tControlId id() const { return ident_->id; }\n> +\tconst char *name() const { return ident_->name; }\n> +\tValueType type() const { return ident_->type; }\n> +\n> +\tconst Value &min() { return min_; }\n> +\tconst Value &max() { return min_; }\n\nThese can be const functions.\n\n> +\n> +\tstd::string toString() const;\n> +\n> +\tbool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }\n> +\tbool operator==(const ControlId rhs) const { return id() == rhs; }\n> +\n> +private:\n> +\tstruct ControlIdentifier const *ident_;\n> +\tValue min_;\n> +\tValue max_;\n> +\n> +};\n> +\n> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CONTROLS_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index eb2211ae1fc3..06e3feebd23d 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_api = files([\n> 'buffer.h',\n> + 'controls.h',\n\nNot inserted in alphabetical order ;-)\n\n> 'camera.h',\n> 'camera_manager.h',\n> 'event_dispatcher.h',\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> new file mode 100644\n> index 000000000000..b1be46ddb55e\n> --- /dev/null\n> +++ b/src/libcamera/controls.cpp\n> @@ -0,0 +1,160 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * controls.cpp - Control handling\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n\nYou should have this as the first include statement in the file.\n\n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file controls.h\n> + * \\brief Describes control framework and controls supported by a camera\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Controls)\n> +\n> +/**\n> + * \\enum ControlId\n> + * Control Identifiers\n> + * \\var IpaAwbEnable\n> + * bool: Enables or disables the AWB\n> + * \\var ManualExposure\n> + * Manually control the exposure time in milli-seconds\n> + * \\var ManualGain\n> + * Controls the value of the manual gain setting\n> + */\n\nManualBrightness missing.\n\n> +\n> +/**\n> + * \\struct ControlIdentifier\n> + * Defines a Control with a unique ID, a name, and a type.\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> + */\n> +\n> +/*\n> + * Two sets of tables are generated for the static control definitions.\n> + *\n> + * An enum to declare the ID (in controls.h), and a type to establish its\n> + * representation.\n> + *\n> + * Todo: Automate the generation of both tables from a single input table.\n> + * Todo: Consider if this should be a static std::map created at init instead.\n\nI think a static std::map (or maybe unordered_map) is a good idea.\n\n> + */\n> +static const struct ControlIdentifier ControlTypes[] = {\n> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }\n> +\n> +\tCONTROL_TYPE(IpaAwbEnable,\t\tValueBool),\n> +\tCONTROL_TYPE(ManualBrightness,\t\tValueInteger),\n> +\tCONTROL_TYPE(ManualExposure,\t\tValueInteger),\n> +\tCONTROL_TYPE(ManualGain,\t\tValueInteger),\n> +\n> +#undef CONTROL_TYPE\n> +};\n> +\n> +static struct ControlIdentifier const *FindControlType(ControlId id)\n> +{\n> +\tstruct ControlIdentifier const *ident;\n> +\n> +\tfor (ident = ControlTypes;\n> +\t ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];\n> +\t ++ident) {\n> +\t\tif (ident->id == id)\n> +\t\t\treturn ident;\n> +\t}\n> +\n> +\tLOG(Controls, Fatal) << \"Failed to find a ControlType.\";\n> +\n> +\t/* Unreachable. */\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\class ControlInfo\n> + * \\brief Describes the information and capabilities of a Control\n> + */\n> +\n> +/**\n> + * \\brief Construct a ControlInfo with minimum and maximum range parameters.\n> + */\n> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)\n> +\t: ident_(FindControlType(id)), min_(min), max_(max)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn ControlInfo::id()\n> + * \\brief Return the ID of the control information descriptor\n> + * \\return the ControlId\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::name()\n> + * \\brief Return the string name of the control information descriptor\n> + * \\return A string name for the Control\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::type()\n> + * \\brief Return the ValueType of the control information descriptor\n> + * \\return the ControlId\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::min()\n> + * \\brief Reports the minimum value of the control\n> + * \\return a Value with the minimum setting for the control\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::max()\n> + * \\brief Reports the maximum value of the control\n> + * \\return a Value with the maximum setting for the control\n> + */\n> +\n> +/**\n> + * \\brief Provide a string representation of the ControlInfo\n> + */\n> +std::string ControlInfo::toString() const\n> +{\n> +\tstd::stringstream ss;\n> +\n> +\tss << \"Control: \" << name()\n> +\t << \" : Min(\" << min_ << \") Max(\" << max_ << \")\";\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\fn ControlInfo::operator==(const ControlInfo &rhs) const\n> + * \\brief Establish equivalence of ControlInfo. Only the IDs are considered.\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::operator==(const ControlId rhs) const\n> + * \\brief Establish equivalence of ControlInfo, against a ControlID.\n> + */\n> +\n> +/**\n> + * \\brief Provide a string stream representation of the ControlInfo \\a info to\n> + * the \\a stream.\n> + */\n> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> +{\n> +\treturn stream << info.toString();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 8e68373118df..e2c07d79bfb5 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -3,6 +3,7 @@ libcamera_sources = files([\n> 'camera.cpp',\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> + 'controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',\n> -- \n> 2.20.1\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 mail-lj1-x233.google.com (mail-lj1-x233.google.com\n\t[IPv6:2a00:1450:4864:20::233])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FED66157E\n\tfor ;\n\tSun, 23 Jun 2019 17:22:05 +0200 (CEST)","by mail-lj1-x233.google.com with SMTP id m23so10138829lje.12\n\tfor ;\n\tSun, 23 Jun 2019 08:22:05 -0700 (PDT)","from localhost (customer-145-14-112-32.stosn.net. [145.14.112.32])\n\tby smtp.gmail.com with ESMTPSA id\n\t89sm1306925ljs.48.2019.06.23.08.22.03\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 23 Jun 2019 08:22:04 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=64P0JPDvQYHfk40VtkatGfoJcuTqnknDeHDtLuaSHsg=;\n\tb=K/umBTZWafuzZIkuE7E6tGAuz5qehp+2q7gjRy1zPa0JVctt5IfXTj4QeGx68i3kXS\n\t2tKnfFYM7nVdzLlppHjERYuYVTJU0C7Veai4eZj9PXPOmtzmQB1vRo4IrkL+KJwGGeZB\n\tJ7FnqS/0f8ckeIFFKvQcDmhjtHqhuDkZdaIl++OP//lOkqZjIjebRV3xCNSJRd74MuvG\n\tD4xwkNrxLXe1xiQqzhiNY3+3DrxSi2CoIdRANDGzhYfRszgE6prXoKT4w2hEbOXxeq7m\n\tf6+/WhJ0PdWi/9PAJjBuDXXuq47/nvj+xx7v2fPbc2zv9XUx8NJOfTy2qJGnApI7aiYL\n\trlaw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=64P0JPDvQYHfk40VtkatGfoJcuTqnknDeHDtLuaSHsg=;\n\tb=ElzuWMzFqbENHfnGRrAzHGbykcdxI9q1e9GauAmSM9uscQF/NUQeiD9kYo+QuktxxT\n\tCmDcjYVgT3QUn0ALtbntv571txbiXoNyW30BtlFBF8tv6gHK2FkNcLgzkYl9bd8v2+xL\n\tafhNxHx4zXgkbauOS2vrFAV9X6i6n5FB2/mH/X5OQBq3mZvziWN3DaPwVd/JA8+vpGyM\n\tLy4I8jWJpQedLuUK0TpGv+PCHn57J25RJHh9bsRqkYUlYUtCcsJwYGucIiPUxSnjst+5\n\tIbOWbataJ2V5cLNWCbFLigyJpymwp+EHsNSKsx9fiabekr0913zxekz9doveCp8CiWza\n\tFAIA==","X-Gm-Message-State":"APjAAAX3vhz4/ldfVuZbVpY8tItZvo9dWCWlCB4J64WqMTRz3iA1DmsF\n\tTEen7PWtgcmIg1L+Rj7qAxNtcQy9/qk=","X-Google-Smtp-Source":"APXvYqxdFvOLxuF2zaREfKGsOm1/XOKnHMYhhGIE9W85x4SsgQKrY4Q8MpsH3KreEuneLViJleWjEw==","X-Received":"by 2002:a2e:2b57:: with SMTP id\n\tq84mr31831939lje.105.1561303324726; \n\tSun, 23 Jun 2019 08:22:04 -0700 (PDT)","Date":"Sun, 23 Jun 2019 17:22:03 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190623152203.GB32581@bigcity.dyn.berto.se>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190621161401.28337-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 23 Jun 2019 15:22:05 -0000"}},{"id":1979,"web_url":"https://patchwork.libcamera.org/comment/1979/","msgid":"<20190623215240.GB6124@pendragon.ideasonboard.com>","date":"2019-06-23T21:52:40","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Jun 21, 2019 at 05:13:55PM +0100, Kieran Bingham wrote:\n> ControlIdentifiers declare the types of a control, and map their names,\n> and the ControlInfo class allows runtime state to be represented.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/controls.h | 58 ++++++++++++\n> include/libcamera/meson.build | 1 +\n> src/libcamera/controls.cpp | 160 ++++++++++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 4 files changed, 220 insertions(+)\n> create mode 100644 include/libcamera/controls.h\n> create mode 100644 src/libcamera/controls.cpp\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> new file mode 100644\n> index 000000000000..95198d41c4cf\n> --- /dev/null\n> +++ b/include/libcamera/controls.h\n> @@ -0,0 +1,58 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * controls.h - Control handling\n> + */\n> +#ifndef __LIBCAMERA_CONTROLS_H__\n> +#define __LIBCAMERA_CONTROLS_H__\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +enum ControlId : uint32_t {\n> +\t/* IPA Controls */\n> +\tIpaAwbEnable,\n> +\n> +\t/* Manual Controls */\n> +\tManualBrightness,\n> +\tManualExposure,\n> +\tManualGain,\n\nThere will be lots of nitpicking about the names, we'll revisit this\nlater. For the time being I would still remove the IPA prefix and avoid\nmentioning IPA at all, as I think it should remain an internal concept.\n\n> +};\n> +\n> +struct ControlIdentifier {\n> +\tControlId id;\n> +\tconst char *name;\n> +\tValueType type;\n> +};\n> +\n> +class ControlInfo\n> +{\n> +public:\n> +\tControlInfo(ControlId id, Value min = 0, Value max = 0);\n\nShould you define the constructor as explicit as it's callable with a\nsingle argument ? Or does the compiler refuse that as there's more than\none argument, even if all but the first have default values ?\n\n> +\n> +\tControlId id() const { return ident_->id; }\n> +\tconst char *name() const { return ident_->name; }\n> +\tValueType type() const { return ident_->type; }\n> +\n> +\tconst Value &min() { return min_; }\n> +\tconst Value &max() { return min_; }\n> +\n> +\tstd::string toString() const;\n> +\n> +\tbool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }\n> +\tbool operator==(const ControlId rhs) const { return id() == rhs; }\n\nI wonder if these methods should be defined as non-members, as a bool\noperator==(const ControlId lhs, const ControlInfo &rhs) should also be\ndefined for symmetry. Or maybe we should drop the second method, and\nalways write info.id() == id explicitly ? It's not much longer, and\nshould be more explicit.\n\nYou should also define operator!= as !(lhs == rhs).\n\n> +\n> +private:\n> +\tstruct ControlIdentifier const *ident_;\n> +\tValue min_;\n> +\tValue max_;\n> +\n> +};\n> +\n> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n\nDon't you need to #include ?\n\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_CONTROLS_H__ */\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index eb2211ae1fc3..06e3feebd23d 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_api = files([\n> 'buffer.h',\n> + 'controls.h',\n> 'camera.h',\n> 'camera_manager.h',\n> 'event_dispatcher.h',\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> new file mode 100644\n> index 000000000000..b1be46ddb55e\n> --- /dev/null\n> +++ b/src/libcamera/controls.cpp\n> @@ -0,0 +1,160 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * controls.cpp - Control handling\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n\nPlease include this header first, for the same reason as in patch 1/9.\n\n> +\n> +#include \"log.h\"\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file controls.h\n> + * \\brief Describes control framework and controls supported by a camera\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Controls)\n> +\n> +/**\n> + * \\enum ControlId\n> + * Control Identifiers\n> + * \\var IpaAwbEnable\n> + * bool: Enables or disables the AWB\n> + * \\var ManualExposure\n> + * Manually control the exposure time in milli-seconds\n> + * \\var ManualGain\n> + * Controls the value of the manual gain setting\n> + */\n\nThis is an area where I want libcamera to have amazing documentation.\nNot only will each control need to be documented in details, but I also\nwant interactions between controls to be documented. In the meantime,\ncould you already try to format the documentation block appropriately ?\nOne block per control would be ideal.\n\n> +\n> +/**\n> + * \\struct ControlIdentifier\n> + * Defines a Control with a unique ID, a name, and a type.\n\n\\brief ?\n\nI think a more elaborate description would be useful, as so far I'm not\nsure why you have split control information in two parts (although I\nsuspect the reason, but an explicit description would be better :-)).\n\n> + * \\var ControlIdentifier::id\n\nIs the ControlIdentifier:: prefix needed ?\n\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> + */\n> +\n> +/*\n> + * Two sets of tables are generated for the static control definitions.\n> + *\n\ns/.\\n \\*\\n \\* An/:\\r * an/\n\nIs that readable ? :-)\n\n> + * An enum to declare the ID (in controls.h), and a type to establish its\n> + * representation.\n\nThe enum is actually not a table, it doesn't generate data as such.\n\n> + *\n> + * Todo: Automate the generation of both tables from a single input table.\n\ns/Todo: /\\\\todo /\n\nWould it be possible to generate them both from a documentation file ?\n\n> + * Todo: Consider if this should be a static std::map created at init instead.\n\nAny reason why it shouldn't be ? :-) Lookup would be more efficient.\n\n> + */\n> +static const struct ControlIdentifier ControlTypes[] = {\n> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }\n> +\n> +\tCONTROL_TYPE(IpaAwbEnable,\t\tValueBool),\n> +\tCONTROL_TYPE(ManualBrightness,\t\tValueInteger),\n> +\tCONTROL_TYPE(ManualExposure,\t\tValueInteger),\n> +\tCONTROL_TYPE(ManualGain,\t\tValueInteger),\n> +\n> +#undef CONTROL_TYPE\n> +};\n> +\n> +static struct ControlIdentifier const *FindControlType(ControlId id)\n\nThis is a function and should thus start with a lowercase letter.\n\n> +{\n> +\tstruct ControlIdentifier const *ident;\n> +\n> +\tfor (ident = ControlTypes;\n> +\t ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];\n> +\t ++ident) {\n> +\t\tif (ident->id == id)\n> +\t\t\treturn ident;\n> +\t}\n> +\n> +\tLOG(Controls, Fatal) << \"Failed to find a ControlType.\";\n> +\n> +\t/* Unreachable. */\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\class ControlInfo\n> + * \\brief Describes the information and capabilities of a Control\n\ns/Describes/Describe/\n\nAgain a bit more documentation would help.\n\n> + */\n> +\n> +/**\n> + * \\brief Construct a ControlInfo with minimum and maximum range parameters.\n> + */\n> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)\n> +\t: ident_(FindControlType(id)), min_(min), max_(max)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn ControlInfo::id()\n> + * \\brief Return the ID of the control information descriptor\n\ns/Return/Retrieve/\n\nThis is a bit confusing as nothing documents what a \"control information\ndescriptor\" is. I think the documentation will need to be revisited.\n\n> + * \\return the ControlId\n\nThe control ID\n\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::name()\n> + * \\brief Return the string name of the control information descriptor\n\ns/Return/Retrieve/\n\nand below too.\n\n> + * \\return A string name for the Control\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::type()\n> + * \\brief Return the ValueType of the control information descriptor\n> + * \\return the ControlId\n\nThe control type\n\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::min()\n> + * \\brief Reports the minimum value of the control\n> + * \\return a Value with the minimum setting for the control\n\ns/a/A/\n\nHow does that apply to non-integer controls ?\n\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::max()\n> + * \\brief Reports the maximum value of the control\n> + * \\return a Value with the maximum setting for the control\n> + */\n> +\n> +/**\n> + * \\brief Provide a string representation of the ControlInfo\n> + */\n> +std::string ControlInfo::toString() const\n> +{\n> +\tstd::stringstream ss;\n> +\n> +\tss << \"Control: \" << name()\n> +\t << \" : Min(\" << min_ << \") Max(\" << max_ << \")\";\n\nI still need to look at how you use this, but in general it's best to\nshorten the representations as much as possible. I may remove the\n\"Control:\" prefix, maybe with\n\n\tss << name() << \"[\" << min_ << \",\" << max_ << \"]\";\n\nThis would print\n\n\tManualExposure[1,1000]\n\nWe could replace the comma with ..\n\n\tManualExposure[1..1000]\n\n> +\n> +\treturn ss.str();\n> +}\n> +\n> +/**\n> + * \\fn ControlInfo::operator==(const ControlInfo &rhs) const\n> + * \\brief Establish equivalence of ControlInfo. Only the IDs are considered.\n> + */\n> +\n> +/**\n> + * \\fn ControlInfo::operator==(const ControlId rhs) const\n> + * \\brief Establish equivalence of ControlInfo, against a ControlID.\n> + */\n> +\n> +/**\n> + * \\brief Provide a string stream representation of the ControlInfo \\a info to\n> + * the \\a stream.\n> + */\n> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> +{\n> +\treturn stream << info.toString();\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index 8e68373118df..e2c07d79bfb5 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -3,6 +3,7 @@ libcamera_sources = files([\n> 'camera.cpp',\n> 'camera_manager.cpp',\n> 'camera_sensor.cpp',\n> + 'controls.cpp',\n> 'device_enumerator.cpp',\n> 'device_enumerator_sysfs.cpp',\n> 'event_dispatcher.cpp',","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 229506157E\n\tfor ;\n\tSun, 23 Jun 2019 23:53:01 +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 6A2BD500;\n\tSun, 23 Jun 2019 23:53:00 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561326780;\n\tbh=bA5+lvBmPrxPSoNmbbwlXjkdLcTDv4c9N1YQ3810g9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aqpYcXbN7BF5R66czsNwEXMrY9ZruRa/LoKJ4orM1YkkfJbZTDKUYXqKTrWG3G0C4\n\tEtFLyk2Y3nCk5n1oNv8SitE5oazyl9Iwi+cvssHPggXqlU3qb4GuCMbKFljVwudIZB\n\t4hzclGBfpllYPoIua2nwFbEjkqaq7ldnv4blMBN4=","Date":"Mon, 24 Jun 2019 00:52:40 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190623215240.GB6124@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-4-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190621161401.28337-4-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 23 Jun 2019 21:53:01 -0000"}},{"id":1980,"web_url":"https://patchwork.libcamera.org/comment/1980/","msgid":"<20190623215409.GC6124@pendragon.ideasonboard.com>","date":"2019-06-23T21:54:09","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Jun 23, 2019 at 05:22:03PM +0200, Niklas Söderlund wrote:\n> On 2019-06-21 17:13:55 +0100, Kieran Bingham wrote:\n> > ControlIdentifiers declare the types of a control, and map their names,\n> > and the ControlInfo class allows runtime state to be represented.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/controls.h | 58 ++++++++++++\n> > include/libcamera/meson.build | 1 +\n> > src/libcamera/controls.cpp | 160 ++++++++++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 4 files changed, 220 insertions(+)\n> > create mode 100644 include/libcamera/controls.h\n> > create mode 100644 src/libcamera/controls.cpp\n> > \n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > new file mode 100644\n> > index 000000000000..95198d41c4cf\n> > --- /dev/null\n> > +++ b/include/libcamera/controls.h\n> > @@ -0,0 +1,58 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * controls.h - Control handling\n> > + */\n> > +#ifndef __LIBCAMERA_CONTROLS_H__\n> > +#define __LIBCAMERA_CONTROLS_H__\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +enum ControlId : uint32_t {\n> > +\t/* IPA Controls */\n> > +\tIpaAwbEnable,\n> > +\n> > +\t/* Manual Controls */\n> > +\tManualBrightness,\n> > +\tManualExposure,\n> > +\tManualGain,\n> > +};\n> > +\n> > +struct ControlIdentifier {\n> > +\tControlId id;\n> > +\tconst char *name;\n> > +\tValueType type;\n> > +};\n> > +\n> > +class ControlInfo\n> > +{\n> > +public:\n> > +\tControlInfo(ControlId id, Value min = 0, Value max = 0);\n> > +\n> > +\tControlId id() const { return ident_->id; }\n> > +\tconst char *name() const { return ident_->name; }\n> > +\tValueType type() const { return ident_->type; }\n> > +\n> > +\tconst Value &min() { return min_; }\n> > +\tconst Value &max() { return min_; }\n> \n> These can be const functions.\n\nAnd the second one should return max_.\n\n> > +\n> > +\tstd::string toString() const;\n> > +\n> > +\tbool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }\n> > +\tbool operator==(const ControlId rhs) const { return id() == rhs; }\n> > +\n> > +private:\n> > +\tstruct ControlIdentifier const *ident_;\n> > +\tValue min_;\n> > +\tValue max_;\n> > +\n> > +};\n> > +\n> > +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> > +\n> > +} /* namespace libcamera */\n> > +\n> > +#endif /* __LIBCAMERA_CONTROLS_H__ */\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index eb2211ae1fc3..06e3feebd23d 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -1,5 +1,6 @@\n> > libcamera_api = files([\n> > 'buffer.h',\n> > + 'controls.h',\n> \n> Not inserted in alphabetical order ;-)\n\nHow did *I* miss that one ? :-)\n\n> > 'camera.h',\n> > 'camera_manager.h',\n> > 'event_dispatcher.h',\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > new file mode 100644\n> > index 000000000000..b1be46ddb55e\n> > --- /dev/null\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -0,0 +1,160 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * controls.cpp - Control handling\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> \n> You should have this as the first include statement in the file.\n> \n> > +\n> > +#include \"log.h\"\n> > +#include \"utils.h\"\n> > +\n> > +/**\n> > + * \\file controls.h\n> > + * \\brief Describes control framework and controls supported by a camera\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +LOG_DEFINE_CATEGORY(Controls)\n> > +\n> > +/**\n> > + * \\enum ControlId\n> > + * Control Identifiers\n> > + * \\var IpaAwbEnable\n> > + * bool: Enables or disables the AWB\n> > + * \\var ManualExposure\n> > + * Manually control the exposure time in milli-seconds\n> > + * \\var ManualGain\n> > + * Controls the value of the manual gain setting\n> > + */\n> \n> ManualBrightness missing.\n> \n> > +\n> > +/**\n> > + * \\struct ControlIdentifier\n> > + * Defines a Control with a unique ID, a name, and a type.\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> > + */\n> > +\n> > +/*\n> > + * Two sets of tables are generated for the static control definitions.\n> > + *\n> > + * An enum to declare the ID (in controls.h), and a type to establish its\n> > + * representation.\n> > + *\n> > + * Todo: Automate the generation of both tables from a single input table.\n> > + * Todo: Consider if this should be a static std::map created at init instead.\n> \n> I think a static std::map (or maybe unordered_map) is a good idea.\n> \n> > + */\n> > +static const struct ControlIdentifier ControlTypes[] = {\n> > +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }\n> > +\n> > +\tCONTROL_TYPE(IpaAwbEnable,\t\tValueBool),\n> > +\tCONTROL_TYPE(ManualBrightness,\t\tValueInteger),\n> > +\tCONTROL_TYPE(ManualExposure,\t\tValueInteger),\n> > +\tCONTROL_TYPE(ManualGain,\t\tValueInteger),\n> > +\n> > +#undef CONTROL_TYPE\n> > +};\n> > +\n> > +static struct ControlIdentifier const *FindControlType(ControlId id)\n> > +{\n> > +\tstruct ControlIdentifier const *ident;\n> > +\n> > +\tfor (ident = ControlTypes;\n> > +\t ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];\n> > +\t ++ident) {\n> > +\t\tif (ident->id == id)\n> > +\t\t\treturn ident;\n> > +\t}\n> > +\n> > +\tLOG(Controls, Fatal) << \"Failed to find a ControlType.\";\n> > +\n> > +\t/* Unreachable. */\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\class ControlInfo\n> > + * \\brief Describes the information and capabilities of a Control\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a ControlInfo with minimum and maximum range parameters.\n> > + */\n> > +ControlInfo::ControlInfo(ControlId id, Value min, Value max)\n> > +\t: ident_(FindControlType(id)), min_(min), max_(max)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn ControlInfo::id()\n> > + * \\brief Return the ID of the control information descriptor\n> > + * \\return the ControlId\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlInfo::name()\n> > + * \\brief Return the string name of the control information descriptor\n> > + * \\return A string name for the Control\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlInfo::type()\n> > + * \\brief Return the ValueType of the control information descriptor\n> > + * \\return the ControlId\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlInfo::min()\n> > + * \\brief Reports the minimum value of the control\n> > + * \\return a Value with the minimum setting for the control\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlInfo::max()\n> > + * \\brief Reports the maximum value of the control\n> > + * \\return a Value with the maximum setting for the control\n> > + */\n> > +\n> > +/**\n> > + * \\brief Provide a string representation of the ControlInfo\n> > + */\n> > +std::string ControlInfo::toString() const\n> > +{\n> > +\tstd::stringstream ss;\n> > +\n> > +\tss << \"Control: \" << name()\n> > +\t << \" : Min(\" << min_ << \") Max(\" << max_ << \")\";\n> > +\n> > +\treturn ss.str();\n> > +}\n> > +\n> > +/**\n> > + * \\fn ControlInfo::operator==(const ControlInfo &rhs) const\n> > + * \\brief Establish equivalence of ControlInfo. Only the IDs are considered.\n> > + */\n> > +\n> > +/**\n> > + * \\fn ControlInfo::operator==(const ControlId rhs) const\n> > + * \\brief Establish equivalence of ControlInfo, against a ControlID.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Provide a string stream representation of the ControlInfo \\a info to\n> > + * the \\a stream.\n> > + */\n> > +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> > +{\n> > +\treturn stream << info.toString();\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index 8e68373118df..e2c07d79bfb5 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -3,6 +3,7 @@ libcamera_sources = files([\n> > 'camera.cpp',\n> > 'camera_manager.cpp',\n> > 'camera_sensor.cpp',\n> > + 'controls.cpp',\n> > 'device_enumerator.cpp',\n> > 'device_enumerator_sysfs.cpp',\n> > 'event_dispatcher.cpp',","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19CC26157E\n\tfor ;\n\tSun, 23 Jun 2019 23:56:41 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7A18F500;\n\tSun, 23 Jun 2019 23:56:40 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561327000;\n\tbh=+J0EwImJMqlXoHk9ZOGlPFNI+MlS5SIDD9OAWBgYysE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KY3SlYcdiZg5Dz5yY0SpjWLC8kF7hZ9YC4/iCzwOf+ig52LqXPSF6i7523UMP/FmY\n\tHBB5tE/1OedKMxotuIgITd+PvrKjU4JVY9ZNmqfdTGKpC1PUxU9LjxeOXDXyjlliZl\n\tjDxNHWyj9b7+pqdCs9+x5qAR5N951toH0zMJIKX8=","Date":"Mon, 24 Jun 2019 00:54:09 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"Kieran Bingham ,\n\tLibCamera Devel ","Message-ID":"<20190623215409.GC6124@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-4-kieran.bingham@ideasonboard.com>\n\t<20190623152203.GB32581@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190623152203.GB32581@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 23 Jun 2019 21:56:41 -0000"}},{"id":2015,"web_url":"https://patchwork.libcamera.org/comment/2015/","msgid":"<6b48dcd6-3d55-cad1-0cf4-1a8f074ca4b3@ideasonboard.com>","date":"2019-06-24T16:49:35","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 23/06/2019 22:52, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 21, 2019 at 05:13:55PM +0100, Kieran Bingham wrote:\n>> ControlIdentifiers declare the types of a control, and map their names,\n>> and the ControlInfo class allows runtime state to be represented.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/controls.h | 58 ++++++++++++\n>> include/libcamera/meson.build | 1 +\n>> src/libcamera/controls.cpp | 160 ++++++++++++++++++++++++++++++++++\n>> src/libcamera/meson.build | 1 +\n>> 4 files changed, 220 insertions(+)\n>> create mode 100644 include/libcamera/controls.h\n>> create mode 100644 src/libcamera/controls.cpp\n>>\n>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>> new file mode 100644\n>> index 000000000000..95198d41c4cf\n>> --- /dev/null\n>> +++ b/include/libcamera/controls.h\n>> @@ -0,0 +1,58 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * controls.h - Control handling\n>> + */\n>> +#ifndef __LIBCAMERA_CONTROLS_H__\n>> +#define __LIBCAMERA_CONTROLS_H__\n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +enum ControlId : uint32_t {\n>> +\t/* IPA Controls */\n>> +\tIpaAwbEnable,\n>> +\n>> +\t/* Manual Controls */\n>> +\tManualBrightness,\n>> +\tManualExposure,\n>> +\tManualGain,\n> \n> There will be lots of nitpicking about the names, we'll revisit this\n> later. For the time being I would still remove the IPA prefix and avoid\n> mentioning IPA at all, as I think it should remain an internal concept.\n\n\nThese are essentially dummy controls at the moment. I picked a few\narbitrary initial control names. We can change them as you like. Are you\nstill planning to provide a list of expected controls that we should\nsupport?\n\nBut at the moment I expect the controls that are supported to grow\ndynamically as we develop them. I don't think we're going to get them\nall in, in one hit.\n\nI'll drop the Ipa anyway.\n\n\n> \n>> +};\n>> +\n>> +struct ControlIdentifier {\n>> +\tControlId id;\n>> +\tconst char *name;\n>> +\tValueType type;\n>> +};\n>> +\n>> +class ControlInfo\n>> +{\n>> +public:\n>> +\tControlInfo(ControlId id, Value min = 0, Value max = 0);\n> \n> Should you define the constructor as explicit as it's callable with a\n> single argument ? Or does the compiler refuse that as there's more than\n> one argument, even if all but the first have default values ?\n\n\nThat breaks things currently. I'll look into this later, depending on\nwhat happens with the changes to the ControlList class and map construction.\n\n\n\n> \n>> +\n>> +\tControlId id() const { return ident_->id; }\n>> +\tconst char *name() const { return ident_->name; }\n>> +\tValueType type() const { return ident_->type; }\n>> +\n>> +\tconst Value &min() { return min_; }\n>> +\tconst Value &max() { return min_; }\n>> +\n>> +\tstd::string toString() const;\n>> +\n>> +\tbool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }\n>> +\tbool operator==(const ControlId rhs) const { return id() == rhs; }\n> \n> I wonder if these methods should be defined as non-members, as a bool\n> operator==(const ControlId lhs, const ControlInfo &rhs) should also be\n> defined for symmetry. Or maybe we should drop the second method, and\n> always write info.id() == id explicitly ? It's not much longer, and\n> should be more explicit.\n\nThe comparisons are only really for the STL container (ControlList). But\nas long as they are implemented. I don't think it matters if they are in\nor out of the class.\n\nPerhaps simpler to inline them in the class - but that could still be\ndone outside.\n\n\n> You should also define operator!= as !(lhs == rhs).\n\nI don't think this is used by the std::unordered_map ... And I'm not\nsure if I expect users to be doing comparisons ... still it's an easy\nimplementation so it perhaps doesn't hurt.\n\n\n\n>> +\n>> +private:\n>> +\tstruct ControlIdentifier const *ident_;\n>> +\tValue min_;\n>> +\tValue max_;\n>> +\n>> +};\n>> +\n>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> \n> Don't you need to #include ?\n\nSure :) - Added.\n\n> \n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_CONTROLS_H__ */\n>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>> index eb2211ae1fc3..06e3feebd23d 100644\n>> --- a/include/libcamera/meson.build\n>> +++ b/include/libcamera/meson.build\n>> @@ -1,5 +1,6 @@\n>> libcamera_api = files([\n>> 'buffer.h',\n>> + 'controls.h',\n>> 'camera.h',\n>> 'camera_manager.h',\n>> 'event_dispatcher.h',\n>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>> new file mode 100644\n>> index 000000000000..b1be46ddb55e\n>> --- /dev/null\n>> +++ b/src/libcamera/controls.cpp\n>> @@ -0,0 +1,160 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2019, Google Inc.\n>> + *\n>> + * controls.cpp - Control handling\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n> \n> Please include this header first, for the same reason as in patch 1/9.\n\nFixed.\n\n> \n>> +\n>> +#include \"log.h\"\n>> +#include \"utils.h\"\n>> +\n>> +/**\n>> + * \\file controls.h\n>> + * \\brief Describes control framework and controls supported by a camera\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +LOG_DEFINE_CATEGORY(Controls)\n>> +\n>> +/**\n>> + * \\enum ControlId\n>> + * Control Identifiers\n>> + * \\var IpaAwbEnable\n>> + * bool: Enables or disables the AWB\n>> + * \\var ManualExposure\n>> + * Manually control the exposure time in milli-seconds\n>> + * \\var ManualGain\n>> + * Controls the value of the manual gain setting\n>> + */\n> \n> This is an area where I want libcamera to have amazing documentation.\n> Not only will each control need to be documented in details, but I also\n> want interactions between controls to be documented. In the meantime,\n> could you already try to format the documentation block appropriately ?\n> One block per control would be ideal.\n\nOk - but I am not by any means considering these controls as 'final'\nyet. These are just examples to get the framework in.\n\nEach of those is now given their own section.\n\nIf we can generate all the\n\n>> +\n>> +/**\n>> + * \\struct ControlIdentifier\n>> + * Defines a Control with a unique ID, a name, and a type.\n> \n> \\brief ?\n> \n> I think a more elaborate description would be useful, as so far I'm not\n> sure why you have split control information in two parts (although I\n> suspect the reason, but an explicit description would be better :-)).\n\nI've updated this a bit - but it still needs more refinement as the\nimplementation grows. And it depends on how the controls really get\nconstructed.\n\n\n> \n>> + * \\var ControlIdentifier::id\n> \n> Is the ControlIdentifier:: prefix needed ?\n\n\nIt seems that way.\n\nWithout the prefix - doxygen doesn't seem to know the following var's\nare part of the \\struct ControlIdentifier...\n\n> \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>> + */\n>> +\n>> +/*\n>> + * Two sets of tables are generated for the static control definitions.\n>> + *\n> \n> s/.\\n \\*\\n \\* An/:\\r * an/\n> \n> Is that readable ? :-)\n\nIt's a good job I read regex. ... :D (for a *very* limited subset of regex)\n\n\n>> + * An enum to declare the ID (in controls.h), and a type to establish its\n>> + * representation.\n> \n> The enum is actually not a table, it doesn't generate data as such.\n\nMy point is - I don't want to have to list an enum once to create the\nenum, and then a second table to create definitions. I'd like to find a\nway such that creating a control is just an addition to a single table.\n\n\n>> + *\n>> + * Todo: Automate the generation of both tables from a single input table.\n> \n> s/Todo: /\\\\todo /\n> \n> Would it be possible to generate them both from a documentation file ?\n\n^ Equally - there's a third place that would have to be updated. (the docs).\n\nThere's definitely scope here that things should be autogenerated from a\nsingle source.\n\n\n> \n>> + * Todo: Consider if this should be a static std::map created at init instead.\n> \n> Any reason why it shouldn't be ? :-) Lookup would be more efficient.\n\nBecause \\todo - and trying to figure out the details...\n\n\nThis is the point where RFC means 'Help out with suggestions on todo's :-D\n\n\n> \n>> + */\n>> +static const struct ControlIdentifier ControlTypes[] = {\n>> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }\n>> +\n>> +\tCONTROL_TYPE(IpaAwbEnable,\t\tValueBool),\n>> +\tCONTROL_TYPE(ManualBrightness,\t\tValueInteger),\n>> +\tCONTROL_TYPE(ManualExposure,\t\tValueInteger),\n>> +\tCONTROL_TYPE(ManualGain,\t\tValueInteger),\n>> +\n>> +#undef CONTROL_TYPE\n>> +};\n>> +\n>> +static struct ControlIdentifier const *FindControlType(ControlId id)\n> \n> This is a function and should thus start with a lowercase letter.\n\nAck.\n\n> \n>> +{\n>> +\tstruct ControlIdentifier const *ident;\n>> +\n>> +\tfor (ident = ControlTypes;\n>> +\t ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];\n>> +\t ++ident) {\n>> +\t\tif (ident->id == id)\n>> +\t\t\treturn ident;\n>> +\t}\n>> +\n>> +\tLOG(Controls, Fatal) << \"Failed to find a ControlType.\";\n>> +\n>> +\t/* Unreachable. */\n>> +\treturn nullptr;\n>> +}\n>> +\n>> +/**\n>> + * \\class ControlInfo\n>> + * \\brief Describes the information and capabilities of a Control\n> \n> s/Describes/Describe/\n> \n> Again a bit more documentation would help.\n> \n>> + */\n>> +\n>> +/**\n>> + * \\brief Construct a ControlInfo with minimum and maximum range parameters.\n>> + */\n>> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)\n>> +\t: ident_(FindControlType(id)), min_(min), max_(max)\n>> +{\n>> +}\n>> +\n>> +/**\n>> + * \\fn ControlInfo::id()\n>> + * \\brief Return the ID of the control information descriptor\n> \n> s/Return/Retrieve/\n> \n> This is a bit confusing as nothing documents what a \"control information\n> descriptor\" is. I think the documentation will need to be revisited.\n\n\n>> + * \\return the ControlId\n> \n> The control ID\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlInfo::name()\n>> + * \\brief Return the string name of the control information descriptor\n> \n> s/Return/Retrieve/\n> \n> and below too.\n> \n>> + * \\return A string name for the Control\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlInfo::type()\n>> + * \\brief Return the ValueType of the control information descriptor\n>> + * \\return the ControlId\n> \n> The control type\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlInfo::min()\n>> + * \\brief Reports the minimum value of the control\n>> + * \\return a Value with the minimum setting for the control\n> \n> s/a/A/\n> \n> How does that apply to non-integer controls ?\n\nI have no idea yet. \"RFC\" : What are your thoughts?\n\nmin/max are not even yet populated with anything.\n\nPerhaps it only applies to integer controls.\n\n> \n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlInfo::max()\n>> + * \\brief Reports the maximum value of the control\n>> + * \\return a Value with the maximum setting for the control\n>> + */\n>> +\n>> +/**\n>> + * \\brief Provide a string representation of the ControlInfo\n>> + */\n>> +std::string ControlInfo::toString() const\n>> +{\n>> +\tstd::stringstream ss;\n>> +\n>> +\tss << \"Control: \" << name()\n>> +\t << \" : Min(\" << min_ << \") Max(\" << max_ << \")\";\n> \n> I still need to look at how you use this, but in general it's best to\n> shorten the representations as much as possible. I may remove the\n> \"Control:\" prefix, maybe with\n> \n> \tss << name() << \"[\" << min_ << \",\" << max_ << \"]\";\n> \n> This would print\n> \n> \tManualExposure[1,1000]\n> \n> We could replace the comma with ..\n> \n> \tManualExposure[1..1000]\n\nI like that !\n\nDone.\n\n\n> \n>> +\n>> +\treturn ss.str();\n>> +}\n>> +\n>> +/**\n>> + * \\fn ControlInfo::operator==(const ControlInfo &rhs) const\n>> + * \\brief Establish equivalence of ControlInfo. Only the IDs are considered.\n>> + */\n>> +\n>> +/**\n>> + * \\fn ControlInfo::operator==(const ControlId rhs) const\n>> + * \\brief Establish equivalence of ControlInfo, against a ControlID.\n>> + */\n>> +\n>> +/**\n>> + * \\brief Provide a string stream representation of the ControlInfo \\a info to\n>> + * the \\a stream.\n>> + */\n>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n>> +{\n>> +\treturn stream << info.toString();\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index 8e68373118df..e2c07d79bfb5 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -3,6 +3,7 @@ libcamera_sources = files([\n>> 'camera.cpp',\n>> 'camera_manager.cpp',\n>> 'camera_sensor.cpp',\n>> + 'controls.cpp',\n>> 'device_enumerator.cpp',\n>> 'device_enumerator_sysfs.cpp',\n>> 'event_dispatcher.cpp',\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7855061580\n\tfor ;\n\tMon, 24 Jun 2019 18:49:39 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C478A2E7;\n\tMon, 24 Jun 2019 18:49:38 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561394979;\n\tbh=ZBfFfQW3XPwP3YiixrXiEzy0qJkbuXvvey24sWsfa28=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Iic84wPFRPZid4Bv6ozpMsKm+itUIgyS/gR92SJZWA0JHirja8hB+cUqLVys4aU2c\n\tnQPvPBuNwCsWR2VoBq7aEX+swsVNiO85e9hRWOms3Pa9gQ1tWa23l4cUNVXEmBevkI\n\tSZNDSmCJcytuhCMVSCyuGyH5PxDCifz+bUi5TYJU=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-4-kieran.bingham@ideasonboard.com>\n\t<20190623215240.GB6124@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<6b48dcd6-3d55-cad1-0cf4-1a8f074ca4b3@ideasonboard.com>","Date":"Mon, 24 Jun 2019 17:49:35 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190623215240.GB6124@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 16:49:39 -0000"}},{"id":2016,"web_url":"https://patchwork.libcamera.org/comment/2016/","msgid":"<2371b7ed-e9bf-6822-e199-05fdf234e53b@ideasonboard.com>","date":"2019-06-24T16:49:38","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent, Niklas,\n\nOn 23/06/2019 22:54, Laurent Pinchart wrote:\n> On Sun, Jun 23, 2019 at 05:22:03PM +0200, Niklas Söderlund wrote:\n>> On 2019-06-21 17:13:55 +0100, Kieran Bingham wrote:\n>>> ControlIdentifiers declare the types of a control, and map their names,\n>>> and the ControlInfo class allows runtime state to be represented.\n>>>\n>>> Signed-off-by: Kieran Bingham \n>>> ---\n>>> include/libcamera/controls.h | 58 ++++++++++++\n>>> include/libcamera/meson.build | 1 +\n>>> src/libcamera/controls.cpp | 160 ++++++++++++++++++++++++++++++++++\n>>> src/libcamera/meson.build | 1 +\n>>> 4 files changed, 220 insertions(+)\n>>> create mode 100644 include/libcamera/controls.h\n>>> create mode 100644 src/libcamera/controls.cpp\n>>>\n>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n>>> new file mode 100644\n>>> index 000000000000..95198d41c4cf\n>>> --- /dev/null\n>>> +++ b/include/libcamera/controls.h\n>>> @@ -0,0 +1,58 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>> + *\n>>> + * controls.h - Control handling\n>>> + */\n>>> +#ifndef __LIBCAMERA_CONTROLS_H__\n>>> +#define __LIBCAMERA_CONTROLS_H__\n>>> +\n>>> +#include \n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +enum ControlId : uint32_t {\n>>> +\t/* IPA Controls */\n>>> +\tIpaAwbEnable,\n>>> +\n>>> +\t/* Manual Controls */\n>>> +\tManualBrightness,\n>>> +\tManualExposure,\n>>> +\tManualGain,\n>>> +};\n>>> +\n>>> +struct ControlIdentifier {\n>>> +\tControlId id;\n>>> +\tconst char *name;\n>>> +\tValueType type;\n>>> +};\n>>> +\n>>> +class ControlInfo\n>>> +{\n>>> +public:\n>>> +\tControlInfo(ControlId id, Value min = 0, Value max = 0);\n>>> +\n>>> +\tControlId id() const { return ident_->id; }\n>>> +\tconst char *name() const { return ident_->name; }\n>>> +\tValueType type() const { return ident_->type; }\n>>> +\n>>> +\tconst Value &min() { return min_; }\n>>> +\tconst Value &max() { return min_; }\n>>\n>> These can be const functions.\n\nDone.\n\n> \n> And the second one should return max_.\n\n\nDone ... eeep :-(\n\n\n> \n>>> +\n>>> +\tstd::string toString() const;\n>>> +\n>>> +\tbool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }\n>>> +\tbool operator==(const ControlId rhs) const { return id() == rhs; }\n>>> +\n>>> +private:\n>>> +\tstruct ControlIdentifier const *ident_;\n>>> +\tValue min_;\n>>> +\tValue max_;\n>>> +\n>>> +};\n>>> +\n>>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n>>> +\n>>> +} /* namespace libcamera */\n>>> +\n>>> +#endif /* __LIBCAMERA_CONTROLS_H__ */\n>>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>>> index eb2211ae1fc3..06e3feebd23d 100644\n>>> --- a/include/libcamera/meson.build\n>>> +++ b/include/libcamera/meson.build\n>>> @@ -1,5 +1,6 @@\n>>> libcamera_api = files([\n>>> 'buffer.h',\n>>> + 'controls.h',\n>>\n>> Not inserted in alphabetical order ;-)\n> \n> How did *I* miss that one ? :-)\n\nNow sorted correctly.\n\n> \n>>> 'camera.h',\n>>> 'camera_manager.h',\n>>> 'event_dispatcher.h',\n>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n>>> new file mode 100644\n>>> index 000000000000..b1be46ddb55e\n>>> --- /dev/null\n>>> +++ b/src/libcamera/controls.cpp\n>>> @@ -0,0 +1,160 @@\n>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>> +/*\n>>> + * Copyright (C) 2019, Google Inc.\n>>> + *\n>>> + * controls.cpp - Control handling\n>>> + */\n>>> +\n>>> +#include \n>>> +#include \n>>> +#include \n>>> +\n>>> +#include \n>>\n>> You should have this as the first include statement in the file.\n\nFixed.\n\n>>\n>>> +\n>>> +#include \"log.h\"\n>>> +#include \"utils.h\"\n>>> +\n>>> +/**\n>>> + * \\file controls.h\n>>> + * \\brief Describes control framework and controls supported by a camera\n>>> + */\n>>> +\n>>> +namespace libcamera {\n>>> +\n>>> +LOG_DEFINE_CATEGORY(Controls)\n>>> +\n>>> +/**\n>>> + * \\enum ControlId\n>>> + * Control Identifiers\n>>> + * \\var IpaAwbEnable\n>>> + * bool: Enables or disables the AWB\n>>> + * \\var ManualExposure\n>>> + * Manually control the exposure time in milli-seconds\n>>> + * \\var ManualGain\n>>> + * Controls the value of the manual gain setting\n>>> + */\n>>\n>> ManualBrightness missing.\n\nAdded.\n\n>>\n>>> +\n>>> +/**\n>>> + * \\struct ControlIdentifier\n>>> + * Defines a Control with a unique ID, a name, and a type.\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>>> + */\n>>> +\n>>> +/*\n>>> + * Two sets of tables are generated for the static control definitions.\n>>> + *\n>>> + * An enum to declare the ID (in controls.h), and a type to establish its\n>>> + * representation.\n>>> + *\n>>> + * Todo: Automate the generation of both tables from a single input table.\n>>> + * Todo: Consider if this should be a static std::map created at init instead.\n>>\n>> I think a static std::map (or maybe unordered_map) is a good idea.\n\nYes, but I think I still need to work out how to automatically generate\nthe tables from a single data source, and correctly populate them at\ncamera instantiation.\n\n\n>>\n>>> + */\n>>> +static const struct ControlIdentifier ControlTypes[] = {\n>>> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }\n>>> +\n>>> +\tCONTROL_TYPE(IpaAwbEnable,\t\tValueBool),\n>>> +\tCONTROL_TYPE(ManualBrightness,\t\tValueInteger),\n>>> +\tCONTROL_TYPE(ManualExposure,\t\tValueInteger),\n>>> +\tCONTROL_TYPE(ManualGain,\t\tValueInteger),\n>>> +\n>>> +#undef CONTROL_TYPE\n>>> +};\n>>> +\n>>> +static struct ControlIdentifier const *FindControlType(ControlId id)\n>>> +{\n>>> +\tstruct ControlIdentifier const *ident;\n>>> +\n>>> +\tfor (ident = ControlTypes;\n>>> +\t ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];\n>>> +\t ++ident) {\n>>> +\t\tif (ident->id == id)\n>>> +\t\t\treturn ident;\n>>> +\t}\n>>> +\n>>> +\tLOG(Controls, Fatal) << \"Failed to find a ControlType.\";\n>>> +\n>>> +\t/* Unreachable. */\n>>> +\treturn nullptr;\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\class ControlInfo\n>>> + * \\brief Describes the information and capabilities of a Control\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Construct a ControlInfo with minimum and maximum range parameters.\n>>> + */\n>>> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)\n>>> +\t: ident_(FindControlType(id)), min_(min), max_(max)\n>>> +{\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::id()\n>>> + * \\brief Return the ID of the control information descriptor\n>>> + * \\return the ControlId\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::name()\n>>> + * \\brief Return the string name of the control information descriptor\n>>> + * \\return A string name for the Control\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::type()\n>>> + * \\brief Return the ValueType of the control information descriptor\n>>> + * \\return the ControlId\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::min()\n>>> + * \\brief Reports the minimum value of the control\n>>> + * \\return a Value with the minimum setting for the control\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::max()\n>>> + * \\brief Reports the maximum value of the control\n>>> + * \\return a Value with the maximum setting for the control\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Provide a string representation of the ControlInfo\n>>> + */\n>>> +std::string ControlInfo::toString() const\n>>> +{\n>>> +\tstd::stringstream ss;\n>>> +\n>>> +\tss << \"Control: \" << name()\n>>> +\t << \" : Min(\" << min_ << \") Max(\" << max_ << \")\";\n>>> +\n>>> +\treturn ss.str();\n>>> +}\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::operator==(const ControlInfo &rhs) const\n>>> + * \\brief Establish equivalence of ControlInfo. Only the IDs are considered.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\fn ControlInfo::operator==(const ControlId rhs) const\n>>> + * \\brief Establish equivalence of ControlInfo, against a ControlID.\n>>> + */\n>>> +\n>>> +/**\n>>> + * \\brief Provide a string stream representation of the ControlInfo \\a info to\n>>> + * the \\a stream.\n>>> + */\n>>> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n>>> +{\n>>> +\treturn stream << info.toString();\n>>> +}\n>>> +\n>>> +} /* namespace libcamera */\n>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>> index 8e68373118df..e2c07d79bfb5 100644\n>>> --- a/src/libcamera/meson.build\n>>> +++ b/src/libcamera/meson.build\n>>> @@ -3,6 +3,7 @@ libcamera_sources = files([\n>>> 'camera.cpp',\n>>> 'camera_manager.cpp',\n>>> 'camera_sensor.cpp',\n>>> + 'controls.cpp',\n>>> 'device_enumerator.cpp',\n>>> 'device_enumerator_sysfs.cpp',\n>>> 'event_dispatcher.cpp',\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C0F3615BC\n\tfor ;\n\tMon, 24 Jun 2019 18:49:41 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1B147555;\n\tMon, 24 Jun 2019 18:49:41 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561394981;\n\tbh=awY8hoQyQDX2fFTFpEda7b0jtSQYiwp00m5hmQgjmPI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=T6SYgoXiBa439LAlR/ZPd0FTNSn9ZV0uPGBmOJgqbG06bOV8opmBemmHBEV58FQrQ\n\tLdL+gEZkH1gsoWvqVQJ98ivPMqk1Q4+6czP3Y57A1IXiVyPR5U/WVvyCbXZjQ+D1WF\n\tW3Endaz8nwDTAqlBdQI9h2addsFPnsmDCJPU9ClE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart , =?utf-8?q?Niklas?=\n\t=?utf-8?q?_S=C3=B6derlund?= ","Cc":"LibCamera Devel ","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-4-kieran.bingham@ideasonboard.com>\n\t<20190623152203.GB32581@bigcity.dyn.berto.se>\n\t<20190623215409.GC6124@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<2371b7ed-e9bf-6822-e199-05fdf234e53b@ideasonboard.com>","Date":"Mon, 24 Jun 2019 17:49:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.7.0","MIME-Version":"1.0","In-Reply-To":"<20190623215409.GC6124@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 16:49:41 -0000"}},{"id":2018,"web_url":"https://patchwork.libcamera.org/comment/2018/","msgid":"<20190624175833.GR5737@pendragon.ideasonboard.com>","date":"2019-06-24T17:58:33","subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Mon, Jun 24, 2019 at 05:49:35PM +0100, Kieran Bingham wrote:\n> On 23/06/2019 22:52, Laurent Pinchart wrote:\n> > On Fri, Jun 21, 2019 at 05:13:55PM +0100, Kieran Bingham wrote:\n> >> ControlIdentifiers declare the types of a control, and map their names,\n> >> and the ControlInfo class allows runtime state to be represented.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> include/libcamera/controls.h | 58 ++++++++++++\n> >> include/libcamera/meson.build | 1 +\n> >> src/libcamera/controls.cpp | 160 ++++++++++++++++++++++++++++++++++\n> >> src/libcamera/meson.build | 1 +\n> >> 4 files changed, 220 insertions(+)\n> >> create mode 100644 include/libcamera/controls.h\n> >> create mode 100644 src/libcamera/controls.cpp\n> >>\n> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> >> new file mode 100644\n> >> index 000000000000..95198d41c4cf\n> >> --- /dev/null\n> >> +++ b/include/libcamera/controls.h\n> >> @@ -0,0 +1,58 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * controls.h - Control handling\n> >> + */\n> >> +#ifndef __LIBCAMERA_CONTROLS_H__\n> >> +#define __LIBCAMERA_CONTROLS_H__\n> >> +\n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +enum ControlId : uint32_t {\n> >> +\t/* IPA Controls */\n> >> +\tIpaAwbEnable,\n> >> +\n> >> +\t/* Manual Controls */\n> >> +\tManualBrightness,\n> >> +\tManualExposure,\n> >> +\tManualGain,\n> > \n> > There will be lots of nitpicking about the names, we'll revisit this\n> > later. For the time being I would still remove the IPA prefix and avoid\n> > mentioning IPA at all, as I think it should remain an internal concept.\n> \n> These are essentially dummy controls at the moment. I picked a few\n> arbitrary initial control names. We can change them as you like. Are you\n> still planning to provide a list of expected controls that we should\n> support?\n\nThe full list will likely be discussed during our next code camp. For\nthe initial list required to develop the IPA API, I'm sorry about not\nsending a formal e-mail yet. I think we need\n\nAutoExposure (bool)\nManualExpose (int)\nManualGain (int)\n\nThe first control may turn into an exposure mode control with more than\ntwo values.\n\n> But at the moment I expect the controls that are supported to grow\n> dynamically as we develop them. I don't think we're going to get them\n> all in, in one hit.\n> \n> I'll drop the Ipa anyway.\n\nCould you also split it into AutoExposure and AutoWhiteBalance, in order\nto already support IPA development ?\n\n> >> +};\n> >> +\n> >> +struct ControlIdentifier {\n> >> +\tControlId id;\n> >> +\tconst char *name;\n> >> +\tValueType type;\n> >> +};\n> >> +\n> >> +class ControlInfo\n> >> +{\n> >> +public:\n> >> +\tControlInfo(ControlId id, Value min = 0, Value max = 0);\n> > \n> > Should you define the constructor as explicit as it's callable with a\n> > single argument ? Or does the compiler refuse that as there's more than\n> > one argument, even if all but the first have default values ?\n> \n> That breaks things currently. I'll look into this later, depending on\n> what happens with the changes to the ControlList class and map construction.\n\nI think it's a sign of an issue ;-)\n\n> >> +\n> >> +\tControlId id() const { return ident_->id; }\n> >> +\tconst char *name() const { return ident_->name; }\n> >> +\tValueType type() const { return ident_->type; }\n> >> +\n> >> +\tconst Value &min() { return min_; }\n> >> +\tconst Value &max() { return min_; }\n> >> +\n> >> +\tstd::string toString() const;\n> >> +\n> >> +\tbool operator==(const ControlInfo &rhs) const { return id() == rhs.id(); }\n> >> +\tbool operator==(const ControlId rhs) const { return id() == rhs; }\n> > \n> > I wonder if these methods should be defined as non-members, as a bool\n> > operator==(const ControlId lhs, const ControlInfo &rhs) should also be\n> > defined for symmetry. Or maybe we should drop the second method, and\n> > always write info.id() == id explicitly ? It's not much longer, and\n> > should be more explicit.\n> \n> The comparisons are only really for the STL container (ControlList). But\n> as long as they are implemented. I don't think it matters if they are in\n> or out of the class.\n> \n> Perhaps simpler to inline them in the class - but that could still be\n> done outside.\n> \n> > You should also define operator!= as !(lhs == rhs).\n> \n> I don't think this is used by the std::unordered_map ... And I'm not\n> sure if I expect users to be doing comparisons ... still it's an easy\n> implementation so it perhaps doesn't hurt.\n\nI think it's a good practice to define != when == is defined, otherwise\nthings can become confusing.\n\n> >> +\n> >> +private:\n> >> +\tstruct ControlIdentifier const *ident_;\n> >> +\tValue min_;\n> >> +\tValue max_;\n> >> +\n> >> +};\n> >> +\n> >> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &value);\n> > \n> > Don't you need to #include ?\n> \n> Sure :) - Added.\n> \n> >> +\n> >> +} /* namespace libcamera */\n> >> +\n> >> +#endif /* __LIBCAMERA_CONTROLS_H__ */\n> >> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> >> index eb2211ae1fc3..06e3feebd23d 100644\n> >> --- a/include/libcamera/meson.build\n> >> +++ b/include/libcamera/meson.build\n> >> @@ -1,5 +1,6 @@\n> >> libcamera_api = files([\n> >> 'buffer.h',\n> >> + 'controls.h',\n> >> 'camera.h',\n> >> 'camera_manager.h',\n> >> 'event_dispatcher.h',\n> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> >> new file mode 100644\n> >> index 000000000000..b1be46ddb55e\n> >> --- /dev/null\n> >> +++ b/src/libcamera/controls.cpp\n> >> @@ -0,0 +1,160 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2019, Google Inc.\n> >> + *\n> >> + * controls.cpp - Control handling\n> >> + */\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> > \n> > Please include this header first, for the same reason as in patch 1/9.\n> \n> Fixed.\n> \n> >> +\n> >> +#include \"log.h\"\n> >> +#include \"utils.h\"\n> >> +\n> >> +/**\n> >> + * \\file controls.h\n> >> + * \\brief Describes control framework and controls supported by a camera\n> >> + */\n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +LOG_DEFINE_CATEGORY(Controls)\n> >> +\n> >> +/**\n> >> + * \\enum ControlId\n> >> + * Control Identifiers\n> >> + * \\var IpaAwbEnable\n> >> + * bool: Enables or disables the AWB\n> >> + * \\var ManualExposure\n> >> + * Manually control the exposure time in milli-seconds\n> >> + * \\var ManualGain\n> >> + * Controls the value of the manual gain setting\n> >> + */\n> > \n> > This is an area where I want libcamera to have amazing documentation.\n> > Not only will each control need to be documented in details, but I also\n> > want interactions between controls to be documented. In the meantime,\n> > could you already try to format the documentation block appropriately ?\n> > One block per control would be ideal.\n> \n> Ok - but I am not by any means considering these controls as 'final'\n> yet. These are just examples to get the framework in.\n> \n> Each of those is now given their own section.\n> \n> If we can generate all the\n\nYes, all the should be generated :-)\n\nMy point was that it would be nice if you could experiment with\nsplitting the control documentation with a doxygen documentation block\nfor each control, preferably with multiple paragraphs per control (or\nfor one control at least). This is likely what we will be generating, so\nhaving the proper doxygen formatting here will help.\n\n> >> +\n> >> +/**\n> >> + * \\struct ControlIdentifier\n> >> + * Defines a Control with a unique ID, a name, and a type.\n> > \n> > \\brief ?\n> > \n> > I think a more elaborate description would be useful, as so far I'm not\n> > sure why you have split control information in two parts (although I\n> > suspect the reason, but an explicit description would be better :-)).\n> \n> I've updated this a bit - but it still needs more refinement as the\n> implementation grows. And it depends on how the controls really get\n> constructed.\n> \n> >> + * \\var ControlIdentifier::id\n> > \n> > Is the ControlIdentifier:: prefix needed ?\n> \n> It seems that way.\n> \n> Without the prefix - doxygen doesn't seem to know the following var's\n> are part of the \\struct ControlIdentifier...\n\nOK.\n\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> >> + */\n> >> +\n> >> +/*\n> >> + * Two sets of tables are generated for the static control definitions.\n> >> + *\n> > \n> > s/.\\n \\*\\n \\* An/:\\r * an/\n> > \n> > Is that readable ? :-)\n> \n> It's a good job I read regex. ... :D (for a *very* limited subset of regex)\n> \n> >> + * An enum to declare the ID (in controls.h), and a type to establish its\n> >> + * representation.\n> > \n> > The enum is actually not a table, it doesn't generate data as such.\n> \n> My point is - I don't want to have to list an enum once to create the\n> enum, and then a second table to create definitions. I'd like to find a\n> way such that creating a control is just an addition to a single table.\n\nWhat format do you think we should use to document controls and generate\ndata ?\n\n> >> + *\n> >> + * Todo: Automate the generation of both tables from a single input table.\n> > \n> > s/Todo: /\\\\todo /\n> > \n> > Would it be possible to generate them both from a documentation file ?\n> \n> ^ Equally - there's a third place that would have to be updated. (the docs).\n> \n> There's definitely scope here that things should be autogenerated from a\n> single source.\n\nThat's my point, having a single documentation file that generates both\nthe header and the table.\n\n> >> + * Todo: Consider if this should be a static std::map created at init instead.\n> > \n> > Any reason why it shouldn't be ? :-) Lookup would be more efficient.\n> \n> Because \\todo - and trying to figure out the details...\n> \n> This is the point where RFC means 'Help out with suggestions on todo's :-D\n\nThen, if it wasn't explicit enough, I think it should be a map :-)\n\n> >> + */\n> >> +static const struct ControlIdentifier ControlTypes[] = {\n> >> +#define CONTROL_TYPE(_id, _type) { _id, #_id, _type }\n> >> +\n> >> +\tCONTROL_TYPE(IpaAwbEnable,\t\tValueBool),\n> >> +\tCONTROL_TYPE(ManualBrightness,\t\tValueInteger),\n> >> +\tCONTROL_TYPE(ManualExposure,\t\tValueInteger),\n> >> +\tCONTROL_TYPE(ManualGain,\t\tValueInteger),\n> >> +\n> >> +#undef CONTROL_TYPE\n> >> +};\n> >> +\n> >> +static struct ControlIdentifier const *FindControlType(ControlId id)\n> > \n> > This is a function and should thus start with a lowercase letter.\n> \n> Ack.\n> \n> >> +{\n> >> +\tstruct ControlIdentifier const *ident;\n> >> +\n> >> +\tfor (ident = ControlTypes;\n> >> +\t ident != &ControlTypes[ARRAY_SIZE(ControlTypes)];\n> >> +\t ++ident) {\n> >> +\t\tif (ident->id == id)\n> >> +\t\t\treturn ident;\n> >> +\t}\n> >> +\n> >> +\tLOG(Controls, Fatal) << \"Failed to find a ControlType.\";\n> >> +\n> >> +\t/* Unreachable. */\n> >> +\treturn nullptr;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\class ControlInfo\n> >> + * \\brief Describes the information and capabilities of a Control\n> > \n> > s/Describes/Describe/\n> > \n> > Again a bit more documentation would help.\n> > \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Construct a ControlInfo with minimum and maximum range parameters.\n> >> + */\n> >> +ControlInfo::ControlInfo(ControlId id, Value min, Value max)\n> >> +\t: ident_(FindControlType(id)), min_(min), max_(max)\n> >> +{\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::id()\n> >> + * \\brief Return the ID of the control information descriptor\n> > \n> > s/Return/Retrieve/\n> > \n> > This is a bit confusing as nothing documents what a \"control information\n> > descriptor\" is. I think the documentation will need to be revisited.\n> > \n> >> + * \\return the ControlId\n> > \n> > The control ID\n> > \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::name()\n> >> + * \\brief Return the string name of the control information descriptor\n> > \n> > s/Return/Retrieve/\n> > \n> > and below too.\n> > \n> >> + * \\return A string name for the Control\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::type()\n> >> + * \\brief Return the ValueType of the control information descriptor\n> >> + * \\return the ControlId\n> > \n> > The control type\n> > \n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::min()\n> >> + * \\brief Reports the minimum value of the control\n> >> + * \\return a Value with the minimum setting for the control\n> > \n> > s/a/A/\n> > \n> > How does that apply to non-integer controls ?\n> \n> I have no idea yet. \"RFC\" : What are your thoughts?\n> \n> min/max are not even yet populated with anything.\n> \n> Perhaps it only applies to integer controls.\n\nI think we should document that the value is undefined for other types\nof controls, yes. Or would it make sense to also define it for boolean\ncontrols ? A boolean can only take two values, but it may be that in\nsome circumstances a camera would only support one value. We could drop\nthe control in that case, but exposing it with the only value it\nsupports may also be useful.\n\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::max()\n> >> + * \\brief Reports the maximum value of the control\n> >> + * \\return a Value with the maximum setting for the control\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Provide a string representation of the ControlInfo\n> >> + */\n> >> +std::string ControlInfo::toString() const\n> >> +{\n> >> +\tstd::stringstream ss;\n> >> +\n> >> +\tss << \"Control: \" << name()\n> >> +\t << \" : Min(\" << min_ << \") Max(\" << max_ << \")\";\n> > \n> > I still need to look at how you use this, but in general it's best to\n> > shorten the representations as much as possible. I may remove the\n> > \"Control:\" prefix, maybe with\n> > \n> > \tss << name() << \"[\" << min_ << \",\" << max_ << \"]\";\n> > \n> > This would print\n> > \n> > \tManualExposure[1,1000]\n> > \n> > We could replace the comma with ..\n> > \n> > \tManualExposure[1..1000]\n> \n> I like that !\n> \n> Done.\n> \n> >> +\n> >> +\treturn ss.str();\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::operator==(const ControlInfo &rhs) const\n> >> + * \\brief Establish equivalence of ControlInfo. Only the IDs are considered.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn ControlInfo::operator==(const ControlId rhs) const\n> >> + * \\brief Establish equivalence of ControlInfo, against a ControlID.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Provide a string stream representation of the ControlInfo \\a info to\n> >> + * the \\a stream.\n> >> + */\n> >> +std::ostream &operator<<(std::ostream &stream, const ControlInfo &info)\n> >> +{\n> >> +\treturn stream << info.toString();\n> >> +}\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index 8e68373118df..e2c07d79bfb5 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -3,6 +3,7 @@ libcamera_sources = files([\n> >> 'camera.cpp',\n> >> 'camera_manager.cpp',\n> >> 'camera_sensor.cpp',\n> >> + 'controls.cpp',\n> >> 'device_enumerator.cpp',\n> >> 'device_enumerator_sysfs.cpp',\n> >> 'event_dispatcher.cpp',","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4BF6761580\n\tfor ;\n\tMon, 24 Jun 2019 19:58:54 +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 9B7D2510;\n\tMon, 24 Jun 2019 19:58:53 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1561399133;\n\tbh=mgxnJbHijR45HcUbaCcuVjlyh5zqv9eOlz+Sjnw5J8A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HsBZ1/jOsObkhpU1zoCiMEC4uQeeUVngW1W3zzjBkxGzbT2u0FbqlLPRgTAHMwtag\n\ta3IBvHmtF0ug+DfTbv58o2QXNP8Al/jETFpa7nbrZqdcYppbDI66rpE6YsCDjeChI9\n\tPmd3nPfnYyEcZJAf4AnAQ/YNmYhQyhpNMr18eiXk=","Date":"Mon, 24 Jun 2019 20:58:33 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20190624175833.GR5737@pendragon.ideasonboard.com>","References":"<20190621161401.28337-1-kieran.bingham@ideasonboard.com>\n\t<20190621161401.28337-4-kieran.bingham@ideasonboard.com>\n\t<20190623215240.GB6124@pendragon.ideasonboard.com>\n\t<6b48dcd6-3d55-cad1-0cf4-1a8f074ca4b3@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<6b48dcd6-3d55-cad1-0cf4-1a8f074ca4b3@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC PATCH v2 3/9] libcamera: controls:\n\tIntroduce Control structures","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Jun 2019 17:58:54 -0000"}}]