<p>Joshua Colp <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/5842">View Change</a></p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p>(6 comments)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/5842/1//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5842/1//COMMIT_MSG@12">Patch Set #1, Line 12:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs to have a JIRA issue created for it so it gets picked up in a release.</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5842/1/channels/chan_pjsip.c">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5842/1/channels/chan_pjsip.c@1831">Patch Set #1, Line 1831:</a> <code style="font-family:monospace,monospace"> ast_log(LOG_DTMF, "digit_end for AST_SIP_DTMF_AUTO_INFO --> sending DTMF via RFC_4733 \n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5842/1/channels/chan_pjsip.c@1835">Patch Set #1, Line 1835:</a> <code style="font-family:monospace,monospace"> // else continue for DTMF_INFO</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be /* */ and could be better explained.</p><p style="white-space: pre-wrap; word-wrap: break-word;">/* Fall through to DTMF_INFO so that the INFO DTMF is sent */</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5842/1/include/asterisk/res_pjsip.h">File include/asterisk/res_pjsip.h:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5842/1/include/asterisk/res_pjsip.h@366">Patch Set #1, Line 366:</a> <code style="font-family:monospace,monospace"> AST_SIP_DTMF_AUTO_INFO,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This needs to be documented like the others</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip/pjsip_configuration.c">File res/res_pjsip/pjsip_configuration.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip/pjsip_configuration.c@376">Patch Set #1, Line 376:</a> <code style="font-family:monospace,monospace"> } else if (!strcasecmp(var->value, "auto_info")) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">An alembic migration script needs to be created to add this as a valid option to the enum.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Information about how to install alembic:<br>https://wiki.asterisk.org/wiki/display/AST/Managing+Realtime+Databases+with+Alembic</p><p style="white-space: pre-wrap; word-wrap: break-word;">Information about creating a migration script:<br>http://alembic.zzzcomputing.com/en/latest/tutorial.html#create-a-migration-script</p><p style="white-space: pre-wrap; word-wrap: break-word;">And 31cd4f4891ec_add_auto_dtmf_mode.py can be used as a base.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also these are stored in contrib/ast-db-manage/config/versions</p></li></ul></li><li><p><a href="https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip_sdp_rtp.c">File res/res_pjsip_sdp_rtp.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/5842/1/res/res_pjsip_sdp_rtp.c@342">Patch Set #1, Line 342:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ast_log(LOG_DTMF, "Setting dtmf mode to RFC2833\n");<br> }<br> else {<br> ast_rtp_instance_dtmf_mode_set(session_media->rtp, AST_RTP_DTMF_MODE_NONE);<br> ast_log(LOG_DTMF, "Setting dtmf mode to info\n");<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">My comment also applies to these two log messages.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/5842">change 5842</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/5842"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Id185b11e84afd9191a2f269e8443019047765e91 </div>
<div style="display:none"> Gerrit-Change-Number: 5842 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Torrey Searle <tsearle@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 16 Jun 2017 12:22:43 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>