[libcamera-devel] [PATCH v8 4/9] libcamera: camera_sensor: Generate a sensor ID

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Wed, Aug 05, 2020 at 12:48:55PM +0200, Niklas Söderlund wrote:
> The ID is generated from information in the firmware description of the
> sensor if available or from module and model information if the sensor
> is virtual (for example VIMC).
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes sinve v7
> - Use device path stripped of /sys/devices/ prefix instead of modalias
>   for virtual sensors.
> 
> * Changes sinve v6
> - Do not allow for subclassing
> - Add support for virtual sensors
> - Use new sysfs:: helper.
> 
> * Changes since v5
> - Use new utils:: helper.
> - Allow for subclassing
> 
> * Changes since v4
> - Fix spelling.
> 
> * Changes since v3
> - Update commit message.
> - Add description of how ID are generated to comment.
> ---
>  include/libcamera/internal/camera_sensor.h |  3 ++
>  src/libcamera/camera_sensor.cpp            | 37 ++++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 06c8292ca30129de..0f65fca46877e89a 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -67,11 +67,14 @@ protected:
>  	std::string logPrefix() const override;
>  
>  private:
> +	int generateId();
> +
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
>  	unsigned int pad_;
>  
>  	std::string model_;
> +	std::string id_;
>  
>  	V4L2Subdevice::Formats formats_;
>  	Size resolution_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 350f49accad99c7b..695c4ad0028d5c4c 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -9,6 +9,7 @@
>  
>  #include <algorithm>
>  #include <float.h>
> +#include <fstream>

Do you still need this ?

>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
> @@ -16,7 +17,9 @@
>  
>  #include <libcamera/property_ids.h>
>  
> +#include "libcamera/internal/file.h"
>  #include "libcamera/internal/formats.h"
> +#include "libcamera/internal/sysfs.h"
>  #include "libcamera/internal/utils.h"
>  
>  /**
> @@ -204,6 +207,11 @@ int CameraSensor::init()
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Generate a unique ID for the sensor. */
> +	ret = generateId();
> +	if (ret)
> +		return ret;
> +
>  	/* Retrieve and store the camera sensor properties. */
>  	const ControlInfoMap &controls = subdev_->controls();
>  	int32_t propertyValue;
> @@ -541,4 +549,33 @@ std::string CameraSensor::logPrefix() const
>  	return "'" + entity_->name() + "'";
>  }
>  
> +int CameraSensor::generateId()
> +{
> +	const std::string devPath = subdev_->devicePath();
> +
> +	/* Try to get ID from firmware description. */
> +	id_ = sysfs::firmwareNodePath(devPath);
> +	if (!id_.empty())
> +		return 0;
> +
> +	/*
> +	 * Virtual sensors not described in firmware
> +	 *
> +	 * Verify it's a platform device and construct ID from the deive path
> +	 * and model of sensor.
> +	 */
> +	if (devPath.rfind("/sys/devices/platform/", 0) == 0) {

s/rfind/find/ as the match will occur at position 0 in most cases.

Shame https://en.cppreference.com/w/cpp/string/basic_string/starts_with
was only introduced in C++20. We could add it to utils:: if needed.

> +		id_ = devPath + " " + model();
> +
> +		static const std::string dropStr = "/sys/devices/";
> +		if (id_.find(dropStr) == 0)

Given that id_ is guaranteed to start with "/sys/devices/platform/", I
think you can drop this check :-)

> +			id_.erase(0, dropStr.length());

		id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();

(you'll need to include string.h)

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +		return 0;
> +	}
> +
> +	LOG(CameraSensor, Error) << "Can't generate sensor ID";
> +	return -EINVAL;
> +}
> +
>  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart