[{"id":13361,"web_url":"https://patchwork.libcamera.org/comment/13361/","msgid":"<24ddc728-6012-0725-48ae-4cf190fc925e@ideasonboard.com>","date":"2020-10-21T14:51:38","subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 21/10/2020 15:36, Jacopo Mondi wrote:\n> Add two constructors to the ControlInfo class that allows creating\n> a class instance from the list of the control supported values with\n> an optional default value.\n> \n> Signed-off-by: Jacopo Mondi \n\n\nReviewed-by: Kieran Bingham \n\n> ---\n> include/libcamera/controls.h | 3 +++\n> src/libcamera/controls.cpp | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index d1f6d4490c35..0099b6329026 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -269,6 +269,9 @@ public:\n> \t\t\t const ControlValue &max = 0,\n> \t\t\t const ControlValue &def = 0,\n> \t\t\t const std::vector &values = {});\n> +\texplicit ControlInfo(const std::vector &values,\n> +\t\t\t const ControlValue &def);\n> +\texplicit ControlInfo(const std::vector &values);\n> \n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 61feee37a1b8..389ecd5c519c 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -493,6 +493,37 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> {\n> }\n> \n> +/**\n> + * \\brief Construct a ControlInfo from the list of supported values and a default\n> + * \\param[in] values The control supported values\n> + * \\param[in] def The control default value\n> + *\n> + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> + * minimum and maximum values are assigned to the first and last members of\n> + * the values list respectively.\n> + */\n> +ControlInfo::ControlInfo(const std::vector &values,\n> +\t\t\t const ControlValue &def)\n> +\t: def_(def), values_(values)\n> +{\n> +\tmin_ = values_.front();\n> +\tmax_ = values_.back();\n> +}\n> +\n> +/**\n> + * \\brief Construct a ControlInfo from the list of supported values\n> + * \\param[in] values The control supported values\n> + *\n> + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> + * minimum and maximum values are assigned to the first and last members of\n> + * the values list respectively. The ControlInfo default value is set to be\n> + * equal to the minimum one.\n> + */\n> +ControlInfo::ControlInfo(const std::vector &values)\n> +\t: ControlInfo(values, values.front())\n> +{\n> +}\n> +\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 65FBCBDB13\n\tfor ;\n\tWed, 21 Oct 2020 14:51:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E960A61E10;\n\tWed, 21 Oct 2020 16:51:44 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88AD361E0D\n\tfor ;\n\tWed, 21 Oct 2020 16:51:43 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 151C792;\n\tWed, 21 Oct 2020 16:51:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fbYIujpZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603291903;\n\tbh=TfdhY7i4L/SaTZwmicx2h8EAQiJzflcubRWttoWXs/k=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=fbYIujpZoYqHtixL6aliF7GVQnc4X/WqtJdtgZH81HF3rW7RTBMnLCD3HdVIRJ/S0\n\tybN6DAbrkEpGo3krSsSC6jlQQG9AEmKF3spZtC34Hd+QcE21koy15Z/OfPURRFGdln\n\tTEKynFMP/zLa8WDBGrC9TPizvHYJakd3wmfISkdY=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-5-jacopo@jmondi.org>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<24ddc728-6012-0725-48ae-4cf190fc925e@ideasonboard.com>","Date":"Wed, 21 Oct 2020 15:51:38 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201021143635.22846-5-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13384,"web_url":"https://patchwork.libcamera.org/comment/13384/","msgid":"<20201022021214.GW3942@pendragon.ideasonboard.com>","date":"2020-10-22T02:12:14","subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","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 Wed, Oct 21, 2020 at 04:36:25PM +0200, Jacopo Mondi wrote:\n> Add two constructors to the ControlInfo class that allows creating\n\ns/allows/allow/\n\n> a class instance from the list of the control supported values with\n\nI'd talk about \"valid values\" instead of \"supported values\" here and in\nseveral other places. An enumerated control will have all supported\nvalues defined in the yaml file, and ControlInfo will list the valid\nvalues for a particular pipeline handler (or maybe 'valid' and\n'supported' could be switched actually). The point is to explain that\nthere's a difference between what the API defines and what a particular\ncamera supports. This would be best explained, I think, when documenting\nthe concept of enumerated controls as mentioned in the review of 03/14.\n\n> an optional default value.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/controls.h | 3 +++\n> src/libcamera/controls.cpp | 31 +++++++++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+)\n> \n> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> index d1f6d4490c35..0099b6329026 100644\n> --- a/include/libcamera/controls.h\n> +++ b/include/libcamera/controls.h\n> @@ -269,6 +269,9 @@ public:\n> \t\t\t const ControlValue &max = 0,\n> \t\t\t const ControlValue &def = 0,\n> \t\t\t const std::vector &values = {});\n> +\texplicit ControlInfo(const std::vector &values,\n> +\t\t\t const ControlValue &def);\n> +\texplicit ControlInfo(const std::vector &values);\n> \n> \tconst ControlValue &min() const { return min_; }\n> \tconst ControlValue &max() const { return max_; }\n> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> index 61feee37a1b8..389ecd5c519c 100644\n> --- a/src/libcamera/controls.cpp\n> +++ b/src/libcamera/controls.cpp\n> @@ -493,6 +493,37 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> {\n> }\n> \n> +/**\n> + * \\brief Construct a ControlInfo from the list of supported values and a default\n> + * \\param[in] values The control supported values\n> + * \\param[in] def The control default value\n> + *\n> + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> + * minimum and maximum values are assigned to the first and last members of\n\ns/assigned/set/ (otherwise it means the opposite, .front() = min)\ns/members/entries/\n\nSame comments for the next function.\n\nI think you should mention that the values must be sorted in increasing\norder. Is it worth adding an assertion if that's not the case ? You can\nuse std::is_sorted. This is a bit of a rabbit hole, as it requires\ndefining operator< for ControlValue, but we can restrict the\nimplementation for now to the int32 type (as that's what we use for\nenumerated controls), so it may not be too complicated.\n\n> + * the values list respectively.\n> + */\n> +ControlInfo::ControlInfo(const std::vector &values,\n> +\t\t\t const ControlValue &def)\n> +\t: def_(def), values_(values)\n> +{\n> +\tmin_ = values_.front();\n> +\tmax_ = values_.back();\n> +}\n> +\n> +/**\n> + * \\brief Construct a ControlInfo from the list of supported values\n> + * \\param[in] values The control supported values\n> + *\n> + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> + * minimum and maximum values are assigned to the first and last members of\n> + * the values list respectively. The ControlInfo default value is set to be\n> + * equal to the minimum one.\n> + */\n> +ControlInfo::ControlInfo(const std::vector &values)\n> +\t: ControlInfo(values, values.front())\n> +{\n> +}\n> +\n> /**\n> * \\fn ControlInfo::min()\n> * \\brief Retrieve the minimum value of the control","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 51C2EBDB13\n\tfor ;\n\tThu, 22 Oct 2020 02:13:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D12B361059;\n\tThu, 22 Oct 2020 04:13:02 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 107706034F\n\tfor ;\n\tThu, 22 Oct 2020 04:13:01 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7334D53;\n\tThu, 22 Oct 2020 04:13:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EWI/Ldqc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603332780;\n\tbh=9KwmsoboxvXIee1n9hSb76BTuDY2O9A8gHHVtkuVSKU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EWI/Ldqc41YPI7fbixCbCOFUQs4yhCC8gDoCKpqEeeYuT4azzg5vUjU3FRxnDnVYJ\n\tONawoxqIiaw5xEbYcZlnuBO6VGpNdHfTSkKFC0e00OUIWiWhFMJH+nLl1dHG3gkjv/\n\tJfjtTymMJ8qQ0/Y8ZqQCiyY/QEXGOW7xRpiMpSyU=","Date":"Thu, 22 Oct 2020 05:12:14 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201022021214.GW3942@pendragon.ideasonboard.com>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201021143635.22846-5-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13445,"web_url":"https://patchwork.libcamera.org/comment/13445/","msgid":"<20201023091335.iu2o2zcikugcf2g7@uno.localdomain>","date":"2020-10-23T09:13:35","subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Thu, Oct 22, 2020 at 05:12:14AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Oct 21, 2020 at 04:36:25PM +0200, Jacopo Mondi wrote:\n> > Add two constructors to the ControlInfo class that allows creating\n>\n> s/allows/allow/\n>\n> > a class instance from the list of the control supported values with\n>\n> I'd talk about \"valid values\" instead of \"supported values\" here and in\n> several other places. An enumerated control will have all supported\n> values defined in the yaml file, and ControlInfo will list the valid\n> values for a particular pipeline handler (or maybe 'valid' and\n> 'supported' could be switched actually). The point is to explain that\n> there's a difference between what the API defines and what a particular\n> camera supports. This would be best explained, I think, when documenting\n> the concept of enumerated controls as mentioned in the review of 03/14.\n>\n> > an optional default value.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/controls.h | 3 +++\n> > src/libcamera/controls.cpp | 31 +++++++++++++++++++++++++++++++\n> > 2 files changed, 34 insertions(+)\n> >\n> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > index d1f6d4490c35..0099b6329026 100644\n> > --- a/include/libcamera/controls.h\n> > +++ b/include/libcamera/controls.h\n> > @@ -269,6 +269,9 @@ public:\n> > \t\t\t const ControlValue &max = 0,\n> > \t\t\t const ControlValue &def = 0,\n> > \t\t\t const std::vector &values = {});\n> > +\texplicit ControlInfo(const std::vector &values,\n> > +\t\t\t const ControlValue &def);\n> > +\texplicit ControlInfo(const std::vector &values);\n> >\n> > \tconst ControlValue &min() const { return min_; }\n> > \tconst ControlValue &max() const { return max_; }\n> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > index 61feee37a1b8..389ecd5c519c 100644\n> > --- a/src/libcamera/controls.cpp\n> > +++ b/src/libcamera/controls.cpp\n> > @@ -493,6 +493,37 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > {\n> > }\n> >\n> > +/**\n> > + * \\brief Construct a ControlInfo from the list of supported values and a default\n> > + * \\param[in] values The control supported values\n> > + * \\param[in] def The control default value\n> > + *\n> > + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> > + * minimum and maximum values are assigned to the first and last members of\n>\n> s/assigned/set/ (otherwise it means the opposite, .front() = min)\n> s/members/entries/\n>\n> Same comments for the next function.\n>\n> I think you should mention that the values must be sorted in increasing\n> order. Is it worth adding an assertion if that's not the case ? You can\n> use std::is_sorted. This is a bit of a rabbit hole, as it requires\n> defining operator< for ControlValue, but we can restrict the\n> implementation for now to the int32 type (as that's what we use for\n> enumerated controls), so it may not be too complicated.\n>\n\nAs said to Kieran, we're in control of the definition of the enumerate\ncontrols. Currently, I don't see a use case for out-of-order\nenumerated values, and I would rather std::sort them if we want to be\nextra paranoid, since it requires operator< anyway.o\n\n\n> > + * the values list respectively.\n> > + */\n> > +ControlInfo::ControlInfo(const std::vector &values,\n> > +\t\t\t const ControlValue &def)\n> > +\t: def_(def), values_(values)\n> > +{\n> > +\tmin_ = values_.front();\n> > +\tmax_ = values_.back();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a ControlInfo from the list of supported values\n> > + * \\param[in] values The control supported values\n> > + *\n> > + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> > + * minimum and maximum values are assigned to the first and last members of\n> > + * the values list respectively. The ControlInfo default value is set to be\n> > + * equal to the minimum one.\n> > + */\n> > +ControlInfo::ControlInfo(const std::vector &values)\n> > +\t: ControlInfo(values, values.front())\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\fn ControlInfo::min()\n> > * \\brief Retrieve the minimum value of the control\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C2AC5C3D3C\n\tfor ;\n\tFri, 23 Oct 2020 09:13:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 503EF615D5;\n\tFri, 23 Oct 2020 11:13:39 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7D2960350\n\tfor ;\n\tFri, 23 Oct 2020 11:13:37 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 5E68CFF814;\n\tFri, 23 Oct 2020 09:13:37 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 23 Oct 2020 11:13:35 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201023091335.iu2o2zcikugcf2g7@uno.localdomain>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-5-jacopo@jmondi.org>\n\t<20201022021214.GW3942@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201022021214.GW3942@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13452,"web_url":"https://patchwork.libcamera.org/comment/13452/","msgid":"<20201023201530.GD5979@pendragon.ideasonboard.com>","date":"2020-10-23T20:15:30","subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 23, 2020 at 11:13:35AM +0200, Jacopo Mondi wrote:\n> On Thu, Oct 22, 2020 at 05:12:14AM +0300, Laurent Pinchart wrote:\n> > On Wed, Oct 21, 2020 at 04:36:25PM +0200, Jacopo Mondi wrote:\n> > > Add two constructors to the ControlInfo class that allows creating\n> >\n> > s/allows/allow/\n> >\n> > > a class instance from the list of the control supported values with\n> >\n> > I'd talk about \"valid values\" instead of \"supported values\" here and in\n> > several other places. An enumerated control will have all supported\n> > values defined in the yaml file, and ControlInfo will list the valid\n> > values for a particular pipeline handler (or maybe 'valid' and\n> > 'supported' could be switched actually). The point is to explain that\n> > there's a difference between what the API defines and what a particular\n> > camera supports. This would be best explained, I think, when documenting\n> > the concept of enumerated controls as mentioned in the review of 03/14.\n> >\n> > > an optional default value.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/controls.h | 3 +++\n> > > src/libcamera/controls.cpp | 31 +++++++++++++++++++++++++++++++\n> > > 2 files changed, 34 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h\n> > > index d1f6d4490c35..0099b6329026 100644\n> > > --- a/include/libcamera/controls.h\n> > > +++ b/include/libcamera/controls.h\n> > > @@ -269,6 +269,9 @@ public:\n> > > \t\t\t const ControlValue &max = 0,\n> > > \t\t\t const ControlValue &def = 0,\n> > > \t\t\t const std::vector &values = {});\n> > > +\texplicit ControlInfo(const std::vector &values,\n> > > +\t\t\t const ControlValue &def);\n> > > +\texplicit ControlInfo(const std::vector &values);\n> > >\n> > > \tconst ControlValue &min() const { return min_; }\n> > > \tconst ControlValue &max() const { return max_; }\n> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp\n> > > index 61feee37a1b8..389ecd5c519c 100644\n> > > --- a/src/libcamera/controls.cpp\n> > > +++ b/src/libcamera/controls.cpp\n> > > @@ -493,6 +493,37 @@ ControlInfo::ControlInfo(const ControlValue &min,\n> > > {\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Construct a ControlInfo from the list of supported values and a default\n> > > + * \\param[in] values The control supported values\n> > > + * \\param[in] def The control default value\n> > > + *\n> > > + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> > > + * minimum and maximum values are assigned to the first and last members of\n> >\n> > s/assigned/set/ (otherwise it means the opposite, .front() = min)\n> > s/members/entries/\n> >\n> > Same comments for the next function.\n> >\n> > I think you should mention that the values must be sorted in increasing\n> > order. Is it worth adding an assertion if that's not the case ? You can\n> > use std::is_sorted. This is a bit of a rabbit hole, as it requires\n> > defining operator< for ControlValue, but we can restrict the\n> > implementation for now to the int32 type (as that's what we use for\n> > enumerated controls), so it may not be too complicated.\n> \n> As said to Kieran, we're in control of the definition of the enumerate\n> controls. Currently, I don't see a use case for out-of-order\n> enumerated values, and I would rather std::sort them if we want to be\n> extra paranoid, since it requires operator< anyway.o\n\nWhen a pipeline handler or IPA passes the full list of values, as\ngenerated by our scripts and stored in the Values vector, there's\nno issue. However, a pipeline handler or IPA can also restrict the\nvalues they support by passing a manually-written list of values, in\nwhich case errors could happen. That's what I'd like to catch with an\nassertion error, to make sure we don't let this kind of bug creep in.\n\nPlease let me know if you would like me to help with the implementation\nof operator<.\n\n> > > + * the values list respectively.\n> > > + */\n> > > +ControlInfo::ControlInfo(const std::vector &values,\n> > > +\t\t\t const ControlValue &def)\n> > > +\t: def_(def), values_(values)\n> > > +{\n> > > +\tmin_ = values_.front();\n> > > +\tmax_ = values_.back();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a ControlInfo from the list of supported values\n> > > + * \\param[in] values The control supported values\n> > > + *\n> > > + * Construct a ControlInfo from a list of supported values. The ControlInfo\n> > > + * minimum and maximum values are assigned to the first and last members of\n> > > + * the values list respectively. The ControlInfo default value is set to be\n> > > + * equal to the minimum one.\n> > > + */\n> > > +ControlInfo::ControlInfo(const std::vector &values)\n> > > +\t: ControlInfo(values, values.front())\n> > > +{\n> > > +}\n> > > +\n> > > /**\n> > > * \\fn ControlInfo::min()\n> > > * \\brief Retrieve the minimum value of the control","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BE5EEC3D3C\n\tfor ;\n\tFri, 23 Oct 2020 20:16:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3DBC9615D3;\n\tFri, 23 Oct 2020 22:16:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 169D960350\n\tfor ;\n\tFri, 23 Oct 2020 22:16:17 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7B26DB26;\n\tFri, 23 Oct 2020 22:16:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"l04U4qGG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603484176;\n\tbh=vBowUbkFtCBiFtfX3E1wTkTQpqdDnR0Xv9XJAMZKKf8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l04U4qGG0GbwmyTwnQBUblAn20MeGxIj+yfcwL7syS23TpzqvZEH5SIU7JNZsJpoP\n\talyQEgg2R1kBpaSPSueJ6m8dHTewiIMibHQuLeohURLtlOZ//by4T6vYU/fjjNY37E\n\tbjifp+49X18ouZHHEV8Y4Uo5pO6JbuphudOw3Bnc=","Date":"Fri, 23 Oct 2020 23:15:30 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201023201530.GD5979@pendragon.ideasonboard.com>","References":"<20201021143635.22846-1-jacopo@jmondi.org>\n\t<20201021143635.22846-5-jacopo@jmondi.org>\n\t<20201022021214.GW3942@pendragon.ideasonboard.com>\n\t<20201023091335.iu2o2zcikugcf2g7@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201023091335.iu2o2zcikugcf2g7@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 04/14] libcamera: controls:\n\tConstruct from values list","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]