| Message ID | 20190918103424.14536-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 18/09/2019 11:34, Jacopo Mondi wrote: > Now that the control data value and info storage have been unified with > V4L2 control a refresh of naming and minor bits had to be performed. > The control framework implementation uses the word 'control' in types > and definition to refer to different things, making it harder to follow. > > Perform a rename of elements defined by the control framework to enforce > the following concepts: > - control metadata: > static control metadata information such as id, type and name, defined by > libcamera > - control information: > constant information associated with a control used for validation of > control values, provided by pipeline handlers and defined by the > capabilities of the device the control applies to > - control: > a control identifier with an associated value, provided by > applications when setting a control to a specific value You've defined a 'Control' as an alias to DataValue. I suspect that perhaps ControlValue would have been more correct, because Control does not store the control identifier. Thus a 'Control' should be a pair of both a ControlID and a ControlValue... ? Or maybe this makes me think that DataValue should be renamed to just Value then there's no need to express a ControlValue separately... > > Update the framework documentation trying to use those concepts > consistently. I think this patch needs a bit of cleanup, and there are definitely some unrelated arbitrary changes to separate out. There's probably not even that many in fact. Anyway - various discussion points below. (Oh, and now above too) > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/controls.h | 19 ++-- > src/libcamera/controls.cpp | 129 +++++++++++++++++----------- > src/libcamera/gen-controls.awk | 2 +- > src/libcamera/meson.build | 6 +- > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > src/libcamera/pipeline/vimc.cpp | 2 +- > test/controls/control_list.cpp | 2 +- > 7 files changed, 97 insertions(+), 65 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index d3065c0f3f16..174bc92732aa 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -19,7 +19,7 @@ namespace libcamera { > > class Camera; > > -struct ControlIdentifier { > +struct ControlMetadata { > ControlId id; > const char *name; > DataType type; > @@ -37,21 +37,22 @@ public: > std::string toString() const; > > private: > - const struct ControlIdentifier *ident_; > + const struct ControlMetadata *ident_; Shouldn't ident_ be renamed to meta_ as well? > }; > > using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>; > > +using Control = DataValue; I'm initially hesitant here, but I can't actually find a way to disagree :-D I think https://google.github.io/styleguide/cppguide.html#Aliases makes it clear that aliases are acceptable, but should be clearly documented. Not sure what documentation we should provide, but we should likely have something explaining what a Control is used for at a high level, and that it maps to the underlying data type of a DataValue. <Also perhaps a newline here> > class ControlList > { > private: > - using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; > + using ControlMap = std::unordered_map<const ControlInfo *, Control>; > > public: > ControlList(const ControlInfoMap &infoMap); > > - using iterator = ControlListMap::iterator; > - using const_iterator = ControlListMap::const_iterator; > + using iterator = ControlMap::iterator; > + using const_iterator = ControlMap::const_iterator; > > iterator begin() { return controls_.begin(); } > iterator end() { return controls_.end(); } > @@ -64,14 +65,14 @@ public: > std::size_t size() const { return controls_.size(); } > void clear() { controls_.clear(); } > > - DataValue &operator[](ControlId id); > - DataValue &operator[](const ControlInfo *info) { return controls_[info]; } > + Control &operator[](ControlId id); > + Control &operator[](const ControlInfo *info) { return controls_[info]; } > > - void update(const ControlList &list); > + bool merge(const ControlList &list); > > private: > const ControlInfoMap &infoMap_; > - ControlListMap controls_; > + ControlMap controls_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 184d544c05bc..028ffc1e80b9 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -7,9 +7,6 @@ > > #include <libcamera/controls.h> > > -#include <sstream> > -#include <string> > - > #include <libcamera/camera.h> > > #include "log.h" > @@ -17,7 +14,7 @@ > > /** > * \file controls.h > - * \brief Describes control framework and controls supported by a camera > + * \brief Control framework implementation > */ > > namespace libcamera { > @@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls) > */ > > /** > - * \struct ControlIdentifier > - * \brief Describe a ControlId with control specific constant meta-data > + * \struct ControlMetadata > + * \brief Describe the control static meta-data > + * > + * Defines the Control static meta-data information, auto-generated from the > + * ControlId documentation. > + * > + * The control information are defined in the control_id.h file, parsed by s/are/is/ > + * the gen-controls.awk script to produce a control_metadata.cpp file that I'm not sure that gen-controls.awk should be part of public documentation. That's an internal implementation detail, and gen-controls.awk is not distributed with the public-headers. > + * provides a static list of control id with an associated type and name. The > + * file is not for public use, and so no suitable header exists as > + * its sole usage is for the ControlMetadata definition. Aha, as stated here :-D So I think this documentation for struct ControlMetadata should focus on documenting ControlMetadata. > * > - * Defines a Control with a unique ID, a name, and a type. > - * This structure is used as static part of the auto-generated control > - * definitions, which are generated from the ControlId documentation. > + * \todo Expand the control meta-data definition to support more complex > + * control types and describe their characteristics (ie. Number of deta elements s/deta/data/ > + * for compound control types, versioning etc). > * > - * \var ControlIdentifier::id > + * \var ControlMetadata::id > * The unique ID for a control > - * \var ControlIdentifier::name > + * \var ControlMetadata::name > * The string representation of the control > - * \var ControlIdentifier::type > + * \var ControlMetadata::type > * The ValueType required to represent the control value > */ > > /* > - * The controlTypes are automatically generated to produce a control_types.cpp > - * output. This file is not for public use, and so no suitable header exists > - * for this sole usage of the controlTypes reference. As such the extern is > - * only defined here for use during the ControlInfo constructor and should not > + * The ControlTypes map is defined by the generated control_metadata.cpp file > + * and only used here during the ControlInfo construction and should not > * be referenced directly elsewhere. You've removed key information here, which I'm not sure should have been removed. It explained /why/ this extern is defined here, and not in a non-existent control_types.h file. > */ > -extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes; > +extern const std::unordered_map<ControlId, ControlMetadata> controlTypes; > > /** > * \class ControlInfo > * \brief Describe the information and capabilities of a Control > * > - * The ControlInfo represents control specific meta-data which is constant on a > - * per camera basis. ControlInfo classes are constructed by pipeline handlers > - * to expose the controls they support and the metadata needed to utilise those > - * controls. > + * The ControlInfo class associates the control's constant metadata defined by > + * libcamera and collected in the ControlMetadata class, with its static > + * information, such the range of supported values and the control size, > + * described by the DataInfo base class and defined by the capabilities of > + * the Camera device that implements the control. > + * > + * ControlInfo instances are constructed by pipeline handlers and associated > + * with the Camera devices they register to expose the list of controls they > + * support and the static information required to use those controls. > */ > > /** > @@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, > { > auto iter = controlTypes.find(id); > if (iter == controlTypes.end()) { > - LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo"; > + LOG(Controls, Fatal) << "Control " << id << " not defined"; Reporting the id here is indeed more helpful :-) > return; > } > > @@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, > > /** > * \fn ControlInfo::id() > - * \brief Retrieve the control ID > + * \brief Retrieve the control ID from the control constant metadata > * \return The control ID > */ > > /** > * \fn ControlInfo::name() > - * \brief Retrieve the control name string > + * \brief Retrieve the control name string from the control constant metadata > * \return The control name string > */ > > /** > * \fn ControlInfo::type() > - * \brief Retrieve the control designated type > + * \brief Retrieve the control designated type from the control constant metadata > * \return The control type > */ > > @@ -161,21 +170,37 @@ std::string ControlInfo::toString() const > * \brief A map of ControlId to ControlInfo > */ > > +/** > + * \typedef Control Aha - here's the documentation I asked for earlier. I thought I looked for it but didn't find it in my first pass :D > + * \brief Use 'Control' in place of DataValue in the ControlList class I would instead change the brief to what a Control is used for. The fact that a Control is a DataValue is an implementation detail. Something like: "\brief A Control defines the value ..." Ah ... Ok - so I've just hit a logic fail. A "Control" is a pairing of the Control ID and a value isn't it? But here - it's just the value... - Moving that discussion up to the top commit message discussion on what a Control is. > + * > + * \todo If more data and operations than the ones defined by DataValue > + * will need to be implemented for controls, make this typedef a class that > + * inherits from DataValue. I'm not sure that todo helps. It's more of a private comment than a public-documentation todo action. > + */ > + > /** > * \class ControlList > - * \brief Associate a list of ControlId with their values for a camera > + * \brief List of controls values > * > - * A ControlList wraps a map of ControlId to DataValue and provide > - * additional validation against the control information exposed by a Camera. > + * A ControlList wraps a map of ControlId to Control instances and provide > + * additional validation against the control information reported by a Camera. > * > - * A list is only valid for as long as the camera it refers to is valid. After > - * that calling any method of the ControlList class other than its destructor > - * will cause undefined behaviour. > + * ControlList instances are initially created empty and should be populated by > + * assigning a value to each control added to the list. The added control is > + * validated to verify it is supported by the Camera the list refers to, and > + * validate the provided value against the static information provided by the s/ / / (double space between against "and" "the") > + * ControlInfo instance associated with the control. Now that we've converted from storing a camera * to a ControlList reference from the Camera, is it still possible that someone could delete a Camera - and if so - what happens to the validity of the ControlList associated with it, as we have taken a reference. I don't think it magically became safe, so do we need to keep the warning regarding the validity ? > */ > > /** > - * \brief Construct a ControlList with a reference to the Camera it applies on > + * \brief Construct a ControlList with a map of control information Hrm ... that should probably be moved to the previous patch which changes the Control * to a ControlInfoMap reference. > * \param[in] infoMap The ControlInfoMap of the camera the control list refers to > + * > + * The provided \a infoMap collects the control id and the associated static > + * information for each control that can be set on the list. \a infoMap is > + * provided by the Camera the list of controls refers to and it is used to make > + * sure only controls supported by the camera can be added to the list. > */ > ControlList::ControlList(const ControlInfoMap &infoMap) > : infoMap_(infoMap) > @@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap) > /** > * \brief Check if the list contains a control with the specified \a id > * \param[in] id The control ID > - * > - * The behaviour is undefined if the control \a id is not supported by the > - * camera that the ControlList refers to. > - * Is this no longer true? I guess so. > * \return True if the list contains a matching control, false otherwise > */ > bool ControlList::contains(ControlId id) const > @@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const > /** > * \fn ControlList::size() > * \brief Retrieve the number of controls in the list > - * \return The number of DataValue entries stored in the list > + * \return The number of Control entries stored in the list > */ > > /** > @@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const > * The behaviour is undefined if the control \a id is not supported by the I see another undefined behaviour reference there... this needs to be consistent (and correct). > * camera that the ControlList refers to. > * > - * \return A reference to the value of the control identified by \a id > + * \return A reference to the value of the control identified by \a id if the > + * control is supported, an empty control otherwise Is the change from undefined behaviour to returning an emtpy control in this series? If so this documentation change should go in the relevant patch. > */ > -DataValue &ControlList::operator[](ControlId id) > +Control &ControlList::operator[](ControlId id) > { > const auto info = infoMap_.find(id); > if (info == infoMap_.end()) { > LOG(Controls, Error) << "Control " << id << " not supported"; > > - static DataValue empty; > + static Control empty; > return empty; > } > > @@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id) > * This method returns a reference to the control identified by \a info, > * inserting it in the list if the info is not already present. > * > - * \return A reference to the value of the control identified by \a info > + * The behaviour is undefined if the control \a info is not supported by the > + * camera that the ControlList refers to. I'm confused... What's the difference between an empty control, and one that is not supported by the camera... > + * > + * \return A reference to the value of the control identified by \a info if the > + * control is supported, an mpty control otherwise s/mpty/empty/ > */ > > /** > - * \brief Update the list with a union of itself and \a other > - * \param other The other list > + * \brief Merge the control in the list with controls from \a other > + * \param other The control list to merge with > * > * Update the control list to include all values from the \a other list. > - * Elements in the list whose control IDs are contained in \a other are updated > + * Elements in the list whose control IDs are contained in \a other are merged > * with the value from \a other. Elements in the \a other list that have no > * corresponding element in the list are added to the list with their value. > * > - * The behaviour is undefined if the two lists refer to different Camera > - * instances. > + * All controls in \a other should be supported by the list, otherwise no > + * control is updated. > + * > + * \return True if control list have been merged, false otherwise > + * > */ > -void ControlList::update(const ControlList &other) > +bool ControlList::merge(const ControlList &other) Why this rename? This seems like an arbitrary API change. If it's suitable, then it's probably it's own patch. > { > /* > * Make sure if all controls in other's list are supported by this > @@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other) > > if (infoMap_.find(id) == infoMap_.end()) { > LOG(Controls, Error) > - << "Failed to update control list with control: " > + << "Failed to merge control list with control: " > << id; > - return; > + return false; > } > } > > for (auto &control : other) > controls_[control.first] = control.second; > + > + return true; > } > > } /* namespace libcamera */ > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk > index abeb0218546b..b7f6532dfd43 100755 > --- a/src/libcamera/gen-controls.awk > +++ b/src/libcamera/gen-controls.awk > @@ -89,7 +89,7 @@ function GenerateTable(file) { > > EnterNameSpace(file) > > - print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file > + print "extern const std::unordered_map<ControlId, ControlMetadata>" > file > print "controlTypes {" > file > for (i=1; i <= id; ++i) { > printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index c3100a1709e0..c4fcd0569bd7 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -60,13 +60,13 @@ endif > > gen_controls = files('gen-controls.awk') > > -control_types_cpp = custom_target('control_types_cpp', > +control_metadata_cpp = custom_target('control_metadata_cpp', > input : files('controls.cpp'), > - output : 'control_types.cpp', > + output : 'control_metadata.cpp', > depend_files : gen_controls, > command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@']) > > -libcamera_sources += control_types_cpp > +libcamera_sources += control_metadata_cpp > > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh') > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > index dd253f94ca3d..d7f7fb0d6322 100644 > --- a/src/libcamera/pipeline/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo.cpp > @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > > for (auto it : request->controls()) { > const ControlInfo *ci = it.first; > - DataValue &value = it.second; > + Control &value = it.second; > > switch (ci->id()) { > case Brightness: > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > index 3df239bdb277..1d36ec54bc3b 100644 > --- a/src/libcamera/pipeline/vimc.cpp > +++ b/src/libcamera/pipeline/vimc.cpp > @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) > > for (auto it : request->controls()) { > const ControlInfo *ci = it.first; > - DataValue &value = it.second; > + Control &value = it.second; > > switch (ci->id()) { > case Brightness: > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index 3c6eb40c091b..2a90cffe5106 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -159,7 +159,7 @@ protected: > > newList[Brightness] = 128; > newList[Saturation] = 255; > - newList.update(list); > + newList.merge(list); You've changed the merge operation to return a value. Perhaps it's worth checking it if you think that change was warranted. > > list.clear(); > > -- > 2.23.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran On Fri, Sep 20, 2019 at 01:56:42PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 18/09/2019 11:34, Jacopo Mondi wrote: > > Now that the control data value and info storage have been unified with > > V4L2 control a refresh of naming and minor bits had to be performed. > > The control framework implementation uses the word 'control' in types > > and definition to refer to different things, making it harder to follow. > > > > Perform a rename of elements defined by the control framework to enforce > > the following concepts: > > - control metadata: > > static control metadata information such as id, type and name, defined by > > libcamera > > - control information: > > constant information associated with a control used for validation of > > control values, provided by pipeline handlers and defined by the > > capabilities of the device the control applies to > > - control: > > a control identifier with an associated value, provided by > > applications when setting a control to a specific value > > You've defined a 'Control' as an alias to DataValue. I suspect that > perhaps ControlValue would have been more correct, because Control does > > Thus a 'Control' should be a pair of both a ControlID and a > ControlValue... ? > > Or maybe this makes me think that DataValue should be renamed to just > Value then there's no need to express a ControlValue separately... I tried to shorten names, as this is application API to and it's quite cumbersome to write. > > > > > > Update the framework documentation trying to use those concepts > > consistently. > > I think this patch needs a bit of cleanup, and there are definitely some > unrelated arbitrary changes to separate out. There's probably not even > that many in fact. > > Anyway - various discussion points below. (Oh, and now above too) > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/controls.h | 19 ++-- > > src/libcamera/controls.cpp | 129 +++++++++++++++++----------- > > src/libcamera/gen-controls.awk | 2 +- > > src/libcamera/meson.build | 6 +- > > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > > src/libcamera/pipeline/vimc.cpp | 2 +- > > test/controls/control_list.cpp | 2 +- > > 7 files changed, 97 insertions(+), 65 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index d3065c0f3f16..174bc92732aa 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -19,7 +19,7 @@ namespace libcamera { > > > > class Camera; > > > > -struct ControlIdentifier { > > +struct ControlMetadata { > > ControlId id; > > const char *name; > > DataType type; > > @@ -37,21 +37,22 @@ public: > > std::string toString() const; > > > > private: > > - const struct ControlIdentifier *ident_; > > + const struct ControlMetadata *ident_; > > Shouldn't ident_ be renamed to meta_ as well? > Possibly, yes > > }; > > > > using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>; > > > > +using Control = DataValue; > > I'm initially hesitant here, but I can't actually find a way to disagree :-D > > I think https://google.github.io/styleguide/cppguide.html#Aliases makes > it clear that aliases are acceptable, but should be clearly documented. > > Not sure what documentation we should provide, but we should likely have > something explaining what a Control is used for at a high level, and > that it maps to the underlying data type of a DataValue. > > > <Also perhaps a newline here> > > > > class ControlList > > { > > private: > > - using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; > > + using ControlMap = std::unordered_map<const ControlInfo *, Control>; > > > > public: > > ControlList(const ControlInfoMap &infoMap); > > > > - using iterator = ControlListMap::iterator; > > - using const_iterator = ControlListMap::const_iterator; > > + using iterator = ControlMap::iterator; > > + using const_iterator = ControlMap::const_iterator; > > > > iterator begin() { return controls_.begin(); } > > iterator end() { return controls_.end(); } > > @@ -64,14 +65,14 @@ public: > > std::size_t size() const { return controls_.size(); } > > void clear() { controls_.clear(); } > > > > - DataValue &operator[](ControlId id); > > - DataValue &operator[](const ControlInfo *info) { return controls_[info]; } > > + Control &operator[](ControlId id); > > + Control &operator[](const ControlInfo *info) { return controls_[info]; } > > > > - void update(const ControlList &list); > > + bool merge(const ControlList &list); > > > > private: > > const ControlInfoMap &infoMap_; > > - ControlListMap controls_; > > + ControlMap controls_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 184d544c05bc..028ffc1e80b9 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -7,9 +7,6 @@ > > > > #include <libcamera/controls.h> > > > > -#include <sstream> > > -#include <string> > > - > > #include <libcamera/camera.h> > > > > #include "log.h" > > @@ -17,7 +14,7 @@ > > > > /** > > * \file controls.h > > - * \brief Describes control framework and controls supported by a camera > > + * \brief Control framework implementation > > */ > > > > namespace libcamera { > > @@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls) > > */ > > > > /** > > - * \struct ControlIdentifier > > - * \brief Describe a ControlId with control specific constant meta-data > > + * \struct ControlMetadata > > + * \brief Describe the control static meta-data > > + * > > + * Defines the Control static meta-data information, auto-generated from the > > + * ControlId documentation. > > + * > > + * The control information are defined in the control_id.h file, parsed by > > s/are/is/ > > > + * the gen-controls.awk script to produce a control_metadata.cpp file that > > I'm not sure that gen-controls.awk should be part of public documentation. > > That's an internal implementation detail, and gen-controls.awk is not > distributed with the public-headers. I see. I wanted to document it as the first time I had to deal with it, I had quite an hard time to find it out. > > > > > + * provides a static list of control id with an associated type and name. The > > + * file is not for public use, and so no suitable header exists as > > + * its sole usage is for the ControlMetadata definition. > > Aha, as stated here :-D > > So I think this documentation for struct ControlMetadata should focus on > documenting ControlMetadata. > > > * > > - * Defines a Control with a unique ID, a name, and a type. > > - * This structure is used as static part of the auto-generated control > > - * definitions, which are generated from the ControlId documentation. > > + * \todo Expand the control meta-data definition to support more complex > > + * control types and describe their characteristics (ie. Number of deta elements > > s/deta/data/ > > > > + * for compound control types, versioning etc). > > * > > - * \var ControlIdentifier::id > > + * \var ControlMetadata::id > > * The unique ID for a control > > - * \var ControlIdentifier::name > > + * \var ControlMetadata::name > > * The string representation of the control > > - * \var ControlIdentifier::type > > + * \var ControlMetadata::type > > * The ValueType required to represent the control value > > */ > > > > /* > > - * The controlTypes are automatically generated to produce a control_types.cpp > > - * output. This file is not for public use, and so no suitable header exists > > - * for this sole usage of the controlTypes reference. As such the extern is > > - * only defined here for use during the ControlInfo constructor and should not > > + * The ControlTypes map is defined by the generated control_metadata.cpp file > > + * and only used here during the ControlInfo construction and should not > > * be referenced directly elsewhere. > > You've removed key information here, which I'm not sure should have been > removed. It explained /why/ this extern is defined here, and not in a > non-existent control_types.h file. I moved it up, in the comment that referred to gen-controls.awka to explain where ControlMetadata come from. And to be precise "extern is only defined" did not made much sense. If a variable is defined with extern it is expected to be defined elsewhere, and I explained where it is defined, and that it is only used here. > > > > */ > > -extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes; > > +extern const std::unordered_map<ControlId, ControlMetadata> controlTypes; > > > > /** > > * \class ControlInfo > > * \brief Describe the information and capabilities of a Control > > * > > - * The ControlInfo represents control specific meta-data which is constant on a > > - * per camera basis. ControlInfo classes are constructed by pipeline handlers > > - * to expose the controls they support and the metadata needed to utilise those > > - * controls. > > + * The ControlInfo class associates the control's constant metadata defined by > > + * libcamera and collected in the ControlMetadata class, with its static > > + * information, such the range of supported values and the control size, > > + * described by the DataInfo base class and defined by the capabilities of > > + * the Camera device that implements the control. > > + * > > + * ControlInfo instances are constructed by pipeline handlers and associated > > + * with the Camera devices they register to expose the list of controls they > > + * support and the static information required to use those controls. > > */ > > > > /** > > @@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, > > { > > auto iter = controlTypes.find(id); > > if (iter == controlTypes.end()) { > > - LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo"; > > + LOG(Controls, Fatal) << "Control " << id << " not defined"; > > Reporting the id here is indeed more helpful :-) > > > return; > > } > > > > @@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, > > > > /** > > * \fn ControlInfo::id() > > - * \brief Retrieve the control ID > > + * \brief Retrieve the control ID from the control constant metadata > > * \return The control ID > > */ > > > > /** > > * \fn ControlInfo::name() > > - * \brief Retrieve the control name string > > + * \brief Retrieve the control name string from the control constant metadata > > * \return The control name string > > */ > > > > /** > > * \fn ControlInfo::type() > > - * \brief Retrieve the control designated type > > + * \brief Retrieve the control designated type from the control constant metadata > > * \return The control type > > */ > > > > @@ -161,21 +170,37 @@ std::string ControlInfo::toString() const > > * \brief A map of ControlId to ControlInfo > > */ > > > > +/** > > + * \typedef Control > > Aha - here's the documentation I asked for earlier. > > I thought I looked for it but didn't find it in my first pass :D > > > > + * \brief Use 'Control' in place of DataValue in the ControlList class > > I would instead change the brief to what a Control is used for. > > The fact that a Control is a DataValue is an implementation detail. > > Something like: > "\brief A Control defines the value ..." Ah ... > > Ok - so I've just hit a logic fail. A "Control" is a pairing of the > Control ID and a value isn't it? > > But here - it's just the value... - Moving that discussion up to the top > commit message discussion on what a Control is. > For appliation a Control is a value associated to a control. To reduce bikeshedding I would rather change this back to ControlValue if you feel more confortable, but to me it's just a longer name. > > > > + * > > + * \todo If more data and operations than the ones defined by DataValue > > + * will need to be implemented for controls, make this typedef a class that > > + * inherits from DataValue. > > I'm not sure that todo helps. It's more of a private comment than a > public-documentation todo action. > Yes, it's an implementation todo. Should I record it outside of /** ? > > + */ > > + > > /** > > * \class ControlList > > - * \brief Associate a list of ControlId with their values for a camera > > + * \brief List of controls values > > * > > - * A ControlList wraps a map of ControlId to DataValue and provide > > - * additional validation against the control information exposed by a Camera. > > + * A ControlList wraps a map of ControlId to Control instances and provide > > + * additional validation against the control information reported by a Camera. > > * > > - * A list is only valid for as long as the camera it refers to is valid. After > > - * that calling any method of the ControlList class other than its destructor > > - * will cause undefined behaviour. > > + * ControlList instances are initially created empty and should be populated by > > + * assigning a value to each control added to the list. The added control is > > + * validated to verify it is supported by the Camera the list refers to, and > > + * validate the provided value against the static information provided by the > > s/ / / (double space between against "and" "the") > > > + * ControlInfo instance associated with the control. > > Now that we've converted from storing a camera * to a ControlList > reference from the Camera, is it still possible that someone could > delete a Camera - and if so - what happens to the validity of the > ControlList associated with it, as we have taken a reference. I think that if a camera is removed: 1) The IPA module that uses the controlInfoMap is removed to 2) it is not possible to queue request to that camera But yes, you could keep adding controls to a control list that refers to a camera that has been removed, and the ControlInfoMap there referenced would be invalid. I think the right way to handle this would be using a shared pointer, to keep the InfoMap around as long as there are control lists using it. Or simply keep the warning for now. > > I don't think it magically became safe, so do we need to keep the > warning regarding the validity ? > > > > */ > > > > /** > > - * \brief Construct a ControlList with a reference to the Camera it applies on > > + * \brief Construct a ControlList with a map of control information > > Hrm ... that should probably be moved to the previous patch which > changes the Control * to a ControlInfoMap reference. > > > > * \param[in] infoMap The ControlInfoMap of the camera the control list refers to > > + * > > + * The provided \a infoMap collects the control id and the associated static > > + * information for each control that can be set on the list. \a infoMap is > > + * provided by the Camera the list of controls refers to and it is used to make > > > > > + * sure only controls supported by the camera can be added to the list. > > */ > > ControlList::ControlList(const ControlInfoMap &infoMap) > > : infoMap_(infoMap) > > @@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap) > > /** > > * \brief Check if the list contains a control with the specified \a id > > * \param[in] id The control ID > > - * > > - * The behaviour is undefined if the control \a id is not supported by the > > - * camera that the ControlList refers to. > > - * > > Is this no longer true? I guess so. According to the above comment, yes, it might happen > > > > * \return True if the list contains a matching control, false otherwise > > */ > > bool ControlList::contains(ControlId id) const > > @@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const > > /** > > * \fn ControlList::size() > > * \brief Retrieve the number of controls in the list > > - * \return The number of DataValue entries stored in the list > > + * \return The number of Control entries stored in the list > > */ > > > > /** > > @@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const > > * The behaviour is undefined if the control \a id is not supported by the > > I see another undefined behaviour reference there... this needs to be > consistent (and correct). > > > > * camera that the ControlList refers to. > > * > > - * \return A reference to the value of the control identified by \a id > > + * \return A reference to the value of the control identified by \a id if the > > + * control is supported, an empty control otherwise > > Is the change from undefined behaviour to returning an emtpy control in > this series? If so this documentation change should go in the relevant > patch. > > > > */ > > -DataValue &ControlList::operator[](ControlId id) > > +Control &ControlList::operator[](ControlId id) > > { > > const auto info = infoMap_.find(id); > > if (info == infoMap_.end()) { > > LOG(Controls, Error) << "Control " << id << " not supported"; > > > > - static DataValue empty; > > + static Control empty; > > return empty; > > } > > > > @@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id) > > * This method returns a reference to the control identified by \a info, > > * inserting it in the list if the info is not already present. > > * > > - * \return A reference to the value of the control identified by \a info > > + * The behaviour is undefined if the control \a info is not supported by the > > + * camera that the ControlList refers to. > > I'm confused... What's the difference between an empty control, and one > that is not supported by the camera... I agree the undefined behaviour here is not accurate. But the empty control is what is returned IF the control is not supported by the camera. > > > > + * > > + * \return A reference to the value of the control identified by \a info if the > > + * control is supported, an mpty control otherwise > > s/mpty/empty/ > > > > */ > > > > /** > > - * \brief Update the list with a union of itself and \a other > > - * \param other The other list > > + * \brief Merge the control in the list with controls from \a other > > + * \param other The control list to merge with > > * > > * Update the control list to include all values from the \a other list. > > - * Elements in the list whose control IDs are contained in \a other are updated > > + * Elements in the list whose control IDs are contained in \a other are merged > > * with the value from \a other. Elements in the \a other list that have no > > * corresponding element in the list are added to the list with their value. > > * > > - * The behaviour is undefined if the two lists refer to different Camera > > - * instances. > > + * All controls in \a other should be supported by the list, otherwise no > > + * control is updated. > > + * > > + * \return True if control list have been merged, false otherwise > > + * > > */ > > -void ControlList::update(const ControlList &other) > > +bool ControlList::merge(const ControlList &other) > > Why this rename? This seems like an arbitrary API change. If it's > suitable, then it's probably it's own patch. Because the operation is a merge, it is also commented in tests with "merge two lists". Our API is floating, I don't this this change is worth a patch by itself. If you really care so much of having this operation called update() because you named it so, I can drop it to reduce discussions. > > > > { > > /* > > * Make sure if all controls in other's list are supported by this > > @@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other) > > > > if (infoMap_.find(id) == infoMap_.end()) { > > LOG(Controls, Error) > > - << "Failed to update control list with control: " > > + << "Failed to merge control list with control: " > > << id; > > - return; > > + return false; > > } > > } > > > > for (auto &control : other) > > controls_[control.first] = control.second; > > + > > + return true; > > } > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk > > index abeb0218546b..b7f6532dfd43 100755 > > --- a/src/libcamera/gen-controls.awk > > +++ b/src/libcamera/gen-controls.awk > > @@ -89,7 +89,7 @@ function GenerateTable(file) { > > > > EnterNameSpace(file) > > > > - print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file > > + print "extern const std::unordered_map<ControlId, ControlMetadata>" > file > > print "controlTypes {" > file > > for (i=1; i <= id; ++i) { > > printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index c3100a1709e0..c4fcd0569bd7 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -60,13 +60,13 @@ endif > > > > gen_controls = files('gen-controls.awk') > > > > -control_types_cpp = custom_target('control_types_cpp', > > +control_metadata_cpp = custom_target('control_metadata_cpp', > > input : files('controls.cpp'), > > - output : 'control_types.cpp', > > + output : 'control_metadata.cpp', > > depend_files : gen_controls, > > command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@']) > > > > -libcamera_sources += control_types_cpp > > +libcamera_sources += control_metadata_cpp > > > > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh') > > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > index dd253f94ca3d..d7f7fb0d6322 100644 > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > > > > for (auto it : request->controls()) { > > const ControlInfo *ci = it.first; > > - DataValue &value = it.second; > > + Control &value = it.second; > > > > switch (ci->id()) { > > case Brightness: > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > index 3df239bdb277..1d36ec54bc3b 100644 > > --- a/src/libcamera/pipeline/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc.cpp > > @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) > > > > for (auto it : request->controls()) { > > const ControlInfo *ci = it.first; > > - DataValue &value = it.second; > > + Control &value = it.second; > > > > switch (ci->id()) { > > case Brightness: > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > > index 3c6eb40c091b..2a90cffe5106 100644 > > --- a/test/controls/control_list.cpp > > +++ b/test/controls/control_list.cpp > > @@ -159,7 +159,7 @@ protected: > > > > newList[Brightness] = 128; > > newList[Saturation] = 255; > > - newList.update(list); > > + newList.merge(list); > > You've changed the merge operation to return a value. Perhaps it's worth > checking it if you think that change was warranted. Correct. Thanks j > > > > > > list.clear(); > > > > -- > > 2.23.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
On Mon, Sep 23, 2019 at 01:31:57PM +0200, Jacopo Mondi wrote: > Hi Kieran > > On Fri, Sep 20, 2019 at 01:56:42PM +0100, Kieran Bingham wrote: > > Hi Jacopo, > > > > On 18/09/2019 11:34, Jacopo Mondi wrote: > > > Now that the control data value and info storage have been unified with > > > V4L2 control a refresh of naming and minor bits had to be performed. > > > The control framework implementation uses the word 'control' in types > > > and definition to refer to different things, making it harder to follow. > > > > > > Perform a rename of elements defined by the control framework to enforce > > > the following concepts: > > > - control metadata: > > > static control metadata information such as id, type and name, defined by > > > libcamera > > > - control information: > > > constant information associated with a control used for validation of > > > control values, provided by pipeline handlers and defined by the > > > capabilities of the device the control applies to > > > - control: > > > a control identifier with an associated value, provided by > > > applications when setting a control to a specific value > > > > You've defined a 'Control' as an alias to DataValue. I suspect that > > perhaps ControlValue would have been more correct, because Control does > > > > > Thus a 'Control' should be a pair of both a ControlID and a > > ControlValue... ? > > > > Or maybe this makes me think that DataValue should be renamed to just > > Value then there's no need to express a ControlValue separately... > > I tried to shorten names, as this is application API to and it's quite > cumbersome to write. > > > > > > > > > > > Update the framework documentation trying to use those concepts > > > consistently. > > > > I think this patch needs a bit of cleanup, and there are definitely some > > unrelated arbitrary changes to separate out. There's probably not even > > that many in fact. > > > > Anyway - various discussion points below. (Oh, and now above too) > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > include/libcamera/controls.h | 19 ++-- > > > src/libcamera/controls.cpp | 129 +++++++++++++++++----------- > > > src/libcamera/gen-controls.awk | 2 +- > > > src/libcamera/meson.build | 6 +- > > > src/libcamera/pipeline/uvcvideo.cpp | 2 +- > > > src/libcamera/pipeline/vimc.cpp | 2 +- > > > test/controls/control_list.cpp | 2 +- > > > 7 files changed, 97 insertions(+), 65 deletions(-) > > > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > > index d3065c0f3f16..174bc92732aa 100644 > > > --- a/include/libcamera/controls.h > > > +++ b/include/libcamera/controls.h > > > @@ -19,7 +19,7 @@ namespace libcamera { > > > > > > class Camera; > > > > > > -struct ControlIdentifier { > > > +struct ControlMetadata { > > > ControlId id; > > > const char *name; > > > DataType type; > > > @@ -37,21 +37,22 @@ public: > > > std::string toString() const; > > > > > > private: > > > - const struct ControlIdentifier *ident_; > > > + const struct ControlMetadata *ident_; > > > > Shouldn't ident_ be renamed to meta_ as well? > > > > Possibly, yes > > > > }; > > > > > > using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>; > > > > > > +using Control = DataValue; > > > > I'm initially hesitant here, but I can't actually find a way to disagree :-D > > > > I think https://google.github.io/styleguide/cppguide.html#Aliases makes > > it clear that aliases are acceptable, but should be clearly documented. > > > > Not sure what documentation we should provide, but we should likely have > > something explaining what a Control is used for at a high level, and > > that it maps to the underlying data type of a DataValue. > > > > > > <Also perhaps a newline here> > > > > > > > class ControlList > > > { > > > private: > > > - using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; > > > + using ControlMap = std::unordered_map<const ControlInfo *, Control>; > > > > > > public: > > > ControlList(const ControlInfoMap &infoMap); > > > > > > - using iterator = ControlListMap::iterator; > > > - using const_iterator = ControlListMap::const_iterator; > > > + using iterator = ControlMap::iterator; > > > + using const_iterator = ControlMap::const_iterator; > > > > > > iterator begin() { return controls_.begin(); } > > > iterator end() { return controls_.end(); } > > > @@ -64,14 +65,14 @@ public: > > > std::size_t size() const { return controls_.size(); } > > > void clear() { controls_.clear(); } > > > > > > - DataValue &operator[](ControlId id); > > > - DataValue &operator[](const ControlInfo *info) { return controls_[info]; } > > > + Control &operator[](ControlId id); > > > + Control &operator[](const ControlInfo *info) { return controls_[info]; } > > > > > > - void update(const ControlList &list); > > > + bool merge(const ControlList &list); > > > > > > private: > > > const ControlInfoMap &infoMap_; > > > - ControlListMap controls_; > > > + ControlMap controls_; > > > }; > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index 184d544c05bc..028ffc1e80b9 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -7,9 +7,6 @@ > > > > > > #include <libcamera/controls.h> > > > > > > -#include <sstream> > > > -#include <string> > > > - > > > #include <libcamera/camera.h> > > > > > > #include "log.h" > > > @@ -17,7 +14,7 @@ > > > > > > /** > > > * \file controls.h > > > - * \brief Describes control framework and controls supported by a camera > > > + * \brief Control framework implementation > > > */ > > > > > > namespace libcamera { > > > @@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls) > > > */ > > > > > > /** > > > - * \struct ControlIdentifier > > > - * \brief Describe a ControlId with control specific constant meta-data > > > + * \struct ControlMetadata > > > + * \brief Describe the control static meta-data > > > + * > > > + * Defines the Control static meta-data information, auto-generated from the > > > + * ControlId documentation. > > > + * > > > + * The control information are defined in the control_id.h file, parsed by > > > > s/are/is/ > > > > > + * the gen-controls.awk script to produce a control_metadata.cpp file that > > > > I'm not sure that gen-controls.awk should be part of public documentation. > > > > That's an internal implementation detail, and gen-controls.awk is not > > distributed with the public-headers. > > I see. I wanted to document it as the first time I had to deal with > it, I had quite an hard time to find it out. > > > > > > > > > > + * provides a static list of control id with an associated type and name. The > > > + * file is not for public use, and so no suitable header exists as > > > + * its sole usage is for the ControlMetadata definition. > > > > Aha, as stated here :-D > > > > So I think this documentation for struct ControlMetadata should focus on > > documenting ControlMetadata. > > > > > * > > > - * Defines a Control with a unique ID, a name, and a type. > > > - * This structure is used as static part of the auto-generated control > > > - * definitions, which are generated from the ControlId documentation. > > > + * \todo Expand the control meta-data definition to support more complex > > > + * control types and describe their characteristics (ie. Number of deta elements > > > > s/deta/data/ > > > > > > > + * for compound control types, versioning etc). > > > * > > > - * \var ControlIdentifier::id > > > + * \var ControlMetadata::id > > > * The unique ID for a control > > > - * \var ControlIdentifier::name > > > + * \var ControlMetadata::name > > > * The string representation of the control > > > - * \var ControlIdentifier::type > > > + * \var ControlMetadata::type > > > * The ValueType required to represent the control value > > > */ > > > > > > /* > > > - * The controlTypes are automatically generated to produce a control_types.cpp > > > - * output. This file is not for public use, and so no suitable header exists > > > - * for this sole usage of the controlTypes reference. As such the extern is > > > - * only defined here for use during the ControlInfo constructor and should not > > > + * The ControlTypes map is defined by the generated control_metadata.cpp file > > > + * and only used here during the ControlInfo construction and should not > > > * be referenced directly elsewhere. > > > > You've removed key information here, which I'm not sure should have been > > removed. It explained /why/ this extern is defined here, and not in a > > non-existent control_types.h file. > > I moved it up, in the comment that referred to gen-controls.awka to > explain where ControlMetadata come from. > > And to be precise "extern is only defined" did not made much sense. If > a variable is defined with extern it is expected to be defined > elsewhere, and I explained where it is defined, and that it is only > used here. > > > > > > > > */ > > > -extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes; > > > +extern const std::unordered_map<ControlId, ControlMetadata> controlTypes; > > > > > > /** > > > * \class ControlInfo > > > * \brief Describe the information and capabilities of a Control > > > * > > > - * The ControlInfo represents control specific meta-data which is constant on a > > > - * per camera basis. ControlInfo classes are constructed by pipeline handlers > > > - * to expose the controls they support and the metadata needed to utilise those > > > - * controls. > > > + * The ControlInfo class associates the control's constant metadata defined by > > > + * libcamera and collected in the ControlMetadata class, with its static > > > + * information, such the range of supported values and the control size, > > > + * described by the DataInfo base class and defined by the capabilities of > > > + * the Camera device that implements the control. > > > + * > > > + * ControlInfo instances are constructed by pipeline handlers and associated > > > + * with the Camera devices they register to expose the list of controls they > > > + * support and the static information required to use those controls. > > > */ > > > > > > /** > > > @@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, > > > { > > > auto iter = controlTypes.find(id); > > > if (iter == controlTypes.end()) { > > > - LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo"; > > > + LOG(Controls, Fatal) << "Control " << id << " not defined"; > > > > Reporting the id here is indeed more helpful :-) > > > > > return; > > > } > > > > > > @@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, > > > > > > /** > > > * \fn ControlInfo::id() > > > - * \brief Retrieve the control ID > > > + * \brief Retrieve the control ID from the control constant metadata > > > * \return The control ID > > > */ > > > > > > /** > > > * \fn ControlInfo::name() > > > - * \brief Retrieve the control name string > > > + * \brief Retrieve the control name string from the control constant metadata > > > * \return The control name string > > > */ > > > > > > /** > > > * \fn ControlInfo::type() > > > - * \brief Retrieve the control designated type > > > + * \brief Retrieve the control designated type from the control constant metadata > > > * \return The control type > > > */ > > > > > > @@ -161,21 +170,37 @@ std::string ControlInfo::toString() const > > > * \brief A map of ControlId to ControlInfo > > > */ > > > > > > +/** > > > + * \typedef Control > > > > Aha - here's the documentation I asked for earlier. > > > > I thought I looked for it but didn't find it in my first pass :D > > > > > > > + * \brief Use 'Control' in place of DataValue in the ControlList class > > > > I would instead change the brief to what a Control is used for. > > > > The fact that a Control is a DataValue is an implementation detail. > > > > Something like: > > "\brief A Control defines the value ..." Ah ... > > > > Ok - so I've just hit a logic fail. A "Control" is a pairing of the > > Control ID and a value isn't it? > > > > But here - it's just the value... - Moving that discussion up to the top > > commit message discussion on what a Control is. > > > > For appliation a Control is a value associated to a control. To reduce > bikeshedding I would rather change this back to ControlValue if you > feel more confortable, but to me it's just a longer name. > > > > > > > > + * > > > + * \todo If more data and operations than the ones defined by DataValue > > > + * will need to be implemented for controls, make this typedef a class that > > > + * inherits from DataValue. > > > > I'm not sure that todo helps. It's more of a private comment than a > > public-documentation todo action. > > > > Yes, it's an implementation todo. Should I record it outside of /** ? > > > > + */ > > > + > > > /** > > > * \class ControlList > > > - * \brief Associate a list of ControlId with their values for a camera > > > + * \brief List of controls values > > > * > > > - * A ControlList wraps a map of ControlId to DataValue and provide > > > - * additional validation against the control information exposed by a Camera. > > > + * A ControlList wraps a map of ControlId to Control instances and provide > > > + * additional validation against the control information reported by a Camera. > > > * > > > - * A list is only valid for as long as the camera it refers to is valid. After > > > - * that calling any method of the ControlList class other than its destructor > > > - * will cause undefined behaviour. > > > + * ControlList instances are initially created empty and should be populated by > > > + * assigning a value to each control added to the list. The added control is > > > + * validated to verify it is supported by the Camera the list refers to, and > > > + * validate the provided value against the static information provided by the > > > > s/ / / (double space between against "and" "the") > > > > > + * ControlInfo instance associated with the control. > > > > Now that we've converted from storing a camera * to a ControlList > > reference from the Camera, is it still possible that someone could > > delete a Camera - and if so - what happens to the validity of the > > ControlList associated with it, as we have taken a reference. > > I think that if a camera is removed: > 1) The IPA module that uses the controlInfoMap is removed to > 2) it is not possible to queue request to that camera > > But yes, you could keep adding controls to a control list that refers to a > camera that has been removed, and the ControlInfoMap there referenced > would be invalid. I think the right way to handle this would be using > a shared pointer, to keep the InfoMap around as long as there are > control lists using it. Or simply keep the warning for now. > > > > > I don't think it magically became safe, so do we need to keep the > > warning regarding the validity ? > > > > > > > */ > > > > > > /** > > > - * \brief Construct a ControlList with a reference to the Camera it applies on > > > + * \brief Construct a ControlList with a map of control information > > > > Hrm ... that should probably be moved to the previous patch which > > changes the Control * to a ControlInfoMap reference. > > > > > > > * \param[in] infoMap The ControlInfoMap of the camera the control list refers to > > > + * > > > + * The provided \a infoMap collects the control id and the associated static > > > + * information for each control that can be set on the list. \a infoMap is > > > + * provided by the Camera the list of controls refers to and it is used to make > > > > > > > > > + * sure only controls supported by the camera can be added to the list. > > > */ > > > ControlList::ControlList(const ControlInfoMap &infoMap) > > > : infoMap_(infoMap) > > > @@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap) > > > /** > > > * \brief Check if the list contains a control with the specified \a id > > > * \param[in] id The control ID > > > - * > > > - * The behaviour is undefined if the control \a id is not supported by the > > > - * camera that the ControlList refers to. > > > - * > > > > Is this no longer true? I guess so. > > According to the above comment, yes, it might happen > Sorry, to be more precise. In this case (the contains() operation) the behaviour is again undefined if the camera the list refers to is removed. If the control with id \a id is not supported by the camera, false is returned. Note that the implementation has not changed in this patch. $ git diff origin/master src/libcamera/controls.cpp bool ControlList::contains(ControlId id) const { - const ControlInfoMap &controls = camera_->controls(); - const auto iter = controls.find(id); - if (iter == controls.end()) { - LOG(Controls, Error) - << "Camera " << camera_->name() - << " does not support control " << id; + const auto info = infoMap_.find(id); + if (info == infoMap_.end()) { + LOG(Controls, Error) << "Control " << id << " not supported"; return false; } - return controls_.find(&iter->second) != controls_.end(); + return controls_.find(&info->second) != controls_.end(); } So I would re-add the undefined behaviour bits, here and in other places, but make sure to specify it happens when the camera this list refers to is removed. > > > > > > > > * \return True if the list contains a matching control, false otherwise > > > */ > > > bool ControlList::contains(ControlId id) const > > > @@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const > > > /** > > > * \fn ControlList::size() > > > * \brief Retrieve the number of controls in the list > > > - * \return The number of DataValue entries stored in the list > > > + * \return The number of Control entries stored in the list > > > */ > > > > > > /** > > > @@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const > > > * The behaviour is undefined if the control \a id is not supported by the > > > > I see another undefined behaviour reference there... this needs to be > > consistent (and correct). > > > > > > > * camera that the ControlList refers to. > > > * > > > - * \return A reference to the value of the control identified by \a id > > > + * \return A reference to the value of the control identified by \a id if the > > > + * control is supported, an empty control otherwise > > > > Is the change from undefined behaviour to returning an emtpy control in > > this series? If so this documentation change should go in the relevant > > patch. > > > > > > > */ > > > -DataValue &ControlList::operator[](ControlId id) > > > +Control &ControlList::operator[](ControlId id) > > > { > > > const auto info = infoMap_.find(id); > > > if (info == infoMap_.end()) { > > > LOG(Controls, Error) << "Control " << id << " not supported"; > > > > > > - static DataValue empty; > > > + static Control empty; > > > return empty; > > > } > > > > > > @@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id) > > > * This method returns a reference to the control identified by \a info, > > > * inserting it in the list if the info is not already present. > > > * > > > - * \return A reference to the value of the control identified by \a info > > > + * The behaviour is undefined if the control \a info is not supported by the > > > + * camera that the ControlList refers to. > > > > I'm confused... What's the difference between an empty control, and one > > that is not supported by the camera... > > I agree the undefined behaviour here is not accurate. But the empty > control is what is returned IF the control is not supported by the > camera. > > > > > > > > + * > > > + * \return A reference to the value of the control identified by \a info if the > > > + * control is supported, an mpty control otherwise > > > > s/mpty/empty/ > > > > > > > */ > > > > > > /** > > > - * \brief Update the list with a union of itself and \a other > > > - * \param other The other list > > > + * \brief Merge the control in the list with controls from \a other > > > + * \param other The control list to merge with > > > * > > > * Update the control list to include all values from the \a other list. > > > - * Elements in the list whose control IDs are contained in \a other are updated > > > + * Elements in the list whose control IDs are contained in \a other are merged > > > * with the value from \a other. Elements in the \a other list that have no > > > * corresponding element in the list are added to the list with their value. > > > * > > > - * The behaviour is undefined if the two lists refer to different Camera > > > - * instances. > > > + * All controls in \a other should be supported by the list, otherwise no > > > + * control is updated. > > > + * > > > + * \return True if control list have been merged, false otherwise > > > + * > > > */ > > > -void ControlList::update(const ControlList &other) > > > +bool ControlList::merge(const ControlList &other) > > > > Why this rename? This seems like an arbitrary API change. If it's > > suitable, then it's probably it's own patch. > > Because the operation is a merge, it is also commented in tests with > "merge two lists". > > Our API is floating, I don't this this change is worth a patch by > itself. If you really care so much of having this operation called > update() because you named it so, I can drop it to reduce discussions. > > > > > > > > { > > > /* > > > * Make sure if all controls in other's list are supported by this > > > @@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other) > > > > > > if (infoMap_.find(id) == infoMap_.end()) { > > > LOG(Controls, Error) > > > - << "Failed to update control list with control: " > > > + << "Failed to merge control list with control: " > > > << id; > > > - return; > > > + return false; > > > } > > > } > > > > > > for (auto &control : other) > > > controls_[control.first] = control.second; > > > + > > > + return true; > > > } > > > > > > } /* namespace libcamera */ > > > diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk > > > index abeb0218546b..b7f6532dfd43 100755 > > > --- a/src/libcamera/gen-controls.awk > > > +++ b/src/libcamera/gen-controls.awk > > > @@ -89,7 +89,7 @@ function GenerateTable(file) { > > > > > > EnterNameSpace(file) > > > > > > - print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file > > > + print "extern const std::unordered_map<ControlId, ControlMetadata>" > file > > > print "controlTypes {" > file > > > for (i=1; i <= id; ++i) { > > > printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > index c3100a1709e0..c4fcd0569bd7 100644 > > > --- a/src/libcamera/meson.build > > > +++ b/src/libcamera/meson.build > > > @@ -60,13 +60,13 @@ endif > > > > > > gen_controls = files('gen-controls.awk') > > > > > > -control_types_cpp = custom_target('control_types_cpp', > > > +control_metadata_cpp = custom_target('control_metadata_cpp', > > > input : files('controls.cpp'), > > > - output : 'control_types.cpp', > > > + output : 'control_metadata.cpp', > > > depend_files : gen_controls, > > > command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@']) > > > > > > -libcamera_sources += control_types_cpp > > > +libcamera_sources += control_metadata_cpp > > > > > > gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh') > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp > > > index dd253f94ca3d..d7f7fb0d6322 100644 > > > --- a/src/libcamera/pipeline/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo.cpp > > > @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) > > > > > > for (auto it : request->controls()) { > > > const ControlInfo *ci = it.first; > > > - DataValue &value = it.second; > > > + Control &value = it.second; > > > > > > switch (ci->id()) { > > > case Brightness: > > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp > > > index 3df239bdb277..1d36ec54bc3b 100644 > > > --- a/src/libcamera/pipeline/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc.cpp > > > @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) > > > > > > for (auto it : request->controls()) { > > > const ControlInfo *ci = it.first; > > > - DataValue &value = it.second; > > > + Control &value = it.second; > > > > > > switch (ci->id()) { > > > case Brightness: > > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > > > index 3c6eb40c091b..2a90cffe5106 100644 > > > --- a/test/controls/control_list.cpp > > > +++ b/test/controls/control_list.cpp > > > @@ -159,7 +159,7 @@ protected: > > > > > > newList[Brightness] = 128; > > > newList[Saturation] = 255; > > > - newList.update(list); > > > + newList.merge(list); > > > > You've changed the merge operation to return a value. Perhaps it's worth > > checking it if you think that change was warranted. > > Correct. > > Thanks > j > > > > > > > > > > > list.clear(); > > > > > > -- > > > 2.23.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > > -- > > Regards > > -- > > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index d3065c0f3f16..174bc92732aa 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -19,7 +19,7 @@ namespace libcamera { class Camera; -struct ControlIdentifier { +struct ControlMetadata { ControlId id; const char *name; DataType type; @@ -37,21 +37,22 @@ public: std::string toString() const; private: - const struct ControlIdentifier *ident_; + const struct ControlMetadata *ident_; }; using ControlInfoMap = std::unordered_map<ControlId, ControlInfo>; +using Control = DataValue; class ControlList { private: - using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; + using ControlMap = std::unordered_map<const ControlInfo *, Control>; public: ControlList(const ControlInfoMap &infoMap); - using iterator = ControlListMap::iterator; - using const_iterator = ControlListMap::const_iterator; + using iterator = ControlMap::iterator; + using const_iterator = ControlMap::const_iterator; iterator begin() { return controls_.begin(); } iterator end() { return controls_.end(); } @@ -64,14 +65,14 @@ public: std::size_t size() const { return controls_.size(); } void clear() { controls_.clear(); } - DataValue &operator[](ControlId id); - DataValue &operator[](const ControlInfo *info) { return controls_[info]; } + Control &operator[](ControlId id); + Control &operator[](const ControlInfo *info) { return controls_[info]; } - void update(const ControlList &list); + bool merge(const ControlList &list); private: const ControlInfoMap &infoMap_; - ControlListMap controls_; + ControlMap controls_; }; } /* namespace libcamera */ diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 184d544c05bc..028ffc1e80b9 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -7,9 +7,6 @@ #include <libcamera/controls.h> -#include <sstream> -#include <string> - #include <libcamera/camera.h> #include "log.h" @@ -17,7 +14,7 @@ /** * \file controls.h - * \brief Describes control framework and controls supported by a camera + * \brief Control framework implementation */ namespace libcamera { @@ -72,38 +69,50 @@ LOG_DEFINE_CATEGORY(Controls) */ /** - * \struct ControlIdentifier - * \brief Describe a ControlId with control specific constant meta-data + * \struct ControlMetadata + * \brief Describe the control static meta-data + * + * Defines the Control static meta-data information, auto-generated from the + * ControlId documentation. + * + * The control information are defined in the control_id.h file, parsed by + * the gen-controls.awk script to produce a control_metadata.cpp file that + * provides a static list of control id with an associated type and name. The + * file is not for public use, and so no suitable header exists as + * its sole usage is for the ControlMetadata definition. * - * Defines a Control with a unique ID, a name, and a type. - * This structure is used as static part of the auto-generated control - * definitions, which are generated from the ControlId documentation. + * \todo Expand the control meta-data definition to support more complex + * control types and describe their characteristics (ie. Number of deta elements + * for compound control types, versioning etc). * - * \var ControlIdentifier::id + * \var ControlMetadata::id * The unique ID for a control - * \var ControlIdentifier::name + * \var ControlMetadata::name * The string representation of the control - * \var ControlIdentifier::type + * \var ControlMetadata::type * The ValueType required to represent the control value */ /* - * The controlTypes are automatically generated to produce a control_types.cpp - * output. This file is not for public use, and so no suitable header exists - * for this sole usage of the controlTypes reference. As such the extern is - * only defined here for use during the ControlInfo constructor and should not + * The ControlTypes map is defined by the generated control_metadata.cpp file + * and only used here during the ControlInfo construction and should not * be referenced directly elsewhere. */ -extern const std::unordered_map<ControlId, ControlIdentifier> controlTypes; +extern const std::unordered_map<ControlId, ControlMetadata> controlTypes; /** * \class ControlInfo * \brief Describe the information and capabilities of a Control * - * The ControlInfo represents control specific meta-data which is constant on a - * per camera basis. ControlInfo classes are constructed by pipeline handlers - * to expose the controls they support and the metadata needed to utilise those - * controls. + * The ControlInfo class associates the control's constant metadata defined by + * libcamera and collected in the ControlMetadata class, with its static + * information, such the range of supported values and the control size, + * described by the DataInfo base class and defined by the capabilities of + * the Camera device that implements the control. + * + * ControlInfo instances are constructed by pipeline handlers and associated + * with the Camera devices they register to expose the list of controls they + * support and the static information required to use those controls. */ /** @@ -118,7 +127,7 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, { auto iter = controlTypes.find(id); if (iter == controlTypes.end()) { - LOG(Controls, Fatal) << "Attempt to create invalid ControlInfo"; + LOG(Controls, Fatal) << "Control " << id << " not defined"; return; } @@ -127,19 +136,19 @@ ControlInfo::ControlInfo(ControlId id, const DataValue &min, /** * \fn ControlInfo::id() - * \brief Retrieve the control ID + * \brief Retrieve the control ID from the control constant metadata * \return The control ID */ /** * \fn ControlInfo::name() - * \brief Retrieve the control name string + * \brief Retrieve the control name string from the control constant metadata * \return The control name string */ /** * \fn ControlInfo::type() - * \brief Retrieve the control designated type + * \brief Retrieve the control designated type from the control constant metadata * \return The control type */ @@ -161,21 +170,37 @@ std::string ControlInfo::toString() const * \brief A map of ControlId to ControlInfo */ +/** + * \typedef Control + * \brief Use 'Control' in place of DataValue in the ControlList class + * + * \todo If more data and operations than the ones defined by DataValue + * will need to be implemented for controls, make this typedef a class that + * inherits from DataValue. + */ + /** * \class ControlList - * \brief Associate a list of ControlId with their values for a camera + * \brief List of controls values * - * A ControlList wraps a map of ControlId to DataValue and provide - * additional validation against the control information exposed by a Camera. + * A ControlList wraps a map of ControlId to Control instances and provide + * additional validation against the control information reported by a Camera. * - * A list is only valid for as long as the camera it refers to is valid. After - * that calling any method of the ControlList class other than its destructor - * will cause undefined behaviour. + * ControlList instances are initially created empty and should be populated by + * assigning a value to each control added to the list. The added control is + * validated to verify it is supported by the Camera the list refers to, and + * validate the provided value against the static information provided by the + * ControlInfo instance associated with the control. */ /** - * \brief Construct a ControlList with a reference to the Camera it applies on + * \brief Construct a ControlList with a map of control information * \param[in] infoMap The ControlInfoMap of the camera the control list refers to + * + * The provided \a infoMap collects the control id and the associated static + * information for each control that can be set on the list. \a infoMap is + * provided by the Camera the list of controls refers to and it is used to make + * sure only controls supported by the camera can be added to the list. */ ControlList::ControlList(const ControlInfoMap &infoMap) : infoMap_(infoMap) @@ -221,10 +246,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap) /** * \brief Check if the list contains a control with the specified \a id * \param[in] id The control ID - * - * The behaviour is undefined if the control \a id is not supported by the - * camera that the ControlList refers to. - * * \return True if the list contains a matching control, false otherwise */ bool ControlList::contains(ControlId id) const @@ -258,7 +279,7 @@ bool ControlList::contains(const ControlInfo *info) const /** * \fn ControlList::size() * \brief Retrieve the number of controls in the list - * \return The number of DataValue entries stored in the list + * \return The number of Control entries stored in the list */ /** @@ -276,15 +297,16 @@ bool ControlList::contains(const ControlInfo *info) const * The behaviour is undefined if the control \a id is not supported by the * camera that the ControlList refers to. * - * \return A reference to the value of the control identified by \a id + * \return A reference to the value of the control identified by \a id if the + * control is supported, an empty control otherwise */ -DataValue &ControlList::operator[](ControlId id) +Control &ControlList::operator[](ControlId id) { const auto info = infoMap_.find(id); if (info == infoMap_.end()) { LOG(Controls, Error) << "Control " << id << " not supported"; - static DataValue empty; + static Control empty; return empty; } @@ -299,22 +321,29 @@ DataValue &ControlList::operator[](ControlId id) * This method returns a reference to the control identified by \a info, * inserting it in the list if the info is not already present. * - * \return A reference to the value of the control identified by \a info + * The behaviour is undefined if the control \a info is not supported by the + * camera that the ControlList refers to. + * + * \return A reference to the value of the control identified by \a info if the + * control is supported, an mpty control otherwise */ /** - * \brief Update the list with a union of itself and \a other - * \param other The other list + * \brief Merge the control in the list with controls from \a other + * \param other The control list to merge with * * Update the control list to include all values from the \a other list. - * Elements in the list whose control IDs are contained in \a other are updated + * Elements in the list whose control IDs are contained in \a other are merged * with the value from \a other. Elements in the \a other list that have no * corresponding element in the list are added to the list with their value. * - * The behaviour is undefined if the two lists refer to different Camera - * instances. + * All controls in \a other should be supported by the list, otherwise no + * control is updated. + * + * \return True if control list have been merged, false otherwise + * */ -void ControlList::update(const ControlList &other) +bool ControlList::merge(const ControlList &other) { /* * Make sure if all controls in other's list are supported by this @@ -326,14 +355,16 @@ void ControlList::update(const ControlList &other) if (infoMap_.find(id) == infoMap_.end()) { LOG(Controls, Error) - << "Failed to update control list with control: " + << "Failed to merge control list with control: " << id; - return; + return false; } } for (auto &control : other) controls_[control.first] = control.second; + + return true; } } /* namespace libcamera */ diff --git a/src/libcamera/gen-controls.awk b/src/libcamera/gen-controls.awk index abeb0218546b..b7f6532dfd43 100755 --- a/src/libcamera/gen-controls.awk +++ b/src/libcamera/gen-controls.awk @@ -89,7 +89,7 @@ function GenerateTable(file) { EnterNameSpace(file) - print "extern const std::unordered_map<ControlId, ControlIdentifier>" > file + print "extern const std::unordered_map<ControlId, ControlMetadata>" > file print "controlTypes {" > file for (i=1; i <= id; ++i) { printf "\t{ %s, { %s, \"%s\", DataType%s } },\n", names[i], names[i], names[i], types[i] > file diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index c3100a1709e0..c4fcd0569bd7 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -60,13 +60,13 @@ endif gen_controls = files('gen-controls.awk') -control_types_cpp = custom_target('control_types_cpp', +control_metadata_cpp = custom_target('control_metadata_cpp', input : files('controls.cpp'), - output : 'control_types.cpp', + output : 'control_metadata.cpp', depend_files : gen_controls, command : [gen_controls, '@INPUT@', '--code', '@OUTPUT@']) -libcamera_sources += control_types_cpp +libcamera_sources += control_metadata_cpp gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh') diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp index dd253f94ca3d..d7f7fb0d6322 100644 --- a/src/libcamera/pipeline/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo.cpp @@ -231,7 +231,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request) for (auto it : request->controls()) { const ControlInfo *ci = it.first; - DataValue &value = it.second; + Control &value = it.second; switch (ci->id()) { case Brightness: diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp index 3df239bdb277..1d36ec54bc3b 100644 --- a/src/libcamera/pipeline/vimc.cpp +++ b/src/libcamera/pipeline/vimc.cpp @@ -284,7 +284,7 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) for (auto it : request->controls()) { const ControlInfo *ci = it.first; - DataValue &value = it.second; + Control &value = it.second; switch (ci->id()) { case Brightness: diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index 3c6eb40c091b..2a90cffe5106 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -159,7 +159,7 @@ protected: newList[Brightness] = 128; newList[Saturation] = 255; - newList.update(list); + newList.merge(list); list.clear();
Now that the control data value and info storage have been unified with V4L2 control a refresh of naming and minor bits had to be performed. The control framework implementation uses the word 'control' in types and definition to refer to different things, making it harder to follow. Perform a rename of elements defined by the control framework to enforce the following concepts: - control metadata: static control metadata information such as id, type and name, defined by libcamera - control information: constant information associated with a control used for validation of control values, provided by pipeline handlers and defined by the capabilities of the device the control applies to - control: a control identifier with an associated value, provided by applications when setting a control to a specific value Update the framework documentation trying to use those concepts consistently. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/controls.h | 19 ++-- src/libcamera/controls.cpp | 129 +++++++++++++++++----------- src/libcamera/gen-controls.awk | 2 +- src/libcamera/meson.build | 6 +- src/libcamera/pipeline/uvcvideo.cpp | 2 +- src/libcamera/pipeline/vimc.cpp | 2 +- test/controls/control_list.cpp | 2 +- 7 files changed, 97 insertions(+), 65 deletions(-) -- 2.23.0