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

Slater, Joseph
2023-05-25T21:54:28Z

> -----Original Message-----
> From: Kent Gibson <warthog618@gmail.com>
> Sent: Wednesday, May 24, 2023 8:53 PM
> To: Slater, Joseph <joe.slater@windriver.com>
> Cc: linux-gpio@vger.kernel.org; MacLeod, Randy
> <Randy.MacLeod@windriver.com>
> 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@windriver.com wrote:
> > From: Joe Slater <joe.slater@windriver.com>
> >
> > 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@windriver.com>
> > ---
> > 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

>
> Cheers,
> Kent.