[Home]

Summary:ASTERISK-08816: [patch] Playback(<file>|noanswer) and in-band info are not working with chan_skinny
Reporter:pj (pj)Labels:
Date Opened:2007-02-15 13:08:48.000-0600Date Closed:2008-04-07 14:06:26
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) earlyrtp.diff
( 1) earlyrtp2.diff
( 2) earlyrtp2a.diff
( 3) skinny_log.txt
( 4) skinny-9077-earlymedia.diff
Description:tried to playback with noanswer option, it works with chan_sip or chan_iax, but not with chan_skinny, also other inband info messages from telco are not possible with chan_skinny.
eg.: when calling to unavailable gsm phone and telco operator says "person you are calling isn't available" chan_skinny plays normal ringing tone, chan_sip and chan_iax plays this in-band messages fine.
tested with ci$co 7920.
Comments:By: Serge Vecher (serge-v) 2007-02-19 08:56:35.000-0600

can you post some logs, please?
By: pj (pj) 2007-02-19 10:00:43.000-0600

here is my "skinny set debug", probably nothing usefull can be seen here,
messages is played only after Answer.
this is only simple test to demonstrate, other issues with impossibility to hear any inband info from telco is probably related to issue with Playback noanswer.


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
   -- Starting simple switch on '324@PJ'
Setting ringer mode to '1'.
Collected digit: [9]
   -- Asked to indicate 'Stop tone' condition on channel Skinny/324@PJ-7
Collected digit: [5]
   -- Asked to indicate 'Stop tone' condition on channel Skinny/324@PJ-7
Collected digit: [6]
   -- Executing [956@test:1] Playback("Skinny/324@PJ-7", "enum-lookup-successful|noanswer") in new stack
   -- <Skinny/324@PJ-7> Playing 'enum-lookup-successful' (language 'en')
   -- Executing [956@test:2] Wait("Skinny/324@PJ-7", "2") in new stack
   -- Executing [956@test:3] Answer("Skinny/324@PJ-7", "") in new stack
skinny_answer(Skinny/324@PJ-7) on 324@PJ-7
Displaying Prompt Status 'Connected'
   -- Executing [956@test:4] Wait("Skinny/324@PJ-7", "2") in new stack
Received Open Receive Channel Ack
ipaddr = 10.0.0.1:29110
ourip = 193.179.xx.xx:15906
Setting payloadType to '8' (20 ms)
   -- Executing [956@test:5] Playback("Skinny/324@PJ-7", "enum-lookup-successful|noanswer") in new stack
   -- <Skinny/324@PJ-7> Playing 'enum-lookup-successful' (language 'en')
   -- Executing [956@test:6] Hangup("Skinny/324@PJ-7", "") in new stack
 == Spawn extension (test, 956, 6) exited non-zero on 'Skinny/324@PJ-7'
skinny_hangup(Skinny/324@PJ-7) on 324@PJ
Clearing Display





 956 => {
       Playback(enum-lookup-successful|noanswer);
       Wait(2);
       Answer;
       Wait(2);
       Playback(enum-lookup-successful|noanswer);
       Hangup;
 }
By: Jason Parker (jparker) 2007-08-02 15:37:02

Patch uploaded.  I don't know if this is right, but it seems to work.

Basically, we just start the rtp as soon as we start the call, instead of waiting for an answer.
By: pj (pj) 2007-08-03 14:41:17

patch isn't working as expected (or how this is working with sip), it is usable for cases like Playback(<file>|noanswer)
but if I call to non existent pstn number or unavailable gsm/cell phone, I got both ringtone and progress messages from telco.
this messages can be hear only between ringtone silent periods, so I hear: ringing-message-ringing-message...
By: Jason Parker (jparker) 2007-08-03 15:07:31

Can you show a log of what happens when there is inband information, such as the invalid number or unavailable gsm phone cases?
By: pj (pj) 2007-08-03 15:18:35

my setup:
skinny-->asteriskA---iax---asteriskB--->telco

