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

[Kent Gibson <warthog618@xxxxxxxxx>]

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

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.