[libcamera-devel,v4,2/4] libcamera: camera_utils: Add a file for utils common to sensor and lens

14706 diff mbox series

Add a file to locate utils common to camera sensor and the following lens
class. The patch also moves two functions from camera_sensor.cpp used to
generate id and model name for the sensor into the file.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
---
 include/libcamera/internal/camera_sensor.h |  1 -
 include/libcamera/internal/camera_utils.h  | 22 +++++
 include/libcamera/internal/meson.build     |  1 +
 src/libcamera/camera_sensor.cpp            | 45 ++---------
 src/libcamera/camera_utils.cpp             | 94 ++++++++++++++++++++++
 src/libcamera/meson.build                  |  1 +
 6 files changed, 124 insertions(+), 40 deletions(-)
 create mode 100644 include/libcamera/internal/camera_utils.h
 create mode 100644 src/libcamera/camera_utils.cpp

Hi Han-Lin,

Quoting Han-Lin Chen (2021-11-23 12:37:49)
> Add a file to locate utils common to camera sensor and the following lens
> class. The patch also moves two functions from camera_sensor.cpp used to
> generate id and model name for the sensor into the file.
> 

I fear I'm about to get shot down by others on this as people's opinions
might differ ...

Splitting the generateID and model out of this to be able to re-use them
sounds like a great idea ... (and the bit I'm scared of)...

I would have thought that generateID() could be done directly by the
V4L2VideoDevice

And the Model ... could be provided by the V4L2SubDevice....

These are the components that know about those details, and any
heuristics there are based on assumptions about those specific device
forms...


Some typo's highlighted during the move below, which aren't a fault of
the move but could be fixed up while moving...

> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 -
>  include/libcamera/internal/camera_utils.h  | 22 +++++
>  include/libcamera/internal/meson.build     |  1 +
>  src/libcamera/camera_sensor.cpp            | 45 ++---------
>  src/libcamera/camera_utils.cpp             | 94 ++++++++++++++++++++++
>  src/libcamera/meson.build                  |  1 +
>  6 files changed, 124 insertions(+), 40 deletions(-)
>  create mode 100644 include/libcamera/internal/camera_utils.h
>  create mode 100644 src/libcamera/camera_utils.cpp
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index d25a1165..134108ff 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -66,7 +66,6 @@ protected:
>  private:
>         LIBCAMERA_DISABLE_COPY(CameraSensor)
>  
> -       int generateId();
>         int validateSensorDriver();
>         void initVimcDefaultProperties();
>         void initStaticProperties();
> diff --git a/include/libcamera/internal/camera_utils.h b/include/libcamera/internal/camera_utils.h
> new file mode 100644
> index 00000000..04c5e76f
> --- /dev/null
> +++ b/include/libcamera/internal/camera_utils.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_utils.h - Camera related utilities
> + */
> +#ifndef __LIBCAMERA_INTERNAL_CAMERA_UTILS_H__
> +#define __LIBCAMERA_INTERNAL_CAMERA_UTILS_H__
> +
> +#include <string.h>
> +
> +#include "libcamera/internal/v4l2_device.h"
> +
> +namespace libcamera {
> +
> +std::string extractModelFromEntityName(const std::string &entityName);
> +std::string generateIdForV4L2Device(const V4L2Device *dev,
> +                                   const std::string &model);
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_CAMERA_UTILS_H__ */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 665fd6de..de8630fe 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -16,6 +16,7 @@ libcamera_internal_headers = files([
>      'camera_controls.h',
>      'camera_sensor.h',
>      'camera_sensor_properties.h',
> +    'camera_utils.h',
>      'control_serializer.h',
>      'control_validator.h',
>      'delayed_controls.h',
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..e91c2b0a 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -6,14 +6,12 @@
>   */
>  
>  #include "libcamera/internal/camera_sensor.h"
> -#include "libcamera/internal/media_device.h"
>  
>  #include <algorithm>
>  #include <float.h>
>  #include <iomanip>
>  #include <limits.h>
>  #include <math.h>
> -#include <regex>
>  #include <string.h>
>  
>  #include <libcamera/property_ids.h>
> @@ -22,8 +20,9 @@
>  
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera_sensor_properties.h"
> +#include "libcamera/internal/camera_utils.h"
>  #include "libcamera/internal/formats.h"
> -#include "libcamera/internal/sysfs.h"
> +#include "libcamera/internal/media_device.h"
>  
>  /**
>   * \file camera_sensor.h
> @@ -366,21 +365,13 @@ int CameraSensor::initProperties()
>          * part of the entity name before the first space if the name contains
>          * an I2C address, and use the full entity name otherwise.
>          */
> -       std::string entityName = entity_->name();
> -       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> -       std::smatch match;
> -
> -       if (std::regex_search(entityName, match, i2cRegex))
> -               model_ = entityName.substr(0, entityName.find(' '));
> -       else
> -               model_ = entityName;
> -
> +       model_ = extractModelFromEntityName(entity_->name());
>         properties_.set(properties::Model, utils::toAscii(model_));
>  
>         /* Generate a unique ID for the sensor. */
> -       int ret = generateId();
> -       if (ret)
> -               return ret;
> +       id_ = generateIdForV4L2Device(subdev_.get(), model_);
> +       if (id_.empty())
> +               return -EINVAL;
>  
>         /* Initialize the static properties from the sensor database. */
>         initStaticProperties();
> @@ -820,28 +811,4 @@ 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.find("/sys/devices/platform/", 0) == 0) {
> -               id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
> -               return 0;
> -       }
> -
> -       LOG(CameraSensor, Error) << "Can't generate sensor ID";
> -       return -EINVAL;
> -}
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/camera_utils.cpp b/src/libcamera/camera_utils.cpp
> new file mode 100644
> index 00000000..b42911b4
> --- /dev/null
> +++ b/src/libcamera/camera_utils.cpp
> @@ -0,0 +1,94 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * camera_utils.cpp - Camera related utilities
> + */
> +
> +#include "libcamera/internal/camera_utils.h"
> +
> +#include <regex>
> +
> +#include "libcamera/internal/sysfs.h"
> +
> +/**
> + * \file camera_utils.h
> + * \brief Utilities for Camera instances
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \brief Extract the sensor or lens model name from the media entity name
> + * \param[in] entityName The entity name of a lens or sensor
> + *
> + * \return Model name as string
> + */
> +std::string extractModelFromEntityName(const std::string &entityName)
> +{
> +       /*
> +        * Extract the sensor or lens model name from the media entity name.
> +        *
> +        * There is no standardized naming scheme for sensor entities in the
> +        * Linux kernel at the moment.
> +        *
> +        * - The most common rule, used by I2C sensors, associates the model
> +        *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> +        *
> +        * - When the sensor exposes multiple subdevs, the model name is
> +        *   usually followed by a function name, as in the smiapp driver (e.g.
> +        *   'jt8ew9 pixel_array 0-0010').
> +        *
> +        * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> +        *
> +        * Other schemes probably exist. As a best effort heuristic, use the
> +        * part of the entity name before the first space if the name contains
> +        * an I2C address, and use the full entity name otherwise.
> +        */
> +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> +       std::smatch match;
> +
> +       std::string model;
> +       if (std::regex_search(entityName, match, i2cRegex))
> +               model = entityName.substr(0, entityName.find(' '));
> +       else
> +               model = entityName;
> +
> +       return model;
> +}
> +
> +/**
> + * \brief Generate ID for V4L2 device
> + * \param[in] dev The V4L2Device
> + * \param[in] model The ModelName
> + *
> + * Contruct ID from the firmware description. If it doesn't exist, contruct it

/Contruct/Construct/
/contruct/contruct/

> + * from the device path and the provided model name.
> + * If both fails, return an empty string.

/fails/fail/

> + *
> + * \return ID as string
> + */
> +std::string generateIdForV4L2Device(const V4L2Device *dev,
> +                                   const std::string &model)
> +{
> +       const std::string devPath = dev->devicePath();
> +
> +       /* Try to get ID from firmware description. */
> +       std::string id = sysfs::firmwareNodePath(devPath);
> +       if (!id.empty())
> +               return id;
> +
> +       /*
> +        * Virtual device not described in firmware
> +        *
> +        * Verify it's a platform device and construct ID from the deive path

/deive/device/

> +        * and model of a sensor or lens.
> +        */
> +       if (devPath.find("/sys/devices/platform/", 0) == 0) {
> +               return devPath.substr(strlen("/sys/devices/")) + " " + model;
> +       }
> +
> +       return "";
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 6727a777..563ed861 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -8,6 +8,7 @@ libcamera_sources = files([
>      'camera_manager.cpp',
>      'camera_sensor.cpp',
>      'camera_sensor_properties.cpp',
> +    'camera_utils.cpp',
>      'controls.cpp',
>      'control_serializer.cpp',
>      'control_validator.cpp',
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog
>

Hello,

On Tue, Nov 23, 2021 at 04:51:06PM +0000, Kieran Bingham wrote:
> Quoting Han-Lin Chen (2021-11-23 12:37:49)
> > Add a file to locate utils common to camera sensor and the following lens
> > class. The patch also moves two functions from camera_sensor.cpp used to
> > generate id and model name for the sensor into the file.
> 
> I fear I'm about to get shot down by others on this as people's opinions
> might differ ...

Don't we scared :-) (mental self-note: I need to work on communication
skills if I'm scaring people)

> Splitting the generateID and model out of this to be able to re-use them
> sounds like a great idea ... (and the bit I'm scared of)...

generateIdForV4L2Device() is only used in CameraSensor in this patch
series, so I'd keep the function in that class for now.

> I would have thought that generateID() could be done directly by the
> V4L2VideoDevice
> 
> And the Model ... could be provided by the V4L2SubDevice....

I like this idea I think. There's a small risk we'll need different
heuristics for different types of subdevs, but I'd rather work on the
kernel side to improve this (adding a model field to media_entity would
be useful for instance).

> These are the components that know about those details, and any
> heuristics there are based on assumptions about those specific device
> forms...
> 
> 
> Some typo's highlighted during the move below, which aren't a fault of
> the move but could be fixed up while moving...
> 
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 -
> >  include/libcamera/internal/camera_utils.h  | 22 +++++
> >  include/libcamera/internal/meson.build     |  1 +
> >  src/libcamera/camera_sensor.cpp            | 45 ++---------
> >  src/libcamera/camera_utils.cpp             | 94 ++++++++++++++++++++++
> >  src/libcamera/meson.build                  |  1 +
> >  6 files changed, 124 insertions(+), 40 deletions(-)
> >  create mode 100644 include/libcamera/internal/camera_utils.h
> >  create mode 100644 src/libcamera/camera_utils.cpp
> > 
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index d25a1165..134108ff 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -66,7 +66,6 @@ protected:
> >  private:
> >         LIBCAMERA_DISABLE_COPY(CameraSensor)
> >  
> > -       int generateId();
> >         int validateSensorDriver();
> >         void initVimcDefaultProperties();
> >         void initStaticProperties();
> > diff --git a/include/libcamera/internal/camera_utils.h b/include/libcamera/internal/camera_utils.h
> > new file mode 100644
> > index 00000000..04c5e76f
> > --- /dev/null
> > +++ b/include/libcamera/internal/camera_utils.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * camera_utils.h - Camera related utilities
> > + */
> > +#ifndef __LIBCAMERA_INTERNAL_CAMERA_UTILS_H__
> > +#define __LIBCAMERA_INTERNAL_CAMERA_UTILS_H__
> > +
> > +#include <string.h>
> > +
> > +#include "libcamera/internal/v4l2_device.h"
> > +
> > +namespace libcamera {
> > +
> > +std::string extractModelFromEntityName(const std::string &entityName);
> > +std::string generateIdForV4L2Device(const V4L2Device *dev,
> > +                                   const std::string &model);
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_INTERNAL_CAMERA_UTILS_H__ */
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 665fd6de..de8630fe 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -16,6 +16,7 @@ libcamera_internal_headers = files([
> >      'camera_controls.h',
> >      'camera_sensor.h',
> >      'camera_sensor_properties.h',
> > +    'camera_utils.h',
> >      'control_serializer.h',
> >      'control_validator.h',
> >      'delayed_controls.h',
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 9fdb8c09..e91c2b0a 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -6,14 +6,12 @@
> >   */
> >  
> >  #include "libcamera/internal/camera_sensor.h"
> > -#include "libcamera/internal/media_device.h"
> >  
> >  #include <algorithm>
> >  #include <float.h>
> >  #include <iomanip>
> >  #include <limits.h>
> >  #include <math.h>
> > -#include <regex>
> >  #include <string.h>
> >  
> >  #include <libcamera/property_ids.h>
> > @@ -22,8 +20,9 @@
> >  
> >  #include "libcamera/internal/bayer_format.h"
> >  #include "libcamera/internal/camera_sensor_properties.h"
> > +#include "libcamera/internal/camera_utils.h"
> >  #include "libcamera/internal/formats.h"
> > -#include "libcamera/internal/sysfs.h"
> > +#include "libcamera/internal/media_device.h"
> >  
> >  /**
> >   * \file camera_sensor.h
> > @@ -366,21 +365,13 @@ int CameraSensor::initProperties()
> >          * part of the entity name before the first space if the name contains
> >          * an I2C address, and use the full entity name otherwise.
> >          */
> > -       std::string entityName = entity_->name();
> > -       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> > -       std::smatch match;
> > -
> > -       if (std::regex_search(entityName, match, i2cRegex))
> > -               model_ = entityName.substr(0, entityName.find(' '));
> > -       else
> > -               model_ = entityName;
> > -
> > +       model_ = extractModelFromEntityName(entity_->name());
> >         properties_.set(properties::Model, utils::toAscii(model_));
> >  
> >         /* Generate a unique ID for the sensor. */
> > -       int ret = generateId();
> > -       if (ret)
> > -               return ret;
> > +       id_ = generateIdForV4L2Device(subdev_.get(), model_);
> > +       if (id_.empty())
> > +               return -EINVAL;
> >  
> >         /* Initialize the static properties from the sensor database. */
> >         initStaticProperties();
> > @@ -820,28 +811,4 @@ 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.find("/sys/devices/platform/", 0) == 0) {
> > -               id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
> > -               return 0;
> > -       }
> > -
> > -       LOG(CameraSensor, Error) << "Can't generate sensor ID";
> > -       return -EINVAL;
> > -}
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/camera_utils.cpp b/src/libcamera/camera_utils.cpp
> > new file mode 100644
> > index 00000000..b42911b4
> > --- /dev/null
> > +++ b/src/libcamera/camera_utils.cpp
> > @@ -0,0 +1,94 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2021, Google Inc.
> > + *
> > + * camera_utils.cpp - Camera related utilities
> > + */
> > +
> > +#include "libcamera/internal/camera_utils.h"
> > +
> > +#include <regex>
> > +
> > +#include "libcamera/internal/sysfs.h"
> > +
> > +/**
> > + * \file camera_utils.h
> > + * \brief Utilities for Camera instances
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \brief Extract the sensor or lens model name from the media entity name
> > + * \param[in] entityName The entity name of a lens or sensor
> > + *
> > + * \return Model name as string
> > + */
> > +std::string extractModelFromEntityName(const std::string &entityName)
> > +{
> > +       /*
> > +        * Extract the sensor or lens model name from the media entity name.
> > +        *
> > +        * There is no standardized naming scheme for sensor entities in the
> > +        * Linux kernel at the moment.
> > +        *
> > +        * - The most common rule, used by I2C sensors, associates the model
> > +        *   name with the I2C bus number and address (e.g. 'imx219 0-0010').
> > +        *
> > +        * - When the sensor exposes multiple subdevs, the model name is
> > +        *   usually followed by a function name, as in the smiapp driver (e.g.
> > +        *   'jt8ew9 pixel_array 0-0010').
> > +        *
> > +        * - The vimc driver names its sensors 'Sensor A' and 'Sensor B'.
> > +        *
> > +        * Other schemes probably exist. As a best effort heuristic, use the
> > +        * part of the entity name before the first space if the name contains
> > +        * an I2C address, and use the full entity name otherwise.
> > +        */
> > +       std::regex i2cRegex{ " [0-9]+-[0-9a-f]{4}" };
> > +       std::smatch match;
> > +
> > +       std::string model;
> > +       if (std::regex_search(entityName, match, i2cRegex))
> > +               model = entityName.substr(0, entityName.find(' '));
> > +       else
> > +               model = entityName;
> > +
> > +       return model;
> > +}
> > +
> > +/**
> > + * \brief Generate ID for V4L2 device
> > + * \param[in] dev The V4L2Device
> > + * \param[in] model The ModelName
> > + *
> > + * Contruct ID from the firmware description. If it doesn't exist, contruct it
> 
> /Contruct/Construct/
> /contruct/contruct/
> 
> > + * from the device path and the provided model name.
> > + * If both fails, return an empty string.
> 
> /fails/fail/
> 
> > + *
> > + * \return ID as string
> > + */
> > +std::string generateIdForV4L2Device(const V4L2Device *dev,
> > +                                   const std::string &model)
> > +{
> > +       const std::string devPath = dev->devicePath();
> > +
> > +       /* Try to get ID from firmware description. */
> > +       std::string id = sysfs::firmwareNodePath(devPath);
> > +       if (!id.empty())
> > +               return id;
> > +
> > +       /*
> > +        * Virtual device not described in firmware
> > +        *
> > +        * Verify it's a platform device and construct ID from the deive path
> 
> /deive/device/
> 
> > +        * and model of a sensor or lens.
> > +        */
> > +       if (devPath.find("/sys/devices/platform/", 0) == 0) {
> > +               return devPath.substr(strlen("/sys/devices/")) + " " + model;
> > +       }
> > +
> > +       return "";
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 6727a777..563ed861 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -8,6 +8,7 @@ libcamera_sources = files([
> >      'camera_manager.cpp',
> >      'camera_sensor.cpp',
> >      'camera_sensor_properties.cpp',
> > +    'camera_utils.cpp',
> >      'controls.cpp',
> >      'control_serializer.cpp',
> >      'control_validator.cpp',