[{"id":2867,"web_url":"https://patchwork.libcamera.org/comment/2867/","msgid":"<20191013135004.lrzs5yxkpxp6gzha@uno.localdomain>","date":"2019-10-13T13:50:04","subject":"Re: [libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support\n\taccessing controls by numerical ID","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Oct 12, 2019 at 09:44:00PM +0300, Laurent Pinchart wrote:\n> The ControlList class has template get() and set() methods to get and\n> set control values. The methods require a reference to a Control\n> instance, which is only available when calling them with a hardcoded\n> control. In order to support usage of ControlList for V4L2 controls, as\n> well as serialisation and deserialisation of ControlList, we need a way\n> to get and set control values based on a control numerical ID. Add new\n> contains(), get() and set() overload methods to do so.\n>\n> As this change prepares the ControlList to be used for other objects\n> than camera, update its documentation accordingly.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  include/libcamera/controls.h   |  10 ++-\n>  src/ipa/rkisp1/rkisp1.cpp      |   2 +-\n>  src/libcamera/controls.cpp     | 143 ++++++++++++++++++++++++++-------\n>  src/libcamera/request.cpp      |   5 +-\n>  test/controls/control_list.cpp |   2 +-\n>  5 files changed, 125 insertions(+), 37 deletions(-)\n>\n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 999fcf7a3a62..5e6708fe570b 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -126,7 +126,7 @@ private:\n>  \tusing ControlListMap = std::unordered_map<const ControlId *, ControlValue>;\n>\n>  public:\n> -\tControlList(ControlValidator *validator = nullptr);\n> +\tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n>\n>  \tusing iterator = ControlListMap::iterator;\n>  \tusing const_iterator = ControlListMap::const_iterator;\n> @@ -136,11 +136,13 @@ public:\n>  \tconst_iterator begin() const { return controls_.begin(); }\n>  \tconst_iterator end() const { return controls_.end(); }\n>\n> -\tbool contains(const ControlId &id) const;\n>  \tbool empty() const { return controls_.empty(); }\n>  \tstd::size_t size() const { return controls_.size(); }\n>  \tvoid clear() { controls_.clear(); }\n>\n> +\tbool contains(const ControlId &id) const;\n> +\tbool contains(unsigned int id) const;\n> +\n>  \ttemplate<typename T>\n>  \tconst T &get(const Control<T> &ctrl) const\n>  \t{\n> @@ -163,11 +165,15 @@ public:\n>  \t\tval->set<T>(value);\n>  \t}\n>\n> +\tconst ControlValue &get(unsigned int id) const;\n> +\tvoid set(unsigned int id, const ControlValue &value);\n> +\n>  private:\n>  \tconst ControlValue *find(const ControlId &id) const;\n>  \tControlValue *find(const ControlId &id);\n>\n>  \tControlValidator *validator_;\n> +\tconst ControlIdMap *idmap_;\n>  \tControlListMap controls_;\n>  };\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 80138f196184..b0d23dd154be 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -220,7 +220,7 @@ void IPARkISP1::setControls(unsigned int frame)\n>\n>  void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n>  {\n> -\tControlList ctrls;\n> +\tControlList ctrls(controls::controls);\n\nJust to note down that providing a default argument to the ControList\ncontructor would shorten this. But I know you're in favour of having\ncontrols:controls explicitly passed.\n\n>\n>  \tif (aeState)\n>  \t\tctrls.set(controls::AeLocked, aeState == 2);\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 292e48cd6d25..ddd4e6680ce2 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -7,6 +7,7 @@\n>\n>  #include <libcamera/controls.h>\n>\n> +#include <iomanip>\n>  #include <sstream>\n>  #include <string>\n>\n> @@ -16,13 +17,13 @@\n>\n>  /**\n>   * \\file controls.h\n> - * \\brief Describes control framework and controls supported by a camera\n> + * \\brief Framework to handle manage controls related to an object\n\n\"to handle -and- manage\"\nOr just drop one of the two\n\n>   *\n> - * A control is a mean to govern or influence the operation of a camera. Every\n> - * control is defined by a unique numerical ID, a name string and the data type\n> - * of the value it stores. The libcamera API defines a set of standard controls\n> - * in the libcamera::controls namespace, as a set of instances of the Control\n> - * class.\n> + * A control is a mean to govern or influence the operation of an object, and in\n> + * particular of a camera. Every control is defined by a unique numerical ID, a\n> + * name string and the data type of the value it stores. The libcamera API\n> + * defines a set of standard controls in the libcamera::controls namespace, as\n> + * a set of instances of the Control class.\n>   *\n>   * The main way for applications to interact with controls is through the\n>   * ControlList stored in the Request class:\n> @@ -274,7 +275,7 @@ bool ControlValue::operator==(const ControlValue &other) const\n>   * \\class Control\n>   * \\brief Describe a control and its intrinsic properties\n>   *\n> - * The Control class models a control exposed by a camera. Its template type\n> + * The Control class models a control exposed by an object. Its template type\n>   * name T refers to the control data type, and allows methods that operate on\n>   * control values to be defined as template methods using the same type T for\n>   * the control value. See for instance how the ControlList::get() method\n> @@ -293,8 +294,8 @@ bool ControlValue::operator==(const ControlValue &other) const\n>   * long int).\n>   *\n>   * Controls IDs shall be unique. While nothing prevents multiple instances of\n> - * the Control class to be created with the same ID, this may lead to undefined\n> - * behaviour.\n> + * the Control class to be created with the same ID for the same object, doing\n> + * so may cause undefined behaviour.\n>   */\n>\n>  /**\n> @@ -398,18 +399,28 @@ std::string ControlRange::toString() const\n>\n>  /**\n>   * \\class ControlList\n> - * \\brief Associate a list of ControlId with their values for a camera\n> + * \\brief Associate a list of ControlId with their values for an object\n>   *\n> - * A ControlList wraps a map of ControlId to ControlValue and optionally\n> - * validates controls against a ControlValidator.\n> + * The ControlList class stores values of controls exposed by an object. The\n> + * lists returned by the Request::controls() and Request::metadata() methods\n> + * refer to the camera that the request belongs to.\n\nDo we want this ? It leaves V4L2 controls a bit out of the picture\n\n> + *\n> + * Control lists are constructed with a map of all the controls supported by\n> + * their object, and an optional ControlValidator to further validate the\n\nI would drop \"by their object\"\n\n> + * controls.\n>   */\n>\n>  /**\n>   * \\brief Construct a ControlList with an optional control validator\n> + * \\param[in] idmap The ControlId map for the control list target object\n>   * \\param[in] validator The validator (may be null)\n> + *\n> + * For ControlList containing libcamera controls, a global map of all libcamera\n> + * controls is provided by controls::controls and can be used as the \\a idmap\n> + * argument.\n>   */\n> -ControlList::ControlList(ControlValidator *validator)\n> -\t: validator_(validator)\n> +ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> +\t: validator_(validator), idmap_(&idmap)\n>  {\n>  }\n>\n> @@ -449,20 +460,6 @@ ControlList::ControlList(ControlValidator *validator)\n>   * list\n>   */\n>\n> -/**\n> - * \\brief Check if the list contains a control with the specified \\a id\n> - * \\param[in] id The control ID\n> - *\n> - * The behaviour is undefined if the control \\a id is not supported by the\n> - * camera that the ControlList refers to.\n> - *\n> - * \\return True if the list contains a matching control, false otherwise\n> - */\n> -bool ControlList::contains(const ControlId &id) const\n> -{\n> -\treturn controls_.find(&id) != controls_.end();\n> -}\n> -\n>  /**\n>   * \\fn ControlList::empty()\n>   * \\brief Identify if the list is empty\n> @@ -481,7 +478,33 @@ bool ControlList::contains(const ControlId &id) const\n>   */\n>\n>  /**\n> - * \\fn template<typename T> const T &ControlList::get() const\n> + * \\brief Check if the list contains a control with the specified \\a id\n> + * \\param[in] id The control ID\n> + *\n> + * \\return True if the list contains a matching control, false otherwise\n> + */\n> +bool ControlList::contains(const ControlId &id) const\n> +{\n> +\treturn controls_.find(&id) != controls_.end();\n> +}\n> +\n> +/**\n> + * \\brief Check if the list contains a control with the specified \\a id\n\nThe documentation of these two methods is the same. I assume it's ok,\nbut I would mentioned \"numeric id\" in the second version.\n\n> + * \\param[in] id The control numerical ID\n> + *\n> + * \\return True if the list contains a matching control, false otherwise\n> + */\n> +bool ControlList::contains(unsigned int id) const\n> +{\n> +\tconst auto iter = idmap_->find(id);\n> +\tif (iter == idmap_->end())\n> +\t\treturn false;\n> +\n> +\treturn contains(*iter->second);\n> +}\n> +\n> +/**\n> + * \\fn template<typename T> const T &ControlList::get(const Control<T> &ctrl) const\n>   * \\brief Get the value of a control\n>   * \\param[in] ctrl The control\n>   *\n> @@ -496,7 +519,7 @@ bool ControlList::contains(const ControlId &id) const\n>   */\n>\n>  /**\n> - * \\fn template<typename T> void ControlList::set()\n> + * \\fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value)\n>   * \\brief Set the control value to \\a value\n>   * \\param[in] ctrl The control\n>   * \\param[in] value The control value\n> @@ -506,9 +529,67 @@ bool ControlList::contains(const ControlId &id) const\n>   * to the list.\n>   *\n>   * The behaviour is undefined if the control \\a ctrl is not supported by the\n> - * camera that the list refers to.\n> + * object that the list refers to.\n>   */\n>\n> +/**\n> + * \\brief Get the value of control \\a id\n> + * \\param[in] id The control numerical ID\n> + *\n> + * The behaviour is undefined if the control \\a id is not present in the list.\n> + * Use ControlList::contains() to test for the presence of a control in the\n> + * list before retrieving its value.\n> + *\n> + * \\return The control value\n> + */\n> +const ControlValue &ControlList::get(unsigned int id) const\n\nThis, as the ControlId * oriented version, is very easy to miss if the\ncontrol is not present. Not an issue with this patch, anyway.\n\nThe pattern would be like\n\n        const ControlValue &ctrl = list.get(ID);\n        if (ctrl.isNone()) {\n                ...\n        }\n\nShould we document it?\n\n\n> +{\n> +\tstatic ControlValue zero;\n> +\n> +\tconst auto ctrl = idmap_->find(id);\n> +\tif (ctrl == idmap_->end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< std::hex << std::setfill('0')\n> +\t\t\t<< \"Control 0x\" << std::setw(8) << id << \" is not valid\";\n> +\t\treturn zero;\n> +\t}\n> +\n> +\tconst ControlValue *val = find(*ctrl->second);\n> +\tif (!val)\n> +\t\treturn zero;\n> +\n> +\treturn *val;\n> +}\n> +\n> +/**\n> + * \\brief Set the value of control \\a id to \\a value\n> + * \\param[in] id The control ID\n> + * \\param[in] value The control value\n> + *\n> + * This method sets the value of a control in the control list. If the control\n> + * is already present in the list, its value is updated, otherwise it is added\n> + * to the list.\n> + *\n> + * The behaviour is undefined if the control \\a id is not supported by the\n> + * object that the list refers to.\n> + */\n> +void ControlList::set(unsigned int id, const ControlValue &value)\n> +{\n> +\tconst auto ctrl = idmap_->find(id);\n> +\tif (ctrl == idmap_->end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< std::hex << std::setfill('0')\n> +\t\t\t<< \"Control 0x\" << std::setw(8) << id << \" is not valid\";\n\nHere and in the get operation \"is not valid\" or \"is not supported\" ?\n\nAnyway, all minor issues, so please add\nReviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThanks\n   j\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tControlValue *val = find(*ctrl->second);\n> +\tif (!val)\n> +\t\treturn;\n> +\n> +\t*val = value;\n> +}\n> +\n>  const ControlValue *ControlList::find(const ControlId &id) const\n>  {\n>  \tconst auto iter = controls_.find(&id);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index e800f1449888..c14ed1a4d3ce 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -11,6 +11,7 @@\n>\n>  #include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n> +#include <libcamera/control_ids.h>\n>  #include <libcamera/stream.h>\n>\n>  #include \"camera_controls.h\"\n> @@ -64,12 +65,12 @@ Request::Request(Camera *camera, uint64_t cookie)\n>  \t * creating a new instance for each request?\n>  \t */\n>  \tvalidator_ = new CameraControlValidator(camera);\n> -\tcontrols_ = new ControlList(validator_);\n> +\tcontrols_ = new ControlList(controls::controls, validator_);\n>\n>  \t/**\n>  \t * \\todo: Add a validator for metadata controls.\n>  \t */\n> -\tmetadata_ = new ControlList();\n> +\tmetadata_ = new ControlList(controls::controls);\n>  }\n>\n>  Request::~Request()\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index 1bcfecc467b5..5af53f64bb6c 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -42,7 +42,7 @@ protected:\n>  \tint run()\n>  \t{\n>  \t\tCameraControlValidator validator(camera_.get());\n> -\t\tControlList list(&validator);\n> +\t\tControlList list(controls::controls, &validator);\n>\n>  \t\t/* Test that the list is initially empty. */\n>  \t\tif (!list.empty()) {\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD88861562\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 13 Oct 2019 15:48:16 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 2EAFA40002;\n\tSun, 13 Oct 2019 13:48:16 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sun, 13 Oct 2019 15:50:04 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191013135004.lrzs5yxkpxp6gzha@uno.localdomain>","References":"<20191012184407.31684-1-laurent.pinchart@ideasonboard.com>\n\t<20191012184407.31684-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"xnl5ywbr644u3hta\"","Content-Disposition":"inline","In-Reply-To":"<20191012184407.31684-8-laurent.pinchart@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support\n\taccessing controls by numerical ID","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 13 Oct 2019 13:48:16 -0000"}},{"id":2869,"web_url":"https://patchwork.libcamera.org/comment/2869/","msgid":"<20191013140518.GB4886@pendragon.ideasonboard.com>","date":"2019-10-13T14:05:18","subject":"Re: [libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support\n\taccessing controls by numerical ID","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Oct 13, 2019 at 03:50:04PM +0200, Jacopo Mondi wrote:\n> On Sat, Oct 12, 2019 at 09:44:00PM +0300, Laurent Pinchart wrote:\n> > The ControlList class has template get() and set() methods to get and\n> > set control values. The methods require a reference to a Control\n> > instance, which is only available when calling them with a hardcoded\n> > control. In order to support usage of ControlList for V4L2 controls, as\n> > well as serialisation and deserialisation of ControlList, we need a way\n> > to get and set control values based on a control numerical ID. Add new\n> > contains(), get() and set() overload methods to do so.\n> >\n> > As this change prepares the ControlList to be used for other objects\n> > than camera, update its documentation accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  include/libcamera/controls.h   |  10 ++-\n> >  src/ipa/rkisp1/rkisp1.cpp      |   2 +-\n> >  src/libcamera/controls.cpp     | 143 ++++++++++++++++++++++++++-------\n> >  src/libcamera/request.cpp      |   5 +-\n> >  test/controls/control_list.cpp |   2 +-\n> >  5 files changed, 125 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index 999fcf7a3a62..5e6708fe570b 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -126,7 +126,7 @@ private:\n> >  \tusing ControlListMap = std::unordered_map<const ControlId *, ControlValue>;\n> >\n> >  public:\n> > -\tControlList(ControlValidator *validator = nullptr);\n> > +\tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n> >\n> >  \tusing iterator = ControlListMap::iterator;\n> >  \tusing const_iterator = ControlListMap::const_iterator;\n> > @@ -136,11 +136,13 @@ public:\n> >  \tconst_iterator begin() const { return controls_.begin(); }\n> >  \tconst_iterator end() const { return controls_.end(); }\n> >\n> > -\tbool contains(const ControlId &id) const;\n> >  \tbool empty() const { return controls_.empty(); }\n> >  \tstd::size_t size() const { return controls_.size(); }\n> >  \tvoid clear() { controls_.clear(); }\n> >\n> > +\tbool contains(const ControlId &id) const;\n> > +\tbool contains(unsigned int id) const;\n> > +\n> >  \ttemplate<typename T>\n> >  \tconst T &get(const Control<T> &ctrl) const\n> >  \t{\n> > @@ -163,11 +165,15 @@ public:\n> >  \t\tval->set<T>(value);\n> >  \t}\n> >\n> > +\tconst ControlValue &get(unsigned int id) const;\n> > +\tvoid set(unsigned int id, const ControlValue &value);\n> > +\n> >  private:\n> >  \tconst ControlValue *find(const ControlId &id) const;\n> >  \tControlValue *find(const ControlId &id);\n> >\n> >  \tControlValidator *validator_;\n> > +\tconst ControlIdMap *idmap_;\n> >  \tControlListMap controls_;\n> >  };\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 80138f196184..b0d23dd154be 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -220,7 +220,7 @@ void IPARkISP1::setControls(unsigned int frame)\n> >\n> >  void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> >  {\n> > -\tControlList ctrls;\n> > +\tControlList ctrls(controls::controls);\n> \n> Just to note down that providing a default argument to the ControList\n> contructor would shorten this. But I know you're in favour of having\n> controls:controls explicitly passed.\n\nYes, I think it's best to get the callers to think about it and pass the\nidmap explicitly. If applications were to create ControlList instances\nthemselves I could reconsider this (but possibly through a different\nsolution), but as that's not the case, I prefer keeping it explicit.\n\n> >\n> >  \tif (aeState)\n> >  \t\tctrls.set(controls::AeLocked, aeState == 2);\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 292e48cd6d25..ddd4e6680ce2 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -7,6 +7,7 @@\n> >\n> >  #include <libcamera/controls.h>\n> >\n> > +#include <iomanip>\n> >  #include <sstream>\n> >  #include <string>\n> >\n> > @@ -16,13 +17,13 @@\n> >\n> >  /**\n> >   * \\file controls.h\n> > - * \\brief Describes control framework and controls supported by a camera\n> > + * \\brief Framework to handle manage controls related to an object\n> \n> \"to handle -and- manage\"\n> Or just drop one of the two\n\nOops, fixed.\n\n> >   *\n> > - * A control is a mean to govern or influence the operation of a camera. Every\n> > - * control is defined by a unique numerical ID, a name string and the data type\n> > - * of the value it stores. The libcamera API defines a set of standard controls\n> > - * in the libcamera::controls namespace, as a set of instances of the Control\n> > - * class.\n> > + * A control is a mean to govern or influence the operation of an object, and in\n> > + * particular of a camera. Every control is defined by a unique numerical ID, a\n> > + * name string and the data type of the value it stores. The libcamera API\n> > + * defines a set of standard controls in the libcamera::controls namespace, as\n> > + * a set of instances of the Control class.\n> >   *\n> >   * The main way for applications to interact with controls is through the\n> >   * ControlList stored in the Request class:\n> > @@ -274,7 +275,7 @@ bool ControlValue::operator==(const ControlValue &other) const\n> >   * \\class Control\n> >   * \\brief Describe a control and its intrinsic properties\n> >   *\n> > - * The Control class models a control exposed by a camera. Its template type\n> > + * The Control class models a control exposed by an object. Its template type\n> >   * name T refers to the control data type, and allows methods that operate on\n> >   * control values to be defined as template methods using the same type T for\n> >   * the control value. See for instance how the ControlList::get() method\n> > @@ -293,8 +294,8 @@ bool ControlValue::operator==(const ControlValue &other) const\n> >   * long int).\n> >   *\n> >   * Controls IDs shall be unique. While nothing prevents multiple instances of\n> > - * the Control class to be created with the same ID, this may lead to undefined\n> > - * behaviour.\n> > + * the Control class to be created with the same ID for the same object, doing\n> > + * so may cause undefined behaviour.\n> >   */\n> >\n> >  /**\n> > @@ -398,18 +399,28 @@ std::string ControlRange::toString() const\n> >\n> >  /**\n> >   * \\class ControlList\n> > - * \\brief Associate a list of ControlId with their values for a camera\n> > + * \\brief Associate a list of ControlId with their values for an object\n> >   *\n> > - * A ControlList wraps a map of ControlId to ControlValue and optionally\n> > - * validates controls against a ControlValidator.\n> > + * The ControlList class stores values of controls exposed by an object. The\n> > + * lists returned by the Request::controls() and Request::metadata() methods\n> > + * refer to the camera that the request belongs to.\n> \n> Do we want this ? It leaves V4L2 controls a bit out of the picture\n\nI did it on purpose, as ControlList is exposed to applications, and V4L2\ncontrols are not. I didn't want to mention V4L2 anywhere in this file,\nwhile still keeping it generic. That's why I replaced instances of\n\"camera\" with \"object\", and added an explicit note that the two\nControlList instances visible to applications are related to a camera.\n\n> > + *\n> > + * Control lists are constructed with a map of all the controls supported by\n> > + * their object, and an optional ControlValidator to further validate the\n> \n> I would drop \"by their object\"\n\n\"with a map of all the controls supported\" sounds to me that it's\nmissing something. Supported by what ? :-)\n\n> > + * controls.\n> >   */\n> >\n> >  /**\n> >   * \\brief Construct a ControlList with an optional control validator\n> > + * \\param[in] idmap The ControlId map for the control list target object\n> >   * \\param[in] validator The validator (may be null)\n> > + *\n> > + * For ControlList containing libcamera controls, a global map of all libcamera\n> > + * controls is provided by controls::controls and can be used as the \\a idmap\n> > + * argument.\n> >   */\n> > -ControlList::ControlList(ControlValidator *validator)\n> > -\t: validator_(validator)\n> > +ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> > +\t: validator_(validator), idmap_(&idmap)\n> >  {\n> >  }\n> >\n> > @@ -449,20 +460,6 @@ ControlList::ControlList(ControlValidator *validator)\n> >   * list\n> >   */\n> >\n> > -/**\n> > - * \\brief Check if the list contains a control with the specified \\a id\n> > - * \\param[in] id The control ID\n> > - *\n> > - * The behaviour is undefined if the control \\a id is not supported by the\n> > - * camera that the ControlList refers to.\n> > - *\n> > - * \\return True if the list contains a matching control, false otherwise\n> > - */\n> > -bool ControlList::contains(const ControlId &id) const\n> > -{\n> > -\treturn controls_.find(&id) != controls_.end();\n> > -}\n> > -\n> >  /**\n> >   * \\fn ControlList::empty()\n> >   * \\brief Identify if the list is empty\n> > @@ -481,7 +478,33 @@ bool ControlList::contains(const ControlId &id) const\n> >   */\n> >\n> >  /**\n> > - * \\fn template<typename T> const T &ControlList::get() const\n> > + * \\brief Check if the list contains a control with the specified \\a id\n> > + * \\param[in] id The control ID\n> > + *\n> > + * \\return True if the list contains a matching control, false otherwise\n> > + */\n> > +bool ControlList::contains(const ControlId &id) const\n> > +{\n> > +\treturn controls_.find(&id) != controls_.end();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Check if the list contains a control with the specified \\a id\n> \n> The documentation of these two methods is the same. I assume it's ok,\n> but I would mentioned \"numeric id\" in the second version.\n\nIsn't it already specified in the param ?\n\n> > + * \\param[in] id The control numerical ID\n> > + *\n> > + * \\return True if the list contains a matching control, false otherwise\n> > + */\n> > +bool ControlList::contains(unsigned int id) const\n> > +{\n> > +\tconst auto iter = idmap_->find(id);\n> > +\tif (iter == idmap_->end())\n> > +\t\treturn false;\n> > +\n> > +\treturn contains(*iter->second);\n> > +}\n> > +\n> > +/**\n> > + * \\fn template<typename T> const T &ControlList::get(const Control<T> &ctrl) const\n> >   * \\brief Get the value of a control\n> >   * \\param[in] ctrl The control\n> >   *\n> > @@ -496,7 +519,7 @@ bool ControlList::contains(const ControlId &id) const\n> >   */\n> >\n> >  /**\n> > - * \\fn template<typename T> void ControlList::set()\n> > + * \\fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value)\n> >   * \\brief Set the control value to \\a value\n> >   * \\param[in] ctrl The control\n> >   * \\param[in] value The control value\n> > @@ -506,9 +529,67 @@ bool ControlList::contains(const ControlId &id) const\n> >   * to the list.\n> >   *\n> >   * The behaviour is undefined if the control \\a ctrl is not supported by the\n> > - * camera that the list refers to.\n> > + * object that the list refers to.\n> >   */\n> >\n> > +/**\n> > + * \\brief Get the value of control \\a id\n> > + * \\param[in] id The control numerical ID\n> > + *\n> > + * The behaviour is undefined if the control \\a id is not present in the list.\n> > + * Use ControlList::contains() to test for the presence of a control in the\n> > + * list before retrieving its value.\n> > + *\n> > + * \\return The control value\n> > + */\n> > +const ControlValue &ControlList::get(unsigned int id) const\n> \n> This, as the ControlId * oriented version, is very easy to miss if the\n> control is not present. Not an issue with this patch, anyway.\n> \n> The pattern would be like\n> \n>         const ControlValue &ctrl = list.get(ID);\n>         if (ctrl.isNone()) {\n>                 ...\n>         }\n> \n> Should we document it?\n\nNo, we shouldn't, as that's incorrect code. The documentation of the\nmethod clearly specifies that the behaiour is unspecified if the control\nis not present in the list. We don't want to document a way to rely on\nunspecified behaviour.\n\n> > +{\n> > +\tstatic ControlValue zero;\n> > +\n> > +\tconst auto ctrl = idmap_->find(id);\n> > +\tif (ctrl == idmap_->end()) {\n> > +\t\tLOG(Controls, Error)\n> > +\t\t\t<< std::hex << std::setfill('0')\n> > +\t\t\t<< \"Control 0x\" << std::setw(8) << id << \" is not valid\";\n> > +\t\treturn zero;\n> > +\t}\n> > +\n> > +\tconst ControlValue *val = find(*ctrl->second);\n> > +\tif (!val)\n> > +\t\treturn zero;\n> > +\n> > +\treturn *val;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set the value of control \\a id to \\a value\n> > + * \\param[in] id The control ID\n> > + * \\param[in] value The control value\n> > + *\n> > + * This method sets the value of a control in the control list. If the control\n> > + * is already present in the list, its value is updated, otherwise it is added\n> > + * to the list.\n> > + *\n> > + * The behaviour is undefined if the control \\a id is not supported by the\n> > + * object that the list refers to.\n> > + */\n> > +void ControlList::set(unsigned int id, const ControlValue &value)\n> > +{\n> > +\tconst auto ctrl = idmap_->find(id);\n> > +\tif (ctrl == idmap_->end()) {\n> > +\t\tLOG(Controls, Error)\n> > +\t\t\t<< std::hex << std::setfill('0')\n> > +\t\t\t<< \"Control 0x\" << std::setw(8) << id << \" is not valid\";\n> \n> Here and in the get operation \"is not valid\" or \"is not supported\" ?\n\n\"is not supported\" seems better, I'll switch to it.\n\n> Anyway, all minor issues, so please add\n> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>\n\nThank you.\n\n> > +\t\treturn;\n> > +\t}\n> > +\n> > +\tControlValue *val = find(*ctrl->second);\n> > +\tif (!val)\n> > +\t\treturn;\n> > +\n> > +\t*val = value;\n> > +}\n> > +\n> >  const ControlValue *ControlList::find(const ControlId &id) const\n> >  {\n> >  \tconst auto iter = controls_.find(&id);\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index e800f1449888..c14ed1a4d3ce 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -11,6 +11,7 @@\n> >\n> >  #include <libcamera/buffer.h>\n> >  #include <libcamera/camera.h>\n> > +#include <libcamera/control_ids.h>\n> >  #include <libcamera/stream.h>\n> >\n> >  #include \"camera_controls.h\"\n> > @@ -64,12 +65,12 @@ Request::Request(Camera *camera, uint64_t cookie)\n> >  \t * creating a new instance for each request?\n> >  \t */\n> >  \tvalidator_ = new CameraControlValidator(camera);\n> > -\tcontrols_ = new ControlList(validator_);\n> > +\tcontrols_ = new ControlList(controls::controls, validator_);\n> >\n> >  \t/**\n> >  \t * \\todo: Add a validator for metadata controls.\n> >  \t */\n> > -\tmetadata_ = new ControlList();\n> > +\tmetadata_ = new ControlList(controls::controls);\n> >  }\n> >\n> >  Request::~Request()\n> > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> > index 1bcfecc467b5..5af53f64bb6c 100644\n> > --- a/test/controls/control_list.cpp\n> > +++ b/test/controls/control_list.cpp\n> > @@ -42,7 +42,7 @@ protected:\n> >  \tint run()\n> >  \t{\n> >  \t\tCameraControlValidator validator(camera_.get());\n> > -\t\tControlList list(&validator);\n> > +\t\tControlList list(controls::controls, &validator);\n> >\n> >  \t\t/* Test that the list is initially empty. */\n> >  \t\tif (!list.empty()) {","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5615C61562\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 13 Oct 2019 16:05:21 +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 D4DC6A46;\n\tSun, 13 Oct 2019 16:05:20 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1570975521;\n\tbh=0kkt2uv2fu14zX9wnlWteeJc++rBjW7fOSjZpFapbdk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hjeUk7dLSrEUJAjZlwfZUC77K1e4KIb+WmqITADapIUQNERH/ZSsfr/oN+6CkO1RN\n\t4WZLs8MlpnQQGcXpC1JFO3I9Ke3MV9s40V2ZFcNabQ4aRBh8i8Zl46e6yhWlrv93F8\n\tq1Kd2zdhhXlRV1bOiGzgp3bk9sITZsWAC4JSIg90=","Date":"Sun, 13 Oct 2019 17:05:18 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191013140518.GB4886@pendragon.ideasonboard.com>","References":"<20191012184407.31684-1-laurent.pinchart@ideasonboard.com>\n\t<20191012184407.31684-8-laurent.pinchart@ideasonboard.com>\n\t<20191013135004.lrzs5yxkpxp6gzha@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191013135004.lrzs5yxkpxp6gzha@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support\n\taccessing controls by numerical ID","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 13 Oct 2019 14:05:21 -0000"}},{"id":2884,"web_url":"https://patchwork.libcamera.org/comment/2884/","msgid":"<20191013155053.GP23166@bigcity.dyn.berto.se>","date":"2019-10-13T15:50:53","subject":"Re: [libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support\n\taccessing controls by numerical ID","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2019-10-12 21:44:00 +0300, Laurent Pinchart wrote:\n> The ControlList class has template get() and set() methods to get and\n> set control values. The methods require a reference to a Control\n> instance, which is only available when calling them with a hardcoded\n> control. In order to support usage of ControlList for V4L2 controls, as\n> well as serialisation and deserialisation of ControlList, we need a way\n> to get and set control values based on a control numerical ID. Add new\n> contains(), get() and set() overload methods to do so.\n> \n> As this change prepares the ControlList to be used for other objects\n> than camera, update its documentation accordingly.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\n> ---\n>  include/libcamera/controls.h   |  10 ++-\n>  src/ipa/rkisp1/rkisp1.cpp      |   2 +-\n>  src/libcamera/controls.cpp     | 143 ++++++++++++++++++++++++++-------\n>  src/libcamera/request.cpp      |   5 +-\n>  test/controls/control_list.cpp |   2 +-\n>  5 files changed, 125 insertions(+), 37 deletions(-)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index 999fcf7a3a62..5e6708fe570b 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -126,7 +126,7 @@ private:\n>  \tusing ControlListMap = std::unordered_map<const ControlId *, ControlValue>;\n>  \n>  public:\n> -\tControlList(ControlValidator *validator = nullptr);\n> +\tControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);\n>  \n>  \tusing iterator = ControlListMap::iterator;\n>  \tusing const_iterator = ControlListMap::const_iterator;\n> @@ -136,11 +136,13 @@ public:\n>  \tconst_iterator begin() const { return controls_.begin(); }\n>  \tconst_iterator end() const { return controls_.end(); }\n>  \n> -\tbool contains(const ControlId &id) const;\n>  \tbool empty() const { return controls_.empty(); }\n>  \tstd::size_t size() const { return controls_.size(); }\n>  \tvoid clear() { controls_.clear(); }\n>  \n> +\tbool contains(const ControlId &id) const;\n> +\tbool contains(unsigned int id) const;\n> +\n>  \ttemplate<typename T>\n>  \tconst T &get(const Control<T> &ctrl) const\n>  \t{\n> @@ -163,11 +165,15 @@ public:\n>  \t\tval->set<T>(value);\n>  \t}\n>  \n> +\tconst ControlValue &get(unsigned int id) const;\n> +\tvoid set(unsigned int id, const ControlValue &value);\n> +\n>  private:\n>  \tconst ControlValue *find(const ControlId &id) const;\n>  \tControlValue *find(const ControlId &id);\n>  \n>  \tControlValidator *validator_;\n> +\tconst ControlIdMap *idmap_;\n>  \tControlListMap controls_;\n>  };\n>  \n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 80138f196184..b0d23dd154be 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -220,7 +220,7 @@ void IPARkISP1::setControls(unsigned int frame)\n>  \n>  void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n>  {\n> -\tControlList ctrls;\n> +\tControlList ctrls(controls::controls);\n>  \n>  \tif (aeState)\n>  \t\tctrls.set(controls::AeLocked, aeState == 2);\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 292e48cd6d25..ddd4e6680ce2 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -7,6 +7,7 @@\n>  \n>  #include <libcamera/controls.h>\n>  \n> +#include <iomanip>\n>  #include <sstream>\n>  #include <string>\n>  \n> @@ -16,13 +17,13 @@\n>  \n>  /**\n>   * \\file controls.h\n> - * \\brief Describes control framework and controls supported by a camera\n> + * \\brief Framework to handle manage controls related to an object\n>   *\n> - * A control is a mean to govern or influence the operation of a camera. Every\n> - * control is defined by a unique numerical ID, a name string and the data type\n> - * of the value it stores. The libcamera API defines a set of standard controls\n> - * in the libcamera::controls namespace, as a set of instances of the Control\n> - * class.\n> + * A control is a mean to govern or influence the operation of an object, and in\n> + * particular of a camera. Every control is defined by a unique numerical ID, a\n> + * name string and the data type of the value it stores. The libcamera API\n> + * defines a set of standard controls in the libcamera::controls namespace, as\n> + * a set of instances of the Control class.\n>   *\n>   * The main way for applications to interact with controls is through the\n>   * ControlList stored in the Request class:\n> @@ -274,7 +275,7 @@ bool ControlValue::operator==(const ControlValue &other) const\n>   * \\class Control\n>   * \\brief Describe a control and its intrinsic properties\n>   *\n> - * The Control class models a control exposed by a camera. Its template type\n> + * The Control class models a control exposed by an object. Its template type\n>   * name T refers to the control data type, and allows methods that operate on\n>   * control values to be defined as template methods using the same type T for\n>   * the control value. See for instance how the ControlList::get() method\n> @@ -293,8 +294,8 @@ bool ControlValue::operator==(const ControlValue &other) const\n>   * long int).\n>   *\n>   * Controls IDs shall be unique. While nothing prevents multiple instances of\n> - * the Control class to be created with the same ID, this may lead to undefined\n> - * behaviour.\n> + * the Control class to be created with the same ID for the same object, doing\n> + * so may cause undefined behaviour.\n>   */\n>  \n>  /**\n> @@ -398,18 +399,28 @@ std::string ControlRange::toString() const\n>  \n>  /**\n>   * \\class ControlList\n> - * \\brief Associate a list of ControlId with their values for a camera\n> + * \\brief Associate a list of ControlId with their values for an object\n>   *\n> - * A ControlList wraps a map of ControlId to ControlValue and optionally\n> - * validates controls against a ControlValidator.\n> + * The ControlList class stores values of controls exposed by an object. The\n> + * lists returned by the Request::controls() and Request::metadata() methods\n> + * refer to the camera that the request belongs to.\n> + *\n> + * Control lists are constructed with a map of all the controls supported by\n> + * their object, and an optional ControlValidator to further validate the\n> + * controls.\n>   */\n>  \n>  /**\n>   * \\brief Construct a ControlList with an optional control validator\n> + * \\param[in] idmap The ControlId map for the control list target object\n>   * \\param[in] validator The validator (may be null)\n> + *\n> + * For ControlList containing libcamera controls, a global map of all libcamera\n> + * controls is provided by controls::controls and can be used as the \\a idmap\n> + * argument.\n>   */\n> -ControlList::ControlList(ControlValidator *validator)\n> -\t: validator_(validator)\n> +ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)\n> +\t: validator_(validator), idmap_(&idmap)\n>  {\n>  }\n>  \n> @@ -449,20 +460,6 @@ ControlList::ControlList(ControlValidator *validator)\n>   * list\n>   */\n>  \n> -/**\n> - * \\brief Check if the list contains a control with the specified \\a id\n> - * \\param[in] id The control ID\n> - *\n> - * The behaviour is undefined if the control \\a id is not supported by the\n> - * camera that the ControlList refers to.\n> - *\n> - * \\return True if the list contains a matching control, false otherwise\n> - */\n> -bool ControlList::contains(const ControlId &id) const\n> -{\n> -\treturn controls_.find(&id) != controls_.end();\n> -}\n> -\n>  /**\n>   * \\fn ControlList::empty()\n>   * \\brief Identify if the list is empty\n> @@ -481,7 +478,33 @@ bool ControlList::contains(const ControlId &id) const\n>   */\n>  \n>  /**\n> - * \\fn template<typename T> const T &ControlList::get() const\n> + * \\brief Check if the list contains a control with the specified \\a id\n> + * \\param[in] id The control ID\n> + *\n> + * \\return True if the list contains a matching control, false otherwise\n> + */\n> +bool ControlList::contains(const ControlId &id) const\n> +{\n> +\treturn controls_.find(&id) != controls_.end();\n> +}\n> +\n> +/**\n> + * \\brief Check if the list contains a control with the specified \\a id\n> + * \\param[in] id The control numerical ID\n> + *\n> + * \\return True if the list contains a matching control, false otherwise\n> + */\n> +bool ControlList::contains(unsigned int id) const\n> +{\n> +\tconst auto iter = idmap_->find(id);\n> +\tif (iter == idmap_->end())\n> +\t\treturn false;\n> +\n> +\treturn contains(*iter->second);\n> +}\n> +\n> +/**\n> + * \\fn template<typename T> const T &ControlList::get(const Control<T> &ctrl) const\n>   * \\brief Get the value of a control\n>   * \\param[in] ctrl The control\n>   *\n> @@ -496,7 +519,7 @@ bool ControlList::contains(const ControlId &id) const\n>   */\n>  \n>  /**\n> - * \\fn template<typename T> void ControlList::set()\n> + * \\fn template<typename T> void ControlList::set(const Control<T> &ctrl, const T &value)\n>   * \\brief Set the control value to \\a value\n>   * \\param[in] ctrl The control\n>   * \\param[in] value The control value\n> @@ -506,9 +529,67 @@ bool ControlList::contains(const ControlId &id) const\n>   * to the list.\n>   *\n>   * The behaviour is undefined if the control \\a ctrl is not supported by the\n> - * camera that the list refers to.\n> + * object that the list refers to.\n>   */\n>  \n> +/**\n> + * \\brief Get the value of control \\a id\n> + * \\param[in] id The control numerical ID\n> + *\n> + * The behaviour is undefined if the control \\a id is not present in the list.\n> + * Use ControlList::contains() to test for the presence of a control in the\n> + * list before retrieving its value.\n> + *\n> + * \\return The control value\n> + */\n> +const ControlValue &ControlList::get(unsigned int id) const\n> +{\n> +\tstatic ControlValue zero;\n> +\n> +\tconst auto ctrl = idmap_->find(id);\n> +\tif (ctrl == idmap_->end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< std::hex << std::setfill('0')\n> +\t\t\t<< \"Control 0x\" << std::setw(8) << id << \" is not valid\";\n> +\t\treturn zero;\n> +\t}\n> +\n> +\tconst ControlValue *val = find(*ctrl->second);\n> +\tif (!val)\n> +\t\treturn zero;\n> +\n> +\treturn *val;\n> +}\n> +\n> +/**\n> + * \\brief Set the value of control \\a id to \\a value\n> + * \\param[in] id The control ID\n> + * \\param[in] value The control value\n> + *\n> + * This method sets the value of a control in the control list. If the control\n> + * is already present in the list, its value is updated, otherwise it is added\n> + * to the list.\n> + *\n> + * The behaviour is undefined if the control \\a id is not supported by the\n> + * object that the list refers to.\n> + */\n> +void ControlList::set(unsigned int id, const ControlValue &value)\n> +{\n> +\tconst auto ctrl = idmap_->find(id);\n> +\tif (ctrl == idmap_->end()) {\n> +\t\tLOG(Controls, Error)\n> +\t\t\t<< std::hex << std::setfill('0')\n> +\t\t\t<< \"Control 0x\" << std::setw(8) << id << \" is not valid\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tControlValue *val = find(*ctrl->second);\n> +\tif (!val)\n> +\t\treturn;\n> +\n> +\t*val = value;\n> +}\n> +\n>  const ControlValue *ControlList::find(const ControlId &id) const\n>  {\n>  \tconst auto iter = controls_.find(&id);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index e800f1449888..c14ed1a4d3ce 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -11,6 +11,7 @@\n>  \n>  #include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n> +#include <libcamera/control_ids.h>\n>  #include <libcamera/stream.h>\n>  \n>  #include \"camera_controls.h\"\n> @@ -64,12 +65,12 @@ Request::Request(Camera *camera, uint64_t cookie)\n>  \t * creating a new instance for each request?\n>  \t */\n>  \tvalidator_ = new CameraControlValidator(camera);\n> -\tcontrols_ = new ControlList(validator_);\n> +\tcontrols_ = new ControlList(controls::controls, validator_);\n>  \n>  \t/**\n>  \t * \\todo: Add a validator for metadata controls.\n>  \t */\n> -\tmetadata_ = new ControlList();\n> +\tmetadata_ = new ControlList(controls::controls);\n>  }\n>  \n>  Request::~Request()\n> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp\n> index 1bcfecc467b5..5af53f64bb6c 100644\n> --- a/test/controls/control_list.cpp\n> +++ b/test/controls/control_list.cpp\n> @@ -42,7 +42,7 @@ protected:\n>  \tint run()\n>  \t{\n>  \t\tCameraControlValidator validator(camera_.get());\n> -\t\tControlList list(&validator);\n> +\t\tControlList list(controls::controls, &validator);\n>  \n>  \t\t/* Test that the list is initially empty. */\n>  \t\tif (!list.empty()) {\n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x22a.google.com (mail-lj1-x22a.google.com\n\t[IPv6:2a00:1450:4864:20::22a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 515E261562\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 13 Oct 2019 17:50:55 +0200 (CEST)","by mail-lj1-x22a.google.com with SMTP id m7so14227550lji.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 13 Oct 2019 08:50:55 -0700 (PDT)","from localhost (h-93-159.A463.priv.bahnhof.se. [46.59.93.159])\n\tby smtp.gmail.com with ESMTPSA id\n\tn3sm3494554lfl.62.2019.10.13.08.50.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 13 Oct 2019 08:50:53 -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=/BpR03w6PNK8r4hnii8TmCeAm27ScZ6+C7XckwtekcM=;\n\tb=EvYD1UuzSkwpunhsuwRw7Rra+/JNPoD0W3OVK1WLbvJJOE4I6gwrpu0PrViiNltcMV\n\t+DJZ46klTrrHKaNDhFULi6QbBuNrCZxINi0NjviKfJVCu51u1ass1m94AhtOvE/ymCVJ\n\tKLE0PmR9OgosIhKgEN1PdSb2FrmNSuBZb2+9q7f1PBTxgyHPttK1DBJ0lViVls+4VgSW\n\tBEUTgvhw3C2jGScVBcWsN7My2XwbUkqNfYr5kftG38GlZXyK4TfFhyyg9C0rfT7ZgbPj\n\t8D276M5PU95XKw2dP4JocWNNSBvkXvxLRnCPZKo6eLhv+lpy7XJJQuH396h2FKVk+9Hh\n\tJimg==","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=/BpR03w6PNK8r4hnii8TmCeAm27ScZ6+C7XckwtekcM=;\n\tb=TEruiEWZuX2pe2iwyVFe091JhI5Kf46zyOIyQFPlWvcv7j2m0iPKmz1hPKBsOVh9EM\n\tvjn6ZnMolFR73gZ2QXTtoo6k0n5nbWXBsdhArglHa2To7KXBWnSj4nGFSDMPLkyKXxP8\n\tSJj4Qc7RtF9cgGq9XJfoSWW32/mQaIumJTkvW7Di+BkE7TeU3uSCP7rGsW/B0prB5qAg\n\tTkzJQ2yyDOpcbNx0CEd6jvu7A0cFRaWmEi4g1V5LGDrvvI48sIY8+ZrdP/o/i81B5sj1\n\t/wllnSxIdbYF9f4blQeJ5VD4l6e3P6Zwl/OTlfazLsXuxM1YtLeP4rQXSrua7X0iSvb5\n\tNAZw==","X-Gm-Message-State":"APjAAAWGyFosTt0L7h33iVRQXaQOgIeXGakMSOj472g7xKMbuOJ+GDIi\n\tsa07Rhb6tuycx8qcIz5HQFPZhSBxYKY=","X-Google-Smtp-Source":"APXvYqyWnQsgVQdEeCvNxwGr1Gfw2yqF813Xug8tIahxMlQEsnNVl5BoRMPH/Wng+nUKHjNndwiDBQ==","X-Received":"by 2002:a2e:9b8a:: with SMTP id\n\tz10mr16025861lji.80.1570981854571; \n\tSun, 13 Oct 2019 08:50:54 -0700 (PDT)","Date":"Sun, 13 Oct 2019 17:50:53 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191013155053.GP23166@bigcity.dyn.berto.se>","References":"<20191012184407.31684-1-laurent.pinchart@ideasonboard.com>\n\t<20191012184407.31684-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191012184407.31684-8-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 07/14] libcamera: controls: Support\n\taccessing controls by numerical ID","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Sun, 13 Oct 2019 15:50:55 -0000"}}]