[Home]

Summary:ASTERISK-20157: Code Cleanup in app_alarmreceiver
Reporter:Pedro Kiefer (pedrokiefer)Labels:
Date Opened:2012-07-20 12:23:46Date Closed:2012-09-06 08:06:06
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Applications/app_alarmreceiver
Versions:SVN Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 0001-Mainly-code-cleanup.patch
( 1) 0002-Clean-up-load_config-making-it-more-readable.patch
( 2) 0003-Extract-constant-number-to-defines.patch
( 3) 0004-Clean-up-code.patch
( 4) 0005-Remove-checksum-function-out-of-the-receive-ContactI.patch
Description:Some cleanups to the alarm receiver code, making it more readable and manageable.
Comments:By: Pedro Kiefer (pedrokiefer) 2012-07-20 12:24:31.064-0500

Code cleanup
By: Rusty Newton (rnewton) 2012-07-23 17:13:47.251-0500

Can you please remove and re-attach patches 0003-0005 and properly select the code contribution radio button so that your license will be associated?
By: Rusty Newton (rnewton) 2012-07-23 18:14:54.086-0500

Pedro, we marked them correctly now. Let us know if there is any error.
By: Pedro Kiefer (pedrokiefer) 2012-07-24 10:13:30.175-0500

Rusty,
That's it, I guess. Just review the code, to see if it is okey to merge on asterisk code base. If there is any problem with the code, coding style, etc, just let me know, I can remake those patches.
By: Rusty Newton (rnewton) 2012-07-26 18:33:57.876-0500

app_alarmreceiver is under extended support, so you'll want to find another community developer to review. Try posting on http://lists.digium.com/mailman/listinfo/asterisk-dev
By: Pedro Kiefer (pedrokiefer) 2012-07-26 18:55:56.363-0500

Rusty,
I've sent the patches a while back to the list, and I was informed that I should post the patches here.

Anyway, I will ask the dev list to see if anyone is willing to review the patches.
By: Rusty Newton (rnewton) 2012-07-26 18:59:45.658-0500

Correct. Don't send the patches to the list.  

Ask on the list for someone to review and test your patches. Point them here..
By: Kaloyan Kovachev (knk) 2012-07-27 03:59:28.598-0500

I will take a look at the patches and other app_alarmreceiver issues (ASTERISK-20158, ASTERISK-19435, ASTERISK-18417, ASTERISK-16668 and ASTERISK-16694), but i am quite busy at the moment with SS7 changes, so it may take a while.
Meanwhile you may want to take a look at other issues too and post a review with your changes on reviewboard.asterisk.org, so together (because of the extended support) we can close those issues.
I also have some changes waiting for the app, like adding new formats and ALAW support in addition to ULAW, but they are new features for later.
By: Kaloyan Kovachev (knk) 2012-08-03 06:20:31.587-0500

The changes of the code here and in ASTERISK-20158 are fine, but there are some missing curly braces (see Codding Guidelance) and the functions comments should be in Doxygen format.
I would prefer that ademco_verify_checksum() only calculates the checksum and not touch the database, which should be done from caller. Also you may simply do "if (!(checksum = checksum % 15)) return 0;" and similary in the other issue "if (!strncmp(event + 7, ADEMCO_AUDIO_CALL_NEXT, 3)) {"
By: Pedro Kiefer (pedrokiefer) 2012-08-13 12:43:52.875-0500

New version of the patch, after review by Kaloyan Kovachev
By: Pedro Kiefer (pedrokiefer) 2012-08-13 12:44:07.986-0500

New version of the patch, after review by Kaloyan Kovachev
By: Pedro Kiefer (pedrokiefer) 2012-08-13 12:44:22.309-0500

New version of the patch, after review by Kaloyan Kovachev
By: Pedro Kiefer (pedrokiefer) 2012-08-13 12:44:49.319-0500

New version of the patch, after review by Kaloyan Kovachev
By: Jean-Philippe Lord (jplord) 2012-08-16 08:09:53.542-0500

Hello Pedro and Kaloyan

I'm available to test this when required.

Also, it would be great to take care of ASTERISK-16694 at the same time. Could be part of this code clean up as it removes the send_tone_burst and make_tone_burst to use asterisk ast_playtones_start and stop. Cleaned and works better for a few users.

It is related to 2 more (ASTERISK-16668 and ASTERISK-18417) which could be closed.

