| Message ID | 20200718132324.867815-1-niklas.soderlund@ragnatech.se |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > Hello, > minors apart that I will reply to patch-by-patch, I have two questions on the series in general 1) The series introduce and 'id' to be used alongside and alternatively to the camera 'name'. This might just be a matter of terminology, but I find this a bit confusing. Ideally, the 'name' should be the unique part, to which a 'model' (to mimic the API we have for camera sensors) could be added. 2) The API and the cam implementation allow to use 'name' and 'id' interchangibily. Is this a good thing ? Applications should always use 'id' when interfacing to libcamera, and ideally 'name' should be a shortcut for users, to easily select a camera (provided it is unique). If I'm not mistaken it is currently possible to ask libcamera for a 'camera' name, should we drop this and implement that part in the application ? 'cam' and alike can and should use mnemonic names to users, but should libcamera do the same? Do we want an API that allows selecting camera with a name which is not guaranteed to be unique and consistent ? I would say we don't... What do you think ? > This series aims to add a ID to each camera in addition to it's more > user-friendly name. The ID is unique and persistent between reboots of > the same system. The use-case for this is to create a single > machine-friendly ID that can be stored and used to always resolve to the > same camera. > > The idea on how to generate a ID is to take the sysfs path of the sensor > device which is part of each camera pipeline. As the path describes the > location of the sensor hardware it is persistent across reboots and as > the path is read from sysfs it's guaranteed to be unique in the system. > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > video device is used instead. That path resolves to the USB device and > includes the USB bus information so it satisfy the ID requirements. > > While working with this problem it became apparent that two pipelines > diverge from the others on how they name their cameras, raspberrypi and > vimc. This series aligns these two and adds a helper to avoid such > situations in the future. Unfortunately this means the user-friendly > name of the sensor changes but this proves the need for a > machine-friendly ID which luckily this series also adds :-) > > Before this series camera user-friendly names on different systems > looked like this (I do not have access to a simple pipeline device): > > - ipu3 > ov13858 8-0010 > ov5670 10-0036 > - raspberrypi > imx219 > - rkisp1 > ov5695 7-0036 > ov2685 7-003c > - uvcvideo > Logitech Webcam C930e > - vimc > VIMC Sensor B > > With this series applied the user-friendly names machine-friendly ID on > the same systems look like this: > > The format is: > <user-friendly name> (<machine-friendly ID>) > > - ipu3 > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > - raspberrypi > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > - rkisp1 > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > - uvcvideo > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > - vimc > Sensor B (platform/vimc.0) > > Where it previously where possible to select a camera by its > user-friendly name its now possible to also select it using its > machine-friendly one. The following is therefor two equivalent > commands: > > $ cam -c "Logitech Webcam C930e" -C > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > Niklas Söderlund (9): > libcamera: v4l2_device: Add method to lookup device path > libcamera: camera_sensor: Expose a sensor ID > libcamera: camera: Add camera ID > libcamera: camera_manager: Enforce unique camera IDs > libcamera: camera_manager: Try to match camera IDs first > libcamera: pipeline: vimc: Align camera name > libcamera: pipeline: raspberrypi: Align camera name > libcamera: camera: Add create() that operates on CameraSensor > cam: Print camera IDs when listing cameras > > include/libcamera/camera.h | 11 +++- > include/libcamera/internal/camera_sensor.h | 2 + > include/libcamera/internal/v4l2_device.h | 1 + > src/cam/main.cpp | 3 +- > src/libcamera/camera.cpp | 54 +++++++++++++++---- > src/libcamera/camera_manager.cpp | 13 +++++ > src/libcamera/camera_sensor.cpp | 17 ++++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > src/libcamera/pipeline/simple/simple.cpp | 3 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > test/camera/buffer_import.cpp | 2 +- > test/camera/capture.cpp | 2 +- > test/camera/configuration_default.cpp | 2 +- > test/camera/configuration_set.cpp | 2 +- > test/camera/statemachine.cpp | 2 +- > test/controls/control_info_map.cpp | 2 +- > test/controls/control_list.cpp | 2 +- > 21 files changed, 138 insertions(+), 32 deletions(-) > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > Hello, > > This series aims to add a ID to each camera in addition to it's more > user-friendly name. The ID is unique and persistent between reboots of > the same system. The use-case for this is to create a single > machine-friendly ID that can be stored and used to always resolve to the > same camera. > > The idea on how to generate a ID is to take the sysfs path of the sensor > device which is part of each camera pipeline. As the path describes the > location of the sensor hardware it is persistent across reboots and as > the path is read from sysfs it's guaranteed to be unique in the system. > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > video device is used instead. That path resolves to the USB device and > includes the USB bus information so it satisfy the ID requirements. > > While working with this problem it became apparent that two pipelines > diverge from the others on how they name their cameras, raspberrypi and > vimc. This series aligns these two and adds a helper to avoid such > situations in the future. Unfortunately this means the user-friendly > name of the sensor changes but this proves the need for a > machine-friendly ID which luckily this series also adds :-) > > Before this series camera user-friendly names on different systems > looked like this (I do not have access to a simple pipeline device): > > - ipu3 > ov13858 8-0010 > ov5670 10-0036 > - raspberrypi > imx219 > - rkisp1 > ov5695 7-0036 > ov2685 7-003c > - uvcvideo > Logitech Webcam C930e > - vimc > VIMC Sensor B > > With this series applied the user-friendly names machine-friendly ID on > the same systems look like this: > > The format is: > <user-friendly name> (<machine-friendly ID>) > > - ipu3 > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > - raspberrypi > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > - rkisp1 > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > - uvcvideo > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > - vimc > Sensor B (platform/vimc.0) Really an open question, are those name really useful ? I've spent some time going through systemd predictable interfaces names and udevadm test-builtin path_id and indeed they use the same information you have here collected, but they produce a name without / and with a well defined naming scheme (systemd at least) that could be guessed by knowing how the devices are integrated in the system. I don't think we need to go as far as making our camera names predictable, at least I don't see a urgent use case for that, but taking the raw sysfs path seems a bit too simplicistic, especially when you then have to deal with ids like: pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0 which seems a bit long to me. I know this is not expected to be used by users directly, but as per my previous comment I think we should not expose an API in libcamera that works on names which are not guranteed to be unique (althoug application should be able to provide shortcut, maybe using friendly names as reported by pipeline handlers. Trying to list which kind of sensor we should deal with, their number is fairly limited for my understanding: usb and i2c. Wouldn't coupling the bus identifier with the sensor identifier enough to provide a unique and almost-as-friendly-as-a-name id ? Using the use example you have above provided: - ipu3 i2c-8_i2c-OVTID858:00 i2c-10_i2c-INT3479:00 - raspberrypi i2c-10_10-0010 - rkisp1 i2c-7_7-0036 i2c-7_7-003c - uvcvideo usb3_3-2_3-2.4_3-2.4:1.0 - vimc vimc.0 I2c is trivial, use the device identifier and walk the path back until the i2c parent identifier is found, then couple the 2. usb seems a bit more tricky as a full name like usb3_3-2_3-2.4_3-2.4:1.0 could probably be reduced to usb3_3-2.4:1.0 (however, udevadm for my UVC camera gives me: udevadm test-builtin path_id /sys/devices/pci0000:00/0000:00:14.0/usb1/1-8/1-8:1.0 ID_PATH_TAG=pci-0000_00_14_0-usb-0_8_1_0 while I would have expected usb1_1-8:1.0 I know nothing about USB, but it might be worth reading why udev thinks that path tag should in that form https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-path_id.c Vimc is kind of special, you cannot have 2 vimc instances in your system loaded at the same time, so I guess the device name should be enough. What options have you considered in place of the full sysfs path ? Or do you think there's no need to and a string like pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0 could be easily managed by applications ? Thanks j > > Where it previously where possible to select a camera by its > user-friendly name its now possible to also select it using its > machine-friendly one. The following is therefor two equivalent > commands: > > $ cam -c "Logitech Webcam C930e" -C > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > Niklas Söderlund (9): > libcamera: v4l2_device: Add method to lookup device path > libcamera: camera_sensor: Expose a sensor ID > libcamera: camera: Add camera ID > libcamera: camera_manager: Enforce unique camera IDs > libcamera: camera_manager: Try to match camera IDs first > libcamera: pipeline: vimc: Align camera name > libcamera: pipeline: raspberrypi: Align camera name > libcamera: camera: Add create() that operates on CameraSensor > cam: Print camera IDs when listing cameras > > include/libcamera/camera.h | 11 +++- > include/libcamera/internal/camera_sensor.h | 2 + > include/libcamera/internal/v4l2_device.h | 1 + > src/cam/main.cpp | 3 +- > src/libcamera/camera.cpp | 54 +++++++++++++++---- > src/libcamera/camera_manager.cpp | 13 +++++ > src/libcamera/camera_sensor.cpp | 17 ++++++ > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > src/libcamera/pipeline/simple/simple.cpp | 3 +- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > test/camera/buffer_import.cpp | 2 +- > test/camera/capture.cpp | 2 +- > test/camera/configuration_default.cpp | 2 +- > test/camera/configuration_set.cpp | 2 +- > test/camera/statemachine.cpp | 2 +- > test/controls/control_info_map.cpp | 2 +- > test/controls/control_list.cpp | 2 +- > 21 files changed, 138 insertions(+), 32 deletions(-) > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-07-20 15:07:07 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > > Hello, > > > > minors apart that I will reply to patch-by-patch, I have two questions > on the series in general > > 1) The series introduce and 'id' to be used alongside and > alternatively to the camera 'name'. This might just be a matter of > terminology, but I find this a bit confusing. Ideally, the 'name' > should be the unique part, to which a 'model' (to mimic the API we > have for camera sensors) could be added. I'm not against this change. But maybe removing the name() and adding a model() could be done on top as this series main goal is to add a id() field? > > 2) The API and the cam implementation allow to use 'name' and 'id' > interchangibily. Is this a good thing ? Applications should always use > 'id' when interfacing to libcamera, and ideally 'name' should be a > shortcut for users, to easily select a camera (provided it is unique). > If I'm not mistaken it is currently possible to ask libcamera for a > 'camera' name, should we drop this and implement that part in the > application ? 'cam' and alike can and should use mnemonic names to > users, but should libcamera do the same? Do we want an API that allows > selecting camera with a name which is not guaranteed to be unique and > consistent ? I would say we don't... Also I'm not against this change and I think if we all agree this series can be modified to only allow selecting cameras based on id() instead of id() or name() as this version of this series allows. > > What do you think ? > > > This series aims to add a ID to each camera in addition to it's more > > user-friendly name. The ID is unique and persistent between reboots of > > the same system. The use-case for this is to create a single > > machine-friendly ID that can be stored and used to always resolve to the > > same camera. > > > > The idea on how to generate a ID is to take the sysfs path of the sensor > > device which is part of each camera pipeline. As the path describes the > > location of the sensor hardware it is persistent across reboots and as > > the path is read from sysfs it's guaranteed to be unique in the system. > > > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > > video device is used instead. That path resolves to the USB device and > > includes the USB bus information so it satisfy the ID requirements. > > > > While working with this problem it became apparent that two pipelines > > diverge from the others on how they name their cameras, raspberrypi and > > vimc. This series aligns these two and adds a helper to avoid such > > situations in the future. Unfortunately this means the user-friendly > > name of the sensor changes but this proves the need for a > > machine-friendly ID which luckily this series also adds :-) > > > > Before this series camera user-friendly names on different systems > > looked like this (I do not have access to a simple pipeline device): > > > > - ipu3 > > ov13858 8-0010 > > ov5670 10-0036 > > - raspberrypi > > imx219 > > - rkisp1 > > ov5695 7-0036 > > ov2685 7-003c > > - uvcvideo > > Logitech Webcam C930e > > - vimc > > VIMC Sensor B > > > > With this series applied the user-friendly names machine-friendly ID on > > the same systems look like this: > > > > The format is: > > <user-friendly name> (<machine-friendly ID>) > > > > - ipu3 > > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > > - raspberrypi > > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > > - rkisp1 > > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > > - uvcvideo > > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > > - vimc > > Sensor B (platform/vimc.0) > > > > Where it previously where possible to select a camera by its > > user-friendly name its now possible to also select it using its > > machine-friendly one. The following is therefor two equivalent > > commands: > > > > $ cam -c "Logitech Webcam C930e" -C > > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > > > Niklas Söderlund (9): > > libcamera: v4l2_device: Add method to lookup device path > > libcamera: camera_sensor: Expose a sensor ID > > libcamera: camera: Add camera ID > > libcamera: camera_manager: Enforce unique camera IDs > > libcamera: camera_manager: Try to match camera IDs first > > libcamera: pipeline: vimc: Align camera name > > libcamera: pipeline: raspberrypi: Align camera name > > libcamera: camera: Add create() that operates on CameraSensor > > cam: Print camera IDs when listing cameras > > > > include/libcamera/camera.h | 11 +++- > > include/libcamera/internal/camera_sensor.h | 2 + > > include/libcamera/internal/v4l2_device.h | 1 + > > src/cam/main.cpp | 3 +- > > src/libcamera/camera.cpp | 54 +++++++++++++++---- > > src/libcamera/camera_manager.cpp | 13 +++++ > > src/libcamera/camera_sensor.cpp | 17 ++++++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > src/libcamera/pipeline/simple/simple.cpp | 3 +- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > > test/camera/buffer_import.cpp | 2 +- > > test/camera/capture.cpp | 2 +- > > test/camera/configuration_default.cpp | 2 +- > > test/camera/configuration_set.cpp | 2 +- > > test/camera/statemachine.cpp | 2 +- > > test/controls/control_info_map.cpp | 2 +- > > test/controls/control_list.cpp | 2 +- > > 21 files changed, 138 insertions(+), 32 deletions(-) > > > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2020-07-20 16:48:26 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > > Hello, > > > > This series aims to add a ID to each camera in addition to it's more > > user-friendly name. The ID is unique and persistent between reboots of > > the same system. The use-case for this is to create a single > > machine-friendly ID that can be stored and used to always resolve to the > > same camera. > > > > The idea on how to generate a ID is to take the sysfs path of the sensor > > device which is part of each camera pipeline. As the path describes the > > location of the sensor hardware it is persistent across reboots and as > > the path is read from sysfs it's guaranteed to be unique in the system. > > > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > > video device is used instead. That path resolves to the USB device and > > includes the USB bus information so it satisfy the ID requirements. > > > > While working with this problem it became apparent that two pipelines > > diverge from the others on how they name their cameras, raspberrypi and > > vimc. This series aligns these two and adds a helper to avoid such > > situations in the future. Unfortunately this means the user-friendly > > name of the sensor changes but this proves the need for a > > machine-friendly ID which luckily this series also adds :-) > > > > Before this series camera user-friendly names on different systems > > looked like this (I do not have access to a simple pipeline device): > > > > - ipu3 > > ov13858 8-0010 > > ov5670 10-0036 > > - raspberrypi > > imx219 > > - rkisp1 > > ov5695 7-0036 > > ov2685 7-003c > > - uvcvideo > > Logitech Webcam C930e > > - vimc > > VIMC Sensor B > > > > With this series applied the user-friendly names machine-friendly ID on > > the same systems look like this: > > > > The format is: > > <user-friendly name> (<machine-friendly ID>) > > > > - ipu3 > > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > > - raspberrypi > > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > > - rkisp1 > > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > > - uvcvideo > > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > > - vimc > > Sensor B (platform/vimc.0) > > Really an open question, are those name really useful ? > > I've spent some time going through systemd predictable interfaces > names and udevadm test-builtin path_id and indeed they use the same > information you have here collected, but they produce a name without / > and with a well defined naming scheme (systemd at least) that could be > guessed by knowing how the devices are integrated in the system. > > I don't think we need to go as far as making our camera names > predictable, at least I don't see a urgent use case for that, but > taking the raw sysfs path seems a bit too simplicistic, especially > when you then have to deal with ids like: > pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0 > > which seems a bit long to me. > > I know this is not expected to be used by users directly, but as per > my previous comment I think we should not expose an API in libcamera > that works on names which are not guranteed to be unique (althoug > application should be able to provide shortcut, maybe using friendly > names as reported by pipeline handlers. > > Trying to list which kind of sensor we should deal with, their number > is fairly limited for my understanding: usb and i2c. Wouldn't coupling > the bus identifier with the sensor identifier enough to provide a > unique and almost-as-friendly-as-a-name id ? > > Using the use example you have above provided: > - ipu3 > i2c-8_i2c-OVTID858:00 > i2c-10_i2c-INT3479:00 > - raspberrypi > i2c-10_10-0010 > - rkisp1 > i2c-7_7-0036 > i2c-7_7-003c > - uvcvideo > usb3_3-2_3-2.4_3-2.4:1.0 > - vimc > vimc.0 > > I2c is trivial, use the device identifier and walk the path back until > the i2c parent identifier is found, then couple the 2. > > usb seems a bit more tricky as a full name like > usb3_3-2_3-2.4_3-2.4:1.0 > could probably be reduced to > usb3_3-2.4:1.0 > > (however, udevadm for my UVC camera gives me: > udevadm test-builtin path_id /sys/devices/pci0000:00/0000:00:14.0/usb1/1-8/1-8:1.0 > ID_PATH_TAG=pci-0000_00_14_0-usb-0_8_1_0 > while I would have expected usb1_1-8:1.0 > > I know nothing about USB, but it might be worth reading why udev > thinks that path tag should in that form > https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-path_id.c > > Vimc is kind of special, you cannot have 2 vimc instances in your > system loaded at the same time, so I guess the device name should be > enough. > > What options have you considered in place of the full sysfs path ? Or > do you think there's no need to and a string like > pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0 > could be easily managed by applications ? Thanks for pointing out the udev possibility to this problem, I was not aware of it. I kind of like it and would like to use it instead of using the sysfs device path as this series does. The only problem I see is that it would make udev a required dependency. In the past we have gone to great length of keeping udev optional, we even implemented a sysfs based device enumerator to allow for this. If we with what we know now are willing to reevaluate this design idea I would be all for using udev as you describe above. So before I set out to try this out how do we all fell about making udev mandatory? > > Thanks > j > > > > > Where it previously where possible to select a camera by its > > user-friendly name its now possible to also select it using its > > machine-friendly one. The following is therefor two equivalent > > commands: > > > > $ cam -c "Logitech Webcam C930e" -C > > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > > > Niklas Söderlund (9): > > libcamera: v4l2_device: Add method to lookup device path > > libcamera: camera_sensor: Expose a sensor ID > > libcamera: camera: Add camera ID > > libcamera: camera_manager: Enforce unique camera IDs > > libcamera: camera_manager: Try to match camera IDs first > > libcamera: pipeline: vimc: Align camera name > > libcamera: pipeline: raspberrypi: Align camera name > > libcamera: camera: Add create() that operates on CameraSensor > > cam: Print camera IDs when listing cameras > > > > include/libcamera/camera.h | 11 +++- > > include/libcamera/internal/camera_sensor.h | 2 + > > include/libcamera/internal/v4l2_device.h | 1 + > > src/cam/main.cpp | 3 +- > > src/libcamera/camera.cpp | 54 +++++++++++++++---- > > src/libcamera/camera_manager.cpp | 13 +++++ > > src/libcamera/camera_sensor.cpp | 17 ++++++ > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > src/libcamera/pipeline/simple/simple.cpp | 3 +- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > > test/camera/buffer_import.cpp | 2 +- > > test/camera/capture.cpp | 2 +- > > test/camera/configuration_default.cpp | 2 +- > > test/camera/configuration_set.cpp | 2 +- > > test/camera/statemachine.cpp | 2 +- > > test/controls/control_info_map.cpp | 2 +- > > test/controls/control_list.cpp | 2 +- > > 21 files changed, 138 insertions(+), 32 deletions(-) > > > > -- > > 2.27.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Fri, Jul 24, 2020 at 12:08:35PM +0200, Niklas Söderlund wrote: > On 2020-07-20 15:07:07 +0200, Jacopo Mondi wrote: > > On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > > > Hello, > > > > minors apart that I will reply to patch-by-patch, I have two questions > > on the series in general > > > > 1) The series introduce and 'id' to be used alongside and > > alternatively to the camera 'name'. This might just be a matter of > > terminology, but I find this a bit confusing. Ideally, the 'name' > > should be the unique part, to which a 'model' (to mimic the API we > > have for camera sensors) could be added. > > I'm not against this change. But maybe removing the name() and adding a > model() could be done on top as this series main goal is to add a id() > field? We're entering the bikeshedding territory a little bit, but I think it's an important question. We may want to drop "name" altogether, as it's a confusing name (no pun intended). Using friendlyName() and uniqueName(), for instance, would make the API clearer. We can continue bikeshedding on the name, but I think whatever function names we pick, they should be fairly explicit. > > 2) The API and the cam implementation allow to use 'name' and 'id' > > interchangibily. Is this a good thing ? Applications should always use > > 'id' when interfacing to libcamera, and ideally 'name' should be a > > shortcut for users, to easily select a camera (provided it is unique). > > If I'm not mistaken it is currently possible to ask libcamera for a > > 'camera' name, should we drop this and implement that part in the > > application ? 'cam' and alike can and should use mnemonic names to > > users, but should libcamera do the same? Do we want an API that allows > > selecting camera with a name which is not guaranteed to be unique and > > consistent ? I would say we don't... > > Also I'm not against this change and I think if we all agree this series > can be modified to only allow selecting cameras based on id() instead of > id() or name() as this version of this series allows. I'm with Jacopo on this, I'd prefer if libcamera pushed applications to use a safer API, while still offering the option of an application-side manual implementation based on the friendly name. > > What do you think ? > > > > > This series aims to add a ID to each camera in addition to it's more > > > user-friendly name. The ID is unique and persistent between reboots of > > > the same system. The use-case for this is to create a single > > > machine-friendly ID that can be stored and used to always resolve to the > > > same camera. > > > > > > The idea on how to generate a ID is to take the sysfs path of the sensor > > > device which is part of each camera pipeline. As the path describes the > > > location of the sensor hardware it is persistent across reboots and as > > > the path is read from sysfs it's guaranteed to be unique in the system. > > > > > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > > > video device is used instead. That path resolves to the USB device and > > > includes the USB bus information so it satisfy the ID requirements. > > > > > > While working with this problem it became apparent that two pipelines > > > diverge from the others on how they name their cameras, raspberrypi and > > > vimc. This series aligns these two and adds a helper to avoid such > > > situations in the future. Unfortunately this means the user-friendly > > > name of the sensor changes but this proves the need for a > > > machine-friendly ID which luckily this series also adds :-) > > > > > > Before this series camera user-friendly names on different systems > > > looked like this (I do not have access to a simple pipeline device): > > > > > > - ipu3 > > > ov13858 8-0010 > > > ov5670 10-0036 > > > - raspberrypi > > > imx219 > > > - rkisp1 > > > ov5695 7-0036 > > > ov2685 7-003c > > > - uvcvideo > > > Logitech Webcam C930e > > > - vimc > > > VIMC Sensor B > > > > > > With this series applied the user-friendly names machine-friendly ID on > > > the same systems look like this: > > > > > > The format is: > > > <user-friendly name> (<machine-friendly ID>) > > > > > > - ipu3 > > > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > > > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > > > - raspberrypi > > > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > > > - rkisp1 > > > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > > > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > > > - uvcvideo > > > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > > > - vimc > > > Sensor B (platform/vimc.0) > > > > > > Where it previously where possible to select a camera by its > > > user-friendly name its now possible to also select it using its > > > machine-friendly one. The following is therefor two equivalent > > > commands: > > > > > > $ cam -c "Logitech Webcam C930e" -C > > > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > > > > > Niklas Söderlund (9): > > > libcamera: v4l2_device: Add method to lookup device path > > > libcamera: camera_sensor: Expose a sensor ID > > > libcamera: camera: Add camera ID > > > libcamera: camera_manager: Enforce unique camera IDs > > > libcamera: camera_manager: Try to match camera IDs first > > > libcamera: pipeline: vimc: Align camera name > > > libcamera: pipeline: raspberrypi: Align camera name > > > libcamera: camera: Add create() that operates on CameraSensor > > > cam: Print camera IDs when listing cameras > > > > > > include/libcamera/camera.h | 11 +++- > > > include/libcamera/internal/camera_sensor.h | 2 + > > > include/libcamera/internal/v4l2_device.h | 1 + > > > src/cam/main.cpp | 3 +- > > > src/libcamera/camera.cpp | 54 +++++++++++++++---- > > > src/libcamera/camera_manager.cpp | 13 +++++ > > > src/libcamera/camera_sensor.cpp | 17 ++++++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > src/libcamera/pipeline/simple/simple.cpp | 3 +- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > > > test/camera/buffer_import.cpp | 2 +- > > > test/camera/capture.cpp | 2 +- > > > test/camera/configuration_default.cpp | 2 +- > > > test/camera/configuration_set.cpp | 2 +- > > > test/camera/statemachine.cpp | 2 +- > > > test/controls/control_info_map.cpp | 2 +- > > > test/controls/control_list.cpp | 2 +- > > > 21 files changed, 138 insertions(+), 32 deletions(-)
Hello, On Fri, Jul 24, 2020 at 12:12:45PM +0200, Niklas Söderlund wrote: > On 2020-07-20 16:48:26 +0200, Jacopo Mondi wrote: > > On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > > > Hello, > > > > > > This series aims to add a ID to each camera in addition to it's more > > > user-friendly name. The ID is unique and persistent between reboots of > > > the same system. The use-case for this is to create a single > > > machine-friendly ID that can be stored and used to always resolve to the > > > same camera. > > > > > > The idea on how to generate a ID is to take the sysfs path of the sensor > > > device which is part of each camera pipeline. As the path describes the > > > location of the sensor hardware it is persistent across reboots and as > > > the path is read from sysfs it's guaranteed to be unique in the system. > > > > > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > > > video device is used instead. That path resolves to the USB device and > > > includes the USB bus information so it satisfy the ID requirements. > > > > > > While working with this problem it became apparent that two pipelines > > > diverge from the others on how they name their cameras, raspberrypi and > > > vimc. This series aligns these two and adds a helper to avoid such > > > situations in the future. Unfortunately this means the user-friendly > > > name of the sensor changes but this proves the need for a > > > machine-friendly ID which luckily this series also adds :-) > > > > > > Before this series camera user-friendly names on different systems > > > looked like this (I do not have access to a simple pipeline device): > > > > > > - ipu3 > > > ov13858 8-0010 > > > ov5670 10-0036 > > > - raspberrypi > > > imx219 > > > - rkisp1 > > > ov5695 7-0036 > > > ov2685 7-003c > > > - uvcvideo > > > Logitech Webcam C930e > > > - vimc > > > VIMC Sensor B > > > > > > With this series applied the user-friendly names machine-friendly ID on > > > the same systems look like this: > > > > > > The format is: > > > <user-friendly name> (<machine-friendly ID>) > > > > > > - ipu3 > > > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > > > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) The friendly name should really be "Front camera" and "Back camera" on a Soraka device. Same on a Scarlet device, but on a different rkisp1-based system, there may not be a front and a back camera. Imagine a digital microscope, it would be nice to have a friendly name along the lines of "Microscope camera". How to make this happen will likely be challenging, and I'm fine solving the friendly name issue separately from the unique name issue (the former being more complex than the latter), but I'd like to start thinking about it. > > > - raspberrypi > > > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > > > - rkisp1 > > > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > > > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > > > - uvcvideo > > > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) And if we have two identical USB webcams, it would be nice to name them "Logitech Webcam C930e (on port 1)" and "Logitech Webcam C930e (on port 3)" for example. > > > - vimc > > > Sensor B (platform/vimc.0) This one is easy, it's for testing purpose, so we can hardcode any name we find friendly. > > > > Really an open question, are those name really useful ? > > > > I've spent some time going through systemd predictable interfaces > > names and udevadm test-builtin path_id and indeed they use the same > > information you have here collected, but they produce a name without / > > and with a well defined naming scheme (systemd at least) that could be > > guessed by knowing how the devices are integrated in the system. > > > > I don't think we need to go as far as making our camera names > > predictable, at least I don't see a urgent use case for that, but > > taking the raw sysfs path seems a bit too simplicistic, especially > > when you then have to deal with ids like: > > pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0 > > > > which seems a bit long to me. How can is be too long and too simplistic at the same time ? :-) This name should never be presented to the end-user (except by test applications, when the end-user is a developer), so I don't mind much if it's long. That being said, if there's a way to shorten it while still keeping the name stable, I think that's better. > > I know this is not expected to be used by users directly, but as per > > my previous comment I think we should not expose an API in libcamera > > that works on names which are not guranteed to be unique (althoug > > application should be able to provide shortcut, maybe using friendly > > names as reported by pipeline handlers. > > > > Trying to list which kind of sensor we should deal with, their number > > is fairly limited for my understanding: usb and i2c. Wouldn't coupling > > the bus identifier with the sensor identifier enough to provide a > > unique and almost-as-friendly-as-a-name id ? > > > > Using the use example you have above provided: > > - ipu3 > > i2c-8_i2c-OVTID858:00 > > i2c-10_i2c-INT3479:00 > > - raspberrypi > > i2c-10_10-0010 > > - rkisp1 > > i2c-7_7-0036 > > i2c-7_7-003c > > - uvcvideo > > usb3_3-2_3-2.4_3-2.4:1.0 > > - vimc > > vimc.0 Should we take into account the possibility of later implementing support for sensor A, we will have two cameras for the same vimc device. I think this should already be taken into account. Let's try to also consider other cases, even if we don't implement support for them now. One use case I can think of is cameras made of multiple sensors (for instance a sensor with a wide angle lens and a sensor with a tele lens, with seamless switching between the two when zooming). This should however not be too complicated, we can probably just pick one of the two sensors to construct the camera name. Brainstorming mode turned on, what other use cases can we have ? > > I2c is trivial, use the device identifier and walk the path back until > > the i2c parent identifier is found, then couple the 2. Is the I2C bus number stable ? On DT system, for SoC I2C controllers, it should be *if* you have I2C bus aliases in your device tree, but for anything behind a PCI device for instance, I don't think we have any such guarantee. > > usb seems a bit more tricky as a full name like > > usb3_3-2_3-2.4_3-2.4:1.0 > > could probably be reduced to > > usb3_3-2.4:1.0 > > > > (however, udevadm for my UVC camera gives me: > > udevadm test-builtin path_id /sys/devices/pci0000:00/0000:00:14.0/usb1/1-8/1-8:1.0 > > ID_PATH_TAG=pci-0000_00_14_0-usb-0_8_1_0 > > while I would have expected usb1_1-8:1.0 > > > > I know nothing about USB, but it might be worth reading why udev > > thinks that path tag should in that form > > https://github.com/systemd/systemd/blob/master/src/udev/udev-builtin-path_id.c It may also be worth looking at how device names are constructed in the kernel (usb_make_path() for instance). The USB devices are usually identified by walking the USB tree from the root hub, keeping in mind there can be multiple root hubs, and using port numbers along the way as those are stable. The reason why the PCI device is included may be because USB bus numbers are assigned dynamically, and thus may not be stable across reboots. > > Vimc is kind of special, you cannot have 2 vimc instances in your > > system loaded at the same time, so I guess the device name should be > > enough. > > > > What options have you considered in place of the full sysfs path ? Or > > do you think there's no need to and a string like > > pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0 > > could be easily managed by applications ? > > Thanks for pointing out the udev possibility to this problem, I was not > aware of it. I kind of like it and would like to use it instead of using > the sysfs device path as this series does. > > The only problem I see is that it would make udev a required dependency. > In the past we have gone to great length of keeping udev optional, we > even implemented a sysfs based device enumerator to allow for this. If > we with what we know now are willing to reevaluate this design idea I > would be all for using udev as you describe above. So before I set out > to try this out how do we all fell about making udev mandatory? I'd rather keep it optional if possible. As stated in a separate reply, if we make the device enumerator responsible for providing the name, we can have different implementations for udev and udev-less systems, so I don't see any issue in relying on udev when available. > > > Where it previously where possible to select a camera by its > > > user-friendly name its now possible to also select it using its > > > machine-friendly one. The following is therefor two equivalent > > > commands: > > > > > > $ cam -c "Logitech Webcam C930e" -C > > > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > > > > > Niklas Söderlund (9): > > > libcamera: v4l2_device: Add method to lookup device path > > > libcamera: camera_sensor: Expose a sensor ID > > > libcamera: camera: Add camera ID > > > libcamera: camera_manager: Enforce unique camera IDs > > > libcamera: camera_manager: Try to match camera IDs first > > > libcamera: pipeline: vimc: Align camera name > > > libcamera: pipeline: raspberrypi: Align camera name > > > libcamera: camera: Add create() that operates on CameraSensor > > > cam: Print camera IDs when listing cameras > > > > > > include/libcamera/camera.h | 11 +++- > > > include/libcamera/internal/camera_sensor.h | 2 + > > > include/libcamera/internal/v4l2_device.h | 1 + > > > src/cam/main.cpp | 3 +- > > > src/libcamera/camera.cpp | 54 +++++++++++++++---- > > > src/libcamera/camera_manager.cpp | 13 +++++ > > > src/libcamera/camera_sensor.cpp | 17 ++++++ > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > src/libcamera/pipeline/simple/simple.cpp | 3 +- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > > > test/camera/buffer_import.cpp | 2 +- > > > test/camera/capture.cpp | 2 +- > > > test/camera/configuration_default.cpp | 2 +- > > > test/camera/configuration_set.cpp | 2 +- > > > test/camera/statemachine.cpp | 2 +- > > > test/controls/control_info_map.cpp | 2 +- > > > test/controls/control_list.cpp | 2 +- > > > 21 files changed, 138 insertions(+), 32 deletions(-)
Hello, On 2020-07-25 02:44:07 +0300, Laurent Pinchart wrote: > Hello, > > On Fri, Jul 24, 2020 at 12:08:35PM +0200, Niklas Söderlund wrote: > > On 2020-07-20 15:07:07 +0200, Jacopo Mondi wrote: > > > On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > > > > Hello, > > > > > > minors apart that I will reply to patch-by-patch, I have two questions > > > on the series in general > > > > > > 1) The series introduce and 'id' to be used alongside and > > > alternatively to the camera 'name'. This might just be a matter of > > > terminology, but I find this a bit confusing. Ideally, the 'name' > > > should be the unique part, to which a 'model' (to mimic the API we > > > have for camera sensors) could be added. > > > > I'm not against this change. But maybe removing the name() and adding a > > model() could be done on top as this series main goal is to add a id() > > field? > > We're entering the bikeshedding territory a little bit, but I think it's > an important question. We may want to drop "name" altogether, as it's a > confusing name (no pun intended). Using friendlyName() and uniqueName(), > for instance, would make the API clearer. We can continue bikeshedding > on the name, but I think whatever function names we pick, they should be > fairly explicit. I want to avoid bikeshedding so I will only focus of the addition of the new id()/uniequeName() in this series. Then once that is in we can think about how we wish to evolve or remove name(). Some bikeshedding is of course needed, in my first private incarnation of this series I had named the new property uniqueName(). It became very cumbersome in the documentation and felt none obvious, switching to id() felt like the right thing. So while we hash this out I will keep id() as a working name and then I will switch it to whatever the majority likes. > > > > 2) The API and the cam implementation allow to use 'name' and 'id' > > > interchangibily. Is this a good thing ? Applications should always use > > > 'id' when interfacing to libcamera, and ideally 'name' should be a > > > shortcut for users, to easily select a camera (provided it is unique). > > > If I'm not mistaken it is currently possible to ask libcamera for a > > > 'camera' name, should we drop this and implement that part in the > > > application ? 'cam' and alike can and should use mnemonic names to > > > users, but should libcamera do the same? Do we want an API that allows > > > selecting camera with a name which is not guaranteed to be unique and > > > consistent ? I would say we don't... > > > > Also I'm not against this change and I think if we all agree this series > > can be modified to only allow selecting cameras based on id() instead of > > id() or name() as this version of this series allows. > > I'm with Jacopo on this, I'd prefer if libcamera pushed applications to > use a safer API, while still offering the option of an application-side > manual implementation based on the friendly name. Then we all agree on this point and this series will modify the get() function to only operate on the new id() property. > > > > What do you think ? > > > > > > > This series aims to add a ID to each camera in addition to it's more > > > > user-friendly name. The ID is unique and persistent between reboots of > > > > the same system. The use-case for this is to create a single > > > > machine-friendly ID that can be stored and used to always resolve to the > > > > same camera. > > > > > > > > The idea on how to generate a ID is to take the sysfs path of the sensor > > > > device which is part of each camera pipeline. As the path describes the > > > > location of the sensor hardware it is persistent across reboots and as > > > > the path is read from sysfs it's guaranteed to be unique in the system. > > > > > > > > For pipelines that do not have a sensor (UVC) the sysfs path of the main > > > > video device is used instead. That path resolves to the USB device and > > > > includes the USB bus information so it satisfy the ID requirements. > > > > > > > > While working with this problem it became apparent that two pipelines > > > > diverge from the others on how they name their cameras, raspberrypi and > > > > vimc. This series aligns these two and adds a helper to avoid such > > > > situations in the future. Unfortunately this means the user-friendly > > > > name of the sensor changes but this proves the need for a > > > > machine-friendly ID which luckily this series also adds :-) > > > > > > > > Before this series camera user-friendly names on different systems > > > > looked like this (I do not have access to a simple pipeline device): > > > > > > > > - ipu3 > > > > ov13858 8-0010 > > > > ov5670 10-0036 > > > > - raspberrypi > > > > imx219 > > > > - rkisp1 > > > > ov5695 7-0036 > > > > ov2685 7-003c > > > > - uvcvideo > > > > Logitech Webcam C930e > > > > - vimc > > > > VIMC Sensor B > > > > > > > > With this series applied the user-friendly names machine-friendly ID on > > > > the same systems look like this: > > > > > > > > The format is: > > > > <user-friendly name> (<machine-friendly ID>) > > > > > > > > - ipu3 > > > > ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > > > > ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > > > > - raspberrypi > > > > imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > > > > - rkisp1 > > > > ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > > > > ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > > > > - uvcvideo > > > > Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > > > > - vimc > > > > Sensor B (platform/vimc.0) > > > > > > > > Where it previously where possible to select a camera by its > > > > user-friendly name its now possible to also select it using its > > > > machine-friendly one. The following is therefor two equivalent > > > > commands: > > > > > > > > $ cam -c "Logitech Webcam C930e" -C > > > > $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > > > > > > > > Niklas Söderlund (9): > > > > libcamera: v4l2_device: Add method to lookup device path > > > > libcamera: camera_sensor: Expose a sensor ID > > > > libcamera: camera: Add camera ID > > > > libcamera: camera_manager: Enforce unique camera IDs > > > > libcamera: camera_manager: Try to match camera IDs first > > > > libcamera: pipeline: vimc: Align camera name > > > > libcamera: pipeline: raspberrypi: Align camera name > > > > libcamera: camera: Add create() that operates on CameraSensor > > > > cam: Print camera IDs when listing cameras > > > > > > > > include/libcamera/camera.h | 11 +++- > > > > include/libcamera/internal/camera_sensor.h | 2 + > > > > include/libcamera/internal/v4l2_device.h | 1 + > > > > src/cam/main.cpp | 3 +- > > > > src/libcamera/camera.cpp | 54 +++++++++++++++---- > > > > src/libcamera/camera_manager.cpp | 13 +++++ > > > > src/libcamera/camera_sensor.cpp | 17 ++++++ > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > > > > src/libcamera/pipeline/simple/simple.cpp | 3 +- > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > > > > src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > > > > src/libcamera/v4l2_device.cpp | 27 ++++++++++ > > > > test/camera/buffer_import.cpp | 2 +- > > > > test/camera/capture.cpp | 2 +- > > > > test/camera/configuration_default.cpp | 2 +- > > > > test/camera/configuration_set.cpp | 2 +- > > > > test/camera/statemachine.cpp | 2 +- > > > > test/controls/control_info_map.cpp | 2 +- > > > > test/controls/control_list.cpp | 2 +- > > > > 21 files changed, 138 insertions(+), 32 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Sat, Jul 25, 2020 at 10:35:26AM +0200, Niklas Söderlund wrote: > On 2020-07-25 02:44:07 +0300, Laurent Pinchart wrote: > > On Fri, Jul 24, 2020 at 12:08:35PM +0200, Niklas Söderlund wrote: > >> On 2020-07-20 15:07:07 +0200, Jacopo Mondi wrote: > >>> On Sat, Jul 18, 2020 at 03:23:15PM +0200, Niklas Söderlund wrote: > >>>> Hello, > >>> > >>> minors apart that I will reply to patch-by-patch, I have two questions > >>> on the series in general > >>> > >>> 1) The series introduce and 'id' to be used alongside and > >>> alternatively to the camera 'name'. This might just be a matter of > >>> terminology, but I find this a bit confusing. Ideally, the 'name' > >>> should be the unique part, to which a 'model' (to mimic the API we > >>> have for camera sensors) could be added. > >> > >> I'm not against this change. But maybe removing the name() and adding a > >> model() could be done on top as this series main goal is to add a id() > >> field? > > > > We're entering the bikeshedding territory a little bit, but I think it's > > an important question. We may want to drop "name" altogether, as it's a > > confusing name (no pun intended). Using friendlyName() and uniqueName(), > > for instance, would make the API clearer. We can continue bikeshedding > > on the name, but I think whatever function names we pick, they should be > > fairly explicit. > > I want to avoid bikeshedding so I will only focus of the addition of the > new id()/uniequeName() in this series. Then once that is in we can think > about how we wish to evolve or remove name(). > > Some bikeshedding is of course needed, in my first private incarnation > of this series I had named the new property uniqueName(). It became very > cumbersome in the documentation and felt none obvious, switching to id() > felt like the right thing. So while we hash this out I will keep id() as > a working name and then I will switch it to whatever the majority likes. I think this is important bikeshedding :-) I'm fine continuing to develop the patch series without a rename until we reach a consensus, but I think we need to sort out the naming before the final version. This is also not just about the function names, but also about making sure we have a solid scheme for camera identification. Not everything needs to be implemented as part of this patch series, but the design needs to cover the whole problem space. > >>> 2) The API and the cam implementation allow to use 'name' and 'id' > >>> interchangibily. Is this a good thing ? Applications should always use > >>> 'id' when interfacing to libcamera, and ideally 'name' should be a > >>> shortcut for users, to easily select a camera (provided it is unique). > >>> If I'm not mistaken it is currently possible to ask libcamera for a > >>> 'camera' name, should we drop this and implement that part in the > >>> application ? 'cam' and alike can and should use mnemonic names to > >>> users, but should libcamera do the same? Do we want an API that allows > >>> selecting camera with a name which is not guaranteed to be unique and > >>> consistent ? I would say we don't... > >> > >> Also I'm not against this change and I think if we all agree this series > >> can be modified to only allow selecting cameras based on id() instead of > >> id() or name() as this version of this series allows. > > > > I'm with Jacopo on this, I'd prefer if libcamera pushed applications to > > use a safer API, while still offering the option of an application-side > > manual implementation based on the friendly name. > > Then we all agree on this point and this series will modify the get() > function to only operate on the new id() property. > > >>> What do you think ? > >>> > >>>> This series aims to add a ID to each camera in addition to it's more > >>>> user-friendly name. The ID is unique and persistent between reboots of > >>>> the same system. The use-case for this is to create a single > >>>> machine-friendly ID that can be stored and used to always resolve to the > >>>> same camera. > >>>> > >>>> The idea on how to generate a ID is to take the sysfs path of the sensor > >>>> device which is part of each camera pipeline. As the path describes the > >>>> location of the sensor hardware it is persistent across reboots and as > >>>> the path is read from sysfs it's guaranteed to be unique in the system. > >>>> > >>>> For pipelines that do not have a sensor (UVC) the sysfs path of the main > >>>> video device is used instead. That path resolves to the USB device and > >>>> includes the USB bus information so it satisfy the ID requirements. > >>>> > >>>> While working with this problem it became apparent that two pipelines > >>>> diverge from the others on how they name their cameras, raspberrypi and > >>>> vimc. This series aligns these two and adds a helper to avoid such > >>>> situations in the future. Unfortunately this means the user-friendly > >>>> name of the sensor changes but this proves the need for a > >>>> machine-friendly ID which luckily this series also adds :-) > >>>> > >>>> Before this series camera user-friendly names on different systems > >>>> looked like this (I do not have access to a simple pipeline device): > >>>> > >>>> - ipu3 > >>>> ov13858 8-0010 > >>>> ov5670 10-0036 > >>>> - raspberrypi > >>>> imx219 > >>>> - rkisp1 > >>>> ov5695 7-0036 > >>>> ov2685 7-003c > >>>> - uvcvideo > >>>> Logitech Webcam C930e > >>>> - vimc > >>>> VIMC Sensor B > >>>> > >>>> With this series applied the user-friendly names machine-friendly ID on > >>>> the same systems look like this: > >>>> > >>>> The format is: > >>>> <user-friendly name> (<machine-friendly ID>) > >>>> > >>>> - ipu3 > >>>> ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) > >>>> ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) > >>>> - raspberrypi > >>>> imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) > >>>> - rkisp1 > >>>> ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) > >>>> ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) > >>>> - uvcvideo > >>>> Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) > >>>> - vimc > >>>> Sensor B (platform/vimc.0) > >>>> > >>>> Where it previously where possible to select a camera by its > >>>> user-friendly name its now possible to also select it using its > >>>> machine-friendly one. The following is therefor two equivalent > >>>> commands: > >>>> > >>>> $ cam -c "Logitech Webcam C930e" -C > >>>> $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C > >>>> > >>>> Niklas Söderlund (9): > >>>> libcamera: v4l2_device: Add method to lookup device path > >>>> libcamera: camera_sensor: Expose a sensor ID > >>>> libcamera: camera: Add camera ID > >>>> libcamera: camera_manager: Enforce unique camera IDs > >>>> libcamera: camera_manager: Try to match camera IDs first > >>>> libcamera: pipeline: vimc: Align camera name > >>>> libcamera: pipeline: raspberrypi: Align camera name > >>>> libcamera: camera: Add create() that operates on CameraSensor > >>>> cam: Print camera IDs when listing cameras > >>>> > >>>> include/libcamera/camera.h | 11 +++- > >>>> include/libcamera/internal/camera_sensor.h | 2 + > >>>> include/libcamera/internal/v4l2_device.h | 1 + > >>>> src/cam/main.cpp | 3 +- > >>>> src/libcamera/camera.cpp | 54 +++++++++++++++---- > >>>> src/libcamera/camera_manager.cpp | 13 +++++ > >>>> src/libcamera/camera_sensor.cpp | 17 ++++++ > >>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- > >>>> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- > >>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- > >>>> src/libcamera/pipeline/simple/simple.cpp | 3 +- > >>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- > >>>> src/libcamera/pipeline/vimc/vimc.cpp | 4 +- > >>>> src/libcamera/v4l2_device.cpp | 27 ++++++++++ > >>>> test/camera/buffer_import.cpp | 2 +- > >>>> test/camera/capture.cpp | 2 +- > >>>> test/camera/configuration_default.cpp | 2 +- > >>>> test/camera/configuration_set.cpp | 2 +- > >>>> test/camera/statemachine.cpp | 2 +- > >>>> test/controls/control_info_map.cpp | 2 +- > >>>> test/controls/control_list.cpp | 2 +- > >>>> 21 files changed, 138 insertions(+), 32 deletions(-)
Hello, This series aims to add a ID to each camera in addition to it's more user-friendly name. The ID is unique and persistent between reboots of the same system. The use-case for this is to create a single machine-friendly ID that can be stored and used to always resolve to the same camera. The idea on how to generate a ID is to take the sysfs path of the sensor device which is part of each camera pipeline. As the path describes the location of the sensor hardware it is persistent across reboots and as the path is read from sysfs it's guaranteed to be unique in the system. For pipelines that do not have a sensor (UVC) the sysfs path of the main video device is used instead. That path resolves to the USB device and includes the USB bus information so it satisfy the ID requirements. While working with this problem it became apparent that two pipelines diverge from the others on how they name their cameras, raspberrypi and vimc. This series aligns these two and adds a helper to avoid such situations in the future. Unfortunately this means the user-friendly name of the sensor changes but this proves the need for a machine-friendly ID which luckily this series also adds :-) Before this series camera user-friendly names on different systems looked like this (I do not have access to a simple pipeline device): - ipu3 ov13858 8-0010 ov5670 10-0036 - raspberrypi imx219 - rkisp1 ov5695 7-0036 ov2685 7-003c - uvcvideo Logitech Webcam C930e - vimc VIMC Sensor B With this series applied the user-friendly names machine-friendly ID on the same systems look like this: The format is: <user-friendly name> (<machine-friendly ID>) - ipu3 ov13858 8-0010 (pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-OVTID858:00) ov5670 10-0036 (pci0000:00/0000:00:19.2/i2c_designware.5/i2c-10/i2c-INT3479:00) - raspberrypi imx219 10-0010 (platform/soc/3f205000.i2c/i2c-11/i2c-10/10-0010) - rkisp1 ov5695 7-0036 (platform/ff160000.i2c/i2c-7/7-0036) ov2685 7-003c (platform/ff160000.i2c/i2c-7/7-003c) - uvcvideo Logitech Webcam C930e (pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0) - vimc Sensor B (platform/vimc.0) Where it previously where possible to select a camera by its user-friendly name its now possible to also select it using its machine-friendly one. The following is therefor two equivalent commands: $ cam -c "Logitech Webcam C930e" -C $ cam -c "pci0000:00/0000:00:1c.4/0000:08:00.0/0000:09:02.0/0000:25:00.0/usb3/3-2/3-2.4/3-2.4:1.0" -C Niklas Söderlund (9): libcamera: v4l2_device: Add method to lookup device path libcamera: camera_sensor: Expose a sensor ID libcamera: camera: Add camera ID libcamera: camera_manager: Enforce unique camera IDs libcamera: camera_manager: Try to match camera IDs first libcamera: pipeline: vimc: Align camera name libcamera: pipeline: raspberrypi: Align camera name libcamera: camera: Add create() that operates on CameraSensor cam: Print camera IDs when listing cameras include/libcamera/camera.h | 11 +++- include/libcamera/internal/camera_sensor.h | 2 + include/libcamera/internal/v4l2_device.h | 1 + src/cam/main.cpp | 3 +- src/libcamera/camera.cpp | 54 +++++++++++++++---- src/libcamera/camera_manager.cpp | 13 +++++ src/libcamera/camera_sensor.cpp | 17 ++++++ src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++-- .../pipeline/raspberrypi/raspberrypi.cpp | 3 +- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +- src/libcamera/pipeline/simple/simple.cpp | 3 +- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++- src/libcamera/pipeline/vimc/vimc.cpp | 4 +- src/libcamera/v4l2_device.cpp | 27 ++++++++++ test/camera/buffer_import.cpp | 2 +- test/camera/capture.cpp | 2 +- test/camera/configuration_default.cpp | 2 +- test/camera/configuration_set.cpp | 2 +- test/camera/statemachine.cpp | 2 +- test/controls/control_info_map.cpp | 2 +- test/controls/control_list.cpp | 2 +- 21 files changed, 138 insertions(+), 32 deletions(-)