[Asterisk-code-review] res_aeap & res_speech_aeap: Add Asterisk External Application Protocol (asterisk[master])

Wed Jan 12 17:04:39 CST 2022

Attention is currently required from: Joshua Colp.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17330 )

Change subject: res_aeap & res_speech_aeap: Add Asterisk External Application Protocol
......................................................................

Patch Set 3:

(19 comments)

File include/asterisk/res_aeap.h:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/94a495d6_149cb1cd 
PS3, Line 33: 
> These should have \since added, along with other header files
The file itself or each function definition? Also, I thought we quit adding \since to things.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/932c0958_e77ede17 
PS3, Line 218:  */
> Should these connect calls include timeouts?
Done

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/7cd8d8e3_fb31bc7e 
PS3, Line 252: int ast_aeap_disconnect(struct ast_aeap *aeap);
> Does this block until disconnect occurs?
Maybe. Depends on how the underlying transport handles it.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/69857ef3_9a48294a 
PS3, Line 322: 	ast_aeap_on_timeout on_timeout;
> Is this required or optional?
Optional. I'll add that it is such.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/f746af2f_186e5e74 
PS3, Line 326: 	void *obj;
> Specify how lifetime of this is managed, who is responsible
Done

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/42fce5c4_a3c3363b 
PS3, Line 342:  * \param params Additional parameters to consider when sending (optional)
> Specify whether this is to be allocated or not.
Done

File include/asterisk/res_aeap_message.h:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/bba0aa46_7db45dcd 
PS3, Line 36: 	/*! The size of this type */
> This needs further explanation for what this means and what it does.
Done

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/9c81f1d6_d033c9cb 
PS3, Line 201:  * \brief Create an Asterisk external application error message from a given message.
> This doesn't seem to be creating things from a message, and is it actually for creating just an erro […]
Aah yeah I think the original intent was to do that maybe, but then I changed it. Docs updated.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/24a428ef_bb9a868e 
PS3, Line 219:  * \brief Create an Asterisk external application request object, and generate an id for it
> This optionally generates an id.
Done

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/a9ff81eb_0b36b5c8 
PS3, Line 298: int ast_aeap_message_id_generate(struct ast_aeap_message *message);
> Would it be useful for this to return the generated id, as part of this call?
Not sure, but don't see a downside so I'll change it.

File res/res_aeap.c:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/d39e2019_0e8b3853 
PS3, Line 35: 			<configObject name="server">
> Ah, the joy of perception. Previously it was configuring the details for a remote server. […]
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?

Also, maybe was thinking we might want to create a local server type later.

File res/res_aeap/aeap.c:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/ff926886_4d6f4ced 
PS3, Line 57: 	ast_mutex_t lock;
> Is there a need for an additional lock alongside the ao2 one?
Nope, was previously not ao2. No longer needed since converting allocation type. I'll fix.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/13235292_26fdbefc 
PS3, Line 125: 	data = ao2_alloc(sizeof(*data) + strlen(id) + 1, cleanup);
> Does this need to be allocated with a lock?
Don't think so. Changed.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/734302bb_3443dde1 
PS3, Line 174: 	obj = data->obj;
> What is the lifetime guarantee of the returned obj? Is there any? Should special care be taken by th […]
Updated the public docs for this function. Hopefully it's more clear. I'll add a small comment here too.

File res/res_aeap/message.c:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/8e776622_3365b459 
PS3, Line 47: 	msg = ao2_alloc(type->type_size, message_destructor);
> Does this need to be allocated with a lock?
Done

File res/res_aeap/transaction.h:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/90127d5d_f8aa40e4 
PS3, Line 81: void aeap_transaction_end(struct aeap_transaction *tsx, int result);
> What does result mean? What are the possible values for result? Applies to the next function too.
Done

File res/res_aeap/transport.h:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/8a2cb97e_ff2966d4 
PS3, Line 181:  * \returns Total number of bytes read, or less than zero on error
> Document whether this is blocking/non-blocking.
Done

File res/res_speech_aeap.c:

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/4a0ebb59_76147f3b 
PS3, Line 495: 	speech_engine_create_and_register(ast_sorcery_object_get_id(obj), NULL,
> codec_names is always going to be NULL
Not sure what ya mean here? NULL is passed in for it, so yes it will always be NULL here.

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.

https://gerrit.asterisk.org/c/asterisk/+/17330/comment/317f7462_178c71bd 
PS3, Line 530: 	if (ast_aeap_client_config_observer_add(&speech_observer)) {
> Add a comment explaining what this is doing
Done

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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Iaa4b259f84aa63501e5fd2a6fb107f900b4d4ed2
Gerrit-Change-Number: 17330
Gerrit-PatchSet: 3
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Comment-Date: Wed, 12 Jan 2022 23:04:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220112/afcef09c/attachment-0001.html>