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

[Kent Gibson <warthog618@gmail.com>]

On Sun, Jun 04, 2023 at 02:04:21PM +0000, Gabriel Matni wrote:
> From: Gabriel Matni <gabriel.matni@exfo.com>
> 
> Add an idle timeout option. It allows the tool to gracefully exit upon
> expiry.  This option is handy for scripting as it allows the developer to
> take an action when no event has been detected for a given period.
> 

"exit upon expiry" is vague - one of the reasons I wanted the option
renamed --idle-timeout.

So maybe just

"Add an idle timeout option to gpiomon and gpionotify to exit gracefully
when no event has been detected for a given period."

A few other minor nits below.

Otherwise looks good to me.

Cheers,
Kent.

> Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
> ---
> 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    | 15 ++++++++++++++-
>  tools/gpionotify.c | 16 +++++++++++++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> index c2684c2a4dd4..91e602cdbb5e 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 occurred during the specified period\n");

if no events occur for the period specified

>  	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;
> @@ -443,11 +451,16 @@ int main(int argc, char **argv)
>  		print_banner(argc, argv);
>  
>  	for (;;) {
> +		int ret;

Declare at the top of the function.
Hang on - there is one there already for gpiomon, so you don't need this at all.

>  		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 990ca04519b5..863c5d046542 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 occurred during the specified period\n");

as per gpiomon

>  	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;
> @@ -411,11 +420,16 @@ int main(int argc, char **argv)
>  		print_banner(argc, argv);
>  
>  	for (;;) {
> +		int ret;

Declare at the top of the function.

>  		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: b10c5b769978412e315507ae07fa554b09ca189f
> -- 
> 2.25.1