By: Pedro Kiefer (pedrokiefer) 2012-08-16 08:48:46.765-0500

Jean-Philippe,

I'll merge the patch from ASTERISK-16694 on my local branch, clean it up a little, and post a new patch on the issue. Your patch actually closes 4 bugs. ASTERISK-19435 is a issue on the function send_tone_burst, which is remove with your patch.

By: Kaloyan Kovachev (knk) 2012-08-16 09:09:44.073-0500

It will be great if some of the admins can create an account for you in reviewboard, so you can open a review, but if not, when you are ready i will combine the patches in a review or you can post a combined patch here and i will post it on your behalf.

The first beta of Asterisk 11 is out, but the patches are cleanup and bugfixes, so i hope it is not too late for them to be included.
By: Pedro Kiefer (pedrokiefer) 2012-08-16 09:25:47.950-0500

Kaloyan,

I can upload you a combined patch which would close most (maybe all) of the open issues on app_alarmreceiver [ ASTERISK-16668, ASTERISK-19435, ASTERISK-16694, ASTERISK-18417, ASTERISK-20157, ASTERISK-20224, ASTERISK-20158 ]

Can I attach the combined patch to this ticket?
By: Pedro Kiefer (pedrokiefer) 2012-08-16 10:03:04.642-0500

Squashed patch, includes all patches that I've sent lately.
By: Kaloyan Kovachev (knk) 2012-08-17 10:00:44.866-0500

I have posted the review (2075) and addressed the comments i had to speed up the things, but except compiling i can't test it at the moment.
Pedro Kiefer and/or Jean-Philippe Lord, please test if it works OK, so i can update the testing done in the review and mark it with 'ship it' or if you see something else i have missed and should be fixed.
By: Jean-Philippe Lord (jplord) 2012-08-17 10:20:28.495-0500

Hello Kaloyan,

Tested this morning with SVN trunk and all works OK here.
By: Jean-Philippe Lord (jplord) 2012-08-17 10:21:49.381-0500

I should specify, I have tested the Squashed patch from Aug 16th.
By: Kaloyan Kovachev (knk) 2012-08-17 13:21:40.420-0500

I have added some more cleanups. Please test the latest revision (v3) form the review - https://reviewboard.asterisk.org/r/2075/diff/raw/ just to confirm i haven't screwed it.
By: Jean-Philippe Lord (jplord) 2012-08-17 13:50:11.211-0500

Tested. Works here.
By: Pedro Kiefer (pedrokiefer) 2012-09-06 08:06:06.120-0500

Merged to SVN
By: Jean-Philippe Lord (jplord) 2012-09-11 13:43:46.422-0500

I have tested this morning with Asterisk SVN-trunk-r372808M and app_alarmreceiver does not work anymore. I get numerous error when panel dials in as the one below:
 == AlarmReceiver: Received Event 010
 == AlarmReceiver: Bad DTMF character ý  == AlarmReceiver: Nonzero checksum
 == AlarmReceiver: Received Event 100
 == AlarmReceiver: Bad DTMF character ý  == AlarmReceiver: Nonzero checksum
 == AlarmReceiver: Received Event 0C2
 == AlarmReceiver: Bad DTMF character ý  == AlarmReceiver: Nonzero checksum
 == AlarmReceiver: Received Event 299

By: Pedro Kiefer (pedrokiefer) 2012-09-11 13:59:18.486-0500

It seems you are only getting 3 characters at a time, that's weird. I'll take a look on that tomorrow.
By: Kaloyan Kovachev (knk) 2012-09-12 10:58:03.360-0500

please replace on line 182

while (i < length && i < sizeof(digit_string) - 1) {

with

while (i < length) {

By: Jean-Philippe Lord (jplord) 2012-09-14 16:10:21.959-0500

This works for me. Tested.
By: Jean-Philippe Lord (jplord) 2012-09-14 16:10:37.347-0500

Thanks a lot Kaloyan
By: Kaloyan Kovachev (knk) 2012-09-17 03:14:55.100-0500

This is the quick fix. The full fix will be in the new review, but if it will not go in this asterisk version, please open a new issue, so i can upload the fix there.
By: Jean-Philippe Lord (jplord) 2012-09-27 11:34:07.846-0500

Hello Kaloyan,

Issue ASTERISK-20484 created. Thanks for all your help.