[libcamera-devel,2/2] lib: Add V4L2 Device object

75 diff mbox series

Provide a helper V4L2 device object capable of interacting with the
V4L2 Linux Kernel APIs.

A test suite is added at test/v4l2_device/ to utilise and validate the
code base.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/libcamera/include/v4l2_device.h   |  36 +++++++
 src/libcamera/meson.build             |   2 +
 src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
 test/meson.build                      |   2 +
 test/v4l2_device/double_open.cpp      |  32 ++++++
 test/v4l2_device/meson.build          |  12 +++
 test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
 test/v4l2_device/v4l2_device_test.h   |  31 ++++++
 8 files changed, 288 insertions(+)
 create mode 100644 src/libcamera/include/v4l2_device.h
 create mode 100644 src/libcamera/v4l2_device.cpp
 create mode 100644 test/v4l2_device/double_open.cpp
 create mode 100644 test/v4l2_device/meson.build
 create mode 100644 test/v4l2_device/v4l2_device_test.cpp
 create mode 100644 test/v4l2_device/v4l2_device_test.h

Hi Kieran,
    thanks for the patch

On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
> Provide a helper V4L2 device object capable of interacting with the
> V4L2 Linux Kernel APIs.
>
> A test suite is added at test/v4l2_device/ to utilise and validate the
> code base.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/libcamera/include/v4l2_device.h   |  36 +++++++
>  src/libcamera/meson.build             |   2 +
>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++

I would really split test and library code in 2 differen commits.

>  test/meson.build                      |   2 +
>  test/v4l2_device/double_open.cpp      |  32 ++++++
>  test/v4l2_device/meson.build          |  12 +++
>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
>  8 files changed, 288 insertions(+)
>  create mode 100644 src/libcamera/include/v4l2_device.h
>  create mode 100644 src/libcamera/v4l2_device.cpp
>  create mode 100644 test/v4l2_device/double_open.cpp
>  create mode 100644 test/v4l2_device/meson.build
>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
>  create mode 100644 test/v4l2_device/v4l2_device_test.h
>
> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> new file mode 100644
> index 000000000000..619d932d3c82
> --- /dev/null
> +++ b/src/libcamera/include/v4l2_device.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * v4l2_device.h - V4L2 Device API Abstractions
> + */
> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> +#define __LIBCAMERA_V4L2_DEVICE_H__
> +
> +#include <string>
> +
> +#include <linux/videodev2.h>
> +
> +namespace libcamera {
> +
> +class V4L2Device
> +{
> +public:
> +	V4L2Device(const std::string &);
> +	~V4L2Device();
> +
> +	bool isOpen();
> +	int open();
> +	void close();
> +
> +	int queryCap();
> +
> +private:
> +	std::string device_;
> +	int fd_;
> +	int capabilities_;
> +};
> +
> +}

nit:
} /* namespace libcamera */

> +
> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index f632eb5dd779..bbaf9a05ec2c 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,11 +1,13 @@
>  libcamera_sources = files([
>      'log.cpp',
>      'main.cpp',
> +    'v4l2_device.cpp',
>  ])
>
>  libcamera_headers = files([
>      'include/log.h',
>      'include/utils.h',
> +    'include/v4l2_device.h',
>  ])
>
>  libcamera_internal_includes =  include_directories('include')
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> new file mode 100644
> index 000000000000..eea2514f343c
> --- /dev/null
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -0,0 +1,137 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * v4l2_device.cpp - V4L2 Device API
> + */
> +
> +#include <fcntl.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +
> +#include "log.h"
> +#include "v4l2_device.h"
> +
> +/**
> + * \file v4l2_device.cpp

file v4l2_device.h to document here what is defined in the header file

> + * \brief V4L2 Device API
> + */
> +namespace libcamera {
> +
> +/**
> + * \class V4L2Device
> + * \brief V4L2Device object and API.
> + *
> + * The V4L2 Device API class models a single instance of a v4l2 device node.
> + */
> +
> +/**
> + * \fn V4L2Device::V4L2Device(device)

nit: parameter type?

> + *
> + * Default constructor for a V4L2Device object. The \a device specifies a
> + * string to representation of the device object.

My poor English skill do not allow me to say if this is a typo or it
actually makes sense :)

> + */
> +V4L2Device::V4L2Device(const std::string &device)
> +{
> +	device_ = device;
> +	fd_ = -1;
> +	capabilities_ = 0;
> +
> +	LOG(Debug) << "V4L2Device Constructed for " << device_;

Not sure a debug printout is worth here

> +}
> +
> +V4L2Device::~V4L2Device()
> +{
> +	close();
> +
> +	LOG(Debug) << "V4L2Device Destroyed";

ditto

> +}
> +
> +/**
> + * \fn V4L2Device::isOpen(())

one () in excess?

> + *
> + * Checks to see if we have successfully opened a v4l2 video device.
> + */
> +bool V4L2Device::isOpen()
> +{
> +	return (fd_ != -1);

nit: no need to () braces
> +}
> +
> +/**
> + * \fn V4L2Device::open()
> + *
> + * Opens a v4l2 device and queries properties from the device.
> + */
> +int V4L2Device::open()
> +{
> +	int ret;
> +
> +	if (isOpen()) {
> +		LOG(Error) << "Device already open";
> +		return -EBADF;
> +	}
> +
> +	fd_ = ::open(device_.c_str(), O_RDWR);
> +	if (fd_ < 0) {
> +		LOG(Error) << "Failed to open V4L2 device " << device_
> +			   << " : " << strerror(errno);
> +		return -errno;
> +	}
> +
> +	ret = queryCap();

I don't like it much either, but this might have been "int ret = "

> +	if (ret)
> +		return ret;
> +
> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {

I wonder if it wouldn't be better to return the capabilities from
queryCap, and assign them to the class member capabilities_ after this
check here.

Looking at the code I -knew- that the queryCap() function had modified
the capabilities_ and then you're inspecting them here, but it's all a
bit implicit to me...

> +		LOG(Error) << "Device does not support streaming IO";
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * \fn V4L2Device::close()
> + *
> + * Close a v4l2 device handle.
> + */
> +void V4L2Device::close()
> +{
> +	if (fd_ < 0)
> +		return;
> +
> +	::close(fd_);
> +	fd_ = -1;
> +}
> +
> +/**
> + * \fn V4L2Device::queryCap()
> + *
> + * Obtains the capabilities information from the V4L2 Device.
> + */
> +int V4L2Device::queryCap()
> +{
> +	struct v4l2_capability cap = {};
> +	int ret;
> +
> +	if (!isOpen()) {
> +		LOG(Error) << "Attempt to query unopened device";
> +		return -EBADF;
> +	}
> +
> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
> +	if (ret < 0) {
> +		LOG(Error) << "Failed to query device capabilities";
> +		return -1;

It would be useful to return the errno number and possibly print it
out with strerror

> +	}
> +
> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
> +		      ? cap.device_caps : cap.capabilities;
> +
> +	return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/test/meson.build b/test/meson.build
> index 754527324c7d..2bc76c289eea 100644
> --- a/test/meson.build
> +++ b/test/meson.build
> @@ -12,6 +12,8 @@ test_includes_internal = [
>    libcamera_internal_includes,
>  ]
>
> +subdir('v4l2_device')
> +
>  public_tests = [
>    [ 'test_init',      'init.cpp' ],

Not related to this, but weird spacing here

>  ]
> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp
> new file mode 100644
> index 000000000000..1b7c7bfe14b8
> --- /dev/null
> +++ b/test/v4l2_device/double_open.cpp
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * libcamera V4L2 API tests
> + */
> +
> +#include "v4l2_device_test.h"
> +
> +class DoubleOpen : public V4L2DeviceTest
> +{
> +protected:
> +	int run()
> +	{
> +		int ret;
> +
> +		/*
> +		 * Expect failure: The device has already been opened by the
> +		 * V4L2DeviceTest base class
> +		 */
> +		ret = dev->open();
> +		if (!ret) {
> +			fprintf(stderr, "Double open erroneously succeeded\n");
> +			dev->close();
> +			return TEST_FAIL;
> +		}
> +
> +		return TEST_PASS;
> +	}

nit: < one indent level

> +};

I welcome testing, but do we really need tests to make sure a function
fails when called twice? I'm sure you could have sketched it by
calling open twice in the V4L2DeviceTest class

> +
> +TEST_REGISTER(DoubleOpen);
> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
> new file mode 100644
> index 000000000000..41675a303498
> --- /dev/null
> +++ b/test/v4l2_device/meson.build
> @@ -0,0 +1,12 @@
> +# Tests are listed in order of complexity.
> +# They are not alphabetically sorted.
> +v4l2_device_tests = [
> +  [ 'double_open',        'double_open.cpp' ],

Again weird spacing, I'm now thinking this is intentional and I don't
know something

> +]
> +
> +foreach t : v4l2_device_tests
> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
> +		   link_with : test_libraries,
> +		   include_directories : test_includes_internal)
> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
> +endforeach
> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> new file mode 100644
> index 000000000000..ae317a9519c5
> --- /dev/null
> +++ b/test/v4l2_device/v4l2_device_test.cpp
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * libcamera V4L2 API tests
> + */
> +
> +#include "v4l2_device_test.h"
> +
> +using namespace libcamera;
> +
> +V4L2DeviceTest::V4L2DeviceTest()
> +{
> +	dev = nullptr;
> +}
> +
> +int V4L2DeviceTest::init()
> +{
> +	const char *device = "/dev/video0";
> +
> +	/* Validate the device node exists. */
> +	if (!path_exists(device))
> +		return TEST_SKIP;
> +
> +	dev = new V4L2Device(device);
> +	if (!dev)

You should call cleanup here if you fail
Ah no, the Test class should call it if init() fails!
Ah no^2, your constructor cannot fail (good!). My previous comment
still hold though.

> +		return TEST_FAIL;
> +
> +	return dev->open();
> +}
> +
> +void V4L2DeviceTest::cleanup()
> +{
> +	if (dev)
> +		delete dev;
> +};
> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h
> new file mode 100644
> index 000000000000..4374ddc7e932
> --- /dev/null
> +++ b/test/v4l2_device/v4l2_device_test.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * vl42device_test.h - libcamera v4l2device test base class
> + */
> +
> +#include "test.h"
> +#include "v4l2_device.h"

Includes after the inclusion guards

> +
> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_
> +
> +using namespace libcamera;
> +
> +class V4L2DeviceTest : public Test
> +{
> +public:
> +	V4L2DeviceTest();
> +	virtual ~V4L2DeviceTest() {};
> +
> +protected:
> +	int init();
> +	void cleanup();
> +
> +	virtual int run() = 0;
> +
> +	V4L2Device *dev;
> +};
> +
> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
> --
> 2.17.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thanks for the review,

On 21/12/2018 16:25, jacopo mondi wrote:
> Hi Kieran,
>     thanks for the patch
> 
> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
>> Provide a helper V4L2 device object capable of interacting with the
>> V4L2 Linux Kernel APIs.
>>
>> A test suite is added at test/v4l2_device/ to utilise and validate the
>> code base.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>  src/libcamera/include/v4l2_device.h   |  36 +++++++
>>  src/libcamera/meson.build             |   2 +
>>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
> 
> I would really split test and library code in 2 differen commits.

I'm happy to go with group consensus here, but I felt that tests and
code could be in the same commit here, as the test directly affects the
code added.

This means that when doing rebase actions or such, I can do 'git rebase
-i -x "ninja; ninja test"' and validate every commit as I rework things.

(I have a script which does that for me)


>>  test/meson.build                      |   2 +
>>  test/v4l2_device/double_open.cpp      |  32 ++++++
>>  test/v4l2_device/meson.build          |  12 +++
>>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
>>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
>>  8 files changed, 288 insertions(+)
>>  create mode 100644 src/libcamera/include/v4l2_device.h
>>  create mode 100644 src/libcamera/v4l2_device.cpp
>>  create mode 100644 test/v4l2_device/double_open.cpp
>>  create mode 100644 test/v4l2_device/meson.build
>>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
>>  create mode 100644 test/v4l2_device/v4l2_device_test.h
>>
>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>> new file mode 100644
>> index 000000000000..619d932d3c82
>> --- /dev/null
>> +++ b/src/libcamera/include/v4l2_device.h
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * v4l2_device.h - V4L2 Device API Abstractions
>> + */
>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
>> +#define __LIBCAMERA_V4L2_DEVICE_H__
>> +
>> +#include <string>
>> +
>> +#include <linux/videodev2.h>
>> +
>> +namespace libcamera {
>> +
>> +class V4L2Device
>> +{
>> +public:
>> +	V4L2Device(const std::string &);
>> +	~V4L2Device();
>> +
>> +	bool isOpen();
>> +	int open();
>> +	void close();
>> +
>> +	int queryCap();
>> +
>> +private:
>> +	std::string device_;
>> +	int fd_;
>> +	int capabilities_;
>> +};
>> +
>> +}
> 
> nit:
> } /* namespace libcamera */
> 

Ah yes - that might be nice.

How did I miss this?
Do we have this in our templates?

Hrm ... nope, this end comment isn't there.

I've added it to the template document.


>> +
>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index f632eb5dd779..bbaf9a05ec2c 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -1,11 +1,13 @@
>>  libcamera_sources = files([
>>      'log.cpp',
>>      'main.cpp',
>> +    'v4l2_device.cpp',
>>  ])
>>
>>  libcamera_headers = files([
>>      'include/log.h',
>>      'include/utils.h',
>> +    'include/v4l2_device.h',
>>  ])
>>
>>  libcamera_internal_includes =  include_directories('include')
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> new file mode 100644
>> index 000000000000..eea2514f343c
>> --- /dev/null
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -0,0 +1,137 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * v4l2_device.cpp - V4L2 Device API
>> + */
>> +
>> +#include <fcntl.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +
>> +#include "log.h"
>> +#include "v4l2_device.h"
>> +
>> +/**
>> + * \file v4l2_device.cpp
> 
> file v4l2_device.h to document here what is defined in the header file

Oh - So this is supposed to reference the header not declare the current
file. I guess that kind of makes sense :) Always pointless to state the
name of the current file.

> 
>> + * \brief V4L2 Device API
>> + */
>> +namespace libcamera {
>> +
>> +/**
>> + * \class V4L2Device
>> + * \brief V4L2Device object and API.
>> + *
>> + * The V4L2 Device API class models a single instance of a v4l2 device node.
>> + */
>> +
>> +/**
>> + * \fn V4L2Device::V4L2Device(device)
> 
> nit: parameter type?

I believe doxygen collects that automatically?

It generates this in the output:
  libcamera::V4L2Device::V4L2Device(const std::string & device)


>> + *
>> + * Default constructor for a V4L2Device object. The \a device specifies a
>> + * string to representation of the device object.
> 
> My poor English skill do not allow me to say if this is a typo or it
> actually makes sense :)

Ahem - there's certainly a typo there - good spot.

It's also not necessarily correct, and is too close from my Camera class
constructor text.

I'll adapt this to :

 * Default constructor for a V4L2Device object. The \a device specifies
 * the path to the video device node.

> 
>> + */
>> +V4L2Device::V4L2Device(const std::string &device)
>> +{
>> +	device_ = device;
>> +	fd_ = -1;
>> +	capabilities_ = 0;
>> +
>> +	LOG(Debug) << "V4L2Device Constructed for " << device_;
> 
> Not sure a debug printout is worth here

Certainly not for long... they're just here for early dev.

I can just drop these - they're not useful.

> 
>> +}
>> +
>> +V4L2Device::~V4L2Device()
>> +{
>> +	close();
>> +
>> +	LOG(Debug) << "V4L2Device Destroyed";
> 
> ditto
> 
>> +}
>> +
>> +/**
>> + * \fn V4L2Device::isOpen(())
> 
> one () in excess?

Not sure how that happened?

> 
>> + *
>> + * Checks to see if we have successfully opened a v4l2 video device.
>> + */
>> +bool V4L2Device::isOpen()
>> +{
>> +	return (fd_ != -1);
> 
> nit: no need to () braces

Can I keep them ?

Returning more than a simple variable or statement 'openly' feels
horrible to me ...


>> +}
>> +
>> +/**
>> + * \fn V4L2Device::open()
>> + *
>> + * Opens a v4l2 device and queries properties from the device.
>> + */
>> +int V4L2Device::open()
>> +{
>> +	int ret;
>> +
>> +	if (isOpen()) {
>> +		LOG(Error) << "Device already open";
>> +		return -EBADF;
>> +	}
>> +
>> +	fd_ = ::open(device_.c_str(), O_RDWR);
>> +	if (fd_ < 0) {
>> +		LOG(Error) << "Failed to open V4L2 device " << device_
>> +			   << " : " << strerror(errno);
>> +		return -errno;
>> +	}
>> +
>> +	ret = queryCap();
> 
> I don't like it much either, but this might have been "int ret = "

Hrm ... that certainly is late instantiation.

Is that what we're going for ?


> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {
> 
> I wonder if it wouldn't be better to return the capabilities from
> queryCap, and assign them to the class member capabilities_ after this
> check here.
> 
> Looking at the code I -knew- that the queryCap() function had modified
> the capabilities_ and then you're inspecting them here, but it's all a
> bit implicit to me...

How will we determine if the ioctl call succeeded?
 (while avoiding exceptions?)

Capabilities flag can return any free-form int value. (ok - so it's
restricted to the flags available in V4L2 ... but... that's very
function specific)



>> +		LOG(Error) << "Device does not support streaming IO";
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \fn V4L2Device::close()
>> + *
>> + * Close a v4l2 device handle.
>> + */
>> +void V4L2Device::close()
>> +{
>> +	if (fd_ < 0)
>> +		return;
>> +
>> +	::close(fd_);
>> +	fd_ = -1;
>> +}
>> +
>> +/**
>> + * \fn V4L2Device::queryCap()
>> + *
>> + * Obtains the capabilities information from the V4L2 Device.
>> + */
>> +int V4L2Device::queryCap()
>> +{
>> +	struct v4l2_capability cap = {};
>> +	int ret;
>> +
>> +	if (!isOpen()) {
>> +		LOG(Error) << "Attempt to query unopened device";
>> +		return -EBADF;
>> +	}
>> +
>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
>> +	if (ret < 0) {
>> +		LOG(Error) << "Failed to query device capabilities";
>> +		return -1;
> 
> It would be useful to return the errno number and possibly print it
> out with strerror

Good point!

Should this be -errno? Or just return ret?


The more I think about it - the more I'd like to wrap the ioctl call in
the class (or more likely a Device base class) which will handle the
error checks...

That could then be a parent class for your MediaDevice object too, and
the expected V4L2SubDevice ...

What are your thoughts? Over-abstracting? or useful ...


> 
>> +	}
>> +
>> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
>> +		      ? cap.device_caps : cap.capabilities;
>> +
>> +	return 0;
>> +}
>> +
>> +} /* namespace libcamera */
>> diff --git a/test/meson.build b/test/meson.build
>> index 754527324c7d..2bc76c289eea 100644
>> --- a/test/meson.build
>> +++ b/test/meson.build
>> @@ -12,6 +12,8 @@ test_includes_internal = [
>>    libcamera_internal_includes,
>>  ]
>>
>> +subdir('v4l2_device')
>> +
>>  public_tests = [
>>    [ 'test_init',      'init.cpp' ],
> 
> Not related to this, but weird spacing here

This was intentional ... pre-allocation of table spacing. (The table
will have multiple entries...)

> 
>>  ]
>> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp
>> new file mode 100644
>> index 000000000000..1b7c7bfe14b8
>> --- /dev/null
>> +++ b/test/v4l2_device/double_open.cpp
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + */
>> +
>> +#include "v4l2_device_test.h"
>> +
>> +class DoubleOpen : public V4L2DeviceTest
>> +{
>> +protected:
>> +	int run()
>> +	{
>> +		int ret;
>> +
>> +		/*
>> +		 * Expect failure: The device has already been opened by the
>> +		 * V4L2DeviceTest base class
>> +		 */
>> +		ret = dev->open();
>> +		if (!ret) {
>> +			fprintf(stderr, "Double open erroneously succeeded\n");
>> +			dev->close();
>> +			return TEST_FAIL;
>> +		}
>> +
>> +		return TEST_PASS;
>> +	}
> 
> nit: < one indent level

Oh -- for all class functions?


>> +};
> 
> I welcome testing, but do we really need tests to make sure a function
> fails when called twice? 

Yes - I felt this was important.

It's there because once I added the open() and close() calls - it
provided a way to leak fd's if you call open(), open().

The first fd would get silently dropped ...

So I added a test to call open twice, then I added the isOpen() checks.

In other words - the fix for the leak came about directly because I
added the double_open test...


Another reason we I think we should base class the common device
operations (then this test would only apply to the Device object unit
tests anyway.)


> I'm sure you could have sketched it by
> calling open twice in the V4L2DeviceTest class

No - the V4L2DeviceTest class is the base class common to all further
tests for the V4L2Device Object.

I have other patches which add further tests using the V4L2DeviceTest
base class.

>> +
>> +TEST_REGISTER(DoubleOpen);
>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
>> new file mode 100644
>> index 000000000000..41675a303498
>> --- /dev/null
>> +++ b/test/v4l2_device/meson.build
>> @@ -0,0 +1,12 @@
>> +# Tests are listed in order of complexity.
>> +# They are not alphabetically sorted.
>> +v4l2_device_tests = [
>> +  [ 'double_open',        'double_open.cpp' ],
> 
> Again weird spacing, I'm now thinking this is intentional and I don't
> know something

Table pre-allocation for multiple tests.

This isn't my only test, and I'd rather not have to reformat the table
if I add tests with longer filenames.

> 
>> +]
>> +
>> +foreach t : v4l2_device_tests
>> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
>> +		   link_with : test_libraries,
>> +		   include_directories : test_includes_internal)
>> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
>> +endforeach
>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
>> new file mode 100644
>> index 000000000000..ae317a9519c5
>> --- /dev/null
>> +++ b/test/v4l2_device/v4l2_device_test.cpp
>> @@ -0,0 +1,36 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * libcamera V4L2 API tests
>> + */
>> +
>> +#include "v4l2_device_test.h"
>> +
>> +using namespace libcamera;
>> +
>> +V4L2DeviceTest::V4L2DeviceTest()
>> +{
>> +	dev = nullptr;
>> +}
>> +
>> +int V4L2DeviceTest::init()
>> +{
>> +	const char *device = "/dev/video0";
>> +
>> +	/* Validate the device node exists. */
>> +	if (!path_exists(device))
>> +		return TEST_SKIP;
>> +
>> +	dev = new V4L2Device(device);
>> +	if (!dev)
> 
> You should call cleanup here if you fail
> Ah no, the Test class should call it if init() fails!
> Ah no^2, your constructor cannot fail (good!). My previous comment
> still hold though.
> 

Should I instead call cleanup() from my destructor ?



>> +		return TEST_FAIL;
>> +
>> +	return dev->open();
>> +}
>> +
>> +void V4L2DeviceTest::cleanup()
>> +{
>> +	if (dev)
>> +		delete dev;
>> +};
>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h
>> new file mode 100644
>> index 000000000000..4374ddc7e932
>> --- /dev/null
>> +++ b/test/v4l2_device/v4l2_device_test.h
>> @@ -0,0 +1,31 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2018, Google Inc.
>> + *
>> + * vl42device_test.h - libcamera v4l2device test base class
>> + */
>> +
>> +#include "test.h"
>> +#include "v4l2_device.h"
> 
> Includes after the inclusion guards

Ack.


>> +
>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_
>> +
>> +using namespace libcamera;
>> +
>> +class V4L2DeviceTest : public Test
>> +{
>> +public:
>> +	V4L2DeviceTest();
>> +	virtual ~V4L2DeviceTest() {};
>> +
>> +protected:
>> +	int init();
>> +	void cleanup();
>> +
>> +	virtual int run() = 0;
>> +
>> +	V4L2Device *dev;
>> +};
>> +
>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
>> --
>> 2.17.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Kieran,

On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> Thanks for the review,
>
> On 21/12/2018 16:25, jacopo mondi wrote:
> > Hi Kieran,
> >     thanks for the patch
> >
> > On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
> >> Provide a helper V4L2 device object capable of interacting with the
> >> V4L2 Linux Kernel APIs.
> >>
> >> A test suite is added at test/v4l2_device/ to utilise and validate the
> >> code base.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >> ---
> >>  src/libcamera/include/v4l2_device.h   |  36 +++++++
> >>  src/libcamera/meson.build             |   2 +
> >>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
> >
> > I would really split test and library code in 2 differen commits.
>
> I'm happy to go with group consensus here, but I felt that tests and
> code could be in the same commit here, as the test directly affects the
> code added.
>
> This means that when doing rebase actions or such, I can do 'git rebase
> -i -x "ninja; ninja test"' and validate every commit as I rework things.
>
> (I have a script which does that for me)
>

I see, and I feel in this specific case it's not a big deal, but as a
general rule having tests -in the same commit- that adds a specific
feature is not something that holds in the general case.

Tests should accompany new features, and if a patch series adds
anything that is worth being tested, just make sure they get merged
together.

As an example, Niklas' last series is a 12 patches one, with one test
at the end of the series. Your one might be a 3 patches one, with a
test at the end. Or am I missing something of how you would like to
use testing?

Anyway, that's not a big deal at all in this case :)

>
> >>  test/meson.build                      |   2 +
> >>  test/v4l2_device/double_open.cpp      |  32 ++++++
> >>  test/v4l2_device/meson.build          |  12 +++
> >>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
> >>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
> >>  8 files changed, 288 insertions(+)
> >>  create mode 100644 src/libcamera/include/v4l2_device.h
> >>  create mode 100644 src/libcamera/v4l2_device.cpp
> >>  create mode 100644 test/v4l2_device/double_open.cpp
> >>  create mode 100644 test/v4l2_device/meson.build
> >>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
> >>  create mode 100644 test/v4l2_device/v4l2_device_test.h
> >>
> >> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
> >> new file mode 100644
> >> index 000000000000..619d932d3c82
> >> --- /dev/null
> >> +++ b/src/libcamera/include/v4l2_device.h
> >> @@ -0,0 +1,36 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * v4l2_device.h - V4L2 Device API Abstractions
> >> + */
> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >> +#define __LIBCAMERA_V4L2_DEVICE_H__
> >> +
> >> +#include <string>
> >> +
> >> +#include <linux/videodev2.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +class V4L2Device
> >> +{
> >> +public:
> >> +	V4L2Device(const std::string &);

nit: I've been suggested to use parameter's names in function
declarations in header files. I know this is very minor stuff, but at
the very beginning of the development is worth being quite picky on
this minor detail to establish a consistent code base. So please bear
with me here a little more :)

> >> +	~V4L2Device();
> >> +
> >> +	bool isOpen();
> >> +	int open();
> >> +	void close();
> >> +
> >> +	int queryCap();
> >> +
> >> +private:
> >> +	std::string device_;
> >> +	int fd_;
> >> +	int capabilities_;

Ok, I'm saying it: "What about reverse-xmas-tree order for variable
declarations?"

Here, let's the bike-shedding begin

> >> +};
> >> +
> >> +}
> >
> > nit:
> > } /* namespace libcamera */
> >
>
> Ah yes - that might be nice.
>
> How did I miss this?
> Do we have this in our templates?
>
> Hrm ... nope, this end comment isn't there.
>
> I've added it to the template document.

Oh thanks, I missed it was not there, but I think that rule comes from
the C++ style guide cited in our coding style document.

>
>
> >> +
> >> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index f632eb5dd779..bbaf9a05ec2c 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -1,11 +1,13 @@
> >>  libcamera_sources = files([
> >>      'log.cpp',
> >>      'main.cpp',
> >> +    'v4l2_device.cpp',
> >>  ])
> >>
> >>  libcamera_headers = files([
> >>      'include/log.h',
> >>      'include/utils.h',
> >> +    'include/v4l2_device.h',
> >>  ])
> >>
> >>  libcamera_internal_includes =  include_directories('include')
> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> >> new file mode 100644
> >> index 000000000000..eea2514f343c
> >> --- /dev/null
> >> +++ b/src/libcamera/v4l2_device.cpp
> >> @@ -0,0 +1,137 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * v4l2_device.cpp - V4L2 Device API
> >> + */
> >> +
> >> +#include <fcntl.h>
> >> +#include <string.h>
> >> +#include <unistd.h>
> >> +
> >> +#include <sys/ioctl.h>
> >> +#include <sys/mman.h>
> >> +
> >> +#include "log.h"
> >> +#include "v4l2_device.h"
> >> +
> >> +/**
> >> + * \file v4l2_device.cpp
> >
> > file v4l2_device.h to document here what is defined in the header file
>
> Oh - So this is supposed to reference the header not declare the current
> file. I guess that kind of makes sense :) Always pointless to state the
> name of the current file.
>

It allows documenting the code in the cpp file where the implementation is,
but to refer to definitions in the header file. I know, confusing.

> >
> >> + * \brief V4L2 Device API
> >> + */
> >> +namespace libcamera {
> >> +
> >> +/**
> >> + * \class V4L2Device
> >> + * \brief V4L2Device object and API.
> >> + *
> >> + * The V4L2 Device API class models a single instance of a v4l2 device node.
> >> + */
> >> +
> >> +/**
> >> + * \fn V4L2Device::V4L2Device(device)
> >
> > nit: parameter type?
>
> I believe doxygen collects that automatically?
>
> It generates this in the output:
>   libcamera::V4L2Device::V4L2Device(const std::string & device)

Oh really? That's nice, I did it in all functions and I could have
avoided that...
>
>
> >> + *
> >> + * Default constructor for a V4L2Device object. The \a device specifies a
> >> + * string to representation of the device object.
> >
> > My poor English skill do not allow me to say if this is a typo or it
> > actually makes sense :)
>
> Ahem - there's certainly a typo there - good spot.
>
> It's also not necessarily correct, and is too close from my Camera class
> constructor text.
>
> I'll adapt this to :
>
>  * Default constructor for a V4L2Device object. The \a device specifies
>  * the path to the video device node.
>
> >
> >> + */
> >> +V4L2Device::V4L2Device(const std::string &device)
> >> +{
> >> +	device_ = device;
> >> +	fd_ = -1;
> >> +	capabilities_ = 0;
> >> +
> >> +	LOG(Debug) << "V4L2Device Constructed for " << device_;
> >
> > Not sure a debug printout is worth here
>
> Certainly not for long... they're just here for early dev.
>
> I can just drop these - they're not useful.
>

I think that would be better

> >
> >> +}
> >> +
> >> +V4L2Device::~V4L2Device()
> >> +{
> >> +	close();
> >> +
> >> +	LOG(Debug) << "V4L2Device Destroyed";
> >
> > ditto
> >
> >> +}
> >> +
> >> +/**
> >> + * \fn V4L2Device::isOpen(())
> >
> > one () in excess?
>
> Not sure how that happened?
>
> >
> >> + *
> >> + * Checks to see if we have successfully opened a v4l2 video device.
> >> + */
> >> +bool V4L2Device::isOpen()
> >> +{
> >> +	return (fd_ != -1);
> >
> > nit: no need to () braces
>
> Can I keep them ?
>

Sure!

> Returning more than a simple variable or statement 'openly' feels
> horrible to me ...

:)

>
> >> +}
> >> +
> >> +/**
> >> + * \fn V4L2Device::open()
> >> + *
> >> + * Opens a v4l2 device and queries properties from the device.
> >> + */
> >> +int V4L2Device::open()
> >> +{
> >> +	int ret;
> >> +
> >> +	if (isOpen()) {
> >> +		LOG(Error) << "Device already open";
> >> +		return -EBADF;
> >> +	}
> >> +
> >> +	fd_ = ::open(device_.c_str(), O_RDWR);
> >> +	if (fd_ < 0) {
> >> +		LOG(Error) << "Failed to open V4L2 device " << device_
> >> +			   << " : " << strerror(errno);
> >> +		return -errno;
> >> +	}
> >> +
> >> +	ret = queryCap();
> >
> > I don't like it much either, but this might have been "int ret = "
>
> Hrm ... that certainly is late instantiation.
>
> Is that what we're going for ?

That's how I would interpret
https://google.github.io/styleguide/cppguide.html#Local_Variables

It could be discussed though. Again, it is only worth discussing this
minor things as we're at the beginning and trying to establish a well
consistent code base

>
>
> >
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {
> >
> > I wonder if it wouldn't be better to return the capabilities from
> > queryCap, and assign them to the class member capabilities_ after this
> > check here.
> >
> > Looking at the code I -knew- that the queryCap() function had modified
> > the capabilities_ and then you're inspecting them here, but it's all a
> > bit implicit to me...
>
> How will we determine if the ioctl call succeeded?
>  (while avoiding exceptions?)

Well, 'queryCap()' would return -1 (or what more appropriate, like the
errno value)

>
> Capabilities flag can return any free-form int value. (ok - so it's
> restricted to the flags available in V4L2 ... but... that's very
> function specific)

Can it return values < 0 ?

>
>
>
> >> +		LOG(Error) << "Device does not support streaming IO";
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * \fn V4L2Device::close()
> >> + *
> >> + * Close a v4l2 device handle.
> >> + */
> >> +void V4L2Device::close()
> >> +{
> >> +	if (fd_ < 0)
> >> +		return;
> >> +
> >> +	::close(fd_);
> >> +	fd_ = -1;
> >> +}
> >> +
> >> +/**
> >> + * \fn V4L2Device::queryCap()
> >> + *
> >> + * Obtains the capabilities information from the V4L2 Device.
> >> + */
> >> +int V4L2Device::queryCap()
> >> +{
> >> +	struct v4l2_capability cap = {};
> >> +	int ret;
> >> +
> >> +	if (!isOpen()) {
> >> +		LOG(Error) << "Attempt to query unopened device";
> >> +		return -EBADF;
> >> +	}
> >> +
> >> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
> >> +	if (ret < 0) {
> >> +		LOG(Error) << "Failed to query device capabilities";
> >> +		return -1;
> >
> > It would be useful to return the errno number and possibly print it
> > out with strerror
>
> Good point!
>
> Should this be -errno? Or just return ret?

Returning 'ret' is equivalent to return -1. -errno is the appropriate
one.

>
>
> The more I think about it - the more I'd like to wrap the ioctl call in
> the class (or more likely a Device base class) which will handle the
> error checks...
>
> That could then be a parent class for your MediaDevice object too, and
> the expected V4L2SubDevice ...
>
> What are your thoughts? Over-abstracting? or useful ...

No, I think it is useful. Actually, more than the MediaDevice itself
(which I expect to instantiate V4L2Devices and not extend it) I think
it might be worth implementing a V4L2Object that provides protected
functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device
and V4L2Subdevice sub-class it.

I'm not sure right now how I would model that, if the base class should
do things like "setFormat()" or either provides an "ioctl()" function and
have sub-classes implement the "setFormat()" logic. This boils down to
how different that would be the implementation between a device and a
subdevice. What do you think?

>
>
> >
> >> +	}
> >> +
> >> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
> >> +		      ? cap.device_caps : cap.capabilities;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/test/meson.build b/test/meson.build
> >> index 754527324c7d..2bc76c289eea 100644
> >> --- a/test/meson.build
> >> +++ b/test/meson.build
> >> @@ -12,6 +12,8 @@ test_includes_internal = [
> >>    libcamera_internal_includes,
> >>  ]
> >>
> >> +subdir('v4l2_device')
> >> +
> >>  public_tests = [
> >>    [ 'test_init',      'init.cpp' ],
> >
> > Not related to this, but weird spacing here
>
> This was intentional ... pre-allocation of table spacing. (The table
> will have multiple entries...)

Why would you pre-allocate this?
>
> >
> >>  ]
> >> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp
> >> new file mode 100644
> >> index 000000000000..1b7c7bfe14b8
> >> --- /dev/null
> >> +++ b/test/v4l2_device/double_open.cpp
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * libcamera V4L2 API tests
> >> + */
> >> +
> >> +#include "v4l2_device_test.h"
> >> +
> >> +class DoubleOpen : public V4L2DeviceTest
> >> +{
> >> +protected:
> >> +	int run()
> >> +	{
> >> +		int ret;
> >> +
> >> +		/*
> >> +		 * Expect failure: The device has already been opened by the
> >> +		 * V4L2DeviceTest base class
> >> +		 */
> >> +		ret = dev->open();
> >> +		if (!ret) {
> >> +			fprintf(stderr, "Double open erroneously succeeded\n");
> >> +			dev->close();
> >> +			return TEST_FAIL;
> >> +		}
> >> +
> >> +		return TEST_PASS;
> >> +	}
> >
> > nit: < one indent level
>
> Oh -- for all class functions?

Oh damn, I got confused by having the full implementation here.

For 'big' functions like this one, we don't want to have inlined by
implementing them in the definition. Now, I'm struggling to find a
final answer to the question "Are functions implemented in the class
definition automatically inlined?" I bet the compiler is smart enough
to find out if it makes sense to inline or not a function even when
its implementation is in the header file, but in general you could define
the class, and implement the function outside of the definition, even
if in the same cpp file.

>
>
> >> +};
> >
> > I welcome testing, but do we really need tests to make sure a function
> > fails when called twice?
>
> Yes - I felt this was important.
>
> It's there because once I added the open() and close() calls - it
> provided a way to leak fd's if you call open(), open().
>
> The first fd would get silently dropped ...
>
> So I added a test to call open twice, then I added the isOpen() checks.
>
> In other words - the fix for the leak came about directly because I
> added the double_open test...
>

Fine then, thanks for the explanation.

>
> Another reason we I think we should base class the common device
> operations (then this test would only apply to the Device object unit
> tests anyway.)
>
>
> > I'm sure you could have sketched it by
> > calling open twice in the V4L2DeviceTest class
>
> No - the V4L2DeviceTest class is the base class common to all further
> tests for the V4L2Device Object.
>
> I have other patches which add further tests using the V4L2DeviceTest
> base class.
>
> >> +
> >> +TEST_REGISTER(DoubleOpen);
> >> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
> >> new file mode 100644
> >> index 000000000000..41675a303498
> >> --- /dev/null
> >> +++ b/test/v4l2_device/meson.build
> >> @@ -0,0 +1,12 @@
> >> +# Tests are listed in order of complexity.
> >> +# They are not alphabetically sorted.
> >> +v4l2_device_tests = [
> >> +  [ 'double_open',        'double_open.cpp' ],
> >
> > Again weird spacing, I'm now thinking this is intentional and I don't
> > know something
>
> Table pre-allocation for multiple tests.
>
> This isn't my only test, and I'd rather not have to reformat the table
> if I add tests with longer filenames.
>
> >
> >> +]
> >> +
> >> +foreach t : v4l2_device_tests
> >> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
> >> +		   link_with : test_libraries,
> >> +		   include_directories : test_includes_internal)
> >> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
> >> +endforeach
> >> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
> >> new file mode 100644
> >> index 000000000000..ae317a9519c5
> >> --- /dev/null
> >> +++ b/test/v4l2_device/v4l2_device_test.cpp
> >> @@ -0,0 +1,36 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * libcamera V4L2 API tests
> >> + */
> >> +
> >> +#include "v4l2_device_test.h"
> >> +
> >> +using namespace libcamera;
> >> +
> >> +V4L2DeviceTest::V4L2DeviceTest()
> >> +{
> >> +	dev = nullptr;
> >> +}
> >> +
> >> +int V4L2DeviceTest::init()
> >> +{
> >> +	const char *device = "/dev/video0";
> >> +
> >> +	/* Validate the device node exists. */
> >> +	if (!path_exists(device))
> >> +		return TEST_SKIP;
> >> +
> >> +	dev = new V4L2Device(device);
> >> +	if (!dev)
> >
> > You should call cleanup here if you fail
> > Ah no, the Test class should call it if init() fails!
> > Ah no^2, your constructor cannot fail (good!). My previous comment
> > still hold though.
> >
>
> Should I instead call cleanup() from my destructor ?

My point was that the object would not get destroyed here. Because the
test base class does not call cleanup() after init fails
and you don't not call it either. But I was confused actually, as the
base test class does this

       +#define TEST_REGISTER(klass)                                           \
       +int main(int argc, char *argv[])                                       \
       +{                                                                      \
       +       return klass().execute();                                       \
       +}

And this creates and destroys the object, which calls your destructor,
so I -guess- this is fine actually.
Not calling 'cleaup()' after a failed init is being discussed now
elsewhere.

Anyway, your constructor cannot fail, so you could remove that
check...

>
>
>
> >> +		return TEST_FAIL;
> >> +
> >> +	return dev->open();
> >> +}
> >> +
> >> +void V4L2DeviceTest::cleanup()
> >> +{
> >> +	if (dev)
> >> +		delete dev;
> >> +};
> >> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h
> >> new file mode 100644
> >> index 000000000000..4374ddc7e932
> >> --- /dev/null
> >> +++ b/test/v4l2_device/v4l2_device_test.h
> >> @@ -0,0 +1,31 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >> +/*
> >> + * Copyright (C) 2018, Google Inc.
> >> + *
> >> + * vl42device_test.h - libcamera v4l2device test base class
> >> + */
> >> +
> >> +#include "test.h"
> >> +#include "v4l2_device.h"
> >
> > Includes after the inclusion guards
>
> Ack.
>
>
> >> +
> >> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
> >> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_
> >> +
> >> +using namespace libcamera;
> >> +
> >> +class V4L2DeviceTest : public Test
> >> +{
> >> +public:
> >> +	V4L2DeviceTest();
> >> +	virtual ~V4L2DeviceTest() {};
> >> +
> >> +protected:
> >> +	int init();
> >> +	void cleanup();
> >> +
> >> +	virtual int run() = 0;
> >> +
> >> +	V4L2Device *dev;
> >> +};
> >> +
> >> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
> >> --
> >> 2.17.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards
> --
> Kieran

Hello,

On Sunday, 23 December 2018 22:54:09 EET Jacopo Mondi wrote:
> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:
> > On 21/12/2018 16:25, jacopo mondi wrote:
> >> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
> >>> Provide a helper V4L2 device object capable of interacting with the
> >>> V4L2 Linux Kernel APIs.
> >>> 
> >>> A test suite is added at test/v4l2_device/ to utilise and validate the
> >>> code base.
> >>> 
> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>> ---
> >>> 
> >>>  src/libcamera/include/v4l2_device.h   |  36 +++++++
> >>>  src/libcamera/meson.build             |   2 +
> >>>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
> >> 
> >> I would really split test and library code in 2 differen commits.
> > 
> > I'm happy to go with group consensus here, but I felt that tests and
> > code could be in the same commit here, as the test directly affects the
> > code added.
> > 
> > This means that when doing rebase actions or such, I can do 'git rebase
> > -i -x "ninja; ninja test"' and validate every commit as I rework things.
> > 
> > (I have a script which does that for me)
> 
> I see, and I feel in this specific case it's not a big deal, but as a
> general rule having tests -in the same commit- that adds a specific
> feature is not something that holds in the general case.
> 
> Tests should accompany new features, and if a patch series adds
> anything that is worth being tested, just make sure they get merged
> together.
> 
> As an example, Niklas' last series is a 12 patches one, with one test
> at the end of the series. Your one might be a 3 patches one, with a
> test at the end. Or am I missing something of how you would like to
> use testing?
> 
> Anyway, that's not a big deal at all in this case :)

I'm also slightly more in favour of separating code and tests in different 
patches. It's indeed not a big deal in this case, but when the patches and 
series grow bigger, bundling everything together creates too large patches.

> >>>  test/meson.build                      |   2 +
> >>>  test/v4l2_device/double_open.cpp      |  32 ++++++
> >>>  test/v4l2_device/meson.build          |  12 +++
> >>>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
> >>>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
> >>>  8 files changed, 288 insertions(+)
> >>>  create mode 100644 src/libcamera/include/v4l2_device.h
> >>>  create mode 100644 src/libcamera/v4l2_device.cpp
> >>>  create mode 100644 test/v4l2_device/double_open.cpp
> >>>  create mode 100644 test/v4l2_device/meson.build
> >>>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
> >>>  create mode 100644 test/v4l2_device/v4l2_device_test.h
> >>> 
> >>> diff --git a/src/libcamera/include/v4l2_device.h
> >>> b/src/libcamera/include/v4l2_device.h new file mode 100644
> >>> index 000000000000..619d932d3c82
> >>> --- /dev/null
> >>> +++ b/src/libcamera/include/v4l2_device.h
> >>> @@ -0,0 +1,36 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2018, Google Inc.
> >>> + *
> >>> + * v4l2_device.h - V4L2 Device API Abstractions
> >>> + */
> >>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
> >>> +#define __LIBCAMERA_V4L2_DEVICE_H__
> >>> +
> >>> +#include <string>
> >>> +
> >>> +#include <linux/videodev2.h>
> >>> +
> >>> +namespace libcamera {
> >>> +
> >>> +class V4L2Device

V4L2Device or V4l2Device ? The latter looks weird to me, but capitalizing only 
the first letter of an abbreviation makes sense as a rule (and that's what the 
Google C++ Coding Style mandates).

> >>> +{
> >>> +public:
> >>> +	V4L2Device(const std::string &);
> 
> nit: I've been suggested to use parameter's names in function
> declarations in header files. I know this is very minor stuff, but at
> the very beginning of the development is worth being quite picky on
> this minor detail to establish a consistent code base. So please bear
> with me here a little more :)

I was about to say the same :-) Parameter names make function prototypes more 
explicit.

> >>> +	~V4L2Device();
> >>> +
> >>> +	bool isOpen();

s/;/ const;

> >>> +	int open();
> >>> +	void close();
> >>> +
> >>> +	int queryCap();
> >>> +
> >>> +private:
> >>> +	std::string device_;
> >>> +	int fd_;
> >>> +	int capabilities_;
> 
> Ok, I'm saying it: "What about reverse-xmas-tree order for variable
> declarations?"
> 
> Here, let's the bike-shedding begin

It also makes sense to group them by purpose. I'd take the line size into 
account only when no other criterion applies. In this case the order proposed 
by Kieran makes sense to me.

Please also note that we can't reorganize members freely as they generally 
breaks binary compatibility (at least for classes exposed through the public 
API).

> >>> +};
> >>> +
> >>> +}
> >> 
> >> nit:
> >> } /* namespace libcamera */
> > 
> > Ah yes - that might be nice.
> > 
> > How did I miss this?
> > Do we have this in our templates?
> > 
> > Hrm ... nope, this end comment isn't there.
> > 
> > I've added it to the template document.
> 
> Oh thanks, I missed it was not there, but I think that rule comes from
> the C++ style guide cited in our coding style document.
> 
> >>> +
> >>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */

