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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Mon, May 29, 2023 at 08:20:44PM +0000, Gabriel Matni wrote:
> From: Gabriel Matni <gabriel.matni@xxxxxxxx>
> 
> 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@xxxxxxxx>
> ---
> 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.