[libcamera-devel] [PATCH v2 2/4] libcamera: camera_sensor: Set default sensor location to Unknown

[Jacopo Mondi]

Hi Niklas,

On Fri, Feb 12, 2021 at 09:10:34AM +0100, Niklas Söderlund wrote:
> Hello Paul,
>
> Thanks for your work.
>
> On 2021-02-12 14:48:14 +0900, Paul Elder wrote:
> > Instead of choosing some arbitrary location for the sensor when its
> > location is unknown, set it explicitly to unknown.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > ---
> > No change in v2
> > ---
> >  src/libcamera/camera_sensor.cpp | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index c9e8d49b..474055ba 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -446,7 +446,7 @@ int CameraSensor::initProperties()
> >  			break;
> >  		}
> >  	} else {
> > -		propertyValue = properties::CameraLocationExternal;
> > +		propertyValue = properties::CameraLocationUnknown;
>
> I'm still unconvinced about the use-case for unknown :-)
>
> To me it looks like we are pushing the problem of an incomplete firmware
> description on to applications. How should an application handle a
> external camera vs an unknown one?
>
> I would like to understand the advantage of adding a new location here
> other then just to pass a CTS test that as far as I understands it fails
> as the device it's running on have incomplete firmware. Is that not a
> valid fail of CTS?
>

Your concerns are valid, but is it any better to default to 'Front" if
the information is not available in the firmware. Bear in mind that
location is also used to construct the unique camera name, and
instead of having two cameras arbitrary assigned to the front location
isn't it better to just report that the location isn't known ?

I this it's fair for libcamera to not assume any arbitrary defaults
and have firmware fixed :)

> >  	}
> >  	properties_.set(properties::Location, propertyValue);
> >
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel