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

[rmudgett]

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



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21192>

    Need a
    <depend>app_chanspy</depend>



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21191>

    Document the (On write) hook_id.
    
    Is this the first function that has different parameters for read and write?



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21190>

    Update to what the function accepts for parameters.



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21189>

    Use <variable>HOOK_CHANNEL</variable> and <variable>HOOK_ID</variable>



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21196>

    These should not be uppercase.  That indicates they are macros.



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21193>

    Doxygen



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21194>

    Doxygen



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21195>

    Doxygen



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21182>

    You need to check if args.interval is NULL.  Also the sscanf should be using "%30u" instead.  Also passing a NULL string pointer to printf will crash on Solaris.
    
    Missing \n



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21184>

    Need to add a \n to the message.



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21180>

    Doesn't hook_id need to be checked for NULL when calling ast_channel_datastore_find() to get what you want?  Also passing a NULL string pointer to printf will crash on Solaris.



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21181>

    Idem.



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21185>

    Maybe use ast_true() and ast_false() instead.



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21186>

    Missing \n



/trunk/funcs/func_periodic_hook.c
<https://reviewboard.asterisk.org/r/3362/#comment21187>

    Need to count right for the priority.  You have two priority 2's.


- rmudgett


On April 1, 2014, 11:43 a.m., Russell Bryant wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3362/
> -----------------------------------------------------------
> 
> (Updated April 1, 2014, 11:43 a.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 411583 
> 
> 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.
> 
> [hooks]
> 
> exten => beep,1,Answer()
>     same => n,Verbose(1,Channel name: ${HOOK_CHANNEL})
>     same => n,Verbose(1,Hook ID: ${HOOK_ID})
>     same => n,Playback(beep)
> 
> [test]
> 
> exten => 100,1,Answer()
>     same => n,Set(BEEP_ID=${PERIODIC_HOOK(hooks,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/20140402/89c1528e/attachment-0001.html>