[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.
> 
> rmudgett wrote:
>     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?
>
> 
> Matt Jordan wrote:
>     Well, yes - but at the same time, that's not Review Board's problem. To quote Linus:
>     
>     {quote}
>     Now, some people will claim that having 8-character indentations makes
>     the code move too far to the right, and makes it hard to read on a
>     80-character terminal screen.  The answer to that is that if you need
>     more than 3 levels of indentation, you're screwed anyway, and should fix
>     your program.
>     {quote}
>     
>     Yes, this means we're screwed in so many places in Asterisk it's not really funny (and no, I'm not advocating for 8 character indentations), but when our indentations with 4-character spacing pushes the code so far over that it can't render properly in Review Board, that's a sign that a change is warranted.

I'm going to commit this patch and the corresponding libpri patch.  In a separate patch I'll extract the PRI_EVENT_RING case into its own function to reduce the indentation and eliminate the duplicated code setting up the incoming call channel.  That will improve the readability and maintainability.


- 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/20140512/f2ccfd0d/attachment-0001.html>