Summary:	ASTERISK-06683: [patch] No Manager event on loss/return of E1
Reporter:	Frederic LE FOLL (flefoll)	Labels:
Date Opened:	2006-04-03 05:54:06	Date Closed:	2006-05-31 15:27:16
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Channels/chan_zap
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) patch_alarm_rev27812.txt ( 1) patch_alarm.txt
Description:	When we unplug an E1 link, we see alarms reported by ast_log, but we see no alarm reported to Asterisk Manager clients. Brief analysis - In chan_zap.c, two functions may handle ZT_EVENT_ALARM / ZT_EVENT_NOALARM : zt_handle_event() and handle_init_event(). In both functions, the same ast_log calls appear ("Detected alarm" / "Alarm cleared"), but only zt_handle_event() includes a call to manager_event() to report the alarm Alarm / AlarmClear to AMI clients. Proposal - When I patch chan_zap.c in order to add with manager_event() calls added in handle_init_event() after ZT_EVENT_ALARM and ZT_EVENT_NO_ALARM, then Manager clients do receive Alarm/AlarmClear events when we unplug/replug the E1.
Comments:	By: Olle Johansson (oej) 2006-04-04 02:51:19 Events that signal status on/off for something from now on needs the same name - no EventOn EventOff, but rather Event: Alarm status:on status: off By: Frederic LE FOLL (flefoll) 2006-04-04 08:20:03 I understand your note as related to Manager events (as opposed to chan_zap ZT_... events). ISDN links can indeed generate several different alarms and these alarms may appear/disappear progressively (not all together). I only tested the simple case "Link down / Link up". In this case, from chan_zap point of view, it means that the two existing (1.2.6) manager_event after ZT_EVENT_ALARM and ZT_EVENT_NOALARM should be changed as you describe to "Event: Alarm Status: on" (or ...off), and the same thing should apply to the copy/paste that I suggested after the other ZT_EVENT_ALARM/ZT_EVENT_NOALARM set. My first understanding is that chan_zap may receive successive ZT_EVENT_ALARM events as long as there is one alarm at least, then get_alarms() returns a bitmask and alarm2str() returns the text string associated to the *first* bit in the bitmask. Finally, chan_zap receives ZT_EVENT_NOALARM when everything comes back ok. In order to do what you want, I guess chan_zap should memorize last bitmask receive and calculate the difference with the new one, in order to send as many Manager messages "Event: Alarm Alarm: <some color> Status: on/off" ? I had a look at the some of the svn branches (trunk, branches/1.2, team/oej/managerstuff) and I did not see changes in chan_zap's Manager events generation. Are there already changes in progress in svn ? By: Olle Johansson (oej) 2006-04-04 08:44:56 At this point, we are discussing how to clean up old manager events. We can't change them on a whim, since that would break backwards compatibility. However, we can deny new events that break the new coding guide lines... So for now, we will let the old stuff be there until we change the manager protocol version and move on to a new protocol. By: Frederic LE FOLL (flefoll) 2006-04-04 09:07:23 Sounds wise. Anyway, the patch I proposed does not strictly introduce *new* Manager events : it is just a copy/paste of existing events thrown on ZT_EVENT_ALARM/ZT_EVENT_NOALARM. Let's say that it "propagates" the coding type that you want to change. So, I may reformulate the issue like this : "In order to provide Manager client with all chan_zap alarms, it seems necessary to equip handle_init_event() as well as zt_handle_event() with manager_event() calls (whatever coding rule and syntax we may want to apply to Manager events). Can you give any information about the plans for these Manager evolutions (1.4, ...) ? By: Russell Bryant (russell) 2006-05-05 19:06:39 added to the 1.2 branch and trunk in revisions 25123 and 25124, thanks! By: Frederic LE FOLL (flefoll) 2006-05-15 03:48:41 Hello, I've just had a look today (I was away last week) at asterisk / trunk /channels/chan_zap.c with ViewVC, and I see a problem in the patch : it applied one line too far in handle_init_event() / switch(event) { ... } code : -  'case ZT_EVENT_NOALARM' now is followed by a manager_event() call, but the call appears AFTER the break instruction, instead of before ! - 'case ZT_EVENT_ALARM' now is followed by a manager_event() call, but the call was inserted after the /* fall thru intentionally */ comment, instead of before. I checked builds 25124, 26417 and 26531, and they all have the problem. By: Serge Vecher (serge-v) 2006-05-17 11:39:17 flefoll: can you please attach a new patch? Thanks. By: Frederic LE FOLL (flefoll) 2006-05-18 06:25:39 The new patch_alarm_rev27812.txt is a patch upon trunk/channels/chan_zap.c revision 27812 (current head). It removes the misplaced manager_events (previous patch failure ?) and inserts them at the proper place (i.e. one line above). The file is the output of a "diff -u Rev27812/chan_zap.c patch/chan_zap.c". By: Russell Bryant (russell) 2006-05-19 09:51:58 fixed in the trunk in revision 28592, thanks! By: Frederic LE FOLL (flefoll) 2006-05-31 02:27:32 I see that the first patch, which did not apply correctly (still no idea why) was applied not only to asterisk/trunk/channels/chan_zap.c rev 25124, but also to asterisk/branches/1.2/channels/chan_zap.c rev 25123. Unluckily, the second patch that fixed the situation was applied to trunk only (rev 28592), not to branches/1.2 ! Consequently, asterisk 1.2.8 has the additional manager_event() calls at the wrong place. It makes one of them dead code (after a 'break') while the other offset is a cosmetic problem only (after the /* fall thru intentionally */ comment). According to my test, patch file patch_alarm_rev27812.txt is applicable to chan_zap.c rev 29969 to fixup this error : [...]$ patch asterisk-1.2.8/channels/chan_zap.c patch_alarm_rev27812.txt patching file asterisk-1.2.8/channels/chan_zap.c Hunk #1 succeeded at 6338 (offset -157 lines). ... and manager_event() calls are back at the right place. By: Russell Bryant (russell) 2006-05-31 15:27:15 fixed in the 1.2 branch in revision 31127, thanks again!