[Asterisk-code-review] res pjsip: Add DTMF INFO Failback mode (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/5842 )

Change subject: res_pjsip:  Add DTMF INFO Failback mode
......................................................................


Patch Set 1: Code-Review-1

(6 comments)

In order to be included in 13 and 14 this also needs a test written as it is a new feature, otherwise it can only go into master.

https://gerrit.asterisk.org/#/c/5842/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/5842/1//COMMIT_MSG@12
PS1, Line 12: 
This needs to have a JIRA issue created for it so it gets picked up in a release.


https://gerrit.asterisk.org/#/c/5842/1/channels/chan_pjsip.c
File channels/chan_pjsip.c:

https://gerrit.asterisk.org/#/c/5842/1/channels/chan_pjsip.c@1831
PS1, Line 1831: 			ast_log(LOG_DTMF, "digit_end for AST_SIP_DTMF_AUTO_INFO  -->  sending DTMF via RFC_4733 \n");
Generally DTMF log messages are for info about passing DTMF, not quite for this. They could still exist but would need to also include the channel name to be more useful.


https://gerrit.asterisk.org/#/c/5842/1/channels/chan_pjsip.c@1835
PS1, Line 1835: 		// else  continue for  DTMF_INFO
This should be /* */ and could be better explained.

/* Fall through to DTMF_INFO so that the INFO DTMF is sent */


https://gerrit.asterisk.org/#/c/5842/1/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/5842/1/include/asterisk/res_pjsip.h@366
PS1, Line 366: 	AST_SIP_DTMF_AUTO_INFO,
This needs to be documented like the others


https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip/pjsip_configuration.c
File res/res_pjsip/pjsip_configuration.c:

https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip/pjsip_configuration.c@376
PS1, Line 376: 	} else if (!strcasecmp(var->value, "auto_info")) {
An alembic migration script needs to be created to add this as a valid option to the enum.

Information about how to install alembic:
https://wiki.asterisk.org/wiki/display/AST/Managing+Realtime+Databases+with+Alembic

Information about creating a migration script:
http://alembic.zzzcomputing.com/en/latest/tutorial.html#create-a-migration-script

And 31cd4f4891ec_add_auto_dtmf_mode.py can be used as a base.

Also these are stored in contrib/ast-db-manage/config/versions


https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip_sdp_rtp.c
File res/res_pjsip_sdp_rtp.c:

https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip_sdp_rtp.c@342
PS1, Line 342: 			ast_log(LOG_DTMF, "Setting dtmf mode to RFC2833\n");
             : 		}
             : 		else {
             : 			ast_rtp_instance_dtmf_mode_set(session_media->rtp, AST_RTP_DTMF_MODE_NONE);
             : 			ast_log(LOG_DTMF, "Setting dtmf mode to info\n");
My comment also applies to these two log messages.



-- 
To view, visit https://gerrit.asterisk.org/5842
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Id185b11e84afd9191a2f269e8443019047765e91
Gerrit-Change-Number: 5842
Gerrit-PatchSet: 1
Gerrit-Owner: Torrey Searle <tsearle at gmail.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Fri, 16 Jun 2017 12:22:43 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170616/6148782a/attachment.html>