[libgpiod][PATCH v2 0/4] tools: timeout handling improvements

[libgpiod][PATCH v2 0/4] tools: timeout handling improvements

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Here's an assortment of improvements to parsing and handling of timeouts
in gpio-tools.

I still decided to unify the period parsing between gpioset and gpioget even
if it doesn't make much sense for gpioget to support periods longer than
fractions of a second. Let users decide.

v1 -> v2:
- extend the series with renaming the timeout variable, using ppoll() instead
  of poll() and supporting minutes as time unit
- drop already applied patch

Bartosz Golaszewski (4):
  tools: rename timeout to idle_timeout in gpiomon and gpionotify
  tools: use ppoll() where higher timeout resolution makes sense
  tools: allow longer time periods
  tools: add minutes as a new supported time unit

 configure.ac         |  2 ++
 tools/gpioget.c      |  4 ++--
 tools/gpiomon.c      | 28 +++++++++++++++++++++-------
 tools/gpionotify.c   | 16 ++++++++++++----
 tools/gpioset.c      | 16 ++++++++--------
 tools/tools-common.c | 32 ++++++++++++++++++++++----------
 tools/tools-common.h |  5 +++--
 7 files changed, 70 insertions(+), 33 deletions(-)

-- 
2.40.1

[libgpiod][PATCH v2 1/4] tools: rename timeout to idle_timeout in gpiomon and gpionotify

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use a more meaningful name for the variable storing the idle timeout
value.

Suggested-by: Kent Gibson <warthog618@gmail.com>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpiomon.c    | 8 ++++----
 tools/gpionotify.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index e3abb2d..40e6ac2 100644
--- a/tools/gpiomon.c
+++ b/tools/gpiomon.c
@@ -30,7 +30,7 @@ struct config {
 	const char *fmt;
 	enum gpiod_line_clock event_clock;
 	int timestamp_fmt;
-	int timeout;
+	int idle_timeout;
 };
 
 static void print_help(void)
@@ -143,7 +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;
+	cfg->idle_timeout = -1;
 
 	for (;;) {
 		optc = getopt_long(argc, argv, shortopts, longopts, &opti);
@@ -176,7 +176,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 			cfg->fmt = optarg;
 			break;
 		case 'i':
-			cfg->timeout = parse_period_or_die(optarg) / 1000;
+			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
 			break;
 		case 'l':
 			cfg->active_low = true;
@@ -453,7 +453,7 @@ int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.timeout);
+		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
 		if (ret < 0)
 			die_perror("error polling for events");
 
diff --git a/tools/gpionotify.c b/tools/gpionotify.c
index 2c56590..d2aee15 100644
--- a/tools/gpionotify.c
+++ b/tools/gpionotify.c
@@ -23,7 +23,7 @@ struct config {
 	const char *chip_id;
 	const char *fmt;
 	int timestamp_fmt;
-	int timeout;
+	int idle_timeout;
 };
 
 static void print_help(void)
@@ -108,7 +108,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 	int opti, optc;
 
 	memset(cfg, 0, sizeof(*cfg));
-	cfg->timeout = -1;
+	cfg->idle_timeout = -1;
 
 	for (;;) {
 		optc = getopt_long(argc, argv, shortopts, longopts, &opti);
@@ -132,7 +132,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 			cfg->fmt = optarg;
 			break;
 		case 'i':
-			cfg->timeout = parse_period_or_die(optarg) / 1000;
+			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
 			break;
 		case 'n':
 			cfg->events_wanted = parse_uint_or_die(optarg);
@@ -422,7 +422,7 @@ int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.timeout);
+		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
 		if (ret < 0)
 			die_perror("error polling for events");
 
-- 
2.40.1

[libgpiod][PATCH v2 2/4] tools: use ppoll() where higher timeout resolution makes sense

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We allow timeout units to be specified in microseconds but for poll() we
need to round them up to milliseconds. Switch to ppoll() to avoid losing
precision.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/gpiomon.c    | 12 ++++++++++--
 tools/gpionotify.c | 12 ++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index 40e6ac2..728a671 100644
