<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/2393/">https://reviewboard.asterisk.org/r/2393/</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 4th, 2013, 8:44 p.m. UTC, <b>rmudgett</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;">Since this is a chan_skinny specific feature, it should not affect other channel drivers.
I think this feature could work this way:
1) Device requests feature.
2) Device supplies exten to attempt pickup.
3) Driver creates a channel.
4) Driver sets a channel variable with the requested exten to pickup. Say SKINNY_PICKUP_EXTEN?
5) Driver starts dialplan at a location configured by the skinny.conf file for this feature.
6) Dialplan uses Pickup(${SKINNY_PICKUP_EXTEN}@context) to fetch the call.
7) If the pickup attempt was not successful, Pickup continues in the dialplan so the user could do something else like play a file.
At the very least, I don't think special directed pickup group options are needed. The existing pickup groups and named pickup groups are sufficient to restrict the available extensions to select the requested exten.
</pre>
</blockquote>
<p>On April 5th, 2013, 2:38 a.m. UTC, <b>wedhorn</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;">Broadly, the idea was not to be skinny specific. Sure my code only implements the functionality for skinny, but the backend functions for this functionality were written in features.c (basically copying ast_pickup_chan handling) so other channels could also implement. I didn't write it for SIP, or any other channel driver because I've no idea about the those code bases or RFC's. However, given that group pickup is implemented in various other channel drivers (eg SIP/IAX) I see no reason that this functionality couldn't be implemented in those other drivers.
Your suggested method would be horribly convoluted for skinny. Also it is significantly different from ast_chan_pickup which is very similar to this code in both functionality and code. I can't really see why we would want two completly different ways to do something that is so similar ("group pickup" vs "group pickup where exten matches").
Agree to a certain extent on the groups. Suggest I remove directed_callgroup (should remove changes to sip) but leave directed_pickupgroup (should only be skinny code - or other channel that implements this functionality). The reason being that directed pickup typically encompasses a larger set than group pickup.</pre>
</blockquote>
<p>On May 2nd, 2013, 4:48 p.m. UTC, <b>rmudgett</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;">For Asterisk 12, the features.c file is for the most part being gutted. For this directed pickup, it really would be better if you implemented it like I suggested so the administrator could control the call pickup in dialplan. Also, only channel drivers that intimately control the behavior of their devices like chan_skinny can support this kind of feature within the channel driver. Other channel drivers for protocols like SIP and IAX would need to do this in dialplan anyway.
If I could, I would remove the features.conf pickup feature map. There is no good reason why channel drivers should know about a special pickup exten. All you need to do is put an exten in your dialplan using the Pickup application. The dialplan even allows more flexibility. Admittedly, to get feature parity with the special pickup exten, the Pickup application needs to add pickup success/fail sound options.
Could a channel variable be used in place of your configured directed pickup groups? This would eliminate the need to modify the ast_channel struct and allow other channel drivers like SIP and IAX to participate without changing their drivers. Another benefit is that you would not be limited to 64 groups.</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;">Okay, I don't see any technical reason why this can't be implemented through the dialplan, but the question really becomes should it be. This patch is a small extension to ast_pickup_call which is currently in channels misdn, mgcp, sip (I am aware of gulp), skinny, vpb, dahdi and unistim.
Currently chan_sip (I haven't really looked at the other drivers usage of ast_pickup_call) seems to use ast_pickup_call for a general pickup and has a do_magic_pickup that does a pbx_exec on pbx_findapp("Pickup"). Although I don't know enough to be certain, it would appear that SIP would be able to utilise the new ast_pickup_call_directed. Further the naming gives the idea that do_magic_pickup is a bit of a hack.
While I appreciate your approach of running all this stuff through the dialplan and remove features.c pickup stuff completely, is that the intended direction for asterisk. If so, this should be noted somewhere and plans should be made to move at least the main chan drivers away from using ast_pickup_call. If not, I think the approach in this patch is probably the best given that functionally this is basically a copy and paste of ast_pickup_call with only a couple of lines or real changes (ie that extension matching stuff in features.c).
Personally I'm against the approach of trying to run everything through the dialplan, but that's only my opinion.</pre>
<br />
<p>- wedhorn</p>
<br />
<p>On April 5th, 2013, 7:16 a.m. UTC, wedhorn wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/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 wedhorn.</div>
<p style="color: grey;"><i>Updated April 5, 2013, 7:16 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">Added ast_pickup_call_directed to features. Included some refactoring of ast_pickup_call to reuse same code.
In order to use ast_pickup_call, the chan passed in must have the exten set of the intended target. Will only pickup the target if the exten's match and the pickupgroupdirected and callgroupdirected of the chans match.
Also add ast_channel_callgroupdirected and ast_channel_pickupgroupdirected functions (and set functions) including code to set both of these in skinny and sip.
Code to actually do a directed pickup included in skinny.</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;">Directed pickup to non ringing skinny and non ringing sip device (with matching and unset groups). Returns without picking up.
Directed pickup to ringing skinny and ringing sip device, with matching groups, picks up call, with unmatching groups return without pickup.</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/CHANGES <span style="color: grey">(383948)</span></li>
<li>/trunk/channels/chan_skinny.c <span style="color: grey">(383948)</span></li>
<li>/trunk/configs/skinny.conf.sample <span style="color: grey">(383948)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(383948)</span></li>
<li>/trunk/include/asterisk/features.h <span style="color: grey">(383948)</span></li>
<li>/trunk/main/channel_internal_api.c <span style="color: grey">(383948)</span></li>
<li>/trunk/main/features.c <span style="color: grey">(383948)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2393/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>