port-probe: refactor AT command handling

Make it use a GVariant in the response processor, as the AT command handling in
the MMBaseModem.

TODO

@@ -96,10 +96,6 @@ example to update the signal quality value or check registration status.
      a rev0, revA or revB CDMA network. We need to expose the exact access
      technology in the interface.
 
-  ** Consolidate the AT command processor in MMPluginBase; possibly just let it
-     return a GVariant instead of GValue, as with the AT command processor in
-     the MMBaseModem.
-
   ** Fix object names to show proper inheritance? For example:
      - MMPort, MMPortSerial, MMPortSerialAt, MMPortSerialQcdm
      - MMModem (instead of MMBaseModem), MMModemBroadband, MMModemPots

plugins/nokia/mm-plugin-nokia.c

@@ -20,6 +20,7 @@
 
 #include <libmm-common.h>
 
+#include "mm-log.h"
 #include "mm-plugin-nokia.h"
 #include "mm-broadband-modem-nokia.h"
 
@@ -31,47 +32,10 @@ int mm_plugin_minor_version = MM_PLUGIN_MINOR_VERSION;
 /*****************************************************************************/
 /* CUSTOM INIT */
 
-static gboolean
-parse_init_last (const gchar *response,
-                 const GError *error,
-                 GValue *result,
-                 GError **result_error)
-{
-    if (error) {
-        *result_error = g_error_copy (error);
-        return FALSE;
-    }
-
-    /* Otherwise, done. And also report that it's an AT port. */
-    g_value_init (result, G_TYPE_BOOLEAN);
-    g_value_set_boolean (result, TRUE);
-    return TRUE;
-}
-
-static gboolean
-parse_init (const gchar *response,
-            const GError *error,
-            GValue *result,
-            GError **result_error)
-{
-    if (error) {
-        /* On timeout, request to retry */
-        if (g_error_matches (error,
-                             MM_SERIAL_ERROR,
-                             MM_SERIAL_ERROR_RESPONSE_TIMEOUT))
-            return FALSE; /* Retry */
-    }
-
-    /* Otherwise, done. And also report that it's an AT port. */
-    g_value_init (result, G_TYPE_BOOLEAN);
-    g_value_set_boolean (result, TRUE);
-    return TRUE;
-}
-
 static const MMPortProbeAtCommand custom_init[] = {
-    { "ATE1 E0", parse_init },
+    { "ATE1 E0", 3, mm_port_probe_response_processor_is_at },
-    { "ATE1 E0", parse_init },
+    { "ATE1 E0", 3, mm_port_probe_response_processor_is_at },
-    { "ATE1 E0", parse_init_last },
+    { "ATE1 E0", 3, mm_port_probe_response_processor_is_at },
     { NULL }
 };
 

src/Makefile.am

@@ -188,8 +188,8 @@ ModemManager_SOURCES = \
 	mm-serial-parsers.h \
 	mm-port-probe.h \
 	mm-port-probe.c \
-	mm-port-probe-at-command.h \
+	mm-port-probe-at.h \
-	mm-port-probe-at-command.c \
+	mm-port-probe-at.c \
 	mm-port-probe-cache.h \
 	mm-port-probe-cache.c \
 	mm-plugin.c \

src/mm-port-probe-at-command.c

@@ -1,135 +0,0 @@
-/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
-/*
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details:
- *
- * Copyright (C) 2008 - 2009 Novell, Inc.
- * Copyright (C) 2009 - 2011 Red Hat, Inc.
- * Copyright (C) 2011 Aleksander Morgado <aleksander@gnu.org>
- */
-
-#define _GNU_SOURCE  /* for strcasestr */
-#include <string.h>
-
-#include <glib.h>
-
-#include <ModemManager.h>
-#include <mm-errors-types.h>
-
-#include "mm-port-probe.h"
-#include "mm-port-probe-at-command.h"
-
-/* ---- AT probing ---- */
-
-static gboolean
-parse_at (const gchar *response,
-          const GError *error,
-          GValue *result,
-          GError **result_error)
-{
-    if (error) {
-        /* On timeout, request to retry */
-        if (g_error_matches (error,
-                             MM_SERIAL_ERROR,
-                             MM_SERIAL_ERROR_RESPONSE_TIMEOUT))
-            return FALSE; /* Retry */
-
-        /* If error is not recognizable, request to abort */
-        if (error->domain != MM_MOBILE_EQUIPMENT_ERROR) {
-            *result_error = g_error_copy (error);
-            g_prefix_error (result_error,
-                            "Couldn't parse AT reply. ");
-            return FALSE;
-        }
-
-        /* If the modem returned a recognizable error,
-         * it can do AT commands */
-        g_value_init (result, G_TYPE_BOOLEAN);
-        g_value_set_boolean (result, TRUE);
-        return TRUE;
-    }
-
-    /* No error reported, valid AT port! */
-    g_value_init (result, G_TYPE_BOOLEAN);
-    g_value_set_boolean (result, TRUE);
-    return TRUE;
-}
-
-static const MMPortProbeAtCommand at_probing[] = {
-    { "AT",  parse_at },
-    { "AT",  parse_at },
-    { "AT",  parse_at },
-    { NULL }
-};
-
-const MMPortProbeAtCommand *
-mm_port_probe_at_command_get_probing (void)
-{
-    return at_probing;
-}
-
-/* ---- VENDOR probing ---- */
-
-static gboolean
-parse_vendor (const gchar *response,
-              const GError *error,
-              GValue *result,
-              GError **result_error)
-{
-    gchar *str;
-
-    str = g_strstrip (g_strdelimit (g_strdup (response), "\r\n", ' '));
-    g_value_init (result, G_TYPE_STRING);
-    g_value_take_string (result, str);
-    return TRUE;
-}
-
-static const MMPortProbeAtCommand vendor_probing[] = {
-    { "+CGMI", parse_vendor },
-    { "+GMI",  parse_vendor },
-    { "I",     parse_vendor },
-    { NULL }
-};
-
-const MMPortProbeAtCommand *
-mm_port_probe_at_command_get_vendor_probing (void)
-{
-    return vendor_probing;
-}
-
-/* ---- PRODUCT probing ---- */
-
-static gboolean
-parse_product (const gchar *response,
-               const GError *error,
-               GValue *result,
-               GError **result_error)
-
-{
-    gchar *str;
-
-    str = g_strstrip (g_strdelimit (g_strdup (response), "\r\n", ' '));
-    g_value_init (result, G_TYPE_STRING);
-    g_value_take_string (result, str);
-    return TRUE;
-}
-
-static const MMPortProbeAtCommand product_probing[] = {
-    { "+CGMM", parse_product },
-    { "+GMM",  parse_product },
-    { "I",     parse_product },
-    { NULL }
-};
-
-const MMPortProbeAtCommand *
-mm_port_probe_at_command_get_product_probing (void)
-{
-    return product_probing;
-}

src/mm-port-probe-at-command.h

@@ -1,58 +0,0 @@
-/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
-/*
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details:
- *
- * Copyright (C) 2011 Aleksander Morgado <aleksander@gnu.org>
- */
-
-#ifndef MM_PORT_PROBE_AT_COMMAND_H
-#define MM_PORT_PROBE_AT_COMMAND_H
-
-#include <glib.h>
-
-/* Struct to configure port probing commands */
-typedef struct {
-    /* The AT command */
-    const gchar *command;
-
-    /* The response processor. The expected result depends on the
-     * probing type:
-     *  - AT             --> G_TYPE_BOOLEAN
-     *  - Vendor         --> G_TYPE_STRING
-     *  - Product        --> G_TYPE_STRING
-     * When a result is given, TRUE is returned.
-     * When no result is given, FALSE is returned and:
-     *   - If result_error given, it should be treated as a critical error,
-     *     and abort the probing.
-     *   - If no result_error is given, we can just go on to the next command
-     *     in the group.
-     *
-     * A special case to consider is the initialization commands, used by
-     * some plugins. In this case, there is no expected result, but plugins may
-     * set an optional boolean result, specifying whether the port is an AT port
-     * or not.
-     *  - Initialization --> NONE | G_TYPE_BOOLEAN
-     * When the initialization is considered enough, TRUE is returned, and
-     * FALSE otherwise.
-     */
-    gboolean (* response_processor) (const gchar *response,
-                                     const GError *error,
-                                     GValue *result,
-                                     GError **result_error);
-} MMPortProbeAtCommand;
-
-/* Default commands used during probing */
-const MMPortProbeAtCommand *mm_port_probe_at_command_get_probing (void);
-const MMPortProbeAtCommand *mm_port_probe_at_command_get_vendor_probing (void);
-const MMPortProbeAtCommand *mm_port_probe_at_command_get_product_probing (void);
-
-#endif /* MM_PORT_PROBE_AT_COMMAND_H */
-

src/mm-port-probe-at.c

@@ -0,0 +1,93 @@
+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details:
+ *
+ * Copyright (C) 2008 - 2009 Novell, Inc.
+ * Copyright (C) 2009 - 2011 Red Hat, Inc.
+ * Copyright (C) 2011 - 2012 Aleksander Morgado <aleksander@gnu.org>
+ * Copyright (C) 2012 Google, Inc.
+ */
+
+#define _GNU_SOURCE  /* for strcasestr */
+#include <string.h>
+
+#include <glib.h>
+
+#include <ModemManager.h>
+#include <mm-errors-types.h>
+
+#include "mm-log.h"
+#include "mm-port-probe.h"
+#include "mm-port-probe-at.h"
+#include "mm-serial-parsers.h"
+
+/* ---- AT probing ---- */
+
+gboolean
+mm_port_probe_response_processor_is_at (const gchar *command,
+                                        const gchar *response,
+                                        gboolean last_command,
+                                        const GError *error,
+                                        GVariant **result,
+                                        GError **result_error)
+{
+    if (error) {
+        mm_dbg ("Parsing AT got: '%s'", error->message);
+
+        /* Timeout errors are the only ones not fatal;
+         * they will just go on to the next command. */
+        if (g_error_matches (error,
+                             MM_SERIAL_ERROR,
+                             MM_SERIAL_ERROR_RESPONSE_TIMEOUT)) {
+            return FALSE;
+        }
+
+        /* If error is NOT known by the parser, request to abort */
+        if (!mm_serial_parser_v1_is_known_error (error)) {
+            *result_error = g_error_copy (error);
+            g_prefix_error (result_error,
+                            "Fatal error parsing AT reply. ");
+            return FALSE;
+        }
+
+        /* If the modem returned a recognizable error,
+         * it can do AT commands */
+        *result = g_variant_new_boolean (TRUE);
+        return TRUE;
+    }
+
+    /* No error reported, valid AT port! */
+    *result = g_variant_new_boolean (TRUE);
+    return TRUE;
+}
+
+/* ---- String probing ---- */
+
+gboolean
+mm_port_probe_response_processor_string (const gchar *command,
+                                         const gchar *response,
+                                         gboolean last_command,
+                                         const GError *error,
+                                         GVariant **result,
+                                         GError **result_error)
+{
+    gchar *str;
+
+    if (error)
+        /* Try with the next command, if any */
+        return FALSE;
+
+    str = g_strstrip (g_strdelimit (g_strdup (response), "\r\n", ' '));
+    *result = g_variant_new_string (str);
+    g_free (str);
+
+    return TRUE;
+}

src/mm-port-probe-at.h

@@ -0,0 +1,79 @@
+/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details:
+ *
+ * Copyright (C) 2011 - 2012 Aleksander Morgado <aleksander@gnu.org>
+ * Copyright (C) 2012 Google, Inc.
+ */
+
+#ifndef MM_PORT_PROBE_AT_H
+#define MM_PORT_PROBE_AT_H
+
+#include <glib.h>
+
+/* The response processor. The expected result depends on the
+ * probing type:
+ *  - AT             --> Boolean
+ *  - Vendor         --> String
+ *  - Product        --> String
+ *
+ * TRUE must be returned when the operation is to be considered successful,
+ * and a result may be given.
+ *
+ * FALSE must be returned when:
+ *  - A GError is propagated into result_error, which will be treated as a
+ *    critical error and therefore the operation will be aborted.
+ *  - When no result_error is given, to instruct the operation to go on with
+ *    the next scheduled command.
+ *
+ * A special case to consider is the initialization commands, used by
+ * some plugins. In this case, there is no expected result, but plugins may
+ * set an optional boolean result, specifying whether the port is an AT port
+ * or not.
+ *  - Initialization --> None or Boolean
+ * When the initialization is considered enough, TRUE is returned, and
+ * FALSE otherwise.
+ */
+typedef gboolean (* MMPortProbeAtResponseProcessor) (const gchar *command,
+                                                     const gchar *response,
+                                                     gboolean last_command,
+                                                     const GError *error,
+                                                     GVariant **result,
+                                                     GError **result_error);
+
+/* Struct to configure port probing commands */
+typedef struct {
+    /* The AT command */
+    gchar *command;
+    /* Timeout of the command, in seconds */
+    guint timeout;
+    /* The response processor */
+    MMPortProbeAtResponseProcessor response_processor;
+} MMPortProbeAtCommand;
+
+/* Common helper response processors */
+
+/* Every string received as response, will be set as result */
+gboolean mm_port_probe_response_processor_string (const gchar *command,
+                                                  const gchar *response,
+                                                  gboolean last_command,
+                                                  const GError *error,
+                                                  GVariant **result,
+                                                  GError **result_error);
+/* Generic response parser for AT probing, useful also in custom init commands */
+gboolean mm_port_probe_response_processor_is_at (const gchar *command,
+                                                 const gchar *response,
+                                                 gboolean last_command,
+                                                 const GError *error,
+                                                 GVariant **result,
+                                                 GError **result_error);
+
+#endif /* MM_PORT_PROBE_AT_H */

src/mm-port-probe.c

@@ -27,7 +27,7 @@
 #include "mm-at-serial-port.h"
 #include "mm-serial-port.h"
 #include "mm-serial-parsers.h"
-#include "mm-port-probe-at-command.h"
+#include "mm-port-probe-at.h"
 #include "libqcdm/src/commands.h"
 #include "libqcdm/src/utils.h"
 #include "libqcdm/src/errors.h"
@@ -66,7 +66,7 @@ typedef struct {
     const MMPortProbeAtCommand *at_commands;
     /* Current AT Result processor */
     void (* at_result_processor) (MMPortProbe *self,
-                                  GValue *result);
+                                  GVariant *result);
 } PortProbeRunTask;
 
 struct _MMPortProbePrivate {
@@ -292,14 +292,14 @@ serial_probe_qcdm (MMPortProbe *self)
 
 static void
 serial_probe_at_product_result_processor (MMPortProbe *self,
-                                          GValue *result)
+                                          GVariant *result)
 {
     if (result) {
         /* If any result given, it must be a string */
-        g_assert (G_VALUE_HOLDS_STRING (result));
+        g_assert (g_variant_is_of_type (result, G_VARIANT_TYPE_STRING));
 
         mm_dbg ("(%s) product probing finished", self->priv->name);
-        self->priv->product = g_utf8_casefold (g_value_get_string (result), -11);
+        self->priv->product = g_utf8_casefold (g_variant_get_string (result, NULL), -1);
         self->priv->flags |= MM_PORT_PROBE_AT_PRODUCT;
         return;
     }
@@ -311,14 +311,14 @@ serial_probe_at_product_result_processor (MMPortProbe *self,
 
 static void
 serial_probe_at_vendor_result_processor (MMPortProbe *self,
-                                         GValue *result)
+                                         GVariant *result)
 {
     if (result) {
         /* If any result given, it must be a string */
-        g_assert (G_VALUE_HOLDS_STRING (result));
+        g_assert (g_variant_is_of_type (result, G_VARIANT_TYPE_STRING));
 
         mm_dbg ("(%s) vendor probing finished", self->priv->name);
-        self->priv->vendor = g_utf8_casefold (g_value_get_string (result), -1);
+        self->priv->vendor = g_utf8_casefold (g_variant_get_string (result, NULL), -1);
         self->priv->flags |= MM_PORT_PROBE_AT_VENDOR;
         return;
     }
@@ -330,13 +330,13 @@ serial_probe_at_vendor_result_processor (MMPortProbe *self,
 
 static void
 serial_probe_at_result_processor (MMPortProbe *self,
-                                  GValue *result)
+                                  GVariant *result)
 {
     if (result) {
         /* If any result given, it must be a boolean */
-        g_assert (G_VALUE_HOLDS_BOOLEAN (result));
+        g_assert (g_variant_is_of_type (result, G_VARIANT_TYPE_BOOLEAN));
 
-        if (g_value_get_boolean (result)) {
+        if (g_variant_get_boolean (result)) {
             mm_dbg ("(%s) port is AT-capable", self->priv->name);
             self->priv->is_at = TRUE;
             self->priv->flags |= MM_PORT_PROBE_AT;
@@ -357,13 +357,13 @@ serial_probe_at_result_processor (MMPortProbe *self,
 
 static void
 serial_probe_at_custom_init_result_processor (MMPortProbe *self,
-                                              GValue *result)
+                                              GVariant *result)
 {
     PortProbeRunTask *task = self->priv->task;
 
     /* No result is really expected here, but we could get a boolean to indicate
      * AT support */
-    if (G_VALUE_HOLDS_BOOLEAN (result))
+    if (result)
         serial_probe_at_result_processor (self, result);
 
     /* Reset so that it doesn't get scheduled again */
@@ -377,14 +377,16 @@ serial_probe_at_parse_response (MMAtSerialPort *port,
                                 MMPortProbe *self)
 {
     PortProbeRunTask *task = self->priv->task;
-    GValue result = { 0 };
+    GVariant *result = NULL;
     GError *result_error = NULL;
 
     /* If already cancelled, do nothing else */
     if (port_probe_run_is_cancelled (self))
         return;
 
-    if (!task->at_commands->response_processor (response->str,
+    if (!task->at_commands->response_processor (task->at_commands->command,
+                                                response->str,
+                                                !!task->at_commands[1].command,
                                                 error,
                                                 &result,
                                                 &result_error)) {
@@ -418,14 +420,11 @@ serial_probe_at_parse_response (MMAtSerialPort *port,
         return;
     }
 
-    /* Got some processed result */
+    /* Run result processor.
-    if (G_IS_VALUE (&result)) {
+     * Note that custom init commands are allowed to not return anything */
-        task->at_result_processor (self, &result);
+    task->at_result_processor (self, result);
-        g_value_unset (&result);
+    if (result)
-    } else {
+        g_variant_unref (result);
-        /* Custom init commands are allowed to not return anything */
-        task->at_result_processor (self, NULL);
-    }
 
     /* Reschedule probing */
     serial_probe_schedule (self);
@@ -445,13 +444,34 @@ serial_probe_at (MMPortProbe *self)
     mm_at_serial_port_queue_command (
         MM_AT_SERIAL_PORT (task->serial),
         task->at_commands->command,
-        3,
+        task->at_commands->timeout,
         task->cancellable,
         (MMAtSerialResponseFn)serial_probe_at_parse_response,
         self);
     return FALSE;
 }
 
+static const MMPortProbeAtCommand at_probing[] = {
+    { "AT",  3, mm_port_probe_response_processor_is_at },
+    { "AT",  3, mm_port_probe_response_processor_is_at },
+    { "AT",  3, mm_port_probe_response_processor_is_at },
+    { NULL }
+};
+
+static const MMPortProbeAtCommand vendor_probing[] = {
+    { "+CGMI", 3, mm_port_probe_response_processor_string },
+    { "+GMI",  3, mm_port_probe_response_processor_string },
+    { "I",     3, mm_port_probe_response_processor_string },
+    { NULL }
+};
+
+static const MMPortProbeAtCommand product_probing[] = {
+    { "+CGMM", 3, mm_port_probe_response_processor_string },
+    { "+GMM",  3, mm_port_probe_response_processor_string },
+    { "I",     3, mm_port_probe_response_processor_string },
+    { NULL }
+};
+
 static void
 serial_probe_schedule (MMPortProbe *self)
 {
@@ -476,21 +496,21 @@ serial_probe_schedule (MMPortProbe *self)
              !(self->priv->flags & MM_PORT_PROBE_AT)) {
         /* Prepare AT probing */
         task->at_result_processor = serial_probe_at_result_processor;
-        task->at_commands = mm_port_probe_at_command_get_probing ();
+        task->at_commands = at_probing;
     }
     /* Vendor requested and not already probed? */
     else if ((task->flags & MM_PORT_PROBE_AT_VENDOR) &&
         !(self->priv->flags & MM_PORT_PROBE_AT_VENDOR)) {
         /* Prepare AT vendor probing */
         task->at_result_processor = serial_probe_at_vendor_result_processor;
-        task->at_commands = mm_port_probe_at_command_get_vendor_probing ();
+        task->at_commands = vendor_probing;
     }
     /* Product requested and not already probed? */
     else if ((task->flags & MM_PORT_PROBE_AT_PRODUCT) &&
              !(self->priv->flags & MM_PORT_PROBE_AT_PRODUCT)) {
         /* Prepare AT product probing */
         task->at_result_processor = serial_probe_at_product_result_processor;
-        task->at_commands = mm_port_probe_at_command_get_product_probing ();
+        task->at_commands = product_probing;
     }
 
     /* If a next AT group detected, go for it */

src/mm-port-probe.h

@@ -23,7 +23,7 @@
 #define G_UDEV_API_IS_SUBJECT_TO_CHANGE
 #include <gudev/gudev.h>
 
-#include "mm-port-probe-at-command.h"
+#include "mm-port-probe-at.h"
 #include "mm-at-serial-port.h"
 
 #define MM_TYPE_PORT_PROBE            (mm_port_probe_get_type ())