<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/2048/">https://reviewboard.asterisk.org/r/2048/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 18th, 2012, 1:41 p.m., <b>Mark Michelson</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;">The part of this code that I have a lot of trouble with is the focus in pbx.c on finding ringing channels and the oldest ringing channel. I don't feel that the oldest ringing channel is something that is generally useful in extension state callbacks. The only time this information is useful is when reporting state changes in chan_sip with regards to call pickup.
I think that the state callback should have the necessary information such that consumers of the callback could determine the oldest ringing channel if they want. Calculating the oldest ringing channel on every state change is not practical.</pre>
</blockquote>
<p>On July 19th, 2012, 6:30 a.m., <b>Guenther Kelleter</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;">But if determination of oldest ringing channel was moved to chan_sip, every SIP callback would have to determine it for itself walking the channel list over and over again when there are multiple SIP subscriptions monitoring the same hint, wasting time as well. This doesn't look better to me.
Is it acceptable to determine the oldest ringing channel only when the state contains AST_EXTENSION_RINGING, instead of moving it to chan_sip? But then collecting other than ringing channels would be useless and should be left out.
Or even better only determine it right before a first extended callback is encountered?
I think we have only these alternatives.</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;">I think you're on the right track with your second idea.
One possible way to minimize unnecessary computations is to keep extended callbacks in one container and standard callbacks in a different one. If the extended callbacks container has any elements in it, then you know that you will need to calculate the oldest ringing channel and to allocate device_state_info structures. If the container is empty, then you can skip all of that processing.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 18th, 2012, 1:41 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2048/diff/1/?file=30290#file30290line5065" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/pbx.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int handle_statechange(void *datap)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5065</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">((</span><span class="n">same_state</span> <span class="o">=</span> <span class="n">state</span> <span class="o">==</span> <span class="n">hint</span><span class="o">-></span><span class="n">laststate</span><span class="p">)</span> <span class="o">&&</span> <span class="p">(</span><span class="o">~</span><span class="n">state</span> <span class="o">&</span> <span class="n">AST_EXTENSION_RINGING</span><span class="p">))</span> <span class="p">{</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5066</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_cleanup</span><span class="p">(</span><span class="n">device_state_info</span><span class="p">);</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">4907</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">continue</span><span class="p">;</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5067</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">continue</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">4908</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">5068</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p">}</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This change is unacceptable. With this change, you only will ever report extension state changes if the state has changed to AST_EXTENSION_RINGING.</pre>
</blockquote>
<p>On July 19th, 2012, 6:24 a.m., <b>Guenther Kelleter</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;">No, this logic is correct, although one has to look at it twice.
The change will be ignored immediately if it stayed the same (as in the original code) and does not include AST_EXTENSION_RINGING (new) -> Change may be reported if it changed or if it contains AST_EXTENSION_RINGING.
Later on only the extended callbacks will be called when the aggregated state didn't change.
I checked that the BLF lamp of a scubscribed extension is really lit up when the extension changes to INUSE.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Okay, I think the problem for me here was that I misread the "&&" as "||" in my mind.</pre>
<br />
<p>- Mark</p>
<br />
<p>On July 16th, 2012, 10:14 a.m., Guenther Kelleter 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, Mark Michelson and Matt Jordan.</div>
<div>By Guenther Kelleter.</div>
<p style="color: grey;"><i>Updated July 16, 2012, 10:14 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;">(For Digium: this is the implementation of the feature described in chapter 2 of the document SIP_NOTIFY_dialog_event_infos_DATUS_v1_4.odt)
This patch extends the extension state callbacks so that monitoring channels (as chan_sip) get more information of the devices which are responsible for an extension state change. The additional information is needed by chan_sip to present names/numbers of the caller and callee in an early-state SIP notification. Users of extenstion state callback not interested in the additional information are not affected by the changes.
Motivation: to present the involved party's name/number in an early-state nofification (used by the notified device as a pickup offer) one after another so that a user can see which call he will pick up in an undirected pickup.
Such a pickup offer to a user shall indicate the same call (number/name-A calls number/name-B) as the call which would be picked up when an undirected pickup is executed.
Users interested in additional state info must use the new functions ast_extension_state_add_extended() resp. ast_extension_state_add_destroy_extended() to register an extended state callback. When the callback is registered this way, an extra member device_state_info of struct ast_state_cb_info is passed to the callback in addition to the aggregated extension state. This container holds an object for every device of the monitored extension hint consisting of the device name, the device state and a channel reference to the channel which (presumably) caused the device state.
The information is used by chan_sip for early-state notifications. When the state of a device changes and the new state contains AST_EVENT_RINGING, an early-state notification is sent to the subscribed devices with the caller/callee names/numbers of the oldest ringing channel of the monitored extension.
The notified user may then invoke a direct pickup, which will pickup exactly this channel.
Users of the old non-extended callbacks will only be called when the aggregated state did change (same behavior as before). Users of the extended callback will also be called when the state is unchanged but does contain AST_EVENT_RINGING. That could be the case if two channels are ringing at one device and one of them hangs up, so the aggregated state does not change. This way the monitoring channel can create a new early-state notification with the now ringing party-ids.
This patch requires the channel creation time member of ast_channel and the changes for the direct pickup to pick up the oldest ringing channel, which are both introduced in review https://reviewboard.asterisk.org/r/2043/.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Checked the name/number of caller/callee displayed on the notified phone.
Checked that a second call at an already ringing device does not invoke a notification as long as the first call is ringing, instead it is sent when the first ringing channel is picked up by some other party or hung up.
Checked that the picked up caller is the same as advertized in the early-state notification when more than one calls are ringing on the subscribed extension.
Ref counting for the containers and channels checked.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/channels/chan_sip.c <span style="color: grey">(370071)</span></li>
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(370071)</span></li>
<li>/trunk/include/asterisk/pbx.h <span style="color: grey">(370071)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(370071)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2048/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>