Tools: Add more robustness to check_volume_levels.m measure #985

singalsu · 2022-11-22T19:01:00Z

No description provided.

tools/check_volume_levels.m

singalsu · 2022-11-23T09:57:45Z

@aiChaoSONG @marc-hb I still get these "[WARNING] SOF_TEST_TOP_PID=1531 already defined, multiple lib.sh inclusions?". I see only one line source "$TESTDIR/case-lib/lib.sh" in the script. How can I fix it?

ShriramShastry

Looks good !!

tools/check_volume_levels.m

ShriramShastry

I am fine with the changes. Thank you!!

btian1

not clear with below statement:

This patch delays the check window if the gain settling seems
to be still happening in the beginning of level check window.
The settling is detected with differential of gain.

Could you use short sentence to describe?

aiChaoSONG · 2022-11-25T06:54:40Z

@aiChaoSONG @marc-hb I still get these "[WARNING] SOF_TEST_TOP_PID=1531 already defined, multiple lib.sh inclusions?". I see only one line source "$TESTDIR/case-lib/lib.sh" in the script. How can I fix it?

sof-test will write PID to a lock file in case you run multiple test case at the same time. maybe you interrupt the test, and the lock file not deleted? try manually delete the lock file /tmp/sof-test-cardX.lock, X is the sof card ID.

singalsu · 2022-11-25T18:12:15Z

not clear with below statement:

This patch delays the check window if the gain settling seems to be still happening in the beginning of level check window. The settling is detected with differential of gain.

Could you use short sentence to describe?

By window I mean an acceptance criteria over time and gain in decibels. E.g. x milliseconds to y milliseconds, measured gain within a +/- b decibels. Actually I'm not evaluating every point in window but compute average gain from that measure window and compare to required. Any fail will fail the entire test.

By computing the differential of measured gain, I see from the values if the gain is constant or changing. The added code is shifting the measurement window to later in time if the gain is not constant in in the beginning.

Did this explain better - should I add this to commit message?

marc-hb · 2022-11-28T20:34:57Z

@aiChaoSONG @marc-hb I still get these "[WARNING] SOF_TEST_TOP_PID=1531 already defined, multiple lib.sh inclusions?". I see only one line source "$TESTDIR/case-lib/lib.sh" in the script. How can I fix it?

sof-test will write PID to a lock file in case you run multiple test case at the same time. maybe you interrupt the test, and the lock file not deleted? try manually delete the lock file /tmp/sof-test-cardX.lock, X is the sof card ID.

No, that would be a different error message. SOF_TEST_TOP_PID=1531 already defined really means that the variable SOF_TEST_TOP_PID is already defined which is a serious sof-test coding problem, just look at git grep -C 5 SOF_TEST_TOP_PID and see for yourself.

I don't see how this could be related to matlab changes though. @singalsu is that independent of this PR? Can you reproduce with the current main branch and if yes please share how exactly in a new, different sof-test issue.

singalsu · 2022-11-29T11:40:28Z

I don't see how this could be related to matlab changes though. @singalsu is that independent of this PR? Can you reproduce with the current main branch and if yes please share how exactly in a new, different sof-test issue.

The error is not printed if I remove from test end line sof-kernel-log-check.sh "$KERNEL_CHECKPOINT". It's added in commit 8af0a79. Maybe something that it needs is missing.

marc-hb · 2022-11-29T17:52:34Z

@singalsu if you have a deterministic reproduction without this PR please file a new sof-test bug with complete reproduction steps.

singalsu · 2022-11-30T14:39:28Z

@singalsu if you have a deterministic reproduction without this PR please file a new sof-test bug with complete reproduction steps.

I will but it requires merge of this PR. The script does not proceed to last line where sof-kernel-log-check.sh is called if there is a test fail. Or in other words the Matlab part of test always fails without this improvement. Somehow things (controls) happen in DUT slower than before and failure rate is 100%. Also we don't have tests for control latencies trend, as an idea for future.

singalsu · 2022-11-30T14:51:32Z

@btian1 I improved the commit text of first commit, no other changes, is it OK now?

All, can you please re-review. The previous accepts were dropped from force push.

singalsu · 2022-12-07T17:49:20Z

The PR update adds 4th commit that avoids need for parallel SOF repo checkout.

tools/check_volume_levels.m

singalsu · 2023-02-22T12:36:19Z

@ShriramShastry you approve got cleared, can you please check again. There's only the one " " add to 1st commit.

singalsu · 2023-02-22T14:06:23Z

@marc-hb I just checked that this passed with SOF v2.2 on UP2. I had to make a small change to the shell wrapper since the loopback I2S device in sof-apl-nocodec.tplg is no more hw:0,0 but hw:0,2:

diff --git a/test-case/check-volume-levels.sh b/test-case/check-volume-levels.sh
index 0695320..d819835 100755
--- a/test-case/check-volume-levels.sh
+++ b/test-case/check-volume-levels.sh
@@ -40,7 +40,7 @@ setup_kernel_check_point
 tplg=${OPT_VAL['t']}
 
 TPLGREADER="$TESTDIR"/tools/sof-tplgreader.py
-PCM_ID=0
+PCM_ID=2
 CAP_FORMAT="S16_LE"
 CAP_RATE="48000"
 VOL_P30DB=80

I'd like to make fixes incrementally and address the finding of loopback I2S a separate patch. Its not impacting the Matlab code of this PR.

Also need to make the test to work with a test subset if there is no more mute switch, since those have been removed from IPC4 topologies.

ShriramShastry · 2023-02-23T08:10:44Z

tools/check_volume_levels.m

+	x = zeros(n, 1);
+	x(:,1) = amp(1)*sin(2*pi*f(1)*t);
+	for i=2:length(f)
+		x(:,1) = x(:,1) + amp(i)*sin(2*pi*f(i)*t+ph(i))';


I think x(:,1) is no longer needed , Just x is sufficient
I am thinking it's like below snippet

x = zeros(10,1);
y = linspace(-10,10,10);
x = y + 102.34

y assigns some linearly separated data to replace the loop x equation.

It's now simplified to just x . I was apparently worried about vector orientations when doing this and did not clean up afterwards. The mixing of sine preserved the vector and did not make it a 2D matrix (a pitfall with Matlab language with column and row vectors).

ShriramShastry

A minor comment !! If it is fine with you.

ShriramShastry · 2023-02-23T10:22:30Z

tools/check_volume_levels.m

+
+	x = zeros(n, 1);
+	x(:,1) = amp(1)*sin(2*pi*f(1)*t);
+	for i=2:length(f)


Why is the loop initialized from 2 rather than 1?

It's because the first frequency f(1) sine wave is mixed before the loop. Any additional frequencies f(2) to f(end) are mixed in the loop. But yes, it can be simplified to e.g.

x = zeros(n, 1); for i=1:length(f) x(:,1) = x(:,1) + amp(i)*sin(2*pi*f(i)*t+ph(i))'; end

singalsu · 2023-03-03T10:13:53Z

@ShriramShastry I updated the tone generator function as you proposed. Can you please review again?

I just checked on UP2 sof-apl-nocodec.tplg that the updated code works and test passes so the change was OK.

Next PR for volume checker should be to add support for IPC4 topologies without switch control, and solve the discovery of looped back I2S PCMs, or get the PCM IDs externally.

ShriramShastry

A few sanity-related comments have been added.

Kindly include space where it is needed. The implementation looks to have improved significantly.

Thank you

ShriramShastry · 2023-03-03T12:13:05Z

tools/check_volume_levels.m

+			if max(abs(dg)) > dg_tol
+				n_idx = length(idx);
+				dg_rev = dg(end:-1:1);
+				idx_add = length(dg) - find(abs(dg_rev) > 0.1, 1, 'first') + 1;


Can 0.1 be superseded by dg_tol?

Yep, it can be the same

ShriramShastry · 2023-03-03T12:18:12Z

tools/check_volume_levels.m

 	gains =  meas.levels - lm.sine_dbfs;
 	sv = size(tc.volumes);
 	for j = 1:sv(2)
 		for i = 1:sv(1)
+			% Initial location to test


could you please add a space between ts = tc.vctimes(i)+tc.meas(1); as ts = tc.vctimes(i) + tc.meas(1);

ShriramShastry · 2023-03-03T12:19:11Z

tools/check_volume_levels.m

+% sof/tools/test/audio/test_utils/multitone.m
+function x = multitone( fs, f, amp, tlength )
+	n = round(fs*tlength);
+	t = (0:n-1)/fs;


can you please add space for the operation t = (0:n-1)/fs; like t = (0:n-1) / fs;

Sure, but wouldn't then this be more consistent with even more spaces t = (0 : n - 1) / fs; ?

I admit I'm lazy to type spaces and this kind of not-all-spaces typed equations are everywhere. There is no Matlab style guide and e.g. "indent code" in Matlab IDE is not touching these. Also Octave Mode in Emacs differs from Matlab's indentations. But we can when applicable use SOF C style.

ShriramShastry · 2023-03-03T12:19:49Z

tools/check_volume_levels.m

+	t = (0:n-1)/fs;
+	nf = length(f);
+	if nf > 1
+		ph = rand(nf, 1)*2*pi;


I think we can do a space for rand(nf, 1)2pi; like rand(nf, 1) * 2 * pi;

ShriramShastry · 2023-03-03T12:22:59Z

tools/check_volume_levels.m

+
+	x = zeros(n, 1);
+	for i=1:length(f)
+		x = x + amp(i)*sin(2*pi*f(i)*t+ph(i))';


