Forum: GNU Radio Bug in freq_xlating_fir_filter_XXX

C369ebbe1655201988e376af57460f23?d=identicon&s=25 Achilleas Anastasopoulos (Guest)
on 2013-10-09 03:40
(Received via mailing list)
I was playing around with

fir_filter_XXX

and

freq_xlating_fir_filter_XXX

and noticed that the two do not give the same output
for the same input (and center_freq=0 in the xlating filter).

Looking at the implementation of the latter
it is obvious why: the taps are reversed in the line:

std::reverse(ctaps.begin(), ctaps.end());

For consistency the taps should not be reversed (as in all other
filters)
We also need to set

float fwT0 = 2 * M_PI * d_center_freq / d_sampling_freq;

(instead of the minus sign in the code).

unless there is an objection, I will go ahead and push a correction,
Achilleas
C539637020fd56193dd6daec746c4a84?d=identicon&s=25 Tom Rondeau (Guest)
on 2013-10-09 15:21
(Received via mailing list)
On Tue, Oct 8, 2013 at 9:39 PM, Achilleas Anastasopoulos
<anastas@umich.edu> wrote:
> for the same input (and center_freq=0 in the xlating filter).
>
> Looking at the implementation of the latter
> it is obvious why: the taps are reversed in the line:
>
> std::reverse(ctaps.begin(), ctaps.end());
>
> For consistency the taps should not be reversed (as in all other filters)
> We also need to set

Yes, please submit a patch for this. The taps are reversed inside the
fir filters, so this is redundant and confusing. Most people probably
use symmetric filter taps, which is why it has not been found.

> float fwT0 = 2 * M_PI * d_center_freq / d_sampling_freq;
>
> (instead of the minus sign in the code).
>
> unless there is an objection, I will go ahead and push a correction,
> Achilleas

Don't change the sign of the frequency. I know this is controversial,
but from my experience with users, more people find the current way
easier to understand. We're telling the filter what the center
frequency is, which means that we will take a signal at Fc and
downshift it to DC. To me, if we're on carrier Fc and we specify -Fc
as the "Center Frequency", that's more confusing.

Tom
C369ebbe1655201988e376af57460f23?d=identicon&s=25 Achilleas Anastasopoulos (Guest)
on 2013-10-09 16:46
(Received via mailing list)
I will submit the patch.

regarding the sign change in frequency, I didn't mean to change the
convention:
the sign change IS REQUIRED in order to KEEP the convention because now
the taps are not reversed...

Achilleas
C539637020fd56193dd6daec746c4a84?d=identicon&s=25 Tom Rondeau (Guest)
on 2013-10-09 17:04
(Received via mailing list)
On Wed, Oct 9, 2013 at 10:45 AM, Achilleas Anastasopoulos
<anastas@umich.edu> wrote:
> I will submit the patch.
>
> regarding the sign change in frequency, I didn't mean to change the
> convention:
> the sign change IS REQUIRED in order to KEEP the convention because now
> the taps are not reversed...
>
> Achilleas

Sorry, Achilleas, I'm not seeing it. In the common case of a symmetric
FIR filter, the reverse function doesn't change any behavior, but the
minus sine definitely does.

I don't see how reversing the order of the filter taps and changing
the sign have anything to do with each other.

Tom
C369ebbe1655201988e376af57460f23?d=identicon&s=25 Achilleas Anastasopoulos (Guest)
on 2013-10-09 19:00
(Received via mailing list)
Maybe I am wrong, but here is the idea:

the original taps are "taps".
then inside the freq_xlating filter new "combined" taps are generated
as follows

comb_t = taps_t *exp(-j A t)

then the COMBINED filter is reversed.
The effect of that is that essentially we have the filter

reversed_t = taps_{-t} *exp( + j A t)

If we drop the reversing part from the process (commenting out one line
of
code) we will end up
with the filter nonreversed with

nonrevered_t = comb_t = taps_t *exp(-j A t)

Comparing the reveresed and non-reversed we see that even when taps are
symmetric, the frequency sign gas changed so we need to change it back!

let me know if i am missing something,
Achilleas
C369ebbe1655201988e376af57460f23?d=identicon&s=25 Achilleas Anastasopoulos (Guest)
on 2013-10-10 05:01
(Received via mailing list)
I attach the patch for this correction
(for some reason I cannot git push...)

Achilleas


On Wed, Oct 9, 2013 at 12:59 PM, Achilleas Anastasopoulos
<anastas@umich.edu
C539637020fd56193dd6daec746c4a84?d=identicon&s=25 Tom Rondeau (Guest)
on 2013-10-10 17:24
(Received via mailing list)
On Wed, Oct 9, 2013 at 11:01 PM, Achilleas Anastasopoulos
<anastas@umich.edu> wrote:
> I attach the patch for this correction
> (for some reason I cannot git push...)
>
> Achilleas

Sorry for the delay getting back to you. I walked through the math
myself but couldn't find where you were wrong, but I knew this patch
changed the sign of frequency translation. Just try it with a sine
wave a 1 kHz and set the Center Frequency to 1 kHz. The signal out is
not at 2 kHz.

Turns out we were both missing something. This just spins the filter
from baseband to bandpass around fwT0, but there's still the rotator
(d_r) that is responsible for spinning the output down.

Basically, we're changing x(t) -> (mult by -fwT0) -> LPF -> y(t)
Into: x(t) -> BPF -> (mult by fwT0) -> y(t)

(The block is also taking into account downsampling that's not
accounted for above such that we downsample in the filter before down
shifting in frequency.)

So this, I think, is the correct patch:

diff --git a/gr-filter/lib/freq_xlating_fir_filter_XXX_impl.cc.t
b/gr-filter/lib/freq_xlating_fir_filter_XXX_impl.cc.t
index 72a2c05..f3c8ffd 100644
--- a/gr-filter/lib/freq_xlating_fir_filter_XXX_impl.cc.t
+++ b/gr-filter/lib/freq_xlating_fir_filter_XXX_impl.cc.t
@@ -77,14 +77,15 @@ namespace gr {
     {
       std::vector<gr_complex> ctaps(d_proto_taps.size());

-      float fwT0 = -2 * M_PI * d_center_freq / d_sampling_freq;
+      //float fwT0 = -2 * M_PI * d_center_freq / d_sampling_freq;
+      float fwT0 = 2 * M_PI * d_center_freq / d_sampling_freq;
       for(unsigned int i = 0; i < d_proto_taps.size(); i++) {
        ctaps[i] = d_proto_taps[i] * exp(gr_complex(0, i * fwT0));
       }

-      std::reverse(ctaps.begin(), ctaps.end());
+      //std::reverse(ctaps.begin(), ctaps.end());
       d_composite_fir->set_taps(ctaps);
-      d_r.set_phase_incr(exp(gr_complex(0, fwT0 * decimation())));
+      d_r.set_phase_incr(exp(gr_complex(0, -fwT0 * decimation())));
     }

     void


Tom
C369ebbe1655201988e376af57460f23?d=identicon&s=25 Achilleas Anastasopoulos (Guest)
on 2013-10-10 17:54
(Received via mailing list)
I stand corrected.
Everything works fine with the new patch now!

thanks for the help,
Achilleas
F762b223a87f868db769f9a297fe976c?d=identicon&s=25 Mike W. (mike_w)
on 2013-10-10 18:28
I have been struggling with the same problems now I have finally got
audio blocks working by using the Build_Gnu_radio script and not
pybombs.

I have two identical GRC applications running on two computers (one
virtual) implementing a DBPSK demodulator reading from a stored sample
file. With 3.6.5.1 the demodulator works perfectly. With 3.7.2 it does
not work very well, if at all, the error rate is much much higher.

Now I have corrected for the file source / frequency translation sign
error, I still can't make it work - so I assume something has regressed
in either the low pass filter block or the DBPSK demodulator block.

Mike
C539637020fd56193dd6daec746c4a84?d=identicon&s=25 Tom Rondeau (Guest)
on 2013-10-10 18:53
(Received via mailing list)
On Thu, Oct 10, 2013 at 11:53 AM, Achilleas Anastasopoulos
<anastas@umich.edu> wrote:
> I stand corrected.
> Everything works fine with the new patch now!
>
> thanks for the help,
> Achilleas

Ok, updated, tested (with updated and improved QA code) and pushed.

Also added a bit of an explanation to the build_composite_fir (as per
Ben Hilburn's blog post from the other day).

Thanks for pointing this out!

Tom
Please log in before posting. Registration is free and takes only a minute.
Existing account

NEW: Do you have a Google/GoogleMail, Yahoo or Facebook account? No registration required!
Log in with Google account | Log in with Yahoo account | Log in with Facebook account
No account? Register here.