----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3430/#review11541 -----------------------------------------------------------
The vast majority of these findings are simply Coding Guideline violations. Please review the Coding Guidelines on the Asterisk wiki: https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines In the future, I highly recommend checking your code against those guidelines before posting a review. /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21294> There should be spaces between each item: { 0x4a, 0x61, 0x6e, ...} /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21293> Remove the extraneous spaces here. /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21295> In general, don't leave commented code in patches. Either the code is good enough to go in, or it should be removed. If a comment is needed, leave a comment. /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21296> Extraneous white space /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21297> Please review the coding guidelines for formatting of code. This: send_mute (pte,(mute&0x01)); Should be written: send_mute(pte, (mute & 0x01)); /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21298> Spacing /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21299> Extraneous white space /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21300> White space /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21301> Remove commented code /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21302> Keep the XXX. Certain editors know to flag that. /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21303> Spacing: d->dtmfduration = 0 /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21304> Spacing /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21305> The CHANGES file should be updated to reflect the new setting for chan_unistim. /trunk/channels/chan_unistim.c <https://reviewboard.asterisk.org/r/3430/#comment21306> Do not put condition expressions on one line: if (d->dtmfduration > 150) { d->dtmfduration = 150; } - Matt Jordan On April 9, 2014, 10:25 a.m., Peter Whisker wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3430/ > ----------------------------------------------------------- > > (Updated April 9, 2014, 10:25 a.m.) > > > Review request for Asterisk Developers. > > > Repository: Asterisk > > > Description > ------- > > I have been actively testing Unistim devices (i2002 & i2004) and have > encountered issues with > 1) dialtone wrong > 2) inability to select DTMF playback duration > 3) inability to set date to match than of a Nortel CS1000 > 4) mute causing playback to be muted as well as microphone > 5) Timer displayed in French (Duree) > > The attached patch fixes as follows: > 1) The "modulation" should not be referenced for tone+tone as it refers to > the on-off characteristic I believe - this often resulted in a single tone > rather than the multitone as in the UK. > 2) I have added the unistim.conf variable dtmf_duration which can select the > DTMF playback duration from 0ms to 150ms (0 is off and is the new default) > 3) I have enabled the transmission of MonthLabels (in English) which are sent > with the date and changed the dateformat variable to accept the values 0-3 as > per the UNISTIM standard (2 & 3 match the previous 1 & 2 formats). > 4) I have enabled the "Mute" packet and merged it into the previous code. It > now only mutes the microphone. (Improvements welcome) > 5) Changed Duree to Timer on i2004 display > > > Diffs > ----- > > /trunk/configs/unistim.conf.sample 411700 > /trunk/channels/chan_unistim.c 411700 > > Diff: https://reviewboard.asterisk.org/r/3430/diff/ > > > Testing > ------- > > In use here > > > Thanks, > > Peter Whisker > >
-- _____________________________________________________________________ -- 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
