<p> Attention is currently required from: Joshua Colp. </p>
<p><a href="https://gerrit.asterisk.org/c/asterisk/+/17330">View Change</a></p><p>19 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_aeap.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/94a495d6_149cb1cd">Patch Set #3, Line 33:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">These should have \since added, along with other header files</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The file itself or each function definition? Also, I thought we quit adding \since to things.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/932c0958_e77ede17">Patch Set #3, Line 218:</a> <code style="font-family:monospace,monospace"> */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Should these connect calls include timeouts?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/7cd8d8e3_fb31bc7e">Patch Set #3, Line 252:</a> <code style="font-family:monospace,monospace">int ast_aeap_disconnect(struct ast_aeap *aeap);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does this block until disconnect occurs?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe. Depends on how the underlying transport handles it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/69857ef3_9a48294a">Patch Set #3, Line 322:</a> <code style="font-family:monospace,monospace">        ast_aeap_on_timeout on_timeout;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is this required or optional?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Optional. I'll add that it is such.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/f746af2f_186e5e74">Patch Set #3, Line 326:</a> <code style="font-family:monospace,monospace">  void *obj;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Specify how lifetime of this is managed, who is responsible</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/42fce5c4_a3c3363b">Patch Set #3, Line 342:</a> <code style="font-family:monospace,monospace"> * \param params Additional parameters to consider when sending (optional)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Specify whether this is to be allocated or not.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File include/asterisk/res_aeap_message.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/bba0aa46_7db45dcd">Patch Set #3, Line 36:</a> <code style="font-family:monospace,monospace">     /*! The size of this type */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This needs further explanation for what this means and what it does.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/9c81f1d6_d033c9cb">Patch Set #3, Line 201:</a> <code style="font-family:monospace,monospace"> * \brief Create an Asterisk external application error message from a given message.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This doesn't seem to be creating things from a message, and is it actually for creating just an erro […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Aah yeah I think the original intent was to do that maybe, but then I changed it. Docs updated.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/24a428ef_bb9a868e">Patch Set #3, Line 219:</a> <code style="font-family:monospace,monospace"> * \brief Create an Asterisk external application request object, and generate an id for it</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This optionally generates an id.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/a9ff81eb_0b36b5c8">Patch Set #3, Line 298:</a> <code style="font-family:monospace,monospace">int ast_aeap_message_id_generate(struct ast_aeap_message *message);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Would it be useful for this to return the generated id, as part of this call?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure, but don't see a downside so I'll change it.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_aeap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/d39e2019_0e8b3853">Patch Set #3, Line 35:</a> <code style="font-family:monospace,monospace">                        <configObject name="server"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Ah, the joy of perception. Previously it was configuring the details for a remote server. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ah yeah heh I debated this a bit. I finally settled on changing to represent the local client config. At the time it seemed to make more sense to my brain since we're configuring the thing that is controlled by asterisk, which is the client. But can see it either way really. I can change back if ya want?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also, maybe was thinking we might want to create a local server type later.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_aeap/aeap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/ff926886_4d6f4ced">Patch Set #3, Line 57:</a> <code style="font-family:monospace,monospace">        ast_mutex_t lock;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Is there a need for an additional lock alongside the ao2 one?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nope, was previously not ao2. No longer needed since converting allocation type. I'll fix.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/13235292_26fdbefc">Patch Set #3, Line 125:</a> <code style="font-family:monospace,monospace"> data = ao2_alloc(sizeof(*data) + strlen(id) + 1, cleanup);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does this need to be allocated with a lock?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Don't think so. Changed.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/734302bb_3443dde1">Patch Set #3, Line 174:</a> <code style="font-family:monospace,monospace">    obj = data->obj;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What is the lifetime guarantee of the returned obj? Is there any? Should special care be taken by th […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Updated the public docs for this function. Hopefully it's more clear. I'll add a small comment here too.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_aeap/message.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/8e776622_3365b459">Patch Set #3, Line 47:</a> <code style="font-family:monospace,monospace">      msg = ao2_alloc(type->type_size, message_destructor);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does this need to be allocated with a lock?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_aeap/transaction.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/90127d5d_f8aa40e4">Patch Set #3, Line 81:</a> <code style="font-family:monospace,monospace">void aeap_transaction_end(struct aeap_transaction *tsx, int result);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What does result mean? What are the possible values for result? Applies to the next function too.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_aeap/transport.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/8a2cb97e_ff2966d4">Patch Set #3, Line 181:</a> <code style="font-family:monospace,monospace"> * \returns Total number of bytes read, or less than zero on error</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Document whether this is blocking/non-blocking.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File res/res_speech_aeap.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/4a0ebb59_76147f3b">Patch Set #3, Line 495:</a> <code style="font-family:monospace,monospace">     speech_engine_create_and_register(ast_sorcery_object_get_id(obj), NULL,</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">codec_names is always going to be NULL</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure what ya mean here? NULL is passed in for it, so yes it will always be NULL here.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Perhaps you mean it's not used anywhere else? If so that field is used a bit below for unit test if the TEST_FRAMEWORK is enabled.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/17330/comment/317f7462_178c71bd">Patch Set #3, Line 530:</a> <code style="font-family:monospace,monospace">      if (ast_aeap_client_config_observer_add(&speech_observer)) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add a comment explaining what this is doing</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/17330">change 17330</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/17330"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Iaa4b259f84aa63501e5fd2a6fb107f900b4d4ed2 </div>
<div style="display:none"> Gerrit-Change-Number: 17330 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 12 Jan 2022 23:04:39 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>