[libcamera-devel] [PATCH] libcamera: v4l2_device: Log control id instead of errorIdx

[Umang Jain]

Hi Jacopo,

On 9/28/22 11:30 AM, Jacopo Mondi wrote:
> Hi Umang
>
> On Sun, Sep 25, 2022 at 05:53:24PM +0530, Umang Jain via libcamera-devel wrote:
>> v4l2_ext_controls.errorIdx (in the case of single failing control for
>> VIDIOC_*_EXT_CTRLS calls) represents the index of that control.
>> Since it is a single control, we can print the control id rather than
>> its index. This improves logging as the id can be easily co-related
>> with the controls while reading the log.
> If we do that we lose the ability to report errors during the
> valiation step:

Validation errors are reported separately, just above this patch's hunk(s).

See here: 
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_device.cpp#n239
>
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-g-ext-ctrls.html?highlight=ext_ctrl#c.V4L.VIDIOC_S_EXT_CTRLS
> If the error is associated with a particular control, then error_idx
> is set to the index of that control. If the error is not related to a
> specific control, or the validation step failed (see below), then
> error_idx is set to count. The value is undefined if the ioctl
> returned 0 (success).
>
> There's more context in the documentation page about validation.
>
> Also, if a single control has failed, error_idx will report the index
> of such control already, so I'm missing what we gain here...

index for e.g.  '3' is less readable than control id, say : (0x009819e2)

Hence we already print controls (in the debug mode, see below) with 
their control-ids, one can easily figure out which (specific) control is 
failing just by looking at the log. Hence, we "gain" read-ability here.
...
[1:40:41.345462669] [16014] DEBUG V4L2 v4l2_device.cpp:625 
/dev/video13[16:cap]: Control: Blue Balance (0x0098090f)
[1:40:41.345590216] [16014] DEBUG V4L2 v4l2_device.cpp:625 
/dev/video13[16:cap]: Control: Colour Correction Matrix (0x009819e1)
[1:40:41.345707893] [16014] DEBUG V4L2 v4l2_device.cpp:625 
/dev/video13[16:cap]: Control: Lens Shading (0x009819e2)
[1:40:41.345818848] [16014] DEBUG V4L2 v4l2_device.cpp:625 
/dev/video13[16:cap]: Control: Black Level (0x009819e3)
....


>
> Thanks
>     j
>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/libcamera/v4l2_device.cpp | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 83901763..c60f7c91 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -244,7 +244,8 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>>   		}
>>
>>   		/* A specific control failed. */
>> -		LOG(V4L2, Error) << "Unable to read control " << errorIdx
>> +		const unsigned int id = v4l2Ctrls[errorIdx].id;
>> +		LOG(V4L2, Error) << "Unable to read control " << utils::hex(id)
>>   				 << ": " << strerror(-ret);
>>
>>   		v4l2Ctrls.resize(errorIdx);
>> @@ -354,7 +355,8 @@ int V4L2Device::setControls(ControlList *ctrls)
>>   		}
>>
>>   		/* A specific control failed. */
>> -		LOG(V4L2, Error) << "Unable to set control " << errorIdx
>> +		const unsigned int id = v4l2Ctrls[errorIdx].id;
>> +		LOG(V4L2, Error) << "Unable to set control " << utils::hex(id)
>>   				 << ": " << strerror(-ret);
>>
>>   		v4l2Ctrls.resize(errorIdx);
>> --
>> 2.37.3
>>