[Home]

Summary:ASTERISK-09642: [branch] Transfer implementation
Reporter:Damien Wedhorn (wedhorn)Labels:
Date Opened:2007-06-11 04:33:45Date Closed:2008-06-25 14:43:00
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_skinny-more-packet-cleanup.txt
( 1) chan_skinny-transfer-trunk.txt
( 2) chan_skinny-transfer-trunk-v10.txt
( 3) chan_skinny-transfer-trunk-v11.txt
( 4) chan_skinny-transfer-trunk-v12.txt
( 5) chan_skinny-transfer-trunk-v2.txt
( 6) chan_skinny-transfer-trunk-v3.txt
( 7) chan_skinny-transfer-trunk-v4.txt
( 8) chan_skinny-transfer-trunk-v5.txt
( 9) chan_skinny-transfer-trunk-v8.txt
(10) chan_skinny-transfer-trunk-v9.txt
(11) full.asterisk.vanbaak.info
(12) transfer_v6.diff
(13) transfer_v6a.diff
(14) transfer_v6b.diff
(15) transfer_v7.diff
(16) transfer1.diff
(17) transfer2.diff
Description:Initial patch adding transfer to chan_skinny. Works, but lots of bugs and more work to be done. Basic functionality is <XFER> to start transfer, puts call on hold and gives you a dialtone. Dial number and after connected press <XFER> again and call is transferred.

After first <XFER> press and before call ringing/connected, you can press <XFER> again to use a blind transfer. In this case transfer occurs when either channel being dialed indicates ringing or answering. When in blind transfer you can press <XFER> again to go back to attended transfer.

<HOLD> toggles between the transferee and transferor.

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

Indications are on the light side (basically none). Asterisk doesn't exit cleanly. I'm sure there's lots more.

Discussion point, the way we are heading with chan_skinny, I think we should rethink the find_sub stuff. We really need to know the active sub on each line, and the active line on each device (probably worth including active sub for the device as well which equals d->activeline->activesub). I've found that the find_sub routines really aren't giving me the sub I am after. I've partially hacked in l->activesub, but it is not complete. Should we remove the find_sub stuff and move to a more direct method of finding subs? Comments?
Comments:By: dea (dea) 2007-06-11 13:16:46

Cool.  The sub features do need some rework, and you've started down the path I was considering.

The unclean exit points to orphaned channels, which I found when adding
the speeddial/redial feature.  We were blindly creating new channels
when we already had a channel.  You patch does not seem to suffer from that,
but I will give it a closer review.

One thing to note about finding the subs, onhook actions tend to report
line 0 (the 7920 appears to always report line 0).  For most devices this
should be interpreted as line 1.  I have just such a fix in my code, but
I have not audited all call paths to make sure it does not break code that
will accept/want line 0.
By: Damien Wedhorn (wedhorn) 2007-06-11 17:19:16

The line 0 issue occurs with the old phones (30VIP and 12SP) as well. These are multiline phones so interpreting as line 1 will lead to issues with these phones. For example, the hold button on a 30VIP will send the HOLD with an instance of 0. We need to know which line and which sub to apply the hold to. Hence the activeline and activesub suggestions.

The only issue I can see if a device could send a message regarding a sub or line that is not the active device or line, but I guess it would be fairly easy and grab the instance and callid and if they are zero use activeline and activesub. Are any devices capable of sending messages that are not related to the active channel (apart from the line buttons of course)?
By: dea (dea) 2007-06-11 17:38:27

The patch does not work for me, but I have a number of changes that
are pending merge from other bugids here.

The core issue is that the phone is sending the wrong call reference,
so no events are handled against the right sub-channel, including
digit collection for dialing...

Still poking though.  If I can figure out why we are getting the wrong
call reference, the ->activesub concept might not be needed.  We only
need to track the number of active sub channels (in any state).
By: dea (dea) 2007-06-11 22:44:16

I can see the point behind activeline, but activesub appears to be the
same as sub after your patch.

I may have uncovered another bit of the mystery in odd line/call-reference
numbers.  The server response packets for transmit_tone and transmit_displaymessage contain the line and callid (reference)

I think that they are part of the process to notify the phone which
call/line is active so any keypad activity will be associated with
the correct call.
By: Damien Wedhorn (wedhorn) 2007-06-11 23:20:04

activesub is needed for the hold toggling. If you do <XFER> then <HOLD>, the active sub is actually line->sub->nextsub. At a later date when we implement callwaiting into the batch, any of the numerous subs under a line could be the active sub.

So far I really haven't looked too hard at notifying the device what is actually going on. I may have overlooked notifying the devices of a change to the active channel. Also, there are a bunch of device states that could play a part here.

Unrelated, but when we implement callwaiting, we really should add subs to the end of the sub->nextsub queue rather than at the beginning of line->sub. We really want a forward list so hold passes us on to the next call on hold and not the previous. We could implement a backward list, but I think that just a single forward list would be sufficient.
By: dea (dea) 2007-06-13 16:31:33

Success!

The transfer code is fine, what is missing is that digit collection
needed a small change:

Near line 4552
if (sub && (sub->owner->_state <  AST_STATE_UP)) {

needs to be:
if (sub && ((sub->owner->_state <  AST_STATE_UP) || sub->onhold)) {

That small change allows me to perform attended transfers.  Blind transfers
need a small change in handle_transfer, change KEYDEF_OFFHOOK to
KEYDEF_OFFHOOKWITHFEAT and add the Transfer button to that template
to get the correct softkey template

wedhorn, can you integrate the above changes into this patch?  Other than
that, I would say this is ready for Trunk....


** Edit  **
One more small enhancement that should be included is to add a
transmit_callstate() to skinny_hold() and skinny_unhold() just
above the calls to transmit_lamp_indication() in each function.

skinny_hold():
transmit_callstate(s, l->instance, SKINNY_HOLD, sub->callid);

skinny_unhold():
transmit_callstate(s, l->instance, SKINNY_CONNECTED, sub->callid);

As always, disclaimer on file.


By: Damien Wedhorn (wedhorn) 2007-06-13 22:37:47

There is still the issue about the dirty exit that needs to be fixed. I'm happy for you to do add your changes and post a new patch if you wish, and fix up the cleanup mess if you really want to. I have a disclaimer on file, qwell can confirm (or see last comment of http://bugs.digium.com/view.php?id=8995 ).

I won't have time for much skinny work for the next week or so (missus gets home today after a month overseas and out of contact, one of my kids birthday today, the other one on saturday and 3 exams next week). Doing anything on chan_skinny could lead to divorce and/or uni failure :\ Anyway, I'll still be checking mantis and -dev.

ps it may be cleaner to include a transmit_callstate in handle_hold() rather than in hold() and unhold() so you don't send 2 different callstates in such a short timeframe.


By: dea (dea) 2007-06-14 01:04:25

I am not getting unclean exits with my changes to your patch, but I
have a heap of other pending skinny changes mixed together.  If I did
not have all of these other changes, I'd submit my updates to your
approach.

I considered putting the transmit_callstate in handle_hold, but it would
require 'walking' the sub to get to line details, thereby increasing the
stack footprint.  We need and already have the line details in _hold()
and _unhold(), so it made sense to put them there.

I understand the family/work issue.  My wife just came back from a week
in Japan and I caught hell for working on * instead of my chores ;-)

I'll be happy to maintain your patch going forward if that is OK with
you....
By: pj (pj) 2007-06-14 02:09:00

can you update transfer patch according to current asterisk trunk?
it fails to apply to trunk rev69257

sorry, it's successfully applied to clean trunk, but is probably in colision with other patch, that I'm currently using for sip/skinny...
0008824: [patch] Remote (called) Party Identification - chan_sip & chan_skinny


By: Damien Wedhorn (wedhorn) 2007-06-14 03:21:29

Uploaded a new patch with DEA's suggestions. Now based on revision 69257. It compiles and runs, although I can't check as I don't have physical access to the phones at the moment (ie at work).

I still get the messy exit, not that messy, but apparently still wanting. On exit I get "Yuck! Error in buffer handling...: Connection reset by peer". I think this still needs fixing.

Dan, happy for you to look after it.

Edit: pj, a quick glance at the other patch and it looks like there are a few clashes.


By: pj (pj) 2007-06-14 07:13:33

A calls B
B want to transfer to C,
B cancels consultative call with C,
B can't resume call with A,
when B press transfer softkey again to call another party D (without calling number), it immediatelly transfers A to C.

it's probably related to hold/resume issue, as I reported in note 0065129 in bugreport 0009887.


By: dea (dea) 2007-06-14 10:58:14

The code will need to be tweaked to allow a caller to revert back to the
original call.  I think we'll need to add a Cancel button to the softkey
template OffHookWithFeatures and let it handle tearing down the second
leg of the call and reverting back to the original channel.
By: dea (dea) 2007-06-14 11:36:50

Oh, and both transfer/hold/resume depend on additional fixes to the
packet structures that have not been posted yet.

I'll try to get a small patch out that addresses just the packet
structures and the code that populates them.  The missing bit is that
a number of packets need to have the line and call reference number in
them that we do not populate today.  These packets 'help' the phone
determine which call is active so that stimulus messages will be linked
to the correct call.
By: Jason Parker (jparker) 2007-06-14 12:36:35

How does CCM handle a canceled call transfer?  Is a Cancel softkey defined?

The only method I'm familiar with is analog three-way calling: You pick up the handset, make a call, flashhook, and if you hangup before dialing a number, your phone will ring - and when you answer, you'll be reconnected with the caller.
By: dea (dea) 2007-06-14 12:41:40

Sorry, bad memory on my part.  There is no cancel, just an 'End Call'

End Call depends on the phone 'knowing' which call is active, which requires
cleanup/fixing more of the packet structures to include the line/call reference
numbers.

I also see a need to make skinny_ss() aware that a call/channel has gone away,
which I think will address the problems with calls being aborted mid-dial and
asterisk getting very unhappy.

Attached is another patch that addresses the packet structures and code that
sets their values (more work is likely needed here).
By: dea (dea) 2007-06-15 16:11:38

More cleanups, packet structure fixes, etc...

The biggest change, which impacts transfers (and why the patch is here),
is updates to the on_hook and EndCall events.  The channel now attempts
to behave properly when there is only one subchannel on a line, or more
than one (small bit of work still needed here).

I did not need l->activesub, and found myself tripping over it, so it
is commented out in areas it gave me problems.  I still like the idea,
but we need to be more selective in where it is used.

The patch chan_skinny-transfer-trunk also has cleanup work that exists in
other patches.  It depends on those cleanups, and I could not really
seperate them.  If/when the other cleanup patches merge, I can regenerate
this patch.  

Current for SVN 69583
By: dea (dea) 2007-06-15 17:32:27

V2 adds correct handling of ending a sub, whether it is the 1st sub, last sub, or somewhere in the middle...

I still need to identify a good way for skinny_ss() to detect that
a subchannel has gone away.
By: dea (dea) 2007-06-19 11:40:49

Three of the packet structure fixes required by newer skinny phones
were reverted in the first couple of patches.  V3 re-adds these.
By: Damien Wedhorn (wedhorn) 2007-06-19 16:52:15

For info, the "Yuck!" message I am getting on exit is regarding music on hold and has nothing to do with this patch. When I remove MoH, I get a clean exit.
By: pj (pj) 2007-06-20 03:33:12

fresh trunk (r70272M) + chan_skinny-transfer-trunk-v3.txt, testing phones 2x7920, 1x7961:
transfer - OK (attended or unattended)
hold/resume - OK
time/date on 7961 still not working
issue is with calling multiple phones, like:
324 => { Dial(Skinny/${EXTEN}@PJ&SIP/${EXTEN}&Skinny/${EXTEN}@PJ2); Congestion; }
when I pickup one of three ringing phones, others skinny keep ringing forever,
this happen also if skinny phone has one call and phone gets another (second) incomming call, it rings forever, even if remote (calling site) hangs up.
By: Damien Wedhorn (wedhorn) 2007-06-20 07:12:53

pj, does that issue only occur with the above patch or is it an issue with trunk that deserves it's own bug?
By: pj (pj) 2007-06-20 08:27:52

with clean trunk, when using multiple phone ringing, issue with "forever ringing" doesn't appear
only persist issue when I send second call to 7961 - very strange things happen in this case, like not possibility to hangup phone or persistent open channels in asterisk, that I must softhangup from CLI, but even in this case, 7961 not ringing forever without transfer patch.

when phone is in "weird state", this appears on CLI, when trying to pickup&hangup.

[Jun 20 15:57:05] WARNING[28276]: chan_skinny.c:3864 handle_onhook_message: Skinny(324@PJ2-15) channel already destroyed
[Jun 20 15:57:07] WARNING[28276]: chan_skinny.c:3864 handle_onhook_message: Skinny(324@PJ2-15) channel already destroyed
[Jun 20 15:57:08] WARNING[28276]: chan_skinny.c:3864 handle_onhook_message: Skinny(324@PJ2-15) channel already destroyed
[Jun 20 15:57:08] WARNING[28276]: chan_skinny.c:3864 handle_onhook_message: Skinny(324@PJ2-15) channel already destroyed


sometimes this tests also bring asterisk to crash:

#0  0xb735c19d in handle_soft_key_event_message (req=<value optimized out>, s=0x81e9de0) at chan_skinny.c:4471
4471                    transmit_tone(s, SKINNY_DIALTONE, l->instance, sub->callid);


By: dea (dea) 2007-06-20 10:54:41

There is a good bit of code duplication between skinny_hangup, handle_onhook_messages and the ENDCALL softkey.

From my test calls, it looks like handle_onhook and the ENDCALL
softkey code should simply use ast_queue_hangup() and we do the
actual teardown in skinny_hangup().  That should fix the
"channel already destroyed" message.

The forever ringing looks more interesting.  You don't have a timeout
on the dial() call, but that would only limit the ringing duration, it
would not stop the other phones from ringing once one had answered.

That works with other channels, so we are missing a test or callback to
identify that the call has been answered.  I'll try to lab that up to
see what is missing.

Wedhorn, have you had a chance to review my changes?  A key part of
this patch is the hold/unhold call states with the correct packet structures.
My test phones now report the correct call reference for all key pad
events.  With the correct call reference, we can find the correct subchannel
and l->activesub is not needed.
By: dea (dea) 2007-06-20 11:51:15

PJ-  I cannot duplicate the issue with continued ringing using a 7920 and 7940 set to ring at the same time.  As soon as I answer one, the other stops ringing.

The date-time issue should be handled seperately.  I have some thoughts, can
you try the patch in 9779 and we can work on it there.
By: Damien Wedhorn (wedhorn) 2007-06-20 16:16:33

I've only had a chance to read your patch, not test it. I've got my final exam tomorrow, so I'll test it over the weekend. I notice your patch still uses activesub for the handling the hard buttons (handle_stimulus_message), it seems that the only information that we get with STIMULUS_MESSAGE is the stimulus and instance, although there is some unknown info (wireshark source doesn't seem to help).

The hard hold button on the 30VIP always seems to return an instance of 0, and no caller id (unless it is in unknown). In other words, we get an event with no information about which sub or line the event is for. I've only been testing my phone with a single line so far, I'll add some more lines to them and see how we go. But I'm guessing we'll probably need d->activeline and l->activesub, or d->activesub (where d->activesub->parent is d->activeline).

The other thing is that some phones do not have an endcall button and so we should probably use an onhook to do the same function (make activesub=related, hangup activesub, do not hangup activesub). This would result in being able to lift the receiver and press unhold to talk to the other party.

Just an idea, it may be cleaner to move the endcall, hangup and similar stuff to its own function and call it from the various places.
By: dea (dea) 2007-06-20 16:52:13

Good luck on your exam.

Without the fixed-up packet structure the phones tend to send
not so meaningfull values, especially for callreference.

If a packet structure has a lineinstance member and an unknown
member, there is a pretty good chance that the unknown is actually
the callreference, and I changed a few of the structures to reflect
that. (I noticed while going over the captures I had this fact, I
also found that a number of packets had the lineinstance/callrefence
is what looked to be the 'padding' area)

The last important point is that is the call_state packet, conferenceid
appears to be the callreference at least in every CCM call I have
captured.  I believe that this change actually allows the phone to
figure which call is active and to send button events with the correct
callreference.

