[Home]

Summary:ASTERISK-07632: [patch] make inbound channel 'Ringing' state consistent with other channels
Reporter:cosmo (cosmo)Labels:
Date Opened:2006-08-30 07:07:52Date Closed:2006-12-09 21:22:33.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_zap
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) chan_zap_ringing.diff
( 1) chan_zap_ringing-1.2.11.diff
( 2) chan_zap-ringing.diff
( 3) chan_zap-ringing-1.2.11.diff
Description:To reproduce:
- Dial from or to a Zapata channel
- Before answering the call, do 'show channels' from the CLI
-> In both cases the Zapata channel state is 'Ringing'

According to channel.h, a channel should be in the AST_STATE_RINGING state when the remote end is ringing. However, chan_zap uses this state also in the event of an incoming call when the local line is ringing.
Comments:By: Serge Vecher (serge-v) 2006-08-30 10:24:21

well, if the local line is ringing, it's technically "ringing" or am I misunderstanding something ... What do you want it to say?
By: cosmo (cosmo) 2006-08-30 10:40:41

With SIP channels, for example, the channel is "ringing" when the call is outgoing and "ring" when the call is incoming, which is also consistent with what channel.h says:

/*! Line is ringing */
#define AST_STATE_RING 4
/*! Remote end is ringing */
#define AST_STATE_RINGING 5

I have a GUI frontend which uses the state field to display the status of all the PBX extensions. This issue prevents the user from telling whether this extension is ringing as a result of an incoming call or waiting for an outgoing call to be answered.
By: Stefano Brandimarte (stevens) 2006-08-30 18:24:52

Hello,

i've some question, regarding this matter: for what kind of zap channels have
You noticed this behaviour? What signalling etc.?

Thanks.
By: Stefano Brandimarte (stevens) 2006-08-30 19:47:11

the first patch uploaded is against SVN trunk 41258M.
By: Stefano Brandimarte (stevens) 2006-08-30 19:50:53

second patch uploaded, for asterisk 1.2.11.

I've tested the changes involved on PRI and FXO/FXS zap channels.

The behaviour now should be consistent with other channel drivers (SIP, IAX2).

Feedback welcome.
By: Stefano Brandimarte (stevens) 2006-08-30 22:09:52

cosmo: i don't have chan_zap.c for 1.2.4, but it's possible to make the change
to Your chan_zap.c, by me or You can try Yourself to apply this little fix. In
the first case, send Your chan_zap.c to stevens at stevens dot it.

Disclaimer already sent to Digium.
By: cosmo (cosmo) 2006-08-31 03:27:32

Thanks stevens. Your patch makes sense. Have you considered all the places in chan_zap that check for state==AST_STATE_RINGING and would be affected by the fact that state=AST_STATE_RING for incoming calls?
By: Stefano Brandimarte (stevens) 2006-08-31 05:18:36

Hello, i've checked all the ast_setstate() calls into chan_zap.c. It seems that
the assumption that state == AST_STATE_UP when dialing out from a Zap channel
isn't true, so it's needed to add also the check for AST_STATE_RING there.

For every other call to ast_setstate() i, at least as for now, haven't found any
erroneous assumption or the like. So this bug could be closed with this fix. But
i would glad to receive feedback when You're ready to test this patch. We're now
using the modified chan_zap.so on more than one PBX and they work properly.

Best regards.
By: Stefano Brandimarte (stevens) 2006-08-31 20:55:31

cosmo: i've read again Your comment, so i hope now to understand better what You
say. My answer is: i've checked (and re-checked just now) every place where the
code expect the state == AST_STATE_RINGING to do some job. I feel that we could
try to use the patch (we're actually already using it) and in the meantime i've
the time to walk along every possible condition to test if the channel works as
expected, a part of the cosmetic "Ring" or "Ringing" status. From the places to
to have a look we can at least exclude where the sigalling is one of SIG_FXSKS,
SIG_FXSLS and SIG_FXSGS.

If You've some feedback, please, let me know. Thanks for Your productive comment.
By: Stefano Brandimarte (stevens) 2006-09-02 11:16:25

after a full review of chan_zap.c code and related functions (from channel.c) in
relation to the channel state change, i have to say the patch is effective and the
only side effect it could cause is to address some other strange behaviour of the
zap channels.

cosmo: try it, if you want. I'll continue to work on it if there're feedbacks. If
not, imho, this bug could be considered fixed->closed, at least for me. :-)
By: cosmo (cosmo) 2006-09-03 10:22:55

stevens: I tested various scenarios with your patch and most of them seem to be working fine, except for one special case of blind transfer:
1. Call from FXS1 to FXS2, don't pick up FXS2, let it ring.
2. Press flash on FXS1, wait for a secondary dial tone and call FXS3.
3. Answer call at FXS3, FXS1 and FXS3 are now in a call, while FXS2 still rings.
4. Put FXS1 on hook (this initiates a blind transfer from FXS2 to FXS3). Now FXS3 is supposed to hear a ringback tone until FXS2 picks up, but there is no tone.

