xmm: wait for +XLSRSTOP URC before starting next session

It is important to gracefully stop the session before starting the
next session, making sure the engine is completely stopped. This can
be ensured by waiting for the +XLSRSTOP URC just after having received
the +XLSRSTOP command response.

We'll do an explicit wait for that URC with a 10s timeout, in order to
avoid waiting forever if the URC is never received. It will be assumed
that the engine is off if the 10s timeout happens, in the same way as
we were doing until now.

During the wait time for the URC, the operation task ownership is
shared among the response processor, the URC handler and the timeout
source. Once any of them decides to complete the task, the others will
automatically avoid attempting to complete the same task.

Based on a patch originally developed by:
Som_SP <somashekhar.puttagangaiah@intel.com>

1 files changed, 190 insertions, 45 deletions

diff --git a/plugins/xmm/mm-shared-xmm.c b/plugins/xmm/mm-shared-xmm.c
index 73004066..90f8867a 100644
--- a/plugins/xmm/mm-shared-xmm.c
+++ b/plugins/xmm/mm-shared-xmm.c
@@ -61,11 +61,15 @@ typedef struct {
     MMPortSerialAt        *gps_port;
     GRegex                *xlsrstop_regex;
     GRegex                *nmea_regex;
+
+    /* Asynchronous GPS engine stop task completion */
+    GTask *pending_gps_engine_stop_task;
 } Private;
 
 static void
 private_free (Private *priv)
 {
+    g_assert (!priv->pending_gps_engine_stop_task);
     g_clear_object (&priv->gps_port);
     if (priv->supported_modes)
         g_array_unref (priv->supported_modes);
@@ -970,7 +974,7 @@ mm_shared_xmm_location_load_capabilities (MMIfaceModemLocation *self,
 }
 
 /*****************************************************************************/
-/* GPS engine state selection */
+/* NMEA trace processing */
 
 static void
 nmea_received (MMPortSerialAt *port,
@@ -984,6 +988,23 @@ nmea_received (MMPortSerialAt *port,
     g_free (trace);
 }
 
+/*****************************************************************************/
+/* GPS engine state selection */
+
+#define GPS_ENGINE_STOP_TIMEOUT_SECS 10
+
+typedef struct {
+    GpsEngineState state;
+    guint          engine_stop_timeout_id;
+} GpsEngineSelectContext;
+
+static void
+gps_engine_select_context_free (GpsEngineSelectContext *ctx)
+{
+    g_assert (!ctx->engine_stop_timeout_id);
+    g_slice_free (GpsEngineSelectContext, ctx);
+}
+
 static gboolean
 gps_engine_state_select_finish (MMSharedXmm   *self,
                                 GAsyncResult  *res,
@@ -997,12 +1018,13 @@ xlcslsr_ready (MMBaseModem  *self,
                GAsyncResult *res,
                GTask        *task)
 {
-    GpsEngineState  state;
-    const gchar    *response;
-    GError         *error = NULL;
-    Private        *priv;
+    GpsEngineSelectContext *ctx;
+    const gchar            *response;
+    GError                 *error = NULL;
+    Private                *priv;
 
     priv = get_private (MM_SHARED_XMM (self));
+    ctx = g_task_get_task_data (task);
 
     response = mm_base_modem_at_command_full_finish (self, res, &error);
     if (!response) {
@@ -1012,7 +1034,7 @@ xlcslsr_ready (MMBaseModem  *self,
         return;
     }
 
-    state = GPOINTER_TO_UINT (g_task_get_task_data (task));
+    mm_obj_dbg (self, "GPS engine started");
 
     g_assert (priv->gps_port);
     mm_port_serial_at_add_unsolicited_msg_handler (priv->gps_port,
@@ -1020,7 +1042,7 @@ xlcslsr_ready (MMBaseModem  *self,
                                                    (MMPortSerialAtUnsolicitedMsgFn)nmea_received,
                                                    self,
                                                    NULL);
-    priv->gps_engine_state = state;
+    priv->gps_engine_state = ctx->state;
 
     g_task_return_boolean (task, TRUE);
     g_object_unref (task);
@@ -1029,17 +1051,17 @@ xlcslsr_ready (MMBaseModem  *self,
 static void
 gps_engine_start (GTask *task)
 {
-    GpsEngineState  state;
-    MMSharedXmm    *self;
-    Private        *priv;
-    GError         *error = NULL;
-    guint           transport_protocol = 0;
-    guint           pos_mode = 0;
-    gchar          *cmd;
+    GpsEngineSelectContext *ctx;
+    MMSharedXmm            *self;
+    Private                *priv;
+    GError                 *error = NULL;
+    guint                   transport_protocol = 0;
+    guint                   pos_mode = 0;
+    gchar                  *cmd;
 
-    self  = g_task_get_source_object (task);
-    priv  = get_private (self);
-    state = GPOINTER_TO_UINT (g_task_get_task_data (task));
+    self = g_task_get_source_object (task);
+    priv = get_private (self);
+    ctx  = g_task_get_task_data (task);
 
     g_assert (!priv->gps_port);
     priv->gps_port = shared_xmm_get_gps_control_port (self, &error);
@@ -1049,7 +1071,7 @@ gps_engine_start (GTask *task)
         return;
     }
 
-    switch (state) {
+    switch (ctx->state) {
         case GPS_ENGINE_STATE_STANDALONE:
             transport_protocol = 2;
             pos_mode = 3;
@@ -1068,6 +1090,8 @@ gps_engine_start (GTask *task)
             break;
     }
 
+    mm_obj_dbg (self, "starting GPS engine...");
+
     /*
      * AT+XLCSLSR
      *    transport_protocol:  2 (invalid) or 1 (supl)
@@ -1097,53 +1121,171 @@ gps_engine_start (GTask *task)
     g_free (cmd);
 }
 
-static void
-xlsrstop_ready (MMBaseModem  *self,
-                GAsyncResult *res,
-                GTask        *task)
+static GTask *
+recover_pending_gps_engine_stop_task (Private *priv)
 {
-    GpsEngineState  state;
-    GError         *error = NULL;
-    Private        *priv;
+    GTask                  *task;
+    GpsEngineSelectContext *ctx;
 
-    mm_base_modem_at_command_full_finish (self, res, &error);
+    /* We're taking over full ownership of the GTask at this point. */
+    if (!priv->pending_gps_engine_stop_task)
+        return NULL;
+    task = g_steal_pointer (&priv->pending_gps_engine_stop_task);
+    ctx = g_task_get_task_data (task);
 
-    priv = get_private (MM_SHARED_XMM (self));
-    state = GPOINTER_TO_UINT (g_task_get_task_data (task));
+    /* remove timeout */
+    if (ctx->engine_stop_timeout_id) {
+        g_source_remove (ctx->engine_stop_timeout_id);
+        ctx->engine_stop_timeout_id = 0;
+    }
+
+    /* disable urc handling */
+    mm_port_serial_at_add_unsolicited_msg_handler (
+        priv->gps_port,
+        priv->xlsrstop_regex,
+        NULL, NULL, NULL);
+
+    return task;
+}
+
+static void
+gps_engine_stopped (GTask *task)
+{
+    MMSharedXmm            *self;
+    GpsEngineSelectContext *ctx;
+    Private                *priv;
+
+    self = g_task_get_source_object (task);
+    priv = get_private (self);
+    ctx  = g_task_get_task_data (task);
 
     g_assert (priv->gps_port);
-    mm_port_serial_at_add_unsolicited_msg_handler (priv->gps_port, priv->nmea_regex, NULL, NULL, NULL);
+    mm_port_serial_at_add_unsolicited_msg_handler (
+        priv->gps_port,
+        priv->nmea_regex,
+        NULL, NULL, NULL);
     g_clear_object (&priv->gps_port);
     priv->gps_engine_state = GPS_ENGINE_STATE_OFF;
 
     /* If already reached requested state, we're done */
-    if (state == priv->gps_engine_state) {
+    if (ctx->state == priv->gps_engine_state) {
         /* If we had an error when requesting this specific state, report it */
-        if (error)
-            g_task_return_error (task, error);
-        else
-            g_task_return_boolean (task, TRUE);
+        g_task_return_boolean (task, TRUE);
         g_object_unref (task);
         return;
     }
 
-    /* Ignore errors if the stop operation was an intermediate one */
-    g_clear_error (&error);
-
     /* Otherwise, start with new state */
     gps_engine_start (task);
 }
 
+static gboolean
+xlsrstop_urc_timeout (MMSharedXmm *self)
+{
+    GTask   *task;
+    Private *priv;
+
+    priv = get_private (self);
+
+    task = recover_pending_gps_engine_stop_task (priv);
+    g_assert (task);
+
+    mm_obj_dbg (self, "timed out waiting for full GPS engine stop report, assuming stopped...");
+    gps_engine_stopped (task);
+
+    return G_SOURCE_REMOVE;
+}
+
+static void
+xlsrstop_urc_received (MMPortSerialAt *port,
+                       GMatchInfo     *info,
+                       MMSharedXmm    *self)
+{
+    GTask   *task;
+    Private *priv;
+
+    priv = get_private (self);
+
+    task = recover_pending_gps_engine_stop_task (priv);
+    g_assert (task);
+
+    mm_obj_dbg (self, "GPS engine fully stopped");
+    gps_engine_stopped (task);
+}
+
+static void
+xlsrstop_ready (MMBaseModem  *self,
+                GAsyncResult *res)
+{
+    g_autoptr(GError)  error = NULL;
+
+    if (!mm_base_modem_at_command_full_finish (self, res, &error)) {
+        Private *priv;
+        GTask   *task;
+
+        mm_obj_dbg (self, "GPS engine stop request failed: %s", error->message);
+
+        priv = get_private (MM_SHARED_XMM (self));
+        task = recover_pending_gps_engine_stop_task (priv);
+        if (task) {
+            g_task_return_error (task, g_steal_pointer (&error));
+            g_object_unref (task);
+        }
+        return;
+    }
+
+    mm_obj_dbg (self, "GPS engine stop request accepted");
+}
+
 static void
 gps_engine_stop (GTask *task)
 {
-    MMSharedXmm *self;
-    Private     *priv;
+    MMSharedXmm            *self;
+    Private                *priv;
+    GpsEngineSelectContext *ctx;
 
     self = g_task_get_source_object (task);
     priv = get_private (self);
+    ctx  = g_task_get_task_data (task);
 
     g_assert (priv->gps_port);
+
+    /* After a +XLSRSTOP command the modem will reply OK to report that the stop
+     * request has been received, but at this point the engine may still be on.
+     * We must wait for the additional +XLSRSTOP URC to tell us that the engine
+     * is really off before going on.
+     *
+     * We do this by setting up a temporary regex match for the URC during this
+     * operation, and also by configuring a timeout so that we don't wait forever
+     * for the URC.
+     *
+     * The initial +XLSRSTOP response will be ignored unless an error is being
+     * reported.
+     *
+     * The operation task is kept in private info because it will be shared by all
+     * the possible paths that may complete it (response, URC, timeout).
+     */
+    if (priv->pending_gps_engine_stop_task) {
+        g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_WRONG_STATE,
+                                 "An engine stop task is already ongoing");
+        g_object_unref (task);
+        return;
+    }
+    priv->pending_gps_engine_stop_task = task;
+
+    mm_obj_dbg (self, "launching GPS engine stop operation...");
+
+    ctx->engine_stop_timeout_id = g_timeout_add_seconds (GPS_ENGINE_STOP_TIMEOUT_SECS,
+                                                         (GSourceFunc) xlsrstop_urc_timeout,
+                                                         self);
+
+    mm_port_serial_at_add_unsolicited_msg_handler (
+        priv->gps_port,
+        priv->xlsrstop_regex,
+        (MMPortSerialAtUnsolicitedMsgFn)xlsrstop_urc_received,
+        self,
+        NULL);
+
     mm_base_modem_at_command_full (MM_BASE_MODEM (self),
                                    priv->gps_port,
                                    "+XLSRSTOP",
@@ -1152,7 +1294,7 @@ gps_engine_stop (GTask *task)
                                    FALSE, /* raw */
                                    NULL, /* cancellable */
                                    (GAsyncReadyCallback)xlsrstop_ready,
-                                   task);
+                                   NULL);
 }
 
 static void
@@ -1161,14 +1303,17 @@ gps_engine_state_select (MMSharedXmm         *self,
                          GAsyncReadyCallback  callback,
                          gpointer             user_data)
 {
-    GTask   *task;
-    Private *priv;
-
-    task = g_task_new (self, NULL, callback, user_data);
-    g_task_set_task_data (task, GUINT_TO_POINTER (state), NULL);
+    GpsEngineSelectContext *ctx;
+    GTask                  *task;
+    Private                *priv;
 
     priv = get_private (self);
 
+    task = g_task_new (self, NULL, callback, user_data);
+    ctx = g_slice_new0 (GpsEngineSelectContext);
+    ctx->state = state;
+    g_task_set_task_data (task, ctx, (GDestroyNotify)gps_engine_select_context_free);
+
     /* If already in the requested state, we're done */
     if (state == priv->gps_engine_state) {
         g_task_return_boolean (task, TRUE);