test-case: add new test case for check mic privacy by arikgreen · Pull Request #1278 · thesofproject/sof-test

arikgreen · 2025-06-16T13:55:06Z

Add new test case to check hardware mic privacy mode enablement feature.
For switching mic privacy state is using a USB relay.
https://github.com/darrylb123/usbrelay

Copilot

Pull Request Overview

This pull request adds a new test case to verify the mic privacy mode by toggling a USB relay and checking audio behavior using alsabat.

Adds the new test script test-case/test-mic-privacy.sh.
Implements test steps including checking for usbrelay installation, running alsabat tests before and after toggling the mic privacy switch.
Logs relevant parameters and captures output for troubleshooting.

Copilot · 2025-06-16T13:56:38Z

test-case/test-mic-privacy.sh

+if [ $alsabat_status -ne 0 ]; then
+    if grep -q -e "Amplitude: 0.0; Percentage: \[0\]" -e "Return value is -1001" "$alsabat_output"
+    then
+        # Do nothing if signal is zero, this is expected
+        # Return value is -1001
+        dlogi "Alsabat output indicates zero signal as expected."
+        :
+    else
+        dloge "alsabat failed with status $alsabat_status, but signal is not zero."
+        __upload_wav_file
+        dloge "alsabat output: $(cat "$alsabat_output")."
+        exit 1
+    fi
+else
+    dloge "alsabat passed, upload the wav files."
+    dloge "alsabat output: $(cat "$alsabat_output")"
+    exit 1
+fi


[nitpick] Consider extracting the alsabat result check and its error handling logic into a separate function to reduce complexity in the main test flow and improve maintainability.

Suggested change

if [ $alsabat_status -ne 0 ]; then

if grep -q -e "Amplitude: 0.0; Percentage: \[0\]" -e "Return value is -1001" "$alsabat_output"

then

# Do nothing if signal is zero, this is expected

# Return value is -1001

dlogi "Alsabat output indicates zero signal as expected."

:

else

dloge "alsabat failed with status $alsabat_status, but signal is not zero."

__upload_wav_file

dloge "alsabat output: $(cat "$alsabat_output")."

exit 1

fi

else

dloge "alsabat passed, upload the wav files."

dloge "alsabat output: $(cat "$alsabat_output")"

exit 1

fi

handle_alsabat_result "$alsabat_status" "$alsabat_output"

handle_alsabat_result "$alsabat_status" "$alsabat_output"

Copilot · 2025-06-16T13:56:39Z

test-case/test-mic-privacy.sh

+	exit 2
+fi
+
+# check if usbrelay tool is installed
+if ! command -v usbrelay >/dev/null 2>&1; then
+  dloge "usbrelay command not found. Please install usbrelay to control the mic privacy switch."
+  exit 2


[nitpick] Using hard-coded exit codes (such as exit 2) may reduce clarity; consider defining constants or adding inline comments to explain the meaning of these values.

Suggested change

exit 2

fi

# check if usbrelay tool is installed

if ! command -v usbrelay >/dev/null 2>&1; then

dloge "usbrelay command not found. Please install usbrelay to control the mic privacy switch."

exit 2

exit $EXIT_CODE_MISSING_PCM # Exit due to missing playback or capture PCM

fi

# check if usbrelay tool is installed

if ! command -v usbrelay >/dev/null 2>&1; then

dloge "usbrelay command not found. Please install usbrelay to control the mic privacy switch."

exit $EXIT_CODE_MISSING_PCM # Exit due to missing usbrelay command

test-case/test-mic-privacy.sh

golowanow · 2025-06-17T13:39:22Z

test-case/test-mic-privacy.sh

+##    Enable MIC privacy.
+##    Run alsabat process perform both playback and capture again.


I think this test should do two more steps:

Disable MIC privacy

playback/capture to check the MIC privacy mode is disabled

there is this check before runs test

test-case/test-mic-privacy.sh

golowanow · 2025-06-17T14:06:23Z

test-case/test-mic-privacy.sh

+fi
+
+dlogi "Turn off the mic privacy switch"
+usbrelay HURTM_1=0


is a check possible on success for relay switch on/off ?

yes, it is. implemented in the function usbrelay_switch in lib.sh

