[libcamera-devel] [PATCH 2/2] lib: Add V4L2 Device object
jacopo mondi
jacopo at jmondi.org
Fri Dec 21 17:25:39 CET 2018
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181221/4122812c/attachment-0001.sig>
More information about the libcamera-devel
mailing list