[libcamera-devel] [PATCH 0/9] libcamera: camera: Add camera ID
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Jul 25 10:35:26 CEST 2020
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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list