fix: tests: SRC tests catch aplay warnings and refactor#1351
Open
ekurdybx wants to merge 1 commit intothesofproject:mainfrom
Open
fix: tests: SRC tests catch aplay warnings and refactor#1351ekurdybx wants to merge 1 commit intothesofproject:mainfrom
ekurdybx wants to merge 1 commit intothesofproject:mainfrom
Conversation
683253f to
4fe141b
Compare
Catch aplay incorrect sample rate warnings and refactor tests Signed-off-by: Emilia Kurdybelska <emiliax.kurdybelska@intel.com>
marc-hb
reviewed
Mar 10, 2026
| check_for_aplay_warnings() | ||
| { | ||
| dlogi "$1" | ||
| if echo "$1" | grep -q "Warning:"; then |
Collaborator
There was a problem hiding this comment.
|
|
||
| # Check aplay output for warnings | ||
| # Arguments: 1-aplay output | ||
| check_for_aplay_warnings() |
Collaborator
There was a problem hiding this comment.
Suggested change
| check_for_aplay_warnings() | |
| check_for_warnings() |
| # shellcheck disable=SC2086 | ||
| arecord $SOF_ALSA_OPTS $SOF_ARECORD_OPTS $1 & PID=$! | ||
| # shellcheck disable=SC2181 | ||
| if [ $? -ne 0 ]; then |
Collaborator
There was a problem hiding this comment.
$? will never be useful when you background a command. To get the status of a backgrounded command you need to wait $PID
| # shellcheck disable=SC2086 | ||
| aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 | ||
| aplay_output=$(aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1) | ||
| # shellcheck disable=SC2181 |
Collaborator
There was a problem hiding this comment.
Why not just:
aplay_output=$(aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1) || errors=$((errors+1))
| fi | ||
| # shellcheck disable=SC2086 | ||
| aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 | ||
| aplay_output=$(aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1) |
Collaborator
There was a problem hiding this comment.
Capturing output loses "real-time" feedback. You can (and should) have both with something like this:
set -o pipefail
aplay $SOF_ALSA_OPTS $SOF_APLAY_OPTS $2 2>&1 | tee _.log || errors=$((errors+1))
check_warnings _.log
There are other ways:
https://stackoverflow.com/questions/1221833/pipe-output-and-capture-exit-status-in-bash
Collaborator
|
Forgot to mention this: The "real" fix should be in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's changed: