[libgpiod][PATCH] tools: gpiomon: add timeout option

[libgpiod][PATCH] tools: gpiomon: add timeout option

[Gabriel Matni]

From: Gabriel Matni <gabriel.matni@exfo.com>

Add a timeout option which allows gpiomon to gracefully exit upon expiry.
This is handy for scripting as it allows developers to implement an action
when no trigger has been detected for a given period of time.

Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
---
diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index cc08f17dd2b4..7ef35fa69b1d 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;
+	unsigned int timeout;
 };
 
 static void print_help(void)
@@ -68,9 +69,12 @@ static void print_help(void)
 	printf("  -s, --strict\t\tabort if requested line names are not unique\n");
 	printf("      --unquoted\tdon't quote line or consumer names\n");
 	printf("      --utc\t\tformat event timestamps as UTC (default for 'realtime')\n");
+	printf("  -t, --timeout <timeout>\n");
+	printf("\t\t\tpoll timeout, format similar to debounce-period\n");
 	printf("  -v, --version\t\toutput version information and exit\n");
 	print_chip_help();
 	print_period_help();
+	print_timeout_help();
 	printf("\n");
 	printf("Format specifiers:\n");
 	printf("  %%o   GPIO line offset\n");
@@ -109,7 +113,7 @@ static int parse_event_clock_or_die(const char *option)
 
 static int parse_config(int argc, char **argv, struct config *cfg)
 {
-	static const char *const shortopts = "+b:c:C:e:E:hF:ln:p:qshv";
+	static const char *const shortopts = "+b:c:C:e:E:hF:ln:p:qst:hv";
 
 	const struct option longopts[] = {
 		{ "active-low",	no_argument,	NULL,		'l' },
@@ -128,6 +132,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 		{ "quiet",	no_argument,	NULL,		'q' },
 		{ "silent",	no_argument,	NULL,		'q' },
 		{ "strict",	no_argument,	NULL,		's' },
+		{ "timeout",	required_argument,	NULL,		't' },
 		{ "unquoted",	no_argument,	NULL,		'Q' },
 		{ "utc",	no_argument,	&cfg->timestamp_fmt,	1 },
 		{ "version",	no_argument,	NULL,		'v' },
@@ -139,6 +144,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);
@@ -188,6 +194,9 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 		case 's':
 			cfg->strict = true;
 			break;
+		case 't':
+			cfg->timeout = parse_period_or_die(optarg) / 1000;
+			break;
 		case 'h':
 			print_help();
 			exit(EXIT_SUCCESS);
@@ -442,11 +451,16 @@ int main(int argc, char **argv)
 		print_banner(argc, argv);
 
 	for (;;) {
+		int ret;
 		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/tools-common.c b/tools/tools-common.c
index a0080fcdae1f..12c350aa8a48 100644
--- a/tools/tools-common.c
+++ b/tools/tools-common.c
@@ -188,6 +188,13 @@ void print_period_help(void)
 	printf("    Supported units are 's', 'ms', and 'us'.\n");
 }
 
+void print_timeout_help(void)
+{
+	printf("\nTimeout:\n");
+	printf("    Timeout is taken as milliseconds unless a unit is specified. e.g. 1s.\n");
+	printf("    Supported units are 's', 'ms'.\n");
+}
+
 #define TIME_BUFFER_SIZE 20
 
 /*
diff --git a/tools/tools-common.h b/tools/tools-common.h
index 434e5ba5d271..2ee8f51b3c7c 100644
--- a/tools/tools-common.h
+++ b/tools/tools-common.h
@@ -92,6 +92,7 @@ unsigned int parse_uint_or_die(const char *option);
 void print_bias_help(void);
 void print_chip_help(void);
 void print_period_help(void);
+void print_timeout_help(void);
 void print_event_time(uint64_t evtime, int format);
 void print_line_attributes(struct gpiod_line_info *info, bool unquoted_strings);
 void print_line_id(struct line_resolver *resolver, int chip_num,

Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

[Kent Gibson]

On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> From: Gabriel Matni <gabriel.matni@exfo.com>
> 
> Add a timeout option which allows gpiomon to gracefully exit upon expiry.
> This is handy for scripting as it allows developers to implement an action
> when no trigger has been detected for a given period of time.
> 

The problem I have with this approach is that gpiomon exiting releases
the line(s) being monitored, so you can lose events.
So I'm not thrilled with the idea as it makes it too easy to throw
together a lossy solution without realising it is lossy.

My preferred solution is to run gpiomon as a coproc and have the
controlling script perform the timeout. e.g.

#!/bin/env bash
coproc gpiomon "$@"
while :
do
        read -t5 -u ${COPROC[0]} event || break
        echo $event
done
kill $COPROC_PID


Bart, if adding the option works for you then I can live with it, but I'm
not keen.

If it were to go ahead I would still like some changes - see below.

> Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
> ---
> diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> index cc08f17dd2b4..7ef35fa69b1d 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;
> +	unsigned int timeout;
>  };

timeout should be signed.

>  
>  static void print_help(void)
> @@ -68,9 +69,12 @@ static void print_help(void)
>  	printf("  -s, --strict\t\tabort if requested line names are not unique\n");
>  	printf("      --unquoted\tdon't quote line or consumer names\n");
>  	printf("      --utc\t\tformat event timestamps as UTC (default for 'realtime')\n");
> +	printf("  -t, --timeout <timeout>\n");
> +	printf("\t\t\tpoll timeout, format similar to debounce-period\n");

Call the option --idle-timeout, and don't provide a short form to
reduce the possibility of confusion with the gpioset -t option, which does
something very different, and to intentionally make it a little less
convenient to access.

Rather than <timeout> use <period>, so it is automatically covered by
print_period_help(), and don't reference debounce-period.

Same option should be added to gpionotify, for consistency if nothing
else.

> --- a/tools/tools-common.c
> +++ b/tools/tools-common.c
> @@ -188,6 +188,13 @@ void print_period_help(void)
>  	printf("    Supported units are 's', 'ms', and 'us'.\n");
>  }
>  
> +void print_timeout_help(void)
> +{
> +	printf("\nTimeout:\n");
> +	printf("    Timeout is taken as milliseconds unless a unit is specified. e.g. 1s.\n");
> +	printf("    Supported units are 's', 'ms'.\n");
> +}
> +

The tools-common changes are unnecessary if you change the help as suggested
above.

And in general, code doesn't belong in common if it only used in one tool.

Cheers,
Kent.

Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

[andy.shevchenko]

Tue, May 30, 2023 at 05:29:23PM +0800, Kent Gibson kirjoitti:
> On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> > From: Gabriel Matni <gabriel.matni@exfo.com>

...

> My preferred solution is to run gpiomon as a coproc and have the
> controlling script perform the timeout. e.g.
> 
> #!/bin/env bash

Oh, this is too bad.

> coproc gpiomon "$@"
> while :
> do
>         read -t5 -u ${COPROC[0]} event || break
>         echo $event
> done
> kill $COPROC_PID

I'm wondering what coproc is and why it requires bash.

What I want to have and keep that working is that all our tools can be run in
Busybox environment (embedded application). That's why I'm against seeing bash
in any form of the tooling.

-- 
With Best Regards,
Andy Shevchenko

Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

[Kent Gibson]

On Tue, May 30, 2023 at 02:21:18PM +0300, andy.shevchenko@gmail.com wrote:
> Tue, May 30, 2023 at 05:29:23PM +0800, Kent Gibson kirjoitti:
> > On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> > > From: Gabriel Matni <gabriel.matni@exfo.com>
> 
> ...
> 
> > My preferred solution is to run gpiomon as a coproc and have the
> > controlling script perform the timeout. e.g.
> > 
> > #!/bin/env bash
> 
> Oh, this is too bad.
> 
> > coproc gpiomon "$@"
> > while :
> > do
> >         read -t5 -u ${COPROC[0]} event || break
> >         echo $event
> > done
> > kill $COPROC_PID
> 
> I'm wondering what coproc is and why it requires bash.
> 

And I'm wondering why your mail got to the list, but not to me directly,
despite being directly addressed.  It isn't even in my junk folder.
And it was gmail to gmail.  Now that is weird.

Anyway, coproc is a feature of many shells, such as bash, zsh, ksh.
I don't think or ash or dash have coproc, but then you cn always use named
pipes to similar effect.  It would be similar to the simple gpioset
daemon I posted the other day, just in reverse.

I did say that coproc was my preferred solution, not that it is the only
one. 

> What I want to have and keep that working is that all our tools can be run in
> Busybox environment (embedded application). That's why I'm against seeing bash
> in any form of the tooling.
> 

It isn't IN the tooling.  It is in the shell that calls the tooling.
The tool test suite does require bash, but that is due to the framework we
use, not coproc.

I take it you would be in favour of an idle timeout option then?

I'm puzzled why no one has ever asked for it before, if it is something
that is in demand.

Cheers,
Kent.

Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

[Bartosz Golaszewski]

On Tue, May 30, 2023 at 3:09 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, May 30, 2023 at 02:21:18PM +0300, andy.shevchenko@gmail.com wrote:
> > Tue, May 30, 2023 at 05:29:23PM +0800, Kent Gibson kirjoitti:
> > > On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> > > > From: Gabriel Matni <gabriel.matni@exfo.com>
> >
> > ...
> >
> > > My preferred solution is to run gpiomon as a coproc and have the
> > > controlling script perform the timeout. e.g.
> > >
> > > #!/bin/env bash
> >
> > Oh, this is too bad.
> >
> > > coproc gpiomon "$@"
> > > while :
> > > do
> > >         read -t5 -u ${COPROC[0]} event || break
> > >         echo $event
> > > done
> > > kill $COPROC_PID
> >
> > I'm wondering what coproc is and why it requires bash.
> >
>
> And I'm wondering why your mail got to the list, but not to me directly,
> despite being directly addressed.  It isn't even in my junk folder.
> And it was gmail to gmail.  Now that is weird.
>
> Anyway, coproc is a feature of many shells, such as bash, zsh, ksh.
> I don't think or ash or dash have coproc, but then you cn always use named
> pipes to similar effect.  It would be similar to the simple gpioset
> daemon I posted the other day, just in reverse.
>
> I did say that coproc was my preferred solution, not that it is the only
> one.
>
> > What I want to have and keep that working is that all our tools can be run in
> > Busybox environment (embedded application). That's why I'm against seeing bash
> > in any form of the tooling.
> >
>
> It isn't IN the tooling.  It is in the shell that calls the tooling.
> The tool test suite does require bash, but that is due to the framework we
> use, not coproc.
>
> I take it you would be in favour of an idle timeout option then?
>
> I'm puzzled why no one has ever asked for it before, if it is something
> that is in demand.
>
> Cheers,
> Kent.

I do see value in this option. I'm not buying the argument about
losing events - the same can be said in reverse - before we even
request a line, we can lose events too.

Gabriel: please address the issues pointed out by Kent if you still
want to add this.

Bart

Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

[Kent Gibson]

On Tue, May 30, 2023 at 09:10:12PM +0200, Bartosz Golaszewski wrote:
> On Tue, May 30, 2023 at 3:09 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, May 30, 2023 at 02:21:18PM +0300, andy.shevchenko@gmail.com wrote:
> > > Tue, May 30, 2023 at 05:29:23PM +0800, Kent Gibson kirjoitti:
> > > > On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> > > > > From: Gabriel Matni <gabriel.matni@exfo.com>
> > >
> > > ...
> >
> > I take it you would be in favour of an idle timeout option then?
> >
> > I'm puzzled why no one has ever asked for it before, if it is something
> > that is in demand.
> >
> > Cheers,
> > Kent.
> 
> I do see value in this option. I'm not buying the argument about
> losing events - the same can be said in reverse - before we even
> request a line, we can lose events too.
> 
> Gabriel: please address the issues pointed out by Kent if you still
> want to add this.
> 

And consider adding a test to the test suite, assuming you have a
suitable test environment.  There are timeout tests for gpioset there
already as an example.

Cheers,
Kent.

RE: [E!] : Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

[Gabriel Matni]

Hello Kent, Bart,

It sounds good, I'll make the recommended changes and submit a new version of the patch.

Thank you!
Gabriel

-----Original Message-----
From: Kent Gibson <warthog618@gmail.com> 
Sent: Tuesday, May 30, 2023 9:26 PM
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: andy.shevchenko@gmail.com; Gabriel Matni <gabriel.matni@exfo.com>; linux-gpio@vger.kernel.org
Subject: [E!] : Re: [libgpiod][PATCH] tools: gpiomon: add timeout option

On Tue, May 30, 2023 at 09:10:12PM +0200, Bartosz Golaszewski wrote:
> On Tue, May 30, 2023 at 3:09 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, May 30, 2023 at 02:21:18PM +0300, andy.shevchenko@gmail.com wrote:
> > > Tue, May 30, 2023 at 05:29:23PM +0800, Kent Gibson kirjoitti:
> > > > On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> > > > > From: Gabriel Matni <gabriel.matni@exfo.com>
> > >
> > > ...
> >
> > I take it you would be in favour of an idle timeout option then?
> >
> > I'm puzzled why no one has ever asked for it before, if it is 
> > something that is in demand.
> >
> > Cheers,
> > Kent.
> 
> I do see value in this option. I'm not buying the argument about 
> losing events - the same can be said in reverse - before we even 
> request a line, we can lose events too.
> 
> Gabriel: please address the issues pointed out by Kent if you still 
> want to add this.
> 

And consider adding a test to the test suite, assuming you have a suitable test environment.  There are timeout tests for gpioset there already as an example.

Cheers,
Kent.