can you please a space for the equation x = x + amp(i) * sin(2 * pi *f(i) * t + ph(i))';

marc-hb · 2023-03-03T18:19:32Z

Kindly include space where it is needed.

There seems to be a couple matlab beautifiers available, @ShriramShastry can you please recommend one? If no formatter is good then at least some codestyle document reference. Consistency is good but it needs to be well defined, we cannot have codestyle discussions repeats in every pull request.

Also: please do NOT reformat every matlab file and destroy git history. We need a reference for new and modified code, not for old, unchanged code. Too late.
https://wiki.openstack.org/wiki/GitCommitMessages#Things_to_avoid_when_creating_commits

This patch delays the check window if the gain settling seems to be still happening in the beginning of level check window. The settling is detected with differential of gain. The window means acceptance criteria over time as gain in decibels. E.g. x milliseconds to y milliseconds, measured gain within a +/- b decibels. The average gain from the measure window is computed and compare to required. Any fail will fail the entire test. The differential of measured gain shows if the gain is constant or changing. The added code is shifting the measurement window to later in time if the gain is not constant in in the beginning. The delay in gain settling can be caused by random alsamixer controls apply delay. Also a very long gain ramp makes the settling to appear later. Signed-off-by: Seppo Ingalsuo <[email protected]>

If a test fails, it's easiest to examine the result graphically. Pass to command line the failed wav files names and last argument set to one to see the observed levels. Signed-off-by: Seppo Ingalsuo <[email protected]>

Matlab requires switch - end, endswitch is not accepted. Signed-off-by: Seppo Ingalsuo <[email protected]>

This patch adds local functions multitone() and level_dbfs() to script check_volume_levels.m to avoid need for SOF repository. The added functions are small and simple so the duplication is not an issue. Signed-off-by: Seppo Ingalsuo <[email protected]>

There's no functional changes, only added blanks between arithmetic operations, comma separated lists, and ":" operator. Signed-off-by: Seppo Ingalsuo <[email protected]>

singalsu · 2023-03-07T12:20:55Z

@ShriramShastry The last commit contains the style changes for non-impacted code. Sine generator style changes are in the commit that adds it.

marc-hb · 2023-03-10T22:13:56Z

There is no Matlab style guide

https://www.google.com/search?q=matlab+beautify
https://www.google.com/search?q=matlab+formatter

marc-hb · 2023-03-13T16:08:53Z

@ShriramShastry all good now?
If there are any whitespace / style issues left they can addressed later. This PR has been under review for a few months now...

Only suspend/resume failures on one device in https://sof-ci.01.org/softestpr/PR985/build164/devicetest/index.html, everything else OK.

marc-hb · 2023-03-13T17:24:11Z

I learned that @ShriramShastry has been out of office. I looked at the changes since he last approved: https://github.com/thesofproject/sof-test/compare/d79bf2af7aeae751ff728c2585d37677f0663424..5eec1168b4ad26f19a744d69a00ea0fbe620eacf

I found only whitespace changes + this for loop change that he requested earlier https://github.com/thesofproject/sof-test/compare/d79bf2af7aeae751ff728c2585d37677f0663424..1218c95eaa5de33a300073367d823089c9f0b1e5

Merging.

marc-hb · 2023-03-14T04:27:35Z

@singalsu if you have a deterministic reproduction without this PR please file a new sof-test bug with complete reproduction steps.

I will but it requires merge of this PR. The script does not proceed to last line where sof-kernel-log-check.sh is called if there is a test fail. Or in other words the Matlab part of test always fails without this improvement. Somehow things (controls) happen in DUT slower than before and failure rate is 100%. Also we don't have tests for control latencies trend, as an idea for future.

@singalsu can you still reproduce?

marc-hb · 2023-04-17T23:41:32Z

SOF_TEST_TOP_PID=17709 already defined just spotted in totally unrelated:

test-case: rename pause-resume to pause-release #1025 (comment)

singalsu requested a review from a team as a code owner November 22, 2022 19:01

singalsu requested a review from ShriramShastry November 23, 2022 07:57

singalsu commented Nov 23, 2022

View reviewed changes

tools/check_volume_levels.m Outdated Show resolved Hide resolved

singalsu force-pushed the volume_levels_check_add_control_robustness branch from 5c10589 to 06fd766 Compare November 23, 2022 10:31

singalsu requested review from btian1 and andrula-song November 23, 2022 10:33

ShriramShastry reviewed Nov 23, 2022

View reviewed changes

tools/check_volume_levels.m Outdated Show resolved Hide resolved

singalsu force-pushed the volume_levels_check_add_control_robustness branch from 06fd766 to cf00256 Compare November 23, 2022 13:20

ShriramShastry previously approved these changes Nov 23, 2022

