Skip to content
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

refine alsabat test script #401

Closed
wants to merge 3 commits into from
Closed

refine alsabat test script #401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07301a0
case-lib: add nocodec mode check and wav file upload functions
keqiaozhang Sep 27, 2020
7b7199a
test-case: remove plughw and refine check-alsabat.sh
keqiaozhang Oct 26, 2020
b2313f2
test-case: fix the shellcheck warning and Logic issue
keqiaozhang Oct 26, 2020
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
13 changes: 13 additions & 0 deletions case-lib/lib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,19 @@ func_lib_get_tplg_path()
return 0
}

func_nocodec_mode()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we dump the result at first and only use the variable later? Three is no need to check this mode multiple time

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we dump the result at first and only use the variable later?

This would be premature optimization. This command is only forking two additional and small processes per alsabat test, that's negligible.

{
uname -r |grep -q "nocodec"
}

func_upload_wav_file()
{
local dir=$1
local file=$2
Copy link
Collaborator

Choose a reason for hiding this comment

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

While double quotes are technically not required for assignments it's safer and good role modelling to just always quote.


find "$dir" -maxdepth 1 -type f -name "$file" -size +0 -exec cp {} "$LOG_ROOT/" \;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Avoid strings when they're not needed.

nocodec_mode()
{
    uname -r | grep -q "nocodec"
}
...
if nocodec_mode; then
...

``

Copy link
Collaborator

Choose a reason for hiding this comment

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

If running uname -r were time-consuming (it's not!) then you could want to save and re-use the result in a pseudo-boolean variable with this longer code.

I repeat this is NOT needed here, just an example for other cases.

codec_mode() # avoids one negation
{
    if time_consuming_uname -r | grep -q "nocodec"; then
      printf 'false'
    else
     printf 'true'
   fi
}
...
codec_m=$(codec_mode)
...
if "$codec_m"; then # (this runs the $codec_m command)
..
fi
...
"$codec_m" || ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any update for this. but I have same idea with this.

die()
{
dloge "$@"
Expand Down
79 changes: 50 additions & 29 deletions test-case/check-alsabat.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@
# remove the existing alsabat wav files
rm -f /tmp/bat.wav.*

source $(dirname ${BASH_SOURCE[0]})/../case-lib/lib.sh
libdir=$(dirname "${BASH_SOURCE[0]}")
# shellcheck source=case-lib/lib.sh
source "$libdir"/../case-lib/lib.sh

OPT_OPT_lst['t']='tplg' OPT_DESC_lst['t']='tplg file, default value is env TPLG: $''TPLG'
OPT_PARM_lst['t']=1 OPT_VALUE_lst['t']="$TPLG"

OPT_OPT_lst['p']='pcm_p' OPT_DESC_lst['p']='pcm for playback. Example: hw:0,0'
OPT_PARM_lst['p']=1 OPT_VALUE_lst['p']=''
Expand All @@ -29,14 +34,15 @@ OPT_PARM_lst['c']=1 OPT_VALUE_lst['c']=''
OPT_OPT_lst['f']='frequency' OPT_DESC_lst['f']='target frequency'
OPT_PARM_lst['f']=1 OPT_VALUE_lst['f']=997

OPT_OPT_lst['n']='frames' OPT_DESC_lst['n']='test frames'
OPT_OPT_lst['n']='frames' OPT_DESC_lst['n']='test frames'
OPT_PARM_lst['n']=1 OPT_VALUE_lst['n']=240000

OPT_OPT_lst['s']='sof-logger' OPT_DESC_lst['s']="Open sof-logger trace the data will store at $LOG_ROOT"
OPT_PARM_lst['s']=0 OPT_VALUE_lst['s']=1

func_opt_parse_option "$@"

tplg=${OPT_VALUE_lst['t']}
pcm_p=${OPT_VALUE_lst['p']}
pcm_c=${OPT_VALUE_lst['c']}
frequency=${OPT_VALUE_lst['f']}
Expand All @@ -48,39 +54,54 @@ then
exit 2
fi

pcmid_p=${pcm_p:-1}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, please double quote for consistency.


[[ ${OPT_VALUE_lst['s']} -eq 1 ]] && func_lib_start_log_collect

function __upload_wav_file
{
# upload the alsabat wav file
for file in /tmp/bat.wav.*
do
size=`ls -l $file | awk '{print $5}'`
if [[ $size -gt 0 ]]; then
cp $file $LOG_ROOT/
fi
done
}
func_pipeline_export "$tplg" "type:playback & id:$pcmid_p"

# parser the parameters of the specified playback pipeline
channel=$(func_pipeline_parse_value 0 ch_max)
rate=$(func_pipeline_parse_value 0 rate)
fmts=$(func_pipeline_parse_value 0 fmts)

# check the PCMs before alsabat test
dlogi "check the PCMs before alsabat test"
[[ $(aplay -Dplug$pcm_p -d 1 /dev/zero -q) ]] && die "Failed to play on PCM: $pcm_p"
[[ $(arecord -Dplug$pcm_c -d 1 /dev/null -q) ]] && die "Failed to capture on PCM: $pcm_c"
aplay -Dplug"$pcm_p" -d 1 /dev/zero -q || die "Failed to play on PCM: $pcm_p"
arecord -Dplug"$pcm_c" -d 1 /dev/null -q || die "Failed to capture on PCM: $pcm_c"
Copy link
Member

Choose a reason for hiding this comment

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

this looks like an unrelated cleanup?

Copy link
Collaborator

@marc-hb marc-hb Oct 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I requested this (serious!) bug fix, see earlier comments. Agreed it should ideally be in a different commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have separated it as a new commit.


# alsabat test
# different PCMs may support different audio formats(like samplerate, channel-counting, etc.).
# use plughw to do the audio format conversions. So we don't need to specify them for each PCM.
dlogc "alsabat -Pplug$pcm_p --standalone -n $frames -F $frequency"
alsabat -Pplug$pcm_p --standalone -n $frames -F $frequency &
# playback may have low latency, add one second delay to aviod recording zero at beginning.
sleep 1
dlogc "alsabat -Cplug$pcm_c -F $frequency"
alsabat -Cplug$pcm_c -F $frequency

# upload failed wav file
if [[ $? != 0 ]]; then
__upload_wav_file
exit 1
fi
for format in $fmts
do
if [ "$format" == "S24_LE" ]; then
dlogi "S24_LE is not supported, skip to test this format"
continue
fi

dlogc "alsabat -P $pcm_p --standalone -n $frames -F $frequency -c $channel -r $rate -f $format"
alsabat -P "$pcm_p" --standalone -n "$frames" -F "$frequency" -c "$channel" -r "$rate" \
-f "$format" & alsabatPID=$!
# playback may have low latency, add one second delay to aviod recording zero at beginning.
Copy link
Member

Choose a reason for hiding this comment

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

avoid

sleep 1

if func_nocodec_mode; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

if nocodec_mode; then would read like plain English.

format_c=$format
channel_c=$channel
else
# USB sound card only supports 1 channel S16_LE format.
format_c=S16_LE
channel_c=1
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit strange to run multiple times the same format on nocodec mode. Can you override and reduce the $fmts list instead before the loop? This will make the loop not care about special cases and simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nocodec tplgs also support different formats like s16_le/s24_le and s32_le, not passthrough mode. so we can check each format in the loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we real need alsabat for all fmts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Checking all formats available is good idea. but S24_LE is always ignored, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes，S24_4LE is not supported by alsabat

Copy link
Member

Choose a reason for hiding this comment

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

You guys need to buy better USB cards, this is not acceptable for channel swap tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea here, we are looking for new cards now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Internal issue 601 just filed about new USB cards, thanks @singalsu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you all. Let's hold this PR. Now we are trying to find a new USB cards w/ stereo capture support, then we can remove the plug completely.

fi

dlogc "alsabat -C $pcm_c -F $frequency -f $format_c -c $channel_c -r $rate"
alsabat -C "$pcm_c" -F "$frequency" -f "$format_c" -c "$channel_c" -r "$rate" || {
func_upload_wav_file "/tmp" "bat.wav.*" || true
exit 1
}
# check the alsabat -P exit code
if ! wait $alsabatPID; then
die "alsabat -P failure"
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for the ! negation

wait $alsabatPID || die "alsabat $alsabatPID failed"

fi
done

exit 0