| Message ID | 20190325234736.12533-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Tue, Mar 26, 2019 at 12:47:36AM +0100, Niklas Söderlund wrote: > Add a flag to indicate if an option can be repeatable. If an option is > repeatable it must be accessed thru the array interface, even if it's > only specified once by the user. > > Also update the usage generator to indicate that tan option is s/tan/that/? > repeatable. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/cam/options.cpp | 21 +++++++++++++++------ > src/cam/options.h | 5 +++-- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/src/cam/options.cpp b/src/cam/options.cpp > index 0dec154815d3cad5..0fdde9d84ba0de0e 100644 > --- a/src/cam/options.cpp > +++ b/src/cam/options.cpp > @@ -96,7 +96,11 @@ bool OptionsBase<T>::parseValue(const T &opt, const Option &option, > break; > } > > - 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 :-) > + > return true; > } > > @@ -128,7 +132,7 @@ bool KeyValueParser::addOption(const char *name, OptionType type, > return false; > > optionsMap_[name] = Option({ 0, type, name, argument, nullptr, > - help, nullptr }); > + help, nullptr, false }); > return true; > } > > @@ -336,7 +340,7 @@ std::vector<OptionValue> OptionValue::toArray() const > > bool OptionsParser::addOption(int opt, OptionType type, const char *help, > const char *name, OptionArgument argument, > - const char *argumentName) > + const char *argumentName, bool array) > { > /* > * Options must have at least a short or long name, and a text message. > @@ -354,16 +358,16 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help, > return false; > > options_.push_back(Option({ opt, type, name, argument, argumentName, > - help, nullptr })); > + help, nullptr, array })); > optionsMap_[opt] = &options_.back(); > return true; > } > > bool OptionsParser::addOption(int opt, KeyValueParser *parser, const char *help, > - const char *name) > + const char *name, bool array) > { > if (!addOption(opt, OptionKeyValue, help, name, ArgumentRequired, > - "key=value[,key=value,...]")) > + "key=value[,key=value,...]", array)) > return false; > > options_.back().keyValueParser = parser; > @@ -461,6 +465,8 @@ void OptionsParser::usage() > length += 1 + strlen(option.argumentName); > if (option.argument == ArgumentOptional) > length += 2; > + if (option.array) > + length += 4; > > if (length > indent) > indent = length; > @@ -494,6 +500,9 @@ void OptionsParser::usage() > argument += "]"; > } > > + if (option.array) > + argument += " ..."; > + > std::cerr << std::setw(indent) << std::left << argument; > > for (const char *help = option.help, *end = help; end; ) { > 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 ? I think the rest is fine. How does the help output look like ? > > bool hasShortOption() const { return isalnum(opt); } > bool hasLongOption() const { return name != nullptr; } > @@ -126,9 +127,9 @@ public: > bool addOption(int opt, OptionType type, const char *help, > const char *name = nullptr, > OptionArgument argument = ArgumentNone, > - const char *argumentName = nullptr); > + const char *argumentName = nullptr, bool array = false); > bool addOption(int opt, KeyValueParser *parser, const char *help, > - const char *name = nullptr); > + const char *name = nullptr, bool array = false); > > Options parse(int argc, char *argv[]); > void usage();
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.
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? Also, be aware the comment reports "camera's first stream". I assume this will be changed as well. Thanks j > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
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@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.
Hi, On 2019-03-26 16:30:10 +0200, Laurent Pinchart wrote: > > > > > > 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. As this is not part of this series I will make a note of this and fix this up in my series which reworks the cam tool to make use of this series. Would we prefer the option to be named --conf, --config or --configuration ? My preference would be --config but I'm open to suggestions.
Hi Niklas, On Wed, Mar 27, 2019 at 01:07:09AM +0100, Niklas Söderlund wrote: > On 2019-03-26 16:30:10 +0200, Laurent Pinchart wrote: > >>> > >>> 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. > > As this is not part of this series I will make a note of this and fix > this up in my series which reworks the cam tool to make use of this > series. Sure. > Would we prefer the option to be named --conf, --config or > --configuration ? My preference would be --config but I'm open to > suggestions. Or maybe --streams ?
diff --git a/src/cam/options.cpp b/src/cam/options.cpp index 0dec154815d3cad5..0fdde9d84ba0de0e 100644 --- a/src/cam/options.cpp +++ b/src/cam/options.cpp @@ -96,7 +96,11 @@ bool OptionsBase<T>::parseValue(const T &opt, const Option &option, break; } - values_[opt] = value; + if (option.array) + values_[opt].add(value); + else + values_[opt] = value; + return true; } @@ -128,7 +132,7 @@ bool KeyValueParser::addOption(const char *name, OptionType type, return false; optionsMap_[name] = Option({ 0, type, name, argument, nullptr, - help, nullptr }); + help, nullptr, false }); return true; } @@ -336,7 +340,7 @@ std::vector<OptionValue> OptionValue::toArray() const bool OptionsParser::addOption(int opt, OptionType type, const char *help, const char *name, OptionArgument argument, - const char *argumentName) + const char *argumentName, bool array) { /* * Options must have at least a short or long name, and a text message. @@ -354,16 +358,16 @@ bool OptionsParser::addOption(int opt, OptionType type, const char *help, return false; options_.push_back(Option({ opt, type, name, argument, argumentName, - help, nullptr })); + help, nullptr, array })); optionsMap_[opt] = &options_.back(); return true; } bool OptionsParser::addOption(int opt, KeyValueParser *parser, const char *help, - const char *name) + const char *name, bool array) { if (!addOption(opt, OptionKeyValue, help, name, ArgumentRequired, - "key=value[,key=value,...]")) + "key=value[,key=value,...]", array)) return false; options_.back().keyValueParser = parser; @@ -461,6 +465,8 @@ void OptionsParser::usage() length += 1 + strlen(option.argumentName); if (option.argument == ArgumentOptional) length += 2; + if (option.array) + length += 4; if (length > indent) indent = length; @@ -494,6 +500,9 @@ void OptionsParser::usage() argument += "]"; } + if (option.array) + argument += " ..."; + std::cerr << std::setw(indent) << std::left << argument; for (const char *help = option.help, *end = help; end; ) { 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; bool hasShortOption() const { return isalnum(opt); } bool hasLongOption() const { return name != nullptr; } @@ -126,9 +127,9 @@ public: bool addOption(int opt, OptionType type, const char *help, const char *name = nullptr, OptionArgument argument = ArgumentNone, - const char *argumentName = nullptr); + const char *argumentName = nullptr, bool array = false); bool addOption(int opt, KeyValueParser *parser, const char *help, - const char *name = nullptr); + const char *name = nullptr, bool array = false); Options parse(int argc, char *argv[]); void usage();
Add a flag to indicate if an option can be repeatable. If an option is repeatable it must be accessed thru the array interface, even if it's only specified once by the user. Also update the usage generator to indicate that tan option is repeatable. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/cam/options.cpp | 21 +++++++++++++++------ src/cam/options.h | 5 +++-- 2 files changed, 18 insertions(+), 8 deletions(-)