<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#ffffff">
Sorry I'm sending this off reviewboard, but this seems easier.<br>
<br>
On 17/05/13 04:30, Mark Michelson wrote:
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<table style="border: 1px solid rgb(201, 195, 153);"
width="100%" bgcolor="#f9f3c9" cellpadding="8">
<tbody>
<tr>
<td> This is an automatically generated e-mail. To reply,
visit: <a moz-do-not-send="true"
href="https://reviewboard.asterisk.org/r/2393/">https://reviewboard.asterisk.org/r/2393/</a>
</td>
</tr>
</tbody>
</table>
<br>
<pre style="white-space: pre-wrap; word-wrap: break-word;">Let me throw my hat into this discussion a bit.
My first thought upon reading this code was that the change was very much inline with other changes that are going on in the features and bridging core, such as a core function to perform blind transfers and attended transfers. Good change!
However, as I looked more, I realized why this might not be the proper approach to take.
First, fine-tuned control is missing. For chan_skinny, a "directedpickupgroup" option was added to restrict access to who can perform directed pickups. I noticed that such an option does not exist for the Pickup() dialplan application and started wondering why. Then it became clear to me. The reason is that in the dialplan, you don't have to go by just callgroups and pickupgroups to determine if a call can be picked up. You can use *anything* you want to do it. The dialplan can restrict based on caller ID, time of day, presence status, or any other possible thing. In addition, call pickup access can be varied from call-to-call instead of relying on a configuration that is set at module load time.
</pre>
</div>
</blockquote>
Granted that the dialplan can give more control, but I think you are
looking at the use of directed_pickup_group in the wrong way. The
directed_pickup_group is not so much used for who can do directed
pickups (although it can certain be used for that), but to limit
what people can pickup. A simple workplace configuration may be that
a device can do a group pickup for calls within their work unit but
do a directed pickup on other work units on the same floor as them.<br>
<br>
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<pre style="white-space: pre-wrap; word-wrap: break-word;">Second, the design places burden on all channel drivers that may wish to call into the new function. I'll once again refer to directedpickupgroup as the example. chan_skinny had to create a new config option in order to be able to use the feature. If any other channel drivers wanted to make use of the directed pickup APIs, they also would have to have directedpickupgroup configurable. The same goes for any other options that may get added.
</pre>
</div>
</blockquote>
Granted. However a similar burden exists for channel drivers that
want to implement directed pickup through the dialplan. Somehow the
channel driver needs to know what extension to activate a pickup and
then pass in the actual extension that wants to be picked up into
the dialplan (eg through a chanvar).<br>
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<pre style="white-space: pre-wrap; word-wrap: break-word;">Third, This method does not allow for as graceful a failure as routing the call to the dialplan would. In the dialplan, if call pickup fails, then it becomes easy to play sound files or present an IVR to the person attempting to pick the call up. If call pickup is handled in the channel driver, then it still could be possible to play sound files to the picker, but then the configuration issue arises again. What sounds do you play? Does that have to be configured per-channel driver? The dialplan again allows for more flexibility in call handling.
</pre>
</div>
</blockquote>
Okay, this one becomes prickly. Generally the channel driver will be
more aware of the user sequence up to this point and as such, the
subsequent sequence of events to provide "good" feedback to the
user. In skinny, a hangup and display a prompt on a failed pickup
works well. On dahdi this would probably be useless. As such,
multiple dialplans may be required to handle different channel
drivers effectively.<br>
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<pre style="white-space: pre-wrap; word-wrap: break-word;">Fourth (and you can't be blamed for not knowing this), this method is going to run into issues if/when bridging of calls starts happening earlier than when it currently happens. At some point in the future, the idea is that a bridge will be built in order to actually dial a channel. Thus, we no longer have a bunch of "if bridged, do this, or else do that" situations any longer. The side effect of that is that the features supported will be set per-channel. These per-channel features will likely be set in the dialplan. During a call pickup, if the picker enters the dialplan, then call features for the picker channel can be set. The picker channel can replace the ringing channel in the bridge, and there is no confusion about what features are enabled for the call. With call pickup being handled in the channel driver, the configuration option problem becomes an issue again.
</pre>
</div>
</blockquote>
I'm not so sure how much this actually relates to the proposed
patch. While I'd need to go back and look at the code in detail, a
lot of the underlying functions used by features pickup (and the
proposed directed pickup) and app_directed_pickup are the same and
actually reside in features.c. If there is a future problem in the
way a channel is assessed, it will likely also exist in
app_directed_pickup and will be fixed with app_directed_pickup.<br>
<br>
As such, either method is likely subject to the same issue/s.<br>
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<pre style="white-space: pre-wrap; word-wrap: break-word;">Essentially the issue here is that adding this function sacrifices flexibility and places burdens on channel drivers in order to be able to support the directed call pickup feature. I wanted to address a couple of other points that should be brought up.
</pre>
</div>
</blockquote>
Yes, it places a burden but the reason we do it is for control and
to provide good feedback on the specific devices we manage.<br>
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<pre style="white-space: pre-wrap; word-wrap: break-word;">You had mentioned do_magic_pickup() in chan_sip. I would not look to this as a bastion of how to handle call pickup :). This was implemented as a quick hack in order to support a call pickup feature present on certain SNOM devices. It suffers from the some of the same problems (lack of fine-tuned control, lack of graceful failure). It should be handled the way Richard has suggested, but for now it currently isn't. Also, the pickupexten option in features.c has the same issues.
</pre>
</div>
</blockquote>
I think you misunderstood me. I was pointing to do_magic_pickup as a
hack that could possibly use the new function. I used
ast_pickup_call as the bastion of how to handle call pickup, not
do_magic_pickup.<br>
<blockquote
cite="mid:20130516183040.23901.87827@hotblack.digium.com"
type="cite">
<div style="font-family: Verdana,Arial,Helvetica,Sans-Serif;">
<pre style="white-space: pre-wrap; word-wrap: break-word;">So the real issue with the approach here, is that you're basically causing a potential maintenance burden for yourself and other channel driver maintainers that wish to use the new core directed pickup APIs. The dialplan method may seem more convoluted from the perspective of a channel driver writer, but the benefits it provides users are huge. A general rule of thumb when working with Asterisk is that if a call can be sent to the dialplan, it should.</pre>
<br>
<p>- Mark</p>
</div>
</blockquote>
And a couple of major things.<br>
<br>
The suggested code is an extension of ast_pickup_call. It uses some
of the same configuration, most importantly the existing callgroups.
As such, if the dialplan method was used, users wanting to implement
both pickup and directed pickup would be required to move all
arrangements to dialplan (including recoding the current channel
drivers that use ast_pickup_call) or maintain two separate groupings
(namely the channel configs for general pickup and dialplan for
directed pickup).<br>
<br>
While the patch looks like there are large changes, most of the
changes in features.c are refactoring of the ast_pickup_call
functions to allow the new function to use them. In effect there are
three code changes directly related to this patch in features.c,
namely the addition of ast_pickup_call_directed (we need a function
to call) which is basically a copy of ast_pickup_call,
ast_pickup_find_by_exten which is basically a copy of
ast_pickup_fund_by_group and find_channel_by_exten which is
basically a copy of find_channel_by_group. To highlight how similar
these functions are, to replace the current group pickup with a
directed pickup only requires to changes to features.c, namely, to
test if an extension has been passed in (in ast_pickup_call) and a
slight modification to the if statement in find_channel_by_group so
that the extension are compared.<br>
<br>
Fundamentally, this is a very minor addition (despite what the diffs
look like) to the existing ast_pickup_call functionality. Arguably
most of the arguments against this method are also valid issue for
ast_pickup_call. Running it through the dialplan separates this from
the existing callgroup arrangements.<br>
<br>
Further, group pickup through the dialplan probably already exists
and as such, users wanting to do the dialplan approach and ignore
the callgroup/pickupgroup/directedpickup group arrangements, already
can.<br>
<br>
Damien<br>
</body>
</html>