[{"id":70,"web_url":"https://patchwork.libcamera.org/comment/70/","msgid":"<20181221162539.GN5597@w540>","date":"2018-12-21T16:25:39","subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n thanks for the patch\n\nOn Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:\n> Provide a helper V4L2 device object capable of interacting with the\n> V4L2 Linux Kernel APIs.\n>\n> A test suite is added at test/v4l2_device/ to utilise and validate the\n> code base.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> src/libcamera/include/v4l2_device.h | 36 +++++++\n> src/libcamera/meson.build | 2 +\n> src/libcamera/v4l2_device.cpp | 137 ++++++++++++++++++++++++++\n\nI would really split test and library code in 2 differen commits.\n\n> test/meson.build | 2 +\n> test/v4l2_device/double_open.cpp | 32 ++++++\n> test/v4l2_device/meson.build | 12 +++\n> test/v4l2_device/v4l2_device_test.cpp | 36 +++++++\n> test/v4l2_device/v4l2_device_test.h | 31 ++++++\n> 8 files changed, 288 insertions(+)\n> create mode 100644 src/libcamera/include/v4l2_device.h\n> create mode 100644 src/libcamera/v4l2_device.cpp\n> create mode 100644 test/v4l2_device/double_open.cpp\n> create mode 100644 test/v4l2_device/meson.build\n> create mode 100644 test/v4l2_device/v4l2_device_test.cpp\n> create mode 100644 test/v4l2_device/v4l2_device_test.h\n>\n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> new file mode 100644\n> index 000000000000..619d932d3c82\n> --- /dev/null\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * v4l2_device.h - V4L2 Device API Abstractions\n> + */\n> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +class V4L2Device\n> +{\n> +public:\n> +\tV4L2Device(const std::string &);\n> +\t~V4L2Device();\n> +\n> +\tbool isOpen();\n> +\tint open();\n> +\tvoid close();\n> +\n> +\tint queryCap();\n> +\n> +private:\n> +\tstd::string device_;\n> +\tint fd_;\n> +\tint capabilities_;\n> +};\n> +\n> +}\n\nnit:\n} /* namespace libcamera */\n\n> +\n> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f632eb5dd779..bbaf9a05ec2c 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,11 +1,13 @@\n> libcamera_sources = files([\n> 'log.cpp',\n> 'main.cpp',\n> + 'v4l2_device.cpp',\n> ])\n>\n> libcamera_headers = files([\n> 'include/log.h',\n> 'include/utils.h',\n> + 'include/v4l2_device.h',\n> ])\n>\n> libcamera_internal_includes = include_directories('include')\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> new file mode 100644\n> index 000000000000..eea2514f343c\n> --- /dev/null\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -0,0 +1,137 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * v4l2_device.cpp - V4L2 Device API\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"v4l2_device.h\"\n> +\n> +/**\n> + * \\file v4l2_device.cpp\n\nfile v4l2_device.h to document here what is defined in the header file\n\n> + * \\brief V4L2 Device API\n> + */\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class V4L2Device\n> + * \\brief V4L2Device object and API.\n> + *\n> + * The V4L2 Device API class models a single instance of a v4l2 device node.\n> + */\n> +\n> +/**\n> + * \\fn V4L2Device::V4L2Device(device)\n\nnit: parameter type?\n\n> + *\n> + * Default constructor for a V4L2Device object. The \\a device specifies a\n> + * string to representation of the device object.\n\nMy poor English skill do not allow me to say if this is a typo or it\nactually makes sense :)\n\n> + */\n> +V4L2Device::V4L2Device(const std::string &device)\n> +{\n> +\tdevice_ = device;\n> +\tfd_ = -1;\n> +\tcapabilities_ = 0;\n> +\n> +\tLOG(Debug) << \"V4L2Device Constructed for \" << device_;\n\nNot sure a debug printout is worth here\n\n> +}\n> +\n> +V4L2Device::~V4L2Device()\n> +{\n> +\tclose();\n> +\n> +\tLOG(Debug) << \"V4L2Device Destroyed\";\n\nditto\n\n> +}\n> +\n> +/**\n> + * \\fn V4L2Device::isOpen(())\n\none () in excess?\n\n> + *\n> + * Checks to see if we have successfully opened a v4l2 video device.\n> + */\n> +bool V4L2Device::isOpen()\n> +{\n> +\treturn (fd_ != -1);\n\nnit: no need to () braces\n> +}\n> +\n> +/**\n> + * \\fn V4L2Device::open()\n> + *\n> + * Opens a v4l2 device and queries properties from the device.\n> + */\n> +int V4L2Device::open()\n> +{\n> +\tint ret;\n> +\n> +\tif (isOpen()) {\n> +\t\tLOG(Error) << \"Device already open\";\n> +\t\treturn -EBADF;\n> +\t}\n> +\n> +\tfd_ = ::open(device_.c_str(), O_RDWR);\n> +\tif (fd_ < 0) {\n> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << device_\n> +\t\t\t << \" : \" << strerror(errno);\n> +\t\treturn -errno;\n> +\t}\n> +\n> +\tret = queryCap();\n\nI don't like it much either, but this might have been \"int ret = \"\n\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tif (!(capabilities_ & V4L2_CAP_STREAMING)) {\n\nI wonder if it wouldn't be better to return the capabilities from\nqueryCap, and assign them to the class member capabilities_ after this\ncheck here.\n\nLooking at the code I -knew- that the queryCap() function had modified\nthe capabilities_ and then you're inspecting them here, but it's all a\nbit implicit to me...\n\n> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn V4L2Device::close()\n> + *\n> + * Close a v4l2 device handle.\n> + */\n> +void V4L2Device::close()\n> +{\n> +\tif (fd_ < 0)\n> +\t\treturn;\n> +\n> +\t::close(fd_);\n> +\tfd_ = -1;\n> +}\n> +\n> +/**\n> + * \\fn V4L2Device::queryCap()\n> + *\n> + * Obtains the capabilities information from the V4L2 Device.\n> + */\n> +int V4L2Device::queryCap()\n> +{\n> +\tstruct v4l2_capability cap = {};\n> +\tint ret;\n> +\n> +\tif (!isOpen()) {\n> +\t\tLOG(Error) << \"Attempt to query unopened device\";\n> +\t\treturn -EBADF;\n> +\t}\n> +\n> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);\n> +\tif (ret < 0) {\n> +\t\tLOG(Error) << \"Failed to query device capabilities\";\n> +\t\treturn -1;\n\nIt would be useful to return the errno number and possibly print it\nout with strerror\n\n> +\t}\n> +\n> +\tcapabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS\n> +\t\t ? cap.device_caps : cap.capabilities;\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/test/meson.build b/test/meson.build\n> index 754527324c7d..2bc76c289eea 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -12,6 +12,8 @@ test_includes_internal = [\n> libcamera_internal_includes,\n> ]\n>\n> +subdir('v4l2_device')\n> +\n> public_tests = [\n> [ 'test_init', 'init.cpp' ],\n\nNot related to this, but weird spacing here\n\n> ]\n> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp\n> new file mode 100644\n> index 000000000000..1b7c7bfe14b8\n> --- /dev/null\n> +++ b/test/v4l2_device/double_open.cpp\n> @@ -0,0 +1,32 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * libcamera V4L2 API tests\n> + */\n> +\n> +#include \"v4l2_device_test.h\"\n> +\n> +class DoubleOpen : public V4L2DeviceTest\n> +{\n> +protected:\n> +\tint run()\n> +\t{\n> +\t\tint ret;\n> +\n> +\t\t/*\n> +\t\t * Expect failure: The device has already been opened by the\n> +\t\t * V4L2DeviceTest base class\n> +\t\t */\n> +\t\tret = dev->open();\n> +\t\tif (!ret) {\n> +\t\t\tfprintf(stderr, \"Double open erroneously succeeded\\n\");\n> +\t\t\tdev->close();\n> +\t\t\treturn TEST_FAIL;\n> +\t\t}\n> +\n> +\t\treturn TEST_PASS;\n> +\t}\n\nnit: < one indent level\n\n> +};\n\nI welcome testing, but do we really need tests to make sure a function\nfails when called twice? I'm sure you could have sketched it by\ncalling open twice in the V4L2DeviceTest class\n\n> +\n> +TEST_REGISTER(DoubleOpen);\n> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build\n> new file mode 100644\n> index 000000000000..41675a303498\n> --- /dev/null\n> +++ b/test/v4l2_device/meson.build\n> @@ -0,0 +1,12 @@\n> +# Tests are listed in order of complexity.\n> +# They are not alphabetically sorted.\n> +v4l2_device_tests = [\n> + [ 'double_open', 'double_open.cpp' ],\n\nAgain weird spacing, I'm now thinking this is intentional and I don't\nknow something\n\n> +]\n> +\n> +foreach t : v4l2_device_tests\n> + exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],\n> +\t\t link_with : test_libraries,\n> +\t\t include_directories : test_includes_internal)\n> + test(t[0], exe, suite: 'v4l2_device', is_parallel: false)\n> +endforeach\n> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n> new file mode 100644\n> index 000000000000..ae317a9519c5\n> --- /dev/null\n> +++ b/test/v4l2_device/v4l2_device_test.cpp\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * libcamera V4L2 API tests\n> + */\n> +\n> +#include \"v4l2_device_test.h\"\n> +\n> +using namespace libcamera;\n> +\n> +V4L2DeviceTest::V4L2DeviceTest()\n> +{\n> +\tdev = nullptr;\n> +}\n> +\n> +int V4L2DeviceTest::init()\n> +{\n> +\tconst char *device = \"/dev/video0\";\n> +\n> +\t/* Validate the device node exists. */\n> +\tif (!path_exists(device))\n> +\t\treturn TEST_SKIP;\n> +\n> +\tdev = new V4L2Device(device);\n> +\tif (!dev)\n\nYou should call cleanup here if you fail\nAh no, the Test class should call it if init() fails!\nAh no^2, your constructor cannot fail (good!). My previous comment\nstill hold though.\n\n> +\t\treturn TEST_FAIL;\n> +\n> +\treturn dev->open();\n> +}\n> +\n> +void V4L2DeviceTest::cleanup()\n> +{\n> +\tif (dev)\n> +\t\tdelete dev;\n> +};\n> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n> new file mode 100644\n> index 000000000000..4374ddc7e932\n> --- /dev/null\n> +++ b/test/v4l2_device/v4l2_device_test.h\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * vl42device_test.h - libcamera v4l2device test base class\n> + */\n> +\n> +#include \"test.h\"\n> +#include \"v4l2_device.h\"\n\nIncludes after the inclusion guards\n\n> +\n> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_\n> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_\n> +\n> +using namespace libcamera;\n> +\n> +class V4L2DeviceTest : public Test\n> +{\n> +public:\n> +\tV4L2DeviceTest();\n> +\tvirtual ~V4L2DeviceTest() {};\n> +\n> +protected:\n> +\tint init();\n> +\tvoid cleanup();\n> +\n> +\tvirtual int run() = 0;\n> +\n> +\tV4L2Device *dev;\n> +};\n> +\n> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */\n> --\n> 2.17.1\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 relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5C9DB60B0B\n\tfor ;\n\tFri, 21 Dec 2018 17:25:41 +0100 (CET)","from w540 (2-224-242-101.ip172.fastwebnet.it [2.224.242.101])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 94AAB1C0007;\n\tFri, 21 Dec 2018 16:25:40 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 21 Dec 2018 17:25:39 +0100","From":"jacopo mondi ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20181221162539.GN5597@w540>","References":"<20181221123724.27290-1-kieran.bingham@ideasonboard.com>\n\t<20181221123724.27290-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"n1q7a47AT2c4WQJe\"","Content-Disposition":"inline","In-Reply-To":"<20181221123724.27290-3-kieran.bingham@ideasonboard.com>","User-Agent":"Mutt/1.5.24 (2015-08-30)","Subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","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":"Fri, 21 Dec 2018 16:25:41 -0000"}},{"id":74,"web_url":"https://patchwork.libcamera.org/comment/74/","msgid":"","date":"2018-12-22T20:30:37","subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the review,\n\nOn 21/12/2018 16:25, jacopo mondi wrote:\n> Hi Kieran,\n> thanks for the patch\n> \n> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:\n>> Provide a helper V4L2 device object capable of interacting with the\n>> V4L2 Linux Kernel APIs.\n>>\n>> A test suite is added at test/v4l2_device/ to utilise and validate the\n>> code base.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> src/libcamera/include/v4l2_device.h | 36 +++++++\n>> src/libcamera/meson.build | 2 +\n>> src/libcamera/v4l2_device.cpp | 137 ++++++++++++++++++++++++++\n> \n> I would really split test and library code in 2 differen commits.\n\nI'm happy to go with group consensus here, but I felt that tests and\ncode could be in the same commit here, as the test directly affects the\ncode added.\n\nThis means that when doing rebase actions or such, I can do 'git rebase\n-i -x \"ninja; ninja test\"' and validate every commit as I rework things.\n\n(I have a script which does that for me)\n\n\n>> test/meson.build | 2 +\n>> test/v4l2_device/double_open.cpp | 32 ++++++\n>> test/v4l2_device/meson.build | 12 +++\n>> test/v4l2_device/v4l2_device_test.cpp | 36 +++++++\n>> test/v4l2_device/v4l2_device_test.h | 31 ++++++\n>> 8 files changed, 288 insertions(+)\n>> create mode 100644 src/libcamera/include/v4l2_device.h\n>> create mode 100644 src/libcamera/v4l2_device.cpp\n>> create mode 100644 test/v4l2_device/double_open.cpp\n>> create mode 100644 test/v4l2_device/meson.build\n>> create mode 100644 test/v4l2_device/v4l2_device_test.cpp\n>> create mode 100644 test/v4l2_device/v4l2_device_test.h\n>>\n>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>> new file mode 100644\n>> index 000000000000..619d932d3c82\n>> --- /dev/null\n>> +++ b/src/libcamera/include/v4l2_device.h\n>> @@ -0,0 +1,36 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n>> + *\n>> + * v4l2_device.h - V4L2 Device API Abstractions\n>> + */\n>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +class V4L2Device\n>> +{\n>> +public:\n>> +\tV4L2Device(const std::string &);\n>> +\t~V4L2Device();\n>> +\n>> +\tbool isOpen();\n>> +\tint open();\n>> +\tvoid close();\n>> +\n>> +\tint queryCap();\n>> +\n>> +private:\n>> +\tstd::string device_;\n>> +\tint fd_;\n>> +\tint capabilities_;\n>> +};\n>> +\n>> +}\n> \n> nit:\n> } /* namespace libcamera */\n> \n\nAh yes - that might be nice.\n\nHow did I miss this?\nDo we have this in our templates?\n\nHrm ... nope, this end comment isn't there.\n\nI've added it to the template document.\n\n\n>> +\n>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index f632eb5dd779..bbaf9a05ec2c 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -1,11 +1,13 @@\n>> libcamera_sources = files([\n>> 'log.cpp',\n>> 'main.cpp',\n>> + 'v4l2_device.cpp',\n>> ])\n>>\n>> libcamera_headers = files([\n>> 'include/log.h',\n>> 'include/utils.h',\n>> + 'include/v4l2_device.h',\n>> ])\n>>\n>> libcamera_internal_includes = include_directories('include')\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> new file mode 100644\n>> index 000000000000..eea2514f343c\n>> --- /dev/null\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -0,0 +1,137 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n>> + *\n>> + * v4l2_device.cpp - V4L2 Device API\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +#include \n>> +\n>> +#include \"log.h\"\n>> +#include \"v4l2_device.h\"\n>> +\n>> +/**\n>> + * \\file v4l2_device.cpp\n> \n> file v4l2_device.h to document here what is defined in the header file\n\nOh - So this is supposed to reference the header not declare the current\nfile. I guess that kind of makes sense :) Always pointless to state the\nname of the current file.\n\n> \n>> + * \\brief V4L2 Device API\n>> + */\n>> +namespace libcamera {\n>> +\n>> +/**\n>> + * \\class V4L2Device\n>> + * \\brief V4L2Device object and API.\n>> + *\n>> + * The V4L2 Device API class models a single instance of a v4l2 device node.\n>> + */\n>> +\n>> +/**\n>> + * \\fn V4L2Device::V4L2Device(device)\n> \n> nit: parameter type?\n\nI believe doxygen collects that automatically?\n\nIt generates this in the output:\n libcamera::V4L2Device::V4L2Device(const std::string & device)\n\n\n>> + *\n>> + * Default constructor for a V4L2Device object. The \\a device specifies a\n>> + * string to representation of the device object.\n> \n> My poor English skill do not allow me to say if this is a typo or it\n> actually makes sense :)\n\nAhem - there's certainly a typo there - good spot.\n\nIt's also not necessarily correct, and is too close from my Camera class\nconstructor text.\n\nI'll adapt this to :\n\n * Default constructor for a V4L2Device object. The \\a device specifies\n * the path to the video device node.\n\n> \n>> + */\n>> +V4L2Device::V4L2Device(const std::string &device)\n>> +{\n>> +\tdevice_ = device;\n>> +\tfd_ = -1;\n>> +\tcapabilities_ = 0;\n>> +\n>> +\tLOG(Debug) << \"V4L2Device Constructed for \" << device_;\n> \n> Not sure a debug printout is worth here\n\nCertainly not for long... they're just here for early dev.\n\nI can just drop these - they're not useful.\n\n> \n>> +}\n>> +\n>> +V4L2Device::~V4L2Device()\n>> +{\n>> +\tclose();\n>> +\n>> +\tLOG(Debug) << \"V4L2Device Destroyed\";\n> \n> ditto\n> \n>> +}\n>> +\n>> +/**\n>> + * \\fn V4L2Device::isOpen(())\n> \n> one () in excess?\n\nNot sure how that happened?\n\n> \n>> + *\n>> + * Checks to see if we have successfully opened a v4l2 video device.\n>> + */\n>> +bool V4L2Device::isOpen()\n>> +{\n>> +\treturn (fd_ != -1);\n> \n> nit: no need to () braces\n\nCan I keep them ?\n\nReturning more than a simple variable or statement 'openly' feels\nhorrible to me ...\n\n\n>> +}\n>> +\n>> +/**\n>> + * \\fn V4L2Device::open()\n>> + *\n>> + * Opens a v4l2 device and queries properties from the device.\n>> + */\n>> +int V4L2Device::open()\n>> +{\n>> +\tint ret;\n>> +\n>> +\tif (isOpen()) {\n>> +\t\tLOG(Error) << \"Device already open\";\n>> +\t\treturn -EBADF;\n>> +\t}\n>> +\n>> +\tfd_ = ::open(device_.c_str(), O_RDWR);\n>> +\tif (fd_ < 0) {\n>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << device_\n>> +\t\t\t << \" : \" << strerror(errno);\n>> +\t\treturn -errno;\n>> +\t}\n>> +\n>> +\tret = queryCap();\n> \n> I don't like it much either, but this might have been \"int ret = \"\n\nHrm ... that certainly is late instantiation.\n\nIs that what we're going for ?\n\n\n> \n>> +\tif (ret)\n>> +\t\treturn ret;\n>> +\n>> +\tif (!(capabilities_ & V4L2_CAP_STREAMING)) {\n> \n> I wonder if it wouldn't be better to return the capabilities from\n> queryCap, and assign them to the class member capabilities_ after this\n> check here.\n> \n> Looking at the code I -knew- that the queryCap() function had modified\n> the capabilities_ and then you're inspecting them here, but it's all a\n> bit implicit to me...\n\nHow will we determine if the ioctl call succeeded?\n (while avoiding exceptions?)\n\nCapabilities flag can return any free-form int value. (ok - so it's\nrestricted to the flags available in V4L2 ... but... that's very\nfunction specific)\n\n\n\n>> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\fn V4L2Device::close()\n>> + *\n>> + * Close a v4l2 device handle.\n>> + */\n>> +void V4L2Device::close()\n>> +{\n>> +\tif (fd_ < 0)\n>> +\t\treturn;\n>> +\n>> +\t::close(fd_);\n>> +\tfd_ = -1;\n>> +}\n>> +\n>> +/**\n>> + * \\fn V4L2Device::queryCap()\n>> + *\n>> + * Obtains the capabilities information from the V4L2 Device.\n>> + */\n>> +int V4L2Device::queryCap()\n>> +{\n>> +\tstruct v4l2_capability cap = {};\n>> +\tint ret;\n>> +\n>> +\tif (!isOpen()) {\n>> +\t\tLOG(Error) << \"Attempt to query unopened device\";\n>> +\t\treturn -EBADF;\n>> +\t}\n>> +\n>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);\n>> +\tif (ret < 0) {\n>> +\t\tLOG(Error) << \"Failed to query device capabilities\";\n>> +\t\treturn -1;\n> \n> It would be useful to return the errno number and possibly print it\n> out with strerror\n\nGood point!\n\nShould this be -errno? Or just return ret?\n\n\nThe more I think about it - the more I'd like to wrap the ioctl call in\nthe class (or more likely a Device base class) which will handle the\nerror checks...\n\nThat could then be a parent class for your MediaDevice object too, and\nthe expected V4L2SubDevice ...\n\nWhat are your thoughts? Over-abstracting? or useful ...\n\n\n> \n>> +\t}\n>> +\n>> +\tcapabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS\n>> +\t\t ? cap.device_caps : cap.capabilities;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/test/meson.build b/test/meson.build\n>> index 754527324c7d..2bc76c289eea 100644\n>> --- a/test/meson.build\n>> +++ b/test/meson.build\n>> @@ -12,6 +12,8 @@ test_includes_internal = [\n>> libcamera_internal_includes,\n>> ]\n>>\n>> +subdir('v4l2_device')\n>> +\n>> public_tests = [\n>> [ 'test_init', 'init.cpp' ],\n> \n> Not related to this, but weird spacing here\n\nThis was intentional ... pre-allocation of table spacing. (The table\nwill have multiple entries...)\n\n> \n>> ]\n>> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp\n>> new file mode 100644\n>> index 000000000000..1b7c7bfe14b8\n>> --- /dev/null\n>> +++ b/test/v4l2_device/double_open.cpp\n>> @@ -0,0 +1,32 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n>> + *\n>> + * libcamera V4L2 API tests\n>> + */\n>> +\n>> +#include \"v4l2_device_test.h\"\n>> +\n>> +class DoubleOpen : public V4L2DeviceTest\n>> +{\n>> +protected:\n>> +\tint run()\n>> +\t{\n>> +\t\tint ret;\n>> +\n>> +\t\t/*\n>> +\t\t * Expect failure: The device has already been opened by the\n>> +\t\t * V4L2DeviceTest base class\n>> +\t\t */\n>> +\t\tret = dev->open();\n>> +\t\tif (!ret) {\n>> +\t\t\tfprintf(stderr, \"Double open erroneously succeeded\\n\");\n>> +\t\t\tdev->close();\n>> +\t\t\treturn TEST_FAIL;\n>> +\t\t}\n>> +\n>> +\t\treturn TEST_PASS;\n>> +\t}\n> \n> nit: < one indent level\n\nOh -- for all class functions?\n\n\n>> +};\n> \n> I welcome testing, but do we really need tests to make sure a function\n> fails when called twice? \n\nYes - I felt this was important.\n\nIt's there because once I added the open() and close() calls - it\nprovided a way to leak fd's if you call open(), open().\n\nThe first fd would get silently dropped ...\n\nSo I added a test to call open twice, then I added the isOpen() checks.\n\nIn other words - the fix for the leak came about directly because I\nadded the double_open test...\n\n\nAnother reason we I think we should base class the common device\noperations (then this test would only apply to the Device object unit\ntests anyway.)\n\n\n> I'm sure you could have sketched it by\n> calling open twice in the V4L2DeviceTest class\n\nNo - the V4L2DeviceTest class is the base class common to all further\ntests for the V4L2Device Object.\n\nI have other patches which add further tests using the V4L2DeviceTest\nbase class.\n\n>> +\n>> +TEST_REGISTER(DoubleOpen);\n>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build\n>> new file mode 100644\n>> index 000000000000..41675a303498\n>> --- /dev/null\n>> +++ b/test/v4l2_device/meson.build\n>> @@ -0,0 +1,12 @@\n>> +# Tests are listed in order of complexity.\n>> +# They are not alphabetically sorted.\n>> +v4l2_device_tests = [\n>> + [ 'double_open', 'double_open.cpp' ],\n> \n> Again weird spacing, I'm now thinking this is intentional and I don't\n> know something\n\nTable pre-allocation for multiple tests.\n\nThis isn't my only test, and I'd rather not have to reformat the table\nif I add tests with longer filenames.\n\n> \n>> +]\n>> +\n>> +foreach t : v4l2_device_tests\n>> + exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],\n>> +\t\t link_with : test_libraries,\n>> +\t\t include_directories : test_includes_internal)\n>> + test(t[0], exe, suite: 'v4l2_device', is_parallel: false)\n>> +endforeach\n>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n>> new file mode 100644\n>> index 000000000000..ae317a9519c5\n>> --- /dev/null\n>> +++ b/test/v4l2_device/v4l2_device_test.cpp\n>> @@ -0,0 +1,36 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n>> + *\n>> + * libcamera V4L2 API tests\n>> + */\n>> +\n>> +#include \"v4l2_device_test.h\"\n>> +\n>> +using namespace libcamera;\n>> +\n>> +V4L2DeviceTest::V4L2DeviceTest()\n>> +{\n>> +\tdev = nullptr;\n>> +}\n>> +\n>> +int V4L2DeviceTest::init()\n>> +{\n>> +\tconst char *device = \"/dev/video0\";\n>> +\n>> +\t/* Validate the device node exists. */\n>> +\tif (!path_exists(device))\n>> +\t\treturn TEST_SKIP;\n>> +\n>> +\tdev = new V4L2Device(device);\n>> +\tif (!dev)\n> \n> You should call cleanup here if you fail\n> Ah no, the Test class should call it if init() fails!\n> Ah no^2, your constructor cannot fail (good!). My previous comment\n> still hold though.\n> \n\nShould I instead call cleanup() from my destructor ?\n\n\n\n>> +\t\treturn TEST_FAIL;\n>> +\n>> +\treturn dev->open();\n>> +}\n>> +\n>> +void V4L2DeviceTest::cleanup()\n>> +{\n>> +\tif (dev)\n>> +\t\tdelete dev;\n>> +};\n>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n>> new file mode 100644\n>> index 000000000000..4374ddc7e932\n>> --- /dev/null\n>> +++ b/test/v4l2_device/v4l2_device_test.h\n>> @@ -0,0 +1,31 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n>> + *\n>> + * vl42device_test.h - libcamera v4l2device test base class\n>> + */\n>> +\n>> +#include \"test.h\"\n>> +#include \"v4l2_device.h\"\n> \n> Includes after the inclusion guards\n\nAck.\n\n\n>> +\n>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_\n>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_\n>> +\n>> +using namespace libcamera;\n>> +\n>> +class V4L2DeviceTest : public Test\n>> +{\n>> +public:\n>> +\tV4L2DeviceTest();\n>> +\tvirtual ~V4L2DeviceTest() {};\n>> +\n>> +protected:\n>> +\tint init();\n>> +\tvoid cleanup();\n>> +\n>> +\tvirtual int run() = 0;\n>> +\n>> +\tV4L2Device *dev;\n>> +};\n>> +\n>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */\n>> --\n>> 2.17.1\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 perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E666F60B0C\n\tfor ;\n\tSat, 22 Dec 2018 21:30:41 +0100 (CET)","from [192.168.0.21]\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 BC4A059;\n\tSat, 22 Dec 2018 21:30:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1545510641;\n\tbh=YxeyeLsnQFog3mDgVeOoTdMgoKQQ5X/1kbADneOtpas=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=jaIX2wum1STuU7covbOWNRj9Pxpu5D0+KTmWnKwcKLlYXxBl+Dlzxtoi+IlsHYhZO\n\t00RzezNmQWgb5Uf2cJMoRk1JJQ8GIKaPBZ4eO4eNkQl4Pcikq+Kg06w7qC6lqM8OI8\n\tzXa/3lLJhHKWuhmvXDnoTosglTQZk6+wEQfYe+HE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"jacopo mondi ","Cc":"LibCamera Devel ","References":"<20181221123724.27290-1-kieran.bingham@ideasonboard.com>\n\t<20181221123724.27290-3-kieran.bingham@ideasonboard.com>\n\t<20181221162539.GN5597@w540>","From":"Kieran Bingham ","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Sat, 22 Dec 2018 20:30:37 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20181221162539.GN5597@w540>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","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":"Sat, 22 Dec 2018 20:30:42 -0000"}},{"id":75,"web_url":"https://patchwork.libcamera.org/comment/75/","msgid":"<20181223205409.GB13765@archlinux>","date":"2018-12-23T20:54:09","subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> Thanks for the review,\n>\n> On 21/12/2018 16:25, jacopo mondi wrote:\n> > Hi Kieran,\n> > thanks for the patch\n> >\n> > On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:\n> >> Provide a helper V4L2 device object capable of interacting with the\n> >> V4L2 Linux Kernel APIs.\n> >>\n> >> A test suite is added at test/v4l2_device/ to utilise and validate the\n> >> code base.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> ---\n> >> src/libcamera/include/v4l2_device.h | 36 +++++++\n> >> src/libcamera/meson.build | 2 +\n> >> src/libcamera/v4l2_device.cpp | 137 ++++++++++++++++++++++++++\n> >\n> > I would really split test and library code in 2 differen commits.\n>\n> I'm happy to go with group consensus here, but I felt that tests and\n> code could be in the same commit here, as the test directly affects the\n> code added.\n>\n> This means that when doing rebase actions or such, I can do 'git rebase\n> -i -x \"ninja; ninja test\"' and validate every commit as I rework things.\n>\n> (I have a script which does that for me)\n>\n\nI see, and I feel in this specific case it's not a big deal, but as a\ngeneral rule having tests -in the same commit- that adds a specific\nfeature is not something that holds in the general case.\n\nTests should accompany new features, and if a patch series adds\nanything that is worth being tested, just make sure they get merged\ntogether.\n\nAs an example, Niklas' last series is a 12 patches one, with one test\nat the end of the series. Your one might be a 3 patches one, with a\ntest at the end. Or am I missing something of how you would like to\nuse testing?\n\nAnyway, that's not a big deal at all in this case :)\n\n>\n> >> test/meson.build | 2 +\n> >> test/v4l2_device/double_open.cpp | 32 ++++++\n> >> test/v4l2_device/meson.build | 12 +++\n> >> test/v4l2_device/v4l2_device_test.cpp | 36 +++++++\n> >> test/v4l2_device/v4l2_device_test.h | 31 ++++++\n> >> 8 files changed, 288 insertions(+)\n> >> create mode 100644 src/libcamera/include/v4l2_device.h\n> >> create mode 100644 src/libcamera/v4l2_device.cpp\n> >> create mode 100644 test/v4l2_device/double_open.cpp\n> >> create mode 100644 test/v4l2_device/meson.build\n> >> create mode 100644 test/v4l2_device/v4l2_device_test.cpp\n> >> create mode 100644 test/v4l2_device/v4l2_device_test.h\n> >>\n> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> >> new file mode 100644\n> >> index 000000000000..619d932d3c82\n> >> --- /dev/null\n> >> +++ b/src/libcamera/include/v4l2_device.h\n> >> @@ -0,0 +1,36 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2018, Google Inc.\n> >> + *\n> >> + * v4l2_device.h - V4L2 Device API Abstractions\n> >> + */\n> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> >> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +class V4L2Device\n> >> +{\n> >> +public:\n> >> +\tV4L2Device(const std::string &);\n\nnit: I've been suggested to use parameter's names in function\ndeclarations in header files. I know this is very minor stuff, but at\nthe very beginning of the development is worth being quite picky on\nthis minor detail to establish a consistent code base. So please bear\nwith me here a little more :)\n\n> >> +\t~V4L2Device();\n> >> +\n> >> +\tbool isOpen();\n> >> +\tint open();\n> >> +\tvoid close();\n> >> +\n> >> +\tint queryCap();\n> >> +\n> >> +private:\n> >> +\tstd::string device_;\n> >> +\tint fd_;\n> >> +\tint capabilities_;\n\nOk, I'm saying it: \"What about reverse-xmas-tree order for variable\ndeclarations?\"\n\nHere, let's the bike-shedding begin\n\n> >> +};\n> >> +\n> >> +}\n> >\n> > nit:\n> > } /* namespace libcamera */\n> >\n>\n> Ah yes - that might be nice.\n>\n> How did I miss this?\n> Do we have this in our templates?\n>\n> Hrm ... nope, this end comment isn't there.\n>\n> I've added it to the template document.\n\nOh thanks, I missed it was not there, but I think that rule comes from\nthe C++ style guide cited in our coding style document.\n\n>\n>\n> >> +\n> >> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index f632eb5dd779..bbaf9a05ec2c 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -1,11 +1,13 @@\n> >> libcamera_sources = files([\n> >> 'log.cpp',\n> >> 'main.cpp',\n> >> + 'v4l2_device.cpp',\n> >> ])\n> >>\n> >> libcamera_headers = files([\n> >> 'include/log.h',\n> >> 'include/utils.h',\n> >> + 'include/v4l2_device.h',\n> >> ])\n> >>\n> >> libcamera_internal_includes = include_directories('include')\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> new file mode 100644\n> >> index 000000000000..eea2514f343c\n> >> --- /dev/null\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -0,0 +1,137 @@\n> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >> +/*\n> >> + * Copyright (C) 2018, Google Inc.\n> >> + *\n> >> + * v4l2_device.cpp - V4L2 Device API\n> >> + */\n> >> +\n> >> +#include \n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +#include \"log.h\"\n> >> +#include \"v4l2_device.h\"\n> >> +\n> >> +/**\n> >> + * \\file v4l2_device.cpp\n> >\n> > file v4l2_device.h to document here what is defined in the header file\n>\n> Oh - So this is supposed to reference the header not declare the current\n> file. I guess that kind of makes sense :) Always pointless to state the\n> name of the current file.\n>\n\nIt allows documenting the code in the cpp file where the implementation is,\nbut to refer to definitions in the header file. I know, confusing.\n\n> >\n> >> + * \\brief V4L2 Device API\n> >> + */\n> >> +namespace libcamera {\n> >> +\n> >> +/**\n> >> + * \\class V4L2Device\n> >> + * \\brief V4L2Device object and API.\n> >> + *\n> >> + * The V4L2 Device API class models a single instance of a v4l2 device node.\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\fn V4L2Device::V4L2Device(device)\n> >\n> > nit: parameter type?\n>\n> I believe doxygen collects that automatically?\n>\n> It generates this in the output:\n> libcamera::V4L2Device::V4L2Device(const std::string & device)\n\nOh really? That's nice, I did it in all functions and I could have\navoided that...\n>\n>\n> >> + *\n> >> + * Default constructor for a V4L2Device object. The \\a device specifies a\n> >> + * string to representation of the device object.\n> >\n> > My poor English skill do not allow me to say if this is a typo or it\n> > actually makes sense :)\n>\n> Ahem - there's certainly a typo there - good spot.\n>\n> It's also not necessarily correct, and is too close from my Camera class\n> constructor text.\n>\n> I'll adapt this to :\n>\n> * Default constructor for a V4L2Device object. The \\a device specifies\n> * the path to the video device node.\n>\n> >\n> >> + */\n> >> +V4L2Device::V4L2Device(const std::string &device)\n> >> +{\n> >> +\tdevice_ = device;\n> >> +\tfd_ = -1;\n> >> +\tcapabilities_ = 0;\n> >> +\n> >> +\tLOG(Debug) << \"V4L2Device Constructed for \" << device_;\n> >\n> > Not sure a debug printout is worth here\n>\n> Certainly not for long... they're just here for early dev.\n>\n> I can just drop these - they're not useful.\n>\n\nI think that would be better\n\n> >\n> >> +}\n> >> +\n> >> +V4L2Device::~V4L2Device()\n> >> +{\n> >> +\tclose();\n> >> +\n> >> +\tLOG(Debug) << \"V4L2Device Destroyed\";\n> >\n> > ditto\n> >\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn V4L2Device::isOpen(())\n> >\n> > one () in excess?\n>\n> Not sure how that happened?\n>\n> >\n> >> + *\n> >> + * Checks to see if we have successfully opened a v4l2 video device.\n> >> + */\n> >> +bool V4L2Device::isOpen()\n> >> +{\n> >> +\treturn (fd_ != -1);\n> >\n> > nit: no need to () braces\n>\n> Can I keep them ?\n>\n\nSure!\n\n> Returning more than a simple variable or statement 'openly' feels\n> horrible to me ...\n\n:)\n\n>\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn V4L2Device::open()\n> >> + *\n> >> + * Opens a v4l2 device and queries properties from the device.\n> >> + */\n> >> +int V4L2Device::open()\n> >> +{\n> >> +\tint ret;\n> >> +\n> >> +\tif (isOpen()) {\n> >> +\t\tLOG(Error) << \"Device already open\";\n> >> +\t\treturn -EBADF;\n> >> +\t}\n> >> +\n> >> +\tfd_ = ::open(device_.c_str(), O_RDWR);\n> >> +\tif (fd_ < 0) {\n> >> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << device_\n> >> +\t\t\t << \" : \" << strerror(errno);\n> >> +\t\treturn -errno;\n> >> +\t}\n> >> +\n> >> +\tret = queryCap();\n> >\n> > I don't like it much either, but this might have been \"int ret = \"\n>\n> Hrm ... that certainly is late instantiation.\n>\n> Is that what we're going for ?\n\nThat's how I would interpret\nhttps://google.github.io/styleguide/cppguide.html#Local_Variables\n\nIt could be discussed though. Again, it is only worth discussing this\nminor things as we're at the beginning and trying to establish a well\nconsistent code base\n\n>\n>\n> >\n> >> +\tif (ret)\n> >> +\t\treturn ret;\n> >> +\n> >> +\tif (!(capabilities_ & V4L2_CAP_STREAMING)) {\n> >\n> > I wonder if it wouldn't be better to return the capabilities from\n> > queryCap, and assign them to the class member capabilities_ after this\n> > check here.\n> >\n> > Looking at the code I -knew- that the queryCap() function had modified\n> > the capabilities_ and then you're inspecting them here, but it's all a\n> > bit implicit to me...\n>\n> How will we determine if the ioctl call succeeded?\n> (while avoiding exceptions?)\n\nWell, 'queryCap()' would return -1 (or what more appropriate, like the\nerrno value)\n\n>\n> Capabilities flag can return any free-form int value. (ok - so it's\n> restricted to the flags available in V4L2 ... but... that's very\n> function specific)\n\nCan it return values < 0 ?\n\n>\n>\n>\n> >> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n> >> +\t\treturn -EINVAL;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn V4L2Device::close()\n> >> + *\n> >> + * Close a v4l2 device handle.\n> >> + */\n> >> +void V4L2Device::close()\n> >> +{\n> >> +\tif (fd_ < 0)\n> >> +\t\treturn;\n> >> +\n> >> +\t::close(fd_);\n> >> +\tfd_ = -1;\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\fn V4L2Device::queryCap()\n> >> + *\n> >> + * Obtains the capabilities information from the V4L2 Device.\n> >> + */\n> >> +int V4L2Device::queryCap()\n> >> +{\n> >> +\tstruct v4l2_capability cap = {};\n> >> +\tint ret;\n> >> +\n> >> +\tif (!isOpen()) {\n> >> +\t\tLOG(Error) << \"Attempt to query unopened device\";\n> >> +\t\treturn -EBADF;\n> >> +\t}\n> >> +\n> >> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);\n> >> +\tif (ret < 0) {\n> >> +\t\tLOG(Error) << \"Failed to query device capabilities\";\n> >> +\t\treturn -1;\n> >\n> > It would be useful to return the errno number and possibly print it\n> > out with strerror\n>\n> Good point!\n>\n> Should this be -errno? Or just return ret?\n\nReturning 'ret' is equivalent to return -1. -errno is the appropriate\none.\n\n>\n>\n> The more I think about it - the more I'd like to wrap the ioctl call in\n> the class (or more likely a Device base class) which will handle the\n> error checks...\n>\n> That could then be a parent class for your MediaDevice object too, and\n> the expected V4L2SubDevice ...\n>\n> What are your thoughts? Over-abstracting? or useful ...\n\nNo, I think it is useful. Actually, more than the MediaDevice itself\n(which I expect to instantiate V4L2Devices and not extend it) I think\nit might be worth implementing a V4L2Object that provides protected\nfunctions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device\nand V4L2Subdevice sub-class it.\n\nI'm not sure right now how I would model that, if the base class should\ndo things like \"setFormat()\" or either provides an \"ioctl()\" function and\nhave sub-classes implement the \"setFormat()\" logic. This boils down to\nhow different that would be the implementation between a device and a\nsubdevice. What do you think?\n\n>\n>\n> >\n> >> +\t}\n> >> +\n> >> +\tcapabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS\n> >> +\t\t ? cap.device_caps : cap.capabilities;\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/test/meson.build b/test/meson.build\n> >> index 754527324c7d..2bc76c289eea 100644\n> >> --- a/test/meson.build\n> >> +++ b/test/meson.build\n> >> @@ -12,6 +12,8 @@ test_includes_internal = [\n> >> libcamera_internal_includes,\n> >> ]\n> >>\n> >> +subdir('v4l2_device')\n> >> +\n> >> public_tests = [\n> >> [ 'test_init', 'init.cpp' ],\n> >\n> > Not related to this, but weird spacing here\n>\n> This was intentional ... pre-allocation of table spacing. (The table\n> will have multiple entries...)\n\nWhy would you pre-allocate this?\n>\n> >\n> >> ]\n> >> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp\n> >> new file mode 100644\n> >> index 000000000000..1b7c7bfe14b8\n> >> --- /dev/null\n> >> +++ b/test/v4l2_device/double_open.cpp\n> >> @@ -0,0 +1,32 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2018, Google Inc.\n> >> + *\n> >> + * libcamera V4L2 API tests\n> >> + */\n> >> +\n> >> +#include \"v4l2_device_test.h\"\n> >> +\n> >> +class DoubleOpen : public V4L2DeviceTest\n> >> +{\n> >> +protected:\n> >> +\tint run()\n> >> +\t{\n> >> +\t\tint ret;\n> >> +\n> >> +\t\t/*\n> >> +\t\t * Expect failure: The device has already been opened by the\n> >> +\t\t * V4L2DeviceTest base class\n> >> +\t\t */\n> >> +\t\tret = dev->open();\n> >> +\t\tif (!ret) {\n> >> +\t\t\tfprintf(stderr, \"Double open erroneously succeeded\\n\");\n> >> +\t\t\tdev->close();\n> >> +\t\t\treturn TEST_FAIL;\n> >> +\t\t}\n> >> +\n> >> +\t\treturn TEST_PASS;\n> >> +\t}\n> >\n> > nit: < one indent level\n>\n> Oh -- for all class functions?\n\nOh damn, I got confused by having the full implementation here.\n\nFor 'big' functions like this one, we don't want to have inlined by\nimplementing them in the definition. Now, I'm struggling to find a\nfinal answer to the question \"Are functions implemented in the class\ndefinition automatically inlined?\" I bet the compiler is smart enough\nto find out if it makes sense to inline or not a function even when\nits implementation is in the header file, but in general you could define\nthe class, and implement the function outside of the definition, even\nif in the same cpp file.\n\n>\n>\n> >> +};\n> >\n> > I welcome testing, but do we really need tests to make sure a function\n> > fails when called twice?\n>\n> Yes - I felt this was important.\n>\n> It's there because once I added the open() and close() calls - it\n> provided a way to leak fd's if you call open(), open().\n>\n> The first fd would get silently dropped ...\n>\n> So I added a test to call open twice, then I added the isOpen() checks.\n>\n> In other words - the fix for the leak came about directly because I\n> added the double_open test...\n>\n\nFine then, thanks for the explanation.\n\n>\n> Another reason we I think we should base class the common device\n> operations (then this test would only apply to the Device object unit\n> tests anyway.)\n>\n>\n> > I'm sure you could have sketched it by\n> > calling open twice in the V4L2DeviceTest class\n>\n> No - the V4L2DeviceTest class is the base class common to all further\n> tests for the V4L2Device Object.\n>\n> I have other patches which add further tests using the V4L2DeviceTest\n> base class.\n>\n> >> +\n> >> +TEST_REGISTER(DoubleOpen);\n> >> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build\n> >> new file mode 100644\n> >> index 000000000000..41675a303498\n> >> --- /dev/null\n> >> +++ b/test/v4l2_device/meson.build\n> >> @@ -0,0 +1,12 @@\n> >> +# Tests are listed in order of complexity.\n> >> +# They are not alphabetically sorted.\n> >> +v4l2_device_tests = [\n> >> + [ 'double_open', 'double_open.cpp' ],\n> >\n> > Again weird spacing, I'm now thinking this is intentional and I don't\n> > know something\n>\n> Table pre-allocation for multiple tests.\n>\n> This isn't my only test, and I'd rather not have to reformat the table\n> if I add tests with longer filenames.\n>\n> >\n> >> +]\n> >> +\n> >> +foreach t : v4l2_device_tests\n> >> + exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],\n> >> +\t\t link_with : test_libraries,\n> >> +\t\t include_directories : test_includes_internal)\n> >> + test(t[0], exe, suite: 'v4l2_device', is_parallel: false)\n> >> +endforeach\n> >> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n> >> new file mode 100644\n> >> index 000000000000..ae317a9519c5\n> >> --- /dev/null\n> >> +++ b/test/v4l2_device/v4l2_device_test.cpp\n> >> @@ -0,0 +1,36 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2018, Google Inc.\n> >> + *\n> >> + * libcamera V4L2 API tests\n> >> + */\n> >> +\n> >> +#include \"v4l2_device_test.h\"\n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +V4L2DeviceTest::V4L2DeviceTest()\n> >> +{\n> >> +\tdev = nullptr;\n> >> +}\n> >> +\n> >> +int V4L2DeviceTest::init()\n> >> +{\n> >> +\tconst char *device = \"/dev/video0\";\n> >> +\n> >> +\t/* Validate the device node exists. */\n> >> +\tif (!path_exists(device))\n> >> +\t\treturn TEST_SKIP;\n> >> +\n> >> +\tdev = new V4L2Device(device);\n> >> +\tif (!dev)\n> >\n> > You should call cleanup here if you fail\n> > Ah no, the Test class should call it if init() fails!\n> > Ah no^2, your constructor cannot fail (good!). My previous comment\n> > still hold though.\n> >\n>\n> Should I instead call cleanup() from my destructor ?\n\nMy point was that the object would not get destroyed here. Because the\ntest base class does not call cleanup() after init fails\nand you don't not call it either. But I was confused actually, as the\nbase test class does this\n\n +#define TEST_REGISTER(klass) \\\n +int main(int argc, char *argv[]) \\\n +{ \\\n + return klass().execute(); \\\n +}\n\nAnd this creates and destroys the object, which calls your destructor,\nso I -guess- this is fine actually.\nNot calling 'cleaup()' after a failed init is being discussed now\nelsewhere.\n\nAnyway, your constructor cannot fail, so you could remove that\ncheck...\n\n>\n>\n>\n> >> +\t\treturn TEST_FAIL;\n> >> +\n> >> +\treturn dev->open();\n> >> +}\n> >> +\n> >> +void V4L2DeviceTest::cleanup()\n> >> +{\n> >> +\tif (dev)\n> >> +\t\tdelete dev;\n> >> +};\n> >> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n> >> new file mode 100644\n> >> index 000000000000..4374ddc7e932\n> >> --- /dev/null\n> >> +++ b/test/v4l2_device/v4l2_device_test.h\n> >> @@ -0,0 +1,31 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2018, Google Inc.\n> >> + *\n> >> + * vl42device_test.h - libcamera v4l2device test base class\n> >> + */\n> >> +\n> >> +#include \"test.h\"\n> >> +#include \"v4l2_device.h\"\n> >\n> > Includes after the inclusion guards\n>\n> Ack.\n>\n>\n> >> +\n> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_\n> >> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_\n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +class V4L2DeviceTest : public Test\n> >> +{\n> >> +public:\n> >> +\tV4L2DeviceTest();\n> >> +\tvirtual ~V4L2DeviceTest() {};\n> >> +\n> >> +protected:\n> >> +\tint init();\n> >> +\tvoid cleanup();\n> >> +\n> >> +\tvirtual int run() = 0;\n> >> +\n> >> +\tV4L2Device *dev;\n> >> +};\n> >> +\n> >> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */\n> >> --\n> >> 2.17.1\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> --\n> Kieran","headers":{"Return-Path":"","Received":["from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D9BB60B21\n\tfor ;\n\tSun, 23 Dec 2018 21:54:12 +0100 (CET)","from archlinux (host54-51-dynamic.16-87-r.retail.telecomitalia.it\n\t[87.16.51.54]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id D70B1240005;\n\tSun, 23 Dec 2018 20:54:10 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Sun, 23 Dec 2018 21:54:09 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"LibCamera Devel ","Message-ID":"<20181223205409.GB13765@archlinux>","References":"<20181221123724.27290-1-kieran.bingham@ideasonboard.com>\n\t<20181221123724.27290-3-kieran.bingham@ideasonboard.com>\n\t<20181221162539.GN5597@w540>\n\t","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"z6Eq5LdranGa6ru8\"","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","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":"Sun, 23 Dec 2018 20:54:12 -0000"}},{"id":290,"web_url":"https://patchwork.libcamera.org/comment/290/","msgid":"<5695091.PZBZBohWtT@avalon>","date":"2019-01-11T17:47:32","subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Sunday, 23 December 2018 22:54:09 EET Jacopo Mondi wrote:\n> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:\n> > On 21/12/2018 16:25, jacopo mondi wrote:\n> >> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:\n> >>> Provide a helper V4L2 device object capable of interacting with the\n> >>> V4L2 Linux Kernel APIs.\n> >>> \n> >>> A test suite is added at test/v4l2_device/ to utilise and validate the\n> >>> code base.\n> >>> \n> >>> Signed-off-by: Kieran Bingham \n> >>> ---\n> >>> \n> >>> src/libcamera/include/v4l2_device.h | 36 +++++++\n> >>> src/libcamera/meson.build | 2 +\n> >>> src/libcamera/v4l2_device.cpp | 137 ++++++++++++++++++++++++++\n> >> \n> >> I would really split test and library code in 2 differen commits.\n> > \n> > I'm happy to go with group consensus here, but I felt that tests and\n> > code could be in the same commit here, as the test directly affects the\n> > code added.\n> > \n> > This means that when doing rebase actions or such, I can do 'git rebase\n> > -i -x \"ninja; ninja test\"' and validate every commit as I rework things.\n> > \n> > (I have a script which does that for me)\n> \n> I see, and I feel in this specific case it's not a big deal, but as a\n> general rule having tests -in the same commit- that adds a specific\n> feature is not something that holds in the general case.\n> \n> Tests should accompany new features, and if a patch series adds\n> anything that is worth being tested, just make sure they get merged\n> together.\n> \n> As an example, Niklas' last series is a 12 patches one, with one test\n> at the end of the series. Your one might be a 3 patches one, with a\n> test at the end. Or am I missing something of how you would like to\n> use testing?\n> \n> Anyway, that's not a big deal at all in this case :)\n\nI'm also slightly more in favour of separating code and tests in different \npatches. It's indeed not a big deal in this case, but when the patches and \nseries grow bigger, bundling everything together creates too large patches.\n\n> >>> test/meson.build | 2 +\n> >>> test/v4l2_device/double_open.cpp | 32 ++++++\n> >>> test/v4l2_device/meson.build | 12 +++\n> >>> test/v4l2_device/v4l2_device_test.cpp | 36 +++++++\n> >>> test/v4l2_device/v4l2_device_test.h | 31 ++++++\n> >>> 8 files changed, 288 insertions(+)\n> >>> create mode 100644 src/libcamera/include/v4l2_device.h\n> >>> create mode 100644 src/libcamera/v4l2_device.cpp\n> >>> create mode 100644 test/v4l2_device/double_open.cpp\n> >>> create mode 100644 test/v4l2_device/meson.build\n> >>> create mode 100644 test/v4l2_device/v4l2_device_test.cpp\n> >>> create mode 100644 test/v4l2_device/v4l2_device_test.h\n> >>> \n> >>> diff --git a/src/libcamera/include/v4l2_device.h\n> >>> b/src/libcamera/include/v4l2_device.h new file mode 100644\n> >>> index 000000000000..619d932d3c82\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/include/v4l2_device.h\n> >>> @@ -0,0 +1,36 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2018, Google Inc.\n> >>> + *\n> >>> + * v4l2_device.h - V4L2 Device API Abstractions\n> >>> + */\n> >>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n> >>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n> >>> +\n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +\n> >>> +namespace libcamera {\n> >>> +\n> >>> +class V4L2Device\n\nV4L2Device or V4l2Device ? The latter looks weird to me, but capitalizing only \nthe first letter of an abbreviation makes sense as a rule (and that's what the \nGoogle C++ Coding Style mandates).\n\n> >>> +{\n> >>> +public:\n> >>> +\tV4L2Device(const std::string &);\n> \n> nit: I've been suggested to use parameter's names in function\n> declarations in header files. I know this is very minor stuff, but at\n> the very beginning of the development is worth being quite picky on\n> this minor detail to establish a consistent code base. So please bear\n> with me here a little more :)\n\nI was about to say the same :-) Parameter names make function prototypes more \nexplicit.\n\n> >>> +\t~V4L2Device();\n> >>> +\n> >>> +\tbool isOpen();\n\ns/;/ const;\n\n> >>> +\tint open();\n> >>> +\tvoid close();\n> >>> +\n> >>> +\tint queryCap();\n> >>> +\n> >>> +private:\n> >>> +\tstd::string device_;\n> >>> +\tint fd_;\n> >>> +\tint capabilities_;\n> \n> Ok, I'm saying it: \"What about reverse-xmas-tree order for variable\n> declarations?\"\n> \n> Here, let's the bike-shedding begin\n\nIt also makes sense to group them by purpose. I'd take the line size into \naccount only when no other criterion applies. In this case the order proposed \nby Kieran makes sense to me.\n\nPlease also note that we can't reorganize members freely as they generally \nbreaks binary compatibility (at least for classes exposed through the public \nAPI).\n\n> >>> +};\n> >>> +\n> >>> +}\n> >> \n> >> nit:\n> >> } /* namespace libcamera */\n> > \n> > Ah yes - that might be nice.\n> > \n> > How did I miss this?\n> > Do we have this in our templates?\n> > \n> > Hrm ... nope, this end comment isn't there.\n> > \n> > I've added it to the template document.\n> \n> Oh thanks, I missed it was not there, but I think that rule comes from\n> the C++ style guide cited in our coding style document.\n> \n> >>> +\n> >>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n\n> >>> diff --git a/src/libcamera/v4l2_device.cpp\n> >>> b/src/libcamera/v4l2_device.cpp\n> >>> new file mode 100644\n> >>> index 000000000000..eea2514f343c\n> >>> --- /dev/null\n> >>> +++ b/src/libcamera/v4l2_device.cpp\n> >>> @@ -0,0 +1,137 @@\n> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2018, Google Inc.\n> >>> + *\n> >>> + * v4l2_device.cpp - V4L2 Device API\n> >>> + */\n> >>> +\n> >>> +#include \n> >>> +#include \n> >>> +#include \n> >>> +\n> >>> +#include \n> >>> +#include \n\nYou can mix the five includes above, no need to separate sys/.\n\n> >>> +#include \"log.h\"\n> >>> +#include \"v4l2_device.h\"\n> >>> +\n> >>> +/**\n> >>> + * \\file v4l2_device.cpp\n> >> \n> >> file v4l2_device.h to document here what is defined in the header file\n> > \n> > Oh - So this is supposed to reference the header not declare the current\n> > file. I guess that kind of makes sense :) Always pointless to state the\n> > name of the current file.\n> \n> It allows documenting the code in the cpp file where the implementation is,\n> but to refer to definitions in the header file. I know, confusing.\n\nTo be exact, it tells Doxygen to generate documentation for all classes, \nfunctions and macros in the referenced file. We thus need a \\file statement \nfor the header, and no \\file statement should be needed for .cpp files as we \nshouldn't define any API there (symbols that are fully private to a .cpp file \nare internal and I don't think they need to appear in the generated \ndocumentation).\n\n> >>> + * \\brief V4L2 Device API\n> >>> + */\n> >>> +namespace libcamera {\n> >>> +\n> >>> +/**\n> >>> + * \\class V4L2Device\n> >>> + * \\brief V4L2Device object and API.\n> >>> + *\n> >>> + * The V4L2 Device API class models a single instance of a v4l2 device\n\ns/API //\ns/a single instance/an instance/ ?\n\n> >>> node.\n> >>> + */\n> >>> +\n> >>> +/**\n> >>> + * \\fn V4L2Device::V4L2Device(device)\n> >> \n> >> nit: parameter type?\n> > \n> > I believe doxygen collects that automatically?\n> > \n> > It generates this in the output:\n> > libcamera::V4L2Device::V4L2Device(const std::string & device)\n\nwhen the function isn't overloaded. But for functions defined in .cpp files \n(as opposed as the inlined functions in the class definition), you can drop \nthe \\fn line completely, doxygen knows that the comment block is related to \nthe function that follows immediately.\n\n> Oh really? That's nice, I did it in all functions and I could have\n> avoided that...\n> \n> >>> + *\n> >>> + * Default constructor for a V4L2Device object. The \\a device\n> >>> specifies a\n> >>> + * string to representation of the device object.\n> >> \n> >> My poor English skill do not allow me to say if this is a typo or it\n> >> actually makes sense :)\n> > \n> > Ahem - there's certainly a typo there - good spot.\n> > \n> > It's also not necessarily correct, and is too close from my Camera class\n> > constructor text.\n> > \n> > I'll adapt this to :\n> > * Default constructor for a V4L2Device object. The \\a device specifies\n> > * the path to the video device node.\n\nThe default constructor would be V4L2Device::V4L2Device(), which you should = \ndelete in the header.\n\n> >>> + */\n> >>> +V4L2Device::V4L2Device(const std::string &device)\n\n\t: device_(device), fd_(fd), capabilities_(0)\n\n> >>> +{\n> >>> +\tdevice_ = device;\n> >>> +\tfd_ = -1;\n> >>> +\tcapabilities_ = 0;\n> >>> +\n> >>> +\tLOG(Debug) << \"V4L2Device Constructed for \" << device_;\n> >> \n> >> Not sure a debug printout is worth here\n> > \n> > Certainly not for long... they're just here for early dev.\n> > \n> > I can just drop these - they're not useful.\n> \n> I think that would be better\n> \n> >>> +}\n> >>> +\n> >>> +V4L2Device::~V4L2Device()\n> >>> +{\n> >>> +\tclose();\n> >>> +\n> >>> +\tLOG(Debug) << \"V4L2Device Destroyed\";\n> >> \n> >> ditto\n> >> \n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\fn V4L2Device::isOpen(())\n> >> \n> >> one () in excess?\n> > \n> > Not sure how that happened?\n\nYou can drop the \\fn line completely anyway (same for the functions below that \nare defined in this file).\n\n> >>> + *\n> >>> + * Checks to see if we have successfully opened a v4l2 video device.\n\n\\brief Check if the device is open\n\\sa open()\n\\return True if the device is open, closed otherwise\n\n> >>> + */\n> >>> +bool V4L2Device::isOpen()\n> >>> +{\n> >>> +\treturn (fd_ != -1);\n> >> \n> >> nit: no need to () braces\n> > \n> > Can I keep them ?\n> \n> Sure!\n> \n> > Returning more than a simple variable or statement 'openly' feels\n> > horrible to me ...\n\nYou will however have to deal with all the code in libcamera that will make \nyou fell horrible, so maybe you'll need to get used to it ;-)\n\n> :)\n>\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\fn V4L2Device::open()\n> >>> + *\n> >>> + * Opens a v4l2 device and queries properties from the device.\n\n\\brief and \\return. Same for the functions below.\n\n> >>> + */\n> >>> +int V4L2Device::open()\n> >>> +{\n> >>> +\tint ret;\n> >>> +\n> >>> +\tif (isOpen()) {\n> >>> +\t\tLOG(Error) << \"Device already open\";\n> >>> +\t\treturn -EBADF;\n\n-EBUSY maybe ?\n\n> >>> +\t}\n> >>> +\n> >>> +\tfd_ = ::open(device_.c_str(), O_RDWR);\n> >>> +\tif (fd_ < 0) {\n\n\t\tret = -errno;\n\n> >>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << device_\n> >>> +\t\t\t << \" : \" << strerror(errno);\n> >>> +\t\treturn -errno;\n\n\t\t\t<< \" : \" << strerror(-ret);\n\treturn ret;\n\n> >>> +\t}\n> >>> +\n> >>> +\tret = queryCap();\n> >> \n> >> I don't like it much either, but this might have been \"int ret = \"\n> > \n> > Hrm ... that certainly is late instantiation.\n> > \n> > Is that what we're going for ?\n> \n> That's how I would interpret\n> https://google.github.io/styleguide/cppguide.html#Local_Variables\n> \n> It could be discussed though. Again, it is only worth discussing this\n> minor things as we're at the beginning and trying to establish a well\n> consistent code base\n\nFor variables used throughout a function (ret or a i loop counter for \ninstance), I think declaring them at the top makes sense.\n\n> >>> +\tif (ret)\n> >>> +\t\treturn ret;\n> >>> +\n> >>> +\tif (!(capabilities_ & V4L2_CAP_STREAMING)) {\n> >> \n> >> I wonder if it wouldn't be better to return the capabilities from\n> >> queryCap, and assign them to the class member capabilities_ after this\n> >> check here.\n> >> \n> >> Looking at the code I -knew- that the queryCap() function had modified\n> >> the capabilities_ and then you're inspecting them here, but it's all a\n> >> bit implicit to me...\n> > \n> > How will we determine if the ioctl call succeeded?\n> > \n> > (while avoiding exceptions?)\n> \n> Well, 'queryCap()' would return -1 (or what more appropriate, like the\n> errno value)\n> \n> > Capabilities flag can return any free-form int value. (ok - so it's\n> > restricted to the flags available in V4L2 ... but... that's very\n> > function specific)\n> \n> Can it return values < 0 ?\n> \n> >>> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n> >>> +\t\treturn -EINVAL;\n> >>> +\t}\n> >>> +\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\fn V4L2Device::close()\n> >>> + *\n> >>> + * Close a v4l2 device handle.\n\nThe file handle isn't exposed as part of the API, so I wouldn't mention it\nhere. You can just explain that is closes the device, releases any resource \nacquired by open(), and that calling the rest of the API on a closed device \nisn't allowed.\n\n> >>> + */\n> >>> +void V4L2Device::close()\n> >>> +{\n> >>> +\tif (fd_ < 0)\n> >>> +\t\treturn;\n> >>> +\n> >>> +\t::close(fd_);\n> >>> +\tfd_ = -1;\n> >>> +}\n> >>> +\n> >>> +/**\n> >>> + * \\fn V4L2Device::queryCap()\n> >>> + *\n> >>> + * Obtains the capabilities information from the V4L2 Device.\n> >>> + */\n> >>> +int V4L2Device::queryCap()\n> >>> +{\n> >>> +\tstruct v4l2_capability cap = {};\n> >>> +\tint ret;\n> >>> +\n> >>> +\tif (!isOpen()) {\n> >>> +\t\tLOG(Error) << \"Attempt to query unopened device\";\n> >>> +\t\treturn -EBADF;\n> >>> +\t}\n\nI think you should restrict queryCap() to be called only from the open() \nfunction and make it private. You could then remove this check. Or, possibly, \neven inline this function in open() as it would then be a single ioctl call.\n\n> >>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);\n> >>> +\tif (ret < 0) {\n> >>> +\t\tLOG(Error) << \"Failed to query device capabilities\";\n> >>> +\t\treturn -1;\n> >> \n> >> It would be useful to return the errno number and possibly print it\n> >> out with strerror\n> > \n> > Good point!\n> > \n> > Should this be -errno? Or just return ret?\n> \n> Returning 'ret' is equivalent to return -1. -errno is the appropriate\n> one.\n> \n> > The more I think about it - the more I'd like to wrap the ioctl call in\n> > the class (or more likely a Device base class) which will handle the\n> > error checks...\n> > \n> > That could then be a parent class for your MediaDevice object too, and\n> > the expected V4L2SubDevice ...\n> > \n> > What are your thoughts? Over-abstracting? or useful ...\n> \n> No, I think it is useful. Actually, more than the MediaDevice itself\n> (which I expect to instantiate V4L2Devices and not extend it) I think\n> it might be worth implementing a V4L2Object that provides protected\n> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device\n> and V4L2Subdevice sub-class it.\n\nI think it's a bit of an overabstraction :-) I believe there will be very \nlittle opportunities to share code between V4l2Device, V4l2Subdevice and \nMediaDevice. I recommend developing them as separate objects, and then \ncreating a common base (or common helper functions) later if we realize enough \ncode is duplicated.\n\n> I'm not sure right now how I would model that, if the base class should\n> do things like \"setFormat()\" or either provides an \"ioctl()\" function and\n> have sub-classes implement the \"setFormat()\" logic. This boils down to\n> how different that would be the implementation between a device and a\n> subdevice. What do you think?\n\nThe format setting model for V4L2 device and V4L2 subdevices is fundamentally \ndifferent. Subdevices take an extra pad number argument, and they propagate \nformats internally within the subdevice, which creates a set of \"subdev \nconfiguration\" objects internally in the kernel (a global one for the active \nparameters and one per file handle for the test parameters). We will have to \nexpose this one way or another to implement trying formats, and I expect the \nV4l2Device and V4l2Subdevice objects to offer different APIs for that reason.\n\n> >>> +\t}\n> >>> +\n> >>> +\tcapabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS\n> >>> +\t\t ? cap.device_caps : cap.capabilities;\n\nHow about caching the full v4l2_capability structure in V4L2Device ?\n\n> >>> +\treturn 0;\n> >>> +}\n> >>> +\n> >>> +} /* namespace libcamera */\n> >>> diff --git a/test/meson.build b/test/meson.build\n> >>> index 754527324c7d..2bc76c289eea 100644\n> >>> --- a/test/meson.build\n> >>> +++ b/test/meson.build\n> >>> @@ -12,6 +12,8 @@ test_includes_internal = [\n> >>> libcamera_internal_includes,\n> >>> ]\n> >>> \n> >>> +subdir('v4l2_device')\n> >>> +\n> >>> public_tests = [\n> >>> [ 'test_init', 'init.cpp' ],\n> >> \n> >> Not related to this, but weird spacing here\n> > \n> > This was intentional ... pre-allocation of table spacing. (The table\n> > will have multiple entries...)\n> \n> Why would you pre-allocate this?\n> \n> >>> ]\n> >>> diff --git a/test/v4l2_device/double_open.cpp\n> >>> b/test/v4l2_device/double_open.cpp new file mode 100644\n> >>> index 000000000000..1b7c7bfe14b8\n> >>> --- /dev/null\n> >>> +++ b/test/v4l2_device/double_open.cpp\n> >>> @@ -0,0 +1,32 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2018, Google Inc.\n> >>> + *\n> >>> + * libcamera V4L2 API tests\n> >>> + */\n> >>> +\n> >>> +#include \"v4l2_device_test.h\"\n> >>> +\n> >>> +class DoubleOpen : public V4L2DeviceTest\n\nMaybe V4L2DeviceDoubleOpenTest to avoid namespace clashes when we'll compile \nmultiple unit tests into a single binary for a test suite ? Or maybe using an \nanynomous namespace for this purpose ?\n\n> >>> +{\n> >>> +protected:\n> >>> +\tint run()\n> >>> +\t{\n> >>> +\t\tint ret;\n> >>> +\n> >>> +\t\t/*\n> >>> +\t\t * Expect failure: The device has already been opened by the\n> >>> +\t\t * V4L2DeviceTest base class\n> >>> +\t\t */\n> >>> +\t\tret = dev->open();\n> >>> +\t\tif (!ret) {\n> >>> +\t\t\tfprintf(stderr, \"Double open erroneously succeeded\\n\");\n\ncout/cerr ?\n\n> >>> +\t\t\tdev->close();\n> >>> +\t\t\treturn TEST_FAIL;\n> >>> +\t\t}\n> >>> +\n> >>> +\t\treturn TEST_PASS;\n> >>> +\t}\n> > > \n> > > nit: < one indent level\n> > \n> > Oh -- for all class functions?\n> \n> Oh damn, I got confused by having the full implementation here.\n> \n> For 'big' functions like this one, we don't want to have inlined by\n> implementing them in the definition. Now, I'm struggling to find a\n> final answer to the question \"Are functions implemented in the class\n> definition automatically inlined?\" I bet the compiler is smart enough\n> to find out if it makes sense to inline or not a function even when\n> its implementation is in the header file, but in general you could define\n> the class, and implement the function outside of the definition, even\n> if in the same cpp file.\n> \n> >>> +};\n> >> \n> >> I welcome testing, but do we really need tests to make sure a function\n> >> fails when called twice?\n> > \n> > Yes - I felt this was important.\n> > \n> > It's there because once I added the open() and close() calls - it\n> > provided a way to leak fd's if you call open(), open().\n> > \n> > The first fd would get silently dropped ...\n> > \n> > So I added a test to call open twice, then I added the isOpen() checks.\n> > \n> > In other words - the fix for the leak came about directly because I\n> > added the double_open test...\n> \n> Fine then, thanks for the explanation.\n> \n> > Another reason we I think we should base class the common device\n> > operations (then this test would only apply to the Device object unit\n> > tests anyway.)\n> > \n> > > I'm sure you could have sketched it by\n> > > calling open twice in the V4L2DeviceTest class\n> > \n> > No - the V4L2DeviceTest class is the base class common to all further\n> > tests for the V4L2Device Object.\n> > \n> > I have other patches which add further tests using the V4L2DeviceTest\n> > base class.\n> > \n> >>> +\n> >>> +TEST_REGISTER(DoubleOpen);\n> >>> diff --git a/test/v4l2_device/meson.build\n> >>> b/test/v4l2_device/meson.build\n> >>> new file mode 100644\n> >>> index 000000000000..41675a303498\n> >>> --- /dev/null\n> >>> +++ b/test/v4l2_device/meson.build\n> >>> @@ -0,0 +1,12 @@\n> >>> +# Tests are listed in order of complexity.\n> >>> +# They are not alphabetically sorted.\n> >>> +v4l2_device_tests = [\n> >>> + [ 'double_open', 'double_open.cpp' ],\n> >> \n> >> Again weird spacing, I'm now thinking this is intentional and I don't\n> >> know something\n> > \n> > Table pre-allocation for multiple tests.\n> > \n> > This isn't my only test, and I'd rather not have to reformat the table\n> > if I add tests with longer filenames.\n> > \n> >>> +]\n> >>> +\n> >>> +foreach t : v4l2_device_tests\n> >>> + exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],\n> >>> +\t\t link_with : test_libraries,\n> >>> +\t\t include_directories : test_includes_internal)\n> >>> + test(t[0], exe, suite: 'v4l2_device', is_parallel: false)\n> >>> +endforeach\n> >>> diff --git a/test/v4l2_device/v4l2_device_test.cpp\n> >>> b/test/v4l2_device/v4l2_device_test.cpp new file mode 100644\n> >>> index 000000000000..ae317a9519c5\n> >>> --- /dev/null\n> >>> +++ b/test/v4l2_device/v4l2_device_test.cpp\n> >>> @@ -0,0 +1,36 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2018, Google Inc.\n> >>> + *\n> >>> + * libcamera V4L2 API tests\n> >>> + */\n> >>> +\n> >>> +#include \"v4l2_device_test.h\"\n> >>> +\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +V4L2DeviceTest::V4L2DeviceTest()\n\n\t: dev_(nullptr)\n\n> >>> +{\n> >>> +\tdev = nullptr;\n> >>> +}\n> >>> +\n> >>> +int V4L2DeviceTest::init()\n> >>> +{\n> >>> +\tconst char *device = \"/dev/video0\";\n\n\tstd::string device(\"/dev/video0\");\n\n> >>> +\n> >>> +\t/* Validate the device node exists. */\n> >>> +\tif (!path_exists(device))\n\nPlease log a message to explain why the test is skipped.\n\n> >>> +\t\treturn TEST_SKIP;\n> >>> +\n> >>> +\tdev = new V4L2Device(device);\n> >>> +\tif (!dev)\n> >> \n> >> You should call cleanup here if you fail\n> >> Ah no, the Test class should call it if init() fails!\n> >> Ah no^2, your constructor cannot fail (good!). My previous comment\n> >> still hold though.\n> > \n> > Should I instead call cleanup() from my destructor ?\n> \n> My point was that the object would not get destroyed here. Because the\n> test base class does not call cleanup() after init fails\n> and you don't not call it either. But I was confused actually, as the\n> base test class does this\n> \n> +#define TEST_REGISTER(klass) \\\n> +int main(int argc, char *argv[]) \\\n> +{ \\\n> + return klass().execute(); \\\n> +}\n> \n> And this creates and destroys the object, which calls your destructor,\n> so I -guess- this is fine actually.\n> Not calling 'cleaup()' after a failed init is being discussed now\n> elsewhere.\n> \n> Anyway, your constructor cannot fail, so you could remove that\n> check...\n> \n> > >> +\t\treturn TEST_FAIL;\n> > >> +\n> > >> +\treturn dev->open();\n> > >> +}\n> > >> +\n> > >> +void V4L2DeviceTest::cleanup()\n> > >> +{\n> > >> +\tif (dev)\n> > >> +\t\tdelete dev;\n\nCan't you safely delete nullptr ?\n\n> > >> +};\n> >>> diff --git a/test/v4l2_device/v4l2_device_test.h\n> >>> b/test/v4l2_device/v4l2_device_test.h new file mode 100644\n> >>> index 000000000000..4374ddc7e932\n> >>> --- /dev/null\n> >>> +++ b/test/v4l2_device/v4l2_device_test.h\n> >>> @@ -0,0 +1,31 @@\n> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >>> +/*\n> >>> + * Copyright (C) 2018, Google Inc.\n> >>> + *\n> >>> + * vl42device_test.h - libcamera v4l2device test base class\n> >>> + */\n> >>> +\n> >>> +#include \"test.h\"\n> >>> +#include \"v4l2_device.h\"\n> >> \n> >> Includes after the inclusion guards\n> > \n> > Ack.\n> > \n> >>> +\n> >>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_\n> >>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_\n\n> >>> +\n> >>> +using namespace libcamera;\n> >>> +\n> >>> +class V4L2DeviceTest : public Test\n> >>> +{\n> >>> +public:\n> >>> +\tV4L2DeviceTest();\n> >>> +\tvirtual ~V4L2DeviceTest() {};\n\nIs this needed, given that the base class already has a virtual destructor ?\n\n> >>> +\n> >>> +protected:\n> >>> +\tint init();\n> >>> +\tvoid cleanup();\n> >>> +\n> >>> +\tvirtual int run() = 0;\n\nI don't think this is needed either.\n\n> >>> +\tV4L2Device *dev;\n\ns/dev/dev_/\n\n> >>> +};\n> >>> +\n> >>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */","headers":{"Return-Path":"","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 8E15960B13\n\tfor ;\n\tFri, 11 Jan 2019 18:46:21 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A49753E;\n\tFri, 11 Jan 2019 18:46:20 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547228781;\n\tbh=KqUSiqvyhbxUZO9U1Vbd0KI64V6r3OFWv1JYGLGbjpE=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=m0VxvHvkkFVhUfgQtQ0dafLhNGf971RK1a5pRey0/Eeq5nF+RJ8UEllDW//oJtgnr\n\txnP1J9wdY0l/mcy78s1ufh1gcIpyQf35OnBPhlQjz863r+23U+NILnsFg/+jFZPwBr\n\tYKWHRLlFxa4fN8Y7UB6sWTyTVuvvPAEmzvb1zYK4=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 11 Jan 2019 19:47:32 +0200","Message-ID":"<5695091.PZBZBohWtT@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181223205409.GB13765@archlinux>","References":"<20181221123724.27290-1-kieran.bingham@ideasonboard.com>\n\t\n\t<20181223205409.GB13765@archlinux>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","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":"Fri, 11 Jan 2019 17:46:21 -0000"}},{"id":298,"web_url":"https://patchwork.libcamera.org/comment/298/","msgid":"","date":"2019-01-14T10:37:23","subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 23/12/2018 20:54, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:\n>> Hi Jacopo,\n>>\n>> Thanks for the review,\n>>\n>> On 21/12/2018 16:25, jacopo mondi wrote:\n>>> Hi Kieran,\n>>> thanks for the patch\n>>>\n>>> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:\n>>>> Provide a helper V4L2 device object capable of interacting with the\n>>>> V4L2 Linux Kernel APIs.\n>>>>\n>>>> A test suite is added at test/v4l2_device/ to utilise and validate the\n>>>> code base.\n>>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>> ---\n>>>> src/libcamera/include/v4l2_device.h | 36 +++++++\n>>>> src/libcamera/meson.build | 2 +\n>>>> src/libcamera/v4l2_device.cpp | 137 ++++++++++++++++++++++++++\n>>>\n>>> I would really split test and library code in 2 differen commits.\n>>\n>> I'm happy to go with group consensus here, but I felt that tests and\n>> code could be in the same commit here, as the test directly affects the\n>> code added.\n>>\n>> This means that when doing rebase actions or such, I can do 'git rebase\n>> -i -x \"ninja; ninja test\"' and validate every commit as I rework things.\n>>\n>> (I have a script which does that for me)\n>>\n> \n> I see, and I feel in this specific case it's not a big deal, but as a\n> general rule having tests -in the same commit- that adds a specific\n> feature is not something that holds in the general case.\n> \n> Tests should accompany new features, and if a patch series adds\n> anything that is worth being tested, just make sure they get merged\n> together.\n> \n> As an example, Niklas' last series is a 12 patches one, with one test\n> at the end of the series. Your one might be a 3 patches one, with a\n> test at the end. Or am I missing something of how you would like to\n> use testing?\n> \n> Anyway, that's not a big deal at all in this case :)\n\nI've split the tests to separate patches.\n\nAs long as the test patch is straight after the relevant code patch -\nthe semantics are the same.\n\n>>>> test/meson.build | 2 +\n>>>> test/v4l2_device/double_open.cpp | 32 ++++++\n>>>> test/v4l2_device/meson.build | 12 +++\n>>>> test/v4l2_device/v4l2_device_test.cpp | 36 +++++++\n>>>> test/v4l2_device/v4l2_device_test.h | 31 ++++++\n>>>> 8 files changed, 288 insertions(+)\n>>>> create mode 100644 src/libcamera/include/v4l2_device.h\n>>>> create mode 100644 src/libcamera/v4l2_device.cpp\n>>>> create mode 100644 test/v4l2_device/double_open.cpp\n>>>> create mode 100644 test/v4l2_device/meson.build\n>>>> create mode 100644 test/v4l2_device/v4l2_device_test.cpp\n>>>> create mode 100644 test/v4l2_device/v4l2_device_test.h\n>>>>\n>>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n>>>> new file mode 100644\n>>>> index 000000000000..619d932d3c82\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera/include/v4l2_device.h\n>>>> @@ -0,0 +1,36 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2018, Google Inc.\n>>>> + *\n>>>> + * v4l2_device.h - V4L2 Device API Abstractions\n>>>> + */\n>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n>>>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +class V4L2Device\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2Device(const std::string &);\n> \n> nit: I've been suggested to use parameter's names in function\n> declarations in header files. I know this is very minor stuff, but at\n> the very beginning of the development is worth being quite picky on\n> this minor detail to establish a consistent code base. So please bear\n> with me here a little more :)\n\nAdded.\n\n> \n>>>> +\t~V4L2Device();\n>>>> +\n>>>> +\tbool isOpen();\n>>>> +\tint open();\n>>>> +\tvoid close();\n>>>> +\n>>>> +\tint queryCap();\n>>>> +\n>>>> +private:\n>>>> +\tstd::string device_;\n>>>> +\tint fd_;\n>>>> +\tint capabilities_;\n> \n> Ok, I'm saying it: \"What about reverse-xmas-tree order for variable\n> declarations?\"\n> \n> Here, let's the bike-shedding begin\n\nI'll respond here in Laurent's mail.\n\n\n> \n>>>> +};\n>>>> +\n>>>> +}\n>>>\n>>> nit:\n>>> } /* namespace libcamera */\n>>>\n>>\n\nAdded.\n\n\n>> Ah yes - that might be nice.\n>>\n>> How did I miss this?\n>> Do we have this in our templates?\n>>\n>> Hrm ... nope, this end comment isn't there.\n>>\n>> I've added it to the template document.\n> \n> Oh thanks, I missed it was not there, but I think that rule comes from\n> the C++ style guide cited in our coding style document.\n> \n>>\n>>\n>>>> +\n>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>> index f632eb5dd779..bbaf9a05ec2c 100644\n>>>> --- a/src/libcamera/meson.build\n>>>> +++ b/src/libcamera/meson.build\n>>>> @@ -1,11 +1,13 @@\n>>>> libcamera_sources = files([\n>>>> 'log.cpp',\n>>>> 'main.cpp',\n>>>> + 'v4l2_device.cpp',\n>>>> ])\n>>>>\n>>>> libcamera_headers = files([\n>>>> 'include/log.h',\n>>>> 'include/utils.h',\n>>>> + 'include/v4l2_device.h',\n>>>> ])\n>>>>\n>>>> libcamera_internal_includes = include_directories('include')\n>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>>> new file mode 100644\n>>>> index 000000000000..eea2514f343c\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>> @@ -0,0 +1,137 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2018, Google Inc.\n>>>> + *\n>>>> + * v4l2_device.cpp - V4L2 Device API\n>>>> + */\n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +#include \"log.h\"\n>>>> +#include \"v4l2_device.h\"\n>>>> +\n>>>> +/**\n>>>> + * \\file v4l2_device.cpp\n>>>\n>>> file v4l2_device.h to document here what is defined in the header file\n>>\n>> Oh - So this is supposed to reference the header not declare the current\n>> file. I guess that kind of makes sense :) Always pointless to state the\n>> name of the current file.\n>>\n> \n> It allows documenting the code in the cpp file where the implementation is,\n> but to refer to definitions in the header file. I know, confusing.\n> \n>>>\n>>>> + * \\brief V4L2 Device API\n>>>> + */\n>>>> +namespace libcamera {\n>>>> +\n>>>> +/**\n>>>> + * \\class V4L2Device\n>>>> + * \\brief V4L2Device object and API.\n>>>> + *\n>>>> + * The V4L2 Device API class models a single instance of a v4l2 device node.\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2Device::V4L2Device(device)\n>>>\n>>> nit: parameter type?\n>>\n>> I believe doxygen collects that automatically?\n>>\n>> It generates this in the output:\n>> libcamera::V4L2Device::V4L2Device(const std::string & device)\n> \n> Oh really? That's nice, I did it in all functions and I could have\n> avoided that...\n>>\n>>\n>>>> + *\n>>>> + * Default constructor for a V4L2Device object. The \\a device specifies a\n>>>> + * string to representation of the device object.\n>>>\n>>> My poor English skill do not allow me to say if this is a typo or it\n>>> actually makes sense :)\n>>\n>> Ahem - there's certainly a typo there - good spot.\n>>\n>> It's also not necessarily correct, and is too close from my Camera class\n>> constructor text.\n>>\n>> I'll adapt this to :\n>>\n>> * Default constructor for a V4L2Device object. The \\a device specifies\n>> * the path to the video device node.\n>>\n>>>\n>>>> + */\n>>>> +V4L2Device::V4L2Device(const std::string &device)\n>>>> +{\n>>>> +\tdevice_ = device;\n>>>> +\tfd_ = -1;\n>>>> +\tcapabilities_ = 0;\n>>>> +\n>>>> +\tLOG(Debug) << \"V4L2Device Constructed for \" << device_;\n>>>\n>>> Not sure a debug printout is worth here\n>>\n>> Certainly not for long... they're just here for early dev.\n>>\n>> I can just drop these - they're not useful.\n>>\n> \n> I think that would be better\n> \n>>>\n>>>> +}\n>>>> +\n>>>> +V4L2Device::~V4L2Device()\n>>>> +{\n>>>> +\tclose();\n>>>> +\n>>>> +\tLOG(Debug) << \"V4L2Device Destroyed\";\n>>>\n>>> ditto\n>>>\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2Device::isOpen(())\n>>>\n>>> one () in excess?\n>>\n>> Not sure how that happened?\n>>\n>>>\n>>>> + *\n>>>> + * Checks to see if we have successfully opened a v4l2 video device.\n>>>> + */\n>>>> +bool V4L2Device::isOpen()\n>>>> +{\n>>>> +\treturn (fd_ != -1);\n>>>\n>>> nit: no need to () braces\n>>\n>> Can I keep them ?\n>>\n> \n> Sure!\n> \n>> Returning more than a simple variable or statement 'openly' feels\n>> horrible to me ...\n> \n> :)\n> \n>>\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2Device::open()\n>>>> + *\n>>>> + * Opens a v4l2 device and queries properties from the device.\n>>>> + */\n>>>> +int V4L2Device::open()\n>>>> +{\n>>>> +\tint ret;\n>>>> +\n>>>> +\tif (isOpen()) {\n>>>> +\t\tLOG(Error) << \"Device already open\";\n>>>> +\t\treturn -EBADF;\n>>>> +\t}\n>>>> +\n>>>> +\tfd_ = ::open(device_.c_str(), O_RDWR);\n>>>> +\tif (fd_ < 0) {\n>>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << device_\n>>>> +\t\t\t << \" : \" << strerror(errno);\n>>>> +\t\treturn -errno;\n>>>> +\t}\n>>>> +\n>>>> +\tret = queryCap();\n>>>\n>>> I don't like it much either, but this might have been \"int ret = \"\n>>\n>> Hrm ... that certainly is late instantiation.\n>>\n>> Is that what we're going for ?\n> \n> That's how I would interpret\n> https://google.github.io/styleguide/cppguide.html#Local_Variables\n> \n> It could be discussed though. Again, it is only worth discussing this\n> minor things as we're at the beginning and trying to establish a well\n> consistent code base\n> \n>>\n>>\n>>>\n>>>> +\tif (ret)\n>>>> +\t\treturn ret;\n>>>> +\n>>>> +\tif (!(capabilities_ & V4L2_CAP_STREAMING)) {\n>>>\n>>> I wonder if it wouldn't be better to return the capabilities from\n>>> queryCap, and assign them to the class member capabilities_ after this\n>>> check here.\n>>>\n>>> Looking at the code I -knew- that the queryCap() function had modified\n>>> the capabilities_ and then you're inspecting them here, but it's all a\n>>> bit implicit to me...\n>>\n>> How will we determine if the ioctl call succeeded?\n>> (while avoiding exceptions?)\n> \n> Well, 'queryCap()' would return -1 (or what more appropriate, like the\n> errno value)\n> \n>>\n>> Capabilities flag can return any free-form int value. (ok - so it's\n>> restricted to the flags available in V4L2 ... but... that's very\n>> function specific)\n> \n> Can it return values < 0 ?\n> \n>>\n>>\n>>\n>>>> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n>>>> +\t\treturn -EINVAL;\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2Device::close()\n>>>> + *\n>>>> + * Close a v4l2 device handle.\n>>>> + */\n>>>> +void V4L2Device::close()\n>>>> +{\n>>>> +\tif (fd_ < 0)\n>>>> +\t\treturn;\n>>>> +\n>>>> +\t::close(fd_);\n>>>> +\tfd_ = -1;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\fn V4L2Device::queryCap()\n>>>> + *\n>>>> + * Obtains the capabilities information from the V4L2 Device.\n>>>> + */\n>>>> +int V4L2Device::queryCap()\n>>>> +{\n>>>> +\tstruct v4l2_capability cap = {};\n>>>> +\tint ret;\n>>>> +\n>>>> +\tif (!isOpen()) {\n>>>> +\t\tLOG(Error) << \"Attempt to query unopened device\";\n>>>> +\t\treturn -EBADF;\n>>>> +\t}\n>>>> +\n>>>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);\n>>>> +\tif (ret < 0) {\n>>>> +\t\tLOG(Error) << \"Failed to query device capabilities\";\n>>>> +\t\treturn -1;\n>>>\n>>> It would be useful to return the errno number and possibly print it\n>>> out with strerror\n>>\n>> Good point!\n>>\n>> Should this be -errno? Or just return ret?\n> \n> Returning 'ret' is equivalent to return -1. -errno is the appropriate\n> one.\n\n\nChanged.\n\n\n> \n>>\n>>\n>> The more I think about it - the more I'd like to wrap the ioctl call in\n>> the class (or more likely a Device base class) which will handle the\n>> error checks...\n>>\n>> That could then be a parent class for your MediaDevice object too, and\n>> the expected V4L2SubDevice ...\n>>\n>> What are your thoughts? Over-abstracting? or useful ...\n> \n> No, I think it is useful. Actually, more than the MediaDevice itself\n> (which I expect to instantiate V4L2Devices and not extend it) I think\n> it might be worth implementing a V4L2Object that provides protected\n> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device\n> and V4L2Subdevice sub-class it.\n> \n> I'm not sure right now how I would model that, if the base class should\n> do things like \"setFormat()\" or either provides an \"ioctl()\" function and\n> have sub-classes implement the \"setFormat()\" logic. This boils down to\n> how different that would be the implementation between a device and a\n> subdevice. What do you think?\n\n\nI think MediaDevice, V4L2Device, and V4L2SubDevice are all 'devices'\nwhich operate on a common linux device node object. So I still think\nthey deserve a base class even it just to define the API that those\nobjects implement.\n\nBut I think Laurent further objected to this in his mail - so I'll move\nthere after this, and not do anything here yet.\n\n\n\n> \n>>\n>>\n>>>\n>>>> +\t}\n>>>> +\n>>>> +\tcapabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS\n>>>> +\t\t ? cap.device_caps : cap.capabilities;\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/test/meson.build b/test/meson.build\n>>>> index 754527324c7d..2bc76c289eea 100644\n>>>> --- a/test/meson.build\n>>>> +++ b/test/meson.build\n>>>> @@ -12,6 +12,8 @@ test_includes_internal = [\n>>>> libcamera_internal_includes,\n>>>> ]\n>>>>\n>>>> +subdir('v4l2_device')\n>>>> +\n>>>> public_tests = [\n>>>> [ 'test_init', 'init.cpp' ],\n>>>\n>>> Not related to this, but weird spacing here\n>>\n>> This was intentional ... pre-allocation of table spacing. (The table\n>> will have multiple entries...)\n> \n> Why would you pre-allocate this?\n\nSpace :)\n\notherwise:\n [ 'test_cows', 'cows.cpp' ],\n [ 'test_fish', 'fish.cpp' ],\n [ 'test_init', 'init.cpp' ],\n [ 'test_mice', 'mice.cpp' ],\n\nbecomes:\n\n- [ 'test_cows', 'cows.cpp' ],\n- [ 'test_fish', 'fish.cpp' ],\n- [ 'test_init', 'init.cpp' ],\n- [ 'test_mice', 'mice.cpp' ],\n+ [ 'test_cows', 'cows.cpp' ],\n+ [ 'test_fish', 'fish.cpp' ],\n+ [ 'test_init', 'init.cpp' ],\n+ [ 'test_longer_names_ugh', 'longer_names_ugh' ],\n+ [ 'test_mice', 'mice.cpp' ],\n\nto add a single line...\n\nI'm starting to think perhaps we make the iterator just look for the\n.cpp extension on top of the test name though to remove the need for the\nsecond column.\n\n\n>>\n>>>\n>>>> ]\n>>>> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp\n>>>> new file mode 100644\n>>>> index 000000000000..1b7c7bfe14b8\n>>>> --- /dev/null\n>>>> +++ b/test/v4l2_device/double_open.cpp\n>>>> @@ -0,0 +1,32 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2018, Google Inc.\n>>>> + *\n>>>> + * libcamera V4L2 API tests\n>>>> + */\n>>>> +\n>>>> +#include \"v4l2_device_test.h\"\n>>>> +\n>>>> +class DoubleOpen : public V4L2DeviceTest\n>>>> +{\n>>>> +protected:\n>>>> +\tint run()\n>>>> +\t{\n>>>> +\t\tint ret;\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * Expect failure: The device has already been opened by the\n>>>> +\t\t * V4L2DeviceTest base class\n>>>> +\t\t */\n>>>> +\t\tret = dev->open();\n>>>> +\t\tif (!ret) {\n>>>> +\t\t\tfprintf(stderr, \"Double open erroneously succeeded\\n\");\n>>>> +\t\t\tdev->close();\n>>>> +\t\t\treturn TEST_FAIL;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\treturn TEST_PASS;\n>>>> +\t}\n>>>\n>>> nit: < one indent level\n>>\n>> Oh -- for all class functions?\n> \n> Oh damn, I got confused by having the full implementation here.\n> \n> For 'big' functions like this one, we don't want to have inlined by\n> implementing them in the definition. Now, I'm struggling to find a\n> final answer to the question \"Are functions implemented in the class\n> definition automatically inlined?\" I bet the compiler is smart enough\n> to find out if it makes sense to inline or not a function even when\n> its implementation is in the header file, but in general you could define\n> the class, and implement the function outside of the definition, even\n> if in the same cpp file.\n\n\nI think for a test (where the API is already defined by the Test class)\nwe don't need to redefine things unnecessarily ?\n\nThis is just the implementation of a single test.\n\n\n>>>> +};\n>>>\n>>> I welcome testing, but do we really need tests to make sure a function\n>>> fails when called twice?\n>>\n>> Yes - I felt this was important.\n>>\n>> It's there because once I added the open() and close() calls - it\n>> provided a way to leak fd's if you call open(), open().\n>>\n>> The first fd would get silently dropped ...\n>>\n>> So I added a test to call open twice, then I added the isOpen() checks.\n>>\n>> In other words - the fix for the leak came about directly because I\n>> added the double_open test...\n>>\n> \n> Fine then, thanks for the explanation.\n> \n>>\n>> Another reason we I think we should base class the common device\n>> operations (then this test would only apply to the Device object unit\n>> tests anyway.)\n>>\n>>\n>>> I'm sure you could have sketched it by\n>>> calling open twice in the V4L2DeviceTest class\n>>\n>> No - the V4L2DeviceTest class is the base class common to all further\n>> tests for the V4L2Device Object.\n>>\n>> I have other patches which add further tests using the V4L2DeviceTest\n>> base class.\n>>\n>>>> +\n>>>> +TEST_REGISTER(DoubleOpen);\n>>>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build\n>>>> new file mode 100644\n>>>> index 000000000000..41675a303498\n>>>> --- /dev/null\n>>>> +++ b/test/v4l2_device/meson.build\n>>>> @@ -0,0 +1,12 @@\n>>>> +# Tests are listed in order of complexity.\n>>>> +# They are not alphabetically sorted.\n>>>> +v4l2_device_tests = [\n>>>> + [ 'double_open', 'double_open.cpp' ],\n>>>\n>>> Again weird spacing, I'm now thinking this is intentional and I don't\n>>> know something\n>>\n>> Table pre-allocation for multiple tests.\n>>\n>> This isn't my only test, and I'd rather not have to reformat the table\n>> if I add tests with longer filenames.\n>>\n>>>\n>>>> +]\n>>>> +\n>>>> +foreach t : v4l2_device_tests\n>>>> + exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],\n>>>> +\t\t link_with : test_libraries,\n>>>> +\t\t include_directories : test_includes_internal)\n>>>> + test(t[0], exe, suite: 'v4l2_device', is_parallel: false)\n>>>> +endforeach\n>>>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp\n>>>> new file mode 100644\n>>>> index 000000000000..ae317a9519c5\n>>>> --- /dev/null\n>>>> +++ b/test/v4l2_device/v4l2_device_test.cpp\n>>>> @@ -0,0 +1,36 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2018, Google Inc.\n>>>> + *\n>>>> + * libcamera V4L2 API tests\n>>>> + */\n>>>> +\n>>>> +#include \"v4l2_device_test.h\"\n>>>> +\n>>>> +using namespace libcamera;\n>>>> +\n>>>> +V4L2DeviceTest::V4L2DeviceTest()\n>>>> +{\n>>>> +\tdev = nullptr;\n>>>> +}\n>>>> +\n>>>> +int V4L2DeviceTest::init()\n>>>> +{\n>>>> +\tconst char *device = \"/dev/video0\";\n>>>> +\n>>>> +\t/* Validate the device node exists. */\n>>>> +\tif (!path_exists(device))\n>>>> +\t\treturn TEST_SKIP;\n>>>> +\n>>>> +\tdev = new V4L2Device(device);\n>>>> +\tif (!dev)\n>>>\n>>> You should call cleanup here if you fail\n>>> Ah no, the Test class should call it if init() fails!\n>>> Ah no^2, your constructor cannot fail (good!). My previous comment\n>>> still hold though.\n>>>\n>>\n>> Should I instead call cleanup() from my destructor ?\n> \n> My point was that the object would not get destroyed here. Because the\n> test base class does not call cleanup() after init fails\n> and you don't not call it either. But I was confused actually, as the\n> base test class does this\n> \n> +#define TEST_REGISTER(klass) \\\n> +int main(int argc, char *argv[]) \\\n> +{ \\\n> + return klass().execute(); \\\n> +}\n> \n> And this creates and destroys the object, which calls your destructor,\n> so I -guess- this is fine actually.\n> Not calling 'cleaup()' after a failed init is being discussed now\n> elsewhere.\n> \n> Anyway, your constructor cannot fail, so you could remove that\n> check...\n\nYes, I don't think we need to check the returns of a call to new. I\nguess if we run out of memory we worry about other things instead :)\n\n\n\n> \n>>\n>>\n>>\n>>>> +\t\treturn TEST_FAIL;\n>>>> +\n>>>> +\treturn dev->open();\n>>>> +}\n>>>> +\n>>>> +void V4L2DeviceTest::cleanup()\n>>>> +{\n>>>> +\tif (dev)\n>>>> +\t\tdelete dev;\n>>>> +};\n>>>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h\n>>>> new file mode 100644\n>>>> index 000000000000..4374ddc7e932\n>>>> --- /dev/null\n>>>> +++ b/test/v4l2_device/v4l2_device_test.h\n>>>> @@ -0,0 +1,31 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2018, Google Inc.\n>>>> + *\n>>>> + * vl42device_test.h - libcamera v4l2device test base class\n>>>> + */\n>>>> +\n>>>> +#include \"test.h\"\n>>>> +#include \"v4l2_device.h\"\n>>>\n>>> Includes after the inclusion guards\n>>\n>> Ack.\n>>\n>>\n>>>> +\n>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_\n>>>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_\n>>>> +\n>>>> +using namespace libcamera;\n>>>> +\n>>>> +class V4L2DeviceTest : public Test\n>>>> +{\n>>>> +public:\n>>>> +\tV4L2DeviceTest();\n>>>> +\tvirtual ~V4L2DeviceTest() {};\n>>>> +\n>>>> +protected:\n>>>> +\tint init();\n>>>> +\tvoid cleanup();\n>>>> +\n>>>> +\tvirtual int run() = 0;\n>>>> +\n>>>> +\tV4L2Device *dev;\n>>>> +};\n>>>> +\n>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */\n>>>> --\n>>>> 2.17.1\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>> --\n>> Kieran","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 7578560C71\n\tfor ;\n\tMon, 14 Jan 2019 11:37:26 +0100 (CET)","from [192.168.0.21]\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 CA877505;\n\tMon, 14 Jan 2019 11:37:25 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547462246;\n\tbh=tmnrrFwefiKP8p8wmmcffpsIT8LBb2XAa6Vvy6Xl8Fc=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Jv35AIvsM8eT6VqzZbyRgUJt9JXirp7BPdDqHGJa4cvCWO2WBDWKDvYMXpXd/gsKk\n\t9yo85Rdi1O1FAy3FpZ+OZKRM1tG+EMMSmgQ3lrN1iGyI78iSnE+xuPO5NPCj4ozyyH\n\tNHGemNpAzDWmA4/ORIEVtloihPQad+HKb8zoxHo4=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi ","Cc":"LibCamera Devel ","References":"<20181221123724.27290-1-kieran.bingham@ideasonboard.com>\n\t<20181221123724.27290-3-kieran.bingham@ideasonboard.com>\n\t<20181221162539.GN5597@w540>\n\t\n\t<20181223205409.GB13765@archlinux>","From":"Kieran Bingham ","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 14 Jan 2019 10:37:23 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<20181223205409.GB13765@archlinux>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","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":"Mon, 14 Jan 2019 10:37:26 -0000"}},{"id":306,"web_url":"https://patchwork.libcamera.org/comment/306/","msgid":"<3b8249f4-81d0-9541-9d61-b8515a89642d@ideasonboard.com>","date":"2019-01-14T14:57:47","subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hola,\n\nOn 11/01/2019 17:47, Laurent Pinchart wrote:\n> Hello,\n> \n> On Sunday, 23 December 2018 22:54:09 EET Jacopo Mondi wrote:\n>> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:\n>>> On 21/12/2018 16:25, jacopo mondi wrote:\n>>>> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:\n>>>>> Provide a helper V4L2 device object capable of interacting with the\n>>>>> V4L2 Linux Kernel APIs.\n>>>>>\n>>>>> A test suite is added at test/v4l2_device/ to utilise and validate the\n>>>>> code base.\n>>>>>\n>>>>> Signed-off-by: Kieran Bingham \n>>>>> ---\n>>>>>\n>>>>> src/libcamera/include/v4l2_device.h | 36 +++++++\n>>>>> src/libcamera/meson.build | 2 +\n>>>>> src/libcamera/v4l2_device.cpp | 137 ++++++++++++++++++++++++++\n>>>>\n>>>> I would really split test and library code in 2 differen commits.\n>>>\n>>> I'm happy to go with group consensus here, but I felt that tests and\n>>> code could be in the same commit here, as the test directly affects the\n>>> code added.\n>>>\n>>> This means that when doing rebase actions or such, I can do 'git rebase\n>>> -i -x \"ninja; ninja test\"' and validate every commit as I rework things.\n>>>\n>>> (I have a script which does that for me)\n>>\n>> I see, and I feel in this specific case it's not a big deal, but as a\n>> general rule having tests -in the same commit- that adds a specific\n>> feature is not something that holds in the general case.\n>>\n>> Tests should accompany new features, and if a patch series adds\n>> anything that is worth being tested, just make sure they get merged\n>> together.\n>>\n>> As an example, Niklas' last series is a 12 patches one, with one test\n>> at the end of the series. Your one might be a 3 patches one, with a\n>> test at the end. Or am I missing something of how you would like to\n>> use testing?\n>>\n>> Anyway, that's not a big deal at all in this case :)\n> \n> I'm also slightly more in favour of separating code and tests in different \n> patches. It's indeed not a big deal in this case, but when the patches and \n> series grow bigger, bundling everything together creates too large patches.\n\nOk - I've split them.\n\n\n\n\n>>>>> test/meson.build | 2 +\n>>>>> test/v4l2_device/double_open.cpp | 32 ++++++\n>>>>> test/v4l2_device/meson.build | 12 +++\n>>>>> test/v4l2_device/v4l2_device_test.cpp | 36 +++++++\n>>>>> test/v4l2_device/v4l2_device_test.h | 31 ++++++\n>>>>> 8 files changed, 288 insertions(+)\n>>>>> create mode 100644 src/libcamera/include/v4l2_device.h\n>>>>> create mode 100644 src/libcamera/v4l2_device.cpp\n>>>>> create mode 100644 test/v4l2_device/double_open.cpp\n>>>>> create mode 100644 test/v4l2_device/meson.build\n>>>>> create mode 100644 test/v4l2_device/v4l2_device_test.cpp\n>>>>> create mode 100644 test/v4l2_device/v4l2_device_test.h\n>>>>>\n>>>>> diff --git a/src/libcamera/include/v4l2_device.h\n>>>>> b/src/libcamera/include/v4l2_device.h new file mode 100644\n>>>>> index 000000000000..619d932d3c82\n>>>>> --- /dev/null\n>>>>> +++ b/src/libcamera/include/v4l2_device.h\n>>>>> @@ -0,0 +1,36 @@\n>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2018, Google Inc.\n>>>>> + *\n>>>>> + * v4l2_device.h - V4L2 Device API Abstractions\n>>>>> + */\n>>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__\n>>>>> +#define __LIBCAMERA_V4L2_DEVICE_H__\n>>>>> +\n>>>>> +#include \n>>>>> +\n>>>>> +#include \n>>>>> +\n>>>>> +namespace libcamera {\n>>>>> +\n>>>>> +class V4L2Device\n> \n> V4L2Device or V4l2Device ? The latter looks weird to me, but capitalizing only \n> the first letter of an abbreviation makes sense as a rule (and that's what the \n> Google C++ Coding Style mandates).\n\nI think I agree on the most part, like if this was a GnuDevice or a\nUsbDevice, but I think having the numbers in affects the appearance.\n\n (I just can't seem to find the lowercase numbers on my keyboard ...)\n\n... Can I file for a just-this-once-rule-exception-allowance-permission?\n\nV4L2 is all-caps in my head... lowering the L seems so wrong...\n\nIf you really feel strongly that this needs to be V4l2 I'll to the\nfind-replace.\n\n\n>>>>> +{\n>>>>> +public:\n>>>>> +\tV4L2Device(const std::string &);\n>>\n>> nit: I've been suggested to use parameter's names in function\n>> declarations in header files. I know this is very minor stuff, but at\n>> the very beginning of the development is worth being quite picky on\n>> this minor detail to establish a consistent code base. So please bear\n>> with me here a little more :)\n> \n> I was about to say the same :-) Parameter names make function prototypes more \n> explicit.\n> \n>>>>> +\t~V4L2Device();\n>>>>> +\n>>>>> +\tbool isOpen();\n> \n> s/;/ const;\n\nUpdated,\n\n\n> \n>>>>> +\tint open();\n>>>>> +\tvoid close();\n>>>>> +\n>>>>> +\tint queryCap();\n>>>>> +\n>>>>> +private:\n>>>>> +\tstd::string device_;\n>>>>> +\tint fd_;\n>>>>> +\tint capabilities_;\n>>\n>> Ok, I'm saying it: \"What about reverse-xmas-tree order for variable\n>> declarations?\"\n>>\n>> Here, let's the bike-shedding begin\n> \n> It also makes sense to group them by purpose. I'd take the line size into \n> account only when no other criterion applies. In this case the order proposed \n> by Kieran makes sense to me.\n> \n> Please also note that we can't reorganize members freely as they generally \n> breaks binary compatibility (at least for classes exposed through the public \n> API).\n\nIn this instance they were essentially added in logical order of their\nusage.\n\nThe device is the string for what the object is. To me this goes first\nregardless.\nThen the FD is the handle to operate on the external object - so it's\nthe second most important variable.\n\nLeaving capabilities ... third :)\n\n\n\n>>>>> +};\n>>>>> +\n>>>>> +}\n>>>>\n>>>> nit:\n>>>> } /* namespace libcamera */\n>>>\n>>> Ah yes - that might be nice.\n>>>\n>>> How did I miss this?\n>>> Do we have this in our templates?\n>>>\n>>> Hrm ... nope, this end comment isn't there.\n>>>\n>>> I've added it to the template document.\n>>\n>> Oh thanks, I missed it was not there, but I think that rule comes from\n>> the C++ style guide cited in our coding style document.\n>>\n>>>>> +\n>>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */\n> \n>>>>> diff --git a/src/libcamera/v4l2_device.cpp\n>>>>> b/src/libcamera/v4l2_device.cpp\n>>>>> new file mode 100644\n>>>>> index 000000000000..eea2514f343c\n>>>>> --- /dev/null\n>>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>>> @@ -0,0 +1,137 @@\n>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2018, Google Inc.\n>>>>> + *\n>>>>> + * v4l2_device.cpp - V4L2 Device API\n>>>>> + */\n>>>>> +\n>>>>> +#include \n>>>>> +#include \n>>>>> +#include \n>>>>> +\n>>>>> +#include \n>>>>> +#include \n> \n> You can mix the five includes above, no need to separate sys/.\n\nUpdated.\n\n> \n>>>>> +#include \"log.h\"\n>>>>> +#include \"v4l2_device.h\"\n>>>>> +\n>>>>> +/**\n>>>>> + * \\file v4l2_device.cpp\n>>>>\n>>>> file v4l2_device.h to document here what is defined in the header file\n>>>\n>>> Oh - So this is supposed to reference the header not declare the current\n>>> file. I guess that kind of makes sense :) Always pointless to state the\n>>> name of the current file.\n>>\n>> It allows documenting the code in the cpp file where the implementation is,\n>> but to refer to definitions in the header file. I know, confusing.\n> \n> To be exact, it tells Doxygen to generate documentation for all classes, \n> functions and macros in the referenced file. We thus need a \\file statement \n> for the header, and no \\file statement should be needed for .cpp files as we \n> shouldn't define any API there (symbols that are fully private to a .cpp file \n> are internal and I don't think they need to appear in the generated \n> documentation).\n\nOk - updated.\n\n>>>>> + * \\brief V4L2 Device API\n>>>>> + */\n>>>>> +namespace libcamera {\n>>>>> +\n>>>>> +/**\n>>>>> + * \\class V4L2Device\n>>>>> + * \\brief V4L2Device object and API.\n>>>>> + *\n>>>>> + * The V4L2 Device API class models a single instance of a v4l2 device\n> \n> s/API //\n> s/a single instance/an instance/ ?\n\nChanged.\n\n> \n>>>>> node.\n>>>>> + */\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn V4L2Device::V4L2Device(device)\n>>>>\n>>>> nit: parameter type?\n>>>\n>>> I believe doxygen collects that automatically?\n>>>\n>>> It generates this in the output:\n>>> libcamera::V4L2Device::V4L2Device(const std::string & device)\n> \n> when the function isn't overloaded. But for functions defined in .cpp files \n> (as opposed as the inlined functions in the class definition), you can drop \n> the \\fn line completely, doxygen knows that the comment block is related to \n> the function that follows immediately.\n\nGreat - less lines is better :)\n\n\n> \n>> Oh really? That's nice, I did it in all functions and I could have\n>> avoided that...\n>>\n>>>>> + *\n>>>>> + * Default constructor for a V4L2Device object. The \\a device\n>>>>> specifies a\n>>>>> + * string to representation of the device object.\n>>>>\n>>>> My poor English skill do not allow me to say if this is a typo or it\n>>>> actually makes sense :)\n>>>\n>>> Ahem - there's certainly a typo there - good spot.\n>>>\n>>> It's also not necessarily correct, and is too close from my Camera class\n>>> constructor text.\n>>>\n>>> I'll adapt this to :\n>>> * Default constructor for a V4L2Device object. The \\a device specifies\n>>> * the path to the video device node.\n> \n> The default constructor would be V4L2Device::V4L2Device(), which you should = \n> delete in the header.\n\nGood point.\n\n\n>>>>> + */\n>>>>> +V4L2Device::V4L2Device(const std::string &device)\n> \n> \t: device_(device), fd_(fd), capabilities_(0)\n> \n>>>>> +{\n>>>>> +\tdevice_ = device;\n>>>>> +\tfd_ = -1;\n>>>>> +\tcapabilities_ = 0;\n>>>>> +\n>>>>> +\tLOG(Debug) << \"V4L2Device Constructed for \" << device_;\n>>>>\n>>>> Not sure a debug printout is worth here\n>>>\n>>> Certainly not for long... they're just here for early dev.\n>>>\n>>> I can just drop these - they're not useful.\n>>\n>> I think that would be better\n>>\n>>>>> +}\n>>>>> +\n>>>>> +V4L2Device::~V4L2Device()\n>>>>> +{\n>>>>> +\tclose();\n>>>>> +\n>>>>> +\tLOG(Debug) << \"V4L2Device Destroyed\";\n>>>>\n>>>> ditto\n>>>>\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn V4L2Device::isOpen(())\n>>>>\n>>>> one () in excess?\n>>>\n>>> Not sure how that happened?\n> \n> You can drop the \\fn line completely anyway (same for the functions below that \n> are defined in this file).\n> \n>>>>> + *\n>>>>> + * Checks to see if we have successfully opened a v4l2 video device.\n> \n> \\brief Check if the device is open\n> \\sa open()\n> \\return True if the device is open, closed otherwise\n> \n>>>>> + */\n>>>>> +bool V4L2Device::isOpen()\n>>>>> +{\n>>>>> +\treturn (fd_ != -1);\n>>>>\n>>>> nit: no need to () braces\n>>>\n>>> Can I keep them ?\n>>\n>> Sure!\n>>\n>>> Returning more than a simple variable or statement 'openly' feels\n>>> horrible to me ...\n> \n> You will however have to deal with all the code in libcamera that will make \n> you fell horrible, so maybe you'll need to get used to it ;-)\n\n\nBut that's \"Other peoples code\" :D\n\n\n> \n>> :)\n>>\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn V4L2Device::open()\n>>>>> + *\n>>>>> + * Opens a v4l2 device and queries properties from the device.\n> \n> \\brief and \\return. Same for the functions below.\n> \n>>>>> + */\n>>>>> +int V4L2Device::open()\n>>>>> +{\n>>>>> +\tint ret;\n>>>>> +\n>>>>> +\tif (isOpen()) {\n>>>>> +\t\tLOG(Error) << \"Device already open\";\n>>>>> +\t\treturn -EBADF;\n> \n> -EBUSY maybe ?\n\nSure.\n\n> \n>>>>> +\t}\n>>>>> +\n>>>>> +\tfd_ = ::open(device_.c_str(), O_RDWR);\n>>>>> +\tif (fd_ < 0) {\n> \n> \t\tret = -errno;\n> \n>>>>> +\t\tLOG(Error) << \"Failed to open V4L2 device \" << device_\n>>>>> +\t\t\t << \" : \" << strerror(errno);\n>>>>> +\t\treturn -errno;\n> \n> \t\t\t<< \" : \" << strerror(-ret);\n> \treturn ret;\n> \n>>>>> +\t}\n>>>>> +\n>>>>> +\tret = queryCap();\n>>>>\n>>>> I don't like it much either, but this might have been \"int ret = \"\n>>>\n>>> Hrm ... that certainly is late instantiation.\n>>>\n>>> Is that what we're going for ?\n>>\n>> That's how I would interpret\n>> https://google.github.io/styleguide/cppguide.html#Local_Variables\n>>\n>> It could be discussed though. Again, it is only worth discussing this\n>> minor things as we're at the beginning and trying to establish a well\n>> consistent code base\n> \n> For variables used throughout a function (ret or a i loop counter for \n> instance), I think declaring them at the top makes sense.\n\nMe too.\n\n\n> \n>>>>> +\tif (ret)\n>>>>> +\t\treturn ret;\n>>>>> +\n>>>>> +\tif (!(capabilities_ & V4L2_CAP_STREAMING)) {\n>>>>\n>>>> I wonder if it wouldn't be better to return the capabilities from\n>>>> queryCap, and assign them to the class member capabilities_ after this\n>>>> check here.\n>>>>\n>>>> Looking at the code I -knew- that the queryCap() function had modified\n>>>> the capabilities_ and then you're inspecting them here, but it's all a\n>>>> bit implicit to me...\n>>>\n>>> How will we determine if the ioctl call succeeded?\n>>>\n>>> (while avoiding exceptions?)\n>>\n>> Well, 'queryCap()' would return -1 (or what more appropriate, like the\n>> errno value)\n>>\n>>> Capabilities flag can return any free-form int value. (ok - so it's\n>>> restricted to the flags available in V4L2 ... but... that's very\n>>> function specific)\n>>\n>> Can it return values < 0 ?\n\nSorry - I should have said \"Can return any free-form unsigned int value.\n\nAnd that actually means that my capabilities_ is the wrong type.\n\nBut I think as per Laurent's suggestion - we could just cache the whole\ncapabilities object in our class - and provide helpers to interpret it\nif necessary.\n\n\n\n>>>>> +\t\tLOG(Error) << \"Device does not support streaming IO\";\n>>>>> +\t\treturn -EINVAL;\n>>>>> +\t}\n>>>>> +\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn V4L2Device::close()\n>>>>> + *\n>>>>> + * Close a v4l2 device handle.\n> \n> The file handle isn't exposed as part of the API, so I wouldn't mention it\n> here. You can just explain that is closes the device, releases any resource \n> acquired by open(), and that calling the rest of the API on a closed device \n> isn't allowed.\n\nOk.\n\n> \n>>>>> + */\n>>>>> +void V4L2Device::close()\n>>>>> +{\n>>>>> +\tif (fd_ < 0)\n>>>>> +\t\treturn;\n>>>>> +\n>>>>> +\t::close(fd_);\n>>>>> +\tfd_ = -1;\n>>>>> +}\n>>>>> +\n>>>>> +/**\n>>>>> + * \\fn V4L2Device::queryCap()\n>>>>> + *\n>>>>> + * Obtains the capabilities information from the V4L2 Device.\n>>>>> + */\n>>>>> +int V4L2Device::queryCap()\n>>>>> +{\n>>>>> +\tstruct v4l2_capability cap = {};\n>>>>> +\tint ret;\n>>>>> +\n>>>>> +\tif (!isOpen()) {\n>>>>> +\t\tLOG(Error) << \"Attempt to query unopened device\";\n>>>>> +\t\treturn -EBADF;\n>>>>> +\t}\n> \n> I think you should restrict queryCap() to be called only from the open() \n> function and make it private. You could then remove this check. Or, possibly, \n> even inline this function in open() as it would then be a single ioctl call.\n\nYes, it shouldn't change - so there is no need to call again later. And\nit's values should be cached...\n\nI'll inline this and simplify.\n\n\n> \n>>>>> +\tret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);\n>>>>> +\tif (ret < 0) {\n>>>>> +\t\tLOG(Error) << \"Failed to query device capabilities\";\n>>>>> +\t\treturn -1;\n>>>>\n>>>> It would be useful to return the errno number and possibly print it\n>>>> out with strerror\n>>>\n>>> Good point!\n>>>\n>>> Should this be -errno? Or just return ret?\n>>\n>> Returning 'ret' is equivalent to return -1. -errno is the appropriate\n>> one.\n>>\n>>> The more I think about it - the more I'd like to wrap the ioctl call in\n>>> the class (or more likely a Device base class) which will handle the\n>>> error checks...\n>>>\n>>> That could then be a parent class for your MediaDevice object too, and\n>>> the expected V4L2SubDevice ...\n>>>\n>>> What are your thoughts? Over-abstracting? or useful ...\n>>\n>> No, I think it is useful. Actually, more than the MediaDevice itself\n>> (which I expect to instantiate V4L2Devices and not extend it) I think\n>> it might be worth implementing a V4L2Object that provides protected\n>> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device\n>> and V4L2Subdevice sub-class it.\n> \n> I think it's a bit of an overabstraction :-) I believe there will be very \n> little opportunities to share code between V4l2Device, V4l2Subdevice and \n> MediaDevice. I recommend developing them as separate objects, and then \n> creating a common base (or common helper functions) later if we realize enough \n> code is duplicated.\n> \n>> I'm not sure right now how I would model that, if the base class should\n>> do things like \"setFormat()\" or either provides an \"ioctl()\" function and\n>> have sub-classes implement the \"setFormat()\" logic. This boils down to\n>> how different that would be the implementation between a device and a\n>> subdevice. What do you think?\n> \n> The format setting model for V4L2 device and V4L2 subdevices is fundamentally \n> different. Subdevices take an extra pad number argument, and they propagate \n> formats internally within the subdevice, which creates a set of \"subdev \n> configuration\" objects internally in the kernel (a global one for the active \n> parameters and one per file handle for the test parameters). We will have to \n> expose this one way or another to implement trying formats, and I expect the \n> V4l2Device and V4l2Subdevice objects to offer different APIs for that reason.\n> \n>>>>> +\t}\n>>>>> +\n>>>>> +\tcapabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS\n>>>>> +\t\t ? cap.device_caps : cap.capabilities;\n> \n> How about caching the full v4l2_capability structure in V4L2Device ?\n\nI'm trying this now - and caching the whole structure has led me to\nwrapping the capabilities structure in a calss to provide helpers such\nas isCapture() isMplane() hasStreaming() which I think makes sense to\nkeeping all the handling of the capabilities API in one place.\n\nA C++ Class inheriting a C structure seems a nice way to wrap our kernel\nstructure objects.. :)\n\n>>>>> +\treturn 0;\n>>>>> +}\n>>>>> +\n>>>>> +} /* namespace libcamera */\n>>>>> diff --git a/test/meson.build b/test/meson.build\n>>>>> index 754527324c7d..2bc76c289eea 100644\n>>>>> --- a/test/meson.build\n>>>>> +++ b/test/meson.build\n>>>>> @@ -12,6 +12,8 @@ test_includes_internal = [\n>>>>> libcamera_internal_includes,\n>>>>> ]\n>>>>>\n>>>>> +subdir('v4l2_device')\n>>>>> +\n>>>>> public_tests = [\n>>>>> [ 'test_init', 'init.cpp' ],\n>>>>\n>>>> Not related to this, but weird spacing here\n>>>\n>>> This was intentional ... pre-allocation of table spacing. (The table\n>>> will have multiple entries...)\n>>\n>> Why would you pre-allocate this?\n>>\n>>>>> ]\n>>>>> diff --git a/test/v4l2_device/double_open.cpp\n>>>>> b/test/v4l2_device/double_open.cpp new file mode 100644\n>>>>> index 000000000000..1b7c7bfe14b8\n>>>>> --- /dev/null\n>>>>> +++ b/test/v4l2_device/double_open.cpp\n>>>>> @@ -0,0 +1,32 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2018, Google Inc.\n>>>>> + *\n>>>>> + * libcamera V4L2 API tests\n>>>>> + */\n>>>>> +\n>>>>> +#include \"v4l2_device_test.h\"\n>>>>> +\n>>>>> +class DoubleOpen : public V4L2DeviceTest\n> \n> Maybe V4L2DeviceDoubleOpenTest to avoid namespace clashes when we'll compile \n> multiple unit tests into a single binary for a test suite ? Or maybe using an \n> anynomous namespace for this purpose ?\n\nAn anonymous namespace sounds good.\nThese files should essentially be closed namespace I think.\n\n\n> \n>>>>> +{\n>>>>> +protected:\n>>>>> +\tint run()\n>>>>> +\t{\n>>>>> +\t\tint ret;\n>>>>> +\n>>>>> +\t\t/*\n>>>>> +\t\t * Expect failure: The device has already been opened by the\n>>>>> +\t\t * V4L2DeviceTest base class\n>>>>> +\t\t */\n>>>>> +\t\tret = dev->open();\n>>>>> +\t\tif (!ret) {\n>>>>> +\t\t\tfprintf(stderr, \"Double open erroneously succeeded\\n\");\n> \n> cout/cerr ?\n> \n>>>>> +\t\t\tdev->close();\n>>>>> +\t\t\treturn TEST_FAIL;\n>>>>> +\t\t}\n>>>>> +\n>>>>> +\t\treturn TEST_PASS;\n>>>>> +\t}\n>>>>\n>>>> nit: < one indent level\n>>>\n>>> Oh -- for all class functions?\n>>\n>> Oh damn, I got confused by having the full implementation here.\n>>\n>> For 'big' functions like this one, we don't want to have inlined by\n>> implementing them in the definition. Now, I'm struggling to find a\n>> final answer to the question \"Are functions implemented in the class\n>> definition automatically inlined?\" I bet the compiler is smart enough\n>> to find out if it makes sense to inline or not a function even when\n>> its implementation is in the header file, but in general you could define\n>> the class, and implement the function outside of the definition, even\n>> if in the same cpp file.\n>>\n>>>>> +};\n>>>>\n>>>> I welcome testing, but do we really need tests to make sure a function\n>>>> fails when called twice?\n>>>\n>>> Yes - I felt this was important.\n>>>\n>>> It's there because once I added the open() and close() calls - it\n>>> provided a way to leak fd's if you call open(), open().\n>>>\n>>> The first fd would get silently dropped ...\n>>>\n>>> So I added a test to call open twice, then I added the isOpen() checks.\n>>>\n>>> In other words - the fix for the leak came about directly because I\n>>> added the double_open test...\n>>\n>> Fine then, thanks for the explanation.\n>>\n>>> Another reason we I think we should base class the common device\n>>> operations (then this test would only apply to the Device object unit\n>>> tests anyway.)\n>>>\n>>>> I'm sure you could have sketched it by\n>>>> calling open twice in the V4L2DeviceTest class\n>>>\n>>> No - the V4L2DeviceTest class is the base class common to all further\n>>> tests for the V4L2Device Object.\n>>>\n>>> I have other patches which add further tests using the V4L2DeviceTest\n>>> base class.\n>>>\n>>>>> +\n>>>>> +TEST_REGISTER(DoubleOpen);\n>>>>> diff --git a/test/v4l2_device/meson.build\n>>>>> b/test/v4l2_device/meson.build\n>>>>> new file mode 100644\n>>>>> index 000000000000..41675a303498\n>>>>> --- /dev/null\n>>>>> +++ b/test/v4l2_device/meson.build\n>>>>> @@ -0,0 +1,12 @@\n>>>>> +# Tests are listed in order of complexity.\n>>>>> +# They are not alphabetically sorted.\n>>>>> +v4l2_device_tests = [\n>>>>> + [ 'double_open', 'double_open.cpp' ],\n>>>>\n>>>> Again weird spacing, I'm now thinking this is intentional and I don't\n>>>> know something\n>>>\n>>> Table pre-allocation for multiple tests.\n>>>\n>>> This isn't my only test, and I'd rather not have to reformat the table\n>>> if I add tests with longer filenames.\n>>>\n>>>>> +]\n>>>>> +\n>>>>> +foreach t : v4l2_device_tests\n>>>>> + exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],\n>>>>> +\t\t link_with : test_libraries,\n>>>>> +\t\t include_directories : test_includes_internal)\n>>>>> + test(t[0], exe, suite: 'v4l2_device', is_parallel: false)\n>>>>> +endforeach\n>>>>> diff --git a/test/v4l2_device/v4l2_device_test.cpp\n>>>>> b/test/v4l2_device/v4l2_device_test.cpp new file mode 100644\n>>>>> index 000000000000..ae317a9519c5\n>>>>> --- /dev/null\n>>>>> +++ b/test/v4l2_device/v4l2_device_test.cpp\n>>>>> @@ -0,0 +1,36 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2018, Google Inc.\n>>>>> + *\n>>>>> + * libcamera V4L2 API tests\n>>>>> + */\n>>>>> +\n>>>>> +#include \"v4l2_device_test.h\"\n>>>>> +\n>>>>> +using namespace libcamera;\n>>>>> +\n>>>>> +V4L2DeviceTest::V4L2DeviceTest()\n> \n> \t: dev_(nullptr)\n\nFixed.\n\n\n> \n>>>>> +{\n>>>>> +\tdev = nullptr;\n>>>>> +}\n>>>>> +\n>>>>> +int V4L2DeviceTest::init()\n>>>>> +{\n>>>>> +\tconst char *device = \"/dev/video0\";\n> \n> \tstd::string device(\"/dev/video0\");\n> \n>>>>> +\n>>>>> +\t/* Validate the device node exists. */\n>>>>> +\tif (!path_exists(device))\n> \n> Please log a message to explain why the test is skipped.\n\nDone.\n\n> \n>>>>> +\t\treturn TEST_SKIP;\n>>>>> +\n>>>>> +\tdev = new V4L2Device(device);\n>>>>> +\tif (!dev)\n>>>>\n>>>> You should call cleanup here if you fail\n>>>> Ah no, the Test class should call it if init() fails!\n>>>> Ah no^2, your constructor cannot fail (good!). My previous comment\n>>>> still hold though.\n>>>\n>>> Should I instead call cleanup() from my destructor ?\n>>\n>> My point was that the object would not get destroyed here. Because the\n>> test base class does not call cleanup() after init fails\n>> and you don't not call it either. But I was confused actually, as the\n>> base test class does this\n>>\n>> +#define TEST_REGISTER(klass) \\\n>> +int main(int argc, char *argv[]) \\\n>> +{ \\\n>> + return klass().execute(); \\\n>> +}\n>>\n>> And this creates and destroys the object, which calls your destructor,\n>> so I -guess- this is fine actually.\n>> Not calling 'cleaup()' after a failed init is being discussed now\n>> elsewhere.\n>>\n>> Anyway, your constructor cannot fail, so you could remove that\n>> check...\n>>\n>>>>> +\t\treturn TEST_FAIL;\n>>>>> +\n>>>>> +\treturn dev->open();\n>>>>> +}\n>>>>> +\n>>>>> +void V4L2DeviceTest::cleanup()\n>>>>> +{\n>>>>> +\tif (dev)\n>>>>> +\t\tdelete dev;\n> \n> Can't you safely delete nullptr ?\n\nYes ;)\n\n\n> \n>>>>> +};\n>>>>> diff --git a/test/v4l2_device/v4l2_device_test.h\n>>>>> b/test/v4l2_device/v4l2_device_test.h new file mode 100644\n>>>>> index 000000000000..4374ddc7e932\n>>>>> --- /dev/null\n>>>>> +++ b/test/v4l2_device/v4l2_device_test.h\n>>>>> @@ -0,0 +1,31 @@\n>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>>> +/*\n>>>>> + * Copyright (C) 2018, Google Inc.\n>>>>> + *\n>>>>> + * vl42device_test.h - libcamera v4l2device test base class\n>>>>> + */\n>>>>> +\n>>>>> +#include \"test.h\"\n>>>>> +#include \"v4l2_device.h\"\n>>>>\n>>>> Includes after the inclusion guards\n>>>\n>>> Ack.\n>>>\n>>>>> +\n>>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_\n>>>>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_\n> \n>>>>> +\n>>>>> +using namespace libcamera;\n>>>>> +\n>>>>> +class V4L2DeviceTest : public Test\n>>>>> +{\n>>>>> +public:\n>>>>> +\tV4L2DeviceTest();\n>>>>> +\tvirtual ~V4L2DeviceTest() {};\n> \n> Is this needed, given that the base class already has a virtual destructor ?\n\nRemoved.\n\n> \n>>>>> +\n>>>>> +protected:\n>>>>> +\tint init();\n>>>>> +\tvoid cleanup();\n>>>>> +\n>>>>> +\tvirtual int run() = 0;\n> \n> I don't think this is needed either.\n\nRemoved.\n\n> \n>>>>> +\tV4L2Device *dev;\n> \n> s/dev/dev_/\n\nFixed.\n\n\n> \n>>>>> +};\n>>>>> +\n>>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */\n> \n\nThanks.","headers":{"Return-Path":"","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 CD74160C83\n\tfor ;\n\tMon, 14 Jan 2019 15:57:50 +0100 (CET)","from [192.168.0.21]\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 C9A7A530;\n\tMon, 14 Jan 2019 15:57:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1547477870;\n\tbh=Ddzvvyw0G1YgiMbmLr2s9zXcG/nnToH0x7YoJfh9qtQ=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=rraJNS3zMOOAUWLXUb3khBY6+c8GGlqKI05XfD+kPad4B4QSF2NlbGTWI4R+/qhFv\n\tQJ/PExmvueIZNZr/i+kW2IhY5X2KM31lDr3/2OFuna1EFTtOltIsXGuoDFNelhM/A1\n\tfTZZ4MTfvi0RPiTfoTYIlNpfouZHdRljk7a1ILF8=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20181221123724.27290-1-kieran.bingham@ideasonboard.com>\n\t\n\t<20181223205409.GB13765@archlinux> <5695091.PZBZBohWtT@avalon>","From":"Kieran Bingham ","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<3b8249f4-81d0-9541-9d61-b8515a89642d@ideasonboard.com>","Date":"Mon, 14 Jan 2019 14:57:47 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.2.1","MIME-Version":"1.0","In-Reply-To":"<5695091.PZBZBohWtT@avalon>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object","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":"Mon, 14 Jan 2019 14:57:52 -0000"}}]