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

[Niklas Söderlund]

Hi Jacopo,

On 2019-03-26 11:59:46 +0100, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Mar 26, 2019 at 11:41:23AM +0100, Niklas Söderlund wrote:
> > Hi Laurent,
> >
> > Thanks for your feedback.
> >
> > 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.
> >
> > > > 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.

> 
> 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!

> 
> Thanks
>    j
> 
> > --
> > Regards,
> > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund