Re: [RFC PATCH 5/5] v4l2-ctl: add an option to list controls in a machine-readable format

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/09/19 22:15, Antonio Ospite wrote:
> On Mon, 7 Jan 2019 11:18:58 +0100
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> On 01/03/2019 07:01 PM, Antonio Ospite wrote:
>>> Add a new option --list-ctrls-values to list the values of controls in
>>> a format which can be passed again to --set-ctrl.
>>>
>>> This can be useful to save and restore device settings:
>>>
>>>   $ v4l2-ctl --list-ctrls-values >settings.txt 2>/dev/null
>>>   $ v4l2-ctl --set-ctrl "$(cat settings.txt)"
>>>
>>> The new option has been tested with the vivid driver and it works well
>>> enough to be useful with a real driver as well.
>>>
>>> String controls are not supported for now, as they may not be parsed
>>> correctly by --set-ctrl if they contain a comma or a single quote.
>>>
>>> Signed-off-by: Antonio Ospite <ao2@ao2.it>
>>> ---
>>>  utils/v4l2-ctl/v4l2-ctl-common.cpp | 72 ++++++++++++++++++++++++++----
>>>  utils/v4l2-ctl/v4l2-ctl.1.in       |  4 ++
>>>  utils/v4l2-ctl/v4l2-ctl.cpp        |  1 +
>>>  utils/v4l2-ctl/v4l2-ctl.h          |  1 +
>>>  4 files changed, 69 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> index 7777b45c..b4124608 100644
>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> [...]
>>> @@ -1102,13 +1146,23 @@ void common_get(cv4l_fd &_fd)
>>>  
>>>  void common_list(cv4l_fd &fd)
>>>  {
>>> -	if (options[OptListCtrls] || options[OptListCtrlsMenus]) {
>>> -		struct print_format classic_format = {
>>> -			.print_class_name = print_class_name,
>>> -			.print_qctrl = print_qctrl,
>>> -			.show_menus = options[OptListCtrlsMenus],
>>> -		};
>>> -
>>> -		list_controls(fd.g_fd(), &classic_format);
>>> +	if (options[OptListCtrls] || options[OptListCtrlsMenus] || options[OptListCtrlsValues]) {
>>> +		if (options[OptListCtrlsValues]) {
>>> +			struct print_format machine_format = {
>>> +				.print_class_name = NULL,
>>> +				.print_qctrl = print_qctrl_values,
>>> +				.show_menus = 0,
>>> +			};
>>> +
>>> +			list_controls(fd.g_fd(), &machine_format);
>>> +		} else {
>>> +			struct print_format classic_format = {
>>> +				.print_class_name = print_class_name,
>>> +				.print_qctrl = print_qctrl,
>>> +				.show_menus = options[OptListCtrlsMenus],
>>> +			};
>>> +
>>> +			list_controls(fd.g_fd(), &classic_format);
>>> +		}
>>
>> I don't like this struct print_format.
>>
> 
> Hi Hans,
> 
> the idea was based on two considerations:
>   1. decide the format once and for all, avoiding to check each time a
>      control is printed.
>   2. have at least some partial infrastructure in case some
>      other export formats were to be added.
> 
> But yeah, as 2. seems quite unlikely I can go with a more essential
> approach for now, no problem.
> 
>> I would prefer something like this:
>>
>> Rename print_qctrl to print_qctrl_readable() and create a new print_qctrl:
>>
>> static void print_qctrl(int fd, struct v4l2_query_ext_ctrl *queryctrl,
>>                 struct v4l2_ext_control *ctrl, int show_menus)
>> {
>> 	if (options[OptListCtrlsValues])
>> 		print_qctrl_values(fd, queryctrl, ctrl, show_menus);
>> 	else
>> 		print_qctrl_readable(fd, queryctrl, ctrl, show_menus);
>> }
>>
> 
> Since "readable" here means "human readable", while "values" is meant
> for a "machine readable" output, I'd "avoid" the word "readable" at
> all and go with "details" or "description":
> 
> 	if (options[OptListCtrlsValues])
> 		print_qctrl_values(fd, queryctrl, ctrl, show_menus);
> 	else
> 		print_qctrl_details(fd, queryctrl, ctrl, show_menus);

Hmm. Perhaps we should just be explicit:

	print_qctrl_machine_readable
	print_qctrl_human_readable

I think that's best.

> 
>> And in print_control you can just skip printing the class name if
>> options[OptListCtrlsValues] is set.
>>
> 
> OK.
> 
>> I would like to see string controls being supported. I would recommend
>> to just write the string as a hexdump. It avoids having to escape characters.
>>
>> The same can be done for compound/array controls. In fact, you could write
>> all controls that way. It would simplify things a lot.
>>
> 
> But then --set-ctrl would need to be extended to parse the hexdump,
> wouldn't it? Do you already have a syntax in mind?

I would add a new --set-ctrl-value option that takes a hexdump.
Basically the inverse of --list-ctrls-values.

Regards,

	Hans

> 
> TBH, I kept things simple hoping to re-use --set-ctrl without too much
> work.
> 
>> Also, when options[OptListCtrlsValues] is set you should skip all WRITE_ONLY
>> controls, all BUTTON controls, and all volatile controls. They are not
>> relevant if you are just interested in controls that can be set.
>>
> 
> That I will do in any case, thank you.
> 
>> Regards,
>>
>> 	Hans
> 
> Thank you,
>    Antonio
>