Use aplay --fatal-errors to stop on underruns or errors

[plbossart]

In test scripts, we often have cases where there is no issue reported in firmware or driver logs, but nasty error messages in userspace, e.g. pcm_write error, bad descriptor, etc.

aplay provides a --fatal-errors command line option, and when set will stop processing and return an error code.

I would think this is really required for CI/QA tests (except maybe in rare resume cases where a .prepare operation is unavoidable). We would also need to test what aplay returns.

Rather than forcing this option in the scripts, I would prefer an environment variable, same as KCFLAGS='-Werror' when compiling

cc:

• [FEATURE] use --fatal-errors for aplay/arecord tests #1120
• Catch firmware errors reported in logs #1173

[plbossart]

@marc-hb @kv2019i @aiChaoSONG @xiulipan what do you think?

[marc-hb]

On one hand, anyone serious and professional about validation should love every possibility to find more errors because that's the whole purpose of testing. I didn't know this option existed, I think it should have been used in tests always.

We most likely want to combine this with set -e #312 for it to be really useful.

On the other hand, the main purpose of Continuous Integration is to find new regressions, NOT to constantly report the same known bugs. When some PR check is red most of the time then most people stop looking at it. The only error example listed inman aplay and man arecord is "xrun" and I'm afraid (and sad) that these would be both non-deterministic and frequent enough to make entire CI results too painful to use?

Rather than forcing this option in the scripts, I would prefer an environment variable, same as KCFLAGS='-Werror' when compiling

Something optional/flexible would be a great way to start. Unfortunately aplay does not support empty arguments like aplay "$maybe_empty" so this would have to be aplay $maybe_empty which implies a lot of shellcheck warnings to manually disable :-(

Is there no ~/.aplayrc of some kind?

[plbossart]

@marc-hb if we get an xrun, we should stop. period. There's no point in continuing further, something really bad just happened and ignoring it is not an option. If we don't then we push the detection of issues to the alsa-bat case, where we will indirectly detect the xrun with a bad THD level.

We could ignore errors to see how resilient the error handling is, but that's a different test IMHO.

We can address the issues above with aplay by having a variable instead of a hard-coded 'aplay'.

[marc-hb]

We can address the issues above with aplay by having a variable instead of a hard-coded 'aplay'.

$aplay_opts ... would require the same shellcheck disable verbosity but my_sof_aplay_function() should solve this.

if we get an xrun, we should stop. period. There's no point in continuing further, something really bad just happened and ignoring it is not an option.

Sorry I assumed a very small amount of underrun was temporarily tolerable while working on bigger issues/bugs.

In that case why even bother making this optional? Let's just gradually (as in "continuous" integration) add --fatal-errors to each test directly in the test code, no indirection. Developers can easily comment it out locally if needed. Why would anyone or anything need to disable it across the whole test suite?

[plbossart]

In that case why even bother making this optional? Let's just gradually (as in "continuous" integration) add --fatal-errors to each test directly in the test code, no indirection. Developers can easily comment it out locally if needed. Why would anyone or anything need to disable it across the whole test suite?

I found out about this yesterday, we need to experiment on what it actually does, possibly with xrun-injection (which I am also not familiar with)
For example, I thought errors would be reported on suspend-resume, but no error is reported.

root@plb:~# aplay --fatal-errors -Dhw:0,2 -c2 -r48000 -f dat /dev/random
Playing raw data '/dev/random' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
Suspended. Trying resume. Failed. Restarting stream. Done.
Suspended. Trying resume. Failed. Restarting stream. Done.
Suspended. Trying resume. Failed. Restarting stream. Done.
^CAborted by signal Interrupt...

The point is really to try to have more data points, currently we have limited information on what happens for known failures, e.g.
#472

[marc-hb]

Got it: it's not just underruns and the rest is unknown.

[aiChaoSONG]

@plbossart As far as I know, there is xrun recovery mechanism in the kernel or alsa-lib, if an xrun is not recovered, I think there may be some hidden issue. Please correct me if I am wrong.

[kv2019i]

@plbossart I think it makes sense to enable '--fatal-errors'. In all our test cases, we have such large buffers (i.e. we are not running with buffer-size of <5ms), that xruns should not happen. Only bigger concern and possible issue if deployed in CI are tests where we suspend while streams are active. It seems based on #489 (comment) , aplay already handles and not raise this case as an error (which is correct, the streaming will be interrupted for suspend).

[plbossart]

@plbossart As far as I know, there is xrun recovery mechanism in the kernel or alsa-lib, if an xrun is not recovered, I think there may be some hidden issue. Please correct me if I am wrong.

the concept of 'recovery' is misleading. ALSA lib essentially stops and restart the pipeline, so there will be a user impact. That's really not supposed to happen, it's a real bug. We shall not accept underflows and xrun as normal.

[plbossart]

@plbossart I think it makes sense to enable '--fatal-errors'. In all our test cases, we have such large buffers (i.e. we are not running with buffer-size of <5ms), that xruns should not happen. Only bigger concern and possible issue if deployed in CI are tests where we suspend while streams are active. It seems based on #489 (comment) , aplay already handles and not raise this case as an error (which is correct, the streaming will be interrupted for suspend).

yep, it's not like we are testing in low-latency cases (which is a separate topic that at some point we'll have to address buffer sizes and not use the defaults).

My problem is that I can't figure out how to go about this change, it's shell coding at its worst and I'll probably break more things. Someone from the 'CI team' needs to look into this.

[aiChaoSONG]

@xiulipan any idea? If no objection, I can help with this step by step.

[marc-hb]

I have something working already