[Home]

Summary:ASTERISK-09315: can't dial/make calls from skinny phone
Reporter:pj (pj)Labels:
Date Opened:2007-04-25 14:16:06Date Closed:2007-07-11 19:59:09
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) skinny-predial-dtmf.txt
( 1) skinny-predial-dtmf-trunk.txt
( 2) skinny-predial-dtmf-trunk-v2.txt
( 3) skinny-predial-dtmf-v2.txt
( 4) skinny-predial-dtmf-v3.txt
( 5) skinny-predial-dtmf-v4.txt
( 6) skinny-predial-dtmf-v5.txt
Description:isn't possible to make calls from skinny phone (ci$co 7920),
when I type number and press dial, I hear only dialtone, digits are collected by asterisk, but not dialed, after timeout asterisk hungup.
something wrong must happed in trunk development during about last two weeks, because previous asterisk version (60xxx) works fine.


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

skinny debug:

unknown1 in handle_stimulus_message is '0'
Received Stimulus: Line(1)
Setting ringer mode to '1'.
skinny_new: tmp->nativeformats=8 fmt=8
Attempting to Clear display on Skinny 324@PJ
Clearing Display
Found device: PJ
Found device: PJ
   -- Starting simple switch on '324@PJ'
Setting ringer mode to '1'.
Found device: PJ
Collected digit: [9]
Collected digit: [5]
Collected digit: [9]
skinny_hangup(Skinny/324@PJ-3) on 324@PJ
Found device: PJ
Clearing Display
Comments:By: pj (pj) 2007-04-25 14:58:54

SVN-trunk-r61478 is working fine,
so it must be something wrong between r61478 and r61800.
By: Joshua C. Colp (jcolp) 2007-04-26 11:48:18

chan_skinny.c itself did not change during that time, so it must be something else... perhaps DTMF?
By: pj (pj) 2007-04-26 14:02:55

SVN-trunk-r61698 also OK
if somebody has feeling, what change can make this issue, please send me svn revisions before and after this change, I can try both and confirm if problematic change was identified.


By: dea (dea) 2007-04-26 18:29:45

Seeing the same thing here.  Chan_skinny detects the digits, but no
level of logging show the digits getting past the channel.

One further odd point I notice here is that the phone continues to
present dial-tone while I attempt to dial.
By: dea (dea) 2007-04-26 18:48:52

Heh.  Turn on DTMF logging and dial and we see the cause.
Russell's enforcement of DMTF duration does not seem to like
the chan_skinny DTMF:

[Apr 26 16:43:57] VERBOSE[30306] logger.c:     -- Starting simple switch on '8012@support'
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF begin '8' received on Skinny/8012@support-7
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF end '8' received on Skinny/8012@support-7, duration 0 ms
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF begin '6' received on Skinny/8012@support-7
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF end '6' received on Skinny/8012@support-7, duration 0 ms
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF begin '5' received on Skinny/8012@support-7
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF end '5' received on Skinny/8012@support-7, duration 0 ms
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF begin '0' received on Skinny/8012@support-7
[Apr 26 16:43:58] DTMF[30306] channel.c: DTMF end '0' received on Skinny/8012@support-7, duration 0
By: pj (pj) 2007-04-27 13:56:09

so, shouldn't be this issue assigned to Russel, if his change in dtmf created problem in other channel (skinny)?
By: dea (dea) 2007-04-27 13:58:41

I think so.  I also think I see a think-o in the new DTMF code,
with an analysis in bug if 9616


By: dea (dea) 2007-04-27 15:53:27

OK, more poking and proding.  It is definately the new DTMF
code.  The skinny channel uses ast_waitfordigit_full, which uses
__ast_read to get the number to call.

There a couple of issues in play:

1.  There are no voice frames on the channel at this time,
   so we cannot clear AST_FLAG_EMULATE_DTMF
2.  The f = &ast_null_frame at the end of case AST_FRAME_DTMF_END
   nukes the DTMF digit before ast_waitfordigit_full can collect
   it.  Commenting the line out allows the first digit to get through
   to chan_skinny, but we still have to cope with issue 1.

An ugly way to handle this would be to check the chan->tech and special
case skinny, but I wonder if that just moves the issue to the next channel
type that uses this method to collect digits to place a call.
By: dea (dea) 2007-04-27 16:10:54

OK, I think I may have a way to handle this without the
special case for skinny.

Near line 2321 (in the section case AST_FRAME_DTMF_BEGIN)

} else {
  ast_set_flag(chan, AST_FLAG_IN_DTMF);
  chan->dtmf_tv = ast_tvnow();
+  /* Clear-out AST_FLAG_EMULATE_DTMF if still set from last digit */
+  ast_clear_flag(chan, AST_FLAG_EMULATE_DTMF);
}

That allows skinny to dail-out again.  It might need a little
logic/housekeeping to make sure it does not interfere with the
concept of enforcing minimum DTMF duration, but at least it gives
channels that might not have voice frames between DTMF frames a
chance to complete the DTMF processing,
By: Michiel van Baak (mvanbaak) 2007-05-01 11:34:48

DEA: your patch does not work on current svn trunk.
I'm now trying to find out how to fix this.
By: Michiel van Baak (mvanbaak) 2007-05-01 12:01:38

never mind. forgot to remove the f = &ast_null_frame
With this removed and the extra line as per your last comment my skinny phones are able to place calls again.
By: pj (pj) 2007-05-02 13:52:28

if patch solved (or workarounded) this issue, can it be included into asterisk trunk?
By: dea (dea) 2007-05-02 13:59:58

I think my hack works at the expense of what Russell is trying
to do with DTMF duration enforcement.

So I would not advocate it be added to trunk, but instead used
as proof that the DTMF changes are what broke chan_skinny and
encourage a proper fix.

Would a Mantis Adminstrator please assign this bug to Russell?
By: Russell Bryant (russell) 2007-05-02 15:18:52

I would be happy to look at this.  However, first, I need a functional skinny phone!  I have a 7970 in my office, but I just haven't set it up properly.  I'll work on it ...
By: dea (dea) 2007-05-02 16:04:30

I thought I would try your in-band tree to see if it helped the
skinny cause.  No such luck.

Looking further, it seems that in chan_skinny.c in function
handle_keypad_button_message() we might be able to 'store'
the digits collected during the dialing phase and pass that
to skinny_ss, or add a variable to the skinnysession structure
to hold the collected digits.  Then skinny_ss would not call
ast_waitfordigit, and we would not need to handle pre-call DTMF
special casing in channel.c

I can test changes to chan_skinny easily, since this is a non-production
system.
By: dea (dea) 2007-05-02 19:07:35

I have reworked skinny_ss not to use ast_waitfordigit.

I am not fully happy with the result, but it works.  Extra eyes welcome.

This one is based on Russell's inband-dtmf tree.

I've verified pre-dial DTMF works and I can join a MeetMe.

It may be ugly enough to empty your bank account and kick your dog...

edit (Oops!  Missed a couple variable replacements in skinny_ss(), new patch in a minute)


By: dea (dea) 2007-05-02 21:36:18

V2 could segfault if the hook state toggled without any digits entered.

I thought about it somemore and restructured how the code waits for
a usable extension in skinny_ss ( I really did not like the first
couple passes).  I also move the storage location of the collected
digits to the device structure.

Other than hating the variable name for the collected digits, I am
reasonably happy with this one.

I do wonder if I should impliment some locking around.  I think so, but
don't know if I need to lock the the device structure for reads, writes
or both.

I implimented a 100ms pause between checking for new digits, but shorter
may be better.


By: dea (dea) 2007-05-03 11:17:21

Small think-o in the timeout detection.  

Oh, and as a small freebie surpise, I found that these changes
also impliment onhook dialing.  You can dial a number with the phone
onhook, pickup the handset/speakerphone or press dial and your call
goes through...
By: dea (dea) 2007-05-04 11:13:51

Saw that more DTMF fixes have been commited to branches/1.4
yesterday, so I ran another round of tests.  Still no joy
for chan_skinny

I'm going to be unavailable for two weeks, so I won't be able
to address issues with my patch after the end of the day for
awhile.

I have a direction/strategy question:
My approach to fixing this has been to rework chan_skinny to not
depend on the core for collecting digits to be dialed.  If it
were not for the changes in DTMF handling, I would classify this
approach as a new feature.  Even without the changes to DTMF
handling the current digit collection process in the channel is
odd/fragile.  

So the question is:  Do I submit the patch as a seperate bug-id as
a feature enhancement (1.6 material)?
By: Michiel van Baak (mvanbaak) 2007-05-05 04:22:48

Because without this patch, or the workaround, dialing with a skinny phone is not possible I would classify this as a bug.

For now I'm using the workaround and it works fine. This weekend I will have some time so I will try your patch to chan_skinny.
By: dea (dea) 2007-05-05 10:44:52

Don't get me wrong, I think the approach I took is the correct
long-term strategy.

The current issue is a result of DTMF changes in the core, which
are still being worked on.  There is a chance that it can be fixed
there.  If it is, then my patch is more of a feature enhancement.

The other side of the arguement is that the current pre-dial digit
collection in chan_skinny places extra restrictions on what changes
can be safely made in core DTMF detection/handling, and my patch
removes those restrictions.
By: pj (pj) 2007-05-07 13:26:40

patching chan_skinny with v4 patch always fail,
tried latest asterisk revision and r62737

patching file channels/chan_skinny.c
Hunk #1 succeeded at 1001 (offset 8 lines).
Hunk #2 succeeded at 2447 (offset 176 lines).
Hunk #3 succeeded at 2492 (offset 176 lines).
Hunk #4 FAILED at 2509.
Hunk ASTERISK-1 succeeded at 2525 (offset 177 lines).
Hunk ASTERISK-2 succeeded at 4280 (offset 248 lines).
1 out of 6 hunks FAILED -- saving rejects to file channels/chan_skinny.c.rej
By: dea (dea) 2007-05-20 17:38:56

I'll get an updated patch posted tomorrow, but looking
at that section, other than a couple logging statements
the only change in that section is to replace every
instance of the variable exten with d->exten

** Edit **
PJ-
 Branches or trunk?

I just applied the patch to branches/1.4 revision 65250 without
any issue.


By: pj (pj) 2007-05-23 14:28:26

I deleted asterisk src directory and make clean/new asterisk svn trunk checkout:
Checked out revision 65675.

DEA, can you check, if uploaded patch is correct?
here is md5sum of patch I downloaded:
a42ab3176b16231221db0a84b16db13d  skinny-predial-dtmf-v4.patch


patch -p0 < skinny-predial-dtmf-v4.patch
patching file channels/chan_skinny.c
Hunk #1 succeeded at 1001 (offset 8 lines).
Hunk #2 FAILED at 2279.
Hunk #3 succeeded at 2417 (offset 101 lines).
Hunk #4 FAILED at 2434.
Hunk ASTERISK-1 succeeded at 2453 (offset 105 lines).
Hunk ASTERISK-2 succeeded at 4260 (offset 228 lines).
2 out of 6 hunks FAILED -- saving rejects to file channels/chan_skinny.c.rej
By: dea (dea) 2007-05-23 16:22:38

The patches are against branches/1.4 (oldest broken version 1st policy),
and may not apply to trunk.

I can spin a fix for trunk if you need it there, but I'd like to get
someone to agree/veto the approach.
By: pj (pj) 2007-05-23 16:59:14

branches are also affected by this issue? I thought that branches are only bugfixed and feature changes applies only for trunk, like new dtmf code that appears between r61478 and r61800 of trunk and probably made this chan_skinny dialing issue :-\
By: dea (dea) 2007-05-23 17:07:09

The new DTMF code also appears to have been in branches.

I started poking at this when skinny dialing broke just before 1.4.3,
and have been developing against a SVN checkout around that time.

I have not tested the tarballs, but svn branches/1.4 is still broken
as of two days ago.
By: pj (pj) 2007-05-24 01:32:22

if chan_skinny is currently broken even in 1.4branch and trunk, I think that DEA's patch should be quickly incorporated into asterisk. any quick fix that solves this issue is better than completely useless chan_skinny.
By: dea (dea) 2007-05-24 10:51:16

Testing will be the key.  PJ, if you cannot use/test branches, let me know
and I will take the time to cut a patch for trunk.
By: pj (pj) 2007-05-24 10:58:49

right, I'm using only trunk versions in semiproduction environment;-)
By: dea (dea) 2007-05-24 12:09:04

Here's a patch for Trunk as of revision 65929.
Compile tested and a single placed call (needed to revert
back to my branches/1.4 testing/dev environment)

Let me know if it still has problems.
By: pj (pj) 2007-05-24 15:59:45

thank for patch for trunk, but it doesn't work for me (tested with 7920), still only dialtone, after timeout channel is hangup, dialing is not possible ;-(
chan_skinny.c was patched successfully

unknown1 in handle_stimulus_message is '0'
Received Stimulus: Line(1)
Setting ringer mode to '1'.
skinny_new: tmp->nativeformats=8 fmt=8
Attempting to Clear display on Skinny 324@PJ
Clearing Display
Found device: PJ
Found device: PJ
   -- Starting simple switch on '324@PJ'
Collected digit: [9]
[May 24 22:55:03] WARNING[6016]: chan_skinny.c:1210 find_line_by_instance: Could not find line with instance '0' on device 'PJ'
Collected digit: [5]
[May 24 22:55:03] WARNING[6016]: chan_skinny.c:1210 find_line_by_instance: Could not find line with instance '0' on device 'PJ'
Collected digit: [9]
[May 24 22:55:03] WARNING[6016]: chan_skinny.c:1210 find_line_by_instance: Could not find line with instance '0' on device 'PJ'
   -- Asked to indicate 'Stop tone' condition on channel Skinny/324@PJ-3
skinny_hangup(Skinny/324@PJ-3) on 324@PJ


By: dea (dea) 2007-05-24 18:58:15

I have to play with my 7920 when I get back into the office.

If I recall it works like a cell phone, you dial, then send right?

I don't see a couple of log messages that I would have expected to
see that were introduced/modified in my patch.

In skinny_ss() just above the first while loop(line 2384) line please add

ast_verbose("Extension so far: %s\n", d->exten);

That should print the collected digits.

If it does not show anything interesting, we can move it into the
while loop above timeout = 0;

If you don't get a chance to test it, I can try to reconfigure
my 7920 to talk to *
By: pj (pj) 2007-05-25 01:48:30

I'm dialing with 7920 like with gsm phone, type number '959' and then press dial button, on 7920 is even not possible to use another method like hangoff and then type number.
dialing from phone book, or from last dialed number list is also not possible, same effect - number is on display, but only dial tone can be hear.
today I'm out of office, so I can try another tests next week. thank you.
By: dea (dea) 2007-05-25 11:57:17

Yup.  The 7920 does some odd things.  No surprise there.

It tends to send the line instance as 0, but chan_skinny
starts counting at 1.  This can also happen on other phones
with onhook dialing.  Small tweak added to 'assume' line 1
if the phone requests line 0.  This fixes part of the remaining
7920 issue and cleans up logs for other phones.

The second remaining issue with the 7920 is that it uses 0 for
the call reference as well, my only other phone seems to start with
2.  In this case if the phone requests reference 0, I select the first
sub channel linked to the line (this may need work/error handling)

So now 7920 phones can dial.  As a by-product I eliminated logs like:
Could not find line with instance '0'
and:
Could not find subchannel with reference '0' on <some device>

Light testing here, calls in and out work.


By: pj (pj) 2007-05-25 14:49:54

great work it working for me, thanks:-)
only small issue is, that now you always clear display when call is completed, it has possitive efect that "Call Progress" od "Connected" messages do not stays on display, as reported in:
0007788: [branch] Cisco 7920 Phone Screen not Cleared on Call Complete
but negative is, that display is also cleared after missed call to skinny phone, missed call info should always stay on phone display.
By: dea (dea) 2007-05-25 15:54:53

The good news is that logic to handle that case would not be
difficult to impliment.

BUT my patch doesn't have any code to clear the display, so that
is an artifact coming from somewhere else.

Thanks for reporting the success.  Maybe we can get another
positive report or two and get this considered for commit.
By: dea (dea) 2007-05-25 21:53:08

OK, so my patch in this bug did not introduce the issue with
excessive clearing of the display, but my vm/speeddial/redial
patch that was added to trunk re-introduced it.

The code was there, but I uncommented it and fixed it up.

In do_housekeeping on line 1798 you can try commenting out this line:
transmit_displaymessage(s, NULL);

A better fix would be to add a flag to the device structure that
a call was missed and not clear the display.  Any activity on the
phone, keypad, going off-hook would clear the flag.

I'll work on that next week.
By: Michiel van Baak (mvanbaak) 2007-05-30 11:21:48

This patch is working for me as well.
Running svn trunk r66585M with this and my regexten/regcontext patch.
Phones tested: 7960 and 7905
By: pj (pj) 2007-05-30 15:45:47

mvanbaak:
do you have also problems with missed calls clearing in status bar on display your 7905/60 as I have on 7920 with latest DEA's patch for trunk?
By: Michiel van Baak (mvanbaak) 2007-05-30 16:28:29

PJ: like DEA said, the display clearing was not introduced by this patch.
I have to say I did not test that yet, had little time at home today to do more testing then: "does it work? yes. fine. report it works."
This weekend I planned to do more skinny development/testing so I will be able to tell you then.
There are some issues in both 1.4 and trunk, this is one of them and russel was trying to get enough supporters for a new 1.4 release so we really need this one in svn as soon as possible. Hopefully before the next release :)
By: dea (dea) 2007-05-30 17:01:08

I looked at making the clearing of the display in do_housekeeping
conditional, and decided I just didn't like it.  We already clear
the display on a number of keypad events, and that seems to be the
best choice.

So to fix PJs issue (not related to this bug), just comment out
or delete this line in do_housekeeping:
transmit_displaymessage(s, NULL);


mvanbaak-  I have a couple items I want to address in chan_skinny.
If you are on the -dev list we can get together there to compare
notes and avoid duplicating our efforts.
By: Michiel van Baak (mvanbaak) 2007-05-30 17:51:19

DEA: I am indeed on the -dev list.
I will post my stuff there soon (tomorrow or saturday, depends on work)

This one is high on list too ;)

Let's start a thread on -dev, that will be a better place then mantis...
By: Russell Bryant (russell) 2007-06-01 14:42:29

This patch has been added to 1.4 and trunk in revisions 66881 and 66882.  Thank you very much for your work!