Summary:	DAHLIN-00002: [patch] DTMF Caller-ID support.
Reporter:	Nils Hammar (ehsnils)	Labels:
Date Opened:	2006-08-23 03:28:50	Date Closed:	2009-09-21 09:51:19
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) dtmfcid_ast6b.patch ( 1) dtmfcid_zap6.patch
Description:	This should resolve Bug 3866. This patch enables the ability for Asterisk to generate DTMF Caller-ID on FXS ports as used in Scandinavia and the Netherlands. This patch contains of two parts, one for Asterisk and one for Zaptel. Code changes: ------------- Be aware that the default of using 'bell' cidsignalling no longer exists, this patch changes it to no signalling at all. Several changes has been performed both to the Zaptel driver and to chan_zap. The most important change is that the hook control ioctl call now passes a struct instead of a single int. The struct now contains control not only over the hook, but also over the polarity switching and has the ability to pass the DTMF caller-ID string, which is then used by the Zaptel driver to generate the caller-ID. This patch actually allows the generation of both the DTMF pre-ring caller-ID:s and the US style post-ring caller-ID:s simultaneously. (Tested) This patch is not intended to solve all ETSI defined caller-id variants. (mostly due to lack of testing equipment) There are a few differences in caller-ID supported by this patch. Some equipment only listens to DTMF caller-ID:s starting with a DTMF 'A' other only with DTMF 'D'. It is possible to select either 'A' or 'D' for a single line, but not both since they conflict. Special code for the Danish version of caller-ID is also added, but not tested. The basic logic for the Danish caller-ID is the same as for Sweden but the DTMF sequence is different and no polarity change is done before the caller-ID transmission. (The idea is that the polarity change shall wake up the equipment listening for caller-ID.) To allow for combination configurations in the code some defines has been changed from sequences to bitmasks. Configuration: -------------- Changes have been made to the configuration file zapata.conf: The 'cidsignalling' now takes more options and also accepts a comma-separated list of options. No spaces are permitted in the list of options. (I have been lazy here) but that should be a minor problem. The 'cidsignalling' can only have one of the dtmf options 'dtmf', 'dtmf_astart' or 'dtmf_dk'. This may or may not be combined with one of either 'bell' or 'v23'. (but 'v23' isn't implemented for FXS as far as I know) Examples of correct 'cidsignalling' lines: cidsignalling=bell cidsignalling=dtmf_dk cidsignalling=bell,dtmf cidsignalling=dtmf_astart,bell As you can see, order does not matter. Examples of incorrect 'cidsignalling' lines: cidsignalling=bell, dtmf cidsignalling=dtmf_astart , dtmf ,bell If more than one line is encountered the last one is the one used. The 'cidstart' takes one or two arguments: 'ring' and/or 'polarity'. Both may be used at the same time and may be entered in any order. Same restrictions regarding spacing exists for 'cidstart'. Bugs: ----- There are probably some bugs present, I will be surprised if not. - V23 caller-ID isn't supported. - One 'bug' is that the time delays around the DTMF string is regulated   by the 'w' dial-string instruction. The delays caused by this may   be excessive but testing has given that it's no big problem. - It seems like the transmission of the caller-ID is interrupted if   another line is answered resulting in a partial caller-ID on the   equipment display on the unanswered device. - Behavior on non-numeric caller-ID strings is not tested.   This is generally not a problem. --- Patch by Nils Hammar, Teleca Sweden West.
Comments:	By: Serge Vecher (serge-v) 2006-08-23 08:51:43 nils: thank you for this contribution. Couple of things need addressing... 1) Patches need to be disclaimed in order for them to be reviewed and considered for inclusion into trunk. Could you please get a disclaimer on file (see bottom of http://bugs.digium.com/main_page.php) and confirm with a note here when done? 2) Please upload patches in uncompressed format for ease of review. Thanks. By: Nils Hammar (ehsnils) 2006-08-23 09:16:40 Disclaimer was already faxed, so I suggest you check the fax. :-) By: Josef Seger (doda) 2006-09-08 14:04:26 I have tested this patch sucessfully in Sweden for 2 weeks. No issues found. By: jmls (jmls) 2006-11-01 05:57:40.000-0600 can we push this forward ? By: Serge Vecher (serge-v) 2006-11-09 12:46:41.000-0600 keep pushing till the victorious end. By: Josef Seger (doda) 2006-11-23 14:59:48.000-0600 Still working flawless for me. I see no problems with pushing this forward. By: Serge Vecher (serge-v) 2006-11-29 09:43:19.000-0600 mattf: would you please give this bug some of your attention? By: Josef Seger (doda) 2006-12-17 06:42:18.000-0600 Is there any progress with this issue? It would be nice to see it integrated in future releases.... By: Magnus (falle) 2007-05-18 06:25:16 Disclaimer on File?   No Could this be the reason why this patch is not getting into the trunk? Maybe ehsnils needs to resend it? By: Leif Madsen (lmadsen) 2007-10-31 11:12:43 ehsnils: We need you to sign the license agreement (available as a link at the top of the bug tracker) in order to move this forward. Would you mind doing that and maybe we can get this bug closed out. Thanks! By: Nils Hammar (ehsnils) 2007-10-31 11:21:04 Obviously somebody nuked the fax, because I did send a fax, see second note! By: Nils Hammar (ehsnils) 2007-10-31 11:23:48 I have now signed on the web page, let's see if that works instead. By: Leif Madsen (lmadsen) 2007-10-31 11:26:32 Hi ehsnils: The license agreement has been updated, and is now associated with the bug tracker in order to track code more easily which has been uploaded here. There is a license agreement link at the top of the bug tracker which you can fill in and send, then that goes to the license department where it is put on file, and then all code uploaded by you automatically is marked as licensed (and you won't get any of those msg's asking if you've got a license on file anymore :)) For this bug since it is obvious you had a disclaimer for this particular patch before the change, then I'll mark it as such (Qwell just told me about a fancy trick I can do for these cases). For future patches, you'll need that license agreement submitted from here (the bug tracker). Thanks for contributing to Asterisk! By: Leif Madsen (lmadsen) 2007-10-31 11:31:19 Thanks for the quick turn around! Hopefully we can move this forward and get it resolved. By: Josef Seger (doda) 2007-11-18 05:44:35.000-0600 I have run this path for more than a year and it works flawless! It would be great to see this patch integrated into 1.4..... Does anybody know if this patch is planned to enter the main trunk soon? By: Olle Johansson (oej) 2007-12-16 08:34:17.000-0600 This has been waiting for too long. Any developer? Sweden calling, but you don't see that! By: Tilghman Lesher (tilghman) 2007-12-19 09:39:13.000-0600 The problem was that this was misfiled under the Asterisk project, when it should have been filed under Zaptel, since it includes non-trivial kernel driver changes.  This will need to be looked over by someone with the knowledge of that codebase. By: Tzafrir Cohen (tzafrir) 2007-12-19 10:09:55.000-0600 Asterisk patch doesn't seem to apply (uses static initialization for configuration). From the description it looks like it breaks too many existing configurations * The default should remain "bell" to avoid breaking exsiting configurations. This can be done by parsing the value of "cidsignalling" line to a temporary variable. This may be also the place to apply tests on it. Using muptiple accumulative lines breaks configuration templates or the ugly [channels] section. * Following kernel coding conventions, I would avoid the typedef ZT_HOOK_DATA and just use 'struct zt_hook_data' * I think that rather than changing the meaning of an existing call, you should add a new ioctl . But that's just me. By: Nils Hammar (ehsnils) 2007-12-19 11:44:12.000-0600 >> Asterisk patch doesn't seem to apply (uses static initialization for configuration). Please explain what you mean? >> * The default should remain "bell" to avoid breaking exsiting configurations. I did a conscious selection to start from a NEUTRAL position instead of a TAINTED (bell) position. The breakage that occurs in itself can be considered harmless and I have tried to document the fact that there is a change. The chain of problems occurring when trying to "preserve" the bell alternative may in turn cause a sequence of more complicated code that in turn is more prone to bugs. >> * Following kernel coding conventions, I would avoid the typedef ZT_HOOK_DATA and just use 'struct zt_hook_data' No big issue with this - I just wanted the code to be easy to read and follow by leaving out unnecessary text (struct). It's more a matter of taste. >> * I think that rather than changing the meaning of an existing call, you should add a new ioctl . But that's just me. I just tried to keep the patch change at a minimum to avoid strange bugs. Inserting a new call may be the way to go if it is necessary to keep backwards compatibility. (Is it? - Is the Zaptel driver used by anything else than Asterisk?) Additional info: ---------------- I started an attempt to port this to 1.4, and I have a setting that builds and works, but currently it doesn't transmit the caller-ID to the phone since there is probably a change in the Zaptel driver that has to be analyzed and tested. I have to investigate this first to resolve why the CID isn't transmitted. By: Tilghman Lesher (tilghman) 2007-12-19 11:52:59.000-0600 The problem with changing an existing ioctl is that if someone upgrades just Zaptel, then Asterisk will break, or if someone upgrades just Asterisk, then it will break.  We are not in danger of using all the available ioctls, so just add a new one. In terms of the default cidsignalling=bell, that is non-negotiable.  There are too many installations that this would break. By: Tzafrir Cohen (tzafrir) 2007-12-19 12:08:27.000-0600 Thanks for your prompt response. Regarding the configuration variable: change the parsing to something of the sort of: int new_cidsignalling = 0 while( ... ) { /* looping over the comma-separated values  if (!strcasecmp(p1, "bell"))    new_cidsignalling |= CID_SIG_BELL;  if (!strcasecmp(p1, "v23"))    new_cidsignalling |= CID_SIG_V23;  ... } cidsignalling = new_cidsignalling; Next, there is the problem with v23. Why does the patch break v23? By: Tzafrir Cohen (tzafrir) 2008-02-24 16:57:06.000-0600 ping Why does the patch break v.23? By: Josef Seger (doda) 2008-04-03 12:42:28 Ehsnils do you have any clue why it breaks v23? By: Leif Madsen (lmadsen) 2008-11-21 09:19:50.000-0600 Pinging this issue since it seems to be very close to being complete, except it seems to break V.23? Anyone going to be able to pick this issue up and get it fixed/committed so we can close this issue? By: Russell Bryant (russell) 2009-06-15 12:05:18 I very much appreciate the contribution! Unfortunately, at this point, the code has fallen very out of date.  It was written against a very old version of Asterisk and Zaptel.  The patch needs to be updated to Asterisk trunk, and ported to DAHDI.  If someone is interested in doing that and verifying that things still work, I would be happy to get someone to work on reviewing this for inclusion. Thanks.