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 <t...@trondeau.com> wrote:
> On Wed, Oct 9, 2013 at 11:01 PM, Achilleas Anastasopoulos
> <anas...@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
>
>
>
> > On Wed, Oct 9, 2013 at 12:59 PM, Achilleas Anastasopoulos
> > <anas...@umich.edu> 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 <t...@trondeau.com> wrote:
> >>>
> >>> On Wed, Oct 9, 2013 at 10:45 AM, Achilleas Anastasopoulos
> >>> <anas...@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
> >>>
> >>>
> >>> > On Wed, Oct 9, 2013 at 9:20 AM, Tom Rondeau <t...@trondeau.com>
> wrote:
> >>> >>
> >>> >> On Tue, Oct 8, 2013 at 9:39 PM, Achilleas Anastasopoulos
> >>> >> <anas...@umich.edu> 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
> >>> >
> >>> >
> >>
> >>
> >
>
_______________________________________________
Discuss-gnuradio mailing list
Discuss-gnuradio@gnu.org
https://lists.gnu.org/mailman/listinfo/discuss-gnuradio
