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

[Niklas Söderlund]

Hi Jacopo,

On 2021-02-12 09:54:45 +0100, Jacopo Mondi wrote:
> 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 think it's better to default to something useful and LOG(Error) that 
we default. I would default as we do to external. That CTS fails due to 
this is just a consequence of incomplete camera integration.

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

So we should refuse to instantiate cameras who don't report locations, I 
like it ;-)

> 
> > >  	}
> > >  	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

-- 
Regards,
Niklas Söderlund