[Home]

Summary:ASTERISK-12024: [patch] DTMF issues on Zap
Reporter:Dmitry Andrianov (dimas)Labels:
Date Opened:2008-05-15 08:58:20Date Closed:2008-12-12 07:44:07.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/PBX
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) log.txt
( 1) v1-12658.patch
( 2) v2-12658.patch
Description:ohh. The long one.
replaces 11635 and 12592

I often have unreliable DTMF recognition on Zap channels. Research shown that DSP works fine, and digits are read from the Zap driver correctly almost 100% of time and corruption occurs later in the VLDTMF emulation code.

So the problems I encountered:
1. There is a situation when DTMF emulation swaps digits. Scenario (user presses 12 with short durations and short pause):

 A. ast_read is called by some application
 B. ast_read calls channel's read and gets DTMF_END(1)
 C. DTMF emulation is started and ast_read sets AST_FLAG_EMULATE_DTMF flag and returns AST_FRAME_DTMF_BEGIN(1)
 D. application calls autoservice_start and AST_FLAG_END_DTMF_ONLY is set on the channel
 E. autoservice thread calls ast_read
 F. eventually channel's technology diver returns DTMF_END(2).
 G. since AST_FLAG_END_DTMF_ONLY is set, ast_read passes DTMF_END(2) thru ast_read returns DTMF_END(2). However it is still in the emulation mode!
 H. Eventually, emulation finishes and ast_read returns DTMF_END(1)

bottom line: 1 and 2 got swapped

Solution (one of, which I choosed) is to always queue DTMFs when we are already in emulation mode. This fix immediately led to next issue:



2. when user quickly rolls over digits (1234) there can be a situation when autoservice thread already read 12 and 34 are already queued into channel's dtmfq but not yet read. Then when autoservice_stop "returns" digits into channels readq, and they get processed by the VLDTMF emulation code AGAIN - they will be queued to dtmfq.

Result - 3412 will be read from the channel instead of 1234.

Solution: mark frames "returned" by autoservice and do NOT process them again.

Major drawback: DTMFs which "passed thru" autoservice won't be re-emulated so only DTMF_ENDs remain and no proper interval between them. In my case when I mess with autoservice it is always an application terminates the call so no worries - to my knowledge applications only need DTMF_END and never BEGINs



3. The next problem is that the code at the beginning of ast_read which is responsible for "injecting" deferred DTMF can inject content of dtmfq even if there is non-empty readq (possible filled by just finished autoservice which means they ae higher prioity).

Resul - again, swapped digits.

Solution: add additional check - do not inject digits from the dtmfq while there is somethign in the channel's readq.


4. Finally AST_FLAG_EMULATE_DTMF is not always set when digit is added to dtmfq. To make sure we ALWAYS queue when somethign is queued already, I added "|| !ast_strlen_zero(chan->dtmfq)" to the conditions used to queue DTMFs.

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


NEEDS TESTING! I tested this on single production machine only. My use cases are very limited.
Comments:By: tbsky (tbsky) 2008-06-11 04:03:03

Dear dimas:

  i am stilling follow your steps to fix asterisk DTMF problems.  this time your patch fix my DTMF callerid problem. they didn't swap digits anymore.
  however the old missing and duplicated DTMF problems seems sometimes happened again.
(bug 11740 and 11911)
  i am trying asterisk 1.4.20.1 + zaptel 1.4.11. i think your patches already included in this version.

By: Dmitry Andrianov (dimas) 2008-06-17 04:00:08

I recommed you upgrading to 1.4.21 because it contains fix for issue 12656. The ussue was also causing DTMF to be lost.

By: tbsky (tbsky) 2008-06-17 05:43:52

Dear dimas:

    is it safe to use your patch with 1.4.21?  i saw one hunk of your patch is already include in 1.4.21 (autoservice.c).  i apply other hunks and it seems fine.
    thanks for your help!!

By: Dmitry Andrianov (dimas) 2008-06-17 07:05:17

The old patch does not apply anymore so I updated it. v2 should apply fine.

NOTE that v2 patch also includes patch for 12874 because they both patch the same file and I was too lazy to remove first patch before changing file for the second. Sorry about that.

By: Russell Bryant (russell) 2008-06-18 05:17:20

Thank you for the detailed report.

By: Russell Bryant (russell) 2008-06-18 05:29:19

I'm happy with everything except for #2.  When a channel comes out of autoservice, it is not always connected to an application.  It could be bridged to another channel.  We _must_ provide BEGIN and END.  I haven't thought of a way to fix it that I would like to propose, yet, but I will keep thinking about it.

By: Dmitry Andrianov (dimas) 2008-06-18 07:11:59

Russell,
I see your point. Well, I have kinda proposed a way of fixing this but you dislike it :)

Just to remind. My proposal was to: completely remove DTMF "normalization" code from ast_read, create standalone DTMF normalization code and use it from the ast_write only.

Bad thing is that neither BEGIN frames nor proper separation in time are guaranteed inside Asterisk anymore. However to my knowledge not a single application depends on BEGIN frames - when they are reading digits they only need END frames. However on the bright side:

* ast_read code simplifies a lot. The AST_FLAG_END_DTMF_ONLY goes away completely from all the places. I believe we can finally eliminate all the VLDTMF+autoservice problems this way.
* every channel has a reference to current DTMF normalizer for this channel and there are two different normalizers possible: one does BEGIN+END while second is "dumb" and does almost nothing except passing END frames. The second one is used by SIP channels with INFO DTMF mode.
* ast_write calls channel normalizer so the end user (being a softphone, zap channel or anything else) always receives good DTMF.
* If some application really cares about "normality" of DTMFs it reads - it also can feed incoming frames them into normalizer and receive normalized frames from it.



By: Bart Fisher (bhfisher) 2008-06-18 22:27:06

I think my problem might be related to this issue.  I my case the calls come from a SIP provide and are bridged to an external IVR system using a port on a TE410P.  I can listen to the sounds between asterisk and the IVR system.  When a call arrives, the inband ANI and DTMF sound normal and are recognized by the IVR system.  However, if the caller is to enter sound DTMF info such as a phone number, the tones sound terrible, barely recognizable as dtmf. Also, on calls that are bridge ZAP to ZAP work properly.  I've applied the patch above and included some logs for calls that worked and fails - The ones to DNIS 5682 failed.
I am using Asterisk 1.4.21
Let me know if you need anything else - The system is off-line at the moment
Thanks, Bart

By: Leif Madsen (lmadsen) 2008-12-09 07:56:07.000-0600

I've reassigned this to Russell to see if he has anything else to add to the issue. Once you are done with this issue Russell, please either close (because you fixed it :)) or reassign as appropriate. Thanks!

By: Russell Bryant (russell) 2008-12-09 15:59:26.000-0600

I'm working on a fix for this.  I'm doing it in a branch because it's going to be invasive to DTMF handling ...

By: Digium Subversion (svnbot) 2008-12-11 08:42:21.000-0600

Repository: asterisk
Revision: 162996

U   team/russell/issue_12658/include/asterisk/channel.h
U   team/russell/issue_12658/main/autoservice.c
U   team/russell/issue_12658/main/channel.c

------------------------------------------------------------------------
r162996 | russell | 2008-12-11 08:42:21 -0600 (Thu, 11 Dec 2008) | 6 lines

Add the first part of the fix for issue ASTERISK-12024.  This fixes a scenario where
autoservice can cause the order of received digits to be swapped.  The error
edge case occurred when there were DTMF frames on the channel readq at the
time that the channel is taken out of autoservice, and deferred signaling
frames are getting put back on the readq.

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

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

By: Digium Subversion (svnbot) 2008-12-11 09:34:51.000-0600

Repository: asterisk
Revision: 163036

U   team/russell/issue_12658/include/asterisk/channel.h
U   team/russell/issue_12658/main/channel.c

------------------------------------------------------------------------
r163036 | russell | 2008-12-11 09:34:51 -0600 (Thu, 11 Dec 2008) | 6 lines

Change up DTMF handling in ast_read() to fix some more issues realted to issue ASTERISK-12024.

The fact that DTMF frames came from two different queues caused problems that could
lead to swapped received digit order.  So, simplify things a bit by only using the
frame readq to make sure the order stays intact.

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

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

By: Russell Bryant (russell) 2008-12-11 10:39:22.000-0600

http://reviewboard.digium.com/r/85/

By: Digium Subversion (svnbot) 2008-12-12 07:44:04.000-0600

Repository: asterisk
Revision: 163448

U   branches/1.4/include/asterisk/channel.h
U   branches/1.4/main/autoservice.c
U   branches/1.4/main/channel.c

------------------------------------------------------------------------
r163448 | russell | 2008-12-12 07:44:03 -0600 (Fri, 12 Dec 2008) | 26 lines

Resolve issues that could cause DTMF to be processed out of order.

These changes come from team/russell/issue_12658

1) Change autoservice to put digits on the head of the channel's frame readq
  instead of the tail.  If there were frames on the readq that autoservice
  had not yet read, the previous code would have resulted in out of order
  processing.  This required a new API call to queue a frame to the head
  of the queue instead of the tail.

2) Change up the processing of DTMF in ast_read().  Some of the problems
  were the result of having two sources of pending DTMF frames.  There
  was the dtmfq and the more generic readq.  Both were used for pending
  DTMF in various scenarios.  Simplifying things to only use the frame
  readq avoids some of the problems.

3) Fix a bug where a DTMF END frame could get passed through when it
  shouldn't have.  If code set END_DTMF_ONLY in the middle of digit emulation,
  and a digit arrived before emulation was complete, digits would get
  processed out of order.

(closes issue ASTERISK-12024)
Reported by: dimas
Tested by: russell, file
Review: http://reviewboard.digium.com/r/85/

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

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