console log from asteriskA (where skinny phone si attached)
[Aug  3 22:31:19]     -- IAX2/bill-gw-4 is making progress passing it to Skinny/324@PJ3-11

this log is exactly same when I calling using sip phone (where progress messages is working):
[Aug  3 22:33:56]     -- IAX2/bill-gw-2 is making progress passing it to SIP/510-082084e8

p.s. I do not use fake ringback in dial command.
By: Jason Parker (jparker) 2007-08-03 15:41:21

I need to see the full log of the asterisk server with the skinny phone connected.  I need to see if there are indications coming in properly, and whether we're handling them.
By: pj (pj) 2007-08-03 15:49:37

debug log attached.
By: Jason Parker (jparker) 2007-08-03 16:01:37

Okay, I think I see what's going on.  Try this...

in skinny_indicate(), under the AST_CONTROL_PROGRESS case, remove the "&& !sub->outgoing", and try it again.

For some reason, somebody decided that PROGRESS shouldn't be handled on outgoing calls - but I think the logic is backwards (or not needed at all, since you probably won't ever get PROGRESS on an incoming call).
By: Jason Parker (jparker) 2007-08-03 16:02:16

I should also mention - this is in addition to the skinny-9077-earlymedia.diff patch.  These are technically two separate, but very related issues (the latter depends on the fix from the former).
By: dea (dea) 2007-08-03 16:02:48

The issue will be in skinny_indicate() in the case of AST_CONTROL_PROGRESS.

We tell the phone to send SKINNY_ALERT tone.  It might be as simple as
sending SKINNY_SILENCE in this case, which would allow inband to pass
unmolested.

Or it might be more complicated.  I'll try to make time to test the idea.


** Edit **
I'm a slow typist and Qwell is quicker.  I think the SKINNY_ALERT tone will still be an issue even with the removal of !sub->outgoing


By: Jason Parker (jparker) 2007-08-03 16:08:00

SKINNY_ALERT may be an issue, but I think another part of the puzzle is that we don't stop the ringing.

In the AST_CONTROL_PROGRESS case, you may also want to do a:

transmit_ringer_mode(s, SKINNY_RING_OFF);
By: dea (dea) 2007-08-03 16:21:51

I believe that transmit_ringer_mode controls the phone ringer and not
the ringback.  transmit_tone starts the ringback when SKINNY_ALERT is
sent and clears it when SKINNY_SILENCE (or any of the other tones) is sent.
By: Jason Parker (jparker) 2007-08-03 16:25:31

DEA: that sounds about right.

So, I guess we need to remove the "&& !sub->outgoing", and change SKINNY_ALERT to SKINNY_SILENCE
By: dea (dea) 2007-08-03 16:47:14

Oddly enough PJ's logs showed that it did get into the AST_CONTROL_PROGRESS case even with the !sub->outgoing

So while I agree the test is pointless and should be removed, the log points
to the possibility that that variable is mis-used/mis-set elsewhere in the code.
By: Jason Parker (jparker) 2007-08-03 16:52:59

wedhorn just brought to my attention that the "outgoing" variable is incorrectly named.  This is set to 1 on *incoming* calls to the phone.
By: Jason Parker (jparker) 2007-08-03 16:54:08

Actually, I guess that would mean that all we really have to do is change the SKINNY_ALERT to SKINNY_SILENCE?

(and as a result of the findings on the outgoing variable - we should probably rename it)
By: dea (dea) 2007-08-03 16:55:44

I think that is the case.  I am not aware of a local number that I can test
with to confirm this, so I am basing my option on the code...
By: Damien Wedhorn (wedhorn) 2007-08-03 18:17:47

Added a patch based on comments and discussions (earlyrtp.diff).

Include an option in skinny.conf to enable early RTP, defaults to early RTP. Basically, if the device is set to earlyrtp, skinny_indicate does not send a transmit_tone to the device, but return -1 to asterisk which tells asterisk to do inband (thanks to qwell and russellb).

The structure is horrible because I didn't want to change the order that packets are sent to the device so we have to if statements for each indicate. Could clean up if skinny devices are happy to receive the transmit_tone after the other stuff.