test-case/test-mic-privacy.sh

golowanow · 2025-06-17T14:19:28Z

test-case/test-mic-privacy.sh

+alsabat -P"$pcm_p" -C"$pcm_c" -c 2 -r $rate || {
+    # upload failed wav file
+    __upload_wav_file
+    exit 1


Suggested change

exit 1

skip_test "No audio loopback connected."

test-case/test-mic-privacy.sh

marc-hb · 2025-06-25T14:49:08Z

test-case/test-mic-privacy.sh

+
+    if [ "$pcm_p" = "" ]||[ "$pcm_c" = "" ]; then
+        dloge "No playback or capture PCM is specified. Skip the alsabat test."
+        exit 2


There is a skip_test 'reason' function, please use it (AI was right!)

marc-hb · 2025-06-25T14:49:49Z

test-case/test-mic-privacy.sh

+    done
+}
+
+function check_playback_capture


Suggested change

function check_playback_capture

check_playback_capture

Unnecessary bashism.

marc-hb · 2025-06-25T14:50:13Z

case-lib/lib.sh

+# needs usbrelay package: https://github.com/darrylb123/usbrelay
+# param1: switch name
+# param2: switch state
+usbrelay_switch() {


Consistency nit

Suggested change

usbrelay_switch() {

usbrelay_switch()

{

marc-hb · 2025-06-25T14:50:33Z

case-lib/lib.sh

+# param2: switch state
+usbrelay_switch() {
+    switch_name=$1
+    state=$2


Suggested change

state=$2

local state=$2

marc-hb · 2025-06-25T14:51:19Z

case-lib/lib.sh

+    switch_name=$1
+    state=$2
+    dlogi "Setting usbrelay switch $switch_name to $state."
+    if ! usbrelay "$switch_name=$state" >/dev/null 2>&1; then


You can easily avoid negations

Suggested change

if ! usbrelay "$switch_name=$state" >/dev/null 2>&1; then

usbrelay "$switch_name=$state" >/dev/null 2>&1 || {

Reads as plain english "do or skip"

marc-hb · 2025-06-25T14:52:01Z

case-lib/lib.sh

+    current_state=$(usbrelay | grep "$switch_name" | awk -F= '{print $2}')
+
+    # Check if current_state is equal to the requested state
+    if [[ "$current_state" != "$state" ]]; then


Nit

Suggested change

if [[ "$current_state" != "$state" ]]; then

[[ "$current_state" == "$state" ]] || {

Reads as "assert or die"

marc-hb · 2025-06-25T14:52:51Z

test-case/test-mic-privacy.sh

+        set_alsa_settings "$MODEL"
+    fi
+
+    logger_disabled || func_lib_start_log_collect


That's coming surprisingly late... are you sure?

yes, you'll might right. move it upper.

arikgreen · 2025-06-25T16:52:49Z

@golowanow & @marc-hb thanks for review. Many valuable comments.

case-lib/lib.sh

golowanow · 2025-07-08T20:08:41Z

case-lib/lib.sh

+    local switch_name=$1
+    local state=$2
+    dlogi "Setting usbrelay switch $switch_name to $state."
+    usbrelay "$switch_name=$state" >/dev/null 2>&1 || {


do you think usbrelay-s output with possible errors is not useful for diagnostics and troubleshooting ?

Suggested change

usbrelay "$switch_name=$state" >/dev/null 2>&1 || {

usbrelay "$switch_name=$state" 2>&1 || {

Discarding stderr is wrong 99.999% of the time. The remaining 0.001% requires a comment with a very strong rationale.

That's because 99.999% of the time, developers use stderr because they think the error is so bad that the message should never be discarded.

Discarding stderr even more wrong in TEST code! It's a really bad code smell.

case-lib/lib.sh

golowanow · 2025-07-08T20:22:02Z

case-lib/lib.sh

+    if [[ "$current_state" == "1" ]]; then
+        dlogi "Current state of $switch_name is: on"
+    elif [[ "$current_state" == "0" ]]; then
+        dlogi "Current state of $switch_name is: off"
+    else
+        dloge "Invalid state for $switch_name: $current_state"
+        exit 1
+    fi


it seems this if/elif/else chunk should be before $current_state == $state comparison, or this function should reject the incorrect $state parameter early at the entry.

I think the most important is state of $current_state and it should be equal expected $state, so we should verify it.
If the $state will not be 0 || 1 then the command usbrelay "$switch_name=$state" returns error.
IMO moving the chunk before $current_state == $state doesn't matter.

test-case/test-mic-privacy.sh

golowanow · 2025-07-08T20:36:57Z

test-case/test-mic-privacy.sh

+    fi
+
+    # check if usbrelay tool is installed
+    command -v usbrelay >/dev/null 2>&1 || {


Suggested change

command -v usbrelay >/dev/null 2>&1 || {

command -v usbrelay 2>&1 || {

case-lib/lib.sh

golowanow · 2025-07-08T20:47:08Z

test-case/test-mic-privacy.sh

+    dlogi "Turn on the mic privacy switch"
+    usbrelay_switch "$relay" 1
+
+    alsabat_output=$(mktemp)


mktemp needs the same explicit template name here, isn't it ?

no needs the explicit template name, without template the cmd return /tmp/tmp.xxxxxx

marc-hb · 2025-07-14T21:36:19Z

case-lib/relay.sh

+    elif [[ "$current_state" == "0" ]]; then
+        dlogi "Current state of $switch_name is: off"
+    else
+        die "Invalid state for $switch_name: $current_state"


case ... in works great for this. Find one example (among many others) at the bottom of 41c1427

marc-hb · 2025-07-14T21:37:02Z

test-case/test-mic-privacy.sh

+
+    logger_disabled || func_lib_start_log_collect
+
+    if [ "$pcm_p" = "" ]||[ "$pcm_c" = "" ]; then


Suggested change

if [ "$pcm_p" = "" ]||[ "$pcm_c" = "" ]; then

if [ -z "$pcm_p" ] || [ -z "$pcm_c" ]; then

marc-hb · 2025-07-14T21:38:02Z

test-case/test-mic-privacy.sh

+    logger_disabled || func_lib_start_log_collect
+
+    if [ "$pcm_p" = "" ]||[ "$pcm_c" = "" ]; then
+        skip_test "No playback or capture PCM is specified. Skip the alsabat test."


Suggested change

skip_test "No playback or capture PCM is specified. Skip the alsabat test."

skip_test "No playback or capture PCM is specified. Skip the $0 test."

There is already some test-case/alsabat.sh somewhere.

marc-hb · 2025-07-14T21:39:42Z

test-case/test-mic-privacy.sh

+    alsabat -P"$pcm_p" -C"$pcm_c" -c 2 -r "$rate" >"$alsabat_output" 2>&1
+    alsabat_status=$?
+
+    if [ $alsabat_status -ne 0 ]; then


Prefer the "early return" pattern

Suggested change

if [ $alsabat_status -ne 0 ]; then

alsabat -P"$pcm_p" -C"$pcm_c" -c 2 -r "$rate" >"$alsabat_output" 2>&1 || {

dloge "alsabat passed unexpectedly, upload the wav files."

__upload_wav_files

die "MIC privacy doesn't work. alsabat output: $(cat "$alsabat_output")"

}

test-case/test-mic-privacy.sh

golowanow · 2025-08-01T19:31:22Z

case-lib/relay.sh

+    local state=$2
+
+    dlogi "Setting usbrelay switch $switch_name to $state."
+    usbrelay "$switch_name=$state" > /dev/null || {


Suggested change

usbrelay "$switch_name=$state" > /dev/null || {

usbrelay "$switch_name=$state" || {

as already discussed #1278 (comment)

The command returns a previous state of relays this could be confusing and imo isn't usable..

golowanow · 2025-08-01T19:34:41Z

case-lib/relay.sh

+
+    dlogi "Setting usbrelay switch $switch_name to $state."
+    usbrelay "$switch_name=$state" > /dev/null || {
+        # if not detect relays hw module, skip the test


Suggested change

# if not detect relays hw module, skip the test

The error message explains already.

golowanow · 2025-08-01T19:41:40Z

test-case/test-mic-privacy.sh

+ALSABAT_WAV_FILES="/tmp/mc.wav.*"
+rm -f "$ALSABAT_WAV_FILES"
+
+TWO_SECONDS=2


does it serve the same purpose as USBRELAY_SETTLE_TIME=0.5s ?
or is it a real USBRELAY_SETTLE_TIME ?

anyway, better to name this variable differently than TWO_SECONDS, just in case someone needs to increase it to 3 seconds

yes, might you right

golowanow · 2025-08-01T19:45:39Z

test-case/test-mic-privacy.sh

+    alsabat -P"$pcm_p" -C"$pcm_c" -c 2 -r "$rate" || {
+        # upload failed wav file
+        __upload_wav_files
+        die "alsabat failed"


Suggested change

die "alsabat failed"

die "check_playback_capture() failed: no loopback connected ?"

golowanow · 2025-08-01T19:50:13Z

test-case/test-mic-privacy.sh

+    # reset sof volume to 0dB
+    reset_sof_volume
+
+    # If MODEL is defined, set proper gain for the platform
+    if [ -z "$MODEL" ]; then
+        # treat as warning only
+        dlogw "NO MODEL is defined. Please define MODEL to run alsa_settings/MODEL.sh"
+    else
+        dlogi "apply alsa settings for alsa_settings/MODEL.sh"
+        set_alsa_settings "$MODEL"
+    fi


Suggested change

# reset sof volume to 0dB

reset_sof_volume

# If MODEL is defined, set proper gain for the platform

if [ -z "$MODEL" ]; then

# treat as warning only

dlogw "NO MODEL is defined. Please define MODEL to run alsa_settings/MODEL.sh"

else

dlogi "apply alsa settings for alsa_settings/MODEL.sh"

set_alsa_settings "$MODEL"

fi

set_alsa()

golowanow · 2025-08-01T19:54:23Z

case-lib/relay.sh

+
+    local switch_name=$1
+    local state=$2
+


please add something like this to log the initial usbrelay status, to see all its relays, also to check it is runnable:

usbrelay --debug || die "usbrelay actual state read failed"

marc-hb

Code looks good, someone must review the actual audio logic.

marc-hb · 2025-09-04T02:13:33Z

case-lib/relay.sh

+# param2: switch state
+usbrelay_switch()
+{
+    [[ "$1" == "--debug" ]] && {


Suggested change

[[ "$1" == "--debug" ]] && {

if [[ "$1" == "--debug" ]]; then {

The && "shortcut" can sometimes save a couple lines but it does not save anything here. Also, && can be deceptive, for instance:

true && grep notfound /etc/passwd || echo 'ELSE is also run!!'

|| saves a negation and is less subtle than &&

See explanation in "Prefer || over && when possible" in #312

marc-hb · 2025-09-05T02:04:10Z

This has the same relay.sh file as #1280 but it's not added in a separate commit.

Add new test case to check hardware mic privacy mode enablement feature. For switching mic privacy state is using a USB relay. https://github.com/darrylb123/usbrelay Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>

The file is added in the separate PR 1280: thesofproject#1280 Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>

The file is added in the separate PR 1278: thesofproject#1278 Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>

arikgreen requested review from a team, golowanow, lgirdwood and marc-hb as code owners June 16, 2025 13:55

arikgreen requested review from KamilxPaszkiet and Copilot June 16, 2025 13:55

Copilot AI reviewed Jun 16, 2025

View reviewed changes

arikgreen force-pushed the awilczax/add-test-mic-prv branch from 9675aae to dba4f4a Compare June 17, 2025 07:10

arikgreen requested review from gbarkadiusz and redzynix June 17, 2025 08:30

golowanow reviewed Jun 17, 2025

View reviewed changes

arikgreen force-pushed the awilczax/add-test-mic-prv branch from dba4f4a to fed81ff Compare June 25, 2025 11:17

marc-hb reviewed Jun 25, 2025

View reviewed changes

arikgreen force-pushed the awilczax/add-test-mic-prv branch from fed81ff to 2b296b1 Compare June 25, 2025 16:48

arikgreen requested review from golowanow and marc-hb July 5, 2025 22:25

golowanow reviewed Jul 8, 2025

View reviewed changes

arikgreen force-pushed the awilczax/add-test-mic-prv branch 4 times, most recently from 32e0fc7 to 74def0a Compare July 14, 2025 21:26

marc-hb reviewed Jul 14, 2025

View reviewed changes

marc-hb mentioned this pull request Jul 24, 2025

test-case: add test jack detection playback #1280

Merged

arikgreen force-pushed the awilczax/add-test-mic-prv branch 4 times, most recently from 7e5bff3 to 3c4a6f7 Compare July 29, 2025 19:42

golowanow reviewed Aug 1, 2025

View reviewed changes

arikgreen force-pushed the awilczax/add-test-mic-prv branch 4 times, most recently from e547530 to 2d582c3 Compare August 4, 2025 17:49

arikgreen force-pushed the awilczax/add-test-mic-prv branch from 2d582c3 to 43b6d50 Compare September 3, 2025 11:11

marc-hb reviewed Sep 4, 2025

View reviewed changes

arikgreen force-pushed the awilczax/add-test-mic-prv branch 2 times, most recently from 9e69829 to d74ea44 Compare September 4, 2025 13:15

KamilxPaszkiet approved these changes Sep 4, 2025

View reviewed changes

arikgreen added 2 commits September 5, 2025 15:23

test-case: add new test case for check mic privacy

0861b34

Add new test case to check hardware mic privacy mode enablement feature. For switching mic privacy state is using a USB relay. https://github.com/darrylb123/usbrelay Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>

test-case: add lib file for test-mic-privacy

6ecdb54

The file is added in the separate PR 1280: thesofproject#1280 Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>

arikgreen force-pushed the awilczax/add-test-mic-prv branch from d74ea44 to 6ecdb54 Compare September 5, 2025 13:32

redzynix approved these changes Sep 5, 2025

View reviewed changes

arikgreen requested a review from cgturner1 September 8, 2025 15:07

arikgreen added a commit to arikgreen/sof-test that referenced this pull request Sep 8, 2025

test-case: add lib file for test-mic-privacy

ce0d396

The file is added in the separate PR 1278: thesofproject#1278 Signed-off-by: Artur Wilczak <arturx.wilczak@intel.com>

cgturner1 approved these changes Sep 8, 2025

View reviewed changes

cgturner1 merged commit 5e71e20 into thesofproject:main Sep 8, 2025
5 of 8 checks passed

		## Enable MIC privacy.
		## Run alsabat process perform both playback and capture again.

	if ! usbrelay "$switch_name=$state" >/dev/null 2>&1; then
	usbrelay "$switch_name=$state" >/dev/null 2>&1 \|\| {

	if [[ "$current_state" != "$state" ]]; then
	[[ "$current_state" == "$state" ]] \|\| {

	usbrelay "$switch_name=$state" >/dev/null 2>&1 \|\| {
	usbrelay "$switch_name=$state" 2>&1 \|\| {

	command -v usbrelay >/dev/null 2>&1 \|\| {
	command -v usbrelay 2>&1 \|\| {


		logger_disabled \|\| func_lib_start_log_collect

		if [ "$pcm_p" = "" ]\|\|[ "$pcm_c" = "" ]; then

	if [ "$pcm_p" = "" ]\|\|[ "$pcm_c" = "" ]; then
	if [ -z "$pcm_p" ] \|\| [ -z "$pcm_c" ]; then

	skip_test "No playback or capture PCM is specified. Skip the alsabat test."
	skip_test "No playback or capture PCM is specified. Skip the $0 test."

-    if [ $alsabat_status -ne 0 ]; then
+    alsabat -P"$pcm_p" -C"$pcm_c" -c 2 -r "$rate" >"$alsabat_output" 2>&1 || {
+            dloge "alsabat passed unexpectedly, upload the wav files."
+        __upload_wav_files
+        die "MIC privacy doesn't work. alsabat output: $(cat "$alsabat_output")"
+    }

	usbrelay "$switch_name=$state" > /dev/null \|\| {
	usbrelay "$switch_name=$state" \|\| {

	die "alsabat failed"
	die "check_playback_capture() failed: no loopback connected ?"

	[[ "$1" == "--debug" ]] && {
	if [[ "$1" == "--debug" ]]; then {

Conversation

arikgreen commented Jun 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jun 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marc-hb Jun 25, 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!

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!

arikgreen commented Jun 25, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

marc-hb Jun 25, 2025 •

edited

Loading