--- a/tools/gpiomon.c
+++ b/tools/gpiomon.c
@@ -176,7 +176,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 			cfg->fmt = optarg;
 			break;
 		case 'i':
-			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
+			cfg->idle_timeout = parse_period_or_die(optarg);
 			break;
 		case 'l':
 			cfg->active_low = true;
@@ -362,6 +362,7 @@ int main(int argc, char **argv)
 	int num_lines, events_done = 0;
 	struct gpiod_edge_event *event;
 	struct line_resolver *resolver;
+	struct timespec idle_timeout;
 	struct gpiod_chip *chip;
 	struct pollfd *pollfds;
 	unsigned int *offsets;
@@ -453,7 +454,14 @@ int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
+		if (cfg.idle_timeout > 0) {
+			idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
+			idle_timeout.tv_nsec =
+					(cfg.idle_timeout % 1000000) * 1000;
+		}
+
+		ret = ppoll(pollfds, resolver->num_chips,
+			    cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
 		if (ret < 0)
 			die_perror("error polling for events");
 
diff --git a/tools/gpionotify.c b/tools/gpionotify.c
index d2aee15..962896c 100644
--- a/tools/gpionotify.c
+++ b/tools/gpionotify.c
@@ -132,7 +132,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
 			cfg->fmt = optarg;
 			break;
 		case 'i':
-			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
+			cfg->idle_timeout = parse_period_or_die(optarg);
 			break;
 		case 'n':
 			cfg->events_wanted = parse_uint_or_die(optarg);
@@ -374,6 +374,7 @@ int main(int argc, char **argv)
 	int i, j, ret, events_done = 0, evtype;
 	struct line_resolver *resolver;
 	struct gpiod_info_event *event;
+	struct timespec idle_timeout;
 	struct gpiod_chip **chips;
 	struct gpiod_chip *chip;
 	struct pollfd *pollfds;
@@ -422,7 +423,14 @@ int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
+		if (cfg.idle_timeout > 0) {
+			idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
+			idle_timeout.tv_nsec =
+					(cfg.idle_timeout % 1000000) * 1000;
+		}
+
+		ret = ppoll(pollfds, resolver->num_chips,
+			    cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
 		if (ret < 0)
 			die_perror("error polling for events");
 
-- 
2.40.1

[libgpiod][PATCH v2 3/4] tools: allow longer time periods

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We currently store time as microseconds in 32-bit integers and allow
seconds as the longest time unit when parsing command-line arguments
limiting the time period possible to specify when passing arguments such
as --hold-period to 35 minutes. Let's use 64-bit integers to vastly
increase that.

Use nanosleep() instead of usleep() to extend the possible sleep time
range.

Reported-by: Gunnar Thörnqvist <gunnar@igl.se>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 configure.ac         |  2 ++
 tools/gpioget.c      |  4 ++--
 tools/gpiomon.c      | 14 ++++++++++----
 tools/gpionotify.c   |  2 +-
 tools/gpioset.c      | 16 ++++++++--------
 tools/tools-common.c | 22 ++++++++++++++++------
 tools/tools-common.h |  5 +++--
 7 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/configure.ac b/configure.ac
index 3b5bbf2..a2370c5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -120,6 +120,8 @@ AS_IF([test "x$with_tools" = xtrue],
 	AC_CHECK_FUNC([asprintf], [], [FUNC_NOT_FOUND_TOOLS([asprintf])])
 	AC_CHECK_FUNC([scandir], [], [FUNC_NOT_FOUND_TOOLS([scandir])])
 	AC_CHECK_FUNC([versionsort], [], [FUNC_NOT_FOUND_TOOLS([versionsort])])
+	AC_CHECK_FUNC([strtoull], [], [FUNC_NOT_FOUND_TOOLS([strtoull])])
+	AC_CHECK_FUNC([nanosleep], [], [FUNC_NOT_FOUND_TOOLS([nanosleep])])
 	AS_IF([test "x$with_gpioset_interactive" = xtrue],
 		[PKG_CHECK_MODULES([LIBEDIT], [libedit >= 3.1])])
 	])
diff --git a/tools/gpioget.c b/tools/gpioget.c
index f611737..bad7667 100644
--- a/tools/gpioget.c
+++ b/tools/gpioget.c
@@ -19,7 +19,7 @@ struct config {
 	bool unquoted;
 	enum gpiod_line_bias bias;
 	enum gpiod_line_direction direction;
-	unsigned int hold_period_us;
+	unsigned long long hold_period_us;
 	const char *chip_id;
 	const char *consumer;
 };
@@ -205,7 +205,7 @@ int main(int argc, char **argv)
 			die_perror("unable to request lines");
 
 		if (cfg.hold_period_us)
-			usleep(cfg.hold_period_us);
+			sleep_us(cfg.hold_period_us);
 
 		ret = gpiod_line_request_get_values(request, values);
 		if (ret)
diff --git a/tools/gpiomon.c b/tools/gpiomon.c
index 728a671..cf1857c 100644
--- a/tools/gpiomon.c
+++ b/tools/gpiomon.c
@@ -5,6 +5,7 @@
 #include <getopt.h>
 #include <gpiod.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <poll.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -24,13 +25,13 @@ struct config {
 	enum gpiod_line_bias bias;
 	enum gpiod_line_edge edges;
 	int events_wanted;
-	unsigned int debounce_period_us;
+	unsigned long long debounce_period_us;
 	const char *chip_id;
 	const char *consumer;
 	const char *fmt;
 	enum gpiod_line_clock event_clock;
 	int timestamp_fmt;
-	int idle_timeout;
+	long long idle_timeout;
 };
 
 static void print_help(void)
