Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/optparse: Add command line parser. #9538

Closed
wants to merge 18 commits into from

Conversation

jcarrano
Copy link
Contributor

@jcarrano jcarrano commented Jul 10, 2018

Contribution description

UPDATE April 2019: I completely reworked the API and the code and this description too.

This PR adds a generic command line parser that can be used by shell commands. See #11400 for an example.

Features

  • Supports short (-c) and long (--color) options and switches in addition to positional arguments (and also optional positional arguments).
  • Handling of arguments beginning with "-" by using "--" to mark the end of options.
  • Configuration for parsers can be stored in static structures and arrays (in flash.)
  • Generates friendly error messages and help strings.
  • Common functionality included: parse ints, floats, set flags, count occurrences, etc.
  • Extensible via user callbacks.

There are two components to this module: the data structures (i.e. the header) and the code itself (i.e. what is run). I'll explain why I consider both separately:

Code.

Currently shell commands implement their own parsing leading to:

  • Duplicated code and duplicated effort.
  • Duplicated bugs.
  • Possible inconsistencies in command line argument syntax.
  • The parsers end up being pretty basic.

The reason is that parsing anything is usually very easy to get wrong.

Data structures

Beyond de-duplicating code, a major motivation for this module is enabling a high-level interface to RIOT commands.

Interaction with RIOT commands is currently mediated by the shell. This is a text interface that is essentially free-form both in its input as well as the output. PR #10624 tries to solve this for the output side. This PR can provide a mechanism by which command input can be delivered. This would involve commands declaring their signature via "opt_conf_t" but, instead of using the code in "optparse.c" to convert lists of strings to values, a special module can be devised specifically for machine-to-machine interaction.

It is out of scope for this PR to explore such a module. The goal of this is to introduce a way to declare command line arguments that decouples the specification from the parsing.

Testing

I included unit tests. They are not yet complete.

Issues/PRs references

See: #11400.
For an example in which parsing took more time than it should, see #9523 . It was not a huge deal, but more than it could have been.

Fixes #3355

@jcarrano jcarrano added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Jul 10, 2018
@miri64 miri64 self-assigned this Jul 10, 2018
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for working on this!

Some first comments:

  • we'll need to see some example code using this module
  • there's a lot of macro usage. please try to remove those.
  • riot convention is to typedef struct-types we use
  • no need to use "extern" for function declarations (and against RIOT conventions)
  • documentation comments should be in doxygen format

@jcarrano
Copy link
Contributor Author

jcarrano commented Jul 11, 2018

@kaspar030 The macros are to help define the setters. However, I'm thinking of removing the setters altogether. The reason I put the setters is to prevent the error where one could forget to set certain member of the structure.

Also, what are your thoughts on splitting the configuration structure so that the parser results are placed somewhere else? That would allow the configuration to be defined in a global const variable.
I'm thinking of making another struct like this:

typedef union opt_data { /**< this used to be in opt_rule */
        int *d_int;
        bool *d_bool;
        double *d_double;
        float *d_float;
        char **d_str;       /**< Pointer to a user-defined pointer */
        const char **d_cstr;  /**< Pointer to a user-defined pointer, constant variant */
        void *cb_data;
} opt_data_t;

typedef struct opt_result {
     void *arg_parser_data; /**< Callback data for arg_parser. moved from opt_conf */
     opt_data_t *rules_result; /*<  */
} opt_result_t;

@kaspar030
Copy link
Contributor

BTW, did you check out existing code, like https://github.com/cofyc/argparse or https://github.com/docopt/docopt.c?

@jcarrano
Copy link
Contributor Author

@kaspar030 docopt generates code (ugh!). argparse is quite similar to this, and seems good, but it does not support as many option types as this module.
In addition, as I mentioned before, I'm thinking it may be a good idea to modify this module so that the configuration is strictly read-only, but I wanted to know what other people thought.

jcarrano added 16 commits April 12, 2019 19:13
Add a generic command line parser that can be used by shell commands.

This is **NOT** getopt.
- Turn macros into functions
- Use DEVELHELP instead of DEBUG
- Use typedefs.
- Add some more comments.
The previous code wuld not accept constant argv.
LOAD_VAR renamed to LAZY_LOAD (because it loads lazily)
Moved variable declarations closer to where they are used.
Unnecessary for microcontrollers, many will not even have support for
it. Also saves space in the opt_rule_t structure (it was the only
8 byte member).
The handling of positional arguments was completely reworked.

Also, the command line format can now be completely specified in
read-only memory and initialized via static macros.
These changes produce a small but noticeable decrease in code size.
@jcarrano jcarrano requested review from MrKevinWeiss and removed request for MrKevinWeiss and smlng May 27, 2019 17:30
@jcarrano jcarrano added Area: sys Area: System CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable and removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Sep 9, 2019
@jcarrano jcarrano closed this Oct 10, 2019
@jcarrano jcarrano deleted the optparse branch October 10, 2019 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide getopt for platforms that don't support it
3 participants