> >>> diff --git a/src/libcamera/v4l2_device.cpp
> >>> b/src/libcamera/v4l2_device.cpp
> >>> new file mode 100644
> >>> index 000000000000..eea2514f343c
> >>> --- /dev/null
> >>> +++ b/src/libcamera/v4l2_device.cpp
> >>> @@ -0,0 +1,137 @@
> >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>> +/*
> >>> + * Copyright (C) 2018, Google Inc.
> >>> + *
> >>> + * v4l2_device.cpp - V4L2 Device API
> >>> + */
> >>> +
> >>> +#include <fcntl.h>
> >>> +#include <string.h>
> >>> +#include <unistd.h>
> >>> +
> >>> +#include <sys/ioctl.h>
> >>> +#include <sys/mman.h>

You can mix the five includes above, no need to separate sys/.

> >>> +#include "log.h"
> >>> +#include "v4l2_device.h"
> >>> +
> >>> +/**
> >>> + * \file v4l2_device.cpp
> >> 
> >> file v4l2_device.h to document here what is defined in the header file
> > 
> > Oh - So this is supposed to reference the header not declare the current
> > file. I guess that kind of makes sense :) Always pointless to state the
> > name of the current file.
> 
> It allows documenting the code in the cpp file where the implementation is,
> but to refer to definitions in the header file. I know, confusing.

To be exact, it tells Doxygen to generate documentation for all classes, 
functions and macros in the referenced file. We thus need a \file statement 
for the header, and no \file statement should be needed for .cpp files as we 
shouldn't define any API there (symbols that are fully private to a .cpp file 
are internal and I don't think they need to appear in the generated 
documentation).

> >>> + * \brief V4L2 Device API
> >>> + */
> >>> +namespace libcamera {
> >>> +
> >>> +/**
> >>> + * \class V4L2Device
> >>> + * \brief V4L2Device object and API.
> >>> + *
> >>> + * The V4L2 Device API class models a single instance of a v4l2 device

s/API //
s/a single instance/an instance/ ?

> >>> node.
> >>> + */
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::V4L2Device(device)
> >> 
> >> nit: parameter type?
> > 
> > I believe doxygen collects that automatically?
> > 
> > It generates this in the output:
> >   libcamera::V4L2Device::V4L2Device(const std::string & device)

when the function isn't overloaded. But for functions defined in .cpp files 
(as opposed as the inlined functions in the class definition), you can drop 
the \fn line completely, doxygen knows that the comment block is related to 
the function that follows immediately.

> Oh really? That's nice, I did it in all functions and I could have
> avoided that...
> 
> >>> + *
> >>> + * Default constructor for a V4L2Device object. The \a device
> >>> specifies a
> >>> + * string to representation of the device object.
> >> 
> >> My poor English skill do not allow me to say if this is a typo or it
> >> actually makes sense :)
> > 
> > Ahem - there's certainly a typo there - good spot.
> > 
> > It's also not necessarily correct, and is too close from my Camera class
> > constructor text.
> > 
> > I'll adapt this to :
> >  * Default constructor for a V4L2Device object. The \a device specifies
> >  * the path to the video device node.

The default constructor would be V4L2Device::V4L2Device(), which you should = 
delete in the header.

> >>> + */
> >>> +V4L2Device::V4L2Device(const std::string &device)

	: device_(device), fd_(fd), capabilities_(0)

> >>> +{
> >>> +	device_ = device;
> >>> +	fd_ = -1;
> >>> +	capabilities_ = 0;
> >>> +
> >>> +	LOG(Debug) << "V4L2Device Constructed for " << device_;
> >> 
> >> Not sure a debug printout is worth here
> > 
> > Certainly not for long... they're just here for early dev.
> > 
> > I can just drop these - they're not useful.
> 
> I think that would be better
> 
> >>> +}
> >>> +
> >>> +V4L2Device::~V4L2Device()
> >>> +{
> >>> +	close();
> >>> +
> >>> +	LOG(Debug) << "V4L2Device Destroyed";
> >> 
> >> ditto
> >> 
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::isOpen(())
> >> 
> >> one () in excess?
> > 
> > Not sure how that happened?

You can drop the \fn line completely anyway (same for the functions below that 
are defined in this file).

> >>> + *
> >>> + * Checks to see if we have successfully opened a v4l2 video device.

\brief Check if the device is open
\sa open()
\return True if the device is open, closed otherwise

> >>> + */
> >>> +bool V4L2Device::isOpen()
> >>> +{
> >>> +	return (fd_ != -1);
> >> 
> >> nit: no need to () braces
> > 
> > Can I keep them ?
> 
> Sure!
> 
> > Returning more than a simple variable or statement 'openly' feels
> > horrible to me ...

You will however have to deal with all the code in libcamera that will make 
you fell horrible, so maybe you'll need to get used to it ;-)

> :)
>
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::open()
> >>> + *
> >>> + * Opens a v4l2 device and queries properties from the device.

\brief and \return. Same for the functions below.

> >>> + */
> >>> +int V4L2Device::open()
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	if (isOpen()) {
> >>> +		LOG(Error) << "Device already open";
> >>> +		return -EBADF;

-EBUSY maybe ?

> >>> +	}
> >>> +
> >>> +	fd_ = ::open(device_.c_str(), O_RDWR);
> >>> +	if (fd_ < 0) {

		ret = -errno;

> >>> +		LOG(Error) << "Failed to open V4L2 device " << device_
> >>> +			   << " : " << strerror(errno);
> >>> +		return -errno;

			<< " : " << strerror(-ret);
	return ret;

> >>> +	}
> >>> +
> >>> +	ret = queryCap();
> >> 
> >> I don't like it much either, but this might have been "int ret = "
> > 
> > Hrm ... that certainly is late instantiation.
> > 
> > Is that what we're going for ?
> 
> That's how I would interpret
> https://google.github.io/styleguide/cppguide.html#Local_Variables
> 
> It could be discussed though. Again, it is only worth discussing this
> minor things as we're at the beginning and trying to establish a well
> consistent code base

For variables used throughout a function (ret or a i loop counter for 
instance), I think declaring them at the top makes sense.

> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {
> >> 
> >> I wonder if it wouldn't be better to return the capabilities from
> >> queryCap, and assign them to the class member capabilities_ after this
> >> check here.
> >> 
> >> Looking at the code I -knew- that the queryCap() function had modified
> >> the capabilities_ and then you're inspecting them here, but it's all a
> >> bit implicit to me...
> > 
> > How will we determine if the ioctl call succeeded?
> > 
> >  (while avoiding exceptions?)
> 
> Well, 'queryCap()' would return -1 (or what more appropriate, like the
> errno value)
> 
> > Capabilities flag can return any free-form int value. (ok - so it's
> > restricted to the flags available in V4L2 ... but... that's very
> > function specific)
> 
> Can it return values < 0 ?
> 
> >>> +		LOG(Error) << "Device does not support streaming IO";
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::close()
> >>> + *
> >>> + * Close a v4l2 device handle.

The file handle isn't exposed as part of the API, so I wouldn't mention it
here. You can just explain that is closes the device, releases any resource 
acquired by open(), and that calling the rest of the API on a closed device 
isn't allowed.

> >>> + */
> >>> +void V4L2Device::close()
> >>> +{
> >>> +	if (fd_ < 0)
> >>> +		return;
> >>> +
> >>> +	::close(fd_);
> >>> +	fd_ = -1;
> >>> +}
> >>> +
> >>> +/**
> >>> + * \fn V4L2Device::queryCap()
> >>> + *
> >>> + * Obtains the capabilities information from the V4L2 Device.
> >>> + */
> >>> +int V4L2Device::queryCap()
> >>> +{
> >>> +	struct v4l2_capability cap = {};
> >>> +	int ret;
> >>> +
> >>> +	if (!isOpen()) {
> >>> +		LOG(Error) << "Attempt to query unopened device";
> >>> +		return -EBADF;
> >>> +	}

I think you should restrict queryCap() to be called only from the open() 
function and make it private. You could then remove this check. Or, possibly, 
even inline this function in open() as it would then be a single ioctl call.

> >>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
> >>> +	if (ret < 0) {
> >>> +		LOG(Error) << "Failed to query device capabilities";
> >>> +		return -1;
> >> 
> >> It would be useful to return the errno number and possibly print it
> >> out with strerror
> > 
> > Good point!
> > 
> > Should this be -errno? Or just return ret?
> 
> Returning 'ret' is equivalent to return -1. -errno is the appropriate
> one.
> 
> > The more I think about it - the more I'd like to wrap the ioctl call in
> > the class (or more likely a Device base class) which will handle the
> > error checks...
> > 
> > That could then be a parent class for your MediaDevice object too, and
> > the expected V4L2SubDevice ...
> > 
> > What are your thoughts? Over-abstracting? or useful ...
> 
> No, I think it is useful. Actually, more than the MediaDevice itself
> (which I expect to instantiate V4L2Devices and not extend it) I think
> it might be worth implementing a V4L2Object that provides protected
> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device
> and V4L2Subdevice sub-class it.

I think it's a bit of an overabstraction :-) I believe there will be very 
little opportunities to share code between V4l2Device, V4l2Subdevice and 
MediaDevice. I recommend developing them as separate objects, and then 
creating a common base (or common helper functions) later if we realize enough 
code is duplicated.

> I'm not sure right now how I would model that, if the base class should
> do things like "setFormat()" or either provides an "ioctl()" function and
> have sub-classes implement the "setFormat()" logic. This boils down to
> how different that would be the implementation between a device and a
> subdevice. What do you think?

The format setting model for V4L2 device and V4L2 subdevices is fundamentally 
different. Subdevices take an extra pad number argument, and they propagate 
formats internally within the subdevice, which creates a set of "subdev 
configuration" objects internally in the kernel (a global one for the active 
parameters and one per file handle for the test parameters). We will have to 
expose this one way or another to implement trying formats, and I expect the 
V4l2Device and V4l2Subdevice objects to offer different APIs for that reason.

> >>> +	}
> >>> +
> >>> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
> >>> +		      ? cap.device_caps : cap.capabilities;

How about caching the full v4l2_capability structure in V4L2Device ?

> >>> +	return 0;
> >>> +}
> >>> +
> >>> +} /* namespace libcamera */
> >>> diff --git a/test/meson.build b/test/meson.build
> >>> index 754527324c7d..2bc76c289eea 100644
> >>> --- a/test/meson.build
> >>> +++ b/test/meson.build
> >>> @@ -12,6 +12,8 @@ test_includes_internal = [
> >>>    libcamera_internal_includes,
> >>>  ]
> >>> 
> >>> +subdir('v4l2_device')
> >>> +
> >>>  public_tests = [
> >>>    [ 'test_init',      'init.cpp' ],
> >> 
> >> Not related to this, but weird spacing here
> > 
> > This was intentional ... pre-allocation of table spacing. (The table
> > will have multiple entries...)
> 
> Why would you pre-allocate this?
> 
> >>>  ]
> >>> diff --git a/test/v4l2_device/double_open.cpp
> >>> b/test/v4l2_device/double_open.cpp new file mode 100644
> >>> index 000000000000..1b7c7bfe14b8
> >>> --- /dev/null
> >>> +++ b/test/v4l2_device/double_open.cpp
> >>> @@ -0,0 +1,32 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2018, Google Inc.
> >>> + *
> >>> + * libcamera V4L2 API tests
> >>> + */
> >>> +
> >>> +#include "v4l2_device_test.h"
> >>> +
> >>> +class DoubleOpen : public V4L2DeviceTest

