Skip to content

New aplay_opts() and arecord_opts() to change flags across all tests #498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
xiulipan merged 1 commit into from
Nov 18, 2020
Merged

New aplay_opts() and arecord_opts() to change flags across all tests #498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ To run a test, call the scripts directly

Example:
```
~/sof-test/test-case$ ./check-playback.sh -h
~/sof-test/test-case$ SOF_ALSA_OPTS='-q --fatal-errors' ./check-playback.sh -h
Usage: ./check-playback.sh [OPTION]

-F | --fmts
Expand All @@ -49,8 +49,25 @@ Usage: ./check-playback.sh [OPTION]
2020-03-19 22:13:32 UTC [INFO] Catch block option from TPLG_BLOCK_LST will block 'pcm=HDA Digital,Media Playback,DMIC16k' for /lib/firmware/intel/sof-tplg/sof-apl-pcm512x.tplg
2020-03-19 22:13:32 UTC [INFO] Run command: 'sof-tplgreader.py /lib/firmware/intel/sof-tplg/sof-apl-pcm512x.tplg -f type:playback,both -b pcm:'HDA Digital,Media Playback,DMIC16k' -s 0 -e' to get BASH Array
2020-03-19 22:13:32 UTC [INFO] Testing: (Round: 1/1) (PCM: Port5 [hw:0,0]<both>) (Loop: 1/3)
2020-03-19 22:13:32 UTC [COMMAND] aplay -q --fatal-errors -Dhw:0,0 -r 48000 -c 2 -f S16_LE -d 4 /dev/zero -v -q
...
```

Some tests support SOF_ALSA_OPTS, SOF_APLAY_OPTS and SOF_ARECORD_OPTS,
work in progress. Where supported, optional parameters in SOF_APLAY_OPTS
and SOF_ARECORD_OPTS are passed to all aplay and arecord
invocations. SOF_ALSA_OPTS parameters are passed to both aplay and
arecord. Warning these environments variables do NOT support parameters
with whitespace or globbing characters, in other words this does NOT
work:

SOF_ALSA_OPTS='--foo --bar="spaces do not work"'

For the up-to-date list of tests supporting these environment variables
run:

git grep -l 'a[[:alnum:]]*_opts'

### tools
To use tool script, call the scripts directly
* `-h` will show the usage for the tool
Expand Down
41 changes: 41 additions & 0 deletions case-lib/lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,47 @@ func_lib_get_tplg_path()
return 0
}

# We must not quote SOF_ALSA_OPTS and disable SC2086 below for two reasons:
#
# 1. We want to support multiple parameters in a single variable, in
# other words we want to support this:
#
# SOF_ALSA_OPTS="--foo --bar"
# aplay $SOF_ALSA_OPTS ...
#
# 2. aplay does not ignore empty arguments anyway, in other words this
# does not work anyway:
#
# SOF_ALSA_OPTS=""
# aplay "$SOF_ALSA_OPTS" ...
#
# This is technically incorrect because it means our SOF_ALSA_OPTS
# cannot support whitespace, for instance this would be split in two
# options: --bar="aaaa and bbbb"
#
# SOF_ALSA_OPTS='--foo --bar="aaaa bbbb"'
#
# To do this "correctly" SOF_ALSA_OPTS etc. should be arrays.
# From https://mywiki.wooledge.org/BashGuide/Arrays "The only safe way
# to represent multiple string elements in Bash is through the use of
# arrays."
#
# However, 1. arrays would complicate the user interface 2. ALSA does not
# seem to need arguments with whitespace or globbing characters.

aplay_opts()

@xiulipan xiulipan Nov 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think opts means options? maybe a more good name for the wrapper function

@marc-hb marc-hb Nov 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been looking for a better but still short name but I didn't find any, suggestions anyone? It has to be kept relatively short because it will be used a lot.

@xiulipan xiulipan Nov 9, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same feeling here. If I had a better name, I should type it already.

