Test-case: Add check volume levels by singalsu · Pull Request #410 · thesofproject/sof-test

singalsu · 2020-10-06T13:56:16Z

This patch adds a shell script that measures volume levels
to compute gains when executed with a nocodec topology where
SSP ports are connected to play -> capture loopback.

The measured gains are compared against test procedure and
error is issued if the gains deviate more than tolerance.

Signed-off-by: Seppo Ingalsuo seppo.ingalsuo@linux.intel.com

singalsu · 2020-10-06T14:13:07Z

Update: I changed echo commands to dlogi and dloge.

singalsu · 2020-10-06T15:29:57Z

Fixed environment variable declarations / exports.

singalsu · 2020-10-06T16:04:16Z

For fun I added it to run-all-tests.sh. It will fail on most devices due to no Octave installed. Also the nocodec topologies do not create the required mute switch control. I'll do a separate SOF PR for it.

singalsu · 2020-10-08T10:52:34Z

@marc-hb Thanks for all the help! I've tried to address all your comments now. Though there are two cases where I'm not yet sure the approach is the best one.

I'll still add to Octave part channels filtering for center frequencies. It will catch then also channels swaps. Also I'll test adding ramp length verification for the 0-100% transitions.

Can this be merged to sof-test as non-enabled test (while need for Octave)?

marc-hb

Thanks for your patience, I think we're getting very close with the shell script. Need someone to review the matlab code and logic.

singalsu · 2020-10-09T15:00:58Z

This test code needs sof-test PR #417 to not fail with channel count and device strings and SOF PR thesofproject/sof#3509 to have needed full-blown volume component controls instantiated.

marc-hb

I believe no one reviewed the Octave code yet :-( Who could do this?

I would be very worrying if we have only one person who can review Octave code, maybe we shouldn't even be using Octave in validation in that case! :-((

This test code needs sof-test PR #417 to not fail with channel count and device strings and SOF PR thesofproject/sof#3509 to have needed full-blown volume component controls instantiated.

I still have a few minor comments but the shell script looks basically ready to me. However considering this test cannot run in CI (yet), we need IMHO evidence of at least one person other than the submitter who could run the test. It doesn't even have to pass, just to run and produce some meaningful audio results anywhere but on the submitter's system. I tried it last week but I couldn't run it on my platform in a reasonable time, not even with some local modifications/hacks. @singalsu should I wait until thesofproject/sof#3509 is merged before trying again?

marc-hb · 2020-10-12T20:16:03Z

https://sof-ci.01.org/softestpr/PR410/build251/devicetest/ experienced a device failure on BSW_CYN_MAX98090

kernel: [ 4.509348] EXT4-fs (mmcblk0p2): warning: maximal mount count reached, running e2fsck is recommended
kernel: [ 4.515925] EXT4-fs (mmcblk0p2): re-mounted. Opts: errors=remount-ro

keyonjie · 2020-10-13T09:02:50Z

@singalsu I am trying to run your test case on TGL nocodec, it fails as it has no 'Switch' kcontrol there, which tplg I should use to run it?

singalsu · 2020-10-13T12:26:39Z

@keyonjie There's need for thesofproject/sof#3509 to add the missing switch controls.

marc-hb

The shell script looks good to me. Before merging I think this still needs:

at least one other person who could run this (even if the tests don't pass)
someone to review the Octave code.

juimonen

LGTM for the octave code

xiulipan

Could we add a check for octave in test case first to avoid accidently run fail for this case.

marc-hb · 2020-10-15T07:47:10Z

Could we add a check for octave in test case first to avoid accidently run fail for this case.

I think this could be useful later once we want to run this in automation but we would like to merge it earlier than that. For now I believe we still don't have any report yet of any other person besides the author who could run it manually.

singalsu · 2020-10-15T09:57:35Z

@xiulipan Yes, makes sense I'll add the check for octave.

@keyonjie Can you please add your review / report of what was need to get it running on your device.

xiulipan

LGTM, let's try to do some integration with CI framework.

keyonjie

Thanks for the case, it looks professional with good coverage on the volume level.
For the pipeline coverage, it only change volume/switch for the capture one, is it worth to add these to playback pipeline also?

keyonjie · 2020-10-16T09:03:58Z

+}
+
+pause () {
+ dlogi "Pausing"


The wording "pause" is somewhat confusion, we are not aim to pause any playback/capture stream, maybe 'sleep_1s'?

The test script really pauses execution so I prefer this. Just sleep is a reserved word and don't want to hard code with the name the length to 1s. It could be adjusted later to more than 1s for better accuracy or less than 1s for lower test time.

nap() ? :-)

yes, either 'nap' / 'nop' or 'idle' sound better to me, or even exec_pause(), as we have test cases naming xxx_pause_release_xxx, I thought we are pausing the playback/capture pipeline at my first glance to the PR.

nap sounds good to me!

keyonjie · 2020-10-16T09:54:58Z

+ fprintf(1, 'PASS\n');
+ else
+ fprintf(1, 'FAIL\n');
+ exit(1);


shall we remove those 2 'exit(1)' as they are making my octave hang when running it with gui mode, logging errors should be enough?

This script is not for GUI mode but for batch processing. For interactive tests with GUI I've commented out those lines. The GUI doesn't hang but it quits with exit().

I'm not aware of simple ways in script to check if we are in Octave GUI. If such exist we could make a custom exit function.

This script is not for GUI mode but for batch processing.
It would be great to find a solution that supports both but waiting for that should IMHO not block this "automation" PR.

@singalsu @marc-hb just removing this 'exit(1)' line will work for both GUI and batch processing per my testing.

The shell script thinks a failed test passed if I omit the exit(1).

Okay, got you now @singalsu

The shell script thinks a failed test passed if I omit the exit(1).
Thanks @singalsu for "testing the test code" and making sure it can report failures. I found this to be incredibly rare in this project.
After only 2 minutes googling (sorry) I found "error()", did you try it? https://octave.org/doc/v4.0.1/Raising-Errors.html

Yes I'm using error() a lot to stop script execution but I haven't used to return code to shell. I'll see if it works. It doesn't quit the GUI like exit does. If it it works, it's more convenient.

Thanks, yes it works for the purpose! I'll switch to error().

marc-hb

This doesn't work for me sometimes,

Considering this test is not enabled in automation yet, working "sometimes" for at least one person besides the submitter is IMHO enough "validation". Merging it will make fixing it easier.

marc-hb · 2020-10-16T16:37:03Z

+}
+
+pause () {
+ dlogi "Pausing"


nap() ? :-)

marc-hb · 2020-10-19T18:17:31Z

jf-icl-rvp-hda-4 boot failed in https://sof-ci.01.org/softestpr/PR410/build274/devicetest/

keyonjie

Looks good, we can add incremental fixes if minor issues existed.

This patch adds a shell script to check that SOF volume component related system applies correctly channels gains and mute switches. The script determines the volume gains from measured sine wave levels and compares to used amixer control values. The test can be executed with a nocodec topology where SSP ports are connected to play -> capture loopback. The loopback avoids need for noise sensitive acoustical test or more complex electrical test. An error is issued if the gains deviate more than used +/- 0.5 dB tolerance. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>

singalsu · 2020-10-20T11:15:53Z

I did the proposed and agreed updates. In addition I added a verbose flag to level check function to prevent additional reports of incorrect level printed while the script tries if swapped channels and swapped controls would pass. I always test the changes with manual add of error to controls or measurements side to see I get the pass and fail correctly.

marc-hb

Looks good, we can add incremental fixes if minor issues existed.

+1 considering this doesn't run in CI yet.

marc-hb · 2020-10-20T22:47:44Z

+	exit 2
+ fi
+
+ test -n "$(command -v octave)" || {


Simpler and logging the location
type octave || { ... }

I didn't know that, it's very compact but not very intuitive. If you are OK to re-review I can change this.

singalsu requested review from aiChaoSONG and keyonjie October 6, 2020 13:56

singalsu requested a review from a team as a code owner October 6, 2020 13:56

singalsu force-pushed the add_volume_levels_check branch from d25cc21 to 60e4330Compare October 6, 2020 14:12

singalsu force-pushed the add_volume_levels_check branch from 60e4330 to 60aa062Compare October 6, 2020 15:27

singalsu force-pushed the add_volume_levels_check branch from 60aa062 to 3fe0934Compare October 6, 2020 16:01

marc-hb requested changes Oct 6, 2020
View reviewed changes

singalsu force-pushed the add_volume_levels_check branch 2 times, most recently from 84956de to fa8a4cdCompare October 8, 2020 10:26

marc-hb requested changes Oct 8, 2020
View reviewed changes

marc-hb requested changes Oct 9, 2020
View reviewed changes

Comment threadtest-case/check-volume-levels.sh Outdated
Comment threadtest-case/check-volume-levels.sh Outdated

singalsu commented Oct 9, 2020
View reviewed changes

Comment threadtest-case/check-volume-levels.sh Outdated

singalsu force-pushed the add_volume_levels_check branch from fa8a4cd to b9fe1c0Compare October 9, 2020 14:57

singalsu changed the title ~~[WIP][RFC] Test-case: Add check volume levels~~[RFC] Test-case: Add check volume levelsOct 9, 2020

singalsu force-pushed the add_volume_levels_check branch from b9fe1c0 to 63f7df0Compare October 9, 2020 15:06

singalsu requested a review from marc-hb October 9, 2020 15:50

singalsu force-pushed the add_volume_levels_check branch from 63f7df0 to f3a1108Compare October 12, 2020 15:28

marc-hb reviewed Oct 12, 2020
View reviewed changes

Comment threadtest-case/check-volume-levels.sh Outdated
Comment threadtest-case/check-volume-levels.sh
Comment threadtest-case/check-volume-levels.sh Outdated
Comment threadtest-case/check-volume-levels.sh Outdated
Comment threadtest-case/check-volume-levels.sh Outdated

singalsu force-pushed the add_volume_levels_check branch from f3a1108 to 56f6144Compare October 13, 2020 12:25

singalsu changed the title ~~[RFC] Test-case: Add check volume levels~~Test-case: Add check volume levelsOct 13, 2020

singalsu requested a review from marc-hb October 13, 2020 15:41

marc-hb reviewed Oct 13, 2020
View reviewed changes

Comment threadtest-case/check-volume-levels.sh Outdated
Comment threadtest-case/check-volume-levels.sh Outdated

juimonen previously approved these changes Oct 14, 2020
View reviewed changes

marc-hb mentioned this pull request Oct 14, 2020
[BUG] BYT: Random stereo channels swaps happen with sof-byt-nocodec.tplg thesofproject/sof#3520
Closed

xiulipan suggested changes Oct 15, 2020
View reviewed changes

singalsu dismissed juimonen’s stale review via bf81226October 15, 2020 10:21

singalsu force-pushed the add_volume_levels_check branch from 0c8c0c8 to bf81226Compare October 15, 2020 10:21

singalsu requested a review from xiulipan October 15, 2020 10:23

singalsu force-pushed the add_volume_levels_check branch from bf81226 to 2005a99Compare October 15, 2020 11:13

xiulipan previously approved these changes Oct 16, 2020
View reviewed changes

keyonjie reviewed Oct 16, 2020
View reviewed changes

Comment threadtest-case/check-volume-levels.sh

keyonjie reviewed Oct 16, 2020
View reviewed changes

marc-hb previously approved these changes Oct 16, 2020
View reviewed changes

marc-hb reviewed Oct 19, 2020
View reviewed changes

Comment threadtest-case/check-volume-levels.sh Outdated

keyonjie previously approved these changes Oct 20, 2020
View reviewed changes

singalsu dismissed stale reviews from keyonjie, marc-hb, and xiulipan via 575bd7bOctober 20, 2020 11:06

singalsu force-pushed the add_volume_levels_check branch from 2005a99 to 575bd7bCompare October 20, 2020 11:06

singalsu requested review from keyonjie, marc-hb and xiulipan October 20, 2020 11:46

marc-hb approved these changes Oct 20, 2020
View reviewed changes

keyonjie approved these changes Oct 21, 2020
View reviewed changes

marc-hb merged commit e4380d1 into thesofproject:masterOct 21, 2020

Uh oh!

Conversation

singalsu commented Oct 6, 2020

Uh oh!

singalsu commented Oct 6, 2020

Uh oh!

singalsu commented Oct 6, 2020

Uh oh!

singalsu commented Oct 6, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu commented Oct 8, 2020

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu commented Oct 9, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-hb commented Oct 12, 2020

Uh oh!

keyonjie commented Oct 13, 2020

Uh oh!

singalsu commented Oct 13, 2020

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

juimonen left a comment • 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!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Oct 15, 2020

Uh oh!

singalsu commented Oct 15, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiulipan left a comment

Choose a reason for hiding this comment

Uh oh!

keyonjie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singalsu commented Oct 9, 2020•
edited
Loading

juimonen left a comment •
edited
Loading

singalsu commented Oct 15, 2020•
edited
Loading