[{"id":2679,"web_url":"https://patchwork.libcamera.org/comment/2679/","msgid":"<7e67d0c2-0e80-585a-9a0e-d0bb81717a4d@ideasonboard.com>","date":"2019-09-20T11:15:07","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use\n\tDataValue and DataInfo","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 18/09/2019 11:34, Jacopo Mondi wrote:\n> Use DataValue and DataInfo in the V4L2 control handling classes to\n> maximize code reuse with the libcamera controls classes.\n\nThis makes me happy :D\n\nThis patch makes a subtle change to the way \"->type()\" is used.\n\nCould that be noted in the commit message please?\n(Just so that it's clear)\n\nSomething like:\n\n\"The control type is now represented by the DataType from the DataValue\nrather than the type_ previously stored in the V4L2ControlInfo\"\n\nOtherwise, I think this all looks fine.\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/include/v4l2_controls.h | 22 ++++--------\n>  src/libcamera/include/v4l2_device.h   |  1 -\n>  src/libcamera/v4l2_controls.cpp       | 49 +++++++--------------------\n>  src/libcamera/v4l2_device.cpp         | 25 ++++++--------\n>  4 files changed, 30 insertions(+), 67 deletions(-)\n> \n> diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> index 10b726504951..27b855f3407f 100644\n> --- a/src/libcamera/include/v4l2_controls.h\n> +++ b/src/libcamera/include/v4l2_controls.h\n> @@ -16,29 +16,18 @@\n>  #include <linux/v4l2-controls.h>\n>  #include <linux/videodev2.h>\n> \n> +#include <libcamera/data_value.h>\n> +\n>  namespace libcamera {\n> \n> -class V4L2ControlInfo\n> +class V4L2ControlInfo : public DataInfo\n>  {\n>  public:\n>  \tV4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n> -\n>  \tunsigned int id() const { return id_; }\n> -\tunsigned int type() const { return type_; }\n> -\tsize_t size() const { return size_; }\n> -\tconst std::string &name() const { return name_; }\n> -\n> -\tint64_t min() const { return min_; }\n> -\tint64_t max() const { return max_; }\n> \n>  private:\n>  \tunsigned int id_;\n> -\tunsigned int type_;\n> -\tsize_t size_;\n> -\tstd::string name_;\n> -\n> -\tint64_t min_;\n> -\tint64_t max_;\n>  };\n> \n>  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;\n> @@ -49,14 +38,15 @@ public:\n>  \tV4L2Control(unsigned int id, int value = 0)\n>  \t\t: id_(id), value_(value) {}\n> \n> -\tint64_t value() const { return value_; }\n> +\tDataType type() const { return value_.type(); }\n\nDo you need to expose the type? Aren't they all int64_t here?\n\nAh - ok I see below we switch from examining the V4L2ControlInfo->type()\nto examining the V4L2Control->type().\n\nSo this is fine.\n\n\n> +\tint64_t value() const { return value_.getInt64(); }\n>  \tvoid setValue(int64_t value) { value_ = value; }\n> \n>  \tunsigned int id() const { return id_; }\n> \n>  private:\n>  \tunsigned int id_;\n> -\tint64_t value_;\n> +\tDataValue value_;\n>  };\n> \n>  class V4L2ControlList\n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index 75a52c33d821..3144adc26e36 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -44,7 +44,6 @@ protected:\n>  private:\n>  \tvoid listControls();\n>  \tvoid updateControls(V4L2ControlList *ctrls,\n> -\t\t\t    const V4L2ControlInfo **controlInfo,\n>  \t\t\t    const struct v4l2_ext_control *v4l2Ctrls,\n>  \t\t\t    unsigned int count);\n> \n> diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> index 84258d9954d0..9bc4929cbd76 100644\n> --- a/src/libcamera/v4l2_controls.cpp\n> +++ b/src/libcamera/v4l2_controls.cpp\n> @@ -69,13 +69,10 @@ namespace libcamera {\n>   * \\param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n>   */\n>  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> +\t: DataInfo(DataValue(static_cast<int64_t>(ctrl.minimum)),\n> +\t\t   DataValue(static_cast<int64_t>(ctrl.maximum))),\n> +\t  id_(ctrl.id)\n>  {\n> -\tid_ = ctrl.id;\n> -\ttype_ = ctrl.type;\n> -\tname_ = static_cast<const char *>(ctrl.name);\n> -\tsize_ = ctrl.elem_size * ctrl.elems;\n> -\tmin_ = ctrl.minimum;\n> -\tmax_ = ctrl.maximum;\n>  }\n> \n>  /**\n> @@ -84,36 +81,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n>   * \\return The V4L2 control ID\n>   */\n> \n> -/**\n> - * \\fn V4L2ControlInfo::type()\n> - * \\brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*\n\nOh - ok - so the types are subtly different now. But I think it's on a\npath towards good reuse of the DataValue, DataInfo classes.\n\n\n> - * \\return The V4L2 control type\n> - */\n> -\n> -/**\n> - * \\fn V4L2ControlInfo::size()\n> - * \\brief Retrieve the control value data size (in bytes)\n> - * \\return The V4L2 control value data size\n> - */\n> -\n> -/**\n> - * \\fn V4L2ControlInfo::name()\n> - * \\brief Retrieve the control user readable name\n> - * \\return The V4L2 control user readable name\n> - */\n> -\n> -/**\n> - * \\fn V4L2ControlInfo::min()\n> - * \\brief Retrieve the control minimum value\n> - * \\return The V4L2 control minimum value\n> - */\n> -\n> -/**\n> - * \\fn V4L2ControlInfo::max()\n> - * \\brief Retrieve the control maximum value\n> - * \\return The V4L2 control maximum value\n> - */\n> -\n>  /**\n>   * \\typedef V4L2ControlInfoMap\n>   * \\brief A map of control ID to V4L2ControlInfo\n> @@ -134,6 +101,10 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n>   * to be directly used but are instead intended to be grouped in\n>   * V4L2ControlList instances, which are then passed as parameters to\n>   * V4L2Device::setControls() and V4L2Device::getControls() operations.\n> + *\n> + * \\todo Currently all V4L2Controls are integers. For the sake of keeping the\n> + * implementation as simpler as possible treat all values as int64. The value()\n> + * and setValue() operations use that single data type for now.\n>   */\n> \n>  /**\n> @@ -143,6 +114,12 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n>   * \\param value The control value\n>   */\n> \n> +/**\n> + * \\fn V4L2Control::type()\n> + * \\brief Retrieve the type of the control\n> + * \\return The control type\n> + */\n> +\n>  /**\n>   * \\fn V4L2Control::value()\n>   * \\brief Retrieve the value of the control\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 349bf2d29704..2b7e3b1993ca 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -210,7 +210,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)\n>  \t\tret = errorIdx;\n>  \t}\n> \n> -\tupdateControls(ctrls, controlInfo, v4l2Ctrls, count);\n> +\tupdateControls(ctrls, v4l2Ctrls, count);\n> \n>  \treturn ret;\n>  }\n> @@ -262,8 +262,8 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n>  \t\tv4l2Ctrls[i].id = info->id();\n> \n>  \t\t/* Set the v4l2_ext_control value for the write operation. */\n> -\t\tswitch (info->type()) {\n> -\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> +\t\tswitch (ctrl->type()) {\n\nHrm. ... this feels like a hidden change between switching on the info\nto the ctrl. I hope that's ok, it probably is.\n\n<second pass edit, Yes I see the change throughout>\n\n\n\n> +\t\tcase DataTypeInteger64:\n>  \t\t\tv4l2Ctrls[i].value64 = ctrl->value();\n>  \t\t\tbreak;\n>  \t\tdefault:\n> @@ -299,7 +299,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n>  \t\tret = errorIdx;\n>  \t}\n> \n> -\tupdateControls(ctrls, controlInfo, v4l2Ctrls, count);\n> +\tupdateControls(ctrls, v4l2Ctrls, count);\n> \n>  \treturn ret;\n>  }\n> @@ -352,8 +352,7 @@ void V4L2Device::listControls()\n>  \t\t    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)\n>  \t\t\tcontinue;\n> \n> -\t\tV4L2ControlInfo info(ctrl);\n> -\t\tswitch (info.type()) {\n> +\t\tswitch (ctrl.type) {\n>  \t\tcase V4L2_CTRL_TYPE_INTEGER:\n>  \t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n>  \t\tcase V4L2_CTRL_TYPE_MENU:\n> @@ -364,11 +363,12 @@ void V4L2Device::listControls()\n>  \t\t\tbreak;\n>  \t\t/* \\todo Support compound controls. */\n>  \t\tdefault:\n> -\t\t\tLOG(V4L2, Debug) << \"Control type '\" << info.type()\n> +\t\t\tLOG(V4L2, Debug) << \"Control type '\" << ctrl.type\n>  \t\t\t\t\t << \"' not supported\";\n>  \t\t\tcontinue;\n>  \t\t}\n> \n> +\t\tV4L2ControlInfo info(ctrl);\n>  \t\tcontrols_.emplace(ctrl.id, info);\n>  \t}\n>  }\n> @@ -382,25 +382,22 @@ void V4L2Device::listControls()\n>   * \\param[in] count The number of controls to update\n>   */\n>  void V4L2Device::updateControls(V4L2ControlList *ctrls,\n> -\t\t\t\tconst V4L2ControlInfo **controlInfo,\n>  \t\t\t\tconst struct v4l2_ext_control *v4l2Ctrls,\n>  \t\t\t\tunsigned int count)\n>  {\n>  \tfor (unsigned int i = 0; i < count; ++i) {\n> -\t\tconst struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> -\t\tconst V4L2ControlInfo *info = controlInfo[i];\n>  \t\tV4L2Control *ctrl = ctrls->getByIndex(i);\n> \n> -\t\tswitch (info->type()) {\n> -\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> -\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> +\t\tswitch (ctrl->type()) {\n> +\t\tcase DataTypeInteger64:\n> +\t\t\tctrl->setValue(v4l2Ctrls[i].value64);\n>  \t\t\tbreak;\n>  \t\tdefault:\n>  \t\t\t/*\n>  \t\t\t * \\todo To be changed when support for string and\n>  \t\t\t * compound controls will be added.\n>  \t\t\t */\n> -\t\t\tctrl->setValue(v4l2Ctrl->value);\n> +\t\t\tctrl->setValue(v4l2Ctrls[i].value);\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\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>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["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 7FA5760BED\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 20 Sep 2019 13:15:10 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F34552F9;\n\tFri, 20 Sep 2019 13:15:09 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1568978110;\n\tbh=BwOtIArcbPsbNxlAgOAkF166CdzdFE9Qu0KhOpZuT/c=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=vhxh87zoIiLgNJW3ZnaHwIhH6v7lv+cAfAuSAAbaFAunNP0Fl8OJOnc2MI802IQfw\n\thtiWF6gR3fbbPfBja04mAcKkmRwFQD3NmmKJJmWLMj4iOHk13eI9tJ1pxqvo2yCwar\n\t1DZYyi6fYN7svrPD9XqbQ4PZZhfYBWj5PYZGrFdQ=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","References":"<20190918103424.14536-1-jacopo@jmondi.org>\n\t<20190918103424.14536-4-jacopo@jmondi.org>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\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+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCJQQYAQoADwIbDAUCWcOUawUJ\n\tB4D+AgAKCRChHkZyEKRh/XJhEACr5iidt/0MZ0rWRMCbZFMWD7D2g6nZeOp+F2zY8CEUW+sd\n\tCDVd9BH9QX9KN5SZo6YtJzMzSzpcx45VwTvtQW0n/6Eujg9EUqblfU9xqvqDmbjEapr5d/OL\n\t21GTALb0owKhA5qDUGEcKGCphpQffKhTNo/BP99jvmJUj7IPSKH97qPypi8/ym8bAxB+uY31\n\tgHTMHf1jMJJ1pRo2tYYPeIIHGDqXBI4sp5GHHF+JcIhgR/e/A6w/dgzHYmQPl2ix5eZYEZbV\n\tTRP+gkX4NV8oHqa/lR+xPOlWElGB57viOSOoWriqxQbFy8XbG1GR8cWlkNtGBGVWaJaSoORP\n\tiowD7irXL91bCyFIqL+7BVk3Jy4uzP744PzE80KwxOp5SQAp9sPzFbgsJrLev90PZySjFHG0\n\twP144DK7nBjOj/J0g9OHVASP1JjK+nw7SDoKnETDIdRC0XmiHXk7TXzPdkvO0UkpHdEPjZUp\n\tWyuc0MqehjR/hTTPt4m/Y14XzEcy6JREIjOrFfUZVho2QpOdv9CNryGdieRTNjUtz463CIaZ\n\tdPBiw9mOMBoNffkn9FIoCjLnAaj9gUAnEHWBZOEviQ5NuyqpeP0YtzI4iaRbSUkYZHej99X3\n\tVmHrdLlMqd/ZgYYbPGSL4AN3FVACb5CxuxEHwo029VcE5U3CSjzqtCoX12tm7A==","Organization":"Ideas on Board","Message-ID":"<7e67d0c2-0e80-585a-9a0e-d0bb81717a4d@ideasonboard.com>","Date":"Fri, 20 Sep 2019 12:15:07 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.8.0","MIME-Version":"1.0","In-Reply-To":"<20190918103424.14536-4-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use\n\tDataValue and DataInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Fri, 20 Sep 2019 11:15:10 -0000"}},{"id":2684,"web_url":"https://patchwork.libcamera.org/comment/2684/","msgid":"<20190923110740.wv7j3xmlamt5cqia@uno.localdomain>","date":"2019-09-23T11:07:53","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use\n\tDataValue and DataInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi kieran,\n\nOn Fri, Sep 20, 2019 at 12:15:07PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 18/09/2019 11:34, Jacopo Mondi wrote:\n> > Use DataValue and DataInfo in the V4L2 control handling classes to\n> > maximize code reuse with the libcamera controls classes.\n>\n> This makes me happy :D\n>\n> This patch makes a subtle change to the way \"->type()\" is used.\n>\n> Could that be noted in the commit message please?\n> (Just so that it's clear)\n>\n> Something like:\n>\n> \"The control type is now represented by the DataType from the DataValue\n> rather than the type_ previously stored in the V4L2ControlInfo\"\n>\n\nYes, good point. This needs to be tracked as it might require a\ncertain careful handling when we'll add support for compound types.\n\n> Otherwise, I think this all looks fine.\n>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\nThanks\n  j\n\n>\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/include/v4l2_controls.h | 22 ++++--------\n> >  src/libcamera/include/v4l2_device.h   |  1 -\n> >  src/libcamera/v4l2_controls.cpp       | 49 +++++++--------------------\n> >  src/libcamera/v4l2_device.cpp         | 25 ++++++--------\n> >  4 files changed, 30 insertions(+), 67 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/v4l2_controls.h b/src/libcamera/include/v4l2_controls.h\n> > index 10b726504951..27b855f3407f 100644\n> > --- a/src/libcamera/include/v4l2_controls.h\n> > +++ b/src/libcamera/include/v4l2_controls.h\n> > @@ -16,29 +16,18 @@\n> >  #include <linux/v4l2-controls.h>\n> >  #include <linux/videodev2.h>\n> >\n> > +#include <libcamera/data_value.h>\n> > +\n> >  namespace libcamera {\n> >\n> > -class V4L2ControlInfo\n> > +class V4L2ControlInfo : public DataInfo\n> >  {\n> >  public:\n> >  \tV4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl);\n> > -\n> >  \tunsigned int id() const { return id_; }\n> > -\tunsigned int type() const { return type_; }\n> > -\tsize_t size() const { return size_; }\n> > -\tconst std::string &name() const { return name_; }\n> > -\n> > -\tint64_t min() const { return min_; }\n> > -\tint64_t max() const { return max_; }\n> >\n> >  private:\n> >  \tunsigned int id_;\n> > -\tunsigned int type_;\n> > -\tsize_t size_;\n> > -\tstd::string name_;\n> > -\n> > -\tint64_t min_;\n> > -\tint64_t max_;\n> >  };\n> >\n> >  using V4L2ControlInfoMap = std::map<unsigned int, V4L2ControlInfo>;\n> > @@ -49,14 +38,15 @@ public:\n> >  \tV4L2Control(unsigned int id, int value = 0)\n> >  \t\t: id_(id), value_(value) {}\n> >\n> > -\tint64_t value() const { return value_; }\n> > +\tDataType type() const { return value_.type(); }\n>\n> Do you need to expose the type? Aren't they all int64_t here?\n>\n> Ah - ok I see below we switch from examining the V4L2ControlInfo->type()\n> to examining the V4L2Control->type().\n>\n> So this is fine.\n>\n>\n> > +\tint64_t value() const { return value_.getInt64(); }\n> >  \tvoid setValue(int64_t value) { value_ = value; }\n> >\n> >  \tunsigned int id() const { return id_; }\n> >\n> >  private:\n> >  \tunsigned int id_;\n> > -\tint64_t value_;\n> > +\tDataValue value_;\n> >  };\n> >\n> >  class V4L2ControlList\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index 75a52c33d821..3144adc26e36 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -44,7 +44,6 @@ protected:\n> >  private:\n> >  \tvoid listControls();\n> >  \tvoid updateControls(V4L2ControlList *ctrls,\n> > -\t\t\t    const V4L2ControlInfo **controlInfo,\n> >  \t\t\t    const struct v4l2_ext_control *v4l2Ctrls,\n> >  \t\t\t    unsigned int count);\n> >\n> > diff --git a/src/libcamera/v4l2_controls.cpp b/src/libcamera/v4l2_controls.cpp\n> > index 84258d9954d0..9bc4929cbd76 100644\n> > --- a/src/libcamera/v4l2_controls.cpp\n> > +++ b/src/libcamera/v4l2_controls.cpp\n> > @@ -69,13 +69,10 @@ namespace libcamera {\n> >   * \\param ctrl The struct v4l2_query_ext_ctrl as returned by the kernel\n> >   */\n> >  V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > +\t: DataInfo(DataValue(static_cast<int64_t>(ctrl.minimum)),\n> > +\t\t   DataValue(static_cast<int64_t>(ctrl.maximum))),\n> > +\t  id_(ctrl.id)\n> >  {\n> > -\tid_ = ctrl.id;\n> > -\ttype_ = ctrl.type;\n> > -\tname_ = static_cast<const char *>(ctrl.name);\n> > -\tsize_ = ctrl.elem_size * ctrl.elems;\n> > -\tmin_ = ctrl.minimum;\n> > -\tmax_ = ctrl.maximum;\n> >  }\n> >\n> >  /**\n> > @@ -84,36 +81,6 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> >   * \\return The V4L2 control ID\n> >   */\n> >\n> > -/**\n> > - * \\fn V4L2ControlInfo::type()\n> > - * \\brief Retrieve the control type as defined by V4L2_CTRL_TYPE_*\n>\n> Oh - ok - so the types are subtly different now. But I think it's on a\n> path towards good reuse of the DataValue, DataInfo classes.\n>\n>\n> > - * \\return The V4L2 control type\n> > - */\n> > -\n> > -/**\n> > - * \\fn V4L2ControlInfo::size()\n> > - * \\brief Retrieve the control value data size (in bytes)\n> > - * \\return The V4L2 control value data size\n> > - */\n> > -\n> > -/**\n> > - * \\fn V4L2ControlInfo::name()\n> > - * \\brief Retrieve the control user readable name\n> > - * \\return The V4L2 control user readable name\n> > - */\n> > -\n> > -/**\n> > - * \\fn V4L2ControlInfo::min()\n> > - * \\brief Retrieve the control minimum value\n> > - * \\return The V4L2 control minimum value\n> > - */\n> > -\n> > -/**\n> > - * \\fn V4L2ControlInfo::max()\n> > - * \\brief Retrieve the control maximum value\n> > - * \\return The V4L2 control maximum value\n> > - */\n> > -\n> >  /**\n> >   * \\typedef V4L2ControlInfoMap\n> >   * \\brief A map of control ID to V4L2ControlInfo\n> > @@ -134,6 +101,10 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> >   * to be directly used but are instead intended to be grouped in\n> >   * V4L2ControlList instances, which are then passed as parameters to\n> >   * V4L2Device::setControls() and V4L2Device::getControls() operations.\n> > + *\n> > + * \\todo Currently all V4L2Controls are integers. For the sake of keeping the\n> > + * implementation as simpler as possible treat all values as int64. The value()\n> > + * and setValue() operations use that single data type for now.\n> >   */\n> >\n> >  /**\n> > @@ -143,6 +114,12 @@ V4L2ControlInfo::V4L2ControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> >   * \\param value The control value\n> >   */\n> >\n> > +/**\n> > + * \\fn V4L2Control::type()\n> > + * \\brief Retrieve the type of the control\n> > + * \\return The control type\n> > + */\n> > +\n> >  /**\n> >   * \\fn V4L2Control::value()\n> >   * \\brief Retrieve the value of the control\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 349bf2d29704..2b7e3b1993ca 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -210,7 +210,7 @@ int V4L2Device::getControls(V4L2ControlList *ctrls)\n> >  \t\tret = errorIdx;\n> >  \t}\n> >\n> > -\tupdateControls(ctrls, controlInfo, v4l2Ctrls, count);\n> > +\tupdateControls(ctrls, v4l2Ctrls, count);\n> >\n> >  \treturn ret;\n> >  }\n> > @@ -262,8 +262,8 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n> >  \t\tv4l2Ctrls[i].id = info->id();\n> >\n> >  \t\t/* Set the v4l2_ext_control value for the write operation. */\n> > -\t\tswitch (info->type()) {\n> > -\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > +\t\tswitch (ctrl->type()) {\n>\n> Hrm. ... this feels like a hidden change between switching on the info\n> to the ctrl. I hope that's ok, it probably is.\n>\n> <second pass edit, Yes I see the change throughout>\n>\n>\n>\n> > +\t\tcase DataTypeInteger64:\n> >  \t\t\tv4l2Ctrls[i].value64 = ctrl->value();\n> >  \t\t\tbreak;\n> >  \t\tdefault:\n> > @@ -299,7 +299,7 @@ int V4L2Device::setControls(V4L2ControlList *ctrls)\n> >  \t\tret = errorIdx;\n> >  \t}\n> >\n> > -\tupdateControls(ctrls, controlInfo, v4l2Ctrls, count);\n> > +\tupdateControls(ctrls, v4l2Ctrls, count);\n> >\n> >  \treturn ret;\n> >  }\n> > @@ -352,8 +352,7 @@ void V4L2Device::listControls()\n> >  \t\t    ctrl.flags & V4L2_CTRL_FLAG_DISABLED)\n> >  \t\t\tcontinue;\n> >\n> > -\t\tV4L2ControlInfo info(ctrl);\n> > -\t\tswitch (info.type()) {\n> > +\t\tswitch (ctrl.type) {\n> >  \t\tcase V4L2_CTRL_TYPE_INTEGER:\n> >  \t\tcase V4L2_CTRL_TYPE_BOOLEAN:\n> >  \t\tcase V4L2_CTRL_TYPE_MENU:\n> > @@ -364,11 +363,12 @@ void V4L2Device::listControls()\n> >  \t\t\tbreak;\n> >  \t\t/* \\todo Support compound controls. */\n> >  \t\tdefault:\n> > -\t\t\tLOG(V4L2, Debug) << \"Control type '\" << info.type()\n> > +\t\t\tLOG(V4L2, Debug) << \"Control type '\" << ctrl.type\n> >  \t\t\t\t\t << \"' not supported\";\n> >  \t\t\tcontinue;\n> >  \t\t}\n> >\n> > +\t\tV4L2ControlInfo info(ctrl);\n> >  \t\tcontrols_.emplace(ctrl.id, info);\n> >  \t}\n> >  }\n> > @@ -382,25 +382,22 @@ void V4L2Device::listControls()\n> >   * \\param[in] count The number of controls to update\n> >   */\n> >  void V4L2Device::updateControls(V4L2ControlList *ctrls,\n> > -\t\t\t\tconst V4L2ControlInfo **controlInfo,\n> >  \t\t\t\tconst struct v4l2_ext_control *v4l2Ctrls,\n> >  \t\t\t\tunsigned int count)\n> >  {\n> >  \tfor (unsigned int i = 0; i < count; ++i) {\n> > -\t\tconst struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > -\t\tconst V4L2ControlInfo *info = controlInfo[i];\n> >  \t\tV4L2Control *ctrl = ctrls->getByIndex(i);\n> >\n> > -\t\tswitch (info->type()) {\n> > -\t\tcase V4L2_CTRL_TYPE_INTEGER64:\n> > -\t\t\tctrl->setValue(v4l2Ctrl->value64);\n> > +\t\tswitch (ctrl->type()) {\n> > +\t\tcase DataTypeInteger64:\n> > +\t\t\tctrl->setValue(v4l2Ctrls[i].value64);\n> >  \t\t\tbreak;\n> >  \t\tdefault:\n> >  \t\t\t/*\n> >  \t\t\t * \\todo To be changed when support for string and\n> >  \t\t\t * compound controls will be added.\n> >  \t\t\t */\n> > -\t\t\tctrl->setValue(v4l2Ctrl->value);\n> > +\t\t\tctrl->setValue(v4l2Ctrls[i].value);\n> >  \t\t\tbreak;\n> >  \t\t}\n> >  \t}\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> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F4E260BCE\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 23 Sep 2019 13:06:14 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id A695D100006;\n\tMon, 23 Sep 2019 11:06:13 +0000 (UTC)"],"Date":"Mon, 23 Sep 2019 13:07:53 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190923110740.wv7j3xmlamt5cqia@uno.localdomain>","References":"<20190918103424.14536-1-jacopo@jmondi.org>\n\t<20190918103424.14536-4-jacopo@jmondi.org>\n\t<7e67d0c2-0e80-585a-9a0e-d0bb81717a4d@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"5pcubqmmmdqvf3rs\"","Content-Disposition":"inline","In-Reply-To":"<7e67d0c2-0e80-585a-9a0e-d0bb81717a4d@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: v4l2_controls: Use\n\tDataValue and DataInfo","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","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":"Mon, 23 Sep 2019 11:06:14 -0000"}}]