| Message ID | 20200728234225.3505868-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Wed, Jul 29, 2020 at 01:42:24AM +0200, Niklas Söderlund wrote: > Generate camera names that are unique and persistent between system > resets. The name is constructed from the USB vendor and product > information that is stored on USB device itself and the USB bus and > device numbers where the hardware is plugged in. > > Before this change example of camera names: > > Venus USB2.0 Camera: Venus USB2 > Logitech Webcam C930e > > After this change the same cameras are: > > 0ac8:3420:3:10 > 046d:0843:3:4 > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 34 +++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 93e3dc17e3a7105e..72f3f5d9f7c414d0 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> > @@ -81,6 +82,9 @@ public: > bool match(DeviceEnumerator *enumerator) override; > > private: > + std::string generateName(const MediaDevice *media, > + const std::string path); > + > int processControl(ControlList *controls, unsigned int id, > const ControlValue &value); > int processControls(UVCCameraData *data, Request *request); > @@ -379,6 +383,28 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > return 0; > } > > +std::string PipelineHandlerUVC::generateName(const MediaDevice *media, Is media used ? > + const std::string path) I would have passed the CameraData instance and got the path from there. > +{ > + static const std::vector<std::string> files = { "idVendor", "idProduct", "busnum", "devnum" }; less than 120 cols ? > + std::vector<std::string> values; > + std::string value; > + > + for (const std::string &name : files) { > + std::ifstream file(path + "/../" + name); > + > + if (!file.is_open()) > + return ""; Return or just skip that file ? If you want to return is this worth an error message ? > + > + std::getline(file, value); > + file.close(); This applies to the other patches as well, reading std::ifstream::close documentation I see: "Note that any open file is automatically closed when the ifstream object is destroyed." Do you need to call close ? I got suspicious as it felt weird that an STL library opens a file on construction but requires manual closure.. > + > + values.push_back(value); > + } > + > + return utils::join(values, ":"); > +} > + > bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > { > MediaDevice *media; > @@ -405,8 +431,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > return false; > > /* Create and register the camera. */ > + std::string name = generateName(media, data->video_->devicePath()); > + if (name.empty()) { > + LOG(UVC, Error) << "Failed to generate camera name"; > + return false; > + } Oh the error is here, and is better here than in the function. But then you have to fail if you don't find one of the files... I think what you have here is right then... > + > 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, name, streams); nits apart Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > registerCamera(std::move(camera), std::move(data)); > > /* Enable hot-unplug notifications. */ > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your comments. On 2020-07-29 10:14:59 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Wed, Jul 29, 2020 at 01:42:24AM +0200, Niklas Söderlund wrote: > > Generate camera names that are unique and persistent between system > > resets. The name is constructed from the USB vendor and product > > information that is stored on USB device itself and the USB bus and > > device numbers where the hardware is plugged in. > > > > Before this change example of camera names: > > > > Venus USB2.0 Camera: Venus USB2 > > Logitech Webcam C930e > > > > After this change the same cameras are: > > > > 0ac8:3420:3:10 > > 046d:0843:3:4 > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 34 +++++++++++++++++++- > > 1 file changed, 33 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 93e3dc17e3a7105e..72f3f5d9f7c414d0 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> > > @@ -81,6 +82,9 @@ public: > > bool match(DeviceEnumerator *enumerator) override; > > > > private: > > + std::string generateName(const MediaDevice *media, > > + const std::string path); > > + > > int processControl(ControlList *controls, unsigned int id, > > const ControlValue &value); > > int processControls(UVCCameraData *data, Request *request); > > @@ -379,6 +383,28 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) > > return 0; > > } > > > > +std::string PipelineHandlerUVC::generateName(const MediaDevice *media, > > Is media used ? > > > + const std::string path) > > I would have passed the CameraData instance and got the path from > there. I have no strong preference so I will make it so in v4. > > > +{ > > + static const std::vector<std::string> files = { "idVendor", "idProduct", "busnum", "devnum" }; > > less than 120 cols ? > > > + std::vector<std::string> values; > > + std::string value; > > + > > + for (const std::string &name : files) { > > + std::ifstream file(path + "/../" + name); > > + > > + if (!file.is_open()) > > + return ""; > > Return or just skip that file ? If you want to return is this worth an > error message ? All files are mandatory so its all pass or we should fail. > > + > > + std::getline(file, value); > > + file.close(); > > This applies to the other patches as well, reading > std::ifstream::close documentation I see: > > "Note that any open file is automatically closed when the ifstream object is destroyed." > > Do you need to call close ? I got suspicious as it felt weird that an > STL library opens a file on construction but requires manual closure.. I like to explicitly close the file, specially here as we loop I think it makes the code more readable. > > > + > > + values.push_back(value); > > + } > > + > > + return utils::join(values, ":"); > > +} > > + > > bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > { > > MediaDevice *media; > > @@ -405,8 +431,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) > > return false; > > > > /* Create and register the camera. */ > > + std::string name = generateName(media, data->video_->devicePath()); > > + if (name.empty()) { > > + LOG(UVC, Error) << "Failed to generate camera name"; > > + return false; > > + } > > Oh the error is here, and is better here than in the function. But > then you have to fail if you don't find one of the files... I think > what you have here is right then... > > > + > > 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, name, streams); > > nits apart > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks. > > Thanks > j > > > registerCamera(std::move(camera), std::move(data)); > > > > /* Enable hot-unplug notifications. */ > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 93e3dc17e3a7105e..72f3f5d9f7c414d0 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> @@ -81,6 +82,9 @@ public: bool match(DeviceEnumerator *enumerator) override; private: + std::string generateName(const MediaDevice *media, + const std::string path); + int processControl(ControlList *controls, unsigned int id, const ControlValue &value); int processControls(UVCCameraData *data, Request *request); @@ -379,6 +383,28 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request) return 0; } +std::string PipelineHandlerUVC::generateName(const MediaDevice *media, + const std::string path) +{ + static const std::vector<std::string> files = { "idVendor", "idProduct", "busnum", "devnum" }; + std::vector<std::string> values; + std::string value; + + for (const std::string &name : files) { + std::ifstream file(path + "/../" + name); + + if (!file.is_open()) + return ""; + + std::getline(file, value); + file.close(); + + values.push_back(value); + } + + return utils::join(values, ":"); +} + bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) { MediaDevice *media; @@ -405,8 +431,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator) return false; /* Create and register the camera. */ + std::string name = generateName(media, data->video_->devicePath()); + if (name.empty()) { + LOG(UVC, Error) << "Failed to generate camera name"; + 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, name, streams); registerCamera(std::move(camera), std::move(data)); /* Enable hot-unplug notifications. */
Generate camera names that are unique and persistent between system resets. The name is constructed from the USB vendor and product information that is stored on USB device itself and the USB bus and device numbers where the hardware is plugged in. Before this change example of camera names: Venus USB2.0 Camera: Venus USB2 Logitech Webcam C930e After this change the same cameras are: 0ac8:3420:3:10 046d:0843:3:4 Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 34 +++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)