[libgpiod][PATCH 0/2] gpio-tools: allow specifying longer time periods

[libgpiod][PATCH 0/2] gpio-tools: allow specifying longer time periods

[Bartosz Golaszewski]

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

Hi Kent!

Gunnar Thörnqvist reported that he no longer can use a command like:

    $ gpioset -t 0 -p 3600s -c gpiochip0 17=0
    gpioset: invalid period: 3600s

Due to the above error. I realized we only allow ~35 minutes as we store
milliseconds in 32-bit signed integers. Here's a proposal to allow longer
time periods by using 64-bit variables.

Bartosz Golaszewski (2):
  build: fix configure error messages on missing functions
  tools: allow longer time periods

 configure.ac         |  8 +++++---
 tools/gpioget.c      |  4 ++--
 tools/gpiomon.c      | 19 ++++++++++++++-----
 tools/gpioset.c      | 16 ++++++++--------
 tools/tools-common.c | 22 ++++++++++++++++------
 tools/tools-common.h |  5 +++--
 6 files changed, 48 insertions(+), 26 deletions(-)

-- 
2.40.1

[libgpiod][PATCH 1/2] build: fix configure error messages on missing functions

[Bartosz Golaszewski]

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

Fix three incorrect messages that report missing library functions as
required to build the core library when they are actually needed to build
the gpio-tools.

Fixes: 9e69d7552cf2 ("configure: improve the header and library function checks")
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 configure.ac | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index 04787d4..3b5bbf2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -117,9 +117,9 @@ AM_CONDITIONAL([WITH_GPIOSET_INTERACTIVE],
 AS_IF([test "x$with_tools" = xtrue],
 	[# These are only needed to build tools
 	AC_CHECK_FUNC([daemon], [], [FUNC_NOT_FOUND_TOOLS([daemon])])
-	AC_CHECK_FUNC([asprintf], [], [FUNC_NOT_FOUND_LIB([asprintf])])
-	AC_CHECK_FUNC([scandir], [], [FUNC_NOT_FOUND_LIB([scandir])])
-	AC_CHECK_FUNC([versionsort], [], [FUNC_NOT_FOUND_LIB([versionsort])])
+	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])])
 	AS_IF([test "x$with_gpioset_interactive" = xtrue],
 		[PKG_CHECK_MODULES([LIBEDIT], [libedit >= 3.1])])
 	])
-- 
2.40.1

Re: [libgpiod][PATCH 1/2] build: fix configure error messages on missing functions

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 11:33 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Fix three incorrect messages that report missing library functions as
> required to build the core library when they are actually needed to build
> the gpio-tools.
>
> Fixes: 9e69d7552cf2 ("configure: improve the header and library function checks")
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  configure.ac | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 04787d4..3b5bbf2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -117,9 +117,9 @@ AM_CONDITIONAL([WITH_GPIOSET_INTERACTIVE],
>  AS_IF([test "x$with_tools" = xtrue],
>         [# These are only needed to build tools
>         AC_CHECK_FUNC([daemon], [], [FUNC_NOT_FOUND_TOOLS([daemon])])
> -       AC_CHECK_FUNC([asprintf], [], [FUNC_NOT_FOUND_LIB([asprintf])])
> -       AC_CHECK_FUNC([scandir], [], [FUNC_NOT_FOUND_LIB([scandir])])
> -       AC_CHECK_FUNC([versionsort], [], [FUNC_NOT_FOUND_LIB([versionsort])])
> +       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])])
>         AS_IF([test "x$with_gpioset_interactive" = xtrue],
>                 [PKG_CHECK_MODULES([LIBEDIT], [libedit >= 3.1])])
>         ])
> --
> 2.40.1
>

This is non-controversial, so I applied it.

Bart

[libgpiod][PATCH 2/2] tools: allow longer time periods

[Bartosz Golaszewski]

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

We currently store time as milliseconds 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      | 19 ++++++++++++++-----
 tools/gpioset.c      | 16 ++++++++--------
 tools/tools-common.c | 22 ++++++++++++++++------
 tools/tools-common.h |  5 +++--
 6 files changed, 45 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 e3abb2d..a8a3302 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 timeout;
+	long long timeout;
 };
 
 static void print_help(void)
@@ -389,9 +390,17 @@ 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("invalid debounce period: %llu",
+			    cfg.debounce_period_us);
+
 		gpiod_line_settings_set_debounce_period_us(
-			settings, cfg.debounce_period_us);
+			settings, (unsigned long)cfg.debounce_period_us);
+	}
+
+	if (cfg.timeout > INT_MAX)
+		die("invalid idle timeout: %llu", cfg.timeout);
 
 	gpiod_line_settings_set_event_clock(settings, cfg.event_clock);
 	gpiod_line_settings_set_edge_detection(settings, cfg.edges);
