Skip to content

Tools: Test: Audio: Valgrind run related fixes #7719

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.

Sign up for GitHub

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

Merged
kv2019i merged 2 commits into from
Jun 6, 2023
Merged

Tools: Test: Audio: Valgrind run related fixes #7719

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc9a31b
Tools: Test: Audio: Valgrind run related fixes
singalsu May 31, 2023
127e98f
Tools: Test: Audio: Fix syntax error in test_run.m
singalsu Jun 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tools/test/audio/process_test.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,9 @@
test.thdnf_max = []; % Set per component
Copy link
Collaborator

@kv2019i kv2019i Jun 5, 2023

Choose a reason for hiding this comment

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

Minor nit: the "Fixes commit-id" tag not quite following Linux style.

If your patch fixes a bug in a specific commit, e.g. you found an issue using
``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
the SHA-1 ID, and the one line summary.  Do not split the tag across multiple
lines, tags are exempt from the "wrap at 75 columns" rule in order to simplify
parsing scripts.  For example::

»       Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")

Copy link
Collaborator

@marc-hb marc-hb Jun 5, 2023

Choose a reason for hiding this comment

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

There are git scripts that scan branches for that specific Fixes: syntax.

Copy link
Collaborator Author

@singalsu singalsu Jun 6, 2023

Choose a reason for hiding this comment

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

The commit message is now fixed. You may want to re-review because of 2nd commit added to fix the syntax error found by Sriram. And strange that it didn't error.

test.dr_db_min = 80; % Min. DR
test.fr_rp_max_db = 0.5; % Allow 0.5 dB frequency response ripple

% No need to collect trace
test.trace = '';
Copy link
Contributor

@ShriramShastry ShriramShastry Jun 5, 2023

Choose a reason for hiding this comment

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

At line number 101, tn = 1;, tn is not used; please consider using it or removing it.

At line number 321, min_bits = min(test.bits_in, test.bits_out);, do you intend to use minimum bits elsewhere ? Otherwise, it's unnecessary code that should be removed.

Copy link
Collaborator

@marc-hb marc-hb Jun 5, 2023

Choose a reason for hiding this comment

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

@ShriramShastry how is this related to this PR?

Copy link
Contributor

@ShriramShastry ShriramShastry Jun 6, 2023

Choose a reason for hiding this comment

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

It has nothing to do with PR and does not rely on PR to succeed. A little correction can help greatly improve code quality, and I believe Seppo won't mind (because he wrote the majority of the code) considering the proposal.

Copy link
Collaborator

@marc-hb marc-hb Jun 6, 2023

Choose a reason for hiding this comment

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

These look like interesting suggestions but they're still off-topic here.

Copy link
Contributor

@ShriramShastry ShriramShastry Jun 6, 2023

Choose a reason for hiding this comment

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

Hmm - sure it's off the topic, what are your thoughts if you notice improvement and believe that rectification would be beneficial without adding extra work? I am not sure if the chosen approach is OK or NOT, and for the PR, I request Seppo to consider but leave the decision to him.

Copy link
Collaborator Author

@singalsu singalsu Jun 6, 2023

Choose a reason for hiding this comment

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

Thanks for suggestions. They are better to leave to another PR later. If you like you are welcome to make a PR since you found them. Or I can do later when time, as you prefer.

Copy link
Contributor

@ShriramShastry ShriramShastry Jun 6, 2023 •
edited by marc-hb
Loading

Choose a reason for hiding this comment

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

I would want to propose that you incorporate it as well as as you can take over

Thank you!!

Copy link
Collaborator

@marc-hb marc-hb Jun 6, 2023

Choose a reason for hiding this comment

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

Hmm - sure it's off the topic, what are your thoughts if you notice improvement and believe that rectification would be beneficial without adding extra work?

The very first thing when making slightly off-topic comments is to clearly warn that they are off-topic to save confusion and time. A simple "off-topic" prefix is enough. Try putting yourself in the shoes of maintainers who have to review a lot of PRs and switch context many times per day.

If some comments are totally off-topic then there are many other places. Email or instant messaging of course, or you can submit a throw-away, draft PR, or you can start a new github issue or discussion, maybe a github gist. You can also git blame the code and comment in the old, merged PR where the code appeared in the first place. Some people don't like the last option (I do). There are really a lot of options that don't noise and slow down unrelated PRs.

Copy link
Collaborator

@marc-hb marc-hb Jun 6, 2023

Choose a reason for hiding this comment

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

https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html

end

function test = test_run_process(test)
Expand Down
10 changes: 6 additions & 4 deletions tools/test/audio/test_utils/test_run.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
test.nch_in = test.nch;
end

fprintf(fh, '#!/bin/sh\n', test.comp);
fprintf(fh, '#!/bin/sh\n');
fprintf(fh, 'COMP=\"%s\"\n', test.comp);
fprintf(fh, 'DIRECTION=playback\n');
fprintf(fh, 'BITS_IN=%d\n', test.bits_in);
Expand Down Expand Up @@ -92,14 +92,16 @@
fprintf(fh, 'XTRUN=\n');
end

% Override defaults in comp_run.sh
fprintf(fh, 'VALGRIND=false\n');
fclose(fh);

arg = sprintf('-t %s', fn_config);
rcmd = sprintf('%s %s', ex, arg);
fprintf('Running ''%s''...\n', rcmd);
system(rcmd);
ret = system(rcmd);
if ret
error('''%s'' returned status %d\n', rcmd, ret);
end

Copy link
Contributor

@ShriramShastry ShriramShastry Jun 5, 2023

Choose a reason for hiding this comment

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

At line number 58, fprintf(fh, '#!/bin/sh\n', test.comp); Isn't the output format strange?

Copy link
Collaborator

@marc-hb marc-hb Jun 5, 2023

Choose a reason for hiding this comment

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

Good catch: it does look like a bug but how is it related to this PR?

Copy link
Collaborator Author

@singalsu singalsu Jun 6, 2023

Choose a reason for hiding this comment

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

Yep, I can add a fix commit to this PR. Strange that this is not causing errors.

delete(fn_config);

end