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

[libgpiod][PATCH v3 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.

v2 -> v3:
- only initialize the timespec containing the timeout value once before
  entering the for loop

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 v3 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 v3 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>
Reviewed-by: Kent Gibson <warthog618@gmail.com>
---
 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..7135843 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;
@@ -450,10 +451,17 @@ int main(int argc, char **argv)
 	if (cfg.banner)
 		print_banner(argc, argv);
 
+	if (cfg.idle_timeout > 0) {
+		idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
+		idle_timeout.tv_nsec =
+				(cfg.idle_timeout % 1000000) * 1000;
+	}
+
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
+		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..08f5da9 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;
@@ -419,10 +420,17 @@ int main(int argc, char **argv)
 	if (cfg.banner)
 		print_banner(argc, argv);
 
+	if (cfg.idle_timeout > 0) {
+		idle_timeout.tv_sec = cfg.idle_timeout / 1000000;
+		idle_timeout.tv_nsec =
+				(cfg.idle_timeout % 1000000) * 1000;
+	}
+
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.idle_timeout);
+		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 v3 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 7135843..88b5ccf 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 08f5da9..e74ca96 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 v3 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 v3 0/4] tools: timeout handling improvements

[Bartosz Golaszewski]

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


On Tue, 23 Apr 2024 12:04:48 +0200, Bartosz Golaszewski wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/4] tools: rename timeout to idle_timeout in gpiomon and gpionotify
      commit: 9df101d6d6bac5a9ef42692034f3c2cfe9f2f521
[2/4] tools: use ppoll() where higher timeout resolution makes sense
      commit: bb5a2bbf5d254830502c3ef40ea22c49f557782c
[3/4] tools: allow longer time periods
      commit: e943b144d5f01c4f3da0f7898f0ec244a07e9aa6
[4/4] tools: add minutes as a new supported time unit
      commit: c34a572c535000d48dde12a805b38731dc33deb1

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>