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

[Niklas Söderlund]

Hi Laurent,

On 2021-02-21 21:16:36 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Fri, Feb 19, 2021 at 06:19:36PM +0100, Niklas Söderlund wrote:
> > On 2021-02-19 16:49:32 +0000, Kieran Bingham wrote:
> > > On 19/02/2021 14:36, Niklas Söderlund wrote:
> > > > 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.
> 
> It's an interesting idea, but given that our log levels mechanism is
> based on an integer level from least important (Debug) to most important
> (Fatak), I wonder where you would put the deprecated level in that range
> :-)

I would put it just bellow Fatal, as it would be a fatal error in a few 
releases. 

> 
> > 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.
> 
> I'd keep Warning here, as it's not an error (yet), we can still operate.
> 
> > > >     }
> > > > 
> > > >>  		propertyValue = properties::CameraLocationExternal;
> > > >>  	}
> > > >>  	properties_.set(properties::Location, propertyValue);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund