[libcamera-devel,v7,7/9] libcamera: pipeline: uvcvideo: Generate unique camera names

9190 diff mbox series

Generate camera names that are unique and persistent between system
resets. The name is constructed from the USB device information as well
as the USB controller on the host.

Before this change example of camera names:

Venus USB2.0 Camera: Venus USB2
Logitech Webcam C930e

After this change the same cameras are:

\_SB_.PCI0.RP05.PXSX-2.1.1:1.0-0ac8:3420
\_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843

On OF-based system:

base/soc/usb@7e980000/usb-port@1-1.3:1.0-0ac8:3420

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
* Changes since v5
- New algorithm to generate IDs.

* Changes since v3
- Switch argument to generateName() to UVCCameraData pointer.
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 77 +++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

Hi Niklas,

Thank you for the patch.

On Tue, Aug 04, 2020 at 06:13:56PM +0200, Niklas Söderlund wrote:
> Generate camera names that are unique and persistent between system
> resets. The name is constructed from the USB device information as well
> as the USB controller on the host.
> 
> Before this change example of camera names:
> 
> Venus USB2.0 Camera: Venus USB2
> Logitech Webcam C930e
> 
> After this change the same cameras are:
> 
> \_SB_.PCI0.RP05.PXSX-2.1.1:1.0-0ac8:3420
> \_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843
> 
> On OF-based system:
> 
> base/soc/usb@7e980000/usb-port@1-1.3:1.0-0ac8:3420
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
> * Changes since v5
> - New algorithm to generate IDs.
> 
> * Changes since v3
> - Switch argument to generateName() to UVCCameraData pointer.
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 77 +++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 93e3dc17e3a7105e..70c057b543f85c7b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <algorithm>
> +#include <fstream>
>  #include <iomanip>
>  #include <math.h>
>  #include <tuple>
> @@ -20,6 +21,7 @@
>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
> +#include "libcamera/internal/sysfs.h"
>  #include "libcamera/internal/utils.h"
>  #include "libcamera/internal/v4l2_controls.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -81,6 +83,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>  
>  private:
> +	std::string generateID(const UVCCameraData *data);

s/ID/Id/

> +
>  	int processControl(ControlList *controls, unsigned int id,
>  			   const ControlValue &value);
>  	int processControls(UVCCameraData *data, Request *request);
> @@ -379,6 +383,71 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
>  	return 0;
>  }
>  
> +std::string PipelineHandlerUVC::generateID(const UVCCameraData *data)
> +{
> +	const std::string path = data->video_->devicePath();
> +
> +	/* Creata a device ID from the USB devices vendor and product ID. */
> +	std::string deviceId;
> +	for (const std::string &name : { "idVendor", "idProduct" }) {
> +		std::ifstream file(path + "/../" + name);
> +
> +		if (!file.is_open())
> +			return {};
> +
> +		std::string value;
> +		std::getline(file, value);
> +		file.close();
> +
> +		deviceId += value + (deviceId.empty() ? ":" : "");

		if (!deviceId.empty())
			deviceId += ":";

		deviceId += value;

so this will work with more than two iterations :-)

> +	}

Of course you could also use utils::join() if you wanted.

I'd move this block after the next, to follow the same order as in the
ID string, but maybe it's just me.

> +
> +	/*
> +	 * Create a USB ID from the device path which has the known format:
> +	 *
> +	 *	bus , "-", ports, ":", config, ".", interface ;

s/bus ,/ bus,/ (maybe path = ... ?)

> +	 *	bus = number ;
> +	 *	ports = port, [ ".", ports ] ;
> +	 *	port = number ;
> +	 *	config = number ;
> +	 *	interface = number ;
> +	 *
> +	 * Example: 3-2.4:1.0
> +	 *
> +	 * The bus is not guaranteed to be stable and needs to be stripped from
> +	 * the USB ID. The final USB ID is built up of the ports, config and
> +	 * interface properties.
> +	 *
> +	 * Example 2.4:1.0.
> +	 */
> +	std::string usbId = basename(path.c_str());
> +	usbId = usbId.substr(usbId.find('-') + 1, std::string::npos);

