Summary:	ASTERISK-12135: Queue holdtime smoothing filter is not boxcar (FIR), but exponential average (IIR).
Reporter:	David Woolley (davidw)	Labels:
Date Opened:	2008-06-03 13:01:08	Date Closed:	2008-06-17 10:51:22
Priority:	Trivial	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_queue
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:
Description:	The comment for recalc_holdtime says that it implements a recursive boxcar algorithm.  Even with the "recursive" qualifier, boxcar averaging refers to simple rolling average filters, which are finite impulse response ones.  The filter that recalc_holdtime actually implements is an exponential average, which is an infinite impulse response filter. ****** ADDITIONAL INFORMATION ****** Incidentally, "recursive boxcar" has only three non-Asterisk Google hits, all of which are clearly finite impulse response.  "boxcar function" clearly refers to something with finite duration. The recursive refers to an optimisation in which you add the next input value into a running total and then subtract it out n samples later, rather than summing the last n samples each time.  The boxcar, effectively, refers to the fact that all the historic samples that are actually used have the same weight. The recursive boxcar formulation is an FIR formulation, but the resulting filter can be reduced to IIR form, which is not true of the filter used in Asterisk.
Comments:	By: Mark Michelson (mmichelson) 2008-06-13 13:35:52 I understand that the comment and code do not match, but I guess there needs to be a distinction made regarding this report. Does the comment need to be changed to conform to what's in the code or does the code need to be changed to conform to what the comment says? Unfortunately, I don't really grasp the gain from changing the code to be a FIR function so my initial reaction upon seeing such a report is to change the comment. I didn't write the holdtime calculation function, but looking at it, I have to believe it was written the way it was to be lightweight. By: David Woolley (davidw) 2008-06-17 07:48:51 I marked it as text, because I was suggesting the comment be changed.  Basically I was hoping it was a bit more sophisticated than it is, and didn't recognize the "boxcar" term,  but when I looked it up I found that it was being misused. By: Mark Michelson (mmichelson) 2008-06-17 10:41:13 Okay, well that certainly makes things easy. :) I'll get the comment changed to be more accurate. By: Digium Subversion (svnbot) 2008-06-17 10:49:57 Repository: asterisk Revision: 123274 U   branches/1.4/apps/app_queue.c ------------------------------------------------------------------------ r123274 | mmichelson | 2008-06-17 10:49:55 -0500 (Tue, 17 Jun 2008) | 12 lines davidw pointed out that the holdtime calculation used by app_queue does not use "boxcar" filtering as the comments say. The term "boxcar" means that the number of samples used to calculate stays constant, with new samples replacing the oldest ones. The queue holdtime calculation uses all holdtime samples collected since the queue was loaded, so the comment has been changed to be accurate. (closes issue ASTERISK-12135) Reported by: davidw ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=123274 By: Digium Subversion (svnbot) 2008-06-17 10:50:44 Repository: asterisk Revision: 123275 _U  trunk/ U   trunk/apps/app_queue.c ------------------------------------------------------------------------ r123275 | mmichelson | 2008-06-17 10:50:43 -0500 (Tue, 17 Jun 2008) | 20 lines Merged revisions 123274 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r123274 | mmichelson | 2008-06-17 10:56:55 -0500 (Tue, 17 Jun 2008) | 12 lines davidw pointed out that the holdtime calculation used by app_queue does not use "boxcar" filtering as the comments say. The term "boxcar" means that the number of samples used to calculate stays constant, with new samples replacing the oldest ones. The queue holdtime calculation uses all holdtime samples collected since the queue was loaded, so the comment has been changed to be accurate. (closes issue ASTERISK-12135) Reported by: davidw ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=123275 By: Digium Subversion (svnbot) 2008-06-17 10:51:22 Repository: asterisk Revision: 123276 _U  branches/1.6.0/ U   branches/1.6.0/apps/app_queue.c ------------------------------------------------------------------------ r123276 | mmichelson | 2008-06-17 10:51:21 -0500 (Tue, 17 Jun 2008) | 28 lines Merged revisions 123275 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r123275 | mmichelson | 2008-06-17 10:57:43 -0500 (Tue, 17 Jun 2008) | 20 lines Merged revisions 123274 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r123274 | mmichelson | 2008-06-17 10:56:55 -0500 (Tue, 17 Jun 2008) | 12 lines davidw pointed out that the holdtime calculation used by app_queue does not use "boxcar" filtering as the comments say. The term "boxcar" means that the number of samples used to calculate stays constant, with new samples replacing the oldest ones. The queue holdtime calculation uses all holdtime samples collected since the queue was loaded, so the comment has been changed to be accurate. (closes issue ASTERISK-12135) Reported by: davidw ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=123276