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

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Apr 09, 2024 at 11:33:33AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxx>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> ---
>  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.