[libcamera-devel] [PATCH] libcamera: device_enumerator: add DeviceEnumeratorSysfs class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri May 3 00:56:36 CEST 2019
Hi Paul,
Thank you for the patch.
On Thu, May 02, 2019 at 05:26:51PM -0400, Paul Elder wrote:
> A udev-based device enumerator is not sufficient, since libudev is an
> optional dependency, or udev might fail. In these cases, we should fall
> back to using sysfs to enumerate devices.
>
> Add a DeviceEnumeratorSysfs class which is a specialization of
> DeviceEnumerator that uses sysfs to enumerate media devices on the
> system.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Documentation/Doxyfile.in | 2 +
> src/libcamera/device_enumerator.cpp | 6 +-
> src/libcamera/device_enumerator_sysfs.cpp | 109 ++++++++++++++++++
> .../include/device_enumerator_sysfs.h | 31 +++++
> src/libcamera/meson.build | 2 +
> 5 files changed, 148 insertions(+), 2 deletions(-)
> create mode 100644 src/libcamera/device_enumerator_sysfs.cpp
> create mode 100644 src/libcamera/include/device_enumerator_sysfs.h
>
> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in
> index 950ad4f..5dd4445 100644
> --- a/Documentation/Doxyfile.in
> +++ b/Documentation/Doxyfile.in
> @@ -835,6 +835,8 @@ RECURSIVE = YES
>
> EXCLUDE = ../src/libcamera/device_enumerator_udev.cpp \
> ../src/libcamera/include/device_enumerator_udev.h \
> + ../src/libcamera/device_enumerator_sysfs.cpp \
> + ../src/libcamera/include/device_enumerator_sysfs.h \
> ../src/libcamera/pipeline/
Could you please keep those entries ordered alphabetically ?
>
> # The EXCLUDE_SYMLINKS tag can be used to select whether or not files or
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index f6878b3..5c44eb1 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -7,6 +7,7 @@
>
> #include "device_enumerator.h"
> #include "device_enumerator_udev.h"
> +#include "device_enumerator_sysfs.h"
Here too :-)
>
> #include <string.h>
>
> @@ -153,8 +154,9 @@ std::unique_ptr<DeviceEnumerator> DeviceEnumerator::create()
> * Either udev is not available or udev initialization failed. Fall back
> * on the sysfs enumerator.
> */
> -
> - /** \todo Add a sysfs-based enumerator. */
> + enumerator = utils::make_unique<DeviceEnumeratorSysfs>();
> + if (!enumerator->init())
> + return enumerator;
>
> return nullptr;
> }
> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> new file mode 100644
> index 0000000..6dd04e8
> --- /dev/null
> +++ b/src/libcamera/device_enumerator_sysfs.cpp
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018-2019, Google Inc.
As this is a new file, 2019 is sufficient.
> + *
> + * device_enumerator_sysfs.cpp - udev-based device enumerator
sysfs/based ?
> + */
> +
> +#include "device_enumerator_sysfs.h"
> +
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <fstream>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "log.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(DeviceEnumerator)
> +
> +DeviceEnumeratorSysfs::DeviceEnumeratorSysfs()
> +{
> +}
> +
> +DeviceEnumeratorSysfs::~DeviceEnumeratorSysfs()
> +{
> +}
> +
Unless I'm mistaken you don't need to define empty constructors and
destructors (except for the destructor at the base of the hierarchy of a
virtual class), the compiler will handle this for you.
> +int DeviceEnumeratorSysfs::init()
> +{
> + return 0;
> +}
> +
> +int DeviceEnumeratorSysfs::enumerate()
> +{
> + struct dirent *ent;
> + struct stat devstat;
> + char *end;
You can declare devstat and end within the loop.
> + DIR *dir;
> +
> + static const char *sysfs_dirs[] = {
> + "/sys/media/devices",
> + "/sys/block/media/devices",
> + "/sys/bus/media/devices",
> + "/sys/class/media/devices",
The first and second entries look really suspicious. Do you need them ?
Do you need the last entry ?
> + };
> +
> + for (const char *dirname : sysfs_dirs) {
> + dir = opendir(dirname);
> + if (dir)
> + break;
> + }
> +
> + while ((ent = readdir(dir)) != nullptr) {
> + unsigned int idx;
> + std::string devnode = std::string();
No need for initialisers, the variable will be constructed with the
default constructor of std::string in any case.
> +
> + if (strncmp(ent->d_name, "media", 5))
> + continue;
> +
> + idx = strtoul(ent->d_name + 5, &end, 10);
> + if (*end != '\0')
> + continue;
> +
> + devnode = "/dev/media" + std::to_string(idx);
> +
> + /* Verify that the device node exists. */
> + if (stat(devnode.c_str(), &devstat) < 0)
> + continue;
How about logging a warning message here ?
> +
> + addDevice(devnode);
> + }
> +
> + closedir(dir);
> +
> + return 0;
> +}
> +
> +std::string DeviceEnumeratorSysfs::lookupDeviceNode(int major, int minor)
> +{
> + std::string deviceNode = std::string();
> + std::string line = std::string();
No need for initialisers here either.
> + std::ifstream uevent_file;
We use camelCase for variables, this should be named ueventFile.
> + size_t c_cnt;
And this could just be named count.
> +
> + uevent_file.open("/sys/dev/char/" + std::to_string(major) + ":" +
> + std::to_string(minor) + "/uevent");
> + if (!uevent_file)
> + return std::string();
> +
> + while (uevent_file >> line) {
> + if (line.find("DEVNAME") != std::string::npos) {
> + c_cnt = line.find("=");
I think we should require the line to start with "DEVNAME=" exactly.
Maybe
if (line.find("DEVNAME=") == 0) {
deviceNode = "dev" + line.substr(strlen("DEVNAME="));
break;
}
> + deviceNode = "/dev/" + line.substr(c_cnt + 1);
> + break;
> + }
> + }
> +
> + uevent_file.close();
> +
> + return deviceNode;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h
> new file mode 100644
> index 0000000..f32d6ae
> --- /dev/null
> +++ b/src/libcamera/include/device_enumerator_sysfs.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018-2019, Google Inc.
Just 2019 here too.
> + *
> + * device_enumerator_sysfs.h - sysfs-based device enumerator
> + */
> +#ifndef __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__
> +#define __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__
> +
> +#include <string>
> +
> +#include "device_enumerator.h"
> +
> +namespace libcamera {
> +
> +class DeviceEnumeratorSysfs : public DeviceEnumerator
> +{
> +public:
> + DeviceEnumeratorSysfs();
> + ~DeviceEnumeratorSysfs();
> +
> + int init() final;
> + int enumerate() final;
You can declare the whole class final instead of individual members with
class DeviceEnumeratorSysfs final : public DeviceEnumerator
> +
> +private:
> + std::string lookupDeviceNode(int major, int minor) final;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_DEVICE_ENUMERATOR_SYSFS_H__ */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 2b67823..ca9ed7a 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -4,6 +4,7 @@ libcamera_sources = files([
> 'camera_manager.cpp',
> 'camera_sensor.cpp',
> 'device_enumerator.cpp',
> + 'device_enumerator_sysfs.cpp',
> 'event_dispatcher.cpp',
> 'event_dispatcher_poll.cpp',
> 'event_notifier.cpp',
> @@ -27,6 +28,7 @@ libcamera_headers = files([
> 'include/camera_sensor.h',
> 'include/device_enumerator.h',
> 'include/device_enumerator_udev.h',
> + 'include/device_enumerator_sysfs.h',
Alphabetical order here too please.
> 'include/event_dispatcher_poll.h',
> 'include/formats.h',
> 'include/log.h',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list