[Home]

Summary:ASTERISK-01108: [patch] chan_mgcp features / fixes combo
Reporter:serkan (serkan)Labels:
Date Opened:2004-02-26 11:53:48.000-0600Date Closed:2008-01-15 14:47:55.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_mgcp.c
( 1) chan_mgcp.c.diff
( 2) chan_mgcp.c.diff.2
( 3) chan_mgcp.c.diff.3
( 4) chan_mgcp.c.diff.4
( 5) chan_mgcp.c.diff.5
Description:Have been working with asterisk MGCP for a while and came up with several enhancements / fixes while testing against Occam Networks MG box.

The attached chan_mgcp.c is based on revision 1.34, and on top, has the following features and fixes:

  -- packet retransmit mechanism
  -- per endpoint/subchannel mgcp command sequencing.
  -- better transaction handling
  -- fixed some mem leaks
  -- run-time configuration reload
  -- distinguish CA and GW default MGCP ports
  -- prevent clipping of DTMF tones in an established call
  -- fixed a few crash scenarios in 3-way
  -- fix for a few cases where asterisk and MGW end-up in conflicting ep states

Briefly tested the modified MGCP and it seems to work with no flaws.

I apologize for packing everything into one ticket but this is a result of an expedited development process.

Please feel free to evaluate and incorporate the whole or part of the changes into the asterisk repository.  

I will continue to perform tests with more coverage in the following days and I can also provide fixes to this version.

Comments:By: zoa (zoa) 2004-02-26 11:57:05.000-0600

could you post a diff -u ?
By: serkan (serkan) 2004-02-26 13:08:27.000-0600

diff is attached
By: florian (florian) 2004-03-10 09:20:35.000-0600

Any chance this also covers bug 0001155 ?(http://bugs.digium.com/bug_view_page.php?bug_id=0001155)
By: serkan (serkan) 2004-03-10 16:01:38.000-0600

It may have been fixed as a side effect. I would give it a try.
By: duanecox (duanecox) 2004-03-16 12:03:23.000-0600

I applied this patch to my current CVS, it works great.  Thanks.

I could only reload MGCP via 'reload' and not 'MGCP reload' ??
By: duanecox (duanecox) 2004-03-16 12:34:32.000-0600

spoke too soon, I like the reload feature _a_lot_
but it caused me some problems with endpoint mappings, not all enpoints, just the first one listed in mgcp.conf
By: serkan (serkan) 2004-03-16 21:02:26.000-0600

+ I don't observe the problem you mentioned. Can you send your mgcp.conf and some screen outputs. It will be easier for me to figure out.
+ Also I uploaded chan_mgcp.c.diff.2. It has the mgcp reload command.
By: duanecox (duanecox) 2004-03-16 21:25:36.000-0600

serkan: can I send it to you via email?  my email is dcox@illicom.net
By: sergi (sergi) 2004-03-17 11:13:13.000-0600

Hello Serkan,

I try your chan_mgcp.c file and think that you make great work.
I have some notes about it:
I found that find_subchannel function dooe not returns correct pointer to subchannel when handling mgcp responses, because it performs search by gateway name and in case of response there is no gateway name given - only message id. It always returns subchannel pointer to last gateway listed in mgcp.conf file(in case of mgcp endpoint response of course).
Also you had commented the following line in several functions :
add_header(&resp, "X", sub->txident);
Without this mu endpoints are not establishing connection. I removed comments and everything starts work. I have Askey mgcp gateway (VG101)
By: duanecox (duanecox) 2004-03-17 11:17:33.000-0600

Maybe that is the reason I am having endpoint problems.  I am providing debugs to serkan, maybe he can rewrite that.
By: serkan (serkan) 2004-03-17 11:57:45.000-0600

Thanks for the feedback folks.
I have just noticed the issue with find_subchannel.
Working with Duane for the fix since I don't have MGCP gear now.
I will post diff.3 once Duane verifies the fix.
By: serkan (serkan) 2004-03-17 14:05:43.000-0600

uploaded diff.3 which includes:
-- fix for find_subchannel issue in case of multiple MGCP gateways
-- uncommented X header additions for Askey compat
By: serkan (serkan) 2004-03-17 17:01:37.000-0600

uploaded diff.4.

-- fix for numeric IP syntax (bug ids: 0000471 and 0001227). The numeric IP for outgoing requests is enclosed in brackets now.

Let me know any issues.
By: duanecox (duanecox) 2004-03-17 18:22:56.000-0600

awesome, i will be the first one to test! :)

Hope it makes it into CVS soon!
By: Mark Spencer (markster) 2004-03-17 23:43:31.000-0600

Serkan: A few things:

