[asterisk-bugs] [Zaptel 0008290]: [patch] zap hookstate is never set offhook if wctdm module is loaded with an active fxo line

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=8290 
====================================================================== 
Reported By:                tmarkov
Assigned To:                mattf
====================================================================== 
Project:                    Zaptel
Issue ID:                   8290
Category:                   wctdm
Reproducibility:            always
Severity:                   minor
Priority:                   normal
Status:                     assigned
Zaptel Version:              1.2.10 
SVN Branch (only for SVN checkouts, not tarball releases): N/A 
SVN Revision (number only!):  
Disclaimer on File?:        Yes 
Request Review:              
====================================================================== 
Date Submitted:             11-03-2006 17:28 CST
Last Modified:              12-04-2007 12:44 CST
====================================================================== 
Summary:                    [patch] zap hookstate is never set offhook if wctdm
module is loaded with an active fxo line
Description: 
If #define ZAP_CHECK_HOOKSTATE is uncommented in chan_zap.c, then the
hookstate stays onhook if the wctdm module is loaded with an active line. 
Removing line and reinserting will cause the hookstate to initialize
properly.  The problem appears to be that wc->mod[card].fxo.battery never
gets initialized to 0, so in wctdm_voicedaa_check_hook() the test "if
(!wc->mod[card].fxo.battery && !wc->mod[card].fxo.battdebounce)" is never
entered, and the hookstate remains onhook upon module startup.  Only a
cycling of line voltage will correct the problem.  I have attached a patch
which initializes the battery variable and fixes the problem for me.
====================================================================== 

---------------------------------------------------------------------- 
 meneault - 12-04-07 12:44  
---------------------------------------------------------------------- 
flefoll wrote:
>- chan->rxhooksig is written only from outside zaptel-base.c, through
hook >functions zt_hooksig() and zt_rbsbits().
>Consequently, EXCEPT for the init sequence, it is the job of
"sub-drivers" >(wctdm.c or others) to update chan->rxhooksig.

This is exactly what I meant. zt_hooksig is called by "sub-drivers" but
the code that touches chan->rxhooksig is part of zaptel-base.c.
For the moment chan->rxhooksig is only modified by zaptel-base.c code, in
theory "sub-drivers" shouldn't even be aware of chan->rxhooksig from my
point of view.
They only have to update hook state by calling zt_hooksig.

zaptel-base.c is meant to handle all the tricky code of zaptel while
sub-drivers don't have to deal with all this complexity and should only be
aware of a simple interface (zt_hooksig, zt_register...). Now you should
understand why I said that chan->rxhooksig shouldn't be touched by
"sub-drivers".
This is only a question of design consideration and maybe the maintainers
have a different point of view on this.

I agree that this point of view is problematic concerning our issue,
because the only solution in that case is to call regularly zt_hooksig
because this is the only interface available to sub-drivers. This is why I
made the patch that way.
I tried to leverage the cost of this by only calling zt_hooksig every
battdebounce ms. Moreover, If you look at zt_hooksig in the common case It
would only lock on chan's lock and then return because rxhooksig hasn't
changed since last call. So the cost is only a question of getting
channel's lock every battdebounce ms.

And with your solution (done safely i.e. locking on channel's lock before
testing the value of rxhooksig) this would be still slower because you want
to do it every time voice_daa_check_hook is called thus would cause more
locking concurrent accesses.

>in zt_ctl_ioctl(ZT_STARTUP or ZT_CHANCONFIG) 

The main issue is clearly here, I wanted to avoid changing zaptel-base.c
so as the patch to be accepted faster (when you look at the first patch
proposed by tmarkov it modified the problematic code with compile-time
defines, I wanted to avoid compile-time defines -- really bad for code
clarity -- and avoid changing
zaptel-base.c even if this is where the problem lies).
I thought that a patch modifying zaptel-base.c, zaptel.h, wctdm.c would
hardly be accepted and as wctdm.c was to be modified (because of its race
condition) I decided to only patch wctdm.c hiding the issue with the least
negative impact possible.

Anyway, now that the issue has been raised, the correct way to handle the
issue is that way:
ZT_STARTUP and ZT_CHANCONFIG should trigger "sub-drivers" that rxhooksig
has changed (and not the other way). The easy solution is to add a callback
function to the span structure (zt_hooksig_reset) so as to warn
"sub-drivers" that they should call zt_hooksig again.

mattf please tell us what is your point of view, If you want me to write
the cleanest patch, it will be straight forward but now it is up to you to
give your opinion. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
12-04-07 12:44  meneault       Note Added: 0074766                          
======================================================================