Re: [libgpiod][PATCH V4 1/1] tools: gpiomon/gpionotify: add idle-timeout option

Bartosz Golaszewski <brgl@xxxxxxxx> · Mon, 12 Jun 2023 18:05:11 +0200

On Mon, Jun 12, 2023 at 3:31 AM Gabriel Matni <gabriel.matni@xxxxxxxxx> wrote:
>
> From: Gabriel Matni <gabriel.matni@xxxxxxxx>
>
> Add an idle timeout option to gpiomon and gpionotify to exit gracefully
> when no event has been detected for a given period.
>
> Signed-off-by: Gabriel Matni <gabriel.matni@xxxxxxxx>
> ---
> V3 -> V4: Submitting patch with git send-email
>
> V2 -> V3: Addressed the following review comments:
> - Simplify the commit message
> - Clarify the help description of the --idle-timeout option
> - Declare ret variable at the top of function for gpionotify, not required for gpiomon
>
> V1 -> V2: Addressed the following review comments:
> - Renamed timeout option to idle-timeout
> - Timeout value is now signed
> - Reused print_period_help() for idle-timeout option
> - Added the idle-timeout option to gpionotify for consistency
>
>  tools/gpiomon.c    | 14 +++++++++++++-
>  tools/gpionotify.c | 17 +++++++++++++++--
>  2 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> index c2684c2..e3abb2d 100644
> --- a/tools/gpiomon.c
> +++ b/tools/gpiomon.c
> @@ -30,6 +30,7 @@ struct config {
>         const char *fmt;
>         enum gpiod_line_clock event_clock;
>         int timestamp_fmt;
> +       int timeout;
>  };
>
>  static void print_help(void)
> @@ -57,6 +58,8 @@ static void print_help(void)
>         printf("\t\t\tBy default 'realtime' is formatted as UTC, others as raw u64.\n");
>         printf("  -h, --help\t\tdisplay this help and exit\n");
>         printf("  -F, --format <fmt>\tspecify a custom output format\n");
> +       printf("      --idle-timeout <period>\n");
> +       printf("\t\t\texit gracefully if no events occur for the period specified\n");

I think it would be useful to state the time units used by this option here.

Bart

>         printf("  -l, --active-low\ttreat the line as active low, flipping the sense of\n");
>         printf("\t\t\trising and falling edges\n");
>         printf("      --localtime\tformat event timestamps as local time\n");
> @@ -123,6 +126,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>                 { "event-clock", required_argument, NULL,       'E' },
>                 { "format",     required_argument, NULL,        'F' },
>                 { "help",       no_argument,    NULL,           'h' },
> +               { "idle-timeout",       required_argument,      NULL,           'i' },
>                 { "localtime",  no_argument,    &cfg->timestamp_fmt,    2 },
>                 { "num-events", required_argument, NULL,        'n' },
>                 { "quiet",      no_argument,    NULL,           'q' },
> @@ -139,6 +143,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>         memset(cfg, 0, sizeof(*cfg));
>         cfg->edges = GPIOD_LINE_EDGE_BOTH;
>         cfg->consumer = "gpiomon";
> +       cfg->timeout = -1;
>
>         for (;;) {
>                 optc = getopt_long(argc, argv, shortopts, longopts, &opti);
> @@ -170,6 +175,9 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>                 case 'F':
>                         cfg->fmt = optarg;
>                         break;
> +               case 'i':
> +                       cfg->timeout = parse_period_or_die(optarg) / 1000;
> +                       break;
>                 case 'l':
>                         cfg->active_low = true;
>                         break;
> @@ -445,9 +453,13 @@ int main(int argc, char **argv)
>         for (;;) {
>                 fflush(stdout);
>
> -               if (poll(pollfds, resolver->num_chips, -1) < 0)
> +               ret = poll(pollfds, resolver->num_chips, cfg.timeout);
> +               if (ret < 0)
>                         die_perror("error polling for events");
>
> +               if (ret == 0)
> +                       goto done;
> +
>                 for (i = 0; i < resolver->num_chips; i++) {
>                         if (pollfds[i].revents == 0)
>                                 continue;
> diff --git a/tools/gpionotify.c b/tools/gpionotify.c
> index 990ca04..2c56590 100644
> --- a/tools/gpionotify.c
> +++ b/tools/gpionotify.c
> @@ -23,6 +23,7 @@ struct config {
>         const char *chip_id;
>         const char *fmt;
>         int timestamp_fmt;
> +       int timeout;
>  };
>
>  static void print_help(void)
> @@ -43,6 +44,8 @@ static void print_help(void)
>         printf("\t\t\t(default is all events)\n");
>         printf("  -h, --help\t\tdisplay this help and exit\n");
>         printf("  -F, --format <fmt>\tspecify a custom output format\n");
> +       printf("      --idle-timeout <period>\n");
> +       printf("\t\t\texit gracefully if no events occur for the period specified\n");
>         printf("      --localtime\tconvert event timestamps to local time\n");
>         printf("  -n, --num-events <num>\n");
>         printf("\t\t\texit after processing num events\n");
> @@ -52,6 +55,7 @@ static void print_help(void)
>         printf("      --utc\t\tconvert event timestamps to UTC\n");
>         printf("  -v, --version\t\toutput version information and exit\n");
>         print_chip_help();
> +       print_period_help();
>         printf("\n");
>         printf("Format specifiers:\n");
>         printf("  %%o   GPIO line offset\n");
> @@ -89,6 +93,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>                 { "event",      required_argument, NULL,        'e' },
>                 { "format",     required_argument, NULL,        'F' },
>                 { "help",       no_argument,    NULL,           'h' },
> +               { "idle-timeout",       required_argument,      NULL,           'i' },
>                 { "localtime",  no_argument,    &cfg->timestamp_fmt, 2 },
>                 { "num-events", required_argument, NULL,        'n' },
>                 { "quiet",      no_argument,    NULL,           'q' },
> @@ -103,6 +108,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>         int opti, optc;
>
>         memset(cfg, 0, sizeof(*cfg));
> +       cfg->timeout = -1;
>
>         for (;;) {
>                 optc = getopt_long(argc, argv, shortopts, longopts, &opti);
> @@ -125,6 +131,9 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>                 case 'F':
>                         cfg->fmt = optarg;
>                         break;
> +               case 'i':
> +                       cfg->timeout = parse_period_or_die(optarg) / 1000;
> +                       break;
>                 case 'n':
>                         cfg->events_wanted = parse_uint_or_die(optarg);
>                         break;
> @@ -362,7 +371,7 @@ static void event_print(struct gpiod_info_event *event,
>
>  int main(int argc, char **argv)
>  {
> -       int i, j, events_done = 0, evtype;
> +       int i, j, ret, events_done = 0, evtype;
>         struct line_resolver *resolver;
>         struct gpiod_info_event *event;
>         struct gpiod_chip **chips;
> @@ -413,9 +422,13 @@ int main(int argc, char **argv)
>         for (;;) {
>                 fflush(stdout);
>
> -               if (poll(pollfds, resolver->num_chips, -1) < 0)
> +               ret = poll(pollfds, resolver->num_chips, cfg.timeout);
> +               if (ret < 0)
>                         die_perror("error polling for events");
>
> +               if (ret == 0)
> +                       goto done;
> +
>                 for (i = 0; i < resolver->num_chips; i++) {
>                         if (pollfds[i].revents == 0)
>                                 continue;
>
> base-commit: 9788bdd3d6791205431e65366dcc518446f7ca6a
> --
> 2.25.1
>