[asterisk-dev] [Code Review] 2393: Add directed pickup to features

Thu May 16 16:37:22 CDT 2013

Sorry I'm sending this off reviewboard, but this seems easier.

On 17/05/13 04:30, Mark Michelson wrote:
> This is an automatically generated e-mail. To reply, visit: 
> https://reviewboard.asterisk.org/r/2393/
>
>
> 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.
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.

> 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.
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).
> 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.
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.
> 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.
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.

As such, either method is likely subject to the same issue/s.
> 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.
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.
> 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.
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.
> 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.
>
> - Mark
>
And a couple of major things.

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).

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.

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.

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.

Damien
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130517/592912db/attachment.htm>