@@ -453,7 +462,7 @@ int main(int argc, char **argv)
 	for (;;) {
 		fflush(stdout);
 
-		ret = poll(pollfds, resolver->num_chips, cfg.timeout);
+		ret = poll(pollfds, resolver->num_chips, (int)cfg.timeout);
 		if (ret < 0)
 			die_perror("error polling for events");
 
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

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Kent Gibson]

On Tue, Apr 09, 2024 at 11:33:33AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We currently store time as milliseconds 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.
>

I don't think all timers should be extended, only where it
makes sense to do so, so gpioset (toggle and hold periods).
And maybe gpiomon (idle timeout), though you haven't extended that one,
cos poll()?  Maybe switch that to ppoll()?

More on this below.

> 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      | 19 ++++++++++++++-----
>  tools/gpioset.c      | 16 ++++++++--------
>  tools/tools-common.c | 22 ++++++++++++++++------
>  tools/tools-common.h |  5 +++--
>  6 files changed, 45 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);

Got a use case where a hold period is measured in more than seconds?
Specifically for a get.

>
>  		ret = gpiod_line_request_get_values(request, values);
>  		if (ret)
> diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> index e3abb2d..a8a3302 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 timeout;
> +	long long timeout;

Can we rename this to idle_timeout?  A variable named "timeout" is
lacking context.

>  };
>
>  static void print_help(void)
> @@ -389,9 +390,17 @@ 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("invalid debounce period: %llu",
> +			    cfg.debounce_period_us);
> +
>  		gpiod_line_settings_set_debounce_period_us(
> -			settings, cfg.debounce_period_us);
> +			settings, (unsigned long)cfg.debounce_period_us);
> +	}
> +
> +	if (cfg.timeout > INT_MAX)
> +		die("invalid idle timeout: %llu", cfg.timeout);
>

Not a fan of parsing to long, only to do a smaller range check here.
How about providing two parsers - one for int sized periods and
one for long periods, e.g. parse_long_period().

Cheers,
Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 2:56 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Apr 09, 2024 at 11:33:33AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We currently store time as milliseconds 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.
> >
>
> I don't think all timers should be extended, only where it
> makes sense to do so, so gpioset (toggle and hold periods).
> And maybe gpiomon (idle timeout), though you haven't extended that one,
> cos poll()?  Maybe switch that to ppoll()?
>
> More on this below.

Makes sense.

>
> > 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      | 19 ++++++++++++++-----
> >  tools/gpioset.c      | 16 ++++++++--------
> >  tools/tools-common.c | 22 ++++++++++++++++------
> >  tools/tools-common.h |  5 +++--
> >  6 files changed, 45 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);
>
> Got a use case where a hold period is measured in more than seconds?
> Specifically for a get.
>

Yeah, like Gunnar responded, he needs to hold the line for an hour. I
think it makes sense.

> >
> >               ret = gpiod_line_request_get_values(request, values);
> >               if (ret)
> > diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> > index e3abb2d..a8a3302 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 timeout;
> > +     long long timeout;
>
> Can we rename this to idle_timeout?  A variable named "timeout" is
> lacking context.
>

Sure but it's a different patch. Also: it's your code, just send me
the patch. :)

> >  };
> >
> >  static void print_help(void)
> > @@ -389,9 +390,17 @@ 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("invalid debounce period: %llu",
> > +                         cfg.debounce_period_us);
> > +
> >               gpiod_line_settings_set_debounce_period_us(
> > -                     settings, cfg.debounce_period_us);
> > +                     settings, (unsigned long)cfg.debounce_period_us);
> > +     }
> > +
> > +     if (cfg.timeout > INT_MAX)
> > +             die("invalid idle timeout: %llu", cfg.timeout);
> >
>
> Not a fan of parsing to long, only to do a smaller range check here.
> How about providing two parsers - one for int sized periods and
> one for long periods, e.g. parse_long_period().

I actually prefer to parse the larger range and then limit the max
size. I would be fine with adding a limit argument to parse_period()
like long long parse_period(const char *option, long long limit);

Bartosz

>
> Cheers,
> Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Kent Gibson]

