[Home]

Summary:ASTERISK-10913: Interdigit timeout is half time of the defined time
Reporter:Eduardo Ferro (eferro)Labels:
Date Opened:2007-11-28 12:37:27.000-0600Date Closed:2007-12-03 14:58:04.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_mgcp
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) half_timeout-1.4.patch
( 1) half_timeout-1.4.v2.patch
( 2) half_timeout-trunk.patch
( 3) half_timeout-trunk.v2.patch
Description:The interdigit timeout at mgcp_ss function of channels/chan_mgcp.c is not the defined time (at the same file). Is just the half part because the loop_pause is substract twice for each loop pass.

It very easy to test it because if you change the timeout, allways get a time out interdigit at twice faster than the second specified.

I test it in 1.4.14, 1.4 branch and trunk, and with the same result.



****** ADDITIONAL INFORMATION ******

This patch is for the 1.4 branch:
--------------------------------------------------------------------------------
===================================================================
--- channels/chan_mgcp.c        (revision 89965)
+++ channels/chan_mgcp.c        (working copy)
@@ -2619,7 +2619,7 @@
               while (strlen(p->dtmf_buf) == len){
                       ast_safe_sleep(chan, loop_pause);
                       timeout -= loop_pause;
-                       if ( (timeout -= loop_pause) <= 0){
+                       if ( (timeout = loop_pause) <= 0){
                               res = 0;
                               break;
                       }

--------------------------------------------------------------------------------
This patch is for the trunk:
--------------------------------------------------------------------------------
Index: channels/chan_mgcp.c
===================================================================
--- channels/chan_mgcp.c        (revision 89681)
+++ channels/chan_mgcp.c        (working copy)
@@ -2623,7 +2623,7 @@
               while (strlen(p->dtmf_buf) == len){
                       ast_safe_sleep(chan, loop_pause);
                       timeout -= loop_pause;
-                       if ( (timeout -= loop_pause) <= 0){
+                       if ( (timeout = loop_pause) <= 0){
                               res = 0;
                               break;
                       }

--------------------------------------------------------------------------------
Comments:By: snuffy (snuffy) 2007-11-28 13:32:39.000-0600

eferro, code should not be in description should be attached as file.
Because of the disclaimer that you need to have signed.
By: Mark Michelson (mmichelson) 2007-11-29 18:22:21.000-0600

While I will admit that I'm not very familiar with the MGCP code, this patch seems a bit off. It doesn't make sense to subtract the loop_pause from timeout and then immediately set timeout equal to loop_pause. Perhaps removing the first subtraction operation and leaving the if statement the same as it was is the proper fix?
By: Eduardo Ferro (eferro) 2007-11-29 19:19:46.000-0600

Sorry... This is not the correct patch :(

the correct one shoul change the - "if ( (timeout -= loop_pause) <= 0){"  for "- if ( (timeout - loop_pause) <= 0){"

So the patch is like:
-------------------------------------------------------------------------------
--- channels/chan_mgcp.c (revision 89681)
+++ channels/chan_mgcp.c (working copy)
@@ -2623,7 +2623,7 @@
               while (strlen(p->dtmf_buf) == len){
                       ast_safe_sleep(chan, loop_pause);
                       timeout -= loop_pause;
- if ( (timeout -= loop_pause) <= 0){
+ if ( (timeout - loop_pause) <= 0){
                               res = 0;
                               break;
                       }
-------------------------------------------------------------------------------

Sorry for the mmistake
By: Eduardo Ferro (eferro) 2007-12-01 01:58:05.000-0600

I think the correct patch is half_timeout-1.4.v2.patch  because in the loop if you already subtract from timeout the loop pause, then you should only check the timeout value in the condition
By: Digium Subversion (svnbot) 2007-12-03 14:57:16.000-0600

Repository: asterisk
Revision: 90639

U   branches/1.4/channels/chan_mgcp.c

------------------------------------------------------------------------
r90639 | mmichelson | 2007-12-03 14:57:16 -0600 (Mon, 03 Dec 2007) | 5 lines

Changing some bad logic when calculating the interdigit timeout.

(closes issue ASTERISK-10913, reported and patched by eferro)


------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-12-03 14:58:04.000-0600

Repository: asterisk
Revision: 90644

_U  trunk/
U   trunk/channels/chan_mgcp.c

------------------------------------------------------------------------
r90644 | mmichelson | 2007-12-03 14:58:04 -0600 (Mon, 03 Dec 2007) | 13 lines

Merged revisions 90639 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r90639 | mmichelson | 2007-12-03 14:59:51 -0600 (Mon, 03 Dec 2007) | 5 lines

Changing some bad logic when calculating the interdigit timeout.

(closes issue ASTERISK-10913, reported and patched by eferro)


........

------------------------------------------------------------------------