Asterisk
  1. Asterisk
  2. ASTERISK-10148

[patch] DTMF detect debouncing logic can miss whole digits (dsp.c)

    Details

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

      Description

      As reported on asterisk-dev at http://lists.digium.com/pipermail/asterisk-dev/2007-August/029149.html, inband DTMF detection is unreliable when the tones are not perfect.

      Having investigated the issue using a test harness around dsp.c, using audio files of varying quality captured directly from the Zaptel data before the dsp processing, I believe I have found the issue.

      It turns out that the problem with missing digits is in the debouncing,
      for which I think the logic is wrong.

      The debouncing of hits within dtmf_detect() is currently as follows:

      if (hit) {
      if (hit == s->hits[2] && hit != s->hits[1] && hit != s->hits[0])

      { // we register that we really have a digit }

      }
      s->hits[0] = s->hits[1];
      s->hits[1] = s->hits[2];
      s->hits[2] = hit;

      So hits[] is a shift register of old hits/non-hits.

      If we get a clean start of digit (say, 0), the sequence is as follows:

      (- - -) 0
      (- - 0) 0 : register that we have a digit (leading edge)
      (- 0 0) 0
      (0 0 0) 0

      However, if we get a bounce at the start, the whole digit can get missed:

      (- - -) 0
      (- - 0) -
      (- 0 -) 0
      (0 - 0) 0
      (- 0 0) 0
      (0 0 0) 0

      None of the above sequences triggers the leading-edge detector, which is looking for a match in the previous hit, preceded by two non-matches. Consequently the whole digit is missed.

      What should really happen is that the previous hit should match the current hit, and they should be different from the PREVIOUS DEBOUNCED HIT. It also appears that there is currently no debouncing of the trailing edge; the first non-hit terminates the digit.

      I have reworked the debouncing logic as follows:

      a) Debouncing is now done on both leading and trailing edges.

      b) The dtmf_detect() function now returns the debounced value (mhit).

      c) The behaviour if OLD_DSP_ROUTINES is defined has been left unchanged.

      With the change, all my test files had their DTMF detected reliably, where some of them previously had several digits missed. I will shortly be applying it to a live 1.2 system that has been having chronic DTMF issues in some calls. I will post the results here.

      The same technique should probably be applied to MF detection too, but I have not done so.

      I have attached patches for 1.2, 1.4 and trunk. This really ought to be fixed in 1.2, but if policy dictates otherwise, then at least the patch here can be fetched and applied by people who need it.

      1. dsp.c-1.2.diff
        3 kB
        Tony Mountifield
      2. dsp.c-1.4.diff
        3 kB
        Tony Mountifield
      3. dsp.c-trunk.diff
        3 kB
        Tony Mountifield

        Activity

        Hide
        Curt Moore added a comment -

        I've noticed some weirdness with DTMF detection when entering a voicemail PIN when dialing in to check voicemail; hopefully these changes will help. I'll be happy to test once the license is accepted and I can pull the patches. Great work!

        Show
        Curt Moore added a comment - I've noticed some weirdness with DTMF detection when entering a voicemail PIN when dialing in to check voicemail; hopefully these changes will help. I'll be happy to test once the license is accepted and I can pull the patches. Great work!
        Hide
        Tony Mountifield added a comment -

        I can confirm that after a day or two operation with the new code, the live system has been working fine, and does not appear to randomly drop incoming digits any more.

        Show
        Tony Mountifield added a comment - I can confirm that after a day or two operation with the new code, the live system has been working fine, and does not appear to randomly drop incoming digits any more.
        Hide
        Digium Subversion added a comment -

        Repository: asterisk
        Revision: 80820

        ------------------------------------------------------------------------
        r80820 | russell | 2007-08-24 15:06:00 -0500 (Fri, 24 Aug 2007) | 7 lines

        Improve the debouncing logic in the DTMF detector to fix some reliability
        issues. Previously, this code used a shift register of hits and non-hits.
        However, if the start of the digit isn't clean, it is possible for the
        leading edge detector to miss the digit. These changes replace the flawed
        shift register logic and also does the debouncing on the trailing edge as well.
        (closes issue ASTERISK-10148, many thanks to softins for the patch)

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

        Show
        Digium Subversion added a comment - Repository: asterisk Revision: 80820 ------------------------------------------------------------------------ r80820 | russell | 2007-08-24 15:06:00 -0500 (Fri, 24 Aug 2007) | 7 lines Improve the debouncing logic in the DTMF detector to fix some reliability issues. Previously, this code used a shift register of hits and non-hits. However, if the start of the digit isn't clean, it is possible for the leading edge detector to miss the digit. These changes replace the flawed shift register logic and also does the debouncing on the trailing edge as well. (closes issue ASTERISK-10148 , many thanks to softins for the patch) ------------------------------------------------------------------------
        Hide
        Digium Subversion added a comment -

        Repository: asterisk
        Revision: 80821

        ------------------------------------------------------------------------
        r80821 | russell | 2007-08-24 15:07:34 -0500 (Fri, 24 Aug 2007) | 15 lines

        Merged revisions 80820 via svnmerge from
        https://origsvn.digium.com/svn/asterisk/branches/1.4

        ........
        r80820 | russell | 2007-08-24 15:24:05 -0500 (Fri, 24 Aug 2007) | 7 lines

        Improve the debouncing logic in the DTMF detector to fix some reliability
        issues. Previously, this code used a shift register of hits and non-hits.
        However, if the start of the digit isn't clean, it is possible for the
        leading edge detector to miss the digit. These changes replace the flawed
        shift register logic and also does the debouncing on the trailing edge as well.
        (closes issue ASTERISK-10148, many thanks to softins for the patch)

        ........

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

        Show
        Digium Subversion added a comment - Repository: asterisk Revision: 80821 ------------------------------------------------------------------------ r80821 | russell | 2007-08-24 15:07:34 -0500 (Fri, 24 Aug 2007) | 15 lines Merged revisions 80820 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r80820 | russell | 2007-08-24 15:24:05 -0500 (Fri, 24 Aug 2007) | 7 lines Improve the debouncing logic in the DTMF detector to fix some reliability issues. Previously, this code used a shift register of hits and non-hits. However, if the start of the digit isn't clean, it is possible for the leading edge detector to miss the digit. These changes replace the flawed shift register logic and also does the debouncing on the trailing edge as well. (closes issue ASTERISK-10148 , many thanks to softins for the patch) ........ ------------------------------------------------------------------------
        Hide
        Digium Subversion added a comment -

        Repository: asterisk
        Revision: 80877

        ------------------------------------------------------------------------
        r80877 | murf | 2007-08-24 16:59:52 -0500 (Fri, 24 Aug 2007) | 70 lines

        Merged revisions 80790,80817,80819,80821,80850 via svnmerge from
        https://origsvn.digium.com/svn/asterisk/trunk

        ................
        r80790 | murf | 2007-08-24 13:03:39 -0600 (Fri, 24 Aug 2007) | 9 lines

        Merged revisions 80789 via svnmerge from
        https://origsvn.digium.com/svn/asterisk/branches/1.4

        ........
        r80789 | murf | 2007-08-24 12:52:15 -0600 (Fri, 24 Aug 2007) | 1 line

        From a complaint by jmls, I realize that the message in cdr_disposition is unnecessary. To get failure disposition, just return -1; no use having more than one case do that.
        ........

        ................
        r80817 | tilghman | 2007-08-24 13:50:16 -0600 (Fri, 24 Aug 2007) | 2 lines

        Fix documentation for Set (closes issue ASTERISK-10160)

        ................
        r80819 | bweschke | 2007-08-24 14:21:17 -0600 (Fri, 24 Aug 2007) | 11 lines

        Merged revisions 80818 via svnmerge from
        https://origsvn.digium.com/svn/asterisk/branches/1.4

        ........
        r80818 | bweschke | 2007-08-24 15:52:06 -0400 (Fri, 24 Aug 2007) | 3 lines

        A minor correction to the available logic of autofill. If a queue member is paused, they're not really "available" so don't count them as such. Somewhat related to issue ASTERISK-9835

        ........

        ................
        r80821 | russell | 2007-08-24 14:25:39 -0600 (Fri, 24 Aug 2007) | 15 lines

        Merged revisions 80820 via svnmerge from
        https://origsvn.digium.com/svn/asterisk/branches/1.4

        ........
        r80820 | russell | 2007-08-24 15:24:05 -0500 (Fri, 24 Aug 2007) | 7 lines

        Improve the debouncing logic in the DTMF detector to fix some reliability
        issues. Previously, this code used a shift register of hits and non-hits.
        However, if the start of the digit isn't clean, it is possible for the
        leading edge detector to miss the digit. These changes replace the flawed
        shift register logic and also does the debouncing on the trailing edge as well.
        (closes issue ASTERISK-10148, many thanks to softins for the patch)

        ........

        ................
        r80850 | russell | 2007-08-24 15:23:14 -0600 (Fri, 24 Aug 2007) | 13 lines

        Merged revisions 80849 via svnmerge from
        https://origsvn.digium.com/svn/asterisk/branches/1.4

        ........
        r80849 | russell | 2007-08-24 16:22:50 -0500 (Fri, 24 Aug 2007) | 5 lines

        If dnsmgr is in use, and no DNS servers are available when Asterisk first
        starts, then don't give up on poking peers. Allow the poke to get rescheduled
        so that it will work once the dnsmgr is able to resolve the host.
        (closes issue ASTERISK-10138, patch by jamesgolovich)

        ........

        ................

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

        Show
        Digium Subversion added a comment - Repository: asterisk Revision: 80877 ------------------------------------------------------------------------ r80877 | murf | 2007-08-24 16:59:52 -0500 (Fri, 24 Aug 2007) | 70 lines Merged revisions 80790,80817,80819,80821,80850 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r80790 | murf | 2007-08-24 13:03:39 -0600 (Fri, 24 Aug 2007) | 9 lines Merged revisions 80789 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r80789 | murf | 2007-08-24 12:52:15 -0600 (Fri, 24 Aug 2007) | 1 line From a complaint by jmls, I realize that the message in cdr_disposition is unnecessary. To get failure disposition, just return -1; no use having more than one case do that. ........ ................ r80817 | tilghman | 2007-08-24 13:50:16 -0600 (Fri, 24 Aug 2007) | 2 lines Fix documentation for Set (closes issue ASTERISK-10160 ) ................ r80819 | bweschke | 2007-08-24 14:21:17 -0600 (Fri, 24 Aug 2007) | 11 lines Merged revisions 80818 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r80818 | bweschke | 2007-08-24 15:52:06 -0400 (Fri, 24 Aug 2007) | 3 lines A minor correction to the available logic of autofill. If a queue member is paused, they're not really "available" so don't count them as such. Somewhat related to issue ASTERISK-9835 ........ ................ r80821 | russell | 2007-08-24 14:25:39 -0600 (Fri, 24 Aug 2007) | 15 lines Merged revisions 80820 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r80820 | russell | 2007-08-24 15:24:05 -0500 (Fri, 24 Aug 2007) | 7 lines Improve the debouncing logic in the DTMF detector to fix some reliability issues. Previously, this code used a shift register of hits and non-hits. However, if the start of the digit isn't clean, it is possible for the leading edge detector to miss the digit. These changes replace the flawed shift register logic and also does the debouncing on the trailing edge as well. (closes issue ASTERISK-10148 , many thanks to softins for the patch) ........ ................ r80850 | russell | 2007-08-24 15:23:14 -0600 (Fri, 24 Aug 2007) | 13 lines Merged revisions 80849 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r80849 | russell | 2007-08-24 16:22:50 -0500 (Fri, 24 Aug 2007) | 5 lines If dnsmgr is in use, and no DNS servers are available when Asterisk first starts, then don't give up on poking peers. Allow the poke to get rescheduled so that it will work once the dnsmgr is able to resolve the host. (closes issue ASTERISK-10138 , patch by jamesgolovich) ........ ................ ------------------------------------------------------------------------

          People

          • Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development