[asterisk-dev] [Code Review] MFC/R2 support for chan_dahdi

Sun Dec 14 07:01:39 CST 2008

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/40/#review221
-----------------------------------------------------------


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment431>

    This is pretty minor, but you could optimize the size of this struct by indicating that certain fields only require one bit:
    
       unsigned int allow_collect_calls:1;


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment432>

    Don't use C++ style comments.
    
    Also, you don't need to explicitly initialize these.  They get initialized to 0 for you, automatically.  In fact, not including the initialization is sometimes an optimization, depending on the compiler, because it's less initialized data values to store in the compiled code.


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment433>

    Be careful with this.  The value that you get back from getvar_helper() is _only_ valid as long as the ast_channel is locked.


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment434>

    Please use ast_debug() for DEBUG logging.


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment435>

    To be pedantic ... use '\0' instead of 0.


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment436>

    I'm afraid we'll have to remove "wtf?" from this log message.  We have received complaints in the past for having a sense of humor in the logs.  :-(


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment437>

    Is this the only reason dahdi_new would not return a channel?  What about an allocation failure?  I would leave the error logging up to dahdi_new().


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment438>

    I would probably make this a verbose message - ast_verb()


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment439>

    To reduce indentation here, I would change the first condition to:
    
    if (!p->owner) {
       /* couple lines of error handling */
       return;
    }
    
    /* bunch of code that used to be inside the if block */


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment440>

    50 is a bit more than strlen("Context - ")  :-)
    
    Also, please put spaces around the '+'


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment441>

    Please remove the commented out code if it's not needed anymore.
    
    Also, make sure you compile your code after running configure with --enable-dev-mode.  That will turn on some additional compiler warnings and then force you to fix them by treating all warnings as errors.


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment442>

    You can simplify this by just doing:
    
       return &ast_null_frame;


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment443>

    space after while


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment444>

    use a for loop here


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment445>

    for loop here, too


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment446>

    You can use ARRAY_LEN() here


/trunk/channels/chan_dahdi.c
<http://reviewboard.digium.com/r/40/#comment447>

    Try using strsep(), instead


- Russell


On 2008-11-17 09:48:05, Moises Silva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/40/
> -----------------------------------------------------------
> 
> (Updated 2008-11-17 09:48:05)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> MFC/R2 support for chan_dahdi.
> 
> The code for tone detection (main/dsp.c) and generation (DAHDI MF support in chan_dahdi.c) is not actually being used now. In the early stages of development, spandsp was not LGPL and that's why applications can provide a MF interface to generate and detect the tones required for this signaling to work. Now that spandsp is LGPL, openr2 has embedded the R2 MF detector and generator and is the one currently being used. I had issues (tones either not being generated or not being detected properly) in some x86 64 bit, and the detection was 50% slower than the one in spandsp. Unless there is some important reason you can think of to switch back to DAHDI and main/dsp generator and detector, I suggest to get rid of that code, but I wanted to ask first. 
> 
> There is an issue with the way I currently handle how many monitor threads I launch. This code will launch as many monitor threads as group of channels are specified in the chan_dahdi.conf file.
> 
> channel=1-15,17-31
> 
> That would launch 2 monitor threads and use 2 r2link structures. There is maximum of NUM_SPANS r2link structures statically allocated when the module is loaded.
> 
> channel=1,2,3,4,5,6,7,8,9,10,11,12,13,14,15
> 
> That would launch 15 threads, 15 r2link structures would be used with only 1 channel each. If some user specify the channels this way, a single DAHDI span would consume all the available structures. Not a common scenario though, since most users would just specify 1-15 in such cases. Should I get rid of that mechanism for launching monitor threads?
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 157241 
>   /trunk/configs/chan_dahdi.conf.sample 157241 
>   /trunk/configure.ac 157241 
>   /trunk/makeopts.in 157241 
> 
> Diff: http://reviewboard.digium.com/r/40/diff
> 
> 
> Testing
> -------
> 
> Lots of users currently using this in 1.2, 1.4 and 1.6 in Brazil, México and Argentina. An unknown number of users (but at least 1) for each of the following countries: colombia, nepal, thailand, venezuela, perú and probably others. The code in chan_dahdi.c is independent of the variants though, which are handled by the OpenR2 library.
> 
> 
> Thanks,
> 
> Moises
> 
>