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).
------------
I am requesting some feedback for this approach so that I can finalize it and submit it for merging into the gnuradio trunk.
best
Achilleas
peak_detector2_fb_impl_old.cc