1) Have you filed a disclaimer for the changes yet?  See the main page of http://bugs.digium.com for information if you haven't.

2) I'm a little nervous about putting your patch in -stable.  Would it be easy for you to make the minimum changes and attach a patch against -stable just for fixing bugs, and not restructuring the entire
3) What was your reasoning in creating three different message queues?

4) Have you thought through the implications of deleting gateways and/or endpoints during a reload and how that affects any active channels, etc?

Thanks for all your hard work on MGCP.
By: serkan (serkan) 2004-03-18 10:54:06.000-0600

Hi Mark,

1) I will file the disclaimer shortly.
2) I understand your nervousness since this patch combines too many things. I will e-mail you more info on this.
3) Per RFC 3435, endpoint commands can be sent in parallel with some sequencing restrictions. To keep it short,
 -- there should be only one outstanding command per connection (cx_queue),
 -- there should be only one outstanding notification req per endpoint (rqnt_queue),
 -- no hard req for the rest of the commands (cmd_queue)

Within the current chan_mgcp architecture, I found it easier and simpler to satisfy these requirements with three different queues compared to alternative approaches (per-command state variables etc.)

4) Reload impact on active channels is considered and tested. Copied some code from chan_sip.c.
By: serkan (serkan) 2004-03-18 15:10:13.000-0600

I uploaded diff.5.

Changed the way [] fix was implemented: Renamed isnamefqdn as isnamedottedip and changed the associated logic (as it was misleading for the code reader). Thanks to Duane for suggestion. Just wanted to make sure it was right in case it goes into the CVS.
By: sergi (sergi) 2004-03-19 02:08:51.000-0600

Hi Serkan,

I have several suggestion about more imrovement of chan_mgcp.c. If you are working on such big improvement of it.
1)(minor) As you know the mgcp request messages that asterisk sends to endpoints is sequenced and variable that is used for it is called oseq. There is no limit on increasing of them. In case of many endpoints it may cause overloading problem, because it has type static int it can get  negative value and the endpoint will not work correctly. Also, I think there must be some kind of storage of oseq last value, because asterisk always starts with value of 0 and if in some case asterisk crashes after restarting it will send sequencies starting from 1 and if endpoint receives allready such sequense number (during variable period of time endpoints are ignoring messages with same sequence, this period is configurable on endpoints)  will not response. I worked with other call manager(Clarent) and it works in mentioned way.
2) (major) When endpoint sends NTFY requests to asterisk there is no mechanism to avoid dublicating ones(with sequence id allready received). Asterisk works correctly and on all requests it transmits response, but also it handles it.In case  of heavy network load or other network conditions dublicated NTFY messages may occures, wich will cause asterisk to not correctly get for example the dialed dtmf number(users press key on phone once but asterisk gets it twice). I see this situation in my network.
3) (major) As you know when there is established call between aterisk and endpoint there is ident assigned to each call on endpoint and asterisk. If in some case asterisk crashes or due to network condition endpoint do not receive dlcx request and it stays on ednpoint (it does in ASKEY).After that in case of new call new ident has been assigned and than endpoint do not work correctly. I  think Asterisk must send DLCX request if it see in AUEP response that endpoint has ident and it is on hook. I dont know if these situation happens with other mgcp endpoints, but you can check it trying to kill asterisk(during conversation) and start it again and checking the AUEP response.
By: serkan (serkan) 2004-03-19 12:34:04.000-0600

Hi Sergi,

You are right in all three points. And there are also many other things missing/incorrect in the current implementation. This patch has been an effort to straighten up some fundamental issues as well as establish a base for further enhancements. Going forward I feel uncomfortable in adding more to this patch except for the fixes for the stuff it introduces.

IMHO the right thing to do is to open a separate ticket for the issues you mentioned. If the patch becomes part of CVS I would be happy to address more issues as time permits.
By: Mark Spencer (markster) 2004-03-19 15:42:07.000-0600

Serkan, I'm ready to apply the patch as soon as you've sent in a disclaimer, just let me know.  Thanks!
By: Mark Spencer (markster) 2004-03-19 18:02:15.000-0600

I've added this patch to CVS.  Thank you very much for your contribution, and if there are issues, lets put them in a new ticket.  Thanks!
By: Digium Subversion (svnbot) 2008-01-15 14:47:55.000-0600

Repository: asterisk
Revision: 2483

U   trunk/channels/chan_mgcp.c

------------------------------------------------------------------------
r2483 | markster | 2008-01-15 14:47:54 -0600 (Tue, 15 Jan 2008) | 2 lines

Major MGCP enhancements (*very* big thank you to serkan and Sentito) (bug ASTERISK-1108)

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

http://svn.digium.com/view/asterisk?view=rev&revision=2483