Re: [libgpiod][PATCH 1/1] gpio-tools-test.bats: modify delays in toggle test

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Thu, May 25, 2023 at 11:54 PM Slater, Joseph
<joe.slater@xxxxxxxxxxxxx> wrote:
>
>
>
> > -----Original Message-----
> > From: Kent Gibson <warthog618@xxxxxxxxx>
> > Sent: Wednesday, May 24, 2023 8:53 PM
> > To: Slater, Joseph <joe.slater@xxxxxxxxxxxxx>
> > Cc: linux-gpio@xxxxxxxxxxxxxxx; MacLeod, Randy
> > <Randy.MacLeod@xxxxxxxxxxxxx>
> > Subject: Re: [libgpiod][PATCH 1/1] gpio-tools-test.bats: modify delays in toggle
> > test
> >
> > On Wed, May 24, 2023 at 02:09:45PM -0700, joe.slater@xxxxxxxxxxxxx wrote:
> > > From: Joe Slater <joe.slater@xxxxxxxxxxxxx>
> > >
> > > The test "gpioset: toggle (continuous)" uses fixed delays to test
> > > toggling values.  This is not reliable, so we switch to looking for
> > > transitions from one value to another.
> > >
> >
> > That test is prone to spurious failures if either the test or gpioset get delayed for
> > any reason, so good idea.
> >
> > > Signed-off-by: Joe Slater <joe.slater@xxxxxxxxxxxxx>
> > > ---
> > >  tools/gpio-tools-test.bats | 24 +++++++++++++++++++-----
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/gpio-tools-test.bats b/tools/gpio-tools-test.bats
> > > index adbce94..977d718 100755
> > > --- a/tools/gpio-tools-test.bats
> > > +++ b/tools/gpio-tools-test.bats
> > > @@ -141,6 +141,20 @@ gpiosim_check_value() {
> > >     [ "$VAL" = "$EXPECTED" ]
> > >  }
> > >
> > > +gpiosim_wait_value() {
> > > +   local OFFSET=$2
> > > +   local EXPECTED=$3
> > > +   local DEVNAME=${GPIOSIM_DEV_NAME[$1]}
> > > +   local CHIPNAME=${GPIOSIM_CHIP_NAME[$1]}
> > > +
> > > +   for i in {1..10} ; do
> > > +
> >       VAL=$(<$GPIOSIM_SYSFS/$DEVNAME/$CHIPNAME/sim_gpio$OFFSET/v
> > alue)
> > > +           [ "$VAL" = "$EXPECTED" ] && return
> > > +           sleep 0.1
> > > +   done
> > > +   return 1
> > > +}
> > > +
> >
> > Not a huge fan of the loop limit and sleep period being hard coded, as those
> > control the time to wait, but as it is only used in the one place I can live with it.
> >
> > >  gpiosim_cleanup() {
> > >     for CHIP in ${!GPIOSIM_CHIP_NAME[@]}
> > >     do
> > > @@ -1567,15 +1581,15 @@ request_release_line() {
> > >     gpiosim_check_value sim0 4 0
> > >     gpiosim_check_value sim0 7 0
> > >
> > > -   sleep 1
> > > -
> > > -   gpiosim_check_value sim0 1 0
> > > +   # sleeping fixed amounts can be unreliable, so we
> > > +   # sync to the toggles
> > > +   #
> > > +   gpiosim_wait_value sim0 1 0
> > >     gpiosim_check_value sim0 4 1
> > >     gpiosim_check_value sim0 7 1
> > >
> >
> > The comment is confusing once the sleep is removed, so just drop it.
> > If you want to describe what gpiosim_wait_value() does and when it should be
> > used then add that before the function itself.
> >
> > The test toggles the line at 1s intervals to try to improve the chances of the test
> > and gpioset staying in sync.
> > Could that be reduced now, without impacting reliability?
> > (this test suite being glacial is a personal bugbear)
>
> [Slater, Joseph] I'll get rid of the comment and try the test with a shorter toggle time.
> The series of 159 tests takes, maybe, 10-15 minutes for me, so I don't think saving a
> second or two here would make much difference, though.
> Joe
>

That can't be right, are you running it on a toaster? It takes 26
seconds on my regular lenovo thinkpad laptop which is still longer
than I'd like but quite acceptable for now (though I agree it would be
great to improve it).

Bart