The second argument defaults to std::string::npos, so you could leave it
out if you wanted.

> +
> +	/* Create a controller ID from first device described in firmware. */
> +	std::string controllerId;
> +	std::string searchPath = path;
> +	while (true) {
> +		searchPath += "/..";
> +		char *realPath = realpath(searchPath.c_str(), nullptr);
> +		if (!realPath) {
> +			LOG(UVC, Error) << "Failed to lookup " << searchPath;
> +			return {};
> +		}
> +		searchPath = realPath;
> +		free(realPath);
> +
> +		if (searchPath.empty() || searchPath == "/") {
> +			LOG(UVC, Error) << "Can not find controller ID";
> +			return {};
> +		}

As path is guaranteed to be a real path, I think this could be
simplified.

		std::string::size_type pos = searchPath.rfind('-');
		if (pos <= 1) {
			LOG(UVC, Error) << "Can not find controller ID";
			return {};
		}

		searchPath = searchPath::substr(0, pos);

> +
> +		if (!sysfs::firmwareId(searchPath, &controllerId))
> +			break;
> +	}
> +
> +	return controllerId + "-" + usbId + "-" + deviceId;
> +}
> +
>  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  {
>  	MediaDevice *media;
> @@ -405,8 +474,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  		return false;
>  
>  	/* Create and register the camera. */
> +	std::string id = generateID(data.get());
> +	if (id.empty()) {
> +		LOG(UVC, Error) << "Failed to generate camera ID";
> +		return false;
> +	}
> +
>  	std::set<Stream *> streams{ &data->stream_ };
> -	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> +	std::shared_ptr<Camera> camera = Camera::create(this, id, streams);
>  	registerCamera(std::move(camera), std::move(data));
>  
>  	/* Enable hot-unplug notifications. */

Hi Laurent,

Thanks for your feedback.

On 2020-08-04 21:52:47 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Aug 04, 2020 at 06:13:56PM +0200, Niklas Söderlund wrote:
> > Generate camera names that are unique and persistent between system
> > resets. The name is constructed from the USB device information as well
> > as the USB controller on the host.
> > 
> > Before this change example of camera names:
> > 
> > Venus USB2.0 Camera: Venus USB2
> > Logitech Webcam C930e
> > 
> > After this change the same cameras are:
> > 
> > \_SB_.PCI0.RP05.PXSX-2.1.1:1.0-0ac8:3420
> > \_SB_.PCI0.RP05.PXSX-2.4:1.0-046d:0843
> > 
> > On OF-based system:
> > 
> > base/soc/usb@7e980000/usb-port@1-1.3:1.0-0ac8:3420
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> > * Changes since v5
> > - New algorithm to generate IDs.
> > 
> > * Changes since v3
> > - Switch argument to generateName() to UVCCameraData pointer.
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 77 +++++++++++++++++++-
> >  1 file changed, 76 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 93e3dc17e3a7105e..70c057b543f85c7b 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >  
> >  #include <algorithm>
> > +#include <fstream>
> >  #include <iomanip>
> >  #include <math.h>
> >  #include <tuple>
> > @@ -20,6 +21,7 @@
> >  #include "libcamera/internal/log.h"
> >  #include "libcamera/internal/media_device.h"
> >  #include "libcamera/internal/pipeline_handler.h"
> > +#include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/utils.h"
> >  #include "libcamera/internal/v4l2_controls.h"
> >  #include "libcamera/internal/v4l2_videodevice.h"
> > @@ -81,6 +83,8 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >  
> >  private:
> > +	std::string generateID(const UVCCameraData *data);
> 
> s/ID/Id/
> 
> > +
> >  	int processControl(ControlList *controls, unsigned int id,
> >  			   const ControlValue &value);
> >  	int processControls(UVCCameraData *data, Request *request);
> > @@ -379,6 +383,71 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> >  	return 0;
> >  }
> >  
> > +std::string PipelineHandlerUVC::generateID(const UVCCameraData *data)
> > +{
> > +	const std::string path = data->video_->devicePath();
> > +
> > +	/* Creata a device ID from the USB devices vendor and product ID. */
> > +	std::string deviceId;
> > +	for (const std::string &name : { "idVendor", "idProduct" }) {
> > +		std::ifstream file(path + "/../" + name);
> > +
> > +		if (!file.is_open())
> > +			return {};
> > +
> > +		std::string value;
> > +		std::getline(file, value);
> > +		file.close();
> > +
> > +		deviceId += value + (deviceId.empty() ? ":" : "");
> 
> 		if (!deviceId.empty())
> 			deviceId += ":";
> 
> 		deviceId += value;
> 
> so this will work with more than two iterations :-)

:-)

