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

[Kieran Bingham]

On 13/08/2021 15:45, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Fri, Aug 13, 2021 at 03:32:41PM +0100, 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 not fond of this ad-hoc fix, as we would need similar code in other
> locations then. I'd rather try to revive the patch that creates Control
> instances for V4L2 controls.


I posted something like this ... many months ago, and it didtn' get
integrated for probably similar push back.

Yet - here we are - still lacking a /very simple/ key feature.

Leaving the debug^WError logs giving people hex codes to decipher.

How about I add a \todo Remove this when V4L2 controsl print their names
natively ;-)

I want /something/

You're blocked/ not keen on your V4L2 Control implementation because it
does things you don't like, so I have no idea when you'll merge that.

--
Kieran


> 
>> ---
>> 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;
>>  		}
>