[libcamera-devel] [PATCH 3/3] cam: options: Add support for repeatable options

[Laurent Pinchart]

Hi Niklas,

On Tue, Mar 26, 2019 at 12:03:20PM +0100, Niklas Söderlund wrote:
> On 2019-03-26 11:59:46 +0100, Jacopo Mondi wrote:
> > On Tue, Mar 26, 2019 at 11:41:23AM +0100, Niklas Söderlund wrote:
> >> On 2019-03-26 08:28:21 +0200, Laurent Pinchart wrote:
> >>>> -	values_[opt] = value;
> >>>> +	if (option.array)
> >>>> +		values_[opt].add(value);
> >>>> +	else
> >>>> +		values_[opt] = value;
> >>>
> >>> I would be tempted to overload the [] operator for values_ to just do
> >>> the right thing depending on the type, but I think it would be bother
> >>> overkill and confusing. Still tempting to play with the available toys
> >>> though :-)
> >>
> >> Playing with new toys are always fun :-) In this particular instance I
> >> think overloading [] would just add to the confusion.

Agreed.

> >>>> diff --git a/src/cam/options.h b/src/cam/options.h
> >>>> index 6a887416c0070c41..1dac15ea90f2ffd2 100644
> >>>> --- a/src/cam/options.h
> >>>> +++ b/src/cam/options.h
> >>>> @@ -36,6 +36,7 @@ struct Option {
> >>>>  	const char *argumentName;
> >>>>  	const char *help;
> >>>>  	KeyValueParser *keyValueParser;
> >>>> +	bool array;
> >>>
> >>> Maybe isArray ?
> >>
> >> Better, will switch in next version.
> >>
> >>>
> >>> I think the rest is fine. How does the help output look like ?
> >>
> >> $ ./cam --help
> >> Options:
> >>   -c, --camera camera                                   Specify which camera to operate on
> >>   -C, --capture                                         Capture until interrupted by user
> >>   -F, --file[=filename]                                 Write captured frames to disk
> >>                                                         The first '#' character in the file name is expanded to the frame sequence number.
> >>                                                         The default file name is 'frame-#.bin'.
> >>   -f, --format key=value[,key=value,...] ...            Set format of the camera's first stream
> >>                                                         height=integer          Height in pixels
> >>                                                         id=integer              ID of stream
> >>                                                         pixelformat=integer     Pixel format
> >>                                                         width=integer           Width in pixels
> >>   -h, --help                                            Display this help message
> >>   -l, --list                                            List all cameras
> >>
> >> I let you comment on if you like the ... to indicate that an option can
> >> be repeated and post v2 after that.
> >>
> > 
> > I might not be paying too much attention, but I missed the three
> > additiona dots completely. Should we find something more explicit to
> > indicate an option can be repeated multiple times?
> 
> As far as I understand it this is the 'standard' way to document an 
> option can be repeated multiple times. I looked for a reference to 
> support this claim but came up empty handed apart from a bunch of forum 
> posts agreeing with me, but that might be search biased.
> 
> I'm happy to change this if you know of any documentation which 
> describes how it should be documented.

I don't know of a more standard way, and we can always change this
later, so

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > 
> > Also, be aware the comment reports "camera's first stream". I assume
> > this will be changed as well.
> 
> That is a bug! I will fix for next version, thanks!

We may also want to rename the parameter, if will likely specify stream
configuration, not just formats.

-- 
Regards,

Laurent Pinchart