[libcamera-devel] libcamera: device_enumerator: add DeviceEnumeratorSysfs class

1149 diff mbox series

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@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

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@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',