[asterisk-dev] [Code Review] 3430: [channels/chan_unistim.c]: Improvements and bugfixes to chan_unistim.c

[Matt Jordan]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140410/71d9993a/attachment-0001.html>