Re: [Discuss-gnuradio] Bug in freq_xlating_fir_filter_XXX

[Tom Rondeau]

On Thu, Oct 10, 2013 at 11:53 AM, Achilleas Anastasopoulos
<address@hidden> 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



> 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
>> >>> >
>> >>> >
>> >>
>> >>
>> >
>
>