Testbench: Add different in and out channels count and improve shell helper scripts for components #5152

singalsu · 2022-01-05T17:14:30Z

No description provided.

singalsu · 2022-01-05T17:17:51Z

@marc-hb What do you think of the component run scripts? I will do later a shellcheck fix round for details but does the overall approach look sane to you.

I did this work to help make more advanced automatic tests for beamformer and sound direction tracking. The test script (not yet this patch) captures debug trace output from testbench and parses and checks some internal state variables that way during the test run.

marc-hb

Too much code and layers of indirections. Many of the top-level scripts are just configuration files in disguise. The double level of getopt is especially hurting.

My recommendation: add a new ./comp_run.sh --test_config option and get rid of most top level scripts. Keep it simple: have ./comp_run.sh merely source the config file which defines a bunch of test parameters as shell variables.

scripts/host-testbench.sh

tools/test/audio/asrc_run.sh

scripts/host-testbench.sh

marc-hb · 2022-01-10T20:17:08Z

@marc-hb proposed that this PR would be min changes only to support the additional testbench command line parameters. So this script would not change at all.

IMHO it's more effective to submit small, less controversial PRs very quick and easy to review - while still having a clear long term direction in mind. This also provides much more test coverage, easier bisects, etc. I think it's called "Continuous Integration" ;-)

tools/test/audio/test_utils/test_run.m

marc-hb

Nothing standing out as fundamentally wrong in the shell script code but requesting changes because of the sheer number of recommendations.

tools/test/audio/comp_run.sh

marc-hb · 2022-01-11T17:35:51Z

tools/test/audio/comp_run.sh

+	    exit 1
+	    ;;
+    esac
+done


Please move most of this new code to function(s). Rationales: thesofproject/sof-test#740

Here: parse_args()

Don't move the existing code to functions, I mean not in this PR but at least most of the new code.

I did that, hope it's like you had in mind.

marc-hb · 2022-01-11T17:41:36Z

tools/test/audio/comp_run.sh

+    if [ -z "$FN_TRACE" ]; then
+	$VALGRIND_CMD $CMD
+    else
+	$VALGRIND_CMD $CMD 2> "$FN_TRACE"


Have you considered valgrind --log-file? It seems messy to mix valgrind with test output and it's also not clear which one uses stderr and when. It can be convenient in interactive use of course but FN_TRACE looks like the opposite of interactive use.

Not yet done, valgrind outputs into beginning and end, with experience it's easy to spot which is it. But obviously it's not the case for occasional users. The testbench output is quite a mess today, some trace and markup language prints appear mixed. I don't know who needs the markup. It would need a big cleanup with redirect of everything with simulated time stamps.

marc-hb · 2022-01-11T17:44:18Z

tools/test/audio/comp_run.sh

+    echo "FS_OUT=48000"
+    echo "FN_IN=input.raw"
+    echo "FN_OUT=output.raw"
+    echo "FN_TRACE=trace.txt"


Should FN_TRACE really be part of the test config? That sounds too inflexible, I mean it should be possible and even easy to direct the output of any test config to wherever. Either with a flag or global variable or a mix of both. You can have both like this:

# Default value if not already set (and if you want a non-empty default) : ${FN_TRACE:=trace.txt} getopt ... # command line takes precedence over env FN_TRACE=OPTARG

https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

https://github.com/thesofproject/sof-test/blob/967b7bf485a3586318f4acfa45159cc2055423cb/test-case/run-all-tests.sh#L82

The latter is another, actually relatively similar test script. It's far from perfect but worth taking a look at considering the similarities.

OK I'll make a try. The config above is just an example. For interactive usage the config file could be smaller or omitted. Trace is in testbench is just stderr so normal output redirect works.

Done, sort of.

marc-hb · 2022-01-11T17:52:45Z

tools/test/audio/comp_run.sh

+
+shift $((OPTIND -1))
+
+if [[ -z $SOURCED_CONFIG ]]; then


Initialize your booleans to either the true or false strings. Then you can use them in this very terse and readable:

if $SOURCED_CONFIG; then ....

https://github.com/thesofproject/sof/blob/71cd9666a95cc/scripts/build-tools.sh#L104

Caveat: they MUST always be defined to either true or false and inside the script itself (cause don't trust user input). But in this SOURCED_CONFIG case (and probably others) you are already doing that.

You can do the same with not just "true" and "false" but any command or function BTW, including your own functions. Example at https://github.com/thesofproject/sof-test/blob/967b7bf485a3586/test-case/run-all-tests.sh#L126

Most people don't realize this but shell scripting is a "functional" language.

Done for SOURCED_CONFIG and VALGRIND.

marc-hb · 2022-01-11T18:03:00Z

tools/test/audio/comp_run.sh

+shift $((OPTIND -1))
+
+if [[ -z $SOURCED_CONFIG ]]; then
+    if [ $# -ne 8 ]; then


"do || die" / assert style:

Suggested change

if [ $# -ne 8 ]; then

[ -n "$SOURCED_CONFIG" ] || [ $# -eq 8 ] || {

usage

exit 1

}

This maybe works, I haven't tested with Matlab tests, but I don't think it improves readability:

if ! "$SOURCED_CONFIG"; then [ $# -eq 8 ] || { usage "$0" exit 1 } COMP=$1 DIRECTION=$2 BITS_IN=$3 BITS_OUT=$4 FS_IN=$5 FS_OUT=$6 FN_IN=$7 FN_OUT=$8 fi

As above now, but I guess it's not what you were suggesting.

marc-hb · 2022-01-11T18:09:04Z

tools/test/audio/comp_run.sh

-valgrind --leak-check=yes --error-exitcode=1 $CMD
+rm -f "$FN_OUT"
+rm -f "$FN_TRACE"
+if [ "$VALGRIND" = "yes" ]; then


You don't need the if VALGRIND test.

If you drop the 2> redirection and switch to --log-file or something, then you don't need the if FN_TRACE either.

You can run this same command with $MAYBE_VALGRIND maybe empty maybe not and it will Just Work:

$MAYBE_VALGRIND $CMD

Again $MAYBE_REDIRECT does not work, you need the if for conditional redirections.

(I'm not suggesting to actually prefix the variable with MAYBE_)

LOL -- I might have used maybe :) Good recommendations, thanks!

I might have used maybe :)

Maybe it's better :-)

Done with $VALGRIND_CMD $CMD, no --log-file yet.

singalsu · 2022-01-18T15:18:21Z

I've not yet addressed the comments. The previous push added enable of verbose traces for library build for testbench. The extra_opts handling is fixed.

marc-hb

Besides some missing quotes (please run shellcheck) the shell script changes look good to me.

I cannot review the matlab and C code unfortunately, not familiar enough with them.

marc-hb · 2022-01-19T19:43:24Z

tools/test/audio/comp_run.sh

+	FS_IN=$5
+	FS_OUT=$6
+	FN_IN=$7
+	FN_OUT=$8


shellcheck warnings?

Yes, shellcheck thinks I should use $VALGRIND_CMD "$CMD". But in that case valgrind things the whole testbench command is the binary to run. Do you ideas how to do this fix in way that still works?

Yes, shellcheck thinks I should use $VALGRIND_CMD "$CMD".

"$CMD" is the tricky one, I wasn't referring to this one. I was only commenting about the easy ones like: FN_OUT="$8".

There are two ways to deal with the $CMD quoting problem:

Add a # shellcheck disable=SC1234 silencer comment that states "thank you shellcheck but I know what I'm doing"

Replace $CMD with a bash array "${CMD[@]}" https://mywiki.wooledge.org/BashGuide/Arrays

Do nothing and do not achieve shellcheck perfection. The fewer warnings the better but does not have to be zero.

Shellcheck didn't complain about likes of FN_OUT=$8. OK, I'll fix those.

This patch adds to command line switch -n for output channels count. Existing -c is for in channels, new -n is for out channels count. Out channels count is same as input if -n is not present. Switch -q is added to quiet the trace output if it is not needed. The Matlab language test scripts for components are updated to use the -t config.sh interface of comp_run.sh. It allows more flexible control of input and output streams. Signed-off-by: Seppo Ingalsuo <[email protected]>

The path append of current directory (.) is not correct for the signal processing package function iirnotch. The issue does not happen with Octave since it has pei_tseng_notch(). The standard notch function is used in all THD+N tests so this issue has caused fail of nearly all tests with Matlab. Signed-off-by: Seppo Ingalsuo <[email protected]>

Some useful algorithms internal traces are output with debug traces. This patch enables the verbose trace (CONFIG_TRACEV) for library build. Signed-off-by: Seppo Ingalsuo <[email protected]>

lgirdwood

Reviewed the C parts. @ShriramShastry can you review the matlab. Thanks

marc-hb

Shellcheck didn't complain about likes of FN_OUT=$8. OK, I'll fix those.

Sorry I forgot the first exception in https://mywiki.wooledge.org/Quotes#When_Should_You_Quote.3F
(assignments)

Instead I follow this rule:
https://mywiki.wooledge.org/Quotes#I.27m_Too_Lazy_to_Read.2C_Just_Tell_Me_What_to_Do

When in doubt, double-quote every expansion in your shell commands.

marc-hb · 2022-01-21T23:03:38Z

tools/test/audio/comp_run.sh

+{
+    cat <<EOFHELP
+Usage:     $0 <options> <comp direction bits_in bits_out fs_in fs_out input output>
+Example 1: $0 volume playback 16 16 48000 48000 input.raw output.raw


Add some -e or other option in this example to make it even clearer that combining flags and positional parameters is OK.

singalsu requested a review from marc-hb January 5, 2022 17:14

marc-hb reviewed Jan 5, 2022

View reviewed changes

scripts/host-testbench.sh Outdated Show resolved Hide resolved

scripts/host-testbench.sh Outdated Show resolved Hide resolved

marc-hb reviewed Jan 6, 2022

View reviewed changes

tools/test/audio/asrc_run.sh Outdated Show resolved Hide resolved

lyakh reviewed Jan 10, 2022

View reviewed changes

scripts/host-testbench.sh Outdated Show resolved Hide resolved

scripts/host-testbench.sh Outdated Show resolved Hide resolved

singalsu mentioned this pull request Jan 10, 2022

Arch: Host: Enable default-no components for testbench build #5173

Merged

singalsu force-pushed the testbench_scripts_improve branch 2 times, most recently from 2e22604 to 5e01616 Compare January 11, 2022 13:50

singalsu requested review from lyakh and marc-hb January 11, 2022 13:54

singalsu marked this pull request as ready for review January 11, 2022 13:54

singalsu requested review from dbaluta, lbetlej, lgirdwood, mmaka1, plbossart and ranj063 as code owners January 11, 2022 13:54

singalsu commented Jan 11, 2022

View reviewed changes

tools/test/audio/test_utils/test_run.m Outdated Show resolved Hide resolved

marc-hb requested changes Jan 11, 2022

View reviewed changes

singalsu force-pushed the testbench_scripts_improve branch from 5e01616 to 963c344 Compare January 18, 2022 15:06

singalsu force-pushed the testbench_scripts_improve branch from 963c344 to abe6db9 Compare January 19, 2022 17:03

singalsu requested a review from marc-hb January 19, 2022 17:14

marc-hb reviewed Jan 19, 2022

View reviewed changes

singalsu added 3 commits January 21, 2022 11:15

Testbench: Enable verbose traces

cdf4e8c

Some useful algorithms internal traces are output with debug traces. This patch enables the verbose trace (CONFIG_TRACEV) for library build. Signed-off-by: Seppo Ingalsuo <[email protected]>

singalsu force-pushed the testbench_scripts_improve branch from abe6db9 to cdf4e8c Compare January 21, 2022 09:17

singalsu requested a review from marc-hb January 21, 2022 09:19

lgirdwood approved these changes Jan 21, 2022

View reviewed changes

marc-hb approved these changes Jan 21, 2022

View reviewed changes

lgirdwood approved these changes Jan 24, 2022

View reviewed changes

lgirdwood merged commit 95d8e94 into thesofproject:main Jan 24, 2022

singalsu deleted the testbench_scripts_improve branch September 16, 2022 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testbench: Add different in and out channels count and improve shell helper scripts for components #5152

Testbench: Add different in and out channels count and improve shell helper scripts for components #5152

singalsu commented Jan 5, 2022

singalsu commented Jan 5, 2022

marc-hb left a comment •

edited

Loading

marc-hb commented Jan 10, 2022

marc-hb left a comment

marc-hb Jan 11, 2022

singalsu Jan 19, 2022

marc-hb Jan 11, 2022

singalsu Jan 19, 2022

marc-hb Jan 11, 2022

singalsu Jan 19, 2022

singalsu Jan 19, 2022

marc-hb Jan 11, 2022

singalsu Jan 19, 2022

marc-hb Jan 11, 2022

singalsu Jan 19, 2022

singalsu Jan 19, 2022

marc-hb Jan 11, 2022 •

edited

Loading

singalsu Jan 12, 2022

marc-hb Jan 12, 2022

singalsu Jan 19, 2022

singalsu commented Jan 18, 2022

marc-hb left a comment

marc-hb Jan 19, 2022

singalsu Jan 20, 2022

marc-hb Jan 20, 2022 •

edited

Loading

singalsu Jan 21, 2022

lgirdwood left a comment

marc-hb left a comment

marc-hb Jan 21, 2022

Testbench: Add different in and out channels count and improve shell helper scripts for components #5152

Testbench: Add different in and out channels count and improve shell helper scripts for components #5152

Conversation

singalsu commented Jan 5, 2022

singalsu commented Jan 5, 2022

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb commented Jan 10, 2022

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Jan 18, 2022

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment •

edited

Loading

marc-hb Jan 11, 2022 •

edited

Loading

marc-hb Jan 20, 2022 •

edited

Loading