[asterisk-dev] [Code Review] Generic Advice of Charge

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/552/#review1666
-----------------------------------------------------------


just some minor style comments for now, nice work so far!


/trunk/include/asterisk/aoc.h
<https://reviewboard.asterisk.org/r/552/#comment3729>

    Please add an AST_ prefix to all of the constants in the API



/trunk/include/asterisk/aoc.h
<https://reviewboard.asterisk.org/r/552/#comment3728>

    If you're going to have a magic entry, how about adding one more called AOC_MULT_NUM_ENTRIES or something



/trunk/include/asterisk/aoc.h
<https://reviewboard.asterisk.org/r/552/#comment3730>

    To actually have this parsed by doxygen, you need to start it by ...
    
    /*!
     * \brief
     ...



/trunk/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment3731>

    spaces around the '+'



/trunk/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment3732>

    indent your break;


- Russell


On 2010-03-12 12:20:54, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/552/
> -----------------------------------------------------------
> 
> (Updated 2010-03-12 12:20:54)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This review represents the AOC additions listed below.  The chan_dahdi component will be updated in this review once it is complete.  I thought it would be useful to get some feedback on my Generic AOC design while I work on the other components.
> 
> Current Additions:
> - Generic AOC encode/decode routines. (Generic AOC must be encoded to be passed on the wire in the AST_CONTROL_AOC frame)
> - AST_CONTROL_AOC frame type to represent generic encoded AOC data
> - AOCMessage manager action for generating an AOC message on a channel
> - SIP sending generic AOC-D messages to snom phones via the snom AOC specification. AOC-E can be sent as well, but currently only on an SIP_INFO message just like the AOC-D messages.
> - Creation of chan_sip page3 flags for the addition of the new 'snom_aoc_enabled' sip.conf option.
> - AOC message generating unit test
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 251943 
>   /trunk/channels/sip/include/sip.h 251943 
>   /trunk/configs/sip.conf.sample 251943 
>   /trunk/include/asterisk/aoc.h PRE-CREATION 
>   /trunk/include/asterisk/frame.h 251943 
>   /trunk/main/aoc.c PRE-CREATION 
>   /trunk/main/channel.c 251943 
>   /trunk/main/manager.c 251943 
>   /trunk/tests/test_aoc.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/552/diff
> 
> 
> Testing
> -------
> 
> I tested generating multiple combinations of AOC-E and AOC-D messages using the AOCMessage manager action.  These messages were queued onto a sip channel and forwarded to a snom phone where I could view the AOC results on the display.
> 
> I also developed an AOC unit test which I verified passes.
> 
> 
> Thanks,
> 
> David
> 
>