> 
> > +	}
> 
> Of course you could also use utils::join() if you wanted.
> 
> I'd move this block after the next, to follow the same order as in the
> ID string, but maybe it's just me.

I had not noticed but now that it's pointed out it drives me mad, I will 
sort them to match the output order :-)

> 
> > +
> > +	/*
> > +	 * Create a USB ID from the device path which has the known format:
> > +	 *
> > +	 *	bus , "-", ports, ":", config, ".", interface ;
> 
> s/bus ,/ bus,/ (maybe path = ... ?)

Good catch, I also like adding of path = ... .

> 
> > +	 *	bus = number ;
> > +	 *	ports = port, [ ".", ports ] ;
> > +	 *	port = number ;
> > +	 *	config = number ;
> > +	 *	interface = number ;
> > +	 *
> > +	 * Example: 3-2.4:1.0
> > +	 *
> > +	 * The bus is not guaranteed to be stable and needs to be stripped from
> > +	 * the USB ID. The final USB ID is built up of the ports, config and
> > +	 * interface properties.
> > +	 *
> > +	 * Example 2.4:1.0.
> > +	 */
> > +	std::string usbId = basename(path.c_str());
> > +	usbId = usbId.substr(usbId.find('-') + 1, std::string::npos);
> 
> The second argument defaults to std::string::npos, so you could leave it
> out if you wanted.
> 
> > +
> > +	/* Create a controller ID from first device described in firmware. */
> > +	std::string controllerId;
> > +	std::string searchPath = path;
> > +	while (true) {
> > +		searchPath += "/..";
> > +		char *realPath = realpath(searchPath.c_str(), nullptr);
> > +		if (!realPath) {
> > +			LOG(UVC, Error) << "Failed to lookup " << searchPath;
> > +			return {};
> > +		}
> > +		searchPath = realPath;
> > +		free(realPath);
> > +
> > +		if (searchPath.empty() || searchPath == "/") {
> > +			LOG(UVC, Error) << "Can not find controller ID";
> > +			return {};
> > +		}
> 
> As path is guaranteed to be a real path, I think this could be
> simplified.
> 
> 		std::string::size_type pos = searchPath.rfind('-');

With s@'-'@'/'@ it works and is much nicer, thanks for this snippet.

> 		if (pos <= 1) {
> 			LOG(UVC, Error) << "Can not find controller ID";
> 			return {};
> 		}
> 
> 		searchPath = searchPath::substr(0, pos);
> 
> > +
> > +		if (!sysfs::firmwareId(searchPath, &controllerId))
> > +			break;
> > +	}
> > +
> > +	return controllerId + "-" + usbId + "-" + deviceId;
> > +}
> > +
> >  bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  {
> >  	MediaDevice *media;
> > @@ -405,8 +474,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  		return false;
> >  
> >  	/* Create and register the camera. */
> > +	std::string id = generateID(data.get());
> > +	if (id.empty()) {
> > +		LOG(UVC, Error) << "Failed to generate camera ID";
> > +		return false;
> > +	}
> > +
> >  	std::set<Stream *> streams{ &data->stream_ };
> > -	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> > +	std::shared_ptr<Camera> camera = Camera::create(this, id, streams);
> >  	registerCamera(std::move(camera), std::move(data));
> >  
> >  	/* Enable hot-unplug notifications. */
> 
> -- 
> Regards,
> 
> Laurent Pinchart