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

Tue Apr 20 18:59:42 CDT 2010

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


/team/rmudgett/aoc_event/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/552/#comment4032>

    Is the fall through intentional?


/team/rmudgett/aoc_event/channels/sig_pri.h
<https://reviewboard.asterisk.org/r/552/#comment4034>

    This can be a bit field and placed with the other bit fields in this conditional.


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4038>

    use struct assignment instead of memcpy(): new_chan->aoc_e = old_chan->aoc_e;


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4039>

    You do not need to do this.  Clearing the valid flag is sufficient.


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4040>

    You do not need to do this.  Clearing the holding_aoce flag is sufficient.


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4042>

    You do not need to do this.  Clearing the valid flag is sufficient.


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4041>

    I don't think doing the hangup delay for this event is necessary.  The DISCONNECT message comes in as PRI_EVENT_HANGUP_REQ.


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4043>

    You do not need to do this.  Clearing the valid flag is sufficient.


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4045>

    Change:
    still present on hangup
    to:
    still present on answer


/team/rmudgett/aoc_event/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/552/#comment4044>

    You do not need to do this.  Clearing the valid flag is sufficient.


/team/rmudgett/aoc_event/doc/advice_of_charge.txt
<https://reviewboard.asterisk.org/r/552/#comment4035>

    Red spots in file.


/team/rmudgett/aoc_event/doc/advice_of_charge.txt
<https://reviewboard.asterisk.org/r/552/#comment4036>

    Change:
    Since in Asterisk
    to:
    Since Asterisk


/team/rmudgett/aoc_event/doc/advice_of_charge.txt
<https://reviewboard.asterisk.org/r/552/#comment4037>

    Change CallTransfer to Normal.
    CallTransfer is unlikely to be possible since that is expected to come in without an active call.


/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment4055>

    Demote this warning message to a debug message.


/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment4054>

    Typo:
    expected


/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment4053>

    A struct assignment is better here.
    decoded->aoc_s_entries[] = *entry;


/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment4052>

    This is a change from the AOC events branch.  The AOC event branch just uses "Reverse".  To be consistent it should just be "Reverse" or "ReverseCharge"


/team/rmudgett/aoc_event/main/aoc.c
<https://reviewboard.asterisk.org/r/552/#comment4051>

    Please avoid using new and other C++ keywords for variable names.


/team/rmudgett/aoc_event/main/manager.c
<https://reviewboard.asterisk.org/r/552/#comment4046>

    memset has already set this value.  Inverting the sense of the if test would be better.


/team/rmudgett/aoc_event/main/manager.c
<https://reviewboard.asterisk.org/r/552/#comment4047>

    memset has already set this value.  Inverting the sense of the if test would be better.


/team/rmudgett/aoc_event/main/manager.c
<https://reviewboard.asterisk.org/r/552/#comment4048>

    goto aocmessage_cleanup after here?


/team/rmudgett/aoc_event/main/manager.c
<https://reviewboard.asterisk.org/r/552/#comment4049>

    goto aocmessage_cleanup after here?


/team/rmudgett/aoc_event/main/manager.c
<https://reviewboard.asterisk.org/r/552/#comment4050>

    goto aocmessage_cleanup after here?


- rmudgett


On 2010-04-20 15:34:37, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/552/
> -----------------------------------------------------------
> 
> (Updated 2010-04-20 15:34:37)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Asterisk Generic AOC Representation
> - 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
> - Manager events for AOC-S, AOC-D, and AOC-E messages
> 
> Asterisk App Support
> - app_dial AOC-S pass-through support on call setup
> - app_queue AOC-S pass-through support on call setup
> 
> AOC Unit Tests
> - AOC Unit Tests for encode/decode routines
> - AOC Unit Test for manager event representation.
> 
> SIP AOC Support
> - Pass-through of generic AOC-D and AOC-E messages to snom phones via the snom AOC specification.
> - Creation of chan_sip page3 flags for the addition of the new 'snom_aoc_enabled' sip.conf option.
> 
> IAX AOC Support
> - Natively supports AOC pass-through through the use of the new AST_CONTROL_AOC frame type
> 
> DAHDI AOC Support
> - ETSI PRI full AOC Pass-through support
> - 'aoc_enable' chan_dahdi.conf option for independently enabling pass-through of AOC-S, AOC-D, AOC-E.
> - 'aoce_delayhangup' option for retrieving AOC-E on disconnect.
> - DAHDI A() dial string option for requesting AOC services.
>   example usage: exten=>1111,1,Dial(DAHDI/g1/1112/A(s,d,e)) ;requests AOC-S, AOC-D, and AOC-E on call setup
> 
> 
> Diffs
> -----
> 
>   /team/rmudgett/aoc_event/channels/sip/include/sip.h 258188 
>   /team/rmudgett/aoc_event/configs/chan_dahdi.conf.sample 258188 
>   /team/rmudgett/aoc_event/configs/sip.conf.sample 258188 
>   /team/rmudgett/aoc_event/doc/advice_of_charge.txt PRE-CREATION 
>   /team/rmudgett/aoc_event/include/asterisk/aoc.h PRE-CREATION 
>   /team/rmudgett/aoc_event/include/asterisk/frame.h 258188 
>   /team/rmudgett/aoc_event/main/aoc.c PRE-CREATION 
>   /team/rmudgett/aoc_event/apps/app_dial.c 258188 
>   /team/rmudgett/aoc_event/apps/app_queue.c 258188 
>   /team/rmudgett/aoc_event/channels/chan_dahdi.c 258188 
>   /team/rmudgett/aoc_event/channels/chan_sip.c 258188 
>   /team/rmudgett/aoc_event/channels/sig_pri.h 258188 
>   /team/rmudgett/aoc_event/channels/sig_pri.c 258188 
>   /team/rmudgett/aoc_event/main/asterisk.c 258188 
>   /team/rmudgett/aoc_event/main/channel.c 258188 
>   /team/rmudgett/aoc_event/main/features.c 258188 
>   /team/rmudgett/aoc_event/main/manager.c 258188 
>   /team/rmudgett/aoc_event/tests/test_aoc.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/552/diff
> 
> 
> Testing
> -------
> 
> I have tested every feature listed in the description to some extent, but only back to back with Asterisk.   I am going to need some community help to test interoperability.
> 
> To test this code, check out both my Asterisk and Libpri changes.
> 
> Asterisk Changes: svn/asterisk/team/dvossel/generic_aoc
> LibPRI Changes: svn/libpri/team/dvossel/aoc_send 
> 
> 
> Thanks,
> 
> David
> 
>