@@ -390,9 +391,14 @@ int main(int argc, char **argv)
 	if (cfg.active_low)
 		gpiod_line_settings_set_active_low(settings, true);
 
-	if (cfg.debounce_period_us)
+	if (cfg.debounce_period_us) {
+		if (cfg.debounce_period_us > UINT_MAX)
+			die("maximum debounce period is %uus, got %lluus",
+			    UINT_MAX, cfg.debounce_period_us);
+
 		gpiod_line_settings_set_debounce_period_us(
-			settings, cfg.debounce_period_us);
+			settings, (unsigned long)cfg.debounce_period_us);
+	}
 
 	gpiod_line_settings_set_event_clock(settings, cfg.event_clock);
 	gpiod_line_settings_set_edge_detection(settings, cfg.edges);
diff --git a/tools/gpionotify.c b/tools/gpionotify.c
index 962896c..53bd1d0 100644
--- a/tools/gpionotify.c
+++ b/tools/gpionotify.c
@@ -23,7 +23,7 @@ struct config {
 	const char *chip_id;
 	const char *fmt;
 	int timestamp_fmt;
-	int idle_timeout;
+	long long idle_timeout;
 };
 
 static void print_help(void)
diff --git a/tools/gpioset.c b/tools/gpioset.c
index 863da4a..46dde07 100644
--- a/tools/gpioset.c
+++ b/tools/gpioset.c
@@ -28,8 +28,8 @@ struct config {
 	enum gpiod_line_bias bias;
 	enum gpiod_line_drive drive;
 	int toggles;
-	unsigned int *toggle_periods;
-	unsigned int hold_period_us;
+	unsigned long long *toggle_periods;
+	unsigned long long hold_period_us;
 	const char *chip_id;
 	const char *consumer;
 };
@@ -94,10 +94,10 @@ static int parse_drive_or_die(const char *option)
 	return 0;
 }
 
-static int parse_periods_or_die(char *option, unsigned int **periods)
+static int parse_periods_or_die(char *option, unsigned long long **periods)
 {
 	int i, num_periods = 1;
-	unsigned int *pp;
+	unsigned long long *pp;
 	char *end;
 
 	for (i = 0; option[i] != '\0'; i++)
@@ -376,7 +376,7 @@ static void toggle_all_lines(struct line_resolver *resolver)
  * and apply the values to the requests.
  * offset and values are scratch pads for working.
  */
-static void toggle_sequence(int toggles, unsigned int *toggle_periods,
+static void toggle_sequence(int toggles, unsigned long long *toggle_periods,
 			    struct gpiod_line_request **requests,
 			    struct line_resolver *resolver,
 			    unsigned int *offsets,
@@ -388,7 +388,7 @@ static void toggle_sequence(int toggles, unsigned int *toggle_periods,
 		return;
 
 	for (;;) {
-		usleep(toggle_periods[i]);
+		sleep_us(toggle_periods[i]);
 		toggle_all_lines(resolver);
 		apply_values(requests, resolver, offsets, values);
 
@@ -826,7 +826,7 @@ static void interact(struct gpiod_line_request **requests,
 				printf("invalid period: '%s'\n", words[1]);
 				goto cmd_ok;
 			}
-			usleep(period_us);
+			sleep_us(period_us);
 			goto cmd_ok;
 		}
 
@@ -981,7 +981,7 @@ int main(int argc, char **argv)
 	}
 
 	if (cfg.hold_period_us)
-		usleep(cfg.hold_period_us);
+		sleep_us(cfg.hold_period_us);
 
 #ifdef GPIOSET_INTERACTIVE
 	if (cfg.interactive)
diff --git a/tools/tools-common.c b/tools/tools-common.c
index 64592d3..500e9a2 100644
--- a/tools/tools-common.c
+++ b/tools/tools-common.c
@@ -112,12 +112,12 @@ int parse_bias_or_die(const char *option)
 	return GPIOD_LINE_BIAS_DISABLED;
 }
 
-int parse_period(const char *option)
+long long parse_period(const char *option)
 {
-	unsigned long p, m = 0;
+	unsigned long long p, m = 0;
 	char *end;
 
-	p = strtoul(option, &end, 10);
+	p = strtoull(option, &end, 10);
 
 	switch (*end) {
 	case 'u':
@@ -147,15 +147,15 @@ int parse_period(const char *option)
 	}
 
 	p *= m;
-	if (*end != '\0' || p > INT_MAX)
+	if (*end != '\0' || p > LLONG_MAX)
 		return -1;
 
 	return p;
 }
 
-unsigned int parse_period_or_die(const char *option)
+unsigned long long parse_period_or_die(const char *option)
 {
-	int period = parse_period(option);
+	long long period = parse_period(option);
 
 	if (period < 0)
 		die("invalid period: %s", option);
@@ -163,6 +163,16 @@ unsigned int parse_period_or_die(const char *option)
 	return period;
 }
 
+void sleep_us(unsigned long long period)
+{
+	struct timespec	spec;
+
+	spec.tv_sec = period / 1000000;
+	spec.tv_nsec = (period % 1000000) * 1000;
+
+	nanosleep(&spec, NULL);
+}
+
 int parse_uint(const char *option)
 {
 	unsigned long o;
diff --git a/tools/tools-common.h b/tools/tools-common.h
index c82317a..bc63080 100644
--- a/tools/tools-common.h
+++ b/tools/tools-common.h
@@ -87,8 +87,9 @@ void die(const char *fmt, ...) NORETURN PRINTF(1, 2);
 void die_perror(const char *fmt, ...) NORETURN PRINTF(1, 2);
 void print_version(void);
 int parse_bias_or_die(const char *option);
-int parse_period(const char *option);
-unsigned int parse_period_or_die(const char *option);
+long long parse_period(const char *option);
+unsigned long long parse_period_or_die(const char *option);
+void sleep_us(unsigned long long period);
 int parse_uint(const char *option);
 unsigned int parse_uint_or_die(const char *option);
 void print_bias_help(void);
-- 
2.40.1

[libgpiod][PATCH v2 4/4] tools: add minutes as a new supported time unit

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Make it more convenient to specify longer time periods in gpio-tools by
introducing minutes as the new time unit.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 tools/tools-common.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/tools-common.c b/tools/tools-common.c
index 500e9a2..4340bce 100644
--- a/tools/tools-common.c
+++ b/tools/tools-common.c
@@ -138,10 +138,12 @@ long long parse_period(const char *option)
 	}
 
 	if (m) {
-		if (*end != 's')
+		if (*end == '\0')
+			m = 60000000;
+		else if (*end == 's')
+			end++;
+		else
 			return -1;
-
-		end++;
 	} else {
 		m = 1000;
 	}
@@ -213,7 +215,7 @@ void print_period_help(void)
 {
 	printf("\nPeriods:\n");
 	printf("    Periods are taken as milliseconds unless units are specified. e.g. 10us.\n");
-	printf("    Supported units are 's', 'ms', and 'us'.\n");
+	printf("    Supported units are 'm', 's', 'ms', and 'us' for minutes, seconds, milliseconds and microseconds respectively.\n");
 }
 
 #define TIME_BUFFER_SIZE 20
-- 
2.40.1

Re: [libgpiod][PATCH v2 2/4] tools: use ppoll() where higher timeout resolution makes sense

[Kent Gibson]

On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We allow timeout units to be specified in microseconds but for poll() we
> need to round them up to milliseconds. Switch to ppoll() to avoid losing
> precision.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  tools/gpiomon.c    | 12 ++++++++++--
>  tools/gpionotify.c | 12 ++++++++++--
>  2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> index 40e6ac2..728a671 100644
> --- a/tools/gpiomon.c
> +++ b/tools/gpiomon.c
> @@ -176,7 +176,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>  			cfg->fmt = optarg;
>  			break;
>  		case 'i':
> -			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
> +			cfg->idle_timeout = parse_period_or_die(optarg);
>  			break;
>  		case 'l':
>  			cfg->active_low = true;
> @@ -362,6 +362,7 @@ int main(int argc, char **argv)
>  	int num_lines, events_done = 0;
>  	struct gpiod_edge_event *event;
>  	struct line_resolver *resolver;
> +	struct timespec idle_timeout;
>  	struct gpiod_chip *chip;
>  	struct pollfd *pollfds;
>  	unsigned int *offsets;
> @@ -453,7 +454,14 @@ int main(int argc, char **argv)
>  	for (;;) {
>  		fflush(stdout);
>
> -		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> +		if (cfg.idle_timeout > 0) {
> +			idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> +			idle_timeout.tv_nsec =
> +					(cfg.idle_timeout % 1000000) * 1000;
> +		}
> +
> +		ret = ppoll(pollfds, resolver->num_chips,
> +			    cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
>  		if (ret < 0)
>  			die_perror("error polling for events");
>

One minor nit - I would introduce a timespec pointer initialised to NULL
and set to point to idle_timeout within the if rather than repeat the
cfg.idle_timeout > 0 test.

But that is just personal preference, so either way,

Reviewed-by: Kent Gibson <warthog618@gmail.com>

for the series.

Cheers,
Kent.

> diff --git a/tools/gpionotify.c b/tools/gpionotify.c
> index d2aee15..962896c 100644
> --- a/tools/gpionotify.c
> +++ b/tools/gpionotify.c
> @@ -132,7 +132,7 @@ static int parse_config(int argc, char **argv, struct config *cfg)
>  			cfg->fmt = optarg;
>  			break;
>  		case 'i':
> -			cfg->idle_timeout = parse_period_or_die(optarg) / 1000;
> +			cfg->idle_timeout = parse_period_or_die(optarg);
>  			break;
>  		case 'n':
>  			cfg->events_wanted = parse_uint_or_die(optarg);
> @@ -374,6 +374,7 @@ int main(int argc, char **argv)
>  	int i, j, ret, events_done = 0, evtype;
>  	struct line_resolver *resolver;
>  	struct gpiod_info_event *event;
> +	struct timespec idle_timeout;
>  	struct gpiod_chip **chips;
>  	struct gpiod_chip *chip;
>  	struct pollfd *pollfds;
> @@ -422,7 +423,14 @@ int main(int argc, char **argv)
>  	for (;;) {
>  		fflush(stdout);
>
> -		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> +		if (cfg.idle_timeout > 0) {
> +			idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> +			idle_timeout.tv_nsec =
> +					(cfg.idle_timeout % 1000000) * 1000;
> +		}
> +
> +		ret = ppoll(pollfds, resolver->num_chips,
> +			    cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
>  		if (ret < 0)
>  			die_perror("error polling for events");
>
> --
> 2.40.1
>

Re: [libgpiod][PATCH v2 2/4] tools: use ppoll() where higher timeout resolution makes sense

[Bartosz Golaszewski]

On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> >
> > -             ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> > +             if (cfg.idle_timeout > 0) {
> > +                     idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> > +                     idle_timeout.tv_nsec =
> > +                                     (cfg.idle_timeout % 1000000) * 1000;
> > +             }
> > +
> > +             ret = ppoll(pollfds, resolver->num_chips,
> > +                         cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
> >               if (ret < 0)
> >                       die_perror("error polling for events");
> >
>
> One minor nit - I would introduce a timespec pointer initialised to NULL
> and set to point to idle_timeout within the if rather than repeat the
> cfg.idle_timeout > 0 test.
>

Actually we can avoid it by doing it once before we enter the for(;;)
loop. It's passed by constant pointer to ppoll() anyway and having the
struct AND pointer to it initialized to NULL sounds more complex than
it needs to be. I'll do it in v2.

> But that is just personal preference, so either way,
>
> Reviewed-by: Kent Gibson <warthog618@gmail.com>
>

Thanks, I'll keep the tag if you don't mind the above solution?

Bart

Re: [libgpiod][PATCH v2 2/4] tools: use ppoll() where higher timeout resolution makes sense

[Kent Gibson]

On Mon, Apr 22, 2024 at 08:15:46PM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 17, 2024 at 9:23 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Apr 16, 2024 at 11:52:20PM +0200, Bartosz Golaszewski wrote:
> > >
> > > -             ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
> > > +             if (cfg.idle_timeout > 0) {
> > > +                     idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
> > > +                     idle_timeout.tv_nsec =
> > > +                                     (cfg.idle_timeout % 1000000) * 1000;
> > > +             }
> > > +
> > > +             ret = ppoll(pollfds, resolver->num_chips,
> > > +                         cfg.idle_timeout > 0 ? &idle_timeout : NULL, NULL);
> > >               if (ret < 0)
> > >                       die_perror("error polling for events");
> > >
> >
> > One minor nit - I would introduce a timespec pointer initialised to NULL
> > and set to point to idle_timeout within the if rather than repeat the
> > cfg.idle_timeout > 0 test.
> >
>
> Actually we can avoid it by doing it once before we enter the for(;;)
> loop. It's passed by constant pointer to ppoll() anyway and having the
> struct AND pointer to it initialized to NULL sounds more complex than
> it needs to be. I'll do it in v2.
>

Ah, I overlooked the for loop, so you are right that it should be
initialised before that.

> > But that is just personal preference, so either way,
> >
> > Reviewed-by: Kent Gibson <warthog618@gmail.com>
> >
>
> Thanks, I'll keep the tag if you don't mind the above solution?
>

Sure.

Cheers,
Kent.