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

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2393/#review8634
-----------------------------------------------------------


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.

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.

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.

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.

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.

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.

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 Michelson


On April 5, 2013, 7:16 a.m., wedhorn wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2393/
> -----------------------------------------------------------
> 
> (Updated April 5, 2013, 7:16 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 383948 
>   /trunk/channels/chan_skinny.c 383948 
>   /trunk/configs/skinny.conf.sample 383948 
>   /trunk/include/asterisk/channel.h 383948 
>   /trunk/include/asterisk/features.h 383948 
>   /trunk/main/channel_internal_api.c 383948 
>   /trunk/main/features.c 383948 
> 
> Diff: https://reviewboard.asterisk.org/r/2393/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> wedhorn
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130516/c2f7de18/attachment.htm>