Summary:ASTERISK-07072: [patch] Q921 debugging options
Reporter:Frederic LE FOLL (flefoll)Labels:
Date Opened:2006-06-01 07:35:28Date Closed:2006-06-06 20:11:44
Versions:Frequency of
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