[asterisk-dev] [Code Review] 2648: func_channel: set dtmf_features

[Matt Jordan]

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



/trunk/funcs/func_channel.c
<https://reviewboard.asterisk.org/r/2648/#comment17768>

    Why wouldn't this be R/W?
    
    It may be useful to know what features have been applied to a channel.



/trunk/funcs/func_channel.c
<https://reviewboard.asterisk.org/r/2648/#comment17769>

    Didn't we agree that these should be uppercase, since they are applied to the channel calling the function and not the peer?



/trunk/include/asterisk/bridging_basic.h
<https://reviewboard.asterisk.org/r/2648/#comment17767>

    Two problems here:
    
    First, the implementation of this function only returns -1.
    
    Second, functions that have multiple error values should typically return an enum. This doesn't really justify that however.
    
    Since the only reason to return multiple values is to inform callers why it failed so that they can log different ERROR messages, just have this log the ERROR message.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17770>

    Upper case



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17771>

    Don't check for upper case and then lower case the option.
    
    Instead, just pass the character to set_feature_flag_from_char and look for an upper case character. If it isn't upper case, return -1 and fail gracefully with an invalid option.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2648/#comment17772>

    Sentences should begin with a capital letter.
    
    This should also be a WARNING (invalid configurations are typically warnings)


- Matt Jordan


On July 1, 2013, 2:41 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2648/
> -----------------------------------------------------------
> 
> (Updated July 1, 2013, 2:41 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21876
>     https://issues.asterisk.org/jira/browse/ASTERISK-21876
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Allows setting dtmf features via a channel set function (namely CHANNEL(dtmf_features)
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_channel.c 393154 
>   /trunk/include/asterisk/bridging_basic.h 393154 
>   /trunk/main/bridging_basic.c 393154 
> 
> Diff: https://reviewboard.asterisk.org/r/2648/diff/
> 
> 
> Testing
> -------
> 
> Stuck two channels in the following extension:
> 
> exten => 10,1,NoOp
> exten => 10,n,Answer()
> exten => 10,n,Set(CHANNEL(dtmf_features)=wktx)
> exten => 10,n,Wait(50)
> 
> Then bridged them together with the manager bridge command and checked to see if both channels could park, transfer, and attended transfer.
> 
> It's worth noting that the dial application overrides this datastore entirely, so these don't get applied to dial as-is. This might need to be addressed.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130702/37a880b3/attachment-0001.htm>