Works for me (tones are obviously different to the device tones) on 30VIPs. Haven't tried a playback. Also, haven't change sub->outgoing to sub->incoming.
By: dea (dea) 2007-08-03 18:19:19

Very nice wedhorn.  Looks to cover all of the requirements and is even
optional.  It might be argued that since this is new behaviour,
it should default to off, but I think this a reasonable improvement and
should be the default.

I think we're best suited with the embedded if() conditions.  A number of
resent bugs were related to how pick some skinny phones can be about the
order of packets, and how they impact the phones ability to track callstate.


By: pj (pj) 2007-08-04 02:17:04

now it works as expected, ready for trunk, thanks.
btw, is there any reason, why to disable earlyrtp for skinny? if option for this is really needed to be in skinny.conf


By: pj (pj) 2007-09-05 14:27:36

is there any reason, why not to include this work into trunk? when I last tested, it works. or do you plan to do some improvements in this? I think, only one: not to include config option to turn off earlyrtp, because it is inconsistent with other channels (sip), where earlyrtp is also by default on, and can't be disabled (as I know) and no practical reason exist, why to disable earlyrtp.
By: pj (pj) 2007-10-09 19:39:06

this patch is failing applying to current trunk (r85177), please update and/or merge into trunk...

patching file channels/chan_skinny.c
Hunk #1 succeeded at 1194 (offset 139 lines).
Hunk #2 succeeded at 2736 (offset 445 lines).
Hunk #3 FAILED at 2763.
By: Damien Wedhorn (wedhorn) 2007-11-17 20:32:36.000-0600

New version (against 89394). This is a different way of hopefully doing the same thing. The advantage of this one is that generally the code isn't aware of whether the indicating is inband or outofband. Basically, added new function skinny_play_tone that replaces transmit_tone. If there is RTP, it plays the tone inband, else it uses transmit_tone.

pj, can you test and see if it still gives you the result you are after.

PS we should really include tonezones in skinny.conf for each device.
By: Damien Wedhorn (wedhorn) 2007-11-17 21:33:49.000-0600

earlyrtp2a.diff uploaded. Same as earlyrtp2.diff except that it includes device configurable tonezones. Tested with tonezone=au (Australia). Correct tones played. Note that you pretty much need earlyrtp as these tones only get used once rtp has started.
By: pj (pj) 2007-11-18 06:39:20.000-0600

tested earlyrtp2a.diff with Asterisk SVN-trunk-r89394M
ringback, early media (progress messages from telco), playback(,noanswer) is working fine with skinny phone ci$co 7920, thanks
I vote for commit patch into trunk.
By: pj (pj) 2007-11-26 11:39:08.000-0600

wedhorn, do you plan still to do some work on this patch? It's working quite well for me, so I think it can be commited into trunk for regular testing. thanks
By: Damien Wedhorn (wedhorn) 2007-11-26 14:42:27.000-0600

pj, I've discussed this patch with Qwell and while the patches work, they arguably go to far in that they don't use the device tones when they probably should. So, more work is required before it can be committed.
By: pj (pj) 2008-01-20 15:22:52.000-0600

wedhorn, qwell some days ago, your patch stops to apply successfully to asterisk trunk, can you update? it would be great, if this issue will be resolved in upcomming asterisk 1.6
By: Jason Parker (jparker) 2008-03-17 16:56:26

I would be much more comfortable committing earlyrtp.diff if it works as expected.  Thoughts?
By: pj (pj) 2008-03-17 23:46:14

agree, as I suggested in msg 0074341, but wedhorn answers, according to discussion with you, Qwell, that: "more work is required before it can be committed."
By: Damien Wedhorn (wedhorn) 2008-03-18 01:14:19

I disagree with earlyrtp.

Basically the patches are:
- earlyrtp: tell asterisk to do the tones
- earlyrtp2: get chan_skinny to do the tones and from a single function
- earlyrtp2a: same as earlyrtp2 plus ability to select tonezone

