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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Tue, Apr 9, 2024 at 2:56 PM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> 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.

Makes sense.

>
> > 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.
>

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.