<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Hi Aleksander,</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">I tested this patch and it looks good to me (I specially like the second one). There is just a little mistake I made in my part of the patch. In the "status changed" log I inverted current status with the previous one.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default"><div class="gmail_default"><font face="arial, helvetica, sans-serif">---</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> plugins/telit/mm-broadband-modem-telit.c | 6 +++---</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> 1 file changed, 3 insertions(+), 3 deletions(-)</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"><br></font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">index 7c93c0d..e78a195 100644</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">--- a/plugins/telit/mm-broadband-modem-telit.c</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+++ b/plugins/telit/mm-broadband-modem-telit.c</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">@@ -125,9 +125,9 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> self->priv->qss_status = cur_qss_status;</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> </font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> if (cur_qss_status != prev_qss_status)</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">- mm_dbg ("QSS: status changed '%s -> %s",</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">- mm_telit_qss_status_get_string (cur_qss_status),</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">- mm_telit_qss_status_get_string (prev_qss_status));</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+ mm_dbg ("QSS: status changed %s -> %s",</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+ mm_telit_qss_status_get_string (prev_qss_status),</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">+ mm_telit_qss_status_get_string (cur_qss_status));</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> </font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> if ((prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status != QSS_STATUS_SIM_REMOVED) ||</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif"> (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status == QSS_STATUS_SIM_REMOVED)) {</font></div><div class="gmail_default"><font face="arial, helvetica, sans-serif">--</font></div></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Can I submit a little patch just for that or do you prefer doing otherwise?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 2 June 2017 at 13:10, Aleksander Morgado <span dir="ltr"><<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">When the async method starts we store already the primary and the<br>
optional secondary port objects in the method context, keeping a full<br>
reference for each.<br>
<br>
When we parse the response for the enabling command, we just reuse the<br>
stored port objects, instead of re-querying the modem to get them, and<br>
this makes sure that the port objects where we want to set the<br>
unsolicited message handlers are still valid. If the ports are gone<br>
in the middle of the enabling operation, the handlers will be set<br>
without errors, even if the ports may likely get completely disposed<br>
when this async method context is disposed.<br>
<br>
Additionally, we make sure that we return an error also for the case<br>
where there is no secondary port and the enabling on the primary port<br>
failed.<br>
<br>
This patch also fixes the use of the at_command_full_finish() method to<br>
complete the at_command_full() async operation, to keep consistency.<br>
---<br>
plugins/telit/mm-broadband-<wbr>modem-telit.c | 102 +++++++++++++-----------------<wbr>-<br>
1 file changed, 41 insertions(+), 61 deletions(-)<br>
<br>
diff --git a/plugins/telit/mm-broadband-<wbr>modem-telit.c b/plugins/telit/mm-broadband-<wbr>modem-telit.c<br>
index 2c1c5420..e609bab1 100644<br>
--- a/plugins/telit/mm-broadband-<wbr>modem-telit.c<br>
+++ b/plugins/telit/mm-broadband-<wbr>modem-telit.c<br>
@@ -103,6 +103,8 @@ typedef enum {<br>
<br>
typedef struct {<br>
QssSetupStep step;<br>
+ MMPortSerialAt *primary;<br>
+ MMPortSerialAt *secondary;<br>
GError *primary_error;<br>
GError *secondary_error;<br>
} QssSetupContext;<br>
@@ -138,6 +140,8 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,<br>
static void<br>
qss_setup_context_free (QssSetupContext *ctx)<br>
{<br>
+ g_clear_object (&(ctx->primary));<br>
+ g_clear_object (&(ctx->secondary));<br>
g_clear_error (&(ctx->primary_error));<br>
g_clear_error (&(ctx->secondary_error));<br>
g_slice_free (QssSetupContext, ctx);<br>
@@ -156,60 +160,37 @@ telit_qss_enable_ready (MMBaseModem *modem,<br>
GAsyncResult *res,<br>
GTask *task)<br>
{<br>
- GError *error = NULL;<br>
MMBroadbandModemTelit *self;<br>
QssSetupContext *ctx;<br>
- MMPortSerialAt *primary;<br>
- MMPortSerialAt *secondary;<br>
+ MMPortSerialAt *port;<br>
+ GError **error;<br>
GRegex *pattern;<br>
<br>
self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));<br>
ctx = g_task_get_task_data (task);<br>
<br>
- mm_base_modem_at_command_<wbr>finish (modem, res, &error);<br>
- if (error) {<br>
- if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT) {<br>
- mm_warn ("QSS: error enabling unsolicited on primary port: %s", error->message);<br>
- ctx->primary_error = error;<br>
- } else if (ctx->step == QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT) {<br>
- mm_warn ("QSS: error enabling unsolicited on secondary port: %s", error->message);<br>
- ctx->secondary_error = error;<br>
- } else {<br>
- g_assert_not_reached ();<br>
- }<br>
+ if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT) {<br>
+ port = ctx->primary;<br>
+ error = &ctx->primary_error;<br>
+ } else if (ctx->step == QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT) {<br>
+ port = ctx->secondary;<br>
+ error = &ctx->secondary_error;<br>
+ } else<br>
+ g_assert_not_reached ();<br>
+<br>
+ if (!mm_base_modem_at_command_<wbr>full_finish (modem, res, error)) {<br>
+ mm_warn ("QSS: error enabling unsolicited on port %s: %s", mm_port_get_device (MM_PORT (port)), (*error)->message);<br>
goto next_step;<br>
}<br>
<br>
pattern = g_regex_new ("#QSS:\\s*([0-3])\\r\\n", G_REGEX_RAW, 0, NULL);<br>
g_assert (pattern);<br>
-<br>
- if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT) {<br>
- primary = mm_base_modem_peek_port_<wbr>primary (MM_BASE_MODEM (self));<br>
- mm_port_serial_at_add_<wbr>unsolicited_msg_handler (<br>
- primary,<br>
- pattern,<br>
- (<wbr>MMPortSerialAtUnsolicitedMsgFn<wbr>)telit_qss_unsolicited_<wbr>handler,<br>
- self,<br>
- NULL);<br>
- }<br>
-<br>
- if (ctx->step == QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT) {<br>
- secondary = mm_base_modem_peek_port_<wbr>secondary (MM_BASE_MODEM (self));<br>
- if (!secondary) {<br>
- mm_port_serial_at_add_<wbr>unsolicited_msg_handler (<br>
- secondary,<br>
- pattern,<br>
- (<wbr>MMPortSerialAtUnsolicitedMsgFn<wbr>)telit_qss_unsolicited_<wbr>handler,<br>
- self,<br>
- NULL);<br>
- } else {<br>
- mm_warn ("QSS could not set handler on secondary port: no secondary port found.");<br>
- ctx->secondary_error = g_error_new (MM_CORE_ERROR,<br>
- MM_CORE_ERROR_FAILED,<br>
- "QSS could not set handler hat secondary port: no secondary port found.");<br>
- }<br>
- }<br>
-<br>
+ mm_port_serial_at_add_<wbr>unsolicited_msg_handler (<br>
+ port,<br>
+ pattern,<br>
+ (<wbr>MMPortSerialAtUnsolicitedMsgFn<wbr>)telit_qss_unsolicited_<wbr>handler,<br>
+ self,<br>
+ NULL);<br>
g_regex_unref (pattern);<br>
<br>
next_step:<br>
@@ -276,7 +257,7 @@ qss_setup_step (GTask *task)<br>
return;<br>
case QSS_SETUP_STEP_ENABLE_PRIMARY_<wbr>PORT:<br>
mm_base_modem_at_command_full (MM_BASE_MODEM (self),<br>
- mm_base_modem_peek_port_<wbr>primary (MM_BASE_MODEM (self)),<br>
+ ctx->primary,<br>
"#QSS=1",<br>
3,<br>
FALSE,<br>
@@ -285,35 +266,32 @@ qss_setup_step (GTask *task)<br>
(GAsyncReadyCallback) telit_qss_enable_ready,<br>
task);<br>
return;<br>
- case QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT: {<br>
- MMPortSerialAt *port;<br>
-<br>
- port = mm_base_modem_peek_port_<wbr>secondary (MM_BASE_MODEM (self));<br>
- if (port) {<br>
+ case QSS_SETUP_STEP_ENABLE_<wbr>SECONDARY_PORT:<br>
+ if (ctx->secondary) {<br>
mm_base_modem_at_command_full (MM_BASE_MODEM (self),<br>
- port,<br>
- "#QSS=1",<br>
- 3,<br>
- FALSE,<br>
- FALSE, /* raw */<br>
- NULL, /* cancellable */<br>
- (GAsyncReadyCallback) telit_qss_enable_ready,<br>
- task);<br>
+ ctx->secondary,<br>
+ "#QSS=1",<br>
+ 3,<br>
+ FALSE,<br>
+ FALSE, /* raw */<br>
+ NULL, /* cancellable */<br>
+ (GAsyncReadyCallback) telit_qss_enable_ready,<br>
+ task);<br>
return;<br>
}<br>
-<br>
/* Fall back to next step */<br>
ctx->step++;<br>
- }<br>
case QSS_SETUP_STEP_LAST:<br>
- if (ctx->primary_error && ctx->secondary_error) {<br>
+ /* If all enabling actions failed (either both, or only primary if<br>
+ * there is no secondary), then we return an error */<br>
+ if (ctx->primary_error &&<br>
+ (ctx->secondary_error || !ctx->secondary))<br>
g_task_return_new_error (task,<br>
MM_CORE_ERROR,<br>
MM_CORE_ERROR_FAILED,<br>
"QSS: couldn't enable unsolicited");<br>
- } else {<br>
+ else<br>
g_task_return_boolean (task, TRUE);<br>
- }<br>
g_object_unref (task);<br>
break;<br>
}<br>
@@ -331,6 +309,8 @@ modem_setup_sim_hot_swap (MMIfaceModem *self,<br>
<br>
ctx = g_slice_new0 (QssSetupContext);<br>
ctx->step = QSS_SETUP_STEP_FIRST;<br>
+ ctx->primary = mm_base_modem_get_port_primary (MM_BASE_MODEM (self));<br>
+ ctx->secondary = mm_base_modem_get_port_<wbr>secondary (MM_BASE_MODEM (self));<br>
<br>
g_task_set_task_data (task, ctx, (GDestroyNotify) qss_setup_context_free);<br>
qss_setup_step (task);<br>
<span class="HOEnZb"><font color="#888888">--<br>
2.13.0<br>
<br>
</font></span></blockquote></div><br></div>