-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test-case: rename pause-resume to pause-release #1025
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree sorry.
The only way to confuse "pause/resume" with suspend/resume is to ignore the first word! I've never seen anyone confusing "pause" with "suspend". So this is a whole lot of code churn and potential git conflicts for solving a minor issue.
The official man aplay
calls it "pause/resume"
"release" is not the opposite of "pause", "release" is the opposite of "acquire". Morever, playing typically acquires resources while pausing releases them.
If other people think a change is really necessary then pause-unpause
or pause-start
would be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit concerned with the transition time with existing scripts running check-pause-resume.sh getting broken with this -- see inline comment. Otherwise seems good.
I support this change 100%, but the only want to avoid naming issues is to use the ALSA terminology: check_pause_push_pause_release is a bit longer but will avoid any confusions. And yes a symlink is required until all references to the old script are removed. |
Can you please share a link to the official ALSA terminology? I did a quick search but I only found "pause/resume" in
What could the much shorter |
Now shellcheck clean before thesofproject#1025 Signed-off-by: Marc Herbert <[email protected]>
https://elixir.bootlin.com/linux/latest/source/include/sound/pcm.h#L100 this is a kernel test, let's use kernel terminology. We have similar confusions in SoundWire between clock_pause, stretch, stop, resume, restart. Use one set of words clearly linked to a well-defined concept is key. Unpause is not such a word, sorry. |
https://sof-ci.01.org/softestpr/PR1025/build285/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=multiple-pause-resume-50 (internal 23679) failed with a strange warning (that did not cause the failure)
|
Now shellcheck clean before #1025 Signed-off-by: Marc Herbert <[email protected]>
shellcheck warnings fixed in #1026, minor conflict now, needs rebase. |
cb4d75e
to
aa43e38
Compare
rebased top of main |
resume term leads to confusion with system resume. Nothing to do with system suspend/resume test. pause-release is more clear. Signed-off-by: Fred Oh <[email protected]>
To support backward compatibility, keep the symbolic link for the time being. check-pause-resume.sh -> check-pause-release.sh multiple-pause-resume.sh -> multiple-pause-release.sh Signed-off-by: Fred Oh <[email protected]>
aa43e38
to
e5f7ac1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the symbolic links you could use an exec
like in test-case/multiple-pipeline-capture.sh
which would give you the possibility of printing a deprecation warning.
@marc-hb I take adding symbolic links is really short term solution. I could update the test case database in same time with the merge without the symbolic link. as soon as this merge. |
There are always more users than you think. In fact there is already one in this repo that you missed, looks like you didn't |
Renamed pause-resume to pause-release. Ref link: thesofproject#1025 Signed-off-by: Fred Oh <[email protected]>
missed run-all-test.sh |
@marc-hb @plbossart @ranj063 do you have any comment for this PR? |
resume term leads to confusion with system resume. Nothing to do with system suspend/resume test. pause-release is more clear.
fixes: #792
test plan in SOF CI system also need to rename the command also when this is merged.