{
dlogc "aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $*"
# shellcheck disable=SC2086
aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS "$@"
Comment thread
xiulipan marked this conversation as resolved.

@plbossart plbossart Nov 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also want to test the return status since it contains an error code when it fails. If you just stop I think the rest of the script will probably think nothing bad happened.

@marc-hb marc-hb Nov 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vast majority of the time set -e (errexit #312) will immediately abort the script, demonstration:

$ SOF_ALSA_OPTS=--bogus ./test-case/check-playback.sh

2020-11-06 19:05:09 UTC [COMMAND] sof-tplgreader.py /lib/firmware/intel/sof-tplg/sof-apl-pcm512x.tplg -f 'type:playback & id:any' -b ' pcm:HDA Digital' -s 0 -e
2020-11-06 19:05:09 UTC [INFO] ===== Testing: (Round: 1/1) (PCM: Port5 [hw:0,0]<playback>) (Loop: 1/3) =====
2020-11-06 19:05:09 UTC [COMMAND] aplay --bogus  -Dhw:0,0 -r 48000 -c 2 -f S16_LE -d 10 /dev/zero -v -q
aplay: unrecognized option '--bogus'
Try `aplay --help' for more information.
2020-11-06 19:05:09 UTC [ERROR] aplay on PCM hw:0,0 failed at 1/3.
2020-11-06 19:05:09 UTC [ERROR] Starting func_exit_handler(), exit status=1, FUNCNAME stack:
2020-11-06 19:05:09 UTC [ERROR]  die()  @  ./test-case/../case-lib/lib.sh
2020-11-06 19:05:09 UTC [ERROR]  main()  @  ./test-case/check-playback.sh:99

Same with ./test-case/check-capture.sh

The other test SOF_ALSA_OPTS=--bogus ./test-case/multiple-pipeline-capture.sh doesn't stop immediately because multiple-pipeline-capture.sh backgrounds the aplay and arecord commands. Testing the error code inside this function would add complexity and not make any difference because the entire function is backgrounded.

What makes a difference with backgrounded commands is to collect ASAP their exit status with wait $aplayPID. This is something sof-test rarely does for now (cause we like our green failures...) but that I systematically requested in every code review involving backgrounded command(s). wait $aplay_optsPID will collect the exit status of the last command in aplay_opts() function which is of course aplay. set -e applies to wait exactly the same and if desired it can be overriden with a better message (#312) the same.

Even without considering backgrounding issues, the caller of aplay_opts caller can generally provide more useful and relevant context if a custom error message is desired.

}
arecord_opts()
{
dlogc "arecord $SOF_ALSA_OPTS $SOF_ARECORD_OPTS $*"
# shellcheck disable=SC2086
arecord $SOF_ALSA_OPTS $SOF_ARECORD_OPTS "$@"

@xiulipan xiulipan Nov 6, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for SOF_ARECORD_OPTS like SOF_APLAY_OPTS

}

die()
{
dloge "$@"
Expand Down
3 changes: 1 addition & 2 deletions test-case/check-capture.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ do
dlogi "using $file as capture output"
fi

dlogc "arecord -D$dev -r $rate -c $channel -f $fmt_elem -d $duration $file -v -q"
Comment thread
xiulipan marked this conversation as resolved.
arecord -D$dev -r $rate -c $channel -f $fmt_elem -d $duration $file -v -q
arecord_opts -D$dev -r $rate -c $channel -f $fmt_elem -d $duration $file -v -q
if [[ $? -ne 0 ]]; then
func_lib_lsof_error_dump $snd
die "arecord on PCM $dev failed at $i/$loop_cnt."
Expand Down
3 changes: 1 addition & 2 deletions test-case/check-playback.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ do
for i in $(seq 1 $loop_cnt)
do
dlogi "===== Testing: (Round: $round/$round_cnt) (PCM: $pcm [$dev]<$type>) (Loop: $i/$loop_cnt) ====="
dlogc "aplay -D$dev -r $rate -c $channel -f $fmt_elem -d $duration $file -v -q"
aplay -D"$dev" -r "$rate" -c "$channel" -f "$fmt_elem" \
aplay_opts -D"$dev" -r "$rate" -c "$channel" -f "$fmt_elem" \
-d "$duration" "$file" -v -q || {
func_lib_lsof_error_dump "$snd"
die "aplay on PCM $dev failed at $i/$loop_cnt."
Expand Down
5 changes: 2 additions & 3 deletions test-case/multiple-pipeline-capture.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ func_lib_setup_kernel_last_line

# now small function define
declare -A APP_LST DEV_LST
APP_LST['playback']='aplay'
APP_LST['playback']='aplay_opts'
DEV_LST['playback']='/dev/zero'
APP_LST['capture']='arecord'
APP_LST['capture']='arecord_opts'
DEV_LST['capture']='/dev/null'

tmp_count=$max_count
Expand All @@ -84,7 +84,6 @@ func_run_pipeline_with_type()

dlogi "Testing: $pcm [$dev]"

dlogc "${APP_LST[$1]} -D $dev -c $channel -r $rate -f $fmt ${DEV_LST[$1]} -q"
"${APP_LST[$1]}" -D $dev -c $channel -r $rate -f $fmt "${DEV_LST[$1]}" -q &

tmp_count=$(expr $tmp_count - 1 )
Expand Down