TinyAlsa: Add support for testing using tinycap and tinyplay. #1260

gbarkadiusz · 2025-03-10T13:46:57Z

Added option SOF_ALSA_TOOL to specify whether to use the TinyASLA or ALSA testing tool. For now, it is only for capturing and playback.
The default tool is ALSA.

This PR introduces also a generic wrapper tinyalsa-wrapper.sh to define SOF_ALSA_TOOL in environments where setting environment variables directly is not possible. This is useful for frameworks or systems with limited configuration capabilities.

test-case/check-playback.sh

marc-hb

This is not scalable at all. It's adding support in only 2 tests and there's already a lot of copy/paste/diverge.

The non-duplicated way is to make aplay_opts into a smarter function that implements all the logic in only one place.

case-lib/lib.sh

marc-hb · 2025-03-18T15:44:09Z

Avoid "spamming" the PR with force-pushes as this brings down limited hardware resources.

test-case/check-capture.sh

golowanow · 2025-03-18T20:29:02Z

Avoid "spamming" the PR with force-pushes as this brings down limited hardware resources.

also, shellcheck can be run locally, before push to PR.

marc-hb · 2025-03-18T21:41:09Z

Don't forget the -x option, as in: shellcheck -x somefile.sh, otherwise you get a lot of false positives. I wonder why -x is not ON by default.

If you forget which option it is, just look in either the CI logs or in git grep shellcheck -- .github/ and run the same. True not just with shellcheck. That's one big advantage of GitHub actions over Jenkins and similar: no "CI secret".

(there can also be drawbacks having everything in the same git repo, I digress)

gbarkadiusz · 2025-03-19T12:30:24Z

@marc-hb @golowanow I've changed the approach according to your suggestions. Now, only the lib.sh has been changed. To force the usage of tinyalsa as the testing tool, the testcase should be run with SOF_TINY_ALSA=true before the testcase.
e.g. SOF_TINY_ALSA=true ./test-case/check-playback.sh
Moreover, I've added the declaration of ALSA-related variables to lib.sh to avoid reference before assignment, as @redzynix mentioned.

case-lib/lib.sh

marc-hb

Just a few minor issues left, otherwise looks good to me. Thanks for your persistence!

case-lib/lib.sh

test-case/check-capture.sh

test-case/check-playback.sh

marc-hb · 2025-03-21T16:29:56Z

case-lib/lib.sh

+   pcm=$(func_pipeline_parse_value "$idx" pcm)
+   # shellcheck disable=SC2034
+   type=$(func_pipeline_parse_value "$idx" type)
+   # shellcheck disable=SC2034


I don't get this warning, are you sure?

If you are sure:

add it only once right before the function and it will apply to whole function. No need to duplicate.

Explain very briefly why it's disabled briefly before it. No one knows what SC2034 is by heart.

SC2034 (warning): variable appears unused. Verify use (or export if used externally).

case-lib/lib.sh

marc-hb · 2025-05-12T17:02:53Z

@marc-hb, this way, as a wrapper script, it allows for both alsa and tinyalsa test case flavors to coexist in our current CI environment's configuration.

I still don't get it sorry. Anywhere you write:

check-tinyalsa.sh sometest.sh test args

You can write instead:

SOF_ALSA_TOOL=tinyalsa sometest.sh test args

... and the effect is the same - but with fewer indirections and complexity.

(The environment variable can also be used in other ways that the wrapper can't do)

marc-hb

I still don't get it sorry.

Thanks @golowanow for the private discussion and explanation. Trying to summarize it here:

sof-test has always been invoked by an internal, higher level, Intel test framework that manages test plans and test runs. This framework has some shortcomings, probably the most important (and somewhat relevant here) being sof-framework/issues/602.

That framework does NOT allow environment variables when defining test plans. The interface only lets the user input a test script and script arguments. So, this wrapper script is a way to disguise and shoehorn an environment variable as a test script, sneaking the real test script as a parameter.

Considering this situation, I cannot think of any better to achieve what this wrapper does. BUT, I think the actual purpose of this script should be much clearer. So interactive users and better test frameworks do NOT start using it when they do not need to.

The wrapper should not be called check-tinyalsa.sh which sounds like something good. It should be called something like tinyalsa-wrapper.sh.
The comments at the top of the wrapper should clearly state something like "This is a wrapper that allows to define SOF_ALSA_TOOL in frameworks that cannot define environment variables. Do not use this script if you can just define SOF_ALSA_TOOL.
The "checking bits" are useful and a very different feature! They should be in a separate "check-tinyalsa.sh` script that performs ONLY the checks and that everyone can use. The former script can call the latter script.

gbarkadiusz · 2025-05-13T10:43:14Z

The "checking bits" are useful and a very different feature! They should be in a separate "check-tinyalsa.sh` script that performs ONLY the checks and that everyone can use. The former script can call the latter script.

I've moved this part to env-check script.

Done

README.md

case-lib/lib.sh

gbarkadiusz · 2025-05-14T06:34:07Z

@marc-hb Please take a look.

marc-hb

I keep finding issues sorry. I don't know which it is:

I didn't see them before
The code has changed
I reported them before but my comments got "lost"

Nothing major.

marc-hb · 2025-05-14T07:08:56Z

case-lib/lib.sh

+# While using TinyALSA (tinycap -b) we need to convert PCM sample fomrat to bits.
+extract_format_number() {
+    # (e.g., extracting '16' from 'S16_LE')
+    format=$(printf '%s' "$1" | grep '[0-9]\+' -o)


format is a very common name for a global variable.

This function returns a single value so it could just print it instead and avoid a global?

marc-hb · 2025-05-14T07:10:11Z

case-lib/lib.sh

+    dev=$(func_pipeline_parse_value "$idx" dev)
+    pcm=$(func_pipeline_parse_value "$idx" pcm)
+    type=$(func_pipeline_parse_value "$idx" type)
+    snd=$(func_pipeline_parse_value "$idx" snd)


That's a lot of global variables with very short names...

I know, but I only moved this section from:
https://github.com/thesofproject/sof-test/pull/1260/files#diff-ac523ac7afa0a9a96eb0ee6c918c10f317d1b201b7124bf10bcf98b611b7324dL78
https://github.com/thesofproject/sof-test/pull/1260/files#diff-4c371b42f9f03814333bc7e33dce34faf542f27087fcf0b5702029f55f630edbL71
to an additional function in lib.sh file.

marc-hb · 2025-05-14T07:10:38Z

case-lib/lib.sh

-    aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS "$@"
+    if [[ "$SOF_ALSA_TOOL" = "tinyalsa" ]]; then
+        # shellcheck disable=SC2154
+	if ! sox -n -r "$rate" -c "$channel" noise.wav synth "$duration" white; then


Indentation? Did you use tabs? I think Github does not like them (I don't either)

True, Fixed.

marc-hb · 2025-05-14T07:12:07Z

case-lib/lib.sh

+        # shellcheck disable=SC2086
+        aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS "$@"
+    else
+        printf '%s' "Unknown alsa tool: $SOF_ALSA_TOOL "


This should be fatal, using die or similar.

marc-hb · 2025-05-14T07:13:32Z

case-lib/lib.sh

+    if [[ "$SOF_ALSA_TOOL" = "tinyalsa" ]]; then
+        # shellcheck disable=SC2154
+	if ! sox -n -r "$rate" -c "$channel" noise.wav synth "$duration" white; then
+            printf '%s' "Error: sox command failed." >&2


Suggested change

printf '%s' "Error: sox command failed." >&2

printf 'Error: sox command failed.\n' >&2

or

Suggested change

printf '%s' "Error: sox command failed." >&2

dloge 'Error: sox command failed."

marc-hb · 2025-05-14T07:18:12Z

case-lib/lib.sh

+
+    if [[ "$SOF_ALSA_TOOL" = "tinyalsa" ]]; then
+        # shellcheck disable=SC2154
+        extract_format_number "$fmt_elem"


Could you add a comment explaining where $fmt_elem comes from? Shellcheck seems to make a good point here....

More global variables :-(

marc-hb · 2025-05-14T07:19:17Z

case-lib/lib.sh

+        # shellcheck disable=SC2086
+        arecord $SOF_ALSA_OPTS $SOF_ARECORD_OPTS "$@"
+    else
+        printf '%s' "Unknown alsa tool: $SOF_ALSA_TOOL "


Should be fatal using die or similar.

marc-hb · 2025-05-14T07:20:48Z

case-lib/lib.sh

+            fi
+        done
+    elif [[ "$SOF_ALSA_TOOL" = "tinyalsa" ]]; then
+        tinymix -D0 controls | sed -e "s/^.*'\(.*\)'.*/\1/" |grep -E 'PGA|gain' |


Nit: later, it would be nice to de-duplicate the sed+grep bit with a function. No need to do it now.

Add support for tinyALSA and update env-check and README file. Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>

Refactor Check-capture and Check-playback tests. Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>

Add tinyalsa-wrapper used to run any test with tinyAlsa testsets. Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>

gbarkadiusz requested a review from a team as a code owner March 10, 2025 13:46

golowanow reviewed Mar 10, 2025

View reviewed changes

test-case/check-playback.sh Outdated Show resolved Hide resolved

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch 2 times, most recently from f96d4e2 to 9029069 Compare March 10, 2025 14:32

marc-hb reviewed Mar 10, 2025

View reviewed changes

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch from 9029069 to 85a2007 Compare March 18, 2025 08:01

gbarkadiusz requested a review from lgirdwood as a code owner March 18, 2025 08:01

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch 5 times, most recently from 8c7f825 to 1f8a70b Compare March 18, 2025 08:31

redzynix reviewed Mar 18, 2025

View reviewed changes

case-lib/lib.sh Outdated Show resolved Hide resolved

golowanow reviewed Mar 18, 2025

View reviewed changes

test-case/check-capture.sh Outdated Show resolved Hide resolved

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch from 1f8a70b to c36f283 Compare March 19, 2025 12:23

marc-hb reviewed Mar 19, 2025

View reviewed changes

case-lib/lib.sh Outdated Show resolved Hide resolved

case-lib/lib.sh Outdated Show resolved Hide resolved

case-lib/lib.sh Outdated Show resolved Hide resolved

marc-hb mentioned this pull request Mar 19, 2025

Use aplay --fatal-errors to stop on underruns or errors #489

Open

golowanow reviewed Mar 20, 2025

View reviewed changes

case-lib/lib.sh Outdated Show resolved Hide resolved

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch 3 times, most recently from 093579d to 19705a9 Compare March 21, 2025 11:34

gbarkadiusz requested review from golowanow and marc-hb March 21, 2025 11:35

marc-hb reviewed Mar 21, 2025

View reviewed changes

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch 2 times, most recently from 3a4e037 to 72aa384 Compare March 27, 2025 11:48

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch 6 times, most recently from 6cee092 to e122226 Compare May 12, 2025 13:59

marc-hb previously requested changes May 12, 2025

View reviewed changes

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch from e122226 to b228706 Compare May 13, 2025 09:18

gbarkadiusz requested review from golowanow and marc-hb May 13, 2025 09:44

golowanow reviewed May 13, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

case-lib/lib.sh Outdated Show resolved Hide resolved

case-lib/lib.sh Outdated Show resolved Hide resolved

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch 2 times, most recently from 0da0218 to 83aae71 Compare May 14, 2025 06:15

golowanow approved these changes May 14, 2025

View reviewed changes

gbarkadiusz requested a review from redzynix May 14, 2025 06:27

redzynix approved these changes May 14, 2025

View reviewed changes

marc-hb reviewed May 14, 2025

View reviewed changes

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch from 83aae71 to 646120c Compare May 14, 2025 10:36

gbarkadiusz added 3 commits May 14, 2025 12:56

TinyAlsa: Add support for tinyAlsa.

6329537

Add support for tinyALSA and update env-check and README file. Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>

TinyALSA:Refactor check-capture and check-playback

03afd54

Refactor Check-capture and Check-playback tests. Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>

TinyALSA: Add wrapper for tinyALSA's tests

8c585a3

Add tinyalsa-wrapper used to run any test with tinyAlsa testsets. Signed-off-by: Arkadiusz Cholewinski <arkadiuszx.cholewinski@intel.com>

gbarkadiusz force-pushed the topic/add_tinyalsa_support branch from 646120c to 8c585a3 Compare May 14, 2025 10:56

gbarkadiusz requested a review from marc-hb May 14, 2025 11:10

marc-hb approved these changes May 14, 2025

View reviewed changes

golowanow merged commit c7bb56c into thesofproject:main May 14, 2025
3 of 8 checks passed

gbarkadiusz mentioned this pull request May 15, 2025

Fix: Set SOF_ALSA_TOOL to default value globally. #1271

Merged

	printf '%s' "Error: sox command failed." >&2
	printf 'Error: sox command failed.\n' >&2

	printf '%s' "Error: sox command failed." >&2
	dloge 'Error: sox command failed."

TinyAlsa: Add support for testing using tinycap and tinyplay. #1260

TinyAlsa: Add support for testing using tinycap and tinyplay. #1260

Uh oh!

Conversation

gbarkadiusz commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marc-hb commented Mar 18, 2025

Uh oh!

Uh oh!

golowanow commented Mar 18, 2025

Uh oh!

marc-hb commented Mar 18, 2025

Uh oh!

gbarkadiusz commented Mar 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-hb left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbarkadiusz Mar 24, 2025 • 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!

Uh oh!

marc-hb commented May 12, 2025

Uh oh!

marc-hb left a comment • 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!

gbarkadiusz commented May 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gbarkadiusz commented May 14, 2025

Uh oh!

marc-hb left a comment • 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!

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

gbarkadiusz commented Mar 10, 2025 •

edited

Loading

marc-hb left a comment •

edited

Loading

gbarkadiusz Mar 24, 2025 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb left a comment •

edited

Loading