Echo removal (was: Re: ModemManager: issues with Nokia handset in git master)

[Aleksander Morgado <aleksander lanedo com>]

Hey,

I'm back again with this issue.

>> The recent commit 0f6d1b2 [1] to fix some N900 behaviour in git master
>> seems to be breaking my Nokia C7 connection. Without the PORT_SEND_DELAY
>> setting, I seem to get the same strange behaviour as described in commit
>> 46d757f [2]: we get some but not all echo characters back. I tried to
>> re-enable the PORT_SEND_DELAY property value to 5000 again, and when
>> that set, the modem now sends all echo-ed characters back (still doesn't
>> disable the echo anyway).
>>
>> But, while debugging the issue, I also found that I could properly fully
>> disable the echo if I first open the port with minicom and run ATE0 in
>> there. After some more debugging, it seems that if we use ATE1 (which is
>> part of the minicom initialization commands) and then ATE0 just when
>> opening the serial connection, the modem ends up properly disabling the
>> echo completely.
>>
>> Attached a patch that follows this logic both in the modem enabling
>> process and in the setup before probing. I tested it with my Nokia C7
>> and seems to work quite well; no need of PORT_SEND_DELAY and no need of
>> having the INIT command twice... Can someone test it with a N900 or some
>> other Nokia handset? If it ends up working properly, we can even skip
>> using the custom v1_e1 response parser in the Nokia plugin. Or maybe I'm
>> just too optimistic... :-)
> 
> Unfortunately the trick doesn't work on the N900, so I've applied
> everything but the mm-generic-gsm.c hunk.  Oddly though, now the N900
> echos back everything upon disconnect, which is the same problem we
> recently had with the Ericsson 5521gw, where "flashing" (setting baud to
> 0 for a short period) resets the ephemeral port attributes like echo and
> such.  Seems to work OK though.
> 

I quite randomly get this issue of echo-garbage received before the
proper reply. It seems that as soon as it happens once, it keeps
happening until I reboot the phone or something. The issue is even more
weird: sometimes the garbage/echo doesn't happen during probing, but
appears once we flash the port and enable the modem (similar to the
issue in the Ericsson one it seems).

The Nokia plugin (and the CDMA ones) uses the v1_e1 parser by default,
which is just the v1 parser but with extra echo cancellation. The echo
cancellation is done by matching the echo as any string not starting
with <CR><LF> and also ending with <CR>. The problem here is that this
method won't properly work for the case of the echo-garbage in the Nokia
case (e.g, note that the echo-garbage in the following example doesn't
end with <CR>):
  (ttyACM0): --> 'AT+CGDCONT=1,"IP","Internet"<CR>'
  (ttyACM0): <-- 'A+GCN=,"P"Itene"'
  (ttyACM0): <-- '<CR><LF>OK<CR><LF>'

The best solution I found so far for this issue is to make the parser
expect <CR><LF> prefix always in the response, removing anything coming
before that <CR><LF>. This is a bit more radical than what it was being
done in the v1_e1 parser, as we remove the rule of ensuring the echo
ends with <CR>. Of course, the best fix would have been to understand
why this issue happens and try to fix it, but I guess that the echo
removal in general isn't that bad.

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.

-- 
Aleksander

>From ac838b97bbd346da55a8748147999ac9a0bb6296 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.
---
 src/mm-at-serial-port.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/src/mm-at-serial-port.c b/src/mm-at-serial-port.c
index 0cec6d0..7762c0d 100644
--- a/src/mm-at-serial-port.c
+++ b/src/mm-at-serial-port.c
@@ -57,6 +57,22 @@ mm_at_serial_port_set_response_parser (MMAtSerialPort *self,
     priv->response_parser_notify = notify;
 }
 
+static void
+remove_echo (GByteArray *response)
+{
+    guint i;
+
+    for (i = 0; i < (response->len - 1); 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);
+            break;
+        }
+    }
+}
+
 static gboolean
 parse_response (MMSerialPort *port, GByteArray *response, GError **error)
 {
@@ -67,6 +83,9 @@ parse_response (MMSerialPort *port, GByteArray *response, GError **error)
 
     g_return_val_if_fail (priv->response_parser_fn != NULL, FALSE);
 
+    /* Remove echo */
+    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);
@@ -178,6 +197,9 @@ parse_unsolicited (MMSerialPort *port, GByteArray *response)
     MMAtSerialPortPrivate *priv = MM_AT_SERIAL_PORT_GET_PRIVATE (self);
     GSList *iter;
 
+    /* Remove echo */
+    remove_echo (response);
+
     for (iter = priv->unsolicited_msg_handlers; iter; iter = iter->next) {
         MMAtUnsolicitedMsgHandler *handler = (MMAtUnsolicitedMsgHandler *) iter->data;
         GMatchInfo *match_info;
-- 
1.7.5.4