Summary:	ASTERISK-07072: [patch] Q921 debugging options
Reporter:	Frederic LE FOLL (flefoll)	Labels:
Date Opened:	2006-06-01 07:35:28	Date Closed:	2006-06-06 20:11:44
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) patch_chan_zap.c_pri_debug.txt ( 1) patch_pri_internal.h_debug_macro.txt ( 2) patch_q921.c_debug_levels.txt ( 3) q921_start_reset.png
Description:	While trying to investigate Asterisk behaviour in case of E1 connection problems, I saw that PRI_DEBUG_Q921_STATE did not explicitely mention L2 state changes (i.e., Q921_LINK_CONNECTION_RELEASED, Q921_LINK_CONNECTION_ESTABLISHED, Q921_AWAITING_ESTABLISH, Q921_AWAITING_RELEASE). On the other hand, PRI_DEBUG_Q921_STATE triggers traces of all timers, NR, RR messages and so on. I propose a patch for q921.c that reassigns debug levels more strictly (imho) : - Q921_STATE : Q921 state changes + connection/disconnection messages - Q921_DUMP : all messages (SABME, UA, NR, RR, ...) and timers - Q921_RAW : raw data This q921.c patch most often tests Q921_DUMP instead of Q921_STATE (except where real state changes occur), and adds a few pri_message() calls where q921_state is modified. The additional pri_message() calls include DBGHEAD and DBGINFO macros in order to display file, line and function information (lightweight version of Asterisk ast_log header). I propose to introduce these macros in a patch for pri_internal.h. Finally, if this patch for Q921 is accepted, it becomes possible to introduce PRI_DEBUG_Q921_STATE in "pri debug" mode (since it will only trace L2 major state changes, and not all L2 periodical messages). So, I propose a patch for Asterisk trunk head chan_zap.c (rev 31126) that adds PRI_DEBUG_Q921_STATE flag in handle_pri_debug() ; additionnaly, this patch also adds PRI_DEBUG_Q931_DEBUG_STATE that was missing in handle_pri_really_debug().
Comments:	By: Frederic LE FOLL (flefoll) 2006-06-02 03:20:07 Still investigating in Q921 state changes (with the help of the patch that I propose here, and a few additional debug traces), I had a closer look at q921_start() and q921_reset() functions. Wandering in SVN trunk tree, I saw that q921.c has not changed in that aspect for ages (I compared current rev 309 and 2003 rev 060). One evolution appeared in pri.c, with the introduction of pri_restart() function (bug ASTERISK-5043, released in 1.2). I tried to draw a "subjective" call graph that I will upload in .png format, if this is possible. This call graph shows a few things : - The recent pric.c:pri_restart() function calls q921_reset(), plus q921_start(). I guess q921_reset() call is useless, since q921_start() already calls q921_reset(). It this were changed, q921_reset() could become a static function. - q921_dchannel_up() first calls q921_reset(), then cancels SABME timer and resets sentrej counter, while q921_reset() already did that. - q921_dchannel_up() calls q921_reset(), which sets L2 state to Q921_LINK_CONNECTION_RELEASED, then overrides L2 state to Q921_LINK_CONNECTION_ESTABLISHED. Maybe be the L2 state could be passed as a argument to q921_reset() ? I don't want to propose a patch for this at the moment, since the remarks above only point out redundant operations, and above all since Q921 is not the easiest part in Asterisk. Any comment ? By: Matthew Fredrickson (mattf) 2006-06-06 17:05:44 Thanks for the state update code.  We'll put in the additional debug information and if it doesn't drive too many people nuts we'll keep it there. By: Matthew Fredrickson (mattf) 2006-06-06 17:22:27 Put in trunk By: Serge Vecher (serge-v) 2006-06-06 20:11:43 libpri r350, trunk r32740