Skip to content

test-case: add equalizer test case - #63

Merged
xiulipan merged 1 commit into
thesofproject:masterfrom
fredoh9:fix/add_eq_test
Mar 19, 2020
Merged

xiulipan merged 1 commit into
thesofproject:masterfrom
fredoh9:fix/add_eq_test

Conversation

@fredoh9

@fredoh9 fredoh9 commented Jan 31, 2020

Copy link
Copy Markdown
Contributor

Tested with sof-apl-eq-pcm512x.tplg on APL_UP2_PCM512X.

Signed-off-by: Fred Oh fred.oh@linux.intel.com

@fredoh9
fredoh9 force-pushed the fix/add_eq_test branch 2 times, most recently from f91442a to 2c9d792 Compare January 31, 2020 18:53
@fredoh9

fredoh9 commented Jan 31, 2020 •

Copy link
Copy Markdown
Contributor Author

Updated with getting amixer control id from the system.

In this case, IIR control id=23, FIR control id=24

ubuntu@jf-fred-up2-pcm512x:~$ amixer controls | grep EQ
numid=24,iface=MIXER,name='EQFIR1.0 EQFIR'
numid=23,iface=MIXER,name='EQIIR1.0 EQIIR'

@fredoh9
fredoh9 requested a review from singalsu January 31, 2020 18:56
stripes416
stripes416 previously approved these changes Jan 31, 2020

@stripes416 stripes416 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Bin-QA Bin-QA left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you need change eqConfRoot as the parameter and add come description to say which file in it
because for the different platform we need to push different file, is that right?

singalsu
singalsu previously approved these changes Feb 3, 2020

@singalsu singalsu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Thanks!

@xiulipan xiulipan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@fredoh9 could we have some check before the test goes on? let finish your TODO

Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
@xiulipan

Copy link
Copy Markdown
Contributor

@fredoh9 Could you check with #10 and add ENV check before start to test with EQ?

@fredoh9
fredoh9 dismissed stale reviews from singalsu and stripes416 via b8c824a February 26, 2020 21:39
@fredoh9
fredoh9 force-pushed the fix/add_eq_test branch 2 times, most recently from d384134 to 108bfd5 Compare February 27, 2020 19:24
@fredoh9
fredoh9 requested a review from xiulipan February 27, 2020 19:26
@fredoh9

fredoh9 commented Feb 27, 2020

Copy link
Copy Markdown
Contributor Author

@aiChaoSONG @libinyang @xiulipan I added more checking with tplgtool. I addressed your commments also. can you review again?

Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved

@xiulipan xiulipan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please make the check more easy and in right order.
Be careful with dir path and hard coded list.
All other looks good.

Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
@fredoh9
fredoh9 requested a review from xiulipan March 13, 2020 06:40

@xiulipan xiulipan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The test part is OK, but we still need to refine the preparation check.

Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
Comment thread test-case/check-audio-equalizer.sh Resolved
@fredoh9
fredoh9 force-pushed the fix/add_eq_test branch 2 times, most recently from 0c4ec81 to a9d4da1 Compare March 17, 2020 09:06
Check currnet topology has eq components otherwise return failure. Query
IIR/FIR control id from amixer and set predefined config using eqctl/*.txt.
Tested with sof-apl-eq-pcm512x.tplg on APL_UP2_PCM512X.

Signed-off-by: Fred Oh <fred.oh@linux.intel.com>
@fredoh9
fredoh9 requested a review from xiulipan March 17, 2020 09:20

@stripes416 stripes416 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This LGTM. Good job @fredoh9

@xiulipan
xiulipan merged commit cb85ac9 into thesofproject:master Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants