[asterisk-dev] [Code Review] 2973: Add equivalent AMI actions for the bridge CLI commands.

[Matt Jordan]

> On Oct. 31, 2013, 3:07 p.m., Olle E Johansson wrote:
> > These are not events, it's actions :-)
> > 
> > For many years I tried to stop implementation of new actions and events like FooStart and FooStop or StopFoo and StartFoo. They are usually not grouped together in documentation and it feels strange. Better to have a FooStatus or just Foo command with a Status: on or off
> > 
> > I know we have a lot of old ones still around, but it's hard to deprecate them. Let's just agree not to add new mistakes :-)
> > 
> > Just my 5 öre.
> > 
> > /O

Well...

We went the other direction (sorry!) Prior to writing the specification for Asterisk 12/AMI 1.4, I tried to research as much as possible why things were the way they were. As it was, there was no clear direction documented on the wiki - or that I could find in the old mailing list archives - on whether it was preferred to have two events indicting the start and completion of an event versus using a field in a single event to indicate that. There is, however, a technical reason to prefer two events over one. As I noted on https://reviewboard.asterisk.org/r/2959/:

{quote}
As far as the substatus field goes, the specification for AMI went with explicit event names to indicate the beginning and ending of a pair of events. That approach was chosen as it allows libraries to key off of the begin or end events if that is the only event that they care about; this reduces the amount of work implementers have to do as they don't have to inspect a sub type field to determine whether or not they care about the event.
{quote}

It's a minor point, but it does let a client that listens for events and only cares about one of the events in a pair - do the following:

{code}
ami.registerEvent('DTMFEnd', self._handle_dtmf)

def _handle_dtmf(self, ami, event):
  ''' I know I have the DTMF End event here, as that's the one I asked for '''
{code}

versus:

{code}
ami.registerEvent('DTMF', self._handle_dtmf)

def _handle_dtmf(self, ami, event):
  ''' I have to check the subtype field to see if it's what I want '''

  if event['subtype'] == 'Begin':
    return

  ...
{code}

For lots of event handlers, that's nice, and it helps prevent logic errors on the part of the consumer. In general, if you want both events, it doesn't have to be more code than the subtype approach. You typically just have to register for both events:

{code}

ami.registerEvent('DTMFBegin', self._handle_dtmf)
ami.registerEvent('DTMFEnd', self._handle_dtmf)

def _handle_dtmf(self, ami, event):
  if event['event'] == 'DTMFBegin':
    # do begin
  elif event['event'] == 'DTMFEnd':
    # do end
{code}

versus:

{code}

ami.registerEvent('DTMF', self._handle_dtmf)

def _handle_dtmf(self, ami, event):
  if event['subtype'] == 'Begin':
    # do begin
  elif event['subtype'] == 'End':
    # do end
{code}

Although removing subtype fields is not super explicit in the AMI 1.4 spec, it is noted that a number of events were going to be broken up, e.g., DTMF => DTMFBegin/DTMFEnd. If I didn't call attention to that loudly enough last year, then I am sorry about that.

Documentation and linking of the events together is a valid concern. Luckily, we *did* document all of the AMI events in the past year, but most of the events are missing the see-also links to their corresponding event. That's a pretty easy thing to fix, as most of the event documentation is in manager_channels - and the rest is pretty easy to find in the source as well. I'll make an issue to get that cleaned up.


- Matt


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


On Oct. 31, 2013, 2:56 p.m., Scott Griepentrog wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2973/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 2:56 p.m.)
> 
> 
> Review request for Asterisk Developers and Matt Jordan.
> 
> 
> Bugs: ASTERISK-22356
>     https://issues.asterisk.org/jira/browse/ASTERISK-22356
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds the following AMI events mirroring their CLI counterparts:
> 
> BridgeDestroy
> BridgeKick
> BridgeTechnologyList
> BridgeTechnologySuspend
> BridgeTechnologyUnsuspend
> 
> In the case of BridgeKick, to simplify the coding both a BridgeUniqueid and a Channel name must be provided to kick a channel.
> 
> The Technology calls are implemented in bridge.c due to the technology link list being located there.
> 
> Also in bridge.c, a new function ast_bridge_find_by_id() is added to expose the bridges ao2 container in a convenient fashion.
> 
> Tests for these AMI commands are here: https://reviewboard.asterisk.org/r/2975/
>  
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/manager_bridges.c 402126 
>   /branches/12/main/bridge.c 402126 
>   /branches/12/include/asterisk/bridge.h 402126 
> 
> Diff: https://reviewboard.asterisk.org/r/2973/diff/
> 
> 
> Testing
> -------
> 
> All tests written passed to insure basic functionality.
> 
> 
> Thanks,
> 
> Scott Griepentrog
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131031/ee37e454/attachment-0001.html>