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

[Kent Gibson <warthog618@xxxxxxxxx>]

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/value)
> +		[ "$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)

Cheers,
Kent.