test-case: add case for DSM feature test

[aiChaoSONG]

• add a wavetool for DSM/WOV/EchoReference feature test (used to analyze wave)
• add DSM test case.

rerequested

[marc-hb]

Before we spend too much time reviewing it, what does this new Python tool do that ALSA BAT doesn't already do?
https://github.com/alsa-project/alsa-utils/blob/master/bat/alsabat.1
https://www.alsa-project.org/wiki/BAT

[aiChaoSONG]

Before we spend too much time reviewing it, what does this new Python tool do that ALSA BAT doesn't already do?
https://github.com/alsa-project/alsa-utils/blob/master/bat/alsabat.1
https://www.alsa-project.org/wiki/BAT

This tool is intended for DSM/WOV test.

• For DSM test, we need to binary compare recorded wave and reference wave, before comparison, we need to trim zeros of recorded wave.
• For WOV test, we use this tool to generate special designed wave, and analyze recorded wave to ensure data integrity in history buffer.
• For future use, I decide to add more complex wave analysis for EQ test.

[marc-hb]

I'm still a bit surprised that tool(s) to perform these sorts of analysis don't already exist but I'm new to this project.

It's nice to split the new tool and the test case in two different commits but I think this doesn't go far enough. Creating a generic tool like this from scratch is a pretty big, long term commitment so it should be more visible and deserves its own PR, not semi-hidden behind the addition of a new test case. Maybe it should be located in sof/tools/ to be even more visible?

[aiChaoSONG]

@marc-hb I'd like to remove vars and use 'args.amp', but for the int8, int16, I think S8, S16 and S32 is better, because aplay and arecord use S16_LE, S32_LE, etc. It's better to add a layer in wavetool (S32_LE->int32) than add a layer in test case

[aiChaoSONG]

@marc-hb Almost every function in wavetool use command line parameters, pass a argparse Namespace class is no better than pass a python dict. at least **cmd denote multiple parameters. I suggest we keep the dict implemetation.

[marc-hb]

pass a argparse Namespace class is no better than pass a python dict.

I agree there's probably not much technical difference (the dict implementation is just a bit more verbose); the problem is serious reader confusion. It took me a very long time and multiple rounds of reviews before I realized all these dictionaries were actually just args, same thing as in every Python script but in disguise here. I kept looking for some variation(s) that I never found.

Almost every function in wavetool use command line parameters,

That's exactly what took me a very long time to realize.

In fact it would be clearer if these functions didn't even take args parameters at all but use a global instead. The idea of function parameters is that they change between invocations whereas args are constants while the script runs.

at least **cmd denote multiple parameters

args or cmd_args also denotes that and much more: exactly where it comes from.

I suggest we keep the dict implemetation.

I suggest a large but very simple and mechanical search/replace requiring very little testing.

Finally, based on the commit message you seem to want this new tool to last and grow:

Add a wave tool for wave generation, wave comparison
and analysis.

This initial verison only supports sinusoids generation and
bit-by-bit comparison between recorded wave and reference
wave.

So let's start with a "clean slate"

[marc-hb]

2's complement underflow bug below

[singalsu]

The silence trim needs some work. My other comments are minor or questions because I'm not familiar with this test.

[aiChaoSONG]

@singalsu Three procedures in the DSM test case:

• generate playback wave, which is a sine wave.
• play sine wave through DSM playback pipeline and recorded it back through DSM capture pipeline
• do binary comparison between playback wave and captured wave.

There is no processing in the DSM component, just copy the playback data back to the capture pipeline.

What the wavetool.py did is just to do binary comparison, to be short, compare S16/S24/S32 value sample by sample.
As the recorded wave is not aligned, so need to trim zero of two sides to make it aligned with playback wave.
In the DSM case, comparison to zero should define if a value is zero, But in WOV(which will be implement next) test, because ADC/DAC is present, we need a zero threshold.

[keyonjie]

LGTM, thanks @aiChaoSONG

review all closed

[aiChaoSONG]

Thanks Marc, Seppo for very useful reviews, educated me a lot. I am going to merge this as WOV test case is en route. Any further review will be applied as bug fix or improvement. Thank you all.

[marc-hb]

I just tried to generate a signal and compare it with itself and it fails like this. @aiChaoSONG how did you test this?

$ ./tools/wavetool.py -gsine && ./tools/wavetool.py -a smart_amp -r tmp.wav -R tmp.wav 

Traceback (most recent call last):
  File "./tools/wavetool.py", line 160, in <module>
    main()
  File "./tools/wavetool.py", line 157, in main
    do_wave_analysis(**cmd_args)
  File "./tools/wavetool.py", line 83, in do_wave_analysis
    analyze_wav_smart_amp(wave, ref_wave, fs_wav, cmd['zero_threshold'])
  File "./tools/wavetool.py", line 113, in analyze_wav_smart_amp
    trimed_wave_ch_2_3, delay2 = trim_wave(wave[:,2:4], zero_threshold)
  File "./tools/wavetool.py", line 91, in trim_wave
    wave_mono = wave[:,0]
IndexError: index 0 is out of bounds for axis 1 with size 0

[aiChaoSONG]

@marc-hb I guess your waves are both 2 channels, right? For the DSM, the recorded wave is 4 channels, so you need -R recorded-four-channel-wave -r two-channel-reference-wave.

[marc-hb]

@marc-hb I guess your waves are both 2 channels, right?

You don't need to guess, you can just run the command I gave.

so you need -R recorded-four-channel-wave -r two-channel-reference-wave.

Indeed sof-test/test-case/check-smart-amplifier.sh seems to generate a reference with two channels.

That's quite confusing, I thought this wavetool was meant to become less specialized.

Now I understand the duplication in analyze_wav_smart_amp() better. Can you at least create a function that compares two channels and removes this duplication? (after reviewing and merging this cleanup #334). I bet it will make the backtrace above a bit clearer.