[asterisk-dev] [Code Review] 3362: func_periodic_hook: New function for periodic hooks.

[Corey Farrell]

> On March 29, 2014, 8:12 p.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 299
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line299>
> >
> >     Can we detach the audio hook here?  I know we escape the hook callback very early but it still feels wasteful to keep it enabled.
> >     
> >     Possibly remove state->disabled variable?
> 
> Russell Bryant wrote:
>     As you said, the callback does just about nothing, so I don't think it really hurts to keep it.
>     
>     But honestly, I would have done it this way if the audiohooks API was built for dynamic adding/removing.  It really isn't.  No other code does that.  It all relies on channel destruction to tear it down.

I took a closer look at audiohook.c, I agree the removal isn't very pretty.


> On March 29, 2014, 8:12 p.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 206
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line206>
> >
> >     state->last_hook is never pre-initialized to ast_tvnow(), so hooks would always trigger on first check after being created or re-enabled.  Unless this behaviour is documented I would expect the first run after create or resume to be delayed state->interval seconds.
> 
> Russell Bryant wrote:
>     Fair.  I had originally intended for it to run immediately, but I think it makes more sense to delay.
> 
> jamuel wrote:
>     So is the implication then if I were to set the interval to 30 seconds that the hook wouldn't run for 30 seconds and then run every 30 seconds?  That seems more counter intuitive.  I'd think that it should run and then repeat every interval seconds.
> 
> Russell Bryant wrote:
>     Yes, that's right.  I keep going back and forth but I think I agree with you that running immediately makes the most sense.
> 
> jamuel wrote:
>     It seems to make more sense especially when the interval is "really long".  When the interval is just a few seconds it doesn't really matter but when it's say 30s or 60s (or even longer) then it seems weird to have to wait that long for it to run the first time--especially in your original use case of wanting to beep to indicate call recording . . .

I'm not going to hold up this review for this finding, but I disagree.  Please point me to one implementation of an interval where the first run is immediately.  I can't think of any.

Also with first run delayed by interval, if you want a beep immediately you can just play one.  With no delay on first interval it's impossible to prevent the immediate beep.


> On March 29, 2014, 8:12 p.m., Corey Farrell wrote:
> > /trunk/funcs/func_periodic_hook.c, line 273
> > <https://reviewboard.asterisk.org/r/3362/diff/4/?file=56930#file56930line273>
> >
> >     Need checks for !ast_strlen_zero on all args.  Maybe also a verify the exten at context exists?
> 
> Russell Bryant wrote:
>     I think this is all handled.  For interval, sscanf() will just fail on an empty string.  For exten and context, defaults of "s" and "default" are applied.
>     
>     Also, I don't think it should check for the exten to exist.  If it doesn't, errors should show up when it tries to run.  Extensions can be added/removed dynamically, so I think we should just assume the best here.

same => n,Set(BEEP_ID=${PERIODIC_HOOK()})

I believe the above dialplan would crash.  With no parameters all fields of args remain NULL.

same => n,Set(BEEP_ID=${PERIODIC_HOOK(5)})

This will result in a value for args.interval, but all other parameters NULL.


- Corey


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


On March 31, 2014, 12:11 p.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3362/
> -----------------------------------------------------------
> 
> (Updated March 31, 2014, 12:11 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
>     This commit introduces a new dialplan function, PERIODIC_HOOK().
>     It allows you run to a dialplan hook on a channel periodically.  The
>     original use case that inspired this was the ability to play a beep
>     periodically into a call being recorded.  The implementation is much
>     more generic though and could be used for many other things.
>     
>     The implementation makes heavy use of existing Asterisk components.
>     It uses a combination of Local channels and ChanSpy() to run some
>     custom dialplan and inject any audio it generates into an active call.
>     
>     The other important bit of the implementation is how it figures out
>     when to trigger the beep playback.  This implementation uses the
>     audiohook API, even though it's not actually touching the audio in any
>     way.  It's a convenient way to get a callback and check if it's time
>     to kick off another beep.  It would be nice if this was timer event
>     based instead of polling based, but unfortunately I don't see a way to
>     do it that won't interfere with other things.
> 
> 
> Diffs
> -----
> 
>   /trunk/funcs/func_periodic_hook.c PRE-CREATION 
>   /trunk/CHANGES 411572 
> 
> Diff: https://reviewboard.asterisk.org/r/3362/diff/
> 
> 
> Testing
> -------
> 
> Called the following extension (100 at test), both letting it run all the way through, as well as hanging up at various points in the middle.
> 
> 
> [macro-beep]
> 
> exten => s,1,Answer()
>     same => n,Verbose(1,Channel name: ${ARG1})
>     same => n,Verbose(1,Hook ID: ${ARG2})
>     same => n,Playback(beep)
> 
> [test]
> 
> exten => 100,1,Answer()
>     same => n,Set(BEEP_ID=${PERIODIC_HOOK(beep,5)})
>     same => n,Wait(20)
>     same => n,Set(PERIODIC_HOOK(${BEEP_ID})=off)
>     same => n,Wait(20)
>     same => n,Set(PERIODIC_HOOK(${BEEP_ID})=on)
>     same => n,Wait(20)
>     same => n,Hangup()
> 
> 
> Thanks,
> 
> Russell Bryant
> 
>

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