Some misconceptions about the "peak_detector2" block

GNU Radio
1

Hi all,

recently there has been some discussion regarding the peak_detector2
block,
both in the github/gnuradio (pull request 404) as well as in the issue
tracker (issue 783).

It is now well accepted that this block is buggy: there are cases the
work
function returns -1, which is a bug (see issue 783 on how to recreate
this
bug).

I believe however that there is a DEEPER misconception about how this
block
works/should work that has resulted in some frustration on what an
appropriate fix should be.
In particular there is an insistence that an appropriate bug fix should
pass the qa_test of this block and it should be [in the spirit] of the
existing algorithm.
In the following I will explain why passing the qa_test is a
consequence
of the buggy behaviour of this block and NOT its feature.
In addition I will suggest what a proper behaviour of this block should
be,
so that others who may want to write their own version of a peak
detector
find it useful.

So the peak_detector block is very reasonable in its conception and its
name is very informative and appropriate. It works as follows:

Reads the input and keeps track of a running average (through a
single-pole iir filter)

When the current input crosses a threshold (= average * a user-defined
factor) upwards the block enters a search state, where it looks for the
maximum value of the input over a window of user-defined length.

This is clearly the intended behaviour of the block according to the
documentation (I don’t know who the original author is…).

It should be obvious that this block is intended for a scenario where
the
inputs are coming continuously and the block is determining the peaks.
A prototypical application is finding the peak after a MF with a
training
sequence in order to determine the start of a packet
(BTW, there is some effort on creating a correlate and sync block, which
is
very relevant to this discussion).

It should also be obvious that both the pole of the IIR filter, the
threshold, AND the window should be properly selected per application
(eg, for a training sequence of length N, it is appropriate to set the
window ~N) otherwise the block will simply not work according to its
specs.

So here are some questions and my suggested answers:

Q1: How should this block behave when it has already crossed the
threshold
upwards and is still inside the user-defined search window but there is
not
enough input data to process?

A1: Clearly the work function should return (consuming all
inputs/outputs
up to the most current peak–but not more) and hopefully the next time
it
is called, the scheduler would provide more samples so that the search
over
the remaining window is exhausted and the peak is reported.

Q2: And then, what if the scheduler does not give this block more input
data? Maybe because (as in the qa_test) this block is connected to a
source
that outputs a finite (=21 !) number of samples?

A2: Since the intended use of this block is a continuous stream of data,
the above situation is an inappropriate use of this block, and thus it
cannot be considered a test case. This means that an appropriate test
case
should make sure that enough tail data is in the finite input so that
the
search window is exhausted.

Q3: What if the user insists in using the block in a way that is not
intended, ie, by providing it with a finite (and small compared to the
search window) number of inputs. Should there be a way that the block
bails
out?

A3: The current implementation has this behaviour but it is a BUG not a
feature: ERRONEOUSLY it decrements the window even if it does not return
any data, so eventually the window becomes smaller than the available
input
and the block exits!
This results in the block passing an ill-conceived qa_test (where the
window is set to 1000 for a peak occurring within 10 samples of the
threshold crossing). However the intended use of the block was never
this
(according to the documentation).

From the above it should be clear that any fix of this block should NOT
be in the spirit of the original one, nor not passing the existing
qa_test
is a measure of its difference from the original block (at least from
the
stated intended behaviour of the original block).

I attach here a clean modification of this block that has the intended
behaviour. It DOES NOT pass (correctly) the inappropriate qa_test,
but it does pass more appropriately designed test(s) (also attached).

2

Achilleas,

I think you’ve completely failed to understand the issue from my
perspective. I do NOT disagree that there is a bug in the code. I also
do
NOT disagree that most of what you’ve tried to do in the rewrite is the
correct way to rethink the block. What I have a problem with is that
you’ve
provided me with a fix that breaks applications that used to run fine,
including the QA test.

As for the applications, there’s a really simple test you can perform.
Apply the peak detector to a sine wave. In the current code, it finds
the
peak of the sine wave. With the new version, it does not just find that
as
the peak, but it outputs as though it’s found the peak for every length
of
the look-ahead value. This could be a valid design choice in a peak
detector where given a window, always emit the highest value in the
window.
That is not what this block is supposed to do, nor is the new block
behaving consistently in this manner.

As for the QA test, the current tests presents a vector to the block and
the block finds the correct peak. With the new code, it doesn’t
complete.
It just hangs. While it is true that stream-based blocks in GNU Radio
expect a continuous stream of data, any block that simply fails to
complete
its processing when the rest of the flowgraph is done is a bug. Instead,
we
have hooks like set_output_multiple and overloading the forecast
function
that help us work with the scheduler to make sure everyone gets the
right
amount of data they require. In this case, you make a good argument that
the block should look beyond it’s current window to see if the max is in
fact reached. If that’s the case, then we need to have the block tell
the
scheduler this. If less data is passed because there is no more data to
process and the flowgraph is shutting down, this block too must shut
down.
It will then do so without providing the right answer. So the QA test
will
fail – but it will complete and report failure in the data.

Please understand the above points when reworking your fix.

Thanks,

Tom

On Fri, Apr 10, 2015 at 4:22 PM, Achilleas A.
<[email protected]