jgaeddert / liquid-dsp

digital signal processing library for software-defined radios
http://liquidsdr.org
MIT License
1.89k stars 443 forks source link

msresamp2 interpolator not working (0.37021f constant) #148

Closed wluker closed 5 years ago

wluker commented 5 years ago

The multi-stage half-band interpolator is not working as I would expect. With only 1 interpolation stage, the interpolator produces incorrect outputs for frequencies above ~0.36 normalized frequency.

The example msresamp2_crcf_example.c has a magic constant 0.3702 at line 98. (https://github.com/jgaeddert/liquid-dsp/blob/master/examples/msresamp2_crcf_example.c#L98) What is the reason for 0.37?

x[i] = cexpf(_Complex_I*2*M_PI*0.37021f*fc*i) * w;

If I change line 98 to what I would expect:

x[i] = cexpf(_Complex_I*2*M_PI*fc*i) * w;

and then run the example:

./msresamp2_crcf_example -r 2 -f 0.4

The example then shows the sidelobe in error.

./msresamp2_crcf_example -r 2 -f 0.4
msresamp2:
    rate    :     2.00000000
    log2(r) :     1.00000000
    type    :   interp
    stages  :   1
multi-stage half-band resampler:
    type                    : interpolator
    number of stages        : 1 stage
    cut-off frequency, fc   :   0.40000001 Fs
    center frequency, f0    :   0.00000000 Fs
    stop-band attenuation   : 60.00 dB
    stage[ 0]  {m=  6, As= 60.00 dB, fc= 0.200, f0= 0.000}
output results:
  output delay              :     6.00000000 samples
  desired resampling rate   :     2.00000000
  measured resampling rate  :     2.00000000    (240/120)
  peak spectrum             :    -0.41734880 dB (expected 0.0 dB)
  peak frequency            :     0.19921875    (expected 0.20000000  )
  max sidelobe              :   -26.61174965 dB (expected at least -60.00 dB)
results written to msresamp2_crcf_example.m
done.

Specifically max sidelobe : -26.61174965 dB (expected at least -60.00 dB)

I'm assuming this is not a surprise since the 0.037021f is hardcoded in the example. Is this a bug, something that should be in the documentation, or am I just misunderstanding the interpolator?

jgaeddert commented 5 years ago

Good catch. The "magic number" was probably something I added for testing at some point, but clearly there is an issue with determining the length based on the filter cut-off frequency. I'll take a look.

jgaeddert commented 5 years ago

Actually now that I look at the code, the "fc" parameter is to choose the filter cutoff frequency with "As" being the stop-band suppression near that frequency. So putting a tone at this frequency would mean it would potentially have a large image image in the band. I confirmed this was the case with your notes.

I think the "magic number" was back off the carrier from this cut-off a bit. This certainly could be made clearer, and I'll try to address this soon.

wluker commented 5 years ago

I think I am misunderstanding the interface. This makes more sense now. My apologies.

So my assumptions now are...

Does this seem reasonable?

jgaeddert commented 5 years ago

Close, but you're on the right path. The design strategy is to adjust the filter length according to the design constraints of each stage. For example, the interpolator with 3 stages will have a longer filter at the first stage (lowest rate), a shorter filter at the second stage (higher rate, but images are further apart and thus easier to reject), and its shortest filter at its last stage (highest rate, but images are very far apart).

I might be computing the filter parameters incorrectly, though. And the interface doesn't clearly indicate its parameters.

jgaeddert commented 5 years ago

So I found a bug with the msresamp2 that I recently fixed in ac3a84359489c; the issue was that I was running the cascaded half-band filters in the wrong order. This caused some headaches with the msresamp object which was causing some strange behavior with stop-band attenuation. I should probably make the unit tests more aggressive to putting a signal near the edge of the filter pass-bands.

wluker commented 5 years ago

Awesome! Thanks for such a useful library.

jgaeddert commented 5 years ago

I should say, the range of commits here really fixed it: 8cc63fd2c0dffa46a33231c1c5ac856e31720cfe...ac3a84359489c6709da87625217c159c9682a35d