[Home]

Summary:ASTERISK-06252: Leading DTMF lost due to lack of delay on Zapata/TDM400P/X100M
Reporter:Nils Hammar (ehsnils)Labels:
Date Opened:2006-02-04 03:35:54.000-0600Date Closed:2011-06-07 14:02:50
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) patch1a.dtmf
( 1) patch2.dtmf
Description:I class this as major due to the risk of unnecessarily annoy innocent people.

I was encountering a situation where the outgoing line to the PBX occasionally (more often than not) dropped the leading numbers dialled. The reason was that the DTMF codes were sent before the PBX was ready to receive them and therefore misplaced my calls. (fortunately I was calling an answering machine at the time, so nobody was annoyed at the time.)

My solution is to actually wait a little (one second is enough) before sending the number. This is achieved by a sleep just before sending the number. It may be crude and there may be better ways to achieve this, but it works.

Provided patch resolved my problem.


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

A new configuration parameter is added to zapata.conf in the [channels] section:

---
[channels]
; Set <n> to a value between 0 and 10, which is the delay
; in seconds before sending the number to the PBX.
dtmfdelay=<n>
---

Values are ranged from 0 to 10, where negative values are interpreted as 0 and values larger than 10 are interpreted as 10. This limits the risk of really strange errors due to misconfiguration of this parameter.
Comments:By: Jason Parker (jparker) 2006-02-05 00:04:09.000-0600

You should read the CODING-GUIDELINES document.  There are several things "wrong" with your patch.

1) Don't use C++ style comments
2) Need to use - if (blah) {
3) You should probably have an else if instead of two if's
4) (somebody please correct me if I'm wrong) sleep == bad
5) You have a randomly placed newline that was added.
6) It would be good if you could also modify the sample config to add this option.
By: Nils Hammar (ehsnils) 2006-02-05 01:40:53.000-0600

Notes of new patches;
   patch1.dtmf - Code modification.
   patch2.dtmf - Sample modification.

1. Fixed.
2. If you aren't satisfied with this modification, please explain. I normally use {} on their own lines to separate block-controls from actual code. Does help finding bugs.
3. Personally else-statements are more often creating problems and decreases readability than they helps therefore I tend to avoid them.
4. I could have been using usleep or done a different solution than with sleep, but to make the code as simple as possible and trigger as few suspicious bugs as possible while maintaining portability I used sleep. This should be OK unless this actually blocks some other functionality causing a race condition that I haven't been able to test.
5. Fixed - remains from my debugging. (Normally I separate variable declarations and code with a newline.)
6. Fixed.
By: Jason Parker (jparker) 2006-02-05 02:11:06.000-0600

2) Much better
3) But an else if means that if it's below 0, it would only run the check once. (also see below)
4) Somebody else will have to explain why sleep is bad, but I'm pretty sure it has to do with the multi-threadedness


You took quite an...interesting...approach with 3.  You did if {} else { if {} } instead of if {} else if {}.  Any reason why?
By: Nils Hammar (ehsnils) 2006-02-05 05:35:27.000-0600

3. I would hardly call it interesting. I try to use extra bracing to allow for additional modifications/additions in future modifications. Today it's a one-liner, tomorrow somebody inserts a debug-statement before the assignment and "surprise" we have a new bug. Not a big problem if the code is well-formatted, but can be an issue when working with less then ideally structured code.

Regarding if the check is run once or twice - it will also depend on how smart the compiler is. Personally I regard else-statements as an evil necessity that should be avoided. A long serial of else/if can often be replaced by a switch/case (but I didn't want to do that hack now).

4. At least when running posix sleep is thread-safe and a sleep in one thread doesn't have a direct impact on another thread (tested under Linux), but I haven't investigated other flavours of sleep.

Thread test program (may be stupid), compiled with "cc -std=c99 -o thr thr.c -lpthread"
---
#include <stdio.h>
#include <pthread.h>
#include <unistd.h>

static pthread_t thr1;
static pthread_t thr2;

void *thread1(void *arg)
{
       for(int i=0;i<10;i++)
       {
               printf("THR1\n");
               sleep(1);
       }
}

void *thread2(void *arg)
{
       for(int i=0;i<10;i++)
       {
               printf("THR2\n");
               sleep(3);
       }
}

int main()
{
       pthread_create(&thr1, NULL, thread1, 0);
       pthread_create(&thr2, NULL, thread2, 0);

       pthread_join(thr1, NULL);
       pthread_join(thr2, NULL);
}
By: Tilghman Lesher (tilghman) 2006-02-05 13:09:26.000-0600

3.  Any committed patch will use else as north has suggested.  Period.  We have coding guidelines for a reason.
4.  You will use ast_safe_sleep(), not sleep() for all cases where you need to wait a period of time before proceeding.

I can be more explicit about the reasons if you like, but you must comply with these guidelines anyway.
By: Nils Hammar (ehsnils) 2006-02-05 13:44:31.000-0600

Valid patches are:
patch1a.dtmf & patch2.dtmf, the other patch files can't be deleted so I was unable to replace them.
ast_safe_sleep() is now used.
By: Matthew Fredrickson (mattf) 2006-02-10 15:11:30.000-0600

Why don't use just use a 'w' character before your Dial() string?
By: Mark Spencer (markster) 2006-02-11 10:37:09.000-0600

"w" is the right solution for this.  Contact Digium tech support if you need assistance.