It seems to be caused by the condition checking for ==AST_STATE_RINGING in attempt_transfer(). I think it should be changed now to AST_STATE_RING.

I'm also concerned about similar comparisons in zt_fixup() and in zt_handle_event() (right before it prints "Enabling ringtone on real and threeway\n"). The problem is that I don't know how to create scenarios that test these parts of code.
By: Stefano Brandimarte (stevens) 2006-09-03 21:06:01

cosmo: thanks for reporting. Yes i agree with Your analysis and regarding the
places where AST_STATE_RINGING is checked; there're others too (e.g. in the
*__zt_exception() function, in the Polarity Switch event; i cannot check this
one with a real test).

zt_fixup is called by ast_do_masquerade (channel.c) and it's involved in the
three way calls and call transfer process creation. There we could also change
AST_STATE_RINGING or simply put a check for both states.

On the other way, attempt_transfer() checks for AST_STATE_RINGING just to start
a zt_indicate() (ast_indicate() refer to that function) where there're the
changed lines and where the channel state will be set to AST_STATE_RINGING.
Maybe just changing the zt_handle_event to check for AST_STATE_RING (or both
states) the specific issue should be solved.

Anyway, we can conduct some more tests on three way calls and call pickups (if
You've already done these tests let me know the results), other than on the one
that You reported as failed, to find a proper solution.
By: Stefano Brandimarte (stevens) 2006-09-05 18:06:58

cosmo: i've updated and uploaded a new patch for 1.2.11 that implements some of
Your suggestions. After making a lot of testing (putting a debug messages into
the code itself) i've choosen to respect the rule that ringing indication should
be sent to channels in RINGING state. A channel in RING state doesn't guarantee
the other is still Ringing. I hope Your ringback problem could be solved so, but
let me ask if the same problem happens even without the slight modifications the
patch introduces. This is because when, in Your example, You put FXS1 on hook to
let FXS3 call FXS2 the FXS3 channel is already into the UP state. I tested some
call transfer, when one zap channel receive a call, then transfer it to another
phone. Let me know Your results. I've code (we're already trying it) to add to
attemp_transfer() something like:

     if (p->subs[SUB_THREEWAY].owner->_state == AST_STATE_RING) {
         tone_zone_play_tone(p->subs[SUB_THREEWAY].zfd, ZT_TONE_RINGTONE);
     }

and

     if (p->subs[SUB_REAL].owner->_state == AST_STATE_RING) {
         tone_zone_play_tone(p->subs[SUB_REAL].zfd, ZT_TONE_RINGTONE);
     }

Try it, if You still miss the ringback on "blind" transfer attempts. Check also
the classical blind transfer, e.g.:

- FXS1 receives a call
- FXS1 puts the call on hold (hook-flash)
- FXS1 calls FXS2
- FXS1 put the phone on hook before FXS2 answers

The calling phone, either an external line or another phone (SIP/IAX2/Zap/...)
should hear the ringback tone. In my case this isn't true, with or without the
patches.
By: Stefano Brandimarte (stevens) 2006-09-05 18:17:39

patch uploaded for SVN 41826M.
By: jmls (jmls) 2006-11-01 10:25:33.000-0600

Can we confirm that the disclaimer has been received ?
Any reason why this patch can't be applied ?
Is the patch valid for 1.4 and trunk as well ?
By: Stefano Brandimarte (stevens) 2006-11-02 03:10:45.000-0600

jmls: yes the disclaimer is already on file. As for the patches, i've not
updated those posted for 1.4 nor for the latest svn trunk (post 1.4 release).
I'll do shortly, but want to know if the approach is correct or not. The patch
is working for us (since the first work) and we're not noticed wrong behaviours
on zap channels (either PRI or analog ones).
By: Stefano Brandimarte (stevens) 2006-11-22 21:08:53.000-0600

checked asterisk-1.4.0-beta3 and SVN-47950 : both still don't correct the wrong
behaviour reported. The patch is actually a couple of line changes. Perhaps it
could be implemented easily without diffs or using the diffs already uploaded.
By: Steve Murphy (murf) 2006-12-07 12:23:37.000-0600

Looks like the work here is mostly if not completely done. I'll take it on and move it to closing.
By: Steve Murphy (murf) 2006-12-09 21:16:15.000-0600

Many thanks to stevens for his patches!

On 1.2, this patch applied in r. 48371
On 1.4,                       r. 48372
on trunk,                     r. 48373
By: Steve Murphy (murf) 2006-12-09 21:22:32.000-0600

If the problems found & fixed, the patches merged into 1.2, 1.4, and trunk, seems like it's time to close the bug!