refine alsabat test script by keqiaozhang · Pull Request #401 · thesofproject/sof-test

keqiaozhang · 2020-09-23T08:12:38Z

No description provided.

marc-hb

Thanks for a shellcheck-clean PR! Saves a lot of review time.

Any idea about the many check-alsabat-headset-playback.sh failures in https://sof-ci.01.org/softestpr/PR401/build216/devicetest/ ? This doesn't look great in an alsabat PR :-(

I think you can write a much less vague PR title and summary than "refine", just summarize the commit messages.

marc-hb · 2020-09-23T17:42:35Z

+    uname -r |grep "nocodec" >> /dev/null &&
+    echo "nocodec"
+}
+


Avoid strings when they're not needed.

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

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

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

marc-hb · 2020-09-23T17:53:07Z

+        if [ "$codec_mode" != "nocodec" ]; then
+            # USB sound card only supports 1 channel S16_LE format.
+            format_c=S16_LE
+            channel_c=1


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.

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.

Do we real need alsabat for all fmts?

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

Yes，S24_4LE is not supported by alsabat

marc-hb · 2020-09-23T17:56:05Z

+            find /tmp -maxdepth 1 -type f -name "bat.wav.*" -size +0 -exec cp {} "$LOG_ROOT/" \;
+            exit 1
+        }
+        timeout 5 tail --pid=$alsabat_P -f /dev/null || die "Failed to stop alsabat playback"


The timeout tail trick seems to work but it's not intuitive to use the tail command without using its main feature. We already discussed an alternative in this other review: #316 (comment)

The "Failed to stop alsabat" message is misleading, it sounds like kill failed. You mean: "alsabat -P failed to stop"

marc-hb · 2020-09-23T17:57:32Z

+
+        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" || {
+            find /tmp -maxdepth 1 -type f -name "bat.wav.*" -size +0 -exec cp {} "$LOG_ROOT/" \;


Sometimes it's good to have a function with a single command only to give a better and clearer name to that command. I think "upload_wav_file" was a very readable name.

PS: same with once-off constants: inlining them makes the code shorter but not always easier to read. It depends.

You probably want :

alsabat ... || { find ... || true exit 1 }

... because of set -e. When find fails, it fails with an undocumented return value (!) which could cause serious issues like #385

Good suggestion, I have added a new function in lib.sh to upload wav files.

xiulipan · 2020-09-24T07:58:31Z

+        if [ "$codec_mode" != "nocodec" ]; then
+            # USB sound card only supports 1 channel S16_LE format.
+            format_c=S16_LE
+            channel_c=1


Do we real need alsabat for all fmts?

marc-hb · 2020-09-25T00:28:19Z

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


The return 0 will become a no-op when this script is converted to set -e.

I recommend you remove it and you invoke the function like this below:

func_upload_wav_file "/tmp" "bat.wav.*" || true

This gives the caller of the function the choice.

(Cc: @RuiqingX )

marc-hb · 2020-09-25T00:37:30Z

+            func_upload_wav_file "/tmp" "bat.wav.*"
+            exit 1
+        }
+        wait $alsabatPID || die "alsabat -P failed to stop"


It's a bit more complicated, sorry I should have spotted this the first time. To be correct this requires two different error messages. Something like this:

wait $alsabatPID && ret=$? if [ $ret = 124 ]; then die "alsabat -P failed to stop - we timed it out" elif [ $ret != 0 ] ; then die "alsabat -P failed, returned $ret" fi

Untested code, please double-check in man timeout and test by this code by temporarily setting a very short timeout or wrong parameters.

Right, if the command times out, and --preserve-status is not set, then exit with status 124.

Untested code, please double-check in man timeout and test by this code by temporarily setting a very short timeout or wrong parameters.

With a clearer, Monday morning brain I see the untested code I gave you was wrong: ret never gets initialized when wait fails because of short circuit evaluation. I'm afraid this means you didn't inject any one-line error in the alsabat command, didn't test it locally and never saw these messages printed. This is worrying.

@marc-hb , I did performed the negative tests to verify this part of code, tried with wrong channel count "3" in alsabat command and returned 1. if alsabat -P does not stop within the specified time, it will return 124. so which scenario you are talking above?

I mean this:

ret=0 (exit 124) && ret=$? echo $ret => 0

I think we are overaction here. just put the alsabat -P in background and use wait to check the exit code, if exit code is non-zero， then exit with "alsabat -P failure".

AFAIK @keqiaozhang didn't recommend to remove the timeout command, he only recommended to handle timeout errors exactly like non-timeout errors.

Question for people with alsabat experience: can alsabat get stuck when something goes wrong?

marc-hb · 2020-09-25T00:37:39Z

+        wait $alsabatPID || die "alsabat -P failed to stop"
+done
+
+exit 0


missing newline. There's probably a setting in your editor that you can change to avoid this.

marc-hb · 2020-09-25T00:40:51Z

-[[ $(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"


It's not something you're changing but it looks like this code was wrong. Can you take the opportunity to try to fix it and change it to:

aplay -Dplug"$pcm_p" -d 1 /dev/zero -q || die "Failed to play on PCM: $pcm_p"

You are right, die will never be executed.

plbossart · 2020-10-22T20:22:30Z

-[[ $(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"


this looks like an unrelated cleanup?

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

i have separated it as a new commit.

plbossart · 2020-10-22T20:23:19Z

+        else
+            # USB sound card only supports 1 channel S16_LE format.
+            format_c=S16_LE
+            channel_c=1


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

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

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

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.

plbossart · 2020-10-22T20:23:44Z

+            exit 1
+        }
+        wait $alsabatPID && ret=$?
+        if [ $ret == 124 ]; then


what the hell does 124 mean?

The meaning of 124 is (again) in the man timeout page, in a previous review command and much better it is on the very next line: "alsabat -P failed to stop, we timed it out."

plbossart · 2020-10-22T20:25:37Z

+        fi
+
+        dlogc "alsabat -P $pcm_p --standalone -n $frames -F $frequency -c $channel -r $rate -f $format"
+        timeout -k 2 6 alsabat -P "$pcm_p" --standalone -n "$frames" -F "$frequency" -c "$channel" \


what does 2 6 mean? seriously add comments on what this does.

The meanings of 2 and 6 are at the top of the man timeout page. I don't think we need to copy/paste into this script the start of the man page of every unusual command but maybe we can have the --long options like this:

timeout --kill-after 2 6 alsabat ...

plbossart · 2020-10-22T20:26:54Z

+        dlogc "alsabat -P $pcm_p --standalone -n $frames -F $frequency -c $channel -r $rate -f $format"
+        timeout -k 2 6 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.


marc-hb

Still concerned about the lack of negative wait testing mentioned at
#401 (comment)

marc-hb · 2020-10-22T22:08:07Z

+            exit 1
+        }
+        wait $alsabatPID && ret=$?
+        if [ $ret == 124 ]; then


The meaning of 124 is (again) in the man timeout page, in a previous review command and much better it is on the very next line: "alsabat -P failed to stop, we timed it out."

marc-hb · 2020-10-22T22:11:06Z

+        fi
+
+        dlogc "alsabat -P $pcm_p --standalone -n $frames -F $frequency -c $channel -r $rate -f $format"
+        timeout -k 2 6 alsabat -P "$pcm_p" --standalone -n "$frames" -F "$frequency" -c "$channel" \


The meanings of 2 and 6 are at the top of the man timeout page. I don't think we need to copy/paste into this script the start of the man page of every unusual command but maybe we can have the --long options like this:

timeout --kill-after 2 6 alsabat ...

marc-hb · 2020-10-22T22:16:24Z

-[[ $(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"


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

add 2 functions to lib.sh. The first is to check whether sof is running in nocodec mode. The second is to add a function to upload wav files. Signed-off-by: Zhang Keqiao <keqiao.zhang@linux.intel.com>

remove use of plughw to run alsabat test at hw level driectly without any conversions done by alsa-lib and refine the script. Signed-off-by: Zhang Keqiao <keqiao.zhang@linux.intel.com>

Use double quote to prevent globbing and fix the logic issue or die will never be executed. Signed-off-by: Zhang Keqiao <keqiao.zhang@linux.intel.com>

xiulipan

Some update is needed. Please check

xiulipan · 2020-10-26T08:43:20Z

    return 0
 }

+func_nocodec_mode()


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

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.

xiulipan · 2020-10-26T08:46:32Z

+    uname -r |grep "nocodec" >> /dev/null &&
+    echo "nocodec"
+}
+


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

marc-hb · 2020-10-26T20:02:40Z

    return 0
 }

+func_nocodec_mode()


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.

marc-hb · 2020-10-26T20:05:16Z

+func_upload_wav_file()
+{
+    local dir=$1
+    local file=$2


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

marc-hb · 2020-10-26T20:06:01Z

 	exit 2
 fi

+pcmid_p=${pcm_p:-1}


Same as above, please double quote for consistency.

marc-hb · 2020-10-26T20:07:36Z

+        # playback may have low latency, add one second delay to aviod recording zero at beginning.
+        sleep 1
+
+        if func_nocodec_mode; then


if nocodec_mode; then would read like plain English.

marc-hb · 2020-10-26T20:09:49Z

+        }
+        # check the alsabat -P exit code
+        if ! wait $alsabatPID; then
+            die "alsabat -P failure"


No need for the ! negation

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

marc-hb · 2020-10-26T20:14:34Z

+            func_upload_wav_file "/tmp" "bat.wav.*"
+            exit 1
+        }
+        wait $alsabatPID || die "alsabat -P failed to stop"


AFAIK @keqiaozhang didn't recommend to remove the timeout command, he only recommended to handle timeout errors exactly like non-timeout errors.

Question for people with alsabat experience: can alsabat get stuck when something goes wrong?

xiulipan

@keqiaozhang The PR is almost good, please fix some typo and address the advice to use a variable instead of call func_nocodec_mode multiple times

xiulipan · 2020-12-03T04:15:56Z

@keqiaozhang Ping for update or close.

keqiaozhang · 2020-12-03T07:36:10Z

@keqiaozhang Ping for update or close.

I just received the new USB card, will try with the new card and refine this patch soon.

keqiaozhang · 2020-12-14T07:58:24Z

Submitted a new one #554.

keqiaozhang requested a review from a team as a code owner September 23, 2020 08:12

marc-hb requested changes Sep 23, 2020

View reviewed changes

keqiaozhang force-pushed the alsabat_refine branch from 16c910f to cfcf979 Compare September 24, 2020 07:14

keqiaozhang mentioned this pull request Sep 24, 2020

check-alsabat.sh: remove use of plughw #241

Closed

xiulipan suggested changes Sep 24, 2020

View reviewed changes

marc-hb requested changes Sep 25, 2020

View reviewed changes

keqiaozhang force-pushed the alsabat_refine branch 2 times, most recently from 3350b5d to 5a04258 Compare September 27, 2020 06:43

mengdonglin mentioned this pull request Sep 28, 2020

[BUG] sof-logger resisted pkill -INT, using -KILL #385

Closed

plbossart requested changes Oct 22, 2020

View reviewed changes

marc-hb requested changes Oct 22, 2020

View reviewed changes

case-lib: add nocodec mode check and wav file upload functions

07301a0

add 2 functions to lib.sh. The first is to check whether sof is running in nocodec mode. The second is to add a function to upload wav files. Signed-off-by: Zhang Keqiao <keqiao.zhang@linux.intel.com>

keqiaozhang force-pushed the alsabat_refine branch from 5a04258 to b2313f2 Compare October 26, 2020 08:25

keqiaozhang added 2 commits October 26, 2020 16:28

test-case: remove plughw and refine check-alsabat.sh

7b7199a

remove use of plughw to run alsabat test at hw level driectly without any conversions done by alsa-lib and refine the script. Signed-off-by: Zhang Keqiao <keqiao.zhang@linux.intel.com>

test-case: fix the shellcheck warning and Logic issue

b2313f2

Use double quote to prevent globbing and fix the logic issue or die will never be executed. Signed-off-by: Zhang Keqiao <keqiao.zhang@linux.intel.com>

xiulipan suggested changes Oct 26, 2020

View reviewed changes

marc-hb reviewed Oct 26, 2020

View reviewed changes

xiulipan suggested changes Oct 27, 2020

View reviewed changes

keqiaozhang closed this Dec 14, 2020

Conversation

keqiaozhang commented Sep 23, 2020

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb Sep 28, 2020 •

edited

Loading

marc-hb Oct 9, 2020 •

edited

Loading

marc-hb Oct 22, 2020 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb Oct 22, 2020 •

edited

Loading