| Message ID | 20220903181037.1406-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 52660f2b13dd9f1032de5e50c416845def66a838 |
| Headers | show |
| Series |
|
| Related | show |
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@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. But that's not required for this patch. I think this is still valid alone: Reviewed-by: Kieran Bingham <kieran.bingham@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 >
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@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@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) > > {
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); @@ -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) {
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@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 149 ++++++++++--------- 1 file changed, 78 insertions(+), 71 deletions(-)