-
Notifications
You must be signed in to change notification settings - Fork 59
tools: Add a new test for pause/resume #1285
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
base: main
Are you sure you want to change the base?
Conversation
The test runs the specified number of pause/resume iterations on a combination of a playback PCM and a capture PCM looping through all available playback and capture PCM devices available in the SOF audio card. It skips the deep buffer playback PCMs for the time being. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
marc-hb
left a comment
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.
Assuming it's the same feature, there is already a pause/resume test in shell script and expect, see c809fff. I wonder why this new one is needed.
I could be wrong but there is currently no support to compile C code in sof-test before deployment.
No harm in adding "manual" test code of course.
|
@ranj063 do you plan to add building and running the test to this PR? I think it would be important to see its results in the same PR |
kv2019i
left a comment
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.
Initial comments inline. I wonder if some existing tool could be used as well -- could be easier to integrate to sof-test runs in CI (versus starting to build C tools and deploy them to DUTs).
|
|
||
| // if card name doesnt contain "sof", skip it | ||
| if (strstr(snd_ctl_card_info_get_name(card_info), "sof") == NULL) { | ||
| snd_ctl_close(ctl); |
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.
This is not so obvious for the user. We could have SOF based cards that doen't have a "sof" prefix. A print to the user would be helpful.
| @@ -0,0 +1,386 @@ | |||
| #include <stdio.h> | |||
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.
Some license needs to be attached.
| } | ||
| // calculate wait time based on the period size and sample rate | ||
|
|
||
| usleep((playback_framesize / 48000) * 1000000); // Convert to microseconds |
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.
This is fixed to 48000 now, would be good to use a define this as many places in code depend on this.
| cleanup_pcm_handles(); | ||
| break; | ||
| } | ||
| usleep((capture_framesize / 48000) * 1000000); // Convert to microseconds |
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.
This is not good, there should be a call on pcm_avail to block until there is enough data to read. This can get out of sync (CPU versus codec/DSP clock). Same for playback.
| snd_pcm_hw_params_set_format(playback_handle, playback_params_1, SND_PCM_FORMAT_S16_LE); | ||
| snd_pcm_hw_params_set_channels(playback_handle, playback_params_1, 2); | ||
| snd_pcm_hw_params_set_rate(playback_handle, playback_params_1, 48000, 0); | ||
| playback_framesize = BUFFER_SIZE / (2 * 4); // 2 channels, 4 bytes per sample |
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.
These should be separeate defines (channel count and sample format).
marc-hb
left a comment
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 could be wrong but there is currently no support to compile C code in sof-test before deployment.
I wonder if some existing tool could be used as well -- could be easier to integrate to sof-test runs in CI (versus starting to build C tools and deploy them to DUTs).
Actually, doesn't this new test belong to https://github.com/alsa-project rather than sof-test? Please correct me but the only SOF-specific bit in this PR seems to be where the test ignores non-SOF cards (why?) Which cards to test looks like it should be a command line argument, not hard-coded.
Hosting this test in ALSA would solve the deployment problem since ALSA already needs a C compiler for deployment.
cc: @golowanow , @cgturner1
Now you can argue that a lot of sof-test is not that SOF-specific either and could/should be moved to https://github.com/alsa-project too. True but there are important differences:
- sof-test/test-case/ code is not brand new; it's much more difficult to move stuff that already exists
- it's shell and Python and not C code and does not have a C deployment issue. Not sure ALSA maintainers would even accept higher level languages much.
- it would be a challenge to disentangle the non-SOF specific bits while maintaining some stable interface with ALSA. Making sure the combinations of sof-test + ALSA versions deployed stay compatible with each other would be "fun".
The test runs the specified number of pause/resume iterations on a combination of a playback PCM and a capture PCM looping through all available playback and capture PCM devices available in the SOF audio card. It skips the deep buffer playback PCMs for the time being.