> 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 > >
-- _____________________________________________________________________ -- 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
