Skip to content

Conversation

@aiChaoSONG
Copy link

No description provided.

@aiChaoSONG aiChaoSONG requested a review from a team as a code owner November 16, 2020 09:09
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please help review #497 (and others) first.

Also let's not add too much to the already ridiculous amount of copy/paste across these two files (they seem 90% identical!!!). You wrote a function so it should be re-usable without copy/paste/diverge...

Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I just realized you are reducing copy/paste/diverge a bit: from 4 instances to 2 instances. How about finishing the job and going to just one instance?

@xiulipan
Copy link
Contributor

@aiChaoSONG Ping for update, also rebase is needed.

Amery Song added 3 commits November 18, 2020 13:36
In these two cases, The variable tmp_count is
used and modified everywhere, this is error-prone.

This patch refine code structure, renames some
variables and add some debug logs to root cause
sof-test#472

Signed-off-by: Amery Song <chao.song@intel.com>
Signed-off-by: Amery Song <chao.song@intel.com>
This patch moves a common function func_error_exit
to lib.sh to eliminate duplicated code in
multiple-pipeline-playback.sh and multiple-pipeline-
capture.sh

Signed-off-by: Amery Song <chao.song@intel.com>
@aiChaoSONG
Copy link
Author

@xiulipan @marc-hb Updated, I going to merge the multiple-pipeline-playback.sh and multiple-pipeline-capture.sh into one script later, so some common code is still there. the function func_error_exit can be used by other scripts later,so it is moved to lib.sh

Copy link
Contributor

@xiulipan xiulipan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try to use local in loops or functions to avoid wrong usage.

sudo dmesg -C
# this variable is modified in func_run_pipeline_with_type,
# need to reassign at every iteration
tmp_count=$max_count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use local for the tmp_count if you do not want it to be used outside.

aplay_count=$(pidof aplay | wc -w)
dlogi "$aplay_count playback process is running"
overall_count=$((arecord_count + aplay_count))
[[ $overall_count -eq $max_count ]] ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use local for the overall_count

@marc-hb
Copy link
Collaborator

marc-hb commented Nov 18, 2020

Please help review #497 (and others) first.

I screwed up #497, apologies. Tentative (and urgent) fix in #521.

It's not practical to work on the same code at the same time because it will cause git conflicts. @aiChaoSONG please pause your work on the multiple pipelines tests for a couple days until I'm done with:

Once I'm all done I promise these tests will be all yours to deduplicate and clean-up! Appreciated.

#521 should fix
https://sof-ci.01.org/softestpr/PR516/build404/devicetest/?model=APL_UP2_NOCODEC&testcase=multiple-pipeline-capture

@marc-hb
Copy link
Collaborator

marc-hb commented Nov 18, 2020

re-synchronizing multiple-pipeline-playback with multiple-pipeline-capture (I totally missed the massive copy/paste)

Submitted in #525

@marc-hb
Copy link
Collaborator

marc-hb commented Dec 1, 2020

I think I got the root cause, see candidate fix in #538

@aiChaoSONG
Copy link
Author

@472 is already root caused and fixed, close.

@aiChaoSONG aiChaoSONG closed this Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants