Details

    • Type: Bug Bug
    • Status: Closed
    • Severity: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Target Release Version/s: None
    • Component/s: Core/PBX
    • Labels:
      None
    • SVN Revision Number:
      116466
    • Mantis ID:
      12658
    • Regression:
      No

      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.

      1. log.txt
        12 kB
      2. v1-12658.patch
        4 kB
        Dmitry Andrianov
      3. v2-12658.patch
        5 kB
        Dmitry Andrianov

        Activity

        Hide
        Russell Bryant added a comment -

        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 ...

        Show
        Russell Bryant added a comment - 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 ...
        Hide
        Digium Subversion added a comment -

        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

        Show
        Digium Subversion added a comment - 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
        Hide
        Digium Subversion added a comment -

        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

        Show
        Digium Subversion added a comment - 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
        Show
        Russell Bryant added a comment - http://reviewboard.digium.com/r/85/
        Hide
        Digium Subversion added a comment -

        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

        Show
        Digium Subversion added a comment - 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

          People

          • Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development