Re: Echo removal

[Aleksander Morgado <aleksander lanedo com>]

> 
> Attached a patch for review, which does this echo removal in the v1
> parser. I'm assuming here that all known devices do prefix their replies
> with <CR><LF> :-) If that is not the case, I can improve the patch to
> make this behaviour configurable per plugin (even for the probing
> process). At least it works super well for my use case. If we do accept
> this patch, we could possibly remove from the sources the v1_e1 parser,
> which wouldn't be needed any more.
> 

Attaching updated version of this patch to handle some corner cases; and
also including some unit tests.

-- 
Aleksander

>From 48ce30df846bb2a742943d7b4d4bb1d695cc0d9c Mon Sep 17 00:00:00 2001
From: Aleksander Morgado <aleksander lanedo com>
Date: Wed, 11 Jan 2012 01:33:05 +0100
Subject: [PATCH] at-serial-port: implement built-in echo/garbage removal

We expect the responses to start always with <CR><LF>. We just remove anything
that comes before that.
---
 .gitignore                      |    1 +
 src/mm-at-serial-port.c         |   27 +++++++++++++
 src/mm-at-serial-port.h         |    4 +-
 src/tests/Makefile.am           |   15 +++++++-
 src/tests/test-at-serial-port.c |   83 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 128 insertions(+), 2 deletions(-)
 create mode 100644 src/tests/test-at-serial-port.c

diff --git a/.gitignore b/.gitignore
index 64e7b89..384f08a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,6 +34,7 @@ test/lsudev
 src/tests/test-modem-helpers
 src/tests/test-charsets
 src/tests/test-qcdm-serial-port
+src/tests/test-at-serial-port
 src/tests/test-sms
 policy/org.freedesktop.modem-manager.policy
 include/
diff --git a/src/mm-at-serial-port.c b/src/mm-at-serial-port.c
index 30da3a3..140d093 100644
--- a/src/mm-at-serial-port.c
+++ b/src/mm-at-serial-port.c
@@ -58,6 +58,27 @@ mm_at_serial_port_set_response_parser (MMAtSerialPort *self,
     priv->response_parser_notify = notify;
 }
 
+void
+mm_at_serial_port_remove_echo (GByteArray *response)
+{
+    guint i;
+
+    if (response->len <= 2)
+        return;
+
+    for (i = 0; i < (response->len - 2); i++) {
+        /* If there is any content before the first
+         * <CR><LF>, assume it's echo or garbage, and skip it */
+        if (response->data[i] == '\r' && response->data[i + 1] == '\n') {
+            if (i > 0)
+                g_byte_array_remove_range (response, 0, i);
+            else
+                /* good, we're already started with <CR><LF> */
+            break;
+        }
+    }
+}
+
 static gboolean
 parse_response (MMSerialPort *port, GByteArray *response, GError **error)
 {
@@ -68,6 +89,9 @@ parse_response (MMSerialPort *port, GByteArray *response, GError **error)
 
     g_return_val_if_fail (priv->response_parser_fn != NULL, FALSE);
 
+    /* Remove echo */
+    mm_at_serial_port_remove_echo (response);
+
     /* Construct the string that AT-parsing functions expect */
     string = g_string_sized_new (response->len + 1);
     g_string_append_len (string, (const char *) response->data, response->len);
@@ -159,6 +183,9 @@ parse_unsolicited (MMSerialPort *port, GByteArray *response)
     MMAtSerialPortPrivate *priv = MM_AT_SERIAL_PORT_GET_PRIVATE (self);
     GSList *iter;
 
+    /* Remove echo */
+    mm_at_serial_port_remove_echo (response);
+
     for (iter = priv->unsolicited_msg_handlers; iter; iter = iter->next) {
         MMAtUnsolicitedMsgHandler *handler = (MMAtUnsolicitedMsgHandler *) iter->data;
         GMatchInfo *match_info;
diff --git a/src/mm-at-serial-port.h b/src/mm-at-serial-port.h
index cec5dc3..689c184 100644
--- a/src/mm-at-serial-port.h
+++ b/src/mm-at-serial-port.h
@@ -80,5 +80,7 @@ void     mm_at_serial_port_queue_command_cached (MMAtSerialPort *self,
                                                  MMAtSerialResponseFn callback,
                                                  gpointer user_data);
 
-#endif /* MM_AT_SERIAL_PORT_H */
+/* Just for unit tests */
+void mm_at_serial_port_remove_echo (GByteArray *response);
 
+#endif /* MM_AT_SERIAL_PORT_H */
diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am
index cc47e66..0ed88c2 100644
--- a/src/tests/Makefile.am
+++ b/src/tests/Makefile.am
@@ -5,6 +5,7 @@ noinst_PROGRAMS = \
 	test-modem-helpers \
 	test-charsets \
 	test-qcdm-serial-port \
+	test-at-serial-port \
 	test-sms
 
 test_modem_helpers_SOURCES = \
@@ -41,6 +42,19 @@ test_qcdm_serial_port_LDADD = \
 	$(top_builddir)/libqcdm/src/libqcdm.la \
 	-lutil
 
+test_at_serial_port_SOURCES = \
+	test-at-serial-port.c
+
+test_at_serial_port_CPPFLAGS = \
+	$(MM_CFLAGS) \
+	-I$(top_srcdir)
+
+test_at_serial_port_LDADD = \
+	$(MM_LIBS) \
+	$(top_builddir)/src/libserial.la \
+	$(top_builddir)/src/libmodem-helpers.la \
+	-lutil
+
 test_sms_SOURCES = \
 	test-sms.c
 
@@ -60,4 +74,3 @@ check-local: test-modem-helpers
 	$(abs_builddir)/test-sms
 
 endif
-
diff --git a/src/tests/test-at-serial-port.c b/src/tests/test-at-serial-port.c
new file mode 100644
index 0000000..76ccadc
--- /dev/null
+++ b/src/tests/test-at-serial-port.c
@@ -0,0 +1,83 @@
+/* -*- 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) 2012 Aleksander Morgado <aleksander gnu org>
+ */
+
+#include <config.h>
+#include <string.h>
+#include <glib.h>
+
+#include "mm-errors.h"
+#include "mm-at-serial-port.h"
+#include "mm-log.h"
+
+typedef struct {
+    gchar *original;
+    gchar *without_echo;
+} EchoRemovalTest;
+
+static const EchoRemovalTest echo_removal_tests[] = {
+    { "\r\n", "\r\n" },
+    { "\r", "\r" },
+    { "\n", "\n" },
+    { "this is a string that ends just with <CR>\r", "this is a string that ends just with <CR>\r" },
+    { "this is a string that ends just with <CR>\n", "this is a string that ends just with <CR>\n" },
+    { "\r\nthis is valid", "\r\nthis is valid" },
+    { "a\r\nthis is valid", "\r\nthis is valid" },
+    { "a\r\n", "\r\n" },
+    { "all this string is to be considered echo\r\n", "\r\n" },
+    { "all this string is to be considered echo\r\nthis is valid", "\r\nthis is valid" },
+};
+
+static void
+at_serial_echo_removal (void)
+{
+    guint i;
+
+    for (i = 0; i < G_N_ELEMENTS (echo_removal_tests); i++) {
+        GByteArray *ba;
+
+        /* Note that we add last NUL also to the byte array, so that we can compare
+         * C strings later on */
+        ba = g_byte_array_sized_new (strlen (echo_removal_tests[i].original) + 1);
+        g_byte_array_prepend (ba,
+                              (guint8 *)echo_removal_tests[i].original,
+                              strlen (echo_removal_tests[i].original) + 1);
+
+        mm_at_serial_port_remove_echo (ba);
+
+        g_assert_cmpstr ((gchar *)ba->data, ==, echo_removal_tests[i].without_echo);
+
+        g_byte_array_unref (ba);
+    }
+}
+
+void
+_mm_log (const char *loc,
+         const char *func,
+         guint32 level,
+         const char *fmt,
+         ...)
+{
+    /* Dummy log function */
+}
+
+int main (int argc, char **argv)
+{
+    g_type_init ();
+    g_test_init (&argc, &argv, NULL);
+
+    g_test_add_func ("/ModemManager/AT-serial/echo-removal", at_serial_echo_removal);
+
+    return g_test_run ();
+}
-- 
1.7.5.4