# Re: Some misconceptions about the "peak\_detector2" block

**URL:** <https://www.ruby-forum.com/t/re-some-misconceptions-about-the-peak-detector2-block/241637>\
**Category:** GNU Radio\
**Created:** [April 20, 2015, 7:09pm UTC](https://www.ruby-forum.com/t/re-some-misconceptions-about-the-peak-detector2-block/241637 "2015-04-20T19:09:10Z")\
**Posts on this page:** 4\
**Page:** 1

<div class="post-metadata">

**Author:** ![Detlef\_R](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/d/58f4c7/32.png) [@Detlef\_R](https://www.ruby-forum.com/u/Detlef_R)\
**Post date:** [April 20, 2015, 7:09pm UTC](https://www.ruby-forum.com/t/re-some-misconceptions-about-the-peak-detector2-block/241637/1 "2015-04-20T19:09:10Z")

</div>

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.

Frank Fu

* * *

From: Tom R.  
Subject: Re: [Discuss-gnuradio] Some misconceptions about the  
“peak\_detector2” block  
Date: Mon, 13 Apr 2015 16:54:47 -0400  
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.  
[address@hidden](mailto:address@hidden) wrote:  
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…).

---

<div class="post-metadata">

**Author:** ![Frank\_Fu](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/f/b5a626/32.png) [@Frank\_Fu](https://www.ruby-forum.com/u/Frank_Fu)\
**Post date:** [April 21, 2015, 8:25pm UTC](https://www.ruby-forum.com/t/re-some-misconceptions-about-the-peak-detector2-block/241637/2 "2015-04-21T20:25:43Z")

</div>

Thanks for your response, Tom. So basically, the documentation needs to  
be updated to better reflect the desired behavior of the block. My  
interpretation of the behavior would be something like The look\_ahead  
value creates a minimum search window. The peak is first searched  
within this window. If the last sample in this window is the peak, the  
block continues searching for the peak until the input flattens or falls  
down.

So, I can see this block containing three states: peak not found, peak  
search in window, and extended peak search. The current trouble, as I  
see it, is during the window search phase. A lot of buggy things happen  
when the window search isnt fully completed within a work call. If  
set\_output\_multiple is not desired, how about creating a saved output  
buffer of length look\_ahead? During the window search phase, the output  
state could be saved until enough inputs come in. The block would have  
to use general\_work instead, because there would be a temporary point  
where input\_samples are consumed, but no outputs are written.

Finally, I hope you would consider creating a third peak detector block  
that would perform the finite window search. It would be extremely  
useful, as it solves many kinds of signal detection problems for a  
receiver.

Frank

---

<div class="post-metadata">

**Author:** ![Frank\_Fu](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/f/b5a626/32.png) [@Frank\_Fu](https://www.ruby-forum.com/u/Frank_Fu)\
**Post date:** [April 21, 2015, 4:28pm UTC](https://www.ruby-forum.com/t/re-some-misconceptions-about-the-peak-detector2-block/241637/3 "2015-04-21T16:28:40Z")

</div>

On Mon, Apr 20, 2015 at 12:47 PM, Frank Fu [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) wrote:

> 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. I’m 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.
> 
> Frank Fu

So I pretty much agree with this now. In the current form of the block,  
the  
function of the look\_ahead doesn’t behave the way that you described  
here,  
Frank. However, looking at the (sparse) documentation for this block, I  
can  
now see why everyone thinks that’s how it’s supposed to behave. From  
[1]:

look\_ahead: The look-ahead value is used when the threshold is found  
to  
locate the peak within this range.

And if that’s how everyone expects it to behave, which is what I’m  
hearing,  
then we should correct that.

Here are a couple of points that I’d like to consider for reworking this  
block. First, the flowgraph must finish no matter how many items are  
passed  
to it. The block was initially designed for an OFDM preamble detector,  
so  
we can’t have it just waiting around for more samples to come in, which  
will add to latency.

I think that the variable “look\_ahead” should probably be renamed to  
“window” or something. The documentation for this block also needs a lot  
of  
work to avoid ambiguity or confusion about what it’s really supposed to  
do  
and how it works. Better QA code and an example would be really useful,  
too.

Finally, I don’t think that set\_output\_multiple is the right answer  
here.  
I’d rather it save state between calls to work if it’s looking in the  
current window. We shouldn’t have a block wait for thousands of samples  
(the default look\_ahead is 1000) before making a decision. And using  
set\_output\_multiple with large values has, in my experience, a  
performance  
hit on a flowgraph.

Tom

[1]

> **[GNU Radio Manual and C++ API Reference: gr::blocks::peak\_detector2\_fb Class...](https://www.gnuradio.org/doc/doxygen/classgr_1_1blocks_1_1peak __detector2__ fb.html)**

---

<div class="post-metadata">

**Author:** ![Frank\_Fu](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/f/b5a626/32.png) [@Frank\_Fu](https://www.ruby-forum.com/u/Frank_Fu)\
**Post date:** [April 22, 2015, 6:06pm UTC](https://www.ruby-forum.com/t/re-some-misconceptions-about-the-peak-detector2-block/241637/4 "2015-04-22T18:06:28Z")

</div>

On Tue, Apr 21, 2015 at 2:23 PM, Frank Fu [removed\_email\_address@domain.invalid](mailto:removed_email_address@domain.invalid) wrote:

> the window search isn’t fully completed within a work call. If  
> set\_output\_multiple is not desired, how about creating a saved output  
> buffer of length look\_ahead? During the window search phase, the output  
> state could be saved until enough inputs come in. The block would have to  
> use general\_work instead, because there would be a temporary point where  
> input\_samples are consumed, but no outputs are written.

Frank,  
See the discussion in [http://gnuradio.org/redmine/issues/783](http://gnuradio.org/redmine/issues/783)

I’ve created a pull request to add Achilleas’ version to maint to  
replace  
the current one. There are still some potential issues that we might run  
in  
to, but this is still a much better answer, and my concerns might be  
completely unfounded. The set\_output\_multiple issue is something that  
I’ve  
seen, but it’s anecdotal evidence. We’d need benchmarking before trying  
to  
develop a different solution.

Finally, I hope you would consider creating a third peak detector block

> that would perform the finite window search. It would be extremely useful,  
> as it solves many kinds of signal detection problems for a receiver.
> 
> Frank

I would consider receiving a pull request for such a block 🙂

But if anyone works on this, please don’t call it peak\_detector3! Give  
it a  
more meaningful name for what it’s doing.

Thanks!  
Tom