[SKIP CI] Enable check-pause-release-suspend-resume.sh to automatically test all PCMs #760

1994lwz · 2021-09-02T01:50:26Z

This PR allows user to specify an input tplg file. Then the test case will call func_pipeline_parse_value to automatically obtain pipeline parameters from the tplg file, and triggers aplay/arecord for all pipelines by default, user can also use the option -S to specify only one PCM to test.

User no longer needs to use parameter to specify each pipeline parameters.

The purpose is to allow this case to automatically test all PCMs, which is more suitable for the automated operation of CI.

Signed-off-by: Wanzhuo.Li wanzhuox.li@intel.com

marc-hb

The commit message body is empty and the subject line is too long.

Please follow:
https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure
... and most other advice on that page.

test-case/check-pause-release-suspend-resume.sh

marc-hb · 2021-09-03T06:13:52Z

test-case/check-pause-release-suspend-resume.sh

+    rate=$(func_pipeline_parse_value "$idx" rate)
+    fmt=$(func_pipeline_parse_value "$idx" fmt)
+    dev=$(func_pipeline_parse_value "$idx" dev)
+    snd=$(func_pipeline_parse_value "$idx" snd)


Before the user could pass these on the command line. Not anymore, isn't that also removing a feature? Please explain in the commit message.

I don't remove a feature, now all these parameters can be parsed from the specified or default tplg file automatically. I explained in the commit message "This patch allows user to specify an input tplg file. Then the test case will call func_pipeline_parse_value to automatically obtain pipeline parameters from the tplg file.

User no longer needs to use parameter to specify each pipeline parameters."

OK the user can edit the topology file if needed, I did not consider that. Thanks for the explanation.

test-case/check-pause-release-suspend-resume.sh

marc-hb

The split into 3 commits is good, much more readable and maintainable, thanks!

One one hand you're removing the need to specify compatible parameters which looks good. On the other hand you're also removing the flexibility to choose any parameter in a supported range. I guess this is probably OK for this test but removing a feature should be at least mentioned in the commit messages.

The test case description in the help also needs to be updated to match the changes.

marc-hb · 2021-09-09T03:58:26Z

test-case/check-pause-release-suspend-resume.sh

+rate=$(func_pipeline_parse_value "$idx" rate)
+fmt=$(func_pipeline_parse_value "$idx" fmt)
+dev=$(func_pipeline_parse_value "$idx" dev)
+snd=$(func_pipeline_parse_value "$idx" snd)


This looks strange; how is it possible to have shell script code inside the TCL script?

How did you test this part?

marc-hb · 2021-09-09T06:18:44Z

test-case/check-pause-release-suspend-resume.sh

-OPT_NAME['p']='pcm'          OPT_DESC['p']='run test case on specified pipelines'
-OPT_HAS_ARG['p']=1             OPT_VAL['p']='id:any'
+OPT_NAME['S']='pcm'          OPT_DESC['S']='run test case on specified pipelines'
+OPT_HAS_ARG['S']=1             OPT_VAL['S']='id:any'


You cannot keep the same help message for a new option that works very differently! Maybe: "run test case on pipelines matching the filter argument"

Maybe you can refer to check-pause-resume.sh to keep your options consistent with it.

@marc-hb The -S is used to specify the same PCM ID parameter as -p, so I copy the the help info. I change the option from -p to -S to align with case check-pause-resume.sh that uses -S to specify PCM ID.

Since 'S' is an abbreviation, I think 'filter_string' is a better full name.

OK I got confused because:

a first commit changes the option behavior, from aplay argument to topology parser argument without changing its name

then a second commit changes its name from -p to -S without changing its help message.

Please change the feature, option name and help message all in the same git commit.

marc-hb · 2021-09-09T06:28:24Z

test-case/check-pause-release-suspend-resume.sh

@@ -51,22 +52,14 @@ set -e
 ##  * No unexpected errors should be present in dmesg during or after test
 ##      completion.

-source $(dirname ${BASH_SOURCE[0]})/../case-lib/lib.sh
+CASEDIR=$(dirname "${BASH_SOURCE[0]}")
+source $CASEDIR/../case-lib/lib.sh


Fix the quoting issues reported by shellcheck. Not all the issues in this script but at least all the ones in new or modified code. Don't add new shellcheck warnings.

# shellcheck source=...

See how other scripts do it.

mengdonglin · 2021-09-09T07:42:28Z

Reviewed the commit messages

ghost · 2021-09-09T09:48:14Z

test-case/check-pause-release-suspend-resume.sh

-OPT_NAME['p']='pcm'          OPT_DESC['p']='run test case on specified pipelines'
-OPT_HAS_ARG['p']=1             OPT_VAL['p']='id:any'
+OPT_NAME['S']='pcm'          OPT_DESC['S']='run test case on specified pipelines'
+OPT_HAS_ARG['S']=1             OPT_VAL['S']='id:any'


Since 'S' is an abbreviation, I think 'filter_string' is a better full name.

ghost · 2021-09-09T10:39:17Z

test-case/check-pause-release-suspend-resume.sh

@@ -75,21 +70,21 @@ OPT_NAME['l']='loop'         OPT_DESC['l']='loop count'
 OPT_HAS_ARG['l']=1             OPT_VAL['l']=5

 OPT_NAME['i']='sleep-period' OPT_DESC['i']='sleep period of aplay, unit is ms'
-OPT_HAS_ARG['i']=1             OPT_VAL['i']='100'
+OPT_HAS_ARG['i']=1             OPT_VAL['i']='200'


I've just tested this in my TGLU_SKU0A32_SDCA device and found that 200ms seems still too short for my device while 300ms will work fine. Maybe we need more investigation to determine a proper period, or just left it unchanged and specify a proper period for each device in our CI?

How about something much longer but still quick like 1s?

marc-hb

It can be painful to transfer code from one commit to another but sorry, each commit must be standalone, usable and run better than all the previous ones; no regression. You also shouldn't add code with warnings in one commit and then fix the warnings in a later commit. Amend the earlier commit and never have warnings in any commit.

Looking at intermediate commit 2b9ab9e right now I see new shell script code in the middle of TCL code which almost certainly shows that it does not work without a later commit that fixes it. That sort of git history is not acceptable, sorry.

To fix git history make sure you're very familiar with interactive rebase
https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History
https://www.thinktecture.com/en/tools/git-interactive-rebase/
https://stackoverflow.com/questions/15213814/how-should-i-move-changes-from-one-commit-to-another
(first guides I found, there are probably better ones).
I know there are github projects that don't care about git history and not require any interactive rebase/rewrite skill, we do require in this project that every commit works and is better than the previous ones for many maintenance reasons: bisecting, branching, cherry-picking, reverting etc. This is also required by many other projects so this is a very useful skill to have.

Also make sure you use the git plugin in your editor, many editors have git plugins that make git much faster to use (fugitive, magit, etc.). If your editor does not have a good git plugin you should consider a different editor

marc-hb · 2021-09-09T17:53:48Z

test-case/check-pause-release-suspend-resume.sh

-OPT_NAME['p']='pcm'          OPT_DESC['p']='run test case on specified pipelines'
-OPT_HAS_ARG['p']=1             OPT_VAL['p']='id:any'
+OPT_NAME['S']='pcm'          OPT_DESC['S']='run test case on specified pipelines'
+OPT_HAS_ARG['S']=1             OPT_VAL['S']='id:any'


OK I got confused because:

a first commit changes the option behavior, from aplay argument to topology parser argument without changing its name

then a second commit changes its name from -p to -S without changing its help message.

Please change the feature, option name and help message all in the same git commit.

marc-hb · 2021-09-09T18:07:24Z

test-case/check-pause-release-suspend-resume.sh

+    rate=$(func_pipeline_parse_value "$idx" rate)
+    fmt=$(func_pipeline_parse_value "$idx" fmt)
+    dev=$(func_pipeline_parse_value "$idx" dev)
+    snd=$(func_pipeline_parse_value "$idx" snd)


OK the user can edit the topology file if needed, I did not consider that. Thanks for the explanation.

marc-hb · 2021-09-09T18:17:26Z

test-case/check-pause-release-suspend-resume.sh

@@ -75,21 +70,21 @@ OPT_NAME['l']='loop'         OPT_DESC['l']='loop count'
 OPT_HAS_ARG['l']=1             OPT_VAL['l']=5

 OPT_NAME['i']='sleep-period' OPT_DESC['i']='sleep period of aplay, unit is ms'
-OPT_HAS_ARG['i']=1             OPT_VAL['i']='100'
+OPT_HAS_ARG['i']=1             OPT_VAL['i']='200'


How about something much longer but still quick like 1s?

marc-hb · 2021-09-09T18:33:25Z

It can be painful to transfer code from one commit to another

Another thing to make life easier is to expedite small, unrelated fixes easier to review in a different, parallel Pull Request to get them merged faster and quickly out of the way. For this and other reasons I rarely submit HEAD into a pull request, almost always HEAD~n.

On some platforms, 100ms is not enough to detect whether the audio thread has entered the pause state. This patch extends the sleep time to 500ms after pause Signed-off-by: Wanzhuo.Li <wanzhuox.li@intel.com>

ghost

I found that we have to force the suspend/resume type for the GLK platform. For check-suspend-resume.sh, it is controlled by -T option, so you should add the same option and forward its value to check-suspend-resume.sh.

sof-test/test-case/check-suspend-resume.sh

Lines 30 to 31 in 0bb4b66

    
           OPT_NAME['T']='type'    OPT_DESC['T']="suspend/resume type from /sys/power/mem_sleep" 
        
           OPT_HAS_ARG['T']=1         OPT_VAL['T']=""

ghost

You really misunderstand my words. I mean we should add an option and forward it instead of a hard-coded, unreliable workaround for GLK. Here I give an example in my review comments.

test-case/check-pause-release-suspend-resume.sh

ghost

I've just triggered a test (6539) for this on all models in the daily test, and all results are green.

marc-hb

The final code changes look mostly OK but the intermediate commits are still not working. Commit c4e5cc6c1875a67 still has shell script in the middle of the TCL expect code, so it is broken (that's fixed by next commit d1628af60b32)

Same thing for the shellcheck warnings: don't fix them in another commit; instead don't add any new warnings at all in the first place.

You must do an interactive rebase and make sure the test can run at each commit. It does not have to PASS on every platform at every commit but it must be able to run (and run better than the previous commit). To make conflict solving easier, perform one complete interactive rebase for each piece of code to move, I mean fix a single problem each time and perform as many interactive rebases as needed.

marc-hb · 2021-09-13T17:08:55Z

test-case/check-pause-release-suspend-resume.sh

 repeat_count=${OPT_VAL['l']}
 sleep_period=${OPT_VAL['i']}
 test_mode=${OPT_VAL['m']}
 file_name=${OPT_VAL['F']}
+tplg=${OPT_VAL['t']}
+
+if [ "${OPT_VAL['T']}" ]; then


Suggested change

if [ "${OPT_VAL['T']}" ]; then

if [ -n "${OPT_VAL['T']}" ]; then

otherwise an incorrect "${OPT_VAL['T']}" could generate very confusing behaviors.

marc-hb · 2021-09-13T17:09:31Z

test-case/check-pause-release-suspend-resume.sh

+if [ "${OPT_VAL['T']}" ]; then
+    opt="-l 1 -T ${OPT_VAL['T']}"
+else
+    opt="-l 1"


Please change opt to something longer and clearer like for instance suspend_opts

marc-hb · 2021-09-13T17:20:45Z

test-case/check-pause-release-suspend-resume.sh

@@ -178,8 +180,7 @@ AUDIO
    #flush the output
    echo
    if [ $ret -ne 0 ]; then
-        sof-process-kill.sh
-        [[ $? -ne 0 ]] && dlogw "Kill process catch error"
+        sof-process-kill.sh || dlogw "Kill process catch error"


sof-process-kill.sh || dlogw "sof-process-kill.sh failed"

plbossart

thanks for updating this script, see comments/suggestions below.

plbossart · 2021-09-14T18:26:05Z

test-case/check-pause-release-suspend-resume.sh

@@ -75,7 +75,7 @@ OPT_NAME['l']='loop'         OPT_DESC['l']='loop count'
 OPT_HAS_ARG['l']=1             OPT_VAL['l']=5

 OPT_NAME['i']='sleep-period' OPT_DESC['i']='sleep period of aplay, unit is ms'
-OPT_HAS_ARG['i']=1             OPT_VAL['i']='100'
+OPT_HAS_ARG['i']=1             OPT_VAL['i']='500'


can I ask what platforms? This is a 5x increase, but the intervals between pause_release and pause_push are rather small.

I also don't get why this is different from the regular pause-resume test.

@YongAnLu00 Yongan test the script on his TGLU_SKU0A32_SDCA device and found that 200ms seems still too short, and Marc suggest change the sleep time much longer but still quick like 1se

plbossart · 2021-09-14T18:28:48Z

test-case/check-pause-release-suspend-resume.sh

@@ -13,6 +13,8 @@ set -e
 ##    have system enter suspend state for 5 secs
 ##    resume from suspend state
 ##    release audio stream from paused state
+##    repeat to all pipelines


the test description is not clear

line 15: is the stream always paused prior to suspend? Or can this be random/variable?
line 16: can we have multiple streams in parallel? This will be required for our tests, in fact if you see thesofproject/linux#3151 I am precisely having issues with suspend-resume with 2 paused streams.

In this PR, we will first implement a single pipeline test of pause-release-suspend-resume, and then we will re-prove a PR to implement multiple pipeline tests.

plbossart · 2021-09-14T18:30:01Z

test-case/check-pause-release-suspend-resume.sh

@@ -74,7 +76,7 @@ OPT_HAS_ARG['i']=1             OPT_VAL['i']='500'
 OPT_NAME['s']='sof-logger'   OPT_DESC['s']="Open sof-logger trace the data will store at $LOG_ROOT"
 OPT_HAS_ARG['s']=0             OPT_VAL['s']=1

-OPT_NAME['t']='tplg'     OPT_DESC['t']='tplg file, default value is env TPLG: $TPLG'
+OPT_NAME['t']='tplg'     OPT_DESC['t']='tplg path, default is environment variable: TPLG'


TPLG is the path+filename, not just the path. This doesn't seem like a valid correction.

Moreover this is a correction of a previous commit in this same PR. The git history is still a mess, don't waste your time reviewing each commit. In addition to having commits that can't run, history that does not bisect, reverts or cherry-picks and confused reviewers, this is also confusing github: so you can't answer some threads unless you select the right commit.

EDIT: as of Sep. 21st (or earlier) the commit history is now clean, thanks!

Back to the description: "path" is ambiguous, for some people path the dirname but for many others like https://docs.python.org/3/library/pathlib.html path = dirname + filename.

"file" is vague but at least every knows it's vague, so no one makes wrong assumptions about it. So I agree "file" is better.

TPLG supports both complete paths and filenames.

plbossart · 2021-09-14T18:31:50Z

test-case/check-pause-release-suspend-resume.sh

+OPT_HAS_ARG['t']=1             OPT_VAL['t']="$TPLG"
+
+OPT_NAME['T']='type'         OPT_DESC['T']="suspend/resume type from /sys/power/mem_sleep"
+OPT_HAS_ARG['T']=1             OPT_VAL['T']=""


I was not able to understand what the option means. the 'from' part threw me off, did you mean instead that it would set the suspend/resume type by writing to sysfs?

This option is merely passed passed-through to test-case/check-suspend-resume.sh and the description is merely copied from there (which does not mean it cannot be improved)

marc-hb · 2021-09-14T18:42:42Z

test-case/check-pause-release-suspend-resume.sh

+OPT_HAS_ARG['t']=1             OPT_VAL['t']="$TPLG"
+
+OPT_NAME['T']='type'         OPT_DESC['T']="suspend/resume type from /sys/power/mem_sleep"
+OPT_HAS_ARG['T']=1             OPT_VAL['T']=""


This option is merely passed passed-through to test-case/check-suspend-resume.sh and the description is merely copied from there (which does not mean it cannot be improved)

marc-hb · 2021-09-14T18:48:57Z

test-case/check-pause-release-suspend-resume.sh

@@ -74,7 +76,7 @@ OPT_HAS_ARG['i']=1             OPT_VAL['i']='500'
 OPT_NAME['s']='sof-logger'   OPT_DESC['s']="Open sof-logger trace the data will store at $LOG_ROOT"
 OPT_HAS_ARG['s']=0             OPT_VAL['s']=1

-OPT_NAME['t']='tplg'     OPT_DESC['t']='tplg file, default value is env TPLG: $TPLG'
+OPT_NAME['t']='tplg'     OPT_DESC['t']='tplg path, default is environment variable: TPLG'


Moreover this is a correction of a previous commit in this same PR. The git history is still a mess, don't waste your time reviewing each commit. In addition to having commits that can't run, history that does not bisect, reverts or cherry-picks and confused reviewers, this is also confusing github: so you can't answer some threads unless you select the right commit.

EDIT: as of Sep. 21st (or earlier) the commit history is now clean, thanks!

Back to the description: "path" is ambiguous, for some people path the dirname but for many others like https://docs.python.org/3/library/pathlib.html path = dirname + filename.

"file" is vague but at least every knows it's vague, so no one makes wrong assumptions about it. So I agree "file" is better.

TPLG supports both complete paths and filenames.

…h pipeline This patch allows user to specify an input tplg file. Then the test case will call func_pipeline_parse_value to automatically obtain pipeline parameters from the tplg file. User no longer needs to use parameter to specify each pipeline parameters. change the option from -p to -S to align with case check-pause-resume.sh that uses -S to specify PCM ID. This case will test all PCMs by default. Signed-off-by: Wanzhuo.Li <wanzhuox.li@intel.com>

Add the -T parameter to specify the execution state of suspend-resume Signed-off-by: Wanzhuo.Li <wanzhuox.li@intel.com>

git breakdown now OK

1994lwz requested a review from a team as a code owner September 2, 2021 01:50

marc-hb requested changes Sep 3, 2021

View reviewed changes

1994lwz requested a review from marc-hb September 7, 2021 01:25

1994lwz changed the title ~~Add a new feature: let the case can automatic testing of all PCMs~~ Add loop: check-pause-release-suspend-resume.sh automatically test all PCMs and optimize the code logic Sep 7, 2021

1994lwz changed the title ~~Add loop: check-pause-release-suspend-resume.sh automatically test all PCMs and optimize the code logic~~ Enable check-pause-release-suspend-resume.sh to automatically test all PCMs Sep 7, 2021

1994lwz requested review from XiaoyunWu6666, a user, keqiaozhang and mengdonglin September 7, 2021 07:38

marc-hb requested changes Sep 9, 2021

View reviewed changes

marc-hb reviewed Sep 9, 2021

View reviewed changes

mengdonglin previously approved these changes Sep 9, 2021

View reviewed changes

1994lwz dismissed mengdonglin’s stale review via 4eb3a0d September 9, 2021 09:25

1994lwz requested review from aiChaoSONG, marc-hb and mengdonglin September 9, 2021 09:29

ghost suggested changes Sep 9, 2021

View reviewed changes

marc-hb requested changes Sep 9, 2021

View reviewed changes

pause-release-suspend-resume: Extend the sleep time to 500ms

f581a64

On some platforms, 100ms is not enough to detect whether the audio thread has entered the pause state. This patch extends the sleep time to 500ms after pause Signed-off-by: Wanzhuo.Li <wanzhuox.li@intel.com>

1994lwz requested a review from marc-hb September 10, 2021 05:39

ghost suggested changes Sep 10, 2021

View reviewed changes

ghost changed the title ~~Enable check-pause-release-suspend-resume.sh to automatically test all PCMs~~ [SKIP CI] Enable check-pause-release-suspend-resume.sh to automatically test all PCMs Sep 13, 2021

ghost previously approved these changes Sep 13, 2021

View reviewed changes

marc-hb requested changes Sep 13, 2021

View reviewed changes

1994lwz dismissed ghost ’s stale review via 9102091 September 14, 2021 02:31

plbossart requested changes Sep 14, 2021

View reviewed changes

marc-hb previously requested changes Sep 14, 2021

View reviewed changes

Wanzhuo.Li added 2 commits September 15, 2021 12:05

pause-release-suspend-resume: Change code to be compatible with GLK

fe183b8

Add the -T parameter to specify the execution state of suspend-resume Signed-off-by: Wanzhuo.Li <wanzhuox.li@intel.com>

1994lwz requested review from marc-hb, a user and plbossart September 22, 2021 02:20

	OPT_NAME['T']='type' OPT_DESC['T']="suspend/resume type from /sys/power/mem_sleep"
	OPT_HAS_ARG['T']=1 OPT_VAL['T']=""

	if [ "${OPT_VAL['T']}" ]; then
	if [ -n "${OPT_VAL['T']}" ]; then

[SKIP CI] Enable check-pause-release-suspend-resume.sh to automatically test all PCMs #760

Are you sure you want to change the base?

[SKIP CI] Enable check-pause-release-suspend-resume.sh to automatically test all PCMs #760

Conversation

1994lwz commented Sep 2, 2021 • edited Loading

marc-hb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1994lwz Sep 9, 2021 • edited Loading

Choose a reason for hiding this 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

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

mengdonglin commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb commented Sep 9, 2021

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

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

plbossart left a comment

Choose a reason for hiding this comment

plbossart Sep 14, 2021 • 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

Choose a reason for hiding this comment

marc-hb Sep 14, 2021 • 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

marc-hb Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

1994lwz commented Sep 2, 2021 •

edited

Loading

1994lwz Sep 9, 2021 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

marc-hb left a comment •

edited

Loading

plbossart Sep 14, 2021 •

edited

Loading

marc-hb Sep 14, 2021 •

edited

Loading

marc-hb Sep 14, 2021 •

edited

Loading