View reviewed changes

btian1 reviewed Nov 25, 2022

View reviewed changes

marc-hb mentioned this pull request Nov 28, 2022

[BUG] IPC4 volume windows ramp was broken with latest sof-dev kernel and sof firmware thesofproject/linux#4026

Closed

andrula-song previously approved these changes Nov 29, 2022

View reviewed changes

singalsu dismissed stale reviews from andrula-song and ShriramShastry via 7ca9e2d November 30, 2022 14:49

singalsu force-pushed the volume_levels_check_add_control_robustness branch from cf00256 to 7ca9e2d Compare November 30, 2022 14:49

singalsu requested review from btian1, ShriramShastry and andrula-song November 30, 2022 14:51

singalsu force-pushed the volume_levels_check_add_control_robustness branch from 7ca9e2d to 83bb3fa Compare December 7, 2022 17:47

marc-hb reviewed Dec 7, 2022

View reviewed changes

tools/check_volume_levels.m Show resolved Hide resolved

btian1 previously approved these changes Dec 15, 2022

View reviewed changes

singalsu dismissed btian1’s stale review via d79bf2a February 22, 2023 12:03

singalsu force-pushed the volume_levels_check_add_control_robustness branch from dae511d to d79bf2a Compare February 22, 2023 12:03

singalsu requested review from btian1 and ShriramShastry February 22, 2023 12:35

ShriramShastry reviewed Feb 23, 2023

View reviewed changes

ShriramShastry previously approved these changes Feb 23, 2023

View reviewed changes

ShriramShastry reviewed Feb 23, 2023

View reviewed changes

singalsu dismissed ShriramShastry’s stale review via 1218c95 March 3, 2023 08:58

singalsu force-pushed the volume_levels_check_add_control_robustness branch from d79bf2a to 1218c95 Compare March 3, 2023 08:58

singalsu requested a review from ShriramShastry March 3, 2023 10:10

ShriramShastry reviewed Mar 3, 2023

View reviewed changes

singalsu added 5 commits March 7, 2023 13:56

Tools: Add option to plot levels with check_volume_levels.m

46ac326

If a test fails, it's easiest to examine the result graphically. Pass to command line the failed wav files names and last argument set to one to see the observed levels. Signed-off-by: Seppo Ingalsuo <[email protected]>

Tools: Fix Matlab compatibility for check_volume_levels.m

902ed6d

Matlab requires switch - end, endswitch is not accepted. Signed-off-by: Seppo Ingalsuo <[email protected]>

Tools: Code style changes to check_volume_levels.m

5eec116

There's no functional changes, only added blanks between arithmetic operations, comma separated lists, and ":" operator. Signed-off-by: Seppo Ingalsuo <[email protected]>

singalsu force-pushed the volume_levels_check_add_control_robustness branch from 1218c95 to 5eec116 Compare March 7, 2023 12:16

singalsu requested a review from ShriramShastry March 7, 2023 12:19

marc-hb approved these changes Mar 13, 2023

View reviewed changes

marc-hb merged commit 32d9fe9 into thesofproject:main Mar 13, 2023

Tools: Add more robustness to check_volume_levels.m measure #985

Tools: Add more robustness to check_volume_levels.m measure #985

Conversation

singalsu commented Nov 22, 2022

singalsu commented Nov 23, 2022

ShriramShastry left a comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

btian1 left a comment • edited Loading

Choose a reason for hiding this comment

aiChaoSONG commented Nov 25, 2022

singalsu commented Nov 25, 2022

marc-hb commented Nov 28, 2022

singalsu commented Nov 29, 2022

marc-hb commented Nov 29, 2022

singalsu commented Nov 30, 2022

singalsu commented Nov 30, 2022

singalsu commented Dec 7, 2022

singalsu commented Feb 22, 2023

singalsu commented Feb 22, 2023

ShriramShastry Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShriramShastry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singalsu commented Mar 3, 2023

ShriramShastry left a comment

Choose a reason for hiding this comment

ShriramShastry Mar 3, 2023 • 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

Choose a reason for hiding this comment

ShriramShastry Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

marc-hb commented Mar 3, 2023 • edited Loading

singalsu commented Mar 7, 2023

marc-hb commented Mar 10, 2023

marc-hb commented Mar 13, 2023 • edited Loading

marc-hb commented Mar 13, 2023

marc-hb commented Mar 14, 2023

marc-hb commented Apr 17, 2023

btian1 left a comment •

edited

Loading

ShriramShastry Feb 23, 2023 •

edited

Loading

ShriramShastry Mar 3, 2023 •

edited

Loading

ShriramShastry Mar 3, 2023 •

edited

Loading

marc-hb commented Mar 3, 2023 •

edited

Loading

marc-hb commented Mar 13, 2023 •

edited

Loading