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

[Laurent Pinchart]

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
:-)

> 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