<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1778/">https://reviewboard.asterisk.org/r/1778/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 10th, 2012, 6:14 a.m., <b>Olle E Johansson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A few thoughts:
* CONTROL_SOMETHING is a source code name and I don't like using that in userspace. There are a few events during the call and we should define them as names, instead of using the event names.
* Enabling all the control frames is not even remotely needed and can possibly be an issue. Does this function receive it before or after or instead of the normal consumer? What state are we in. If you trigger on DTMF and the dial plan listens for DTMF (like in an IVR) then what happens?
* I suggest you add a channel variable to specify which event that triggered the execution
This needs a thorough architecture discussion.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hi Olle. Thank you for your review. Here are my comments.
* CONTROL_SOMETHING is a source code name...: I don't have any preference on this. We used CONTROL_SOMETHING because these symbols are already defined in the code. What I think is that adding other definitions could generate maintenance issues and/or mismatch in the future. But this is just my opinion.
* Enabling all the control frames is not even remotely needed and can possibly be an issue...: The idea is that the list of control frames types will raise a trigger should be specified in the dialplan when calling the function. I couldn't see problems when listening for DTMF in a IVR channel but obviously is up to the caller to define what the gosub function will do. If, for example, the gosub action for a DTMF event is something like "hangup" this should be a problem (probably).
* I suggest you add a channel variable to specify which event that triggered the execution: we decided to have a different behavior. Instead of defining a channel variable to specify which event triggered the execution we preferred to call a specifig gosub extension where the event name is embedded in the extension itself. I mean, triggered by, for example, a CONTROL_HOLD event, the execution will gosub on the ext-CONTROL_HOLD@context. This allow the user to differentiate the code for each different event.
Thanks!</pre>
<br />
<p>- Marco</p>
<br />
<p>On April 10th, 2012, 5:47 a.m., Marco Signorini wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Marco Signorini.</div>
<p style="color: grey;"><i>Updated April 10, 2012, 5:47 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">A filter can be applied to the trigger to limit frames sensibility. This filter can either be a white or black list of control frame types. When no filter type is present, white is used. If no arguments are provided at all, all frames will trigger the specified gosub call.
If the specified gosub extension is not present on the dialplan, the function will gosub to the "s-XXXX" extension in the gosub context where XXXX is the type of the control frame.
It can be set only once on a given channel.
Examples:
exten => 1,1,Set(FRAME_TRIGGER(white, CONTROL_HOLD)=ext,context); trigger gosub ext-CONTROL_HOLD@context only for HOLD type control frames
exten => 1,1,Set(FRAME_TRIGGER(black, CONTROL_HOLD)=ext,context); trigger gosub ext-CONTROL_XXXX@context for all control type frames except for HOLD type control frames
exten => 1,1,Set(FRAME_TRIGGER(CONTROL_HOLD, CONTROL_UNHOLD)=context) ; trigger gosub to s-CONTROL_HOLD@context and s-CONTROL_UNHOLD context only for HOLD and UNHOLD type control frames
This feature was originally developed in order to track MOH events in a call center environment but has general interest.
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19319">ASTERISK-19319</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/funcs/func_frame_trigger.c <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1778/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>