Summary: | ASTERISK-07632: [patch] make inbound channel 'Ringing' state consistent with other channels | ||
Reporter: | cosmo (cosmo) | Labels: | |
Date Opened: | 2006-08-30 07:07:52 | Date Closed: | 2006-12-09 21:22:33.000-0600 |
Priority: | Minor | Regression? | No |
Status: | Closed/Complete | Components: | 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! |