[Asterisk-code-review] app_sendmf: New SendMF application (asterisk[master])

[George Joseph]

Attention is currently required from: N A, Joshua Colp, Sarah Autumn.
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/16080 )

Change subject: app_sendmf: New SendMF application
......................................................................


Patch Set 4: Code-Review-1

(6 comments)

File apps/app_sendmf.c:

https://gerrit.asterisk.org/c/asterisk/+/16080/comment/bd0faa12_3a8d80f4 
PS4, Line 91: 			<parameter name="Receive" required="false">
            : 				<para>Emulate receiving MF on this channel instead of sending it out.</para>
            : 			</parameter>
You didn't implement this (which is fine) but don't you want to add the interval, KP and ST durations?


https://gerrit.asterisk.org/c/asterisk/+/16080/comment/ee6295c9_eeb059f8 
PS4, Line 158: static int senddigit_mf(struct ast_channel *chan, char digit, unsigned int duration, unsigned int durationkp, unsigned int durationst)
             : {
             : 	if (duration < DEFAULT_EMULATE_MF_DURATION) {
             : 		duration = DEFAULT_EMULATE_MF_DURATION;
             : 	}
             : 	if (ast_channel_tech(chan)->send_digit_begin) {
             : 		if (digit == '*')
             : 			duration = durationkp;
             : 		else if (digit == '#' || digit == 'A' || digit == 'B' || digit == 'C')
             : 			duration = durationst;
             : 		senddigit_mf_begin(chan, digit);
             : 		ast_safe_sleep(chan, duration);
             : 	}
             : 	return senddigit_mf_end(chan);
             : }
             : 
             : static int senddigit_external_mf(struct ast_channel *chan, char digit, unsigned int duration,
             : 	unsigned int durationkp, unsigned int durationst)
             : {
             : 	if (duration < DEFAULT_EMULATE_MF_DURATION) {
             : 		duration = DEFAULT_EMULATE_MF_DURATION;
             : 	}
             : 	if (ast_channel_tech(chan)->send_digit_begin) {
             : 		senddigit_mf_begin(chan, digit);
             : 		if (digit == '*')
             : 			duration = durationkp;
             : 		else if (digit == '#' || digit == 'A' || digit == 'B' || digit == 'C')
             : 			duration = durationst;
             : 		usleep(duration * 1000);
             : 	}
             : 	return senddigit_mf_end(chan);
             : }
             : 
Since these are private (unlike their DTMF counterparts) and the only difference between them is the type of sleep, just collapse these into 1 function and pass in the "is_external" flag to indicate which sleep function to call.  See comment below.


https://gerrit.asterisk.org/c/asterisk/+/16080/comment/8cbd5685_edd3c1ee 
PS4, Line 191: static int external_sleep(struct ast_channel *chan, int ms)
             : {
             : 	usleep(ms * 1000);
             : 	return 0;
             : }
If you move this above senddigit and add the is_internal flag, you can collapse the two senddigit functions and eliminate the need for the function pointers in mf_stream.

static int mysleep(struct ast_channel *chan, int ms, int is_external)
{
	if (is_external) {
		usleep(ms * 1000);
	} else {
		ast_safe_sleep(chan, ms);
	}
	return 0;
}


https://gerrit.asterisk.org/c/asterisk/+/16080/comment/462b97ba_bff2682d 
PS4, Line 204: 	int (*my_senddigit)(struct ast_channel *chan, char digit, unsigned int duration, unsigned int durationkp, unsigned int durationst);
You don't need the function pointer stuff if you do the above.


https://gerrit.asterisk.org/c/asterisk/+/16080/comment/7815ff47_8e72f044 
PS4, Line 257: static int mf_stream_digits(struct ast_channel *chan, struct ast_channel *peer, const char *digits, int between, unsigned int duration, unsigned int durationkp, unsigned int durationst)
             : {
             : 	int res;
             : 
             : 	if (peer && ast_autoservice_start(peer)) {
             : 		return -1;
             : 	}
             : 	res = mf_stream(chan, digits, between, duration, durationkp, durationst, 0);
             : 	if (peer && ast_autoservice_stop(peer)) {
             : 		res = -1;
             : 	}
             : 
             : 	return res;
             : }
Again, since this is private, just collapse with mf_stream.


https://gerrit.asterisk.org/c/asterisk/+/16080/comment/17543e11_90ddcaa0 
PS4, Line 338: 	const char *duration = astman_get_header(m, "Duration");
Don't you want to add the interval and other durations?  I know SendDTMF didn't add interval to the AMI app but that doesn't mean you can't.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/16080
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I5d89afdbccee3f86cc702ed96d882f3d351327a4
Gerrit-Change-Number: 16080
Gerrit-PatchSet: 4
Gerrit-Owner: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-CC: Sarah Autumn <sarah at endlesstemple.org>
Gerrit-Attention: N A <mail at interlinked.x10host.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Sarah Autumn <sarah at endlesstemple.org>
Gerrit-Comment-Date: Wed, 04 Aug 2021 15:30:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20210804/8a7f5c5e/attachment.html>