On Tue, Apr 09, 2024 at 05:59:59PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 9, 2024 at 2:56 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Apr 09, 2024 at 11:33:33AM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We currently store time as milliseconds 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.
> > >
> >
> > I don't think all timers should be extended, only where it
> > makes sense to do so, so gpioset (toggle and hold periods).
> > And maybe gpiomon (idle timeout), though you haven't extended that one,
> > cos poll()?  Maybe switch that to ppoll()?
> >
> > More on this below.
>
> Makes sense.
>
> >
> > > 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      | 19 ++++++++++++++-----
> > >  tools/gpioset.c      | 16 ++++++++--------
> > >  tools/tools-common.c | 22 ++++++++++++++++------
> > >  tools/tools-common.h |  5 +++--
> > >  6 files changed, 45 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);
> >
> > Got a use case where a hold period is measured in more than seconds?
> > Specifically for a get.
> >
>
> Yeah, like Gunnar responded, he needs to hold the line for an hour. I
> think it makes sense.
>

And as I noted, I was interested in the get, the point being is a long
period always necessary and appropriate?

> > >
> > >               ret = gpiod_line_request_get_values(request, values);
> > >               if (ret)
> > > diff --git a/tools/gpiomon.c b/tools/gpiomon.c
> > > index e3abb2d..a8a3302 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 timeout;
> > > +     long long timeout;
> >
> > Can we rename this to idle_timeout?  A variable named "timeout" is
> > lacking context.
> >
>
> Sure but it's a different patch. Also: it's your code, just send me
> the patch. :)
>

Check the blame - NOT my code.

> > >  };
> > >
> > >  static void print_help(void)
> > > @@ -389,9 +390,17 @@ 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("invalid debounce period: %llu",
> > > +                         cfg.debounce_period_us);
> > > +
> > >               gpiod_line_settings_set_debounce_period_us(
> > > -                     settings, cfg.debounce_period_us);
> > > +                     settings, (unsigned long)cfg.debounce_period_us);
> > > +     }
> > > +
> > > +     if (cfg.timeout > INT_MAX)
> > > +             die("invalid idle timeout: %llu", cfg.timeout);
> > >
> >
> > Not a fan of parsing to long, only to do a smaller range check here.
> > How about providing two parsers - one for int sized periods and
> > one for long periods, e.g. parse_long_period().
>
> I actually prefer to parse the larger range and then limit the max
> size. I would be fine with adding a limit argument to parse_period()
> like long long parse_period(const char *option, long long limit);
>

I can live with that.

Cheers,
Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Kent Gibson]

On Tue, Apr 09, 2024 at 04:58:52PM +0200, Gunnar Thörnqvist wrote:
> Hi, Got a use case where a hold period is measured in more than seconds?
> Specifically for a get.:::
>
> I can see a large number of use cases where the time can be hours, days and
> weeks. In my case, pin 17 controls a relay that heats water when electricity
> is cheapest. It is ok to only have seconds as unit but the range must be
> larger. /Gunnar
>

I was asking specifically about the case for gpioget, where a long hold
period makes absolutely no sense.

Cheers,
Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Bartosz Golaszewski]

On Tue, Apr 9, 2024 at 6:05 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Apr 09, 2024 at 04:58:52PM +0200, Gunnar Thörnqvist wrote:
> > Hi, Got a use case where a hold period is measured in more than seconds?
> > Specifically for a get.:::
> >
> > I can see a large number of use cases where the time can be hours, days and
> > weeks. In my case, pin 17 controls a relay that heats water when electricity
> > is cheapest. It is ok to only have seconds as unit but the range must be
> > larger. /Gunnar
> >
>
> I was asking specifically about the case for gpioget, where a long hold
> period makes absolutely no sense.
>

One could argue that this option doesn't make sense at all for gpioget. :)

I don't think it hurts to support a longer period of time even if only
for code reuse and less surface for bugs.

Bart

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Kent Gibson]

On Tue, Apr 09, 2024 at 07:24:43PM +0200, Bartosz Golaszewski wrote:
> On Tue, Apr 9, 2024 at 6:05 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Apr 09, 2024 at 04:58:52PM +0200, Gunnar Thörnqvist wrote:
> > > Hi, Got a use case where a hold period is measured in more than seconds?
> > > Specifically for a get.:::
> > >
> > > I can see a large number of use cases where the time can be hours, days and
> > > weeks. In my case, pin 17 controls a relay that heats water when electricity
> > > is cheapest. It is ok to only have seconds as unit but the range must be
> > > larger. /Gunnar
> > >
> >
> > I was asking specifically about the case for gpioget, where a long hold
> > period makes absolutely no sense.
> >
>
> One could argue that this option doesn't make sense at all for gpioget. :)
>

And one would be wrong.  The point of the hold period for gets is to
allow the line to settle after a config change before the get itself is
performed.

> I don't think it hurts to support a longer period of time even if only
> for code reuse and less surface for bugs.
>

Well that is a complicated bit of code.

Cheers,
Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Bartosz Golaszewski]

On Wed, Apr 10, 2024 at 1:37 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Apr 09, 2024 at 07:24:43PM +0200, Bartosz Golaszewski wrote:
> > On Tue, Apr 9, 2024 at 6:05 PM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > On Tue, Apr 09, 2024 at 04:58:52PM +0200, Gunnar Thörnqvist wrote:
> > > > Hi, Got a use case where a hold period is measured in more than seconds?
> > > > Specifically for a get.:::
> > > >
> > > > I can see a large number of use cases where the time can be hours, days and
> > > > weeks. In my case, pin 17 controls a relay that heats water when electricity
> > > > is cheapest. It is ok to only have seconds as unit but the range must be
> > > > larger. /Gunnar
> > > >
> > >
> > > I was asking specifically about the case for gpioget, where a long hold
> > > period makes absolutely no sense.
> > >
> >
> > One could argue that this option doesn't make sense at all for gpioget. :)
> >
>
> And one would be wrong.  The point of the hold period for gets is to
> allow the line to settle after a config change before the get itself is
> performed.
>

One is indeed wrong.

> > I don't think it hurts to support a longer period of time even if only
> > for code reuse and less surface for bugs.
> >
>
> Well that is a complicated bit of code.
>

I'll submit the daemon RFC tomorrow or on Friday. Maybe this change
isn't even needed after all.

Bart

> Cheers,
> Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Kent Gibson]

On Wed, Apr 10, 2024 at 09:53:49AM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 10, 2024 at 1:37 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Tue, Apr 09, 2024 at 07:24:43PM +0200, Bartosz Golaszewski wrote:
> > > On Tue, Apr 9, 2024 at 6:05 PM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 09, 2024 at 04:58:52PM +0200, Gunnar Thörnqvist wrote:
> > > > > Hi, Got a use case where a hold period is measured in more than seconds?
> > > > > Specifically for a get.:::
> > > > >
> > > > > I can see a large number of use cases where the time can be hours, days and
> > > > > weeks. In my case, pin 17 controls a relay that heats water when electricity
> > > > > is cheapest. It is ok to only have seconds as unit but the range must be
> > > > > larger. /Gunnar
> > > > >
> > > >
> > > > I was asking specifically about the case for gpioget, where a long hold
> > > > period makes absolutely no sense.
> > > >
> > >
> > > One could argue that this option doesn't make sense at all for gpioget. :)
> > >
> >
> > And one would be wrong.  The point of the hold period for gets is to
> > allow the line to settle after a config change before the get itself is
> > performed.
> >
>
> One is indeed wrong.
>
> > > I don't think it hurts to support a longer period of time even if only
> > > for code reuse and less surface for bugs.
> > >
> >
> > Well that is a complicated bit of code.
> >
>
> I'll submit the daemon RFC tomorrow or on Friday. Maybe this change
> isn't even needed after all.
>

I'm not completely averse to it, but it isn't the best solution to the
use case described.  It seems to me that if you want to hold the line for
that long then the terminating condition you are after is unlikely to be
elapsed time.

If we do go ahead with it, I think you are right, we should switch to long
periods throughout for consistency.  Assuming a switch to ppoll(), the only
wrinkle then being the debounce period which is constrained to 32bit by
the uAPI.  Returning an error if the debounce is greater than 32bit is
probably best, as anyone trying to debounce for periods measured in minutes
or longer is definitely doing something wrong.
So basically your original patch, but with the switch to ppoll().

Cheers,
Kent.

Re: [libgpiod][PATCH 2/2] tools: allow longer time periods

[Kent Gibson]

On Tue, Apr 09, 2024 at 04:58:52PM +0200, Gunnar Thörnqvist wrote:
> Hi, Got a use case where a hold period is measured in more than seconds?
> Specifically for a get.:::
>
> I can see a large number of use cases where the time can be hours, days and
> weeks. In my case, pin 17 controls a relay that heats water when electricity
> is cheapest. It is ok to only have seconds as unit but the range must be
> larger. /Gunnar
>

Also, releasing the line after an elapsed time, as -p does, is rather
limiting.
This case is actually better suited to Bart's upcoming daemon,
where you could have some other entity, say cron or an automation
script, command the daemon to change the relay state given certain
conditions, including the wall clock time.

If you can't wait for Bart's daemon, you can throw together a simple one
using the interactive mode of gpioset.

e.g. this script:

  #!/bin/bash
  pipe=/tmp/relayd

  mkfifo $pipe

  trap "rm -f $pipe" EXIT

  # as bash will block until something is written to the pipe...
  echo "" > $pipe &
  gpioset -i GPIO23=0 < $pipe > /dev/null

will allow you to send commands to /tmp/relayd to control the state of
your relay.

e.g.

  echo toggle > /tmp/relayd

or

 echo "set GPIO23=1" > /tmp/relayd

Cheers,
Kent.