[libcamera-devel,v6,5/9] libcamera: camera_sensor: Generate a sensor ID

9159 diff mbox series

Generate a constant and unique string ID for the sensor. The ID is
generated from information from the firmware description of the camera
image sensor. The ID is unique and persistent across reboots of the
system and may be used as a camera ID.

The CameraSensor is intended to be sub-classed for specific sensor
modules in the future to enable more advanced use-cases. Controlling the
ID is one such feature that is already needed by the VIMC sensor. As the
VIMC sensor is a virtual pipeline its sensor is not described in the
device firmware and a subclass of CameraSensor is needed to generate a
ID for VIMC sensors.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* 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               | 40 +++++++++++++++++++
 src/libcamera/pipeline/vimc/vimc.cpp          | 19 ++++++++-
 test/camera-sensor.cpp                        |  3 +-
 test/libtest/vimc_sensor_test.h               | 29 ++++++++++++++
 .../v4l2_videodevice_test.cpp                 |  4 +-
 6 files changed, 94 insertions(+), 4 deletions(-)
 create mode 100644 test/libtest/vimc_sensor_test.h

Hi Niklas,
   if I were to be picky, this patch actually contains two things:
- id generation
- accessor

I don't mind too much though

On Mon, Aug 03, 2020 at 11:17:29PM +0200, Niklas Söderlund wrote:
> Generate a constant and unique string ID for the sensor. The ID is
> generated from information from the firmware description of the camera
> image sensor. The ID is unique and persistent across reboots of the
> system and may be used as a camera ID.

I know what 'camera ID' is, but what about
'may be used to identify the camera uniquely in the system'
>
> The CameraSensor is intended to be sub-classed for specific sensor
> modules in the future to enable more advanced use-cases. Controlling the

Ahem, yes, no, maybe...
I presume this will happen sooner or later, but since we trashed the
camera sensor factory we have been working to be able to standardize
things as much as we could to use a single camera sensor class.

> ID is one such feature that is already needed by the VIMC sensor. As the
> VIMC sensor is a virtual pipeline its sensor is not described in the
> device firmware and a subclass of CameraSensor is needed to generate a
> ID for VIMC sensors.

I understand the issue, and actually sub-classing CameraSensor in the
pipeline handler seems acceptable to me, as it does not go in the
direction of having to define a derived class which depends on the
sensor model, as we did with the CameraSensorFactory.

But now I wonder, if pipeline handlers sub-class CameraSensor, they
cannot do that to augment it with information specific to a camera
sensor, unless we assume some pipelines -know- they will work with a
fixed set of sensors and implement a way to identify them at run-time
and instantiate the right derived class, which seems a custom solution
I would prefer not to see in mainline libcamera to be honest.

What remains is that pipeline handlers, being unable to extend
CameraSensor class with sensor-specific information, will have to
define a derived class for... well, right now for the ID only :)

I have not looked at the VIMC pipeline handler patch yet, but I wonder
if that we should not avoid making it extend the camera sensor class
and compute the camera ID without involving it at all.

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * 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               | 40 +++++++++++++++++++
>  src/libcamera/pipeline/vimc/vimc.cpp          | 19 ++++++++-
>  test/camera-sensor.cpp                        |  3 +-
>  test/libtest/vimc_sensor_test.h               | 29 ++++++++++++++
>  .../v4l2_videodevice_test.cpp                 |  4 +-
>  6 files changed, 94 insertions(+), 4 deletions(-)
>  create mode 100644 test/libtest/vimc_sensor_test.h
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 06c8292ca30129de..b3aaaeaf829d97c0 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -47,6 +47,7 @@ public:
>  	int init();
>
>  	const std::string &model() const { return model_; }
> +	const std::string &id() const { return id_; }
>  	const MediaEntity *entity() const { return entity_; }
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>  	const std::vector<Size> &sizes() const { return sizes_; }
> @@ -65,6 +66,7 @@ public:
>
>  protected:
>  	std::string logPrefix() const override;
> +	virtual std::string generateID() const;
>
>  private:
>  	const MediaEntity *entity_;
> @@ -72,6 +74,7 @@ private:
>  	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..282cedecaf28fb3c 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -204,6 +204,11 @@ int CameraSensor::init()
>  	if (ret < 0)
>  		return ret;
>
> +	/* Generate a unique ID for the sensor. */
> +	id_ = generateID();
> +	if (id_.empty())
> +		return -EINVAL;
> +
>  	/* Retrieve and store the camera sensor properties. */
>  	const ControlInfoMap &controls = subdev_->controls();
>  	int32_t propertyValue;
> @@ -283,6 +288,16 @@ int CameraSensor::init()
>   * \return The sensor model name
>   */
>
> +/**
> + * \fn CameraSensor::id()
> + * \brief Retrieve the sensor ID
> + *
> + * The sensor ID is a free-form string that uniquely identifies the sensor in
> + * the system. The ID satisfy the requirements to be used as a camera ID.
> + *
> + * \return The sensor ID
> + */
> +
>  /**
>   * \fn CameraSensor::entity()
>   * \brief Retrieve the sensor media entity
> @@ -541,4 +556,29 @@ std::string CameraSensor::logPrefix() const
>  	return "'" + entity_->name() + "'";
>  }
>
> +/**
> + * \brief Generate a sensor ID for the sensor
> + *
> + * The generated string satisfy the requirements to be used as ID for a camera.

What are those requirments, one could wonder when reading this comment
without the background provided by this series ?

I would, even if I understand it might be 'boring', repeat them here.
They're just three after all

> + * This default implementation retrieves the ID form the firmware ID associated
> + * with the sensors V4L2 subdevice, that meets this criteria.
> + *
> + * It is possible to override this function to allow for more advanced IDs to be
> + * generated for specific sensors. Any alternative implementation is responsible
> + * for that the generated ID satisfy the requirements to be used as a camera ID.
> + *

See above on the part

> + * \sa id()
> + * \return Sensor ID on success or empty string on failure

A string representing the sensor ID, or an empty string on failure

?

> + */
> +std::string CameraSensor::generateID() const
> +{
> +	std::string id;
> +	if (utils::tryLookupFirmwareID(subdev_->devicePath(), &id)) {
> +		LOG(CameraSensor, Error) << "Can't get sensor ID from firmware";
> +		return {};
> +	}
> +
> +	return id;
> +}

