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

["Slater, Joseph" <joe.slater@xxxxxxxxxxxxx>]

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

> 
> Cheers,
> Kent.