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

[Moises Silva]

> On 2009-02-11 16:33:44, Russell Bryant wrote:
> > /trunk/channels/chan_dahdi.c, lines 11051-11081
> > <http://reviewboard.digium.com/r/40/diff/3/?file=2304#file2304line11051>
> >
> >     Is there a guarantee that the time until the next event (that you get from openr2_context_get_time_to_next_event()) will not change in between the time that you get it and when you enter poll() ?  If not, does it matter?

Good catch. It is not important in this case since the only reason we could get delayed (significantly, more than 500ms) to enter poll is when monitored_count_lock is 0, and in that case, we don't really care about channel events since that means the R2 channel events are now being handled by the PBX thread and not the monitor thread. For the sake of being clear I will move the openr2_get_time_to_next_event call just before the poll, which makes a lot of more sense.


> On 2009-02-11 16:33:44, Russell Bryant wrote:
> > /trunk/channels/chan_dahdi.c, lines 16190-16191
> > <http://reviewboard.digium.com/r/40/diff/3/?file=2304#file2304line16190>
> >
> >     Contrary to your description, it actually looks like you create a thread for _every_ channel, regardless of how they are entered into the configuration.  The dahdi_mfcr2 struct gets allocated inside the call to mkintf(), which is called for every channel number.
> >     
> >     I also see that in your mfcr2_monitor function, you have code to handle when there are multiple channels in a dahdi_mfcr2 instance.  So, it looks like you intend to be able to handle multiple channels in the same thread.
> >     
> >     What do you want to do here?

I should probably clarify a bit more the code by trying to accomplish the same in a different way, right now is a bit hackish. You got confused because it seems the dahdi_mfcr2 struct gets allocated inside mkintf() each time we get into mkintf(), but it does not. If you go to build_channels() you will see that only after the loop that makes the calls to mkintf() the r2links_index is incremented, therefore, all channels in the same channel= line get the same r2link. If you feel this is confusing and have some suggestion to change it I am open to it. However, it has been working well that way and now there is no limit on the r2links structs since they are allocated dynamically in dahdi_r2_get_link(). In conclusion, monitor threads are created for several channels per user configuration.

channel=1,2,3,4,5,6,7,8,9,10
This would create 10 monitor threads, one for each channel.
channel=1-10
This would create just 1 thread for the 10 channels. 


- Moises


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


On 2009-01-09 17:04:57, Moises Silva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/40/
> -----------------------------------------------------------
> 
> (Updated 2009-01-09 17:04:57)
> 
> 
> 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 168017 
>   /trunk/configs/chan_dahdi.conf.sample 168017 
>   /trunk/configure.ac 168017 
>   /trunk/makeopts.in 168017 
> 
> 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
> 
>