I think that basically throwing an error and getting asterisk to provide the tones is incorrect. As much as everyone loves comparing skinny to sip/iax, it must be understood that in configurability, it is actually closer to zap (ok, I admit that IP is used as the transport, but that is really the only similarity with SIP and IAX). A sip/iax device is configurable. A skinny device is not. All of the configuration needs to be done in chan_skinny.

sip_indicate only returns -1 (and gets asterisk to play them) if there doesn't seem to be a reasonable way to signal the device. zt_indicate plays the tones from within chan_zap and only returns -1 if the play fails.

Based on this, I think we should handle the tone playing within chan_skinny. Also, I vote for the inclusion of the tonezone configuration as there is nowhere else that it could be put to allow for per device configuration.

All of that said, I am happy to do any work required in getting something committed such as possibly treating old and new telephones differently due to the ability of new phones to use signalling even when there is rtp.

Partially related and something I think needs fixing (although it applies to a bunch of other channels), when I did the transfer stuff, I had to inject tones if required at the transfer leg rather than just indicating to the transferring channel that it should play a tone. This seems to be a common approach, although I can't work out why. I follow suit because we didn't have the ability to play tones inband, but with earlyrtp2, the transfer could be cleaned up to just send and ast_indicate rather than injecting a tone (other channels I think would also need some work).
By: Digium Subversion (svnbot) 2008-04-07 12:55:46

Repository: asterisk
Revision: 113118

U   branches/1.4/channels/chan_skinny.c
U   branches/1.4/configs/skinny.conf.sample

------------------------------------------------------------------------
r113118 | qwell | 2008-04-07 12:55:45 -0500 (Mon, 07 Apr 2008) | 8 lines

Allow playback with noanswer (and add earlyrtp option).

(closes issue ASTERISK-8816)
Reported by: pj
Patches:
     earlyrtp.diff uploaded by wedhorn (license 30)
Tested by: pj, qwell, DEA, wedhorn

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

http://svn.digium.com/view/asterisk?view=rev&revision=113118
By: Digium Subversion (svnbot) 2008-04-07 12:58:05

Repository: asterisk
Revision: 113119

_U  trunk/
U   trunk/channels/chan_skinny.c
U   trunk/configs/skinny.conf.sample

------------------------------------------------------------------------
r113119 | qwell | 2008-04-07 12:58:03 -0500 (Mon, 07 Apr 2008) | 16 lines

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

........
r113118 | qwell | 2008-04-07 13:00:09 -0500 (Mon, 07 Apr 2008) | 8 lines

Allow playback with noanswer (and add earlyrtp option).

(closes issue ASTERISK-8816)
Reported by: pj
Patches:
     earlyrtp.diff uploaded by wedhorn (license 30)
Tested by: pj, qwell, DEA, wedhorn

........

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

http://svn.digium.com/view/asterisk?view=rev&revision=113119
By: Digium Subversion (svnbot) 2008-04-07 14:05:56

Repository: asterisk
Revision: 113174

_U  branches/1.6.0/
U   branches/1.6.0/channels/chan_skinny.c
U   branches/1.6.0/configs/skinny.conf.sample

------------------------------------------------------------------------
r113174 | qwell | 2008-04-07 14:05:56 -0500 (Mon, 07 Apr 2008) | 24 lines

Merged revisions 113119 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

................
r113119 | qwell | 2008-04-07 13:02:51 -0500 (Mon, 07 Apr 2008) | 16 lines

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

........
r113118 | qwell | 2008-04-07 13:00:09 -0500 (Mon, 07 Apr 2008) | 8 lines

Allow playback with noanswer (and add earlyrtp option).

(closes issue ASTERISK-8816)
Reported by: pj
Patches:
     earlyrtp.diff uploaded by wedhorn (license 30)
Tested by: pj, qwell, DEA, wedhorn

........

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

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

http://svn.digium.com/view/asterisk?view=rev&revision=113174
By: Jason Parker (jparker) 2008-04-07 14:06:26

I opted to commit the earlyrtp.diff, as I felt much more comfortable with that.  I don't think we should be doing the playtones directly, as in earlyrtp2a.diff