[libcamera-devel] [PATCH] controls: Report required control names

[Kieran Bingham]

Of course the title prefix of this should be

libcamera: camera_sensor:

--
Kieran


On 13/08/2021 15:32, Kieran Bingham wrote:
> The V4L2 controls which are optional, recommended, and mandatory report
> errors if they are not found by CameraSensor::validateSensorDriver().
> 
> These errors report hex values for the controls, which can easily lead
> to confusion and incorrect assumption about which control needs to be
> investigated.
> 
> This can be seen occuring already in issues such as [0] and is generally
> an unfriendly result to report to users in a warning or error message.
> 
> Adapt the tables of controls to store both the id and name of the
> controls to report a user friendly message that can ease diagnosis of
> any CameraSensor validation issues.
> 
>  [0] https://github.com/raspberrypi/linux/issues/4500
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> ---
> I'm aware of course of the checkstyle changes suggested on V4L2_CID, but
> I thought it was better to keep those macros 'scoped' here, and indented
> accordingly.
> ---
>  src/libcamera/camera_sensor.cpp | 45 ++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 7a3864150c89..1409da1b071c 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -168,19 +168,26 @@ int CameraSensor::validateSensorDriver()
>  {
>  	int err = 0;
>  
> +	struct v4l2_cid {
> +		const uint32_t id;
> +		const char *name;
> +	};
> +
> +	#define V4L2_CID(x) { x, #x }
> +
>  	/*
>  	 * Optional controls are used to register optional sensor properties. If
>  	 * not present, some values will be defaulted.
>  	 */
> -	static constexpr uint32_t optionalControls[] = {
> -		V4L2_CID_CAMERA_SENSOR_ROTATION,
> +	static constexpr v4l2_cid optionalControls[] = {
> +		V4L2_CID(V4L2_CID_CAMERA_SENSOR_ROTATION),
>  	};
>  
>  	const ControlIdMap &controls = subdev_->controls().idmap();
> -	for (uint32_t ctrl : optionalControls) {
> -		if (!controls.count(ctrl))
> +	for (auto &ctrl : optionalControls) {
> +		if (!controls.count(ctrl.id))
>  			LOG(CameraSensor, Debug)
> -				<< "Optional V4L2 control " << utils::hex(ctrl)
> +				<< "Optional V4L2 control " << ctrl.name
>  				<< " not supported";
>  	}
>  
> @@ -188,14 +195,14 @@ int CameraSensor::validateSensorDriver()
>  	 * Recommended controls are similar to optional controls, but will
>  	 * become mandatory in the near future. Be loud if they're missing.
>  	 */
> -	static constexpr uint32_t recommendedControls[] = {
> -		V4L2_CID_CAMERA_ORIENTATION,
> +	static constexpr v4l2_cid recommendedControls[] = {
> +		V4L2_CID(V4L2_CID_CAMERA_ORIENTATION),
>  	};
>  
> -	for (uint32_t ctrl : recommendedControls) {
> -		if (!controls.count(ctrl)) {
> +	for (auto &ctrl : recommendedControls) {
> +		if (!controls.count(ctrl.id)) {
>  			LOG(CameraSensor, Warning)
> -				<< "Recommended V4L2 control " << utils::hex(ctrl)
> +				<< "Recommended V4L2 control " << ctrl.name
>  				<< " not supported";
>  			err = -EINVAL;
>  		}
> @@ -259,18 +266,20 @@ int CameraSensor::validateSensorDriver()
>  	 * For raw sensors, make sure the sensor driver supports the controls
>  	 * required by the CameraSensor class.
>  	 */
> -	static constexpr uint32_t mandatoryControls[] = {
> -		V4L2_CID_EXPOSURE,
> -		V4L2_CID_HBLANK,
> -		V4L2_CID_PIXEL_RATE,
> -		V4L2_CID_VBLANK,
> +	static constexpr v4l2_cid mandatoryControls[] = {
> +		V4L2_CID(V4L2_CID_EXPOSURE),
> +		V4L2_CID(V4L2_CID_HBLANK),
> +		V4L2_CID(V4L2_CID_PIXEL_RATE),
> +		V4L2_CID(V4L2_CID_VBLANK),
>  	};
>  
> +	#undef V4L2_CID
> +
>  	err = 0;
> -	for (uint32_t ctrl : mandatoryControls) {
> -		if (!controls.count(ctrl)) {
> +	for (auto &ctrl : mandatoryControls) {
> +		if (!controls.count(ctrl.id)) {
>  			LOG(CameraSensor, Error)
> -				<< "Mandatory V4L2 control " << utils::hex(ctrl)
> +				<< "Mandatory V4L2 control " << ctrl.name
>  				<< " not available";
>  			err = -EINVAL;
>  		}
>