[libcamera-devel,1/2] libcamera: camera_sensor: Print warning when orientation is unknown
diff mbox series

Message ID 20210219104544.53217-2-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Fix camera location
Related show

Commit Message

Paul Elder Feb. 19, 2021, 10:45 a.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/camera_sensor.cpp | 2 ++
 1 file changed, 2 insertions(+)

Comments

Kieran Bingham Feb. 19, 2021, 10:58 a.m. UTC | #1 
On 19/02/2021 10:45, Paul Elder wrote:
> Print a warining when the orientation of a sensor is unknown. The

s/warining/warning/

> location property is still defaulted to external.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham@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";
>  		propertyValue = properties::CameraLocationExternal;
>  	}
>  	properties_.set(properties::Location, propertyValue);
>
Niklas Söderlund Feb. 19, 2021, 2:36 p.m. UTC | #2 
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@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";
    }

>  		propertyValue = properties::CameraLocationExternal;
>  	}
>  	properties_.set(properties::Location, propertyValue);
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham Feb. 19, 2021, 4:49 p.m. UTC | #3 
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@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?

--
Kieran



>     }
> 
>>  		propertyValue = properties::CameraLocationExternal;
>>  	}
>>  	properties_.set(properties::Location, propertyValue);
>> -- 
>> 2.27.0
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>
Niklas Söderlund Feb. 19, 2021, 5:19 p.m. UTC | #4 
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@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@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel
> > 
> 
> -- 
> Regards
> --
> Kieran
Laurent Pinchart Feb. 21, 2021, 7:16 p.m. UTC | #5 
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@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);
Niklas Söderlund Feb. 21, 2021, 10:17 p.m. UTC | #6 
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@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

Patch
diff mbox series 

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";
 		propertyValue = properties::CameraLocationExternal;
 	}
 	properties_.set(properties::Location, propertyValue);