Maybe V4L2DeviceDoubleOpenTest to avoid namespace clashes when we'll compile 
multiple unit tests into a single binary for a test suite ? Or maybe using an 
anynomous namespace for this purpose ?

> >>> +{
> >>> +protected:
> >>> +	int run()
> >>> +	{
> >>> +		int ret;
> >>> +
> >>> +		/*
> >>> +		 * Expect failure: The device has already been opened by the
> >>> +		 * V4L2DeviceTest base class
> >>> +		 */
> >>> +		ret = dev->open();
> >>> +		if (!ret) {
> >>> +			fprintf(stderr, "Double open erroneously succeeded\n");

cout/cerr ?

> >>> +			dev->close();
> >>> +			return TEST_FAIL;
> >>> +		}
> >>> +
> >>> +		return TEST_PASS;
> >>> +	}
> > > 
> > > nit: < one indent level
> > 
> > Oh -- for all class functions?
> 
> Oh damn, I got confused by having the full implementation here.
> 
> For 'big' functions like this one, we don't want to have inlined by
> implementing them in the definition. Now, I'm struggling to find a
> final answer to the question "Are functions implemented in the class
> definition automatically inlined?" I bet the compiler is smart enough
> to find out if it makes sense to inline or not a function even when
> its implementation is in the header file, but in general you could define
> the class, and implement the function outside of the definition, even
> if in the same cpp file.
> 
> >>> +};
> >> 
> >> I welcome testing, but do we really need tests to make sure a function
> >> fails when called twice?
> > 
> > Yes - I felt this was important.
> > 
> > It's there because once I added the open() and close() calls - it
> > provided a way to leak fd's if you call open(), open().
> > 
> > The first fd would get silently dropped ...
> > 
> > So I added a test to call open twice, then I added the isOpen() checks.
> > 
> > In other words - the fix for the leak came about directly because I
> > added the double_open test...
> 
> Fine then, thanks for the explanation.
> 
> > Another reason we I think we should base class the common device
> > operations (then this test would only apply to the Device object unit
> > tests anyway.)
> > 
> > > I'm sure you could have sketched it by
> > > calling open twice in the V4L2DeviceTest class
> > 
> > No - the V4L2DeviceTest class is the base class common to all further
> > tests for the V4L2Device Object.
> > 
> > I have other patches which add further tests using the V4L2DeviceTest
> > base class.
> > 
> >>> +
> >>> +TEST_REGISTER(DoubleOpen);
> >>> diff --git a/test/v4l2_device/meson.build
> >>> b/test/v4l2_device/meson.build
> >>> new file mode 100644
> >>> index 000000000000..41675a303498
> >>> --- /dev/null
> >>> +++ b/test/v4l2_device/meson.build
> >>> @@ -0,0 +1,12 @@
> >>> +# Tests are listed in order of complexity.
> >>> +# They are not alphabetically sorted.
> >>> +v4l2_device_tests = [
> >>> +  [ 'double_open',        'double_open.cpp' ],
> >> 
> >> Again weird spacing, I'm now thinking this is intentional and I don't
> >> know something
> > 
> > Table pre-allocation for multiple tests.
> > 
> > This isn't my only test, and I'd rather not have to reformat the table
> > if I add tests with longer filenames.
> > 
> >>> +]
> >>> +
> >>> +foreach t : v4l2_device_tests
> >>> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
> >>> +		   link_with : test_libraries,
> >>> +		   include_directories : test_includes_internal)
> >>> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
> >>> +endforeach
> >>> diff --git a/test/v4l2_device/v4l2_device_test.cpp
> >>> b/test/v4l2_device/v4l2_device_test.cpp new file mode 100644
> >>> index 000000000000..ae317a9519c5
> >>> --- /dev/null
> >>> +++ b/test/v4l2_device/v4l2_device_test.cpp
> >>> @@ -0,0 +1,36 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2018, Google Inc.
> >>> + *
> >>> + * libcamera V4L2 API tests
> >>> + */
> >>> +
> >>> +#include "v4l2_device_test.h"
> >>> +
> >>> +using namespace libcamera;
> >>> +
> >>> +V4L2DeviceTest::V4L2DeviceTest()

	: dev_(nullptr)

> >>> +{
> >>> +	dev = nullptr;
> >>> +}
> >>> +
> >>> +int V4L2DeviceTest::init()
> >>> +{
> >>> +	const char *device = "/dev/video0";

	std::string device("/dev/video0");

> >>> +
> >>> +	/* Validate the device node exists. */
> >>> +	if (!path_exists(device))

Please log a message to explain why the test is skipped.

> >>> +		return TEST_SKIP;
> >>> +
> >>> +	dev = new V4L2Device(device);
> >>> +	if (!dev)
> >> 
> >> You should call cleanup here if you fail
> >> Ah no, the Test class should call it if init() fails!
> >> Ah no^2, your constructor cannot fail (good!). My previous comment
> >> still hold though.
> > 
> > Should I instead call cleanup() from my destructor ?
> 
> My point was that the object would not get destroyed here. Because the
> test base class does not call cleanup() after init fails
> and you don't not call it either. But I was confused actually, as the
> base test class does this
> 
> +#define TEST_REGISTER(klass)                                           \
> +int main(int argc, char *argv[])                                       \
> +{                                                                      \
> +       return klass().execute();                                       \
> +}
> 
> And this creates and destroys the object, which calls your destructor,
> so I -guess- this is fine actually.
> Not calling 'cleaup()' after a failed init is being discussed now
> elsewhere.
> 
> Anyway, your constructor cannot fail, so you could remove that
> check...
> 
> > >> +		return TEST_FAIL;
> > >> +
> > >> +	return dev->open();
> > >> +}
> > >> +
> > >> +void V4L2DeviceTest::cleanup()
> > >> +{
> > >> +	if (dev)
> > >> +		delete dev;

Can't you safely delete nullptr ?

> > >> +};
> >>> diff --git a/test/v4l2_device/v4l2_device_test.h
> >>> b/test/v4l2_device/v4l2_device_test.h new file mode 100644
> >>> index 000000000000..4374ddc7e932
> >>> --- /dev/null
> >>> +++ b/test/v4l2_device/v4l2_device_test.h
> >>> @@ -0,0 +1,31 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> >>> +/*
> >>> + * Copyright (C) 2018, Google Inc.
> >>> + *
> >>> + * vl42device_test.h - libcamera v4l2device test base class
> >>> + */
> >>> +
> >>> +#include "test.h"
> >>> +#include "v4l2_device.h"
> >> 
> >> Includes after the inclusion guards
> > 
> > Ack.
> > 
> >>> +
> >>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
> >>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_

> >>> +
> >>> +using namespace libcamera;
> >>> +
> >>> +class V4L2DeviceTest : public Test
> >>> +{
> >>> +public:
> >>> +	V4L2DeviceTest();
> >>> +	virtual ~V4L2DeviceTest() {};

Is this needed, given that the base class already has a virtual destructor ?

> >>> +
> >>> +protected:
> >>> +	int init();
> >>> +	void cleanup();
> >>> +
> >>> +	virtual int run() = 0;

I don't think this is needed either.

> >>> +	V4L2Device *dev;

s/dev/dev_/

> >>> +};
> >>> +
> >>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */

Hi Jacopo,

On 23/12/2018 20:54, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> Thanks for the review,
>>
>> On 21/12/2018 16:25, jacopo mondi wrote:
>>> Hi Kieran,
>>>     thanks for the patch
>>>
>>> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
>>>> Provide a helper V4L2 device object capable of interacting with the
>>>> V4L2 Linux Kernel APIs.
>>>>
>>>> A test suite is added at test/v4l2_device/ to utilise and validate the
>>>> code base.
>>>>
>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>  src/libcamera/include/v4l2_device.h   |  36 +++++++
>>>>  src/libcamera/meson.build             |   2 +
>>>>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
>>>
>>> I would really split test and library code in 2 differen commits.
>>
>> I'm happy to go with group consensus here, but I felt that tests and
>> code could be in the same commit here, as the test directly affects the
>> code added.
>>
>> This means that when doing rebase actions or such, I can do 'git rebase
>> -i -x "ninja; ninja test"' and validate every commit as I rework things.
>>
>> (I have a script which does that for me)
>>
> 
> I see, and I feel in this specific case it's not a big deal, but as a
> general rule having tests -in the same commit- that adds a specific
> feature is not something that holds in the general case.
> 
> Tests should accompany new features, and if a patch series adds
> anything that is worth being tested, just make sure they get merged
> together.
> 
> As an example, Niklas' last series is a 12 patches one, with one test
> at the end of the series. Your one might be a 3 patches one, with a
> test at the end. Or am I missing something of how you would like to
> use testing?
> 
> Anyway, that's not a big deal at all in this case :)

I've split the tests to separate patches.

As long as the test patch is straight after the relevant code patch -
the semantics are the same.

>>>>  test/meson.build                      |   2 +
>>>>  test/v4l2_device/double_open.cpp      |  32 ++++++
>>>>  test/v4l2_device/meson.build          |  12 +++
>>>>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
>>>>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
>>>>  8 files changed, 288 insertions(+)
>>>>  create mode 100644 src/libcamera/include/v4l2_device.h
>>>>  create mode 100644 src/libcamera/v4l2_device.cpp
>>>>  create mode 100644 test/v4l2_device/double_open.cpp
>>>>  create mode 100644 test/v4l2_device/meson.build
>>>>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
>>>>  create mode 100644 test/v4l2_device/v4l2_device_test.h
>>>>
>>>> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h
>>>> new file mode 100644
>>>> index 000000000000..619d932d3c82
>>>> --- /dev/null
>>>> +++ b/src/libcamera/include/v4l2_device.h
>>>> @@ -0,0 +1,36 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2018, Google Inc.
>>>> + *
>>>> + * v4l2_device.h - V4L2 Device API Abstractions
>>>> + */
>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
>>>> +#define __LIBCAMERA_V4L2_DEVICE_H__
>>>> +
>>>> +#include <string>
>>>> +
>>>> +#include <linux/videodev2.h>
>>>> +
>>>> +namespace libcamera {
>>>> +
>>>> +class V4L2Device
>>>> +{
>>>> +public:
>>>> +	V4L2Device(const std::string &);
> 
> nit: I've been suggested to use parameter's names in function
> declarations in header files. I know this is very minor stuff, but at
> the very beginning of the development is worth being quite picky on
> this minor detail to establish a consistent code base. So please bear
> with me here a little more :)

Added.

> 
>>>> +	~V4L2Device();
>>>> +
>>>> +	bool isOpen();
>>>> +	int open();
>>>> +	void close();
>>>> +
>>>> +	int queryCap();
>>>> +
>>>> +private:
>>>> +	std::string device_;
>>>> +	int fd_;
>>>> +	int capabilities_;
> 
> Ok, I'm saying it: "What about reverse-xmas-tree order for variable
> declarations?"
> 
> Here, let's the bike-shedding begin

I'll respond here in Laurent's mail.


> 
>>>> +};
>>>> +
>>>> +}
>>>
>>> nit:
>>> } /* namespace libcamera */
>>>
>>

Added.


>> Ah yes - that might be nice.
>>
>> How did I miss this?
>> Do we have this in our templates?
>>
>> Hrm ... nope, this end comment isn't there.
>>
>> I've added it to the template document.
> 
> Oh thanks, I missed it was not there, but I think that rule comes from
> the C++ style guide cited in our coding style document.
> 
>>
>>
>>>> +
>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>>>> index f632eb5dd779..bbaf9a05ec2c 100644
>>>> --- a/src/libcamera/meson.build
>>>> +++ b/src/libcamera/meson.build
>>>> @@ -1,11 +1,13 @@
>>>>  libcamera_sources = files([
>>>>      'log.cpp',
>>>>      'main.cpp',
>>>> +    'v4l2_device.cpp',
>>>>  ])
>>>>
>>>>  libcamera_headers = files([
>>>>      'include/log.h',
>>>>      'include/utils.h',
>>>> +    'include/v4l2_device.h',
>>>>  ])
>>>>
>>>>  libcamera_internal_includes =  include_directories('include')
>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>>>> new file mode 100644
>>>> index 000000000000..eea2514f343c
>>>> --- /dev/null
>>>> +++ b/src/libcamera/v4l2_device.cpp
>>>> @@ -0,0 +1,137 @@
>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>> +/*
>>>> + * Copyright (C) 2018, Google Inc.
>>>> + *
>>>> + * v4l2_device.cpp - V4L2 Device API
>>>> + */
>>>> +
>>>> +#include <fcntl.h>
>>>> +#include <string.h>
>>>> +#include <unistd.h>
>>>> +
>>>> +#include <sys/ioctl.h>
>>>> +#include <sys/mman.h>
>>>> +
>>>> +#include "log.h"
>>>> +#include "v4l2_device.h"
>>>> +
>>>> +/**
>>>> + * \file v4l2_device.cpp
>>>
>>> file v4l2_device.h to document here what is defined in the header file
>>
>> Oh - So this is supposed to reference the header not declare the current
>> file. I guess that kind of makes sense :) Always pointless to state the
>> name of the current file.
>>
> 
> It allows documenting the code in the cpp file where the implementation is,
> but to refer to definitions in the header file. I know, confusing.
> 
>>>
>>>> + * \brief V4L2 Device API
>>>> + */
>>>> +namespace libcamera {
>>>> +
>>>> +/**
>>>> + * \class V4L2Device
>>>> + * \brief V4L2Device object and API.
>>>> + *
>>>> + * The V4L2 Device API class models a single instance of a v4l2 device node.
>>>> + */
>>>> +
>>>> +/**
>>>> + * \fn V4L2Device::V4L2Device(device)
>>>
>>> nit: parameter type?
>>
>> I believe doxygen collects that automatically?
>>
>> It generates this in the output:
>>   libcamera::V4L2Device::V4L2Device(const std::string & device)
> 
> Oh really? That's nice, I did it in all functions and I could have
> avoided that...
>>
>>
>>>> + *
>>>> + * Default constructor for a V4L2Device object. The \a device specifies a
>>>> + * string to representation of the device object.
>>>
>>> My poor English skill do not allow me to say if this is a typo or it
>>> actually makes sense :)
>>
>> Ahem - there's certainly a typo there - good spot.
>>
>> It's also not necessarily correct, and is too close from my Camera class
>> constructor text.
>>
>> I'll adapt this to :
>>
>>  * Default constructor for a V4L2Device object. The \a device specifies
>>  * the path to the video device node.
>>
>>>
>>>> + */
>>>> +V4L2Device::V4L2Device(const std::string &device)
>>>> +{
>>>> +	device_ = device;
>>>> +	fd_ = -1;
>>>> +	capabilities_ = 0;
>>>> +
>>>> +	LOG(Debug) << "V4L2Device Constructed for " << device_;
>>>
>>> Not sure a debug printout is worth here
>>
>> Certainly not for long... they're just here for early dev.
>>
>> I can just drop these - they're not useful.
>>
> 
> I think that would be better
> 
>>>
>>>> +}
>>>> +
>>>> +V4L2Device::~V4L2Device()
>>>> +{
>>>> +	close();
>>>> +
>>>> +	LOG(Debug) << "V4L2Device Destroyed";
>>>
>>> ditto
>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * \fn V4L2Device::isOpen(())
>>>
>>> one () in excess?
>>
>> Not sure how that happened?
>>
>>>
>>>> + *
>>>> + * Checks to see if we have successfully opened a v4l2 video device.
>>>> + */
>>>> +bool V4L2Device::isOpen()
>>>> +{
>>>> +	return (fd_ != -1);
>>>
>>> nit: no need to () braces
>>
>> Can I keep them ?
>>
> 
> Sure!
> 
>> Returning more than a simple variable or statement 'openly' feels
>> horrible to me ...
> 
> :)
> 
>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * \fn V4L2Device::open()
>>>> + *
>>>> + * Opens a v4l2 device and queries properties from the device.
>>>> + */
>>>> +int V4L2Device::open()
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	if (isOpen()) {
>>>> +		LOG(Error) << "Device already open";
>>>> +		return -EBADF;
>>>> +	}
>>>> +
>>>> +	fd_ = ::open(device_.c_str(), O_RDWR);
>>>> +	if (fd_ < 0) {
>>>> +		LOG(Error) << "Failed to open V4L2 device " << device_
>>>> +			   << " : " << strerror(errno);
>>>> +		return -errno;
>>>> +	}
>>>> +
>>>> +	ret = queryCap();
>>>
>>> I don't like it much either, but this might have been "int ret = "
>>
>> Hrm ... that certainly is late instantiation.
>>
>> Is that what we're going for ?
> 
> That's how I would interpret
> https://google.github.io/styleguide/cppguide.html#Local_Variables
> 
> It could be discussed though. Again, it is only worth discussing this
> minor things as we're at the beginning and trying to establish a well
> consistent code base
> 
>>
>>
>>>
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {
>>>
>>> I wonder if it wouldn't be better to return the capabilities from
>>> queryCap, and assign them to the class member capabilities_ after this
>>> check here.
>>>
>>> Looking at the code I -knew- that the queryCap() function had modified
>>> the capabilities_ and then you're inspecting them here, but it's all a
>>> bit implicit to me...
>>
>> How will we determine if the ioctl call succeeded?
>>  (while avoiding exceptions?)
> 
> Well, 'queryCap()' would return -1 (or what more appropriate, like the
> errno value)
> 
>>
>> Capabilities flag can return any free-form int value. (ok - so it's
>> restricted to the flags available in V4L2 ... but... that's very
>> function specific)
> 
> Can it return values < 0 ?
> 
>>
>>
>>
>>>> +		LOG(Error) << "Device does not support streaming IO";
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \fn V4L2Device::close()
>>>> + *
>>>> + * Close a v4l2 device handle.
>>>> + */
>>>> +void V4L2Device::close()
>>>> +{
>>>> +	if (fd_ < 0)
>>>> +		return;
>>>> +
>>>> +	::close(fd_);
>>>> +	fd_ = -1;
>>>> +}
>>>> +
>>>> +/**
>>>> + * \fn V4L2Device::queryCap()
>>>> + *
>>>> + * Obtains the capabilities information from the V4L2 Device.
>>>> + */
>>>> +int V4L2Device::queryCap()
>>>> +{
>>>> +	struct v4l2_capability cap = {};
>>>> +	int ret;
>>>> +
>>>> +	if (!isOpen()) {
>>>> +		LOG(Error) << "Attempt to query unopened device";
>>>> +		return -EBADF;
>>>> +	}
>>>> +
>>>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
>>>> +	if (ret < 0) {
>>>> +		LOG(Error) << "Failed to query device capabilities";
>>>> +		return -1;
>>>
>>> It would be useful to return the errno number and possibly print it
>>> out with strerror
>>
>> Good point!
>>
>> Should this be -errno? Or just return ret?
> 
> Returning 'ret' is equivalent to return -1. -errno is the appropriate
> one.


Changed.


> 
>>
>>
>> The more I think about it - the more I'd like to wrap the ioctl call in
>> the class (or more likely a Device base class) which will handle the
>> error checks...
>>
>> That could then be a parent class for your MediaDevice object too, and
>> the expected V4L2SubDevice ...
>>
>> What are your thoughts? Over-abstracting? or useful ...
> 
> No, I think it is useful. Actually, more than the MediaDevice itself
> (which I expect to instantiate V4L2Devices and not extend it) I think
> it might be worth implementing a V4L2Object that provides protected
> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device
> and V4L2Subdevice sub-class it.
> 
> I'm not sure right now how I would model that, if the base class should
> do things like "setFormat()" or either provides an "ioctl()" function and
> have sub-classes implement the "setFormat()" logic. This boils down to
> how different that would be the implementation between a device and a
> subdevice. What do you think?


I think MediaDevice, V4L2Device, and V4L2SubDevice are all 'devices'
which operate on a common linux device node object. So I still think
they deserve a base class even it just to define the API that those
objects implement.

But I think Laurent further objected to this in his mail - so I'll move
there after this, and not do anything here yet.



> 
>>
>>
>>>
>>>> +	}
>>>> +
>>>> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
>>>> +		      ? cap.device_caps : cap.capabilities;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +} /* namespace libcamera */
>>>> diff --git a/test/meson.build b/test/meson.build
>>>> index 754527324c7d..2bc76c289eea 100644
>>>> --- a/test/meson.build
>>>> +++ b/test/meson.build
>>>> @@ -12,6 +12,8 @@ test_includes_internal = [
>>>>    libcamera_internal_includes,
>>>>  ]
>>>>
>>>> +subdir('v4l2_device')
>>>> +
>>>>  public_tests = [
>>>>    [ 'test_init',      'init.cpp' ],
>>>
>>> Not related to this, but weird spacing here
>>
>> This was intentional ... pre-allocation of table spacing. (The table
>> will have multiple entries...)
> 
> Why would you pre-allocate this?

Space :)

otherwise:
  [ 'test_cows', 'cows.cpp' ],
  [ 'test_fish', 'fish.cpp' ],
  [ 'test_init', 'init.cpp' ],
  [ 'test_mice', 'mice.cpp' ],

becomes:

-  [ 'test_cows', 'cows.cpp' ],
-  [ 'test_fish', 'fish.cpp' ],
-  [ 'test_init', 'init.cpp' ],
-  [ 'test_mice', 'mice.cpp' ],
+  [ 'test_cows',             'cows.cpp' ],
+  [ 'test_fish',             'fish.cpp' ],
+  [ 'test_init',             'init.cpp' ],
+  [ 'test_longer_names_ugh', 'longer_names_ugh' ],
+  [ 'test_mice',             'mice.cpp' ],

to add a single line...

I'm starting to think perhaps we make the iterator just look for the
.cpp extension on top of the test name though to remove the need for the
second column.


>>
>>>
>>>>  ]
>>>> diff --git a/test/v4l2_device/double_open.cpp b/test/v4l2_device/double_open.cpp
>>>> new file mode 100644
>>>> index 000000000000..1b7c7bfe14b8
>>>> --- /dev/null
>>>> +++ b/test/v4l2_device/double_open.cpp
>>>> @@ -0,0 +1,32 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (C) 2018, Google Inc.
>>>> + *
>>>> + * libcamera V4L2 API tests
>>>> + */
>>>> +
>>>> +#include "v4l2_device_test.h"
>>>> +
>>>> +class DoubleOpen : public V4L2DeviceTest
>>>> +{
>>>> +protected:
>>>> +	int run()
>>>> +	{
>>>> +		int ret;
>>>> +
>>>> +		/*
>>>> +		 * Expect failure: The device has already been opened by the
>>>> +		 * V4L2DeviceTest base class
>>>> +		 */
>>>> +		ret = dev->open();
>>>> +		if (!ret) {
>>>> +			fprintf(stderr, "Double open erroneously succeeded\n");
>>>> +			dev->close();
>>>> +			return TEST_FAIL;
>>>> +		}
>>>> +
>>>> +		return TEST_PASS;
>>>> +	}
>>>
>>> nit: < one indent level
>>
>> Oh -- for all class functions?
> 
> Oh damn, I got confused by having the full implementation here.
> 
> For 'big' functions like this one, we don't want to have inlined by
> implementing them in the definition. Now, I'm struggling to find a
> final answer to the question "Are functions implemented in the class
> definition automatically inlined?" I bet the compiler is smart enough
> to find out if it makes sense to inline or not a function even when
> its implementation is in the header file, but in general you could define
> the class, and implement the function outside of the definition, even
> if in the same cpp file.


I think for a test (where the API is already defined by the Test class)
we don't need to redefine things unnecessarily ?

This is just the implementation of a single test.


>>>> +};
>>>
>>> I welcome testing, but do we really need tests to make sure a function
>>> fails when called twice?
>>
>> Yes - I felt this was important.
>>
>> It's there because once I added the open() and close() calls - it
>> provided a way to leak fd's if you call open(), open().
>>
>> The first fd would get silently dropped ...
>>
>> So I added a test to call open twice, then I added the isOpen() checks.
>>
>> In other words - the fix for the leak came about directly because I
>> added the double_open test...
>>
> 
> Fine then, thanks for the explanation.
> 
>>
>> Another reason we I think we should base class the common device
>> operations (then this test would only apply to the Device object unit
>> tests anyway.)
>>
>>
>>> I'm sure you could have sketched it by
>>> calling open twice in the V4L2DeviceTest class
>>
>> No - the V4L2DeviceTest class is the base class common to all further
>> tests for the V4L2Device Object.
>>
>> I have other patches which add further tests using the V4L2DeviceTest
>> base class.
>>
>>>> +
>>>> +TEST_REGISTER(DoubleOpen);
>>>> diff --git a/test/v4l2_device/meson.build b/test/v4l2_device/meson.build
>>>> new file mode 100644
>>>> index 000000000000..41675a303498
>>>> --- /dev/null
>>>> +++ b/test/v4l2_device/meson.build
>>>> @@ -0,0 +1,12 @@
>>>> +# Tests are listed in order of complexity.
>>>> +# They are not alphabetically sorted.
>>>> +v4l2_device_tests = [
>>>> +  [ 'double_open',        'double_open.cpp' ],
>>>
>>> Again weird spacing, I'm now thinking this is intentional and I don't
>>> know something
>>
>> Table pre-allocation for multiple tests.
>>
>> This isn't my only test, and I'd rather not have to reformat the table
>> if I add tests with longer filenames.
>>
>>>
>>>> +]
>>>> +
>>>> +foreach t : v4l2_device_tests
>>>> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
>>>> +		   link_with : test_libraries,
>>>> +		   include_directories : test_includes_internal)
>>>> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
>>>> +endforeach
>>>> diff --git a/test/v4l2_device/v4l2_device_test.cpp b/test/v4l2_device/v4l2_device_test.cpp
>>>> new file mode 100644
>>>> index 000000000000..ae317a9519c5
>>>> --- /dev/null
>>>> +++ b/test/v4l2_device/v4l2_device_test.cpp
>>>> @@ -0,0 +1,36 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (C) 2018, Google Inc.
>>>> + *
>>>> + * libcamera V4L2 API tests
>>>> + */
>>>> +
>>>> +#include "v4l2_device_test.h"
>>>> +
>>>> +using namespace libcamera;
>>>> +
>>>> +V4L2DeviceTest::V4L2DeviceTest()
>>>> +{
>>>> +	dev = nullptr;
>>>> +}
>>>> +
>>>> +int V4L2DeviceTest::init()
>>>> +{
>>>> +	const char *device = "/dev/video0";
>>>> +
>>>> +	/* Validate the device node exists. */
>>>> +	if (!path_exists(device))
>>>> +		return TEST_SKIP;
>>>> +
>>>> +	dev = new V4L2Device(device);
>>>> +	if (!dev)
>>>
>>> You should call cleanup here if you fail
>>> Ah no, the Test class should call it if init() fails!
>>> Ah no^2, your constructor cannot fail (good!). My previous comment
>>> still hold though.
>>>
>>
>> Should I instead call cleanup() from my destructor ?
> 
> My point was that the object would not get destroyed here. Because the
> test base class does not call cleanup() after init fails
> and you don't not call it either. But I was confused actually, as the
> base test class does this
> 
>        +#define TEST_REGISTER(klass)                                           \
>        +int main(int argc, char *argv[])                                       \
>        +{                                                                      \
>        +       return klass().execute();                                       \
>        +}
> 
> And this creates and destroys the object, which calls your destructor,
> so I -guess- this is fine actually.
> Not calling 'cleaup()' after a failed init is being discussed now
> elsewhere.
> 
> Anyway, your constructor cannot fail, so you could remove that
> check...

Yes, I don't think we need to check the returns of a call to new. I
guess if we run out of memory we worry about other things instead :)



> 
>>
>>
>>
>>>> +		return TEST_FAIL;
>>>> +
>>>> +	return dev->open();
>>>> +}
>>>> +
>>>> +void V4L2DeviceTest::cleanup()
>>>> +{
>>>> +	if (dev)
>>>> +		delete dev;
>>>> +};
>>>> diff --git a/test/v4l2_device/v4l2_device_test.h b/test/v4l2_device/v4l2_device_test.h
>>>> new file mode 100644
>>>> index 000000000000..4374ddc7e932
>>>> --- /dev/null
>>>> +++ b/test/v4l2_device/v4l2_device_test.h
>>>> @@ -0,0 +1,31 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> +/*
>>>> + * Copyright (C) 2018, Google Inc.
>>>> + *
>>>> + * vl42device_test.h - libcamera v4l2device test base class
>>>> + */
>>>> +
>>>> +#include "test.h"
>>>> +#include "v4l2_device.h"
>>>
>>> Includes after the inclusion guards
>>
>> Ack.
>>
>>
>>>> +
>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
>>>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_
>>>> +
>>>> +using namespace libcamera;
>>>> +
>>>> +class V4L2DeviceTest : public Test
>>>> +{
>>>> +public:
>>>> +	V4L2DeviceTest();
>>>> +	virtual ~V4L2DeviceTest() {};
>>>> +
>>>> +protected:
>>>> +	int init();
>>>> +	void cleanup();
>>>> +
>>>> +	virtual int run() = 0;
>>>> +
>>>> +	V4L2Device *dev;
>>>> +};
>>>> +
>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> libcamera-devel mailing list
>>>> libcamera-devel@lists.libcamera.org
>>>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> --
>> Regards
>> --
>> Kieran

Hola,

On 11/01/2019 17:47, Laurent Pinchart wrote:
> Hello,
> 
> On Sunday, 23 December 2018 22:54:09 EET Jacopo Mondi wrote:
>> On Sat, Dec 22, 2018 at 08:30:37PM +0000, Kieran Bingham wrote:
>>> On 21/12/2018 16:25, jacopo mondi wrote:
>>>> On Fri, Dec 21, 2018 at 12:37:24PM +0000, Kieran Bingham wrote:
>>>>> Provide a helper V4L2 device object capable of interacting with the
>>>>> V4L2 Linux Kernel APIs.
>>>>>
>>>>> A test suite is added at test/v4l2_device/ to utilise and validate the
>>>>> code base.
>>>>>
>>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>  src/libcamera/include/v4l2_device.h   |  36 +++++++
>>>>>  src/libcamera/meson.build             |   2 +
>>>>>  src/libcamera/v4l2_device.cpp         | 137 ++++++++++++++++++++++++++
>>>>
>>>> I would really split test and library code in 2 differen commits.
>>>
>>> I'm happy to go with group consensus here, but I felt that tests and
>>> code could be in the same commit here, as the test directly affects the
>>> code added.
>>>
>>> This means that when doing rebase actions or such, I can do 'git rebase
>>> -i -x "ninja; ninja test"' and validate every commit as I rework things.
>>>
>>> (I have a script which does that for me)
>>
>> I see, and I feel in this specific case it's not a big deal, but as a
>> general rule having tests -in the same commit- that adds a specific
>> feature is not something that holds in the general case.
>>
>> Tests should accompany new features, and if a patch series adds
>> anything that is worth being tested, just make sure they get merged
>> together.
>>
>> As an example, Niklas' last series is a 12 patches one, with one test
>> at the end of the series. Your one might be a 3 patches one, with a
>> test at the end. Or am I missing something of how you would like to
>> use testing?
>>
>> Anyway, that's not a big deal at all in this case :)
> 
> I'm also slightly more in favour of separating code and tests in different 
> patches. It's indeed not a big deal in this case, but when the patches and 
> series grow bigger, bundling everything together creates too large patches.

Ok - I've split them.




>>>>>  test/meson.build                      |   2 +
>>>>>  test/v4l2_device/double_open.cpp      |  32 ++++++
>>>>>  test/v4l2_device/meson.build          |  12 +++
>>>>>  test/v4l2_device/v4l2_device_test.cpp |  36 +++++++
>>>>>  test/v4l2_device/v4l2_device_test.h   |  31 ++++++
>>>>>  8 files changed, 288 insertions(+)
>>>>>  create mode 100644 src/libcamera/include/v4l2_device.h
>>>>>  create mode 100644 src/libcamera/v4l2_device.cpp
>>>>>  create mode 100644 test/v4l2_device/double_open.cpp
>>>>>  create mode 100644 test/v4l2_device/meson.build
>>>>>  create mode 100644 test/v4l2_device/v4l2_device_test.cpp
>>>>>  create mode 100644 test/v4l2_device/v4l2_device_test.h
>>>>>
>>>>> diff --git a/src/libcamera/include/v4l2_device.h
>>>>> b/src/libcamera/include/v4l2_device.h new file mode 100644
>>>>> index 000000000000..619d932d3c82
>>>>> --- /dev/null
>>>>> +++ b/src/libcamera/include/v4l2_device.h
>>>>> @@ -0,0 +1,36 @@
>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2018, Google Inc.
>>>>> + *
>>>>> + * v4l2_device.h - V4L2 Device API Abstractions
>>>>> + */
>>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_H__
>>>>> +#define __LIBCAMERA_V4L2_DEVICE_H__
>>>>> +
>>>>> +#include <string>
>>>>> +
>>>>> +#include <linux/videodev2.h>
>>>>> +
>>>>> +namespace libcamera {
>>>>> +
>>>>> +class V4L2Device
> 
> V4L2Device or V4l2Device ? The latter looks weird to me, but capitalizing only 
> the first letter of an abbreviation makes sense as a rule (and that's what the 
> Google C++ Coding Style mandates).

I think I agree on the most part, like if this was a GnuDevice or a
UsbDevice, but I think having the numbers in affects the appearance.

 (I just can't seem to find the lowercase numbers on my keyboard ...)

... Can I file for a just-this-once-rule-exception-allowance-permission?

V4L2 is all-caps in my head... lowering the L seems so wrong...

If you really feel strongly that this needs to be V4l2 I'll to the
find-replace.


>>>>> +{
>>>>> +public:
>>>>> +	V4L2Device(const std::string &);
>>
>> nit: I've been suggested to use parameter's names in function
>> declarations in header files. I know this is very minor stuff, but at
>> the very beginning of the development is worth being quite picky on
>> this minor detail to establish a consistent code base. So please bear
>> with me here a little more :)
> 
> I was about to say the same :-) Parameter names make function prototypes more 
> explicit.
> 
>>>>> +	~V4L2Device();
>>>>> +
>>>>> +	bool isOpen();
> 
> s/;/ const;

Updated,


> 
>>>>> +	int open();
>>>>> +	void close();
>>>>> +
>>>>> +	int queryCap();
>>>>> +
>>>>> +private:
>>>>> +	std::string device_;
>>>>> +	int fd_;
>>>>> +	int capabilities_;
>>
>> Ok, I'm saying it: "What about reverse-xmas-tree order for variable
>> declarations?"
>>
>> Here, let's the bike-shedding begin
> 
> It also makes sense to group them by purpose. I'd take the line size into 
> account only when no other criterion applies. In this case the order proposed 
> by Kieran makes sense to me.
> 
> Please also note that we can't reorganize members freely as they generally 
> breaks binary compatibility (at least for classes exposed through the public 
> API).

In this instance they were essentially added in logical order of their
usage.

The device is the string for what the object is. To me this goes first
regardless.
Then the FD is the handle to operate on the external object - so it's
the second most important variable.

Leaving capabilities ... third :)



>>>>> +};
>>>>> +
>>>>> +}
>>>>
>>>> nit:
>>>> } /* namespace libcamera */
>>>
>>> Ah yes - that might be nice.
>>>
>>> How did I miss this?
>>> Do we have this in our templates?
>>>
>>> Hrm ... nope, this end comment isn't there.
>>>
>>> I've added it to the template document.
>>
>> Oh thanks, I missed it was not there, but I think that rule comes from
>> the C++ style guide cited in our coding style document.
>>
>>>>> +
>>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_H__ */
> 
>>>>> diff --git a/src/libcamera/v4l2_device.cpp
>>>>> b/src/libcamera/v4l2_device.cpp
>>>>> new file mode 100644
>>>>> index 000000000000..eea2514f343c
>>>>> --- /dev/null
>>>>> +++ b/src/libcamera/v4l2_device.cpp
>>>>> @@ -0,0 +1,137 @@
>>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2018, Google Inc.
>>>>> + *
>>>>> + * v4l2_device.cpp - V4L2 Device API
>>>>> + */
>>>>> +
>>>>> +#include <fcntl.h>
>>>>> +#include <string.h>
>>>>> +#include <unistd.h>
>>>>> +
>>>>> +#include <sys/ioctl.h>
>>>>> +#include <sys/mman.h>
> 
> You can mix the five includes above, no need to separate sys/.

Updated.

> 
>>>>> +#include "log.h"
>>>>> +#include "v4l2_device.h"
>>>>> +
>>>>> +/**
>>>>> + * \file v4l2_device.cpp
>>>>
>>>> file v4l2_device.h to document here what is defined in the header file
>>>
>>> Oh - So this is supposed to reference the header not declare the current
>>> file. I guess that kind of makes sense :) Always pointless to state the
>>> name of the current file.
>>
>> It allows documenting the code in the cpp file where the implementation is,
>> but to refer to definitions in the header file. I know, confusing.
> 
> To be exact, it tells Doxygen to generate documentation for all classes, 
> functions and macros in the referenced file. We thus need a \file statement 
> for the header, and no \file statement should be needed for .cpp files as we 
> shouldn't define any API there (symbols that are fully private to a .cpp file 
> are internal and I don't think they need to appear in the generated 
> documentation).

Ok - updated.

>>>>> + * \brief V4L2 Device API
>>>>> + */
>>>>> +namespace libcamera {
>>>>> +
>>>>> +/**
>>>>> + * \class V4L2Device
>>>>> + * \brief V4L2Device object and API.
>>>>> + *
>>>>> + * The V4L2 Device API class models a single instance of a v4l2 device
> 
> s/API //
> s/a single instance/an instance/ ?

Changed.

> 
>>>>> node.
>>>>> + */
>>>>> +
>>>>> +/**
>>>>> + * \fn V4L2Device::V4L2Device(device)
>>>>
>>>> nit: parameter type?
>>>
>>> I believe doxygen collects that automatically?
>>>
>>> It generates this in the output:
>>>   libcamera::V4L2Device::V4L2Device(const std::string & device)
> 
> when the function isn't overloaded. But for functions defined in .cpp files 
> (as opposed as the inlined functions in the class definition), you can drop 
> the \fn line completely, doxygen knows that the comment block is related to 
> the function that follows immediately.

Great - less lines is better :)


> 
>> Oh really? That's nice, I did it in all functions and I could have
>> avoided that...
>>
>>>>> + *
>>>>> + * Default constructor for a V4L2Device object. The \a device
>>>>> specifies a
>>>>> + * string to representation of the device object.
>>>>
>>>> My poor English skill do not allow me to say if this is a typo or it
>>>> actually makes sense :)
>>>
>>> Ahem - there's certainly a typo there - good spot.
>>>
>>> It's also not necessarily correct, and is too close from my Camera class
>>> constructor text.
>>>
>>> I'll adapt this to :
>>>  * Default constructor for a V4L2Device object. The \a device specifies
>>>  * the path to the video device node.
> 
> The default constructor would be V4L2Device::V4L2Device(), which you should = 
> delete in the header.

Good point.


>>>>> + */
>>>>> +V4L2Device::V4L2Device(const std::string &device)
> 
> 	: device_(device), fd_(fd), capabilities_(0)
> 
>>>>> +{
>>>>> +	device_ = device;
>>>>> +	fd_ = -1;
>>>>> +	capabilities_ = 0;
>>>>> +
>>>>> +	LOG(Debug) << "V4L2Device Constructed for " << device_;
>>>>
>>>> Not sure a debug printout is worth here
>>>
>>> Certainly not for long... they're just here for early dev.
>>>
>>> I can just drop these - they're not useful.
>>
>> I think that would be better
>>
>>>>> +}
>>>>> +
>>>>> +V4L2Device::~V4L2Device()
>>>>> +{
>>>>> +	close();
>>>>> +
>>>>> +	LOG(Debug) << "V4L2Device Destroyed";
>>>>
>>>> ditto
>>>>
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * \fn V4L2Device::isOpen(())
>>>>
>>>> one () in excess?
>>>
>>> Not sure how that happened?
> 
> You can drop the \fn line completely anyway (same for the functions below that 
> are defined in this file).
> 
>>>>> + *
>>>>> + * Checks to see if we have successfully opened a v4l2 video device.
> 
> \brief Check if the device is open
> \sa open()
> \return True if the device is open, closed otherwise
> 
>>>>> + */
>>>>> +bool V4L2Device::isOpen()
>>>>> +{
>>>>> +	return (fd_ != -1);
>>>>
>>>> nit: no need to () braces
>>>
>>> Can I keep them ?
>>
>> Sure!
>>
>>> Returning more than a simple variable or statement 'openly' feels
>>> horrible to me ...
> 
> You will however have to deal with all the code in libcamera that will make 
> you fell horrible, so maybe you'll need to get used to it ;-)


But that's "Other peoples code" :D


> 
>> :)
>>
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * \fn V4L2Device::open()
>>>>> + *
>>>>> + * Opens a v4l2 device and queries properties from the device.
> 
> \brief and \return. Same for the functions below.
> 
>>>>> + */
>>>>> +int V4L2Device::open()
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	if (isOpen()) {
>>>>> +		LOG(Error) << "Device already open";
>>>>> +		return -EBADF;
> 
> -EBUSY maybe ?

Sure.

> 
>>>>> +	}
>>>>> +
>>>>> +	fd_ = ::open(device_.c_str(), O_RDWR);
>>>>> +	if (fd_ < 0) {
> 
> 		ret = -errno;
> 
>>>>> +		LOG(Error) << "Failed to open V4L2 device " << device_
>>>>> +			   << " : " << strerror(errno);
>>>>> +		return -errno;
> 
> 			<< " : " << strerror(-ret);
> 	return ret;
> 
>>>>> +	}
>>>>> +
>>>>> +	ret = queryCap();
>>>>
>>>> I don't like it much either, but this might have been "int ret = "
>>>
>>> Hrm ... that certainly is late instantiation.
>>>
>>> Is that what we're going for ?
>>
>> That's how I would interpret
>> https://google.github.io/styleguide/cppguide.html#Local_Variables
>>
>> It could be discussed though. Again, it is only worth discussing this
>> minor things as we're at the beginning and trying to establish a well
>> consistent code base
> 
> For variables used throughout a function (ret or a i loop counter for 
> instance), I think declaring them at the top makes sense.

Me too.


> 
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (!(capabilities_ & V4L2_CAP_STREAMING)) {
>>>>
>>>> I wonder if it wouldn't be better to return the capabilities from
>>>> queryCap, and assign them to the class member capabilities_ after this
>>>> check here.
>>>>
>>>> Looking at the code I -knew- that the queryCap() function had modified
>>>> the capabilities_ and then you're inspecting them here, but it's all a
>>>> bit implicit to me...
>>>
>>> How will we determine if the ioctl call succeeded?
>>>
>>>  (while avoiding exceptions?)
>>
>> Well, 'queryCap()' would return -1 (or what more appropriate, like the
>> errno value)
>>
>>> Capabilities flag can return any free-form int value. (ok - so it's
>>> restricted to the flags available in V4L2 ... but... that's very
>>> function specific)
>>
>> Can it return values < 0 ?

Sorry - I should have said "Can return any free-form unsigned int value.

And that actually means that my capabilities_ is the wrong type.

But I think as per Laurent's suggestion - we could just cache the whole
capabilities object in our class - and provide helpers to interpret it
if necessary.



>>>>> +		LOG(Error) << "Device does not support streaming IO";
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * \fn V4L2Device::close()
>>>>> + *
>>>>> + * Close a v4l2 device handle.
> 
> The file handle isn't exposed as part of the API, so I wouldn't mention it
> here. You can just explain that is closes the device, releases any resource 
> acquired by open(), and that calling the rest of the API on a closed device 
> isn't allowed.

Ok.

> 
>>>>> + */
>>>>> +void V4L2Device::close()
>>>>> +{
>>>>> +	if (fd_ < 0)
>>>>> +		return;
>>>>> +
>>>>> +	::close(fd_);
>>>>> +	fd_ = -1;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * \fn V4L2Device::queryCap()
>>>>> + *
>>>>> + * Obtains the capabilities information from the V4L2 Device.
>>>>> + */
>>>>> +int V4L2Device::queryCap()
>>>>> +{
>>>>> +	struct v4l2_capability cap = {};
>>>>> +	int ret;
>>>>> +
>>>>> +	if (!isOpen()) {
>>>>> +		LOG(Error) << "Attempt to query unopened device";
>>>>> +		return -EBADF;
>>>>> +	}
> 
> I think you should restrict queryCap() to be called only from the open() 
> function and make it private. You could then remove this check. Or, possibly, 
> even inline this function in open() as it would then be a single ioctl call.

Yes, it shouldn't change - so there is no need to call again later. And
it's values should be cached...

I'll inline this and simplify.


> 
>>>>> +	ret = ioctl(fd_, VIDIOC_QUERYCAP, &cap);
>>>>> +	if (ret < 0) {
>>>>> +		LOG(Error) << "Failed to query device capabilities";
>>>>> +		return -1;
>>>>
>>>> It would be useful to return the errno number and possibly print it
>>>> out with strerror
>>>
>>> Good point!
>>>
>>> Should this be -errno? Or just return ret?
>>
>> Returning 'ret' is equivalent to return -1. -errno is the appropriate
>> one.
>>
>>> The more I think about it - the more I'd like to wrap the ioctl call in
>>> the class (or more likely a Device base class) which will handle the
>>> error checks...
>>>
>>> That could then be a parent class for your MediaDevice object too, and
>>> the expected V4L2SubDevice ...
>>>
>>> What are your thoughts? Over-abstracting? or useful ...
>>
>> No, I think it is useful. Actually, more than the MediaDevice itself
>> (which I expect to instantiate V4L2Devices and not extend it) I think
>> it might be worth implementing a V4L2Object that provides protected
>> functions such as 'doIOCTL()' or just 'ioctl()' and have V4L2Device
>> and V4L2Subdevice sub-class it.
> 
> I think it's a bit of an overabstraction :-) I believe there will be very 
> little opportunities to share code between V4l2Device, V4l2Subdevice and 
> MediaDevice. I recommend developing them as separate objects, and then 
> creating a common base (or common helper functions) later if we realize enough 
> code is duplicated.
> 
>> I'm not sure right now how I would model that, if the base class should
>> do things like "setFormat()" or either provides an "ioctl()" function and
>> have sub-classes implement the "setFormat()" logic. This boils down to
>> how different that would be the implementation between a device and a
>> subdevice. What do you think?
> 
> The format setting model for V4L2 device and V4L2 subdevices is fundamentally 
> different. Subdevices take an extra pad number argument, and they propagate 
> formats internally within the subdevice, which creates a set of "subdev 
> configuration" objects internally in the kernel (a global one for the active 
> parameters and one per file handle for the test parameters). We will have to 
> expose this one way or another to implement trying formats, and I expect the 
> V4l2Device and V4l2Subdevice objects to offer different APIs for that reason.
> 
>>>>> +	}
>>>>> +
>>>>> +	capabilities_ = cap.capabilities & V4L2_CAP_DEVICE_CAPS
>>>>> +		      ? cap.device_caps : cap.capabilities;
> 
> How about caching the full v4l2_capability structure in V4L2Device ?

I'm trying this now - and caching the whole structure has led me to
wrapping the capabilities structure in a calss to provide helpers such
as isCapture() isMplane() hasStreaming() which I think makes sense to
keeping all the handling of the capabilities API in one place.

A C++ Class inheriting a C structure seems a nice way to wrap our kernel
structure objects.. :)

>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +} /* namespace libcamera */
>>>>> diff --git a/test/meson.build b/test/meson.build
>>>>> index 754527324c7d..2bc76c289eea 100644
>>>>> --- a/test/meson.build
>>>>> +++ b/test/meson.build
>>>>> @@ -12,6 +12,8 @@ test_includes_internal = [
>>>>>    libcamera_internal_includes,
>>>>>  ]
>>>>>
>>>>> +subdir('v4l2_device')
>>>>> +
>>>>>  public_tests = [
>>>>>    [ 'test_init',      'init.cpp' ],
>>>>
>>>> Not related to this, but weird spacing here
>>>
>>> This was intentional ... pre-allocation of table spacing. (The table
>>> will have multiple entries...)
>>
>> Why would you pre-allocate this?
>>
>>>>>  ]
>>>>> diff --git a/test/v4l2_device/double_open.cpp
>>>>> b/test/v4l2_device/double_open.cpp new file mode 100644
>>>>> index 000000000000..1b7c7bfe14b8
>>>>> --- /dev/null
>>>>> +++ b/test/v4l2_device/double_open.cpp
>>>>> @@ -0,0 +1,32 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2018, Google Inc.
>>>>> + *
>>>>> + * libcamera V4L2 API tests
>>>>> + */
>>>>> +
>>>>> +#include "v4l2_device_test.h"
>>>>> +
>>>>> +class DoubleOpen : public V4L2DeviceTest
> 
> Maybe V4L2DeviceDoubleOpenTest to avoid namespace clashes when we'll compile 
> multiple unit tests into a single binary for a test suite ? Or maybe using an 
> anynomous namespace for this purpose ?

An anonymous namespace sounds good.
These files should essentially be closed namespace I think.


> 
>>>>> +{
>>>>> +protected:
>>>>> +	int run()
>>>>> +	{
>>>>> +		int ret;
>>>>> +
>>>>> +		/*
>>>>> +		 * Expect failure: The device has already been opened by the
>>>>> +		 * V4L2DeviceTest base class
>>>>> +		 */
>>>>> +		ret = dev->open();
>>>>> +		if (!ret) {
>>>>> +			fprintf(stderr, "Double open erroneously succeeded\n");
> 
> cout/cerr ?
> 
>>>>> +			dev->close();
>>>>> +			return TEST_FAIL;
>>>>> +		}
>>>>> +
>>>>> +		return TEST_PASS;
>>>>> +	}
>>>>
>>>> nit: < one indent level
>>>
>>> Oh -- for all class functions?
>>
>> Oh damn, I got confused by having the full implementation here.
>>
>> For 'big' functions like this one, we don't want to have inlined by
>> implementing them in the definition. Now, I'm struggling to find a
>> final answer to the question "Are functions implemented in the class
>> definition automatically inlined?" I bet the compiler is smart enough
>> to find out if it makes sense to inline or not a function even when
>> its implementation is in the header file, but in general you could define
>> the class, and implement the function outside of the definition, even
>> if in the same cpp file.
>>
>>>>> +};
>>>>
>>>> I welcome testing, but do we really need tests to make sure a function
>>>> fails when called twice?
>>>
>>> Yes - I felt this was important.
>>>
>>> It's there because once I added the open() and close() calls - it
>>> provided a way to leak fd's if you call open(), open().
>>>
>>> The first fd would get silently dropped ...
>>>
>>> So I added a test to call open twice, then I added the isOpen() checks.
>>>
>>> In other words - the fix for the leak came about directly because I
>>> added the double_open test...
>>
>> Fine then, thanks for the explanation.
>>
>>> Another reason we I think we should base class the common device
>>> operations (then this test would only apply to the Device object unit
>>> tests anyway.)
>>>
>>>> I'm sure you could have sketched it by
>>>> calling open twice in the V4L2DeviceTest class
>>>
>>> No - the V4L2DeviceTest class is the base class common to all further
>>> tests for the V4L2Device Object.
>>>
>>> I have other patches which add further tests using the V4L2DeviceTest
>>> base class.
>>>
>>>>> +
>>>>> +TEST_REGISTER(DoubleOpen);
>>>>> diff --git a/test/v4l2_device/meson.build
>>>>> b/test/v4l2_device/meson.build
>>>>> new file mode 100644
>>>>> index 000000000000..41675a303498
>>>>> --- /dev/null
>>>>> +++ b/test/v4l2_device/meson.build
>>>>> @@ -0,0 +1,12 @@
>>>>> +# Tests are listed in order of complexity.
>>>>> +# They are not alphabetically sorted.
>>>>> +v4l2_device_tests = [
>>>>> +  [ 'double_open',        'double_open.cpp' ],
>>>>
>>>> Again weird spacing, I'm now thinking this is intentional and I don't
>>>> know something
>>>
>>> Table pre-allocation for multiple tests.
>>>
>>> This isn't my only test, and I'd rather not have to reformat the table
>>> if I add tests with longer filenames.
>>>
>>>>> +]
>>>>> +
>>>>> +foreach t : v4l2_device_tests
>>>>> +  exe = executable(t[0], [t[1], 'v4l2_device_test.cpp'],
>>>>> +		   link_with : test_libraries,
>>>>> +		   include_directories : test_includes_internal)
>>>>> +  test(t[0], exe, suite: 'v4l2_device', is_parallel: false)
>>>>> +endforeach
>>>>> diff --git a/test/v4l2_device/v4l2_device_test.cpp
>>>>> b/test/v4l2_device/v4l2_device_test.cpp new file mode 100644
>>>>> index 000000000000..ae317a9519c5
>>>>> --- /dev/null
>>>>> +++ b/test/v4l2_device/v4l2_device_test.cpp
>>>>> @@ -0,0 +1,36 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2018, Google Inc.
>>>>> + *
>>>>> + * libcamera V4L2 API tests
>>>>> + */
>>>>> +
>>>>> +#include "v4l2_device_test.h"
>>>>> +
>>>>> +using namespace libcamera;
>>>>> +
>>>>> +V4L2DeviceTest::V4L2DeviceTest()
> 
> 	: dev_(nullptr)

Fixed.


> 
>>>>> +{
>>>>> +	dev = nullptr;
>>>>> +}
>>>>> +
>>>>> +int V4L2DeviceTest::init()
>>>>> +{
>>>>> +	const char *device = "/dev/video0";
> 
> 	std::string device("/dev/video0");
> 
>>>>> +
>>>>> +	/* Validate the device node exists. */
>>>>> +	if (!path_exists(device))
> 
> Please log a message to explain why the test is skipped.

Done.

> 
>>>>> +		return TEST_SKIP;
>>>>> +
>>>>> +	dev = new V4L2Device(device);
>>>>> +	if (!dev)
>>>>
>>>> You should call cleanup here if you fail
>>>> Ah no, the Test class should call it if init() fails!
>>>> Ah no^2, your constructor cannot fail (good!). My previous comment
>>>> still hold though.
>>>
>>> Should I instead call cleanup() from my destructor ?
>>
>> My point was that the object would not get destroyed here. Because the
>> test base class does not call cleanup() after init fails
>> and you don't not call it either. But I was confused actually, as the
>> base test class does this
>>
>> +#define TEST_REGISTER(klass)                                           \
>> +int main(int argc, char *argv[])                                       \
>> +{                                                                      \
>> +       return klass().execute();                                       \
>> +}
>>
>> And this creates and destroys the object, which calls your destructor,
>> so I -guess- this is fine actually.
>> Not calling 'cleaup()' after a failed init is being discussed now
>> elsewhere.
>>
>> Anyway, your constructor cannot fail, so you could remove that
>> check...
>>
>>>>> +		return TEST_FAIL;
>>>>> +
>>>>> +	return dev->open();
>>>>> +}
>>>>> +
>>>>> +void V4L2DeviceTest::cleanup()
>>>>> +{
>>>>> +	if (dev)
>>>>> +		delete dev;
> 
> Can't you safely delete nullptr ?

Yes ;)


> 
>>>>> +};
>>>>> diff --git a/test/v4l2_device/v4l2_device_test.h
>>>>> b/test/v4l2_device/v4l2_device_test.h new file mode 100644
>>>>> index 000000000000..4374ddc7e932
>>>>> --- /dev/null
>>>>> +++ b/test/v4l2_device/v4l2_device_test.h
>>>>> @@ -0,0 +1,31 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>>> +/*
>>>>> + * Copyright (C) 2018, Google Inc.
>>>>> + *
>>>>> + * vl42device_test.h - libcamera v4l2device test base class
>>>>> + */
>>>>> +
>>>>> +#include "test.h"
>>>>> +#include "v4l2_device.h"
>>>>
>>>> Includes after the inclusion guards
>>>
>>> Ack.
>>>
>>>>> +
>>>>> +#ifndef __LIBCAMERA_V4L2_DEVICE_TEST_H_
>>>>> +#define __LIBCAMERA_V4L2_DEVICE_TEST_H_
> 
>>>>> +
>>>>> +using namespace libcamera;
>>>>> +
>>>>> +class V4L2DeviceTest : public Test
>>>>> +{
>>>>> +public:
>>>>> +	V4L2DeviceTest();
>>>>> +	virtual ~V4L2DeviceTest() {};
> 
> Is this needed, given that the base class already has a virtual destructor ?

Removed.

> 
>>>>> +
>>>>> +protected:
>>>>> +	int init();
>>>>> +	void cleanup();
>>>>> +
>>>>> +	virtual int run() = 0;
> 
> I don't think this is needed either.

Removed.

> 
>>>>> +	V4L2Device *dev;
> 
> s/dev/dev_/

Fixed.


> 
>>>>> +};
>>>>> +
>>>>> +#endif /* __LIBCAMERA_V4L2_DEVICE_TEST_H_ */
> 

Thanks.