[{"id":2513,"web_url":"https://patchwork.libcamera.org/comment/2513/","msgid":"<20190828111840.GO28351@bigcity.dyn.berto.se>","date":"2019-08-28T11:18:40","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:\n> Add a constructor for the V4L2ControlList class that accepts a list of\n> V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> instance to be used for reading controls only.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_controls.h | 3 +++\n> src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> 2 files changed, 20 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index 10b726504951..86c84e06d741 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -65,6 +65,9 @@ public:\n> \tusing iterator = std::vector::iterator;\n> \tusing const_iterator = std::vector::const_iterator;\n> \n> +\tV4L2ControlList() {}\n> +\tV4L2ControlList(std::vector ids);\n> +\n> \titerator begin() { return controls_.begin(); }\n> \tconst_iterator begin() const { return controls_.begin(); }\n> \titerator end() { return controls_.end(); }\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index 84258d9954d0..eeb325cbfff9 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> * and prepare to be re-used for a new control write/read sequence.\n> */\n> \n> +/**\n> + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> + *\n> + * Construct a V4L2ControlList from a list of control identifiers without any\n> + * value associated. This constructor is particularly useful to create a\n> + * V4L2ControlList that is used to read the values of all controls in the\n> + * \\a ids list. The created V4L2ControlList should not be used to write control\n> + * values unless it is cleared first and then controls with an associated value\n> + * are manually added to it.\n> + */\n\nDo you think it would be worth it to add some code to enforce that no \ncontrols can be written if it's initialized with this constructor?\n\n> +V4L2ControlList::V4L2ControlList(std::vector ids)\n> +{\n> +\tfor (auto id : ids)\n> +\t\tadd(id);\n> +}\n> +\n> /**\n> * \\typedef V4L2ControlList::iterator\n> * \\brief Iterator on the V4L2 controls contained in the instance\n> -- \n> 2.23.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF3E060BF6\n\tfor ;\n\tWed, 28 Aug 2019 13:18:41 +0200 (CEST)","by mail-lf1-x142.google.com with SMTP id h27so1833039lfp.2\n\tfor ;\n\tWed, 28 Aug 2019 04:18:41 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\ts8sm584798ljd.94.2019.08.28.04.18.40\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 28 Aug 2019 04:18:40 -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=Sd7pSl8SJFN0lhyI10iuzrLXtDPdXCC533yzETXsYfc=;\n\tb=DCRUa8nx6jn/lEoVP/CsDPcNzHwDcTTZAvAZabOfJxzRn2lpdzyKKL5JowXENZyRoX\n\tpC8UOXR90mr0NrwOORYgZI1xpoOf1EQf+ZVlw8hVvJ0G/WIkvUV7o0AO11ac0b6dl7JJ\n\tMLaNaJQ7NLPhbUkrCfbdihsLvkQfo4Yv8Wdf+dw4ktx6r1f5ML0BQAnYSnlPvMklta+O\n\tESPf0CuKlxtJJqBnl/zYNxrazt9vYAC3KovoHBUlwDtx48qIx1zkP9sbCUYsizUsn0U1\n\teXBj3MrJvq4hL2GB/e+387pxVuJs9SsdXA4pJQxMF2ykRWZoIthMVwkyv09QNNw5BuaL\n\tC9rg==","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=Sd7pSl8SJFN0lhyI10iuzrLXtDPdXCC533yzETXsYfc=;\n\tb=SKR0/KDVf1MZ39Cjx6/kBRruBlNNQc5Bb/eo+HLyKfJzUsiNRdK5zQlQ15gq5zcgbg\n\tVEf/UJKUwxTxIw4tZnWSlX96lMkNlgbLPbls8/+iJVeT6h8i0NE1xR6QBoWohj+BCZGL\n\t5KTOC4dLb7eRcG3LgOBiQNXVhI15bczSSvFzLPfibMx4Hvia2dME7Vv/TF5LolGDhDFv\n\tlBrVYSa5wmDnDnCpTNpjXA6gs1CFjIjJXWC06NCQdYvm7Qo9ztxxtWCt9L9p2f+pj3le\n\t1w26/0DPhqVQKw8h5S2e1z9z29ttutwMW6qRarulNJEtU9wCdKGridNqU/NSrhDld45A\n\tJfdA==","X-Gm-Message-State":"APjAAAX6noAVPdpCS34E0RIn1WtJEvs7ccrGPUp9TPiT+3U283SIvYxH\n\txmaRc1pfTjssF3089xja7VqcFzH9aJo=","X-Google-Smtp-Source":"APXvYqwNyexoJaVopu74Ly9bJvrzf89ytSUTROi/iTGq9k7lytOEOIL9gfpLC04tq1biPCNM1nhBqQ==","X-Received":"by 2002:ac2:488e:: with SMTP id x14mr2232700lfc.11.1566991121189;\n\tWed, 28 Aug 2019 04:18:41 -0700 (PDT)","Date":"Wed, 28 Aug 2019 13:18:40 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828111840.GO28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190827095008.11405-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 11:18:41 -0000"}},{"id":2525,"web_url":"https://patchwork.libcamera.org/comment/2525/","msgid":"<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>","date":"2019-08-28T16:28:43","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:\n> > Add a constructor for the V4L2ControlList class that accepts a list of\n> > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> > instance to be used for reading controls only.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/v4l2_controls.h | 3 +++\n> > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> > 2 files changed, 20 insertions(+)\n> >\n> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > index 10b726504951..86c84e06d741 100644\n> > --- a/src/libcamera/include/v4l2_controls.h\n> > +++ b/src/libcamera/include/v4l2_controls.h\n> > @@ -65,6 +65,9 @@ public:\n> > \tusing iterator = std::vector::iterator;\n> > \tusing const_iterator = std::vector::const_iterator;\n> >\n> > +\tV4L2ControlList() {}\n> > +\tV4L2ControlList(std::vector ids);\n> > +\n> > \titerator begin() { return controls_.begin(); }\n> > \tconst_iterator begin() const { return controls_.begin(); }\n> > \titerator end() { return controls_.end(); }\n> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > index 84258d9954d0..eeb325cbfff9 100644\n> > --- a/src/libcamera/v4l2_controls.cpp\n> > +++ b/src/libcamera/v4l2_controls.cpp\n> > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > * and prepare to be re-used for a new control write/read sequence.\n> > */\n> >\n> > +/**\n> > + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> > + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> > + *\n> > + * Construct a V4L2ControlList from a list of control identifiers without any\n> > + * value associated. This constructor is particularly useful to create a\n> > + * V4L2ControlList that is used to read the values of all controls in the\n> > + * \\a ids list. The created V4L2ControlList should not be used to write control\n> > + * values unless it is cleared first and then controls with an associated value\n> > + * are manually added to it.\n> > + */\n>\n> Do you think it would be worth it to add some code to enforce that no\n> controls can be written if it's initialized with this constructor?\n>\n\nHow would you do so? The most naive implementation that comes to mind\nit's to add a 'writable' flag to initialize to true only if the\ncontrol has a value assigned and check for the flag\nV4L2Device::setControls()\n\n> > +V4L2ControlList::V4L2ControlList(std::vector ids)\n> > +{\n> > +\tfor (auto id : ids)\n> > +\t\tadd(id);\n> > +}\n> > +\n> > /**\n> > * \\typedef V4L2ControlList::iterator\n> > * \\brief Iterator on the V4L2 controls contained in the instance\n> > --\n> > 2.23.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B3B1460BF6\n\tfor ;\n\tWed, 28 Aug 2019 18:27:11 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 35C541BF203;\n\tWed, 28 Aug 2019 16:27:10 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 28 Aug 2019 18:28:43 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>\n\t<20190828111840.GO28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"c6ybuwtgv5yfso7t\"","Content-Disposition":"inline","In-Reply-To":"<20190828111840.GO28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 28 Aug 2019 16:27:11 -0000"}},{"id":2530,"web_url":"https://patchwork.libcamera.org/comment/2530/","msgid":"<20190829081010.GS28351@bigcity.dyn.berto.se>","date":"2019-08-29T08:10:10","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:\n> > > Add a constructor for the V4L2ControlList class that accepts a list of\n> > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> > > instance to be used for reading controls only.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/include/v4l2_controls.h | 3 +++\n> > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> > > 2 files changed, 20 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > > index 10b726504951..86c84e06d741 100644\n> > > --- a/src/libcamera/include/v4l2_controls.h\n> > > +++ b/src/libcamera/include/v4l2_controls.h\n> > > @@ -65,6 +65,9 @@ public:\n> > > \tusing iterator = std::vector::iterator;\n> > > \tusing const_iterator = std::vector::const_iterator;\n> > >\n> > > +\tV4L2ControlList() {}\n> > > +\tV4L2ControlList(std::vector ids);\n> > > +\n> > > \titerator begin() { return controls_.begin(); }\n> > > \tconst_iterator begin() const { return controls_.begin(); }\n> > > \titerator end() { return controls_.end(); }\n> > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > > index 84258d9954d0..eeb325cbfff9 100644\n> > > --- a/src/libcamera/v4l2_controls.cpp\n> > > +++ b/src/libcamera/v4l2_controls.cpp\n> > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > > * and prepare to be re-used for a new control write/read sequence.\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> > > + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> > > + *\n> > > + * Construct a V4L2ControlList from a list of control identifiers without any\n> > > + * value associated. This constructor is particularly useful to create a\n> > > + * V4L2ControlList that is used to read the values of all controls in the\n> > > + * \\a ids list. The created V4L2ControlList should not be used to write control\n> > > + * values unless it is cleared first and then controls with an associated value\n> > > + * are manually added to it.\n> > > + */\n> >\n> > Do you think it would be worth it to add some code to enforce that no\n> > controls can be written if it's initialized with this constructor?\n> >\n> \n> How would you do so? The most naive implementation that comes to mind\n> it's to add a 'writable' flag to initialize to true only if the\n> control has a value assigned and check for the flag\n> V4L2Device::setControls()\n\nI'm not sure how to best implement it, but the constraint is so specific \nit looked like something that could be described in code. Now that I dig \nthru the code with fresh eyes I can't find the rational for the \nV4L2ControlList needs to be cleared before it's attempted to be used for \nwriting.\n\nThe control values are initialized to 0 using this new constructor and \nif used for writing V4L2Device::setControls() will validate that the \ncontrol exists on the device and bail before writing anything if one the \ncontrols do not exist.\n\nWhat reason do you think there is for the need to manually add controls \nwith a specific value before writing? This is just a specialisation \nwhere all values are initialized to 0, at least in my mind I might be \nmissing something ;-)\n\n> \n> > > +V4L2ControlList::V4L2ControlList(std::vector ids)\n> > > +{\n> > > +\tfor (auto id : ids)\n> > > +\t\tadd(id);\n> > > +}\n> > > +\n> > > /**\n> > > * \\typedef V4L2ControlList::iterator\n> > > * \\brief Iterator on the V4L2 controls contained in the instance\n> > > --\n> > > 2.23.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A88F960BCF\n\tfor ;\n\tThu, 29 Aug 2019 10:10:13 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id t14so2131746lji.4\n\tfor ;\n\tThu, 29 Aug 2019 01:10:13 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\to15sm269205lff.22.2019.08.29.01.10.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 01:10:11 -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=aKZb0Zkw0iwPZrXameYuVXdnbV5IgWfhXSNDWz4U9tY=;\n\tb=aNElcA2Wk3cdBB5HQGa1yB3hqwb8+9RMQyT7K1iY3qvxCnGl5eIy21/r+Puf8PsQ8p\n\tOiMBfpwPmu/UzpZ0rft614Kz52fDp2kKpsdH0+l/yygUoEpwaLgEixW152vKBjIXr4Jz\n\tf6ZRSdxU84i3pugNZ8JvzSEaUveYNaENyFCS5/Fh9jsrEXrsu0W0vlkYxoAsgkOBNPwy\n\tskKIMo7m86oAxVU+peePlgFeUBLSLqI/MEexN+cWVMurUP6JPJr+3B22pVx9rLZEX65w\n\tNQyg3ld7YooiebI3PBDbz52kS2nI64Pke0lP9n8525DVGv0uYGNoFrVyatfPWoo6Uo8F\n\tMgug==","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=aKZb0Zkw0iwPZrXameYuVXdnbV5IgWfhXSNDWz4U9tY=;\n\tb=q8ukMtPOYLuelp/0g4b+Gpg29Yc4KjpzXmYWhMB+m3sVD4pQOBZygxZtOJTLQNVj2O\n\t/GPPrkHhZgxbcj9Ws5Ffd0QeQug4bi3FoJlvyVya2LB+aPOY6et2cqzol7vaJnp86K1j\n\tb+bw5EHw3n79G5jKZ1m5jPUFqnDZipJoI1zcqvYid9dATr3w9E48HwBwoVfOHG1vm5NY\n\t5LMiGVaaU7X44tdNeX6dJksCNTgEWQ0HLApVFXJBS0DEKuv9Xj5Aw5z9C2l+RZq2h/bc\n\tmAALlw9IOfqmCge1taiQUOPd5DAnOUcY+n9vb/GkLiBqYXTXEv4epzn5LQN+cMDs2DIb\n\tonjA==","X-Gm-Message-State":"APjAAAVFwVuNlmoiKWyFLYOlNc4keJGvBqZmckxbWfSt78i3eZvRtt20\n\tLMFOjVG5a0DlQT9K+VFDZ1jJcQ==","X-Google-Smtp-Source":"APXvYqw/4qdkErKI8RGxpnqwNjuaqzfSxn3xwLX6rubjGOPxtv4dYK2h74JqAEQJbPp8GKKtZiG+dg==","X-Received":"by 2002:a2e:85d7:: with SMTP id\n\th23mr4733630ljj.129.1567066212191; \n\tThu, 29 Aug 2019 01:10:12 -0700 (PDT)","Date":"Thu, 29 Aug 2019 10:10:10 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829081010.GS28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>\n\t<20190828111840.GO28351@bigcity.dyn.berto.se>\n\t<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 08:10:13 -0000"}},{"id":2533,"web_url":"https://patchwork.libcamera.org/comment/2533/","msgid":"<20190829085657.dte3rl7bxxsa4j4k@uno.localdomain>","date":"2019-08-29T08:56:57","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote:\n> On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:\n> > > > Add a constructor for the V4L2ControlList class that accepts a list of\n> > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> > > > instance to be used for reading controls only.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/libcamera/include/v4l2_controls.h | 3 +++\n> > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> > > > 2 files changed, 20 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > > > index 10b726504951..86c84e06d741 100644\n> > > > --- a/src/libcamera/include/v4l2_controls.h\n> > > > +++ b/src/libcamera/include/v4l2_controls.h\n> > > > @@ -65,6 +65,9 @@ public:\n> > > > \tusing iterator = std::vector::iterator;\n> > > > \tusing const_iterator = std::vector::const_iterator;\n> > > >\n> > > > +\tV4L2ControlList() {}\n> > > > +\tV4L2ControlList(std::vector ids);\n> > > > +\n> > > > \titerator begin() { return controls_.begin(); }\n> > > > \tconst_iterator begin() const { return controls_.begin(); }\n> > > > \titerator end() { return controls_.end(); }\n> > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > > > index 84258d9954d0..eeb325cbfff9 100644\n> > > > --- a/src/libcamera/v4l2_controls.cpp\n> > > > +++ b/src/libcamera/v4l2_controls.cpp\n> > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > > > * and prepare to be re-used for a new control write/read sequence.\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> > > > + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> > > > + *\n> > > > + * Construct a V4L2ControlList from a list of control identifiers without any\n> > > > + * value associated. This constructor is particularly useful to create a\n> > > > + * V4L2ControlList that is used to read the values of all controls in the\n> > > > + * \\a ids list. The created V4L2ControlList should not be used to write control\n> > > > + * values unless it is cleared first and then controls with an associated value\n> > > > + * are manually added to it.\n> > > > + */\n> > >\n> > > Do you think it would be worth it to add some code to enforce that no\n> > > controls can be written if it's initialized with this constructor?\n> > >\n> >\n> > How would you do so? The most naive implementation that comes to mind\n> > it's to add a 'writable' flag to initialize to true only if the\n> > control has a value assigned and check for the flag\n> > V4L2Device::setControls()\n>\n> I'm not sure how to best implement it, but the constraint is so specific\n> it looked like something that could be described in code. Now that I dig\n> thru the code with fresh eyes I can't find the rational for the\n> V4L2ControlList needs to be cleared before it's attempted to be used for\n> writing.\n>\n> The control values are initialized to 0 using this new constructor and\n> if used for writing V4L2Device::setControls() will validate that the\n> control exists on the device and bail before writing anything if one the\n> controls do not exist.\n\nYes, it validates the control exists\nhttps://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251\n\nbut it will silently set the value of the v4l2 control to write to 0\nhttps://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265\n\nOne could\n V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... };\n device->setControls(&list);\n\nand if the controls in the list accept 0 as a value, set all controls\nto 0 maybe unintentionally.\n\n>\n> What reason do you think there is for the need to manually add controls\n> with a specific value before writing? This is just a specialisation\n> where all values are initialized to 0, at least in my mind I might be\n> missing something ;-)\n>\nWe have no guarantee the controls in the list accept 0, or that they\neven accepts integers at all (even if, we only support integers\ncontrols at the moment).\n\nWe cannot prevent anyone from shooting on his own foot, but to\nme documenting that the list should only be used for reading, or\nenforcing it in the code as you suggested, is a good thing...\n\n> >\n> > > > +V4L2ControlList::V4L2ControlList(std::vector ids)\n> > > > +{\n> > > > +\tfor (auto id : ids)\n> > > > +\t\tadd(id);\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\typedef V4L2ControlList::iterator\n> > > > * \\brief Iterator on the V4L2 controls contained in the instance\n> > > > --\n> > > > 2.23.0\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C379160BCF\n\tfor ;\n\tThu, 29 Aug 2019 10:55:25 +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 relay4-d.mail.gandi.net (Postfix) with ESMTPSA id EA9F5E0003;\n\tThu, 29 Aug 2019 08:55:24 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 29 Aug 2019 10:56:57 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829085657.dte3rl7bxxsa4j4k@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>\n\t<20190828111840.GO28351@bigcity.dyn.berto.se>\n\t<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>\n\t<20190829081010.GS28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"44ylfoqec7k5oc5g\"","Content-Disposition":"inline","In-Reply-To":"<20190829081010.GS28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 08:55:25 -0000"}},{"id":2537,"web_url":"https://patchwork.libcamera.org/comment/2537/","msgid":"<20190829090825.GW28351@bigcity.dyn.berto.se>","date":"2019-08-29T09:08:25","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2019-08-29 10:56:57 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote:\n> > On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:\n> > > > > Add a constructor for the V4L2ControlList class that accepts a list of\n> > > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> > > > > instance to be used for reading controls only.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/libcamera/include/v4l2_controls.h | 3 +++\n> > > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> > > > > 2 files changed, 20 insertions(+)\n> > > > >\n> > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > > > > index 10b726504951..86c84e06d741 100644\n> > > > > --- a/src/libcamera/include/v4l2_controls.h\n> > > > > +++ b/src/libcamera/include/v4l2_controls.h\n> > > > > @@ -65,6 +65,9 @@ public:\n> > > > > \tusing iterator = std::vector::iterator;\n> > > > > \tusing const_iterator = std::vector::const_iterator;\n> > > > >\n> > > > > +\tV4L2ControlList() {}\n> > > > > +\tV4L2ControlList(std::vector ids);\n> > > > > +\n> > > > > \titerator begin() { return controls_.begin(); }\n> > > > > \tconst_iterator begin() const { return controls_.begin(); }\n> > > > > \titerator end() { return controls_.end(); }\n> > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > > > > index 84258d9954d0..eeb325cbfff9 100644\n> > > > > --- a/src/libcamera/v4l2_controls.cpp\n> > > > > +++ b/src/libcamera/v4l2_controls.cpp\n> > > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > > > > * and prepare to be re-used for a new control write/read sequence.\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> > > > > + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> > > > > + *\n> > > > > + * Construct a V4L2ControlList from a list of control identifiers without any\n> > > > > + * value associated. This constructor is particularly useful to create a\n> > > > > + * V4L2ControlList that is used to read the values of all controls in the\n> > > > > + * \\a ids list. The created V4L2ControlList should not be used to write control\n> > > > > + * values unless it is cleared first and then controls with an associated value\n> > > > > + * are manually added to it.\n> > > > > + */\n> > > >\n> > > > Do you think it would be worth it to add some code to enforce that no\n> > > > controls can be written if it's initialized with this constructor?\n> > > >\n> > >\n> > > How would you do so? The most naive implementation that comes to mind\n> > > it's to add a 'writable' flag to initialize to true only if the\n> > > control has a value assigned and check for the flag\n> > > V4L2Device::setControls()\n> >\n> > I'm not sure how to best implement it, but the constraint is so specific\n> > it looked like something that could be described in code. Now that I dig\n> > thru the code with fresh eyes I can't find the rational for the\n> > V4L2ControlList needs to be cleared before it's attempted to be used for\n> > writing.\n> >\n> > The control values are initialized to 0 using this new constructor and\n> > if used for writing V4L2Device::setControls() will validate that the\n> > control exists on the device and bail before writing anything if one the\n> > controls do not exist.\n> \n> Yes, it validates the control exists\n> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251\n> \n> but it will silently set the value of the v4l2 control to write to 0\n> https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265\n> \n> One could\n> V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... };\n> device->setControls(&list);\n> \n> and if the controls in the list accept 0 as a value, set all controls\n> to 0 maybe unintentionally.\n\nAbsolutely, that's what I'm saying :-) We already have this feature with \nthe following code:\n\n V4L2ControlList list;\n list.add(V4L2_CID_...);\n list.add(V4L2_CID_...);\n device->setControls(&list);\n\nSo this new constructor is just a more convenient way to do the above \nand I see no real reason to block this use-case as the behaviour is well \ndefined. If V4L2ControlList::add() value argument did not have an \ndefault value set I agree this would be a slightly different change.\n\nIf we add logic to prevent a V4L2ControlList initialized with this new \nconstructor from being written we should also block V4L2ControlList \nwhere controls where added with just an id an no explicit value from \nbeing written.\n\n> \n> >\n> > What reason do you think there is for the need to manually add controls\n> > with a specific value before writing? This is just a specialisation\n> > where all values are initialized to 0, at least in my mind I might be\n> > missing something ;-)\n> >\n> We have no guarantee the controls in the list accept 0, or that they\n> even accepts integers at all (even if, we only support integers\n> controls at the moment).\n> \n> We cannot prevent anyone from shooting on his own foot, but to\n> me documenting that the list should only be used for reading, or\n> enforcing it in the code as you suggested, is a good thing...\n> \n> > >\n> > > > > +V4L2ControlList::V4L2ControlList(std::vector ids)\n> > > > > +{\n> > > > > +\tfor (auto id : ids)\n> > > > > +\t\tadd(id);\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\typedef V4L2ControlList::iterator\n> > > > > * \\brief Iterator on the V4L2 controls contained in the instance\n> > > > > --\n> > > > > 2.23.0\n> > > > >\n> > > > > _______________________________________________\n> > > > > libcamera-devel mailing list\n> > > > > libcamera-devel@lists.libcamera.org\n> > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\n> > > > --\n> > > > Regards,\n> > > > Niklas Söderlund\n> >\n> >\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA4B860BCF\n\tfor ;\n\tThu, 29 Aug 2019 11:08:26 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id q27so1887623lfo.10\n\tfor ;\n\tThu, 29 Aug 2019 02:08:26 -0700 (PDT)","from localhost (h-177-236.A463.priv.bahnhof.se. [217.31.177.236])\n\tby smtp.gmail.com with ESMTPSA id\n\tr8sm295614lfc.39.2019.08.29.02.08.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Aug 2019 02:08:25 -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=dhV482BX62kVw/b/sblBDYOUqfgd7UXGQks5V09pIUM=;\n\tb=rs7trNfOm7PXnLgnfAKt6hOVjV4PHHmg/s3DTZZZq49gNoQxdW97ZQ6l3aF+Fr5utw\n\tI+cK/K/ORBjiOKYXXQtWi2IZHX9b8a1syqyc5cEaIzWlfcHK28EbWWobcZE6Faq22s7E\n\t+wv+JYSq9BCmtIrvOxKnblGjFvz+JleOgLSVly2R4IauX4l3t8ZgkGl9LxZWIM89CKg5\n\teBSyUR0Za+0xfIgoCi20OcZMW/gx56OGlR3s8DQiA+cEUaZiA/RBnPl7vHjbdoXArDio\n\t7li8u87sveKB6ZppncnYK+i4rWUB6ly0Xohs3+1aoa1TgK/CEe5X+6Z53sFlYAm5+aqy\n\tZVaQ==","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=dhV482BX62kVw/b/sblBDYOUqfgd7UXGQks5V09pIUM=;\n\tb=fmUrxGe8H4nTqca6Z9qeffarNcVB89v/0Fu3KjnGalR6i2b0c9jzO8JRJVYQ1iMo3a\n\tcCoxItuEJ8j1O/aL3s/OO0miGtUJWN2+W9Q0zQI+9GgKzpxS7h7mdjyeFFHAdxFDP6TF\n\tAK0j8o5ghVfFgTat1jnItBylmlX5xgJRxTxfAr2jYWABGJk0Ts0JXXcPF0AVH/8W/s6N\n\tDk+rKUA12Jayo5MjQ6lquxnkJmyAS44loRAaxX6QQlfTck4rBaPSRlF/rAtypyV1FThp\n\t2OhvA8YZ3wlskSTDlozU3Z54/yjfopIo1BVqUIO6TID5laPNzVPQQwHWbgKcvBokF+GS\n\typhA==","X-Gm-Message-State":"APjAAAXRAY9PBp+Bo+EedT8QnHj6jnAOIgM8dD34zO8ftHOItvrserB9\n\th1oC0NV+1q+33Z5DqouTUkVuKszsMDI=","X-Google-Smtp-Source":"APXvYqxFXK1Bl2KUDndllAzI+E1Dw/h6v24qMLnNCCcCVPTye9bqx5UBQpqYTRifG62vxMz4TjotYA==","X-Received":"by 2002:a19:4b0d:: with SMTP id\n\ty13mr5579769lfa.128.1567069706314; \n\tThu, 29 Aug 2019 02:08:26 -0700 (PDT)","Date":"Thu, 29 Aug 2019 11:08:25 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829090825.GW28351@bigcity.dyn.berto.se>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>\n\t<20190828111840.GO28351@bigcity.dyn.berto.se>\n\t<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>\n\t<20190829081010.GS28351@bigcity.dyn.berto.se>\n\t<20190829085657.dte3rl7bxxsa4j4k@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190829085657.dte3rl7bxxsa4j4k@uno.localdomain>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 09:08:27 -0000"}},{"id":2538,"web_url":"https://patchwork.libcamera.org/comment/2538/","msgid":"<20190829091345.xrezkwb5q3fpcclg@uno.localdomain>","date":"2019-08-29T09:13:45","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Aug 29, 2019 at 11:08:25AM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2019-08-29 10:56:57 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Thu, Aug 29, 2019 at 10:10:10AM +0200, Niklas Söderlund wrote:\n> > > On 2019-08-28 18:28:43 +0200, Jacopo Mondi wrote:\n> > > > Hi Niklas,\n> > > >\n> > > > On Wed, Aug 28, 2019 at 01:18:40PM +0200, Niklas Söderlund wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thanks for your work.\n> > > > >\n> > > > > On 2019-08-27 11:50:04 +0200, Jacopo Mondi wrote:\n> > > > > > Add a constructor for the V4L2ControlList class that accepts a list of\n> > > > > > V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> > > > > > instance to be used for reading controls only.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/libcamera/include/v4l2_controls.h | 3 +++\n> > > > > > src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> > > > > > 2 files changed, 20 insertions(+)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > > > > > index 10b726504951..86c84e06d741 100644\n> > > > > > --- a/src/libcamera/include/v4l2_controls.h\n> > > > > > +++ b/src/libcamera/include/v4l2_controls.h\n> > > > > > @@ -65,6 +65,9 @@ public:\n> > > > > > \tusing iterator = std::vector::iterator;\n> > > > > > \tusing const_iterator = std::vector::const_iterator;\n> > > > > >\n> > > > > > +\tV4L2ControlList() {}\n> > > > > > +\tV4L2ControlList(std::vector ids);\n> > > > > > +\n> > > > > > \titerator begin() { return controls_.begin(); }\n> > > > > > \tconst_iterator begin() const { return controls_.begin(); }\n> > > > > > \titerator end() { return controls_.end(); }\n> > > > > > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > > > > > index 84258d9954d0..eeb325cbfff9 100644\n> > > > > > --- a/src/libcamera/v4l2_controls.cpp\n> > > > > > +++ b/src/libcamera/v4l2_controls.cpp\n> > > > > > @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > > > > > * and prepare to be re-used for a new control write/read sequence.\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> > > > > > + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> > > > > > + *\n> > > > > > + * Construct a V4L2ControlList from a list of control identifiers without any\n> > > > > > + * value associated. This constructor is particularly useful to create a\n> > > > > > + * V4L2ControlList that is used to read the values of all controls in the\n> > > > > > + * \\a ids list. The created V4L2ControlList should not be used to write control\n> > > > > > + * values unless it is cleared first and then controls with an associated value\n> > > > > > + * are manually added to it.\n> > > > > > + */\n> > > > >\n> > > > > Do you think it would be worth it to add some code to enforce that no\n> > > > > controls can be written if it's initialized with this constructor?\n> > > > >\n> > > >\n> > > > How would you do so? The most naive implementation that comes to mind\n> > > > it's to add a 'writable' flag to initialize to true only if the\n> > > > control has a value assigned and check for the flag\n> > > > V4L2Device::setControls()\n> > >\n> > > I'm not sure how to best implement it, but the constraint is so specific\n> > > it looked like something that could be described in code. Now that I dig\n> > > thru the code with fresh eyes I can't find the rational for the\n> > > V4L2ControlList needs to be cleared before it's attempted to be used for\n> > > writing.\n> > >\n> > > The control values are initialized to 0 using this new constructor and\n> > > if used for writing V4L2Device::setControls() will validate that the\n> > > control exists on the device and bail before writing anything if one the\n> > > controls do not exist.\n> >\n> > Yes, it validates the control exists\n> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n251\n> >\n> > but it will silently set the value of the v4l2 control to write to 0\n> > https://git.linuxtv.org/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n265\n> >\n> > One could\n> > V4L2ControlList list = { V4L2_CID_..., V4L2_CID_... };\n> > device->setControls(&list);\n> >\n> > and if the controls in the list accept 0 as a value, set all controls\n> > to 0 maybe unintentionally.\n>\n> Absolutely, that's what I'm saying :-) We already have this feature with\n> the following code:\n>\n> V4L2ControlList list;\n> list.add(V4L2_CID_...);\n> list.add(V4L2_CID_...);\n> device->setControls(&list);\n>\n> So this new constructor is just a more convenient way to do the above\n> and I see no real reason to block this use-case as the behaviour is well\n> defined. If V4L2ControlList::add() value argument did not have an\n> default value set I agree this would be a slightly different change.\n>\n> If we add logic to prevent a V4L2ControlList initialized with this new\n> constructor from being written we should also block V4L2ControlList\n> where controls where added with just an id an no explicit value from\n> being written.\n>\n\nIndeed, this applies to all controls added without a value, not just\nthe lists constructed with the newly introduced constructor, which is\njust a shortcut to what you have here described...\n\n> >\n> > >\n> > > What reason do you think there is for the need to manually add controls\n> > > with a specific value before writing? This is just a specialisation\n> > > where all values are initialized to 0, at least in my mind I might be\n> > > missing something ;-)\n> > >\n> > We have no guarantee the controls in the list accept 0, or that they\n> > even accepts integers at all (even if, we only support integers\n> > controls at the moment).\n> >\n> > We cannot prevent anyone from shooting on his own foot, but to\n> > me documenting that the list should only be used for reading, or\n> > enforcing it in the code as you suggested, is a good thing...\n> >\n> > > >\n> > > > > > +V4L2ControlList::V4L2ControlList(std::vector ids)\n> > > > > > +{\n> > > > > > +\tfor (auto id : ids)\n> > > > > > +\t\tadd(id);\n> > > > > > +}\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\typedef V4L2ControlList::iterator\n> > > > > > * \\brief Iterator on the V4L2 controls contained in the instance\n> > > > > > --\n> > > > > > 2.23.0\n> > > > > >\n> > > > > > _______________________________________________\n> > > > > > libcamera-devel mailing list\n> > > > > > libcamera-devel@lists.libcamera.org\n> > > > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >\n> > > > > --\n> > > > > Regards,\n> > > > > Niklas Söderlund\n> > >\n> > >\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n>\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B275860BCF\n\tfor ;\n\tThu, 29 Aug 2019 11:12:13 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 310B81BF212;\n\tThu, 29 Aug 2019 09:12:12 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 29 Aug 2019 11:13:45 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190829091345.xrezkwb5q3fpcclg@uno.localdomain>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>\n\t<20190828111840.GO28351@bigcity.dyn.berto.se>\n\t<20190828162843.xyq7ivemt3cpu5qh@uno.localdomain>\n\t<20190829081010.GS28351@bigcity.dyn.berto.se>\n\t<20190829085657.dte3rl7bxxsa4j4k@uno.localdomain>\n\t<20190829090825.GW28351@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"6t6uch5ougrnvfn7\"","Content-Disposition":"inline","In-Reply-To":"<20190829090825.GW28351@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Thu, 29 Aug 2019 09:12:13 -0000"}},{"id":2573,"web_url":"https://patchwork.libcamera.org/comment/2573/","msgid":"<20190903201717.GC4788@pendragon.ideasonboard.com>","date":"2019-09-03T20:17:17","subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Aug 27, 2019 at 11:50:04AM +0200, Jacopo Mondi wrote:\n> Add a constructor for the V4L2ControlList class that accepts a list of\n> V4L2 control IDs (V4L2_CID_*). The constructor returns a V4L2ControlList\n> instance to be used for reading controls only.\n\nI tend to understand \"for reading controls only\" as meaning for reading\ncontrols and nothing else than controls. Is it just me ? Should this be\nexpressed as \"to be used only for reading controls\" ?\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/v4l2_controls.h | 3 +++\n> src/libcamera/v4l2_controls.cpp | 17 +++++++++++++++++\n> 2 files changed, 20 insertions(+)\n> \n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index 10b726504951..86c84e06d741 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -65,6 +65,9 @@ public:\n> \tusing iterator = std::vector::iterator;\n> \tusing const_iterator = std::vector::const_iterator;\n> \n> +\tV4L2ControlList() {}\n> +\tV4L2ControlList(std::vector ids);\n\nHow about using an initializer_list instead of a vector ?\n\n> +\n> \titerator begin() { return controls_.begin(); }\n> \tconst_iterator begin() const { return controls_.begin(); }\n> \titerator end() { return controls_.end(); }\n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index 84258d9954d0..eeb325cbfff9 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -202,6 +202,23 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> * and prepare to be re-used for a new control write/read sequence.\n> */\n> \n> +/**\n> + * \\brief Construct a V4L2ControlList from a list of V4L2 controls identifiers\n> + * \\param ids A list of V4L2 control identifiers (V4L2_CID_*)\n> + *\n> + * Construct a V4L2ControlList from a list of control identifiers without any\n> + * value associated. This constructor is particularly useful to create a\n\ns/value/values/\n\n> + * V4L2ControlList that is used to read the values of all controls in the\n> + * \\a ids list. The created V4L2ControlList should not be used to write control\n\ns/should/shall/\n\n> + * values unless it is cleared first and then controls with an associated value\n> + * are manually added to it.\n> + */\n> +V4L2ControlList::V4L2ControlList(std::vector ids)\n> +{\n> +\tfor (auto id : ids)\n\nLet's not abuse auto, this is clearly a case where you can write the\nfull type instead.\n\n> +\t\tadd(id);\n> +}\n> +\n> /**\n> * \\typedef V4L2ControlList::iterator\n> * \\brief Iterator on the V4L2 controls contained in the instance","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7137B60BCF\n\tfor ;\n\tTue, 3 Sep 2019 22:17:26 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-18-41-nat.elisa-mobile.fi\n\t[85.76.18.41])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DF9CE542;\n\tTue, 3 Sep 2019 22:17:25 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1567541846;\n\tbh=rkWcvroOgkmoA1RK6J3PS7gcd8HoNpazPE+1Kbs5Cn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iMh56rT5T0im6bh6Q8hhjW6GY/2VQAUtPS+AZTTvKyWXCggyST3MuAAzdaWcTqZIn\n\tTPOSNFfmNMM1VxouBB1qbYqcCBYWXI/yKaKEZXQq0G/3AWIBvUtbVjTy8XL+oc85SO\n\twjcqwJfOYOUJKllDNIni529/KfLEH4KUnOoNklwc=","Date":"Tue, 3 Sep 2019 23:17:17 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190903201717.GC4788@pendragon.ideasonboard.com>","References":"<20190827095008.11405-1-jacopo@jmondi.org>\n\t<20190827095008.11405-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190827095008.11405-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 4/7] libcamera: v4l2_controls:\n\tConstruct from a list of ids","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 03 Sep 2019 20:17:26 -0000"}}]