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
On Tue, Oct 8, 2013 at 9:39 PM, Achilleas A.
[email protected] 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
On Wed, Oct 9, 2013 at 10:45 AM, Achilleas A.
[email protected] 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
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
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
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 A.
<[email protected]
I stand corrected.
Everything works fine with the new patch now!
thanks for the help,
Achilleas
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
On Thu, Oct 10, 2013 at 11:53 AM, Achilleas A.
[email protected] 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 H.'s blog post from the other day).
Thanks for pointing this out!
Tom
On Wed, Oct 9, 2013 at 11:01 PM, Achilleas A.
[email protected] 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;
for(unsigned int i = 0; i < d_proto_taps.size(); i++) {
ctaps[i] = d_proto_taps[i] * exp(gr_complex(0, i * fwT0));
}
Tom