> 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?
>
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.
- Matt
-----------------------------------------------------------
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
>
>
--
_____________________________________________________________________
-- Bandwidth and Colocation Provided by http://www.api-digital.com --
asterisk-dev mailing list
To UNSUBSCRIBE or update options visit:
http://lists.digium.com/mailman/listinfo/asterisk-dev
