[libcamera-devel] [PATCH 1/3] pipeline: uvcvideo: Move camera ID generation to UVCCameraData class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 6 16:40:22 CEST 2022
On Tue, Sep 06, 2022 at 10:25:03AM +0100, Kieran Bingham wrote:
> Quoting Laurent Pinchart via libcamera-devel (2022-09-03 19:10:35)
> > Move the camera ID generation to UVCCameraData, and cache the ID in that
> > class. This will be useful to access the ID in multiple locations, such
> > as when printing error messages.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 149 ++++++++++---------
> > 1 file changed, 78 insertions(+), 71 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 2ae640a31f68..22b67faf309e 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -46,8 +46,15 @@ public:
> > ControlInfoMap::Map *ctrls);
> > void bufferReady(FrameBuffer *buffer);
> >
> > + const std::string &id() const { return id_; }
> > +
> > std::unique_ptr<V4L2VideoDevice> video_;
> > Stream stream_;
> > +
> > +private:
> > + bool generateId();
> > +
> > + std::string id_;
> > };
> >
> > class UVCCameraConfiguration : public CameraConfiguration
> > @@ -81,8 +88,6 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > - std::string generateId(const UVCCameraData *data);
> > -
> > int processControl(ControlList *controls, unsigned int id,
> > const ControlValue &value);
> > int processControls(UVCCameraData *data, Request *request);
> > @@ -384,69 +389,6 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> > return 0;
> > }
> >
> > -std::string PipelineHandlerUVC::generateId(const UVCCameraData *data)
> > -{
> > - const std::string path = data->video_->devicePath();
> > -
> > - /* Create a controller ID from first device described in firmware. */
> > - std::string controllerId;
> > - std::string searchPath = path;
> > - while (true) {
> > - std::string::size_type pos = searchPath.rfind('/');
> > - if (pos <= 1) {
> > - LOG(UVC, Error) << "Can not find controller ID";
> > - return {};
> > - }
> > -
> > - searchPath = searchPath.substr(0, pos);
> > -
> > - controllerId = sysfs::firmwareNodePath(searchPath);
> > - if (!controllerId.empty())
> > - break;
> > - }
> > -
> > - /*
> > - * Create a USB ID from the device path which has the known format:
> > - *
> > - * path = bus, "-", ports, ":", config, ".", interface ;
> > - * 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 = utils::basename(path.c_str());
> > - usbId = usbId.substr(usbId.find('-') + 1);
> > -
> > - /* Creata a device ID from the USB devices vendor and product ID. */
> > - std::string deviceId;
> > - for (const char *name : { "idVendor", "idProduct" }) {
> > - std::ifstream file(path + "/../" + name);
> > -
> > - if (!file.is_open())
> > - return {};
> > -
> > - std::string value;
> > - std::getline(file, value);
> > - file.close();
> > -
> > - if (!deviceId.empty())
> > - deviceId += ":";
> > -
> > - deviceId += value;
> > - }
> > -
> > - return controllerId + "-" + usbId + "-" + deviceId;
> > -}
> > -
> > bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > {
> > MediaDevice *media;
> > @@ -462,12 +404,7 @@ 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::string id = data->id();
> > std::set<Stream *> streams{ &data->stream_ };
> > std::shared_ptr<Camera> camera =
> > Camera::create(std::move(data), id, streams);
>
> I wonder if CameraData should always be responsible for creating and
> storing the id (even for other pipelines). Then Camera::create() would
> get the id from the data it's already being passed.
I've thought about it and then decided to explore a different rabbit
hole :-) I think it's a good idea though, and would prevent a common bug
(that was present in the first version of this patch, before I sent it).
I initially wrote
std::shared_ptr<Camera> camera =
Camera::create(std::move(data), data->id(), streams);
> But that's not required for this patch.
>
> I think this is still valid alone:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > @@ -502,6 +439,12 @@ int UVCCameraData::init(MediaDevice *media)
> >
> > video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
> >
> > + /* Generate the camera ID. */
> > + if (!generateId()) {
> > + LOG(UVC, Error) << "Failed to generate camera ID";
> > + return -EINVAL;
> > + }
> > +
> > properties_.set(properties::Model, utils::toAscii(media->model()));
> >
> > /*
> > @@ -563,6 +506,70 @@ int UVCCameraData::init(MediaDevice *media)
> > return 0;
> > }
> >
> > +bool UVCCameraData::generateId()
> > +{
> > + const std::string path = video_->devicePath();
> > +
> > + /* Create a controller ID from first device described in firmware. */
> > + std::string controllerId;
> > + std::string searchPath = path;
> > + while (true) {
> > + std::string::size_type pos = searchPath.rfind('/');
> > + if (pos <= 1) {
> > + LOG(UVC, Error) << "Can not find controller ID";
> > + return false;
> > + }
> > +
> > + searchPath = searchPath.substr(0, pos);
> > +
> > + controllerId = sysfs::firmwareNodePath(searchPath);
> > + if (!controllerId.empty())
> > + break;
> > + }
> > +
> > + /*
> > + * Create a USB ID from the device path which has the known format:
> > + *
> > + * path = bus, "-", ports, ":", config, ".", interface ;
> > + * 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 = utils::basename(path.c_str());
> > + usbId = usbId.substr(usbId.find('-') + 1);
> > +
> > + /* Creata a device ID from the USB devices vendor and product ID. */
> > + std::string deviceId;
> > + for (const char *name : { "idVendor", "idProduct" }) {
> > + std::ifstream file(path + "/../" + name);
> > +
> > + if (!file.is_open())
> > + return false;
> > +
> > + std::string value;
> > + std::getline(file, value);
> > + file.close();
> > +
> > + if (!deviceId.empty())
> > + deviceId += ":";
> > +
> > + deviceId += value;
> > + }
> > +
> > + id_ = controllerId + "-" + usbId + "-" + deviceId;
> > + return true;
> > +}
> > +
> > void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > ControlInfoMap::Map *ctrls)
> > {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list