Long time debated issue: this is a private function which returns a string
which is then assigned to a class private field.
I wonder if:
1) it could inlined in the caller. it's 3 lines of code
2) if not, shouldn't it assign id_ here and return an error code ?

> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 4f461b928514022d..b3348ab5d987d506 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -38,6 +38,21 @@ namespace libcamera {
>
>  LOG_DEFINE_CATEGORY(VIMC)
>
> +class VimcCameraSensor : public CameraSensor
> +{
> +public:
> +	VimcCameraSensor(const MediaEntity *entity)
> +		: CameraSensor(entity)
> +	{
> +	}
> +
> +protected:
> +	std::string generateID() const override
> +	{
> +		return "VIMC " + entity()->name();
> +	}
> +};
> +

Ah here it is.

In my opinion, this is not necessary, and it could be done in the
pipeline handler by accessing the CameraSensor entity name.

I think the fact you need to declare a fake CameraSensor derived class
for tests is a sign this is not the best direction to take ?

Thanks
  j

>  class VimcCameraData : public CameraData
>  {
>  public:
> @@ -61,7 +76,7 @@ public:
>  	void bufferReady(FrameBuffer *buffer);
>
>  	MediaDevice *media_;
> -	CameraSensor *sensor_;
> +	VimcCameraSensor *sensor_;
>  	V4L2Subdevice *debayer_;
>  	V4L2Subdevice *scaler_;
>  	V4L2VideoDevice *video_;
> @@ -457,7 +472,7 @@ int VimcCameraData::init()
>  		return ret;
>
>  	/* Create and open the camera sensor, debayer, scaler and video device. */
> -	sensor_ = new CameraSensor(media_->getEntityByName("Sensor B"));
> +	sensor_ = new VimcCameraSensor(media_->getEntityByName("Sensor B"));
>  	ret = sensor_->init();
>  	if (ret)
>  		return ret;
> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> index 8c7fd1d2d4445db3..50157b234fd40933 100644
> --- a/test/camera-sensor.cpp
> +++ b/test/camera-sensor.cpp
> @@ -17,6 +17,7 @@
>  #include "libcamera/internal/v4l2_subdevice.h"
>
>  #include "test.h"
> +#include "vimc_sensor_test.h"
>
>  using namespace std;
>  using namespace libcamera;
> @@ -50,7 +51,7 @@ protected:
>  			return TestFail;
>  		}
>
> -		sensor_ = new CameraSensor(entity);
> +		sensor_ = new TestVimcCameraSensor(entity);
>  		if (sensor_->init() < 0) {
>  			cerr << "Unable to initialise camera sensor" << endl;
>  			return TestFail;
> diff --git a/test/libtest/vimc_sensor_test.h b/test/libtest/vimc_sensor_test.h
> new file mode 100644
> index 0000000000000000..1454a1c310f0dc9c
> --- /dev/null
> +++ b/test/libtest/vimc_sensor_test.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * vimc_sensor_test.h - vimc_sensor test class
> + */
> +#ifndef __LIBCAMERA_VIMC_SENSOR_TEST_H__
> +#define __LIBCAMERA_VIMC_SENSOR_TEST_H__
> +
> +#include "libcamera/internal/camera_sensor.h"
> +
> +using namespace libcamera;
> +
> +class TestVimcCameraSensor : public CameraSensor
> +{
> +public:
> +	TestVimcCameraSensor(const MediaEntity *entity)
> +		: CameraSensor(entity)
> +	{
> +	}
> +
> +protected:
> +	std::string generateID() const override
> +	{
> +		return "VIMC " + entity()->name();
> +	}
> +};
> +
> +#endif /* __LIBCAMERA_VIMC_SENSOR_TEST_H__ */
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> index f23aaf8f514bc050..c7a3479963be9b78 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> @@ -12,6 +12,8 @@
>  #include "libcamera/internal/device_enumerator.h"
>  #include "libcamera/internal/media_device.h"
>
> +#include "vimc_sensor_test.h"
> +
>  #include "v4l2_videodevice_test.h"
>
>  using namespace std;
> @@ -61,7 +63,7 @@ int V4L2VideoDeviceTest::init()
>  		return TestFail;
>
>  	if (driver_ == "vimc") {
> -		sensor_ = new CameraSensor(media_->getEntityByName("Sensor A"));
> +		sensor_ = new TestVimcCameraSensor(media_->getEntityByName("Sensor A"));
>  		if (sensor_->init())
>  			return TestFail;
>
> --
> 2.28.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel