ModemManager: Allow plugin to specify custom replies (was: ModemManager: Handle partial replies in AT+CPIN? (READY without OK))

[Aleksander Morgado <aleksander lanedo com>]

Hi all,

> So we have this "AirLink Fastrack Xtend" Sierra Wireless modem [1],
> ModemManager uses the generic GSM plugin to handle it.
> 
> On PIN check the following flow happens:
>   --> 'AT+CPIN?'
>   <-- '<CR><LF>+CPIN: READY<CR><LF>'
> (see [2] for the logs reported by Thomas Bechtold a while ago).
> 
> Once that is received, response is parsed but no successful response is
> found, as there is no "<CR><LF>OK<CR><LF>" after the READY [3]. Thus,
> ModemManager will wait for more data to come, and ends up timing out
> ("Serial command timed out").
> 
> Once timed out, pin_check_done() gets called in mm-generic-gsm.c, where
> both the response GString (with the partial reply) and the GError are
> set. If a GError is set in pin_check_done(), the contents of the GString
> are fully discarded, and therefore, the pin check is retried up to 3
> times and never succeeds.
> 
> Now, what would be the proper way to handle this situation? One option
> is to make the OK on the reply to AT+CPIN? optional, by parsing partial
> replies. See attached patch.
> 
> Another option, I guess, is to write a new plugin to handle this
> specific case. Maybe there is some other plugin handling it?
> 

So, in order to handle this specific case of AT+CPIN? not replying OK,
instead of waiting for the timeout and processing partial replies, which
is quite a dirty hack, it's definitely better to instead allow specific
plugins to set their own expected responses.

The plugin for the modem with this AT+CPIN? reply issue could then
enable a regex like this as valid expected response:
        "\\r\\n\\+CPIN: .*\\r\\n"

The attached patch implements a new method for this purpose:

        void mm_serial_parser_v1_set_custom_regex (gpointer data,
                                                   GRegex *successful,
                                                   GRegex *error);

This method allows setting custom regex expressions for successful and
error expected responses.

The plugin then would need to implement grab_port() and as soon as the
port is created, set a new specific response parser with an extended v1
parser, something like this:

    port = mm_generic_gsm_grab_port (gsm, subsys, name, ptype, error);
    if (port && MM_IS_AT_SERIAL_PORT (port)) {
        gpointer parser;
        GRegex *regex;

        parser = mm_serial_parser_v1_new ();

        /* AT+CPIN? replies will never have an OK appended */
        regex = g_regex_new ("\\r\\n\\+CPIN: .*\\r\\n",
                             G_REGEX_RAW | G_REGEX_OPTIMIZE,
                             0, NULL);
        mm_serial_parser_v1_set_custom_regex (parser, regex, NULL);
        g_regex_unref (regex);

        mm_at_serial_port_set_response_parser (
            MM_AT_SERIAL_PORT (port),
            mm_serial_parser_v1_parse,
            parser,
            mm_serial_parser_v1_destroy);
    }


Comments?

Cheers!

-- 
Aleksander

>From e0a4d4c2db931882b094d0af2ceefb65d60590fd Mon Sep 17 00:00:00 2001
From: Aleksander Morgado <aleksander lanedo com>
Date: Wed, 30 Mar 2011 18:29:15 +0200
Subject: [PATCH 1/2] serial-parser: allow user to provide custom regex for successful and error replies

New mm_serial_parser_v1_set_custom_regex() method.
---
 src/mm-serial-parsers.c |   67 +++++++++++++++++++++++++++++++++++++++++++---
 src/mm-serial-parsers.h |   16 +++++-----
 2 files changed, 70 insertions(+), 13 deletions(-)

diff --git a/src/mm-serial-parsers.c b/src/mm-serial-parsers.c
index 75bcce4..f679831 100644
--- a/src/mm-serial-parsers.c
+++ b/src/mm-serial-parsers.c
@@ -196,13 +196,17 @@ mm_serial_parser_v0_destroy (gpointer data)
 }
 
 typedef struct {
+    /* Regular expressions for successful replies */
     GRegex *regex_ok;
     GRegex *regex_connect;
+    GRegex *regex_custom_successful;
+    /* Regular expressions for error replies */
     GRegex *regex_cme_error;
     GRegex *regex_cme_error_str;
     GRegex *regex_ezx_error;
     GRegex *regex_unknown_error;
     GRegex *regex_connect_failed;
+    GRegex *regex_custom_error;
 } MMSerialParserV1;
 
 gpointer
@@ -221,9 +225,30 @@ mm_serial_parser_v1_new (void)
     parser->regex_unknown_error = g_regex_new ("\\r\\n(ERROR)|(COMMAND NOT SUPPORT)\\r\\n$", flags, 0, NULL);
     parser->regex_connect_failed = g_regex_new ("\\r\\n(NO CARRIER)|(BUSY)|(NO ANSWER)|(NO DIALTONE)\\r\\n$", flags, 0, NULL);
 
+    parser->regex_custom_successful = NULL;
+    parser->regex_custom_error = NULL;
+
     return parser;
 }
 
+void
+mm_serial_parser_v1_set_custom_regex (gpointer data,
+                                      GRegex *successful,
+                                      GRegex *error)
+{
+    MMSerialParserV1 *parser = (MMSerialParserV1 *) data;
+
+    g_return_if_fail (parser != NULL);
+
+    if (parser->regex_custom_successful)
+        g_regex_unref (parser->regex_custom_successful);
+    if (parser->regex_custom_error)
+        g_regex_unref (parser->regex_custom_error);
+
+    parser->regex_custom_successful = successful ? g_regex_ref (successful) : NULL;
+    parser->regex_custom_error = error ? g_regex_ref (error) : NULL;
+}
+
 gboolean
 mm_serial_parser_v1_parse (gpointer data,
                            GString *response,
@@ -244,11 +269,23 @@ mm_serial_parser_v1_parse (gpointer data,
 
     /* First, check for successful responses */
 
-    found = g_regex_match_full (parser->regex_ok, response->str, response->len, 0, 0, NULL, NULL);
-    if (found)
-        remove_matches (parser->regex_ok, response);
-    else
-        found = g_regex_match_full (parser->regex_connect, response->str, response->len, 0, 0, NULL, NULL);
+    /* Custom successful replies first, if any */
+    if (parser->regex_custom_successful)
+        found = g_regex_match_full (parser->regex_custom_successful,
+                                    response->str, response->len,
+                                    0, 0, NULL, NULL);
+
+    if (!found) {
+        found = g_regex_match_full (parser->regex_ok,
+                                    response->str, response->len,
+                                    0, 0, NULL, NULL);
+        if (found)
+            remove_matches (parser->regex_ok, response);
+        else
+            found = g_regex_match_full (parser->regex_connect,
+                                        response->str, response->len,
+                                        0, 0, NULL, NULL);
+    }
 
     if (found) {
         response_clean (response);
@@ -257,6 +294,21 @@ mm_serial_parser_v1_parse (gpointer data,
 
     /* Now failures */
 
+    /* Custom error matches first, if any */
+    if (parser->regex_custom_error) {
+        found = g_regex_match_full (parser->regex_custom_error,
+                                    response->str, response->len,
+                                    0, 0, &match_info, NULL);
+        if (found) {
+            str = g_match_info_fetch (match_info, 1);
+            g_assert (str);
+            local_error = mm_mobile_error_for_code (atoi (str));
+            g_free (str);
+            g_match_info_free (match_info);
+            goto done;
+        }
+    }
+
     /* Numeric CME errors */
     found = g_regex_match_full (parser->regex_cme_error,
                                 response->str, response->len,
@@ -359,6 +411,11 @@ mm_serial_parser_v1_destroy (gpointer data)
     g_regex_unref (parser->regex_unknown_error);
     g_regex_unref (parser->regex_connect_failed);
 
+    if (parser->regex_custom_successful)
+        g_regex_unref (parser->regex_custom_successful);
+    if (parser->regex_custom_error)
+        g_regex_unref (parser->regex_custom_error);
+
     g_slice_free (MMSerialParserV1, data);
 }
 
diff --git a/src/mm-serial-parsers.h b/src/mm-serial-parsers.h
index 3e1fb9f..92361c3 100644
--- a/src/mm-serial-parsers.h
+++ b/src/mm-serial-parsers.h
@@ -22,23 +22,23 @@ gpointer mm_serial_parser_v0_new     (void);
 gboolean mm_serial_parser_v0_parse   (gpointer parser,
                                       GString *response,
                                       GError **error);
-
 void     mm_serial_parser_v0_destroy (gpointer parser);
 
 
-gpointer mm_serial_parser_v1_new     (void);
-gboolean mm_serial_parser_v1_parse   (gpointer parser,
-                                      GString *response,
-                                      GError **error);
-
-void     mm_serial_parser_v1_destroy (gpointer parser);
+gpointer mm_serial_parser_v1_new              (void);
+void     mm_serial_parser_v1_set_custom_regex (gpointer data,
+                                               GRegex *successful,
+                                               GRegex *error);
+gboolean mm_serial_parser_v1_parse            (gpointer parser,
+                                               GString *response,
+                                               GError **error);
+void     mm_serial_parser_v1_destroy          (gpointer parser);
 
 
 gpointer mm_serial_parser_v1_e1_new     (void);
 gboolean mm_serial_parser_v1_e1_parse   (gpointer parser,
                                          GString *response,
                                          GError **error);
-
 void     mm_serial_parser_v1_e1_destroy (gpointer parser);
 
 #endif /* MM_SERIAL_PARSERS_H */
-- 
1.7.1