I did not change the handle_stimulus code as I never seemed to trigger
it in testing.  I only got softkey events.  If you can trigger stimulus
events, the the code for that event might just be identical to the
softkey counterpart.
By: Damien Wedhorn (wedhorn) 2007-06-20 19:01:06

Thanks.

The handle_stimulus seems to be predominantly for hard buttons (typically on the older phones). My phones (old phones) only trigger the handle_stimulus and never the handle_softkey (they don't have any softkeys). The only hard buttons on newer devices seems to be the line buttons, although there may be others.

The stimulus_message seems to be really short. I had a quick look at some CCM dumps I had and the only information that seem to be in the packet was the stimulus_message and the instance. My 30VIPs return an instance of 0 when the hold button is pressed, so I don't think that we have any information at all about which line and/or which sub the message should be applied to.

On the activesub stuff, I think we'll need d->activeline and l->activesub. I a previous post I indicated that we may get away with a d->activesub, but, an inactive line may have an activesub and so this won't work.

Over the weekend I'll look through the dumps I have and grab some more and see if there is anything at all that we could use to remove the activesub stuff, but I honestly don't think I'll find anything.

If we need to keep the activesub (and include activeline), can I get your thoughts on how we implement. As far as I know, with all phones, events can only be used against the activesub. Is it possible to through an event that does not operate on the active sub? If it is not, and if we need the activesub stuff, I would suggest that we use activesub/activeline as the default sub and line selection and we can test against any callreference and instance information provided by the device (and either log an error or just quietly change the sub/line info. Comments?
By: dea (dea) 2007-06-20 19:32:07

The stimulus message structure lists a uint32_t unknown1 in chan_skinny

I suspect that is the callreference.  Until the hold/call_state fixes
get merged calls involving * will always show 0 in the value.

I do not have any phones to test/trigger this theory, but we can test
it next week.  Note that I tweaked the logging in handle_softkey to
include the callreference received in the event (1/1) line 1/call 1

A similar tweak in handle_stimulus will help prove/disprove my theory.

I am not opposed to d->activesub/l->activesub, but I suspect we can
live without it.
By: Damien Wedhorn (wedhorn) 2007-06-20 20:15:29

I don't have access to wireshark at the moment so I can't check, but when I had a look earlier it seemed that the stimulus message only consisted of two bits, the messagetype and the instance. I have noticed the additional unknown in chan_skinny, but it doesn't seem to exist in wireshark, and from memory, if wireshark doesn't know how to handle a bit of the data it still reports it as unknown, which it didn't when I had a look at a stimulus_message packet this morning. I suspect that the unknown bit of stimulus_message in chan_skinny may not actually exist at all.

Anyway, I'll have to do a bit more mucking around and see if there is any more data in a stimulus_message other than typeofmessage and instance. Just for reference, the 30VIP only seems to provide an instance if a LINE button is pressed, otherwise the instance is 0. If you want to have a quick play, you could capture your line button presses as I think they are actually hard buttons and so will go through handle_stimulus, and see if any additional data my be included in the stimulus_message.
By: pj (pj) 2007-06-21 10:57:13

Dan, "forever ring" issue probably not related to paralel ringing,
same issue if I'm calling single phone (regardless if 7920 or 7961),
after hangup, asterisk has no channels active, but phone still ringing!
I must completely turn off 7920, or pickup&hangup 7961 to stop ringing...
SIP phone is working fine, so maybe chan_skinny doesn't send some message to phone to stop ringing before channel hangup?


   -- Executing [324@from-bill:2] Dial("IAX2/bill-31", "Skinny/324@PJ2") in new stack
Found device: PJ2
   -- skinny_request(324@PJ2)
skinny_new: tmp->nativeformats=8 fmt=8
   -- skinny_call(Skinny/324@PJ2-64)
Displaying Prompt Status 'Ring-In'
Setting Callinfo to (0724xxxxxx) from Pavel Jezek(324) on PJ2(1)
Setting ringer mode to '2'.
   -- Called 324@PJ2
   -- Skinny/324@PJ2-64 is ringing
Found device: PJ2
ipbx*CLI>

hangup, phone still ringing
ipbx*CLI>
skinny_hangup(Skinny/324@PJ2-64) on 324@PJ2
 == Spawn extension (from-bill, 324, 2) exited non-zero on 'IAX2/bill-31'
   -- Executing [h@from-bill:1] Hangup("IAX2/bill-31", "") in new stack
 == Spawn extension (from-bill, h, 1) exited non-zero on 'IAX2/bill-31'
   -- Hungup 'IAX2/bill-31'
Found device: PJ2
ipbx*CLI>
ipbx*CLI>

no active channels, phone still ringing
ipbx*CLI> core show channels verbose
Channel              Context              Extension        Prio State   Application  Data                      CallerID        Duration Accountcode BridgedTo
0 active channels
0 active calls


By: pj (pj) 2007-06-21 11:27:35

I restarted my asterisk and it works again! and I know way, how to reproduce this issue again, it't probably second call issue. my setup:
- 7961 - calls to 7920
- another call is placed to 7920, call is not answered, calling site hangup (so I have missed call on 7920)
- 7961 hanging up the first call with 7920
- after call is terminated, 7920 immediatelly begin ring, that can't be stopped
- 7920 was restarted
- from now, any other simple call to 7920 is ringing forever, until asterisk is restarted
By: Michiel van Baak (mvanbaak) 2007-06-21 11:53:24

I have the 'ringing forever' as well.
When I dial my asterisk from the outside, my 7960 starts to ring. When I hangup the outside phone the cisco keeps ringing and ringing. After several 'newcall' -> 'endcall' -> 'newcall' softbutton hits asterisk crashes and the ringing stops.

I'll try to grab a backtrace later tonight.
By: dea (dea) 2007-06-21 11:54:51

Good detective work.  Can you post a skinny debug of that transaction?
Using core set verbose 3 should provide enough context to find the
missing hangup.
By: pj (pj) 2007-06-21 11:58:01

I posted my skinny debug console output in message 0065491.
By: dea (dea) 2007-06-21 12:13:13

Actually your description was enough for me to reproduce it.  In
skinny_hangup() I just need to set the ringer mode to off in two
places.

Around line 2613 inside the if (onlysub){

add transmit_ringer_mode(s, SKINNY_RING_OFF); as the last line before
and the first line after } else {

I'll post a fresh patch, but thought you might try this quick edit
before I did that.
By: pj (pj) 2007-06-21 12:19:24

do you think, that it's so simple? but how to explain, that this issue appears (and persist until asterisk restart) only after I place second call to phone?
with only placing single calls to phone it working without problems.
By: dea (dea) 2007-06-21 12:25:53

These phones are pretty smart and yet still very stupid.

The missing two statements tell the phone to stop ringing.  They
are for the cases where the phone is in use and there is one
call or more than one call.

If you use the same test you described, but call the 7920 again
while it is ringing then hangup without answering it will stop.
By: dea (dea) 2007-06-21 17:46:16

V4 fixes the continued ringing that occurs if a phone in a call gets a second
call and does not answer it.

Also found and fixed two more packet structure issues.
By: Damien Wedhorn (wedhorn) 2007-06-21 21:29:06

Woohoo, finished another semester.

Dan, with your latest patch for packet structures you seem to use reference and callReference, are these the same, and if so it would probably be nice to have the same name.

Re the stimulus_message, I've done some more digging and it's not looking good. The CCM dumps I have been given (all appear to be 79xx phones) indicate that the stimulus message contains the stimulus_type and the instance, followed by 4 bytes of 0x00 (although the only stimulus message I can find in the dumps is the line_button).

So, I tried chan_skinny and my 30VIP's and all I get is the stimulus_type and the instance and nothing else. No trailing 4 bytes of 0x00. So even if the the 4 bytes at the end may contain some reference type data, I don't think we can rely on actually receiving it as some devices don't seem to send anything. Interestingly though, the hold button actually appears to be giving the correct instance as opposed to instance 0.

Dan, seeing as you seem to have the packet structures fairly well covered for your devices, could you try modifying the hardbutton template for one of your devices and make one of the line buttons a hold button. That way would should be able to see what happens with the handle_stimulus sub and see if your devices actually send any additional data in the stimulus_message other than the stimulus_type and instance.
By: dea (dea) 2007-06-22 01:16:45

I'll look into that.  The only traceable phone I have is a copy
of Cisco's softphone and it appears all buttons are treated as
softkeys.  This softphone shares code with the 7971, so I think
most of the new phones will be the same.

I'll also look to cleanup the reference/callreference issue.
I prefer callreference, but I will use whichever is more common.


I hope your exam went well...
By: pj (pj) 2007-06-22 01:34:02

chan_skinny-transfer-trunk-v4.txt fails to apply to trunk (r71062)
patching file channels/chan_skinny.c
Hunk ASTERISK-20 FAILED at 2495.
Hunk ASTERISK-21 FAILED at 2504.


By: dea (dea) 2007-06-22 01:47:37

I think those hunks were merged seperately today.  I'll update my
copy of trunk and fix it up, but you may be OK to compile and test.
By: dea (dea) 2007-06-22 11:16:20

Updated to SVN 71142.
By: pj (pj) 2007-06-22 14:26:11

it still not working correctly (chan_skinny-transfer-trunk-v5.txt)
I have established call between two 7920
when I make second call to 7920, I hear 'second call announcing tone' in 7920, but when remote site hangup call to 7920, I hear something like dial tone in 7920 until I hang up first call.

incomming second call (I hear anouncing tone - short beep):
Setting Callinfo to (0724xxxxxx) from Pavel Jezek(324) on PJ3(1)
Setting ringer mode to '2'.
   -- Called 324@PJ3
   -- Skinny/324@PJ-25 is ringing
   -- Skinny/324@PJ3-26 is ringing
Found device: PJ
Found device: PJ3
   -- SIP/324-081e8610 is ringing


remote site hangup second call to 7920 (I hear 'dialtone' in 7920 until hangup call with first party):

Setting ringer mode to '1'.
skinny_hangup(Skinny/324@PJ3-26) on 324@PJ3
Setting ringer mode to '1'.
skinny_hangup(Skinny/324@PJ-25) on 324@PJ


By: Damien Wedhorn (wedhorn) 2007-06-29 21:12:01

Uploaded a new patch I think, sort of depends on what this new licencing thing decides.

Anyway, I had to do significant changes to line and sub handling to get my old phones to allow for multi line operation correctly with the transfer and hold stuff.

Not pretty, and some of the indications may be wrong, but the functionality now works for me. Any subchannel can be hung up from either end and it seems that a rationale result is given. EG if you are in the middle of a transfer and hangup on a sub, it will leave the other sub on hold so you can offhook then hold to get the old sub back. If the original call of a transfer hangs up, you end up with a single outbound call.

Edit: I've no idea about uploading files with the new mantis, all three versions I've attempted to upload are identical, first was before licence "signing", second was after and third was with checking the code or doc box.


By: Damien Wedhorn (wedhorn) 2007-07-06 18:26:44

My licence has been approved and so my patches are now available. Patches v6, v6a and v6b are identical (can an admin please delete 6a and 6b).
By: Michiel van Baak (mvanbaak) 2007-07-07 08:41:01

v7 as the same as v6, only with correct line numbers for the hunks.
v6 did not apply on my OpenBSD box so applied it on a clean current trunk and made a new diff on linux so I can patch my OpenBSD trunk again ;)

Totally no other changes.
By: Michiel van Baak (mvanbaak) 2007-07-08 05:09:10

With this patch applied I get some weird stuff:
When a call ends, it is not removed from the phone's display.
Example: I checked my voicemail and hit # at the end to make asterisk hangup the call. in asterisk CLI the call is indeed terminated but my 7960 still has the call in the display, with the calltime counter running.
A minute later someone calls me so I see a second call on my 7960 but I cant pickup that call. The remote party hangsup and my 7960 now still has the 'active' call to the voicemail and the incoming call on the display.

Had to restart asterisk so the phones reboot and everything is working again for the first call made/received.
By: Jason Parker (jparker) 2007-08-06 17:11:41

63 out of 84 hunks FAILED -- saving rejects to file channels/chan_skinny.c.rej

Yikes.

Anybody feeling brave and want to update this after all of the recent fixes?
By: Damien Wedhorn (wedhorn) 2007-08-06 18:10:35

I think a lot of the failed hunks have probably already been included in trunk. This patch started to get a life of its own and we should probably fix up some other things before implementing transfer. From memory, the fixes included in the last lot of patches were:
- Transfer (of course)
- Whole bunch of packet fix-ups (DEA)
- Hold cleanup (wedhorn)
- Line handling on old devices (wedhorn)

Also I think DEA did fixed up some dialing issue (but may have been a line issue). I would suggest that we get each individual part into trunk before implementing trunk. I'm happy to do an individual "hold cleanup" and then do some work on fixing up the "line handling".

DEA, have all of your packet changes made it into trunk? I think you also fixed up a dialing issue, but from memory it was related to line handling.

Comments, suggestions? If nothing, I'll try to get the hold cleanup posted in the next couple of days.
By: Michiel van Baak (mvanbaak) 2007-08-07 04:30:05

I think it's best to fix all issues that this patch relies on first.
Otherwise it will continue to stay this way.
Good luck wedhorn.
By: dea (dea) 2007-08-07 10:05:44

The packet structure and callstate fixes should be completely merged
at this time.  I found and fixed more than what was in this patch set.

The hold and redial cleanup patches are lingering attached to a closed
bug report and I should get them updated and attached to their own bug.

I have not had a chance to look at this patch recently, so I am not sure
if you were able to maintain my fixes for the subchannel list handling.
The current code in Trunk and Branches/1.4 does not appear to ever free
a sub at the end of a call.  New subs get inserted to the head of the lines
subchannel list, so there is not a huge performance issue, but I think we
are leaking small amounts of memory.  I also think that we will want/need
to properly handle cleanup subchannels if we want to support multiple calls
per line (1 active, others on hold)

I am away from my development system this week, so I cannot  offer much
other than code review.

On the subchannel handling front, I've noticed that Qwell has a branch with
all of the link-lists converted to the AST_* macros.  I think we could make
a big jump forward if that branch could be merged to Trunk.  It's a big change,
but we seem to have a good group of interested developers that can handle any
clean-up required.
By: dea (dea) 2007-11-16 15:42:10.000-0600

Updated to SVN Trunk 89318

While I was at it, I fixed an issue with the transfering phone
having 'ghost' calls that could not be terminated and that would
lead to segfaults.
By: Michiel van Baak (mvanbaak) 2008-02-07 15:14:17.000-0600

what's the status on this one?
Can we get it merged? or does it need some love first ?
By: dea (dea) 2008-02-07 15:51:12.000-0600

I suspect it will need to be updated to apply to trunk.  No one has commented in along time, so either it is working or everyone gave up.

It still works in my small test and development lab, so I hope it is the former.

I can try to update the patch to apply to trunk later today.
By: Michiel van Baak (mvanbaak) 2008-02-07 15:59:16.000-0600

please update it to current trunk
I'll look at it and convince qwell or myself to merge it
By: dea (dea) 2008-02-07 16:25:42.000-0600

That was not so bad.  A bunch of fuzzy an single reject involving an already merged change involving a single ast_verbose() call.

One aspect of the patch that I am not happy with, but which I do not think
should block it, is the open coded linked list handling of the subchannels.

I think those sections should be replaced with the list handling macro or
better yet the astobj2 style.  Sadly both are more than I can bite off at the
moment...

I do not have any of the old model phones that wedhorn has, so I am not sure that they still work, but I suspect that they do.
By: Michiel van Baak (mvanbaak) 2008-02-09 06:00:05.000-0600

Hate to say it, but it's not working at all in my setup and messes up my 7960.

Here is what happens:
ekiga (6005) calls 7960 (6000) -> pickup on 7960 -> talk -> all is fine
on 7960 I hit transfer -> moh on ekiga and new line on 7960
on 7960 I hit 6002 -> 7905(6002) starts ringing -> still all fine
7905(6002) picks up -> nothing happens
7905(6002) hangs up -> nothing happens and 7960 indicates that it's still ringing 6002
ekiga hangs up -> 7960 indicates it's still on call with 6005
hitting whatever button gives me this on the cli:
[2008-02-09 12:57:58] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'

How can I debug this and provide info you need to fix this ?
By: Michiel van Baak (mvanbaak) 2008-02-09 06:04:39.000-0600

Here's the cli output:
   -- Executing [6000@internal:1] Goto("SIP/thinkpad-082c8c48", "sw-13-6000,10") in new stack
   -- Goto (internal,sw-13-6000,10)
   -- Executing [sw-13-6000@internal:10] Dial("SIP/thinkpad-082c8c48", "Skinny/6000") in new stack
   -- skinny_request(6000)
   -- Called 6000
   -- Skinny/6000@office-1 is ringing
   -- Skinny/6000@office-1 answered SIP/thinkpad-082c8c48
   -- Started music on hold, class 'default', on SIP/thinkpad-082c8c48
   -- Starting simple switch on '6000@office'
   -- Executing [6002@internal:1] Goto("Skinny/6000@office-2", "sw-13-6002,10") in new stack
   -- Goto (internal,sw-13-6002,10)
   -- Executing [sw-13-6002@internal:10] Dial("Skinny/6000@office-2", "Skinny/6002") in new stack
   -- skinny_request(6002)
   -- Called 6002
   -- Skinny/6002@livingroom-3 is ringing
   -- Skinny/6002@livingroom-3 answered Skinny/6000@office-2
 == Spawn extension (internal, sw-13-6002, 10) exited non-zero on 'Skinny/6000@office-2'
[2008-02-09 12:57:21] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
[2008-02-09 12:57:22] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
[2008-02-09 12:57:22] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
[2008-02-09 12:57:22] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
[2008-02-09 12:57:22] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
[2008-02-09 12:57:23] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
   -- Stopped music on hold on SIP/thinkpad-082c8c48
 == Spawn extension (internal, sw-13-6000, 10) exited non-zero on 'SIP/thinkpad-082c8c48'
[2008-02-09 12:57:54] WARNING[23331]: chan_skinny.c:1536 find_subchannel_by_instance_reference: Could not find subchannel with reference '2' on 'office'
asterisk*CLI> core show channels
Channel              Location             State   Application(Data)            
0 active channels
0 active calls
2 calls processed

Had to reset my 7960 here to get it back to a working state
By: dea (dea) 2008-02-09 12:00:07.000-0600

If you can turn on skinny debug just before attempting the transfer, it would
be a great help.

The error "Could not find subchannel with reference '2'" means we already
dropped the subchannel related to this call, but forgot to tell the phone,
so the phone still thinks it is active.

Are you attemping a blind or attended transfer?
By: Michiel van Baak (mvanbaak) 2008-02-09 12:09:54.000-0600

I'm trying an attended transfer.
All dishes done, vaccuumed the house, laundry is done so it's time to grab some debug logs for you :)
By: Michiel van Baak (mvanbaak) 2008-02-09 12:29:35.000-0600

uploaded log.

The situation is less worse then first time. There's some other issue in chan_skinny that makes it hard to use, but that's for another issue I'll post soon.
Here is how the attended transfer goes:
thinkpad is extension 6005 which is ekiga softphone
office is extension 6000 which is the first line on my cisco7960
livingroom is extension 6002 which is the only line on my cisco7905

thinkpad calls office
office answers and talks to thinkpad
office hits 'Transfer', moh starts to play on thinkpad and office shows new line with dialtone
office dials '6002' and livingroom starts to ring
livingroom picks up and talks to office
office hits 'Transfer' again, thinkpad gets transferred to livingroom and they can talk.
office shows it is still in a call with thinkpad.

office wont return to normal, I have to reset it by powercycling it
By: dea (dea) 2008-02-09 13:55:36.000-0600

Attended transfers work here, but blind transfers do not.  The code
looks different and is #if 0'd out at the moment.  I'll try to clean that
up now.

I found that most of the debugging logging is busted.  It looks to be related
to the conversion from ast_verbose to ast_verb.  For testing is just did a
search and replace of ast_verbose( with ast_verb(3,

I believe that to be appropriate and returns debug logging to a working state.
I can provide a patch, but it will have to wait until I get the transfer code
fixed up.  OR is some kind individual with commit rights would make this trivial change, that would also be great.
By: dea (dea) 2008-02-09 16:35:32.000-0600

It turns out the at behaviour was too closely modeled on early Cisco
Call Manager.  It required that the process be:

1.  Press Transfer
2.  Dial the destination
3.  Press Transfer again

This worked for both blind and attended transfers, buit is not exactly
intuitive.  I reworked the code to also support transfering using this
model:

1.  Press Transfer
2.  Dial the destination
3.  Hangup (go on hook or press end call

I also fixed up the call state handling during a transfer that would
have otherwise left the phone thinking it had a call when it did not.
and finally I actually enforced the fact that transfers could be disabled.
Earlier versions of the patch did not check to see if the line had transfers
enabled in skinny.conf, now it does.  The default is disabled, so if not
set in the config, transfers will not work.

Tests that pass:
1.  Blind transfer
   a.  Explicit by the second pressing of Transfer
   b.  Implicit by hanging up once the remote leg is ringing
2.  Attended transfer
   a.  Explicit by the second pressing of Transfer
   b.  Implicit by hanging up once the remote leg has answered
3.  Aborting a transfer
   a. The call reverts to the original, but does not take it off hold
      in case you want to select another transfer destination

I also added a warning to be displayed on the phone if a user tries
to use the transfer feature on a line that does not have it enabled, and
I added the Transfer softkey to the offhook-with-features map so that
users can use the method of transfer->dial->transfer for blind transfers.
By: Damien Wedhorn (wedhorn) 2008-02-09 18:01:17.000-0600

I've been flat on my back for the last week so haven't had a chance to follow this. From memory (it was a while ago), there were some issues with the older phones to do with line/sub matching. The patch was getting so big that I thought we would break it down, but that doesn't seem to have progressed beyond posting the first of many patches.

As it looks like we'll have to do a monolithic patch, I'll do some testing on the old phones with the latest transfer patch. However, due to my current state, I probably wont be able to do anything for at least a week.
By: Michiel van Baak (mvanbaak) 2008-02-10 07:15:44.000-0600

DEA:
I'll try your latest patch, thanks

Have a look at http://bugs.digium.com/view.php?id=11967 for an update on the ast_verbose use in debugging.
By: Michiel van Baak (mvanbaak) 2008-02-13 13:36:59.000-0600

Updated patch after my chan_skinny debug patch was merged into trunk.
No functional change, only made it apply again (13 hunks failed).
I found it no more then fair to fix this after I broke it ;)
By: Michiel van Baak (mvanbaak) 2008-06-09 16:11:20

qwell: ok if I take this one from you and finish it ?
By: Michiel van Baak (mvanbaak) 2008-06-11 12:19:05

another update to trunk
By: Michiel van Baak (mvanbaak) 2008-06-11 12:23:09

Is there anyone who can provide test results please ?
It seems to work correctly here, but I only have a 7905 and a 7960.

If this is going to take some time, please leave a comment as well. I'll create a branch because keeping this one up-to-date to current trunk can be a pain from time to time.
By: Damien Wedhorn (wedhorn) 2008-06-11 18:32:27

I'll try to have a look at it over the weekend. I'll be using the old phones and I vaguely remember the last time I checked there was issues with grabbing the right sub in certain areas (the old phones don't include the instance with certain button presses), so expect some patches.

A branch would be greatly appreciated, but how would we provide patches to the branch?

Also, on a more general note, do we want to include 10396, which I think is already partially included (only on a piecemeal basis).
By: dea (dea) 2008-06-11 23:24:38

When I last worked on this patch set, it worked with 7940/7960, IP Communicator and the 7920/7921 phones.

These phones were not in a production environment, but worked fine in my testing.

Wedhorn's observations did appear true for awhile, and I think we ironed
those out.  If not, then the issue will be with the linked-list handling
of the sub-channels.
By: Michiel van Baak (mvanbaak) 2008-06-12 16:34:21

wedhorn: If I create a branch it will be branched from trunk and set to automerge.
That means it's basically trunk with the stuff we do here.
If you want patches to go in you can post them here and I'll commit them.
So you have to run the branch code instead of trunk.

If you can send a new version of the 10396 patch I can add it to the branch as well if you want.
By: Digium Subversion (svnbot) 2008-06-12 16:37:55

Repository: asterisk
Revision: 122426

A   team/mvanbaak/skinny-tranfer/

------------------------------------------------------------------------
r122426 | mvanbaak | 2008-06-12 16:37:54 -0500 (Thu, 12 Jun 2008) | 2 lines

create branch for issue ASTERISK-9642

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

http://svn.digium.com/view/asterisk?view=rev&revision=122426
By: Michiel van Baak (mvanbaak) 2008-06-12 16:47:42

you can now do:
svn co http://svn.digium.com/svn/asterisk/team/mvanbaak/skinny-transfer to get latest trunk with this patch applied.

Please post patches against that branch if you have those.
By: Michiel van Baak (mvanbaak) 2008-06-25 11:39:18

wedhorn: Did you have time to test this branch ?

It's working great here, and I'm tempted to commit things to trunk
By: Digium Subversion (svnbot) 2008-06-25 14:30:23

Repository: asterisk
Revision: 125096

U   trunk/channels/chan_skinny.c

------------------------------------------------------------------------
r125096 | mvanbaak | 2008-06-25 14:30:22 -0500 (Wed, 25 Jun 2008) | 10 lines

implement transfer functionality in chan_skinny

(closes issue ASTERISK-9642)
Reported by: wedhorn
Patches:
     transfer_v6.diff uploaded by wedhorn (license 30)
     chan_skinny-transfer-trunk-v10.txt uploaded by DEA (license 3)
     chan_skinny-transfer-trunk-v12.txt uploaded by mvanbaak (license 7)
Tested by: DEA, wedhorn, mvanbaak

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

http://svn.digium.com/view/asterisk?view=rev&revision=125096
By: Digium Subversion (svnbot) 2008-06-25 14:43:00

Repository: asterisk
Revision: 125098

_U  branches/1.6.0/

------------------------------------------------------------------------
r125098 | mvanbaak | 2008-06-25 14:42:59 -0500 (Wed, 25 Jun 2008) | 17 lines

Blocked revisions 125096 via svnmerge

........
r125096 | mvanbaak | 2008-06-25 21:37:40 +0200 (Wed, 25 Jun 2008) | 10 lines

implement transfer functionality in chan_skinny

(closes issue ASTERISK-9642)
Reported by: wedhorn
Patches:
     transfer_v6.diff uploaded by wedhorn (license 30)
     chan_skinny-transfer-trunk-v10.txt uploaded by DEA (license 3)
     chan_skinny-transfer-trunk-v12.txt uploaded by mvanbaak (license 7)
Tested by: DEA, wedhorn, mvanbaak

........

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

http://svn.digium.com/view/asterisk?view=rev&revision=125098