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

[Tzafrir Cohen]

On Sat, Nov 08, 2008 at 01:40:47AM -0000, Moises Silva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/40/
> -----------------------------------------------------------
> 
> (Updated 2008-11-07 19:40:47.767742)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary (updated)
> -------
> 
> 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. 

Only thing to consider with spandsp is: which version to use?

> 
> 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.

NUM_SPANS still defaults to 32, IIRC. We only need such a constant
because we maintain an array of "struct pri"-s. Now you use this for
something that is not even a dahdi span.  

What does this monitor thread do? Why not use the existing monitor
thread?

> 
> 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?

(So most users get two monitor threads per dahdi span)

Next thing is, what are all of those extra globals, and why are they
reset when:
1. A channel is destroyed (will it break other calls?)
2. load_module() (just before setup_dahdi() ).

They are not, initialised, however, on reload. Thus if you have:

  ; the default of mfcr2_max_ani is 10
  channel => 1-15,17-31
  
  mfcr2_max_ani = 5
  channel => 32-46,48-62

then after a reload the first span will be set from mfcr2_max_ani=5 as
well.

Please use struct dahdi_chan_conf for your configuration. It is
initialised explicitly.

> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 155505 
>   /trunk/configs/chan_dahdi.conf.sample 155505 
>   /trunk/configure.ac 155505 
>   /trunk/include/asterisk/dsp.h 155505 
>   /trunk/main/dsp.c 155505 
>   /trunk/makeopts.in 155505 
> 
> Diff: http://reviewboard.digium.com/r/40/diff

BTW: there are occasional white spaces at the end of line and such
"reds" there.

-- 
               Tzafrir Cohen
icq#16849755              jabber:tzafrir.cohen at xorcom.com
+972-50-7952406           mailto:tzafrir.cohen at xorcom.com
http://www.xorcom.com  iax:guest at local.xorcom.com/tzafrir