Summary:	ASTERISK-08796: [patch] extend SMDI support to chan_sip
Reporter:	James Rothenberger (jaroth)	Labels:
Date Opened:	2007-02-13 14:07:58.000-0600	Date Closed:	2007-12-12 10:27:29.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) sip_smdi_all_v2.patch ( 1) sip_smdi_all_v3.patch ( 2) sip_smdi_all.patch
Description:	This patch allows SMDI message integration with SIP.  Currently only Zap channels can utilize SMDI messaging.  If all VoIP signalling to Asterisk is done via SIP, there is currently no support for SMDI.  Since SIP signalling packets can not be guaranteed to arrive in the same order as SMDI messages, a new method was added to the smdi module to allow matching of the SIP INVITE source (ex. 0001@192.168.1.10) to the message desk terminal number (ex. 0001, 0002, etc) This patch also allows each voicemail user to specify if they will be using SMDI for MWI notification, instead of having extern_notify set to SMDI.  There are instances when some users will need SMDI notification and others will want to use the built-in MWI support. ****** ADDITIONAL INFORMATION ****** Files contained in the patch: app_voicmail.c res_smdi.c smdi.h chan_sip.c Makefile in channels directory needs the following line added: chan_sip.o: ASTCFLAGS+=-DWITH_SMDI=1 When using SMDI with SIP. This should be made configurable. Three variables with be available in the dialplan: ${SMDI_EXTEN} - the message desk from the SMDI ${SMDI_VM_TYPE} - the SMDI type as mapped to busy, unavailable, etc. ${SMDI_CID} - the caller ID from the SMDI message To use the information set by the SIP INVITE paired with the SMDI message,  you can do something like this: exten => 0001,1,GotoIf($["${SMDI_EXTEN}" = ""]?4) exten => 0001,2,VoiceMail(${SMDI_EXTEN}|${SMDI_VM_TYPE}) exten => 0001,3,Hangup exten => 0001,4,VoiceMailMain()
Comments:	By: James Rothenberger (jaroth) 2007-02-13 15:26:00.000-0600 Additional keywords in sip.conf: usesmdi=true smdiport=/dev/ttyS0 smdipairing=yes ; whether to just take the first smdi message available, or search for a matching message desk number in the INVITE Additional keywords in voicemail.conf: usesmdi=yes (as a parameter for each mailbox) tells system how to set MWI By: Olle Johansson (oej) 2007-02-14 10:31:13.000-0600 For chan_sip: - Don't set default values in the variable declaration, like +static char smdi_port[SMDI_MAX_FILENAME_LEN] = "/dev/ttyS0"; Follow the rest of the global variables, where we set the default value at reload. Always add doxygen-formatted comments. Missing here: + unsigned int use_smdi:1; + struct ast_smdi_interface *smdi_iface; /* serial port */ Global variables need to have "global_" prefix and default settings "default_ " prefix. Follow the coding style of the file, please. Never send messages to LOG_DEBUG without checking option_debug Would suggest using "smdienable" instead of "usesmdi" so that all settings begin with "smdi" channels/Makefile should not set SMDI by default, propably just a mistake. Use menuselect and autoconf In app_voicemail - use ast_true() instead of checking for a "yes" string. Sorry I can't test this, I have no SMDI stuff. By: Olle Johansson (oej) 2007-02-14 10:32:55.000-0600 btw, I'm sure this is a cool add-on for all SMDI users! By: James Rothenberger (jaroth) 2007-02-14 11:04:04.000-0600 I used "usesmdi" because that is what is used for Zap channels, I wanted to stay consistent.  Should I still consider changing the keyword?  We have done quite a bit of testing, and will be doing more in the upcoming weeks.  I am not sure that I understand the best way to configure the Makefiles using menuselect and autoconf. By: James Rothenberger (jaroth) 2007-02-14 11:58:36.000-0600 Version 2 of the patch has all fixes noted except for Makefile changes.  I am not sure how best to proceed here. By: Olle Johansson (oej) 2007-02-14 14:39:06.000-0600 I wasn't aware of the zaptel variable, keep it as "usesmdi" so we're compatible. Sorry. Impressed over fast feedback. Now we need someone that understands SMDI to check the code too, then we shold be all set. By: Olle Johansson (oej) 2007-02-14 14:40:17.000-0600 +++ channels/Makefile (working copy) @@ -109,8 +109,10 @@ chan_misdn.o: ASTCFLAGS+=-Imisdn -misdn_config.o: ASTCFLAGS+=-Imisdn +chan_sip.o: ASTCFLAGS+=-DWITH_SMDI=1 +misdn_config.o: ASTCFLAGS+=-Imisdn + misdn/isdn_lib.o: ASTCFLAGS+=-Wno-strict-aliasing $(if $(filter chan_misdn,$(EMBEDDED_MODS)),modules.link,chan_misdn.so): chan_misdn.o misdn_config.o misdn/isdn_lib.o misdn/isdn_msg_parser.o ----- The misdn change does not belong to this patch. And again, we need to fix the WITH_SMDI so that we use menuselect instead By: Olle Johansson (oej) 2007-02-14 14:40:50.000-0600 btw, astobj.h is already included in chan_sip, so that's not needed again. By: Olle Johansson (oej) 2007-02-14 14:48:34.000-0600 app_voicemail needs to have #ifdef WITH_SMDI for the calls to an external module. By: Matthew Nicholson (mnicholson) 2007-02-14 16:21:43.000-0600 I fixed a few errors in the patch.  You had sizeof(vmu->zonetag) insead of sizeof(vmu->usesmdi) in app_voicemail.c.  Also in res_smdi.c in the ast_smdi_md_message_wait_for_callid() function you ignored the timeout in your inner loop. I do have a few other notes about the code as well.  The ast_smdi_md_message_wait_for_callid() function can change the order of the messages which could cause problems.  The order of the messages should be the same order as the calls come in.  Also if zaptel and sip try and use the same smdi interface (a rather silly idea), things will behave strange (mainly callerid and such will be wrong half the time).  Some sort of reference counting could be used to prevent more than one module from using an interface at a time, but that is a separate problem from this feature. Perhaps the ast_smdi_md_message_wait_for_callid() function should iterate through the message stack, or use the pop and putback functions to keep the order of the messages the same. By: Matthew Nicholson (mnicholson) 2007-02-14 16:23:07.000-0600 I uploaded v3 of the patch with the minor fixes outlined above. By: Olle Johansson (oej) 2007-02-15 11:11:08.000-0600 Thanks, Matt! jaroth: Your turn :-) By: James Rothenberger (jaroth) 2007-02-20 13:55:26.000-0600 I tried to apply the latest patch (v3) and got an error in the run_externnotify method.  It looks like the logic was changed to simply check if the smdi serial interface was set to determine whether to use it or not for MWI notification.  The intended behavior was to allow "usesmdi" to be set on a per-mailbox basis as there could be instances (and we have them) where SOME users will need to be notified via SMDI and some will need "regular" MWI notification (SIP phones).  Having a global SMDI notification will not allow this flexibility.  That is why I checked for "usesmdi" for the mailbox.  Let me know how to proceed with this one, maybe there is a reason for it that I am missing. By: Serge Vecher (serge-v) 2007-02-20 14:13:34.000-0600 you may want to get email updates on this one :) By: Matthew Nicholson (mnicholson) 2007-02-20 14:59:53.000-0600 The externnotify logic should not have changed, you can ignore that part of the patch.  The important change in app_voicemail.c is when reading the config file. By: Patrick Cole (ltd) 2007-03-15 19:46:55 I'm interested...what kind of setup are you running that you need to match SIP calls to SMDI messages? By: James Rothenberger (jaroth) 2007-03-16 10:05:21 We have architected a method to replace aging and discontinued SMDI-based voicemail systems with Asterisk voicemail while maintaining transparent integration to VoIP, Centrex and legacy PBX systems (users).  Most importantly users on legacy phone sets (Centrex and legacy PBX) still get full use of Message Waiting Indicators (MWI) while accessing Asterisk voicemail. By: Michiel van Baak (mvanbaak) 2007-09-12 11:18:29 Can you provide a patch against current trunk? By: James Rothenberger (jaroth) 2007-09-12 12:03:48 I just made a patch for 1.4.11 that we have just started testing.  It looks like there were some changes in the chan_sip.c code that have broken the patch.  Specifically, when I copy the callerID from the SMDI message to p->owner->cid.cid_num, the pointer is NULL.  Maybe the memory for cid.cid_num is now allocated at a different time, I am not sure.  I will be working on it in the next few weeks.  When I have figured out the problem and have tested it, I will provide a new patch.  BTW, the patch submitted above has been in use for a few months on a 1.4.2 version of Asterisk with no problems. By: Michiel van Baak (mvanbaak) 2007-09-12 12:19:43 Thanks for the quick response! Good luck on the new patch. By: Jason Parker (jparker) 2007-11-13 17:27:48.000-0600 I think we're just waiting on an updated patch here. By: Russell Bryant (russell) 2007-12-12 10:27:27.000-0600 I have decided to take on this project with an alternate implementation.  However, let me point out some of the more serious problems that we discovered in this patch. The chunk of code that does most of the work in this patch in chan_sip is done in the part of the code that processes incoming INVITES.  When working on chan_sip, it is critical to recognize that chan_sip uses a single thread for processing incoming messages.  Because of this, you can not introduce code that blocks for any appreciable amount of time. In this case, the code may block for up to a second and a half in some situations.  During that time, no other SIP calls can get processed.  Furthermore, the way that the ast_smdi_md_message_wait_for_callid() function was implemented is very problematic for a couple of different reasons. The first problem with this function is that it implements a busy loop.  That means that as long as this function is running, it will eat up as much CPU time as it possibly can.  This would certainly affect the quality of any active calls on the system. The second big problem with the use of this function is memory management.  This function returns an SMDI message to get processed in chan_sip, but never gets destroyed.  The memory allocated for every SMDI message is being leaked.