[asterisk-dev] [Code Review] 3521: chan_dahdi/sig_pri: Prevent unnecessary PROGRESS events when overlap dialing is enabled.

[rmudgett]

> On May 7, 2014, 10:49 a.m., Matt Jordan wrote:
> > /branches/1.8/channels/sig_pri.c, lines 5275-5290
> > <https://reviewboard.asterisk.org/r/3521/diff/1/?file=58211#file58211line5275>
> >
> >     This is really hard to read, particularly with the level of indentation that is required here.
> >     
> >     Is there is any way the detection of match_more / need_dialtone can be put into small functions, that would be... nice.
> >     
> >     match_more = can_pri_match_more(e, pri);
> >     need_dialtone = do_i_need_dialtone(match_more, e, pri);
> >     
> >     Something like that.

They could be put into encapsulating functions but I'm not sure there is much benefit over just setting the boolean variables as done here.  Admittedly, pri_dchannel() has long been in need of refactoring into smaller functions.

Is it the starting indentation level causing reviewboard to wrap the lines awkwardly that is the main problem?


- rmudgett


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3521/#review11837
-----------------------------------------------------------


On May 1, 2014, 9:35 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3521/
> -----------------------------------------------------------
> 
> (Updated May 1, 2014, 9:35 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: AST-1338
>     https://issues.asterisk.org/jira/browse/AST-1338
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This review and https://reviewboard.asterisk.org/r/3520/ work together to allow Asterisk to control the inband audio available progress indication ie on the SETUP_ACKNOWLEDGE message when dialtone is present.
> 
> When overlap dialing is enabled, the lack of inband audio available information in the SETUP_ACKNOWLEDGE events causes an interoperability problem with SIP.  sig_pri doesn't know if there is dialtone present when a SETUP_ACKNOWLEDGE is received so it assumes it is there and posts an AST_CONTROL_PROGRESS frame.  The SIP channel driver then sends out a 183 Session Progress and blocks the desired 180 Ringing message when the ALERTING message comes in.
> 
> * Made the configure script detect if the installed version of libpri supports the SETUP_ACKNOWLEDGE enhancements.
> 
> * Using the new API, made generate an AST_CONTROL_PROGRESS frame on an incoming SETUP_ACKNOWLEDGE message when the message indicates inband audio is present insetad of assuming that dialtone is present.
> 
> * Using the new API, made SETUP_ACKNOWLEDGE send out an inband audio available indication only if dialtone is expected.  The change also makes the fallback behaviour of sending the PROGRESS message better by sending it only if dialtone is expected.
> 
> * Changed receiving a PROCEEDING message to not generate an AST_CONTROL_PROGRESS frame if the progress indication ie indicates non-end-to-end-ISDN.  This helps interoperability with SIP.
> 
> * Changed sending a PROCEEDING message in response to an AST_CONTROL_PROCEEDING frame to not indicate inband audio available.  It was silly to do so anyway because the channel driver doesn't know if inband audio is even available.  This helps interoperability with SIP.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/include/asterisk/autoconfig.h.in 413194 
>   /branches/1.8/configure.ac 413194 
>   /branches/1.8/configure UNKNOWN 
>   /branches/1.8/channels/sig_pri.c 413194 
> 
> Diff: https://reviewboard.asterisk.org/r/3521/diff/
> 
> 
> Testing
> -------
> 
> Sent calls over an ISDN trunk with overlap dialing enabled that did and did not present dialtone for more digits.
> Observed that the inband audio available progress indication ie was sent when needed on the SETUP_ACKNOWLEDGE message.
> Added a debug message that indicated when Asterisk did and did not receive the inband audio indication for the SETUP_ACKNOWLEDGE message.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140507/9cd35f41/attachment.html>