Ive also been looking for an appropriate fix for peak_detector2. When I
review this thread and the issue tracker, Im uncertain how the block is
supposed to behave. I think most of the developers have looked at the
documentation in the header file, and have tried to make fixes in
accordance with it. Specifically, that the peak search should only be
restricted to the range within the look_ahead parameter. If so, then
Achilleas is very close to an appropriate fix, needing only some
additional calls to set_output_multiple to prevent hanging. The block
would also work fine with a sine wave, as long as the look ahead value
was appropriate, like half the period.
As mentioned previously, the QA test may not necessarily be useful,
given that the input signal is much smaller than the window. Perhaps
the test file can also be modified to get better results. Im willing to
contribute to a fix and help make the peak detector block more stable,
but I would have to know more about how the block should behave. Any
comments or insights are appreciated.
From: Tom R.
Subject: Re: [Discuss-gnuradio] Some misconceptions about the
Date: Mon, 13 Apr 2015 16:54:47 -0400
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.
On Fri, Apr 10, 2015 at 4:22 PM, Achilleas A.
[email protected] wrote:
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
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…).