Re: [Discuss-gnuradio] Bug in freq_xlating_fir_filter_XXX

[Achilleas Anastasopoulos]

I stand corrected.

Everything works fine with the new patch now!

thanks for the help,
Achilleas

On Thu, Oct 10, 2013 at 11:22 AM, Tom Rondeau <address@hidden> wrote:

On Wed, Oct 9, 2013 at 11:01 PM, Achilleas Anastasopoulos
<address@hidden> 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



> On Wed, Oct 9, 2013 at 12:59 PM, Achilleas Anastasopoulos
> <address@hidden> wrote:
>>
>> 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
>>
>>
>>
>> On Wed, Oct 9, 2013 at 11:02 AM, Tom Rondeau <address@hidden> wrote:
>>>
>>> On Wed, Oct 9, 2013 at 10:45 AM, Achilleas Anastasopoulos
>>> <address@hidden> 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
>>>
>>>
>>> > On Wed, Oct 9, 2013 at 9:20 AM, Tom Rondeau <address@hidden> wrote:
>>> >>
>>> >> On Tue, Oct 8, 2013 at 9:39 PM, Achilleas Anastasopoulos
>>> >> <address@hidden> wrote:
>>> >> >
>>> >> > 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
>>> >>
>>> >> 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
>>> >
>>> >
>>
>>
>