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

[jrose]

> On July 6, 2013, 10:07 p.m., Matt Jordan wrote:
> > /trunk/main/bridging_basic.c, lines 142-150
> > <https://reviewboard.asterisk.org/r/2648/diff/4/?file=41117#file41117line142>
> >
> >     Check for the correct case here before calling set_feature_flag_from_char. Raise the WARNING here that an invalid case was provided.
> >     
> >     It is incredibly odd that "tH" will cause a critical error and cause the dialplan function to return -1, but "ZZZZZZZZZZZZZZZZZZZZZZZZZZH" will not.
> 
> jrose wrote:
>     Why is that odd?  We talked about that exact issue in detail already. 'Z' might be a valid feature code here in a future version of Asterisk, so we observe it and note that it's not currently supported. We might also later enabled a way to set new feature codes to be parsed here through plugins. 't' on the other hand will never be a valid feature code in here because it's lower case and would only apply to the peer being called, and that's not something that this function was meant to support.
>     
>     The addition of more 'Z's doesn't do anything since it just corresponds to a binary flag and I don't think we've ever bothered checking for that kind of redundancy in application argument parsing before.  I haven't actually checked though, so I could be wrong. If you want me to check for redundant feature requests in a dial features string and reject those, I can go ahead and do that.

s/dial features string/dtmf features string/


- jrose


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


On July 8, 2013, 3:59 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2648/
> -----------------------------------------------------------
> 
> (Updated July 8, 2013, 3:59 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 393426 
>   /trunk/include/asterisk/bridging_basic.h 393426 
>   /trunk/main/bridging_basic.c 393426 
> 
> 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/20130708/4536a568/attachment.htm>