[libcamera-devel] [PATCH 1/2] libcamera: camera_sensor: Print warning when orientation is unknown

[Niklas Söderlund]

Hello,

On 2021-02-19 16:49:32 +0000, Kieran Bingham wrote:
> On 19/02/2021 14:36, Niklas Söderlund wrote:
> > Hi Paul,
> > 
> > Thanks for your patch.
> > 
> > On 2021-02-19 19:45:43 +0900, Paul Elder wrote:
> >> Print a warining when the orientation of a sensor is unknown. The
> >> location property is still defaulted to external.
> >>
> >> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >> ---
> >>  src/libcamera/camera_sensor.cpp | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> >> index c9e8d49b..397df266 100644
> >> --- a/src/libcamera/camera_sensor.cpp
> >> +++ b/src/libcamera/camera_sensor.cpp
> >> @@ -446,6 +446,8 @@ int CameraSensor::initProperties()
> >>  			break;
> >>  		}
> >>  	} else {
> >> +		LOG(CameraSensor, Warning)
> >> +			<< "No camera location, setting to External";
> > 
> > I think we should mimic validateSensorDriver() here to really push for 
> > that this should be fixed.
> > 
> >     ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
> >     if (ret) {
> > 	    LOG(CameraSensor, Warning)
> > 		    << "Failed to retrieve the sensor crop rectangle";
> > 	    err = -EINVAL;
> >     }
> > 
> >     if (err) {
> > 	    LOG(CameraSensor, Warning)
> > 		    << "The sensor kernel driver needs to be fixed";
> > 	    LOG(CameraSensor, Warning)
> > 		    << "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> 
> As annoying as those messages are, I think that's a good idea ;-)
> 
> Should they be 'Error' instead of 'Warning' too if we want to be really
> loud?

I think this is a bikeshedding area. So to make it even more fun I throw 
in the idea of adding a new LOG level, Depreciated. This type of log 
messages exists in other tools and I find them really useful.

The tool which comes to mind first is ansible. Once they decide a 
feature is being removed or changed they add Depreciated warning when 
it's used in the soon to be removed fashion. Then in a few releases they 
turn the warning into a Fatal. This gives me as a user quiet some time 
to fix my setup to not be effected when upstream moves on.

But as with most bikeshedding I don't feel strongly about it. All I care 
about is that we behave consistently. So if we switch to Error in this 
patch I think the LOG in validateSensorDriver() also should be updated 
to match.

> 
> --
> Kieran
> 
> 
> 
> >     }
> > 
> >>  		propertyValue = properties::CameraLocationExternal;
> >>  	}
> >>  	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
> --
> Kieran

-- 
Regards,
Niklas Söderlund