Re: [PATCH] ModemManager: Enhancements to Icera error reporting and access technology reporting

[Eric Shienbrood <ers google com>]

Thanks Aleksander! New patch file is attached.

Eric

On Tue, Jun 7, 2011 at 2:56 AM, Aleksander Morgado <aleksander lanedo com> wrote:

Hi Eric,

> The additional error detail allows us to know that a connection failed
> because of an invalid APN. Also, when handling access technology
> changes, report the technology in use if we're connected. Finally,
> avoid using CFUN=0 when disabling the modem.

A small thing to fix in the patch.

All callbacks receiving a response from the AT serial port should check
if the modem has already been removed, so in
query_network_error_code_done(), this check is needed just before trying
to parse any reply:

   /* If the modem has already been removed, return without
    * scheduling callback */
   if (mm_callback_info_check_modem_removed (info))
       return;

See
http://cgit.freedesktop.org/ModemManager/ModemManager/commit/?id=9323daec015ecad65c39b6020b62e864c027d858

Cheers!

--
Aleksander

From d145ada9a31ed043d3f9da09c3abf7a1c5f7aff6 Mon Sep 17 00:00:00 2001
From: Eric Shienbrood <ers google com>
Date: Mon, 6 Jun 2011 15:09:15 -0400
Subject: [PATCH] Icera enhancements.

- For connection failures, get additional error detail. Currently,
  the only error codes that are mapped are the 3GPP TS 24.008 codes
  for "Unknown or missing access point name" and "Requested service
  option not subscribed" (which is sometimes returned for an invalid
  APN).
- In Samsung plugin, for disable, don't use CFUN=0, since it prevents
  some useful operations from being performed, e.g., getting PIN lock
  status. Use CFUN=4 instead, which turns off the radios.
- In handling %NWSTATE messages, give the access mode in the connection
  state field precedence over what's in the access technology field, so
  that when connected, the actual technology in use is what gets
  reported.
---
 plugins/Makefile.am            |    3 ++
 plugins/mm-modem-icera.c       |   65 ++++++++++++++++++++++++++++++++--------
 plugins/mm-modem-samsung-gsm.c |    8 ++++-
 3 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/plugins/Makefile.am b/plugins/Makefile.am
index c7a7a3d..4abb026 100644
--- a/plugins/Makefile.am
+++ b/plugins/Makefile.am
@@ -394,6 +394,9 @@ libmm_plugin_cinterion_la_LDFLAGS = \
 	-module \
 	-avoid-version
 
+libmm_plugin_samsung_la_LIBADD = \
+	$(builddir)/libicera-utils.la
+
 udevrulesdir = $(UDEV_BASE_DIR)/rules.d
 udevrules_DATA = \
 	77-mm-ericsson-mbm.rules \
diff --git a/plugins/mm-modem-icera.c b/plugins/mm-modem-icera.c
index b093b34..fd597c0 100644
--- a/plugins/mm-modem-icera.c
+++ b/plugins/mm-modem-icera.c
@@ -192,11 +192,13 @@ nwstate_to_act (const char *str)
         return MM_MODEM_GSM_ACCESS_TECH_UMTS;
     else if (!strcmp (str, "3g"))
         return MM_MODEM_GSM_ACCESS_TECH_UMTS;
-    else if (!strcmp (str, "3G-HSDPA"))
+    else if (!strcmp (str, "R99"))
+        return MM_MODEM_GSM_ACCESS_TECH_UMTS;
+    else if (!strcmp (str, "3G-HSDPA") || !strcmp (str, "HSDPA"))
         return MM_MODEM_GSM_ACCESS_TECH_HSDPA;
-    else if (!strcmp (str, "3G-HSUPA"))
+    else if (!strcmp (str, "3G-HSUPA") || !strcmp (str, "HSUPA"))
         return MM_MODEM_GSM_ACCESS_TECH_HSUPA;
-    else if (!strcmp (str, "3G-HSDPA-HSUPA"))
+    else if (!strcmp (str, "3G-HSDPA-HSUPA") || !strcmp (str, "HSDPA-HSUPA"))
         return MM_MODEM_GSM_ACCESS_TECH_HSPA;
 
     return MM_MODEM_GSM_ACCESS_TECH_UNKNOWN;
@@ -219,7 +221,9 @@ nwstate_changed (MMAtSerialPort *port,
         g_free (str);
     }
 
-    str = g_match_info_fetch (info, 3);
+    str = g_match_info_fetch (info, 4);
+    if (!str || strcmp (str, "-") == 0)
+      str = g_match_info_fetch (info, 3);
     if (str) {
         act = nwstate_to_act (str);
         g_free (str);
@@ -357,13 +361,50 @@ icera_disconnect_done (MMModem *modem,
 }
 
 static void
+query_network_error_code_done (MMAtSerialPort *port,
+                               GString *response,
+                               GError *error,
+                               gpointer user_data)
+{
+    MMModemIcera *self = MM_MODEM_ICERA (user_data);
+    MMModemIceraPrivate *priv = MM_MODEM_ICERA_GET_PRIVATE (self);
+    MMCallbackInfo *info = priv->connect_pending_data;
+    gboolean mobile_error = FALSE;
+
+    /* If the modem has already been removed, return without
+     * scheduling callback */
+    if (mm_callback_info_check_modem_removed (info))
+        return;
+
+    if (info && g_str_has_prefix (response->str, "%IER: ")) {
+        int nw_activation_err;
+        if (sscanf (response->str + 6, "%*d,%*d,%d", &nw_activation_err)) {
+            /* 3GPP TS 24.008 Annex G error codes:
+             * 27 - Unknown or missing access point name
+             * 33 - Requested service option not subscribed
+             */
+            if (nw_activation_err == 27 || nw_activation_err == 33) {
+                info->error =
+                        mm_mobile_error_for_code(MM_MOBILE_ERROR_GPRS_NOT_SUBSCRIBED);
+                mobile_error = TRUE;
+            }
+        }
+    }
+    if (info && !mobile_error) {
+        info->error = g_error_new_literal (MM_MODEM_ERROR,
+                                           MM_MODEM_ERROR_GENERAL,
+                                           "Call setup failed");
+    }
+    connect_pending_done (self);
+}
+
+static void
 connection_enabled (MMAtSerialPort *port,
                     GMatchInfo *match_info,
                     gpointer user_data)
 {
     MMModemIcera *self = MM_MODEM_ICERA (user_data);
-    MMModemIceraPrivate *priv = MM_MODEM_ICERA_GET_PRIVATE (self);
-    MMCallbackInfo *info = priv->connect_pending_data;
+    MMAtSerialPort *primary;
     char *str;
     int status, cid, tmp;
 
@@ -400,12 +441,11 @@ connection_enabled (MMAtSerialPort *port,
         break;
     case 3:
         /* Call setup failure? */
-        if (info) {
-            info->error = g_error_new_literal (MM_MODEM_ERROR,
-                                               MM_MODEM_ERROR_GENERAL,
-                                               "Call setup failed");
-        }
-        connect_pending_done (self);
+        primary = mm_generic_gsm_get_at_port (MM_GENERIC_GSM(self), MM_PORT_TYPE_PRIMARY);
+        g_assert (primary);
+        /* Get additional error details */
+        mm_at_serial_port_queue_command (primary, "AT%IER?", 3,
+                                         query_network_error_code_done, self);
         break;
     default:
         mm_warn ("Unknown Icera connect status %d", status);
@@ -848,4 +888,3 @@ mm_modem_icera_get_type (void)
 
     return icera_type;
 }
-
diff --git a/plugins/mm-modem-samsung-gsm.c b/plugins/mm-modem-samsung-gsm.c
index 3b29eeb..df7a4b4 100755
--- a/plugins/mm-modem-samsung-gsm.c
+++ b/plugins/mm-modem-samsung-gsm.c
@@ -478,8 +478,12 @@ disable (MMModem *modem,
     primary = mm_generic_gsm_get_at_port (MM_GENERIC_GSM (modem), MM_PORT_TYPE_PRIMARY);
     g_assert (primary);
 
-    /* Random command to ensure unsolicited message disable completes */
-    mm_at_serial_port_queue_command (primary, "AT+CFUN=0", 5, disable_unsolicited_done, info);
+    /*
+     * Command to ensure unsolicited message disable completes.
+     * Turns the radios off, which seems like a reasonable
+     * think to do when disabling.
+     */
+    mm_at_serial_port_queue_command (primary, "AT+CFUN=4", 5, disable_unsolicited_done, info);
 }
 
 static void
-- 
1.7.3.1