[PATCH] huawei: fix parsing of ^NDISSTATQRY? response
Dan Williams
dcbw at redhat.com
Tue Aug 6 10:52:41 PDT 2013
On Tue, 2013-08-06 at 10:15 -0700, Ben Chan wrote:
> Thanks Dan. Looks like we sometimes need to ignore the ^PDPDEACT
> unsolicited messages as well.
>
> <debug> [000141.977013] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> <-- '<CR><LF>^RSSI:
> 10<CR><LF><CR><LF>^CSNR:-101,-8<CR><LF><CR><LF>^PDPDEACT:
> 3<CR><LF><CR><LF>^NDISSTAT: 0,,,"IPV4"<CR><LF>'
>
> I'm not sure if there is any other unsolicited message that should be
> ignored. Perhaps we need a catch-all regex for handling uninterested
> messages?
If you've got some time, you can investigate having the parser code
ignore unknown unsolicited responses and look for the expected response.
There are a number of corner cases though, where some devices and some
commands don't send the original command back at all (eg, they just do
"\r\n<data>" instead of "\r\nORIGCMD:<data>") or they send back a
different command than the original command. There's pretty incredible
variation in firmware :)
Dan
> Ben
>
>
> On Tue, Aug 6, 2013 at 7:26 AM, Dan Williams <dcbw at redhat.com> wrote:
>
> > On Mon, 2013-08-05 at 21:58 -0700, Ben Chan wrote:
> > > Hi Dan,
> > >
> > > Now that you mentioned it, the issue isn't quite obvious to me by looking
> > > at the log. I printed out the "response" passed to
> > > mm_huawei_parse_ndisstatqry_response and observed the "^RSSI: 16^M^M"
> > > message in front of the "^NDISSTATQRY" response:
> >
> > I think I see it now. The modem puts a space between the : and the
> > number, and the regex doesn't handle that:
> >
> > self->priv->rssi_regex = g_regex_new ("\\r\\n\\^RSSI:(\\d+)\\r\\n",
> >
> > so I think it should be a simple fixup to all the regexes in Huawei to
> > discard spaces ("\\s*") between the : and the interesting stuff?
> >
> > Dan
> >
> > > <debug> [000027.596303] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > --> 'AT^NDISSTATQRY?<CR>'
> > > <debug> [000027.617122] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > <-- '<CR><LF>^NDISSTATQRY: 0,,,"IPV4"<CR><LF><CR><LF>OK<CR><LF>'
> > > <debug> [000027.617316] [mm-serial-port.c:1018] mm_serial_port_close():
> > > (ttyUSB0) device open count is 1 (close)
> > > <debug> [000028.561321] [mm-serial-port.c:972] mm_serial_port_open():
> > > (ttyUSB0) device open count is 2 (open)
> > > <debug> [000028.561476] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > --> 'AT^NDISSTATQRY?<CR>'
> > > <debug> [000028.584342] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > <-- '<CR><LF>^NDISSTATQRY: 0,,,"IPV4"<CR><LF><CR><LF>OK<CR><LF>'
> > > <debug> [000028.584642] [mm-serial-port.c:1018] mm_serial_port_close():
> > > (ttyUSB0) device open count is 1 (close)
> > > <debug> [000029.218829] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > <-- '<CR><LF>^RSSI: 16<CR><LF><CR><LF>^CSNR:-87,-5<CR><LF>'
> > > <debug> [000029.561465] [mm-serial-port.c:972] mm_serial_port_open():
> > > (ttyUSB0) device open count is 2 (open)
> > > <debug> [000029.561841] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > --> 'AT^NDISSTATQRY?<CR>'
> > > <debug> [000029.582647] [mm-at-serial-port.c:408] debug_log(): (ttyUSB0):
> > > <-- '<CR><LF>^NDISSTATQRY: 0,,,"IPV4"<CR><LF><CR><LF>OK<CR><LF>'
> > > <debug> [000029.582735] [huawei/mm-modem-helpers-huawei.c:42]
> > > mm_huawei_parse_ndisstatqry_response(): Missing ^NDISSTATQRY prefix:
> > > '^RSSI: 16^M
> > > ^M
> > > ^NDISSTATQRY: 0,,,"IPV4"'
> > > <debug> [000029.582747] [huawei/mm-broadband-bearer-huawei.c:133]
> > > connect_ndisstatqry_check_ready(): Modem doesn't properly support
> > > ^NDISSTATQRY command: Missing ^NDISSTATQRY prefix
> > > <debug> [000029.582767] [mm-serial-port.c:1018] mm_serial_port_close():
> > > (ttyUSB0) device open count is 1 (close)
> > > <debug> [000029.582865] [mm-bearer.c:461] connect_ready(): Couldn't
> > connect
> > > bearer '/org/freedesktop/ModemManager1/Bearer/0': 'Connection attempt not
> > > supported'
> > > <info> [000029.582960] [mm-iface-modem.c:1203]
> > > __iface_modem_update_state_internal(): Modem
> > > /org/freedesktop/ModemManager1/Modem/0: state changed (connecting ->
> > > registered)
> > > <debug> [000029.583709] [mm-iface-modem-simple.c:221]
> > > connect_bearer_ready(): Couldn't connect bearer: 'Connection attempt not
> > > supported'
> > >
> > > Thanks,
> > > Ben
> > >
> > > On Mon, Aug 5, 2013 at 9:08 PM, Dan Williams <dcbw at redhat.com> wrote:
> > >
> > > > On Mon, 2013-08-05 at 19:46 -0700, Ben Chan wrote:
> > > > > This patch modifies mm_huawei_parse_ndisstatqry_response() to
> > properly
> > > > > handle a ^NDISSTATQRY? response when it is mixed with another
> > unsolicited
> > > > > response, e.g.
> > > > >
> > > > > ^RSSI: 16\r\n^NDISSTATQRY: 0,,,"IPV4"
> > > >
> > > > We'd expect the unsolicited responses to be cleared out of the response
> > > > string already by the serial parser. Is that not happening? At least
> > > > for ^RSSI, the regex expects a leading \r\n though. Are there any logs
> > > > you've got that show a more complete command sequence?
> > > >
> > > > Dan
> > > >
> > > > > This patch is originally developed by:
> > > > > Franko Fang <fangxiaozhi at huawei.com>
> > > > >
> > > > > And then revised by:
> > > > > Ben Chan <benchan at chromium.org>
> > > > > ---
> > > > > plugins/huawei/mm-modem-helpers-huawei.c | 6 +-
> > > > > plugins/huawei/tests/test-modem-helpers-huawei.c | 70
> > > > +++++++++++++++---------
> > > > > 2 files changed, 48 insertions(+), 28 deletions(-)
> > > > >
> > > > > diff --git a/plugins/huawei/mm-modem-helpers-huawei.c
> > > > b/plugins/huawei/mm-modem-helpers-huawei.c
> > > > > index d9b038d..e1e05c4 100644
> > > > > --- a/plugins/huawei/mm-modem-helpers-huawei.c
> > > > > +++ b/plugins/huawei/mm-modem-helpers-huawei.c
> > > > > @@ -20,6 +20,7 @@
> > > > > #define _LIBMM_INSIDE_MM
> > > > > #include <libmm-glib.h>
> > > > >
> > > > > +#include "mm-log.h"
> > > > > #include "mm-modem-helpers-huawei.h"
> > > > >
> > > > >
> > > >
> > /*****************************************************************************/
> > > > > @@ -37,7 +38,10 @@ mm_huawei_parse_ndisstatqry_response (const gchar
> > > > *response,
> > > > > GMatchInfo *match_info;
> > > > > GError *inner_error = NULL;
> > > > >
> > > > > - if (!response || !g_str_has_prefix (response, "^NDISSTATQRY:"))
> > {
> > > > > + if (response)
> > > > > + response = g_strstr_len (response, -1, "^NDISSTATQRY:");
> > > > > +
> > > > > + if (!response) {
> > > > > g_set_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> > > > "Missing ^NDISSTATQRY prefix");
> > > > > return FALSE;
> > > > > }
> > > > > diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c
> > > > b/plugins/huawei/tests/test-modem-helpers-huawei.c
> > > > > index c1bc0ed..5e2e83d 100644
> > > > > --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
> > > > > +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
> > > > > @@ -17,6 +17,9 @@
> > > > > #include <glib-object.h>
> > > > > #include <locale.h>
> > > > >
> > > > > +#include <ModemManager.h>
> > > > > +
> > > > > +#include "mm-errors-types.h"
> > > > > #include "mm-modem-helpers-huawei.h"
> > > > >
> > > > >
> > > >
> > /*****************************************************************************/
> > > > > @@ -24,6 +27,7 @@
> > > > >
> > > > > typedef struct {
> > > > > const gchar *str;
> > > > > + gboolean expected_result;
> > > > > gboolean expected_ipv4_available;
> > > > > gboolean expected_ipv4_connected;
> > > > > gboolean expected_ipv6_available;
> > > > > @@ -31,39 +35,48 @@ typedef struct {
> > > > > } NdisstatqryTest;
> > > > >
> > > > > static const NdisstatqryTest ndisstatqry_tests[] = {
> > > > > - { "^NDISSTATQRY: 1,,,IPV4\r\n", TRUE, TRUE, FALSE, FALSE },
> > > > > - { "^NDISSTATQRY: 0,,,IPV4\r\n", TRUE, FALSE, FALSE, FALSE },
> > > > > - { "^NDISSTATQRY: 1,,,IPV6\r\n", FALSE, FALSE, TRUE, TRUE },
> > > > > - { "^NDISSTATQRY: 0,,,IPV6\r\n", FALSE, FALSE, TRUE, FALSE },
> > > > > + { "^NDISSTATQRY: 1,,,IPV4\r\n", TRUE, TRUE, TRUE, FALSE,
> > FALSE },
> > > > > + { "^NDISSTATQRY: 0,,,IPV4\r\n", TRUE, TRUE, FALSE, FALSE,
> > FALSE },
> > > > > + { "^NDISSTATQRY: 1,,,IPV6\r\n", TRUE, FALSE, FALSE, TRUE,
> > TRUE },
> > > > > + { "^NDISSTATQRY: 0,,,IPV6\r\n", TRUE, FALSE, FALSE, TRUE,
> > FALSE },
> > > > > { "^NDISSTATQRY: 1,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 1,,,IPV6\r\n", TRUE, TRUE, TRUE, TRUE },
> > > > > + "^NDISSTATQRY: 1,,,IPV6\r\n", TRUE, TRUE, TRUE, TRUE,
> > TRUE },
> > > > > { "^NDISSTATQRY: 1,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 0,,,IPV6\r\n", TRUE, TRUE, TRUE, FALSE },
> > > > > + "^NDISSTATQRY: 0,,,IPV6\r\n", TRUE, TRUE, TRUE, TRUE,
> > FALSE },
> > > > > { "^NDISSTATQRY: 0,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 1,,,IPV6\r\n", TRUE, FALSE, TRUE, TRUE },
> > > > > + "^NDISSTATQRY: 1,,,IPV6\r\n", TRUE, TRUE, FALSE, TRUE,
> > TRUE },
> > > > > { "^NDISSTATQRY: 0,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 0,,,IPV6\r\n", TRUE, FALSE, TRUE, FALSE },
> > > > > - { "^NDISSTATQRY: 1,,,IPV4", TRUE, TRUE, FALSE, FALSE },
> > > > > - { "^NDISSTATQRY: 0,,,IPV4", TRUE, FALSE, FALSE, FALSE },
> > > > > - { "^NDISSTATQRY: 1,,,IPV6", FALSE, FALSE, TRUE, TRUE },
> > > > > - { "^NDISSTATQRY: 0,,,IPV6", FALSE, FALSE, TRUE, FALSE },
> > > > > + "^NDISSTATQRY: 0,,,IPV6\r\n", TRUE, TRUE, FALSE, TRUE,
> > FALSE },
> > > > > + { "^NDISSTATQRY: 1,,,IPV4", TRUE, TRUE, TRUE, FALSE,
> > FALSE },
> > > > > + { "^NDISSTATQRY: 0,,,IPV4", TRUE, TRUE, FALSE, FALSE,
> > FALSE },
> > > > > + { "^NDISSTATQRY: 1,,,IPV6", TRUE, FALSE, FALSE, TRUE,
> > TRUE },
> > > > > + { "^NDISSTATQRY: 0,,,IPV6", TRUE, FALSE, FALSE, TRUE,
> > FALSE },
> > > > > { "^NDISSTATQRY: 1,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 1,,,IPV6", TRUE, TRUE, TRUE, TRUE },
> > > > > + "^NDISSTATQRY: 1,,,IPV6", TRUE, TRUE, TRUE, TRUE,
> > TRUE },
> > > > > { "^NDISSTATQRY: 1,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 0,,,IPV6", TRUE, TRUE, TRUE, FALSE },
> > > > > + "^NDISSTATQRY: 0,,,IPV6", TRUE, TRUE, TRUE, TRUE,
> > FALSE },
> > > > > { "^NDISSTATQRY: 0,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 1,,,IPV6", TRUE, FALSE, TRUE, TRUE },
> > > > > + "^NDISSTATQRY: 1,,,IPV6", TRUE, TRUE, FALSE, TRUE,
> > TRUE },
> > > > > { "^NDISSTATQRY: 0,,,IPV4\r\n"
> > > > > - "^NDISSTATQRY: 0,,,IPV6", TRUE, FALSE, TRUE, FALSE },
> > > > > - { NULL, FALSE, FALSE, FALSE, FALSE }
> > > > > + "^NDISSTATQRY: 0,,,IPV6", TRUE, TRUE, FALSE, TRUE,
> > FALSE },
> > > > > + { "^RSSI: 16\r\n"
> > > > > + "^NDISSTATQRY: 0,,,IPV4", TRUE, TRUE, FALSE, FALSE,
> > FALSE
> > > > },
> > > > > + { "^RSSI: 16\r\n"
> > > > > + "^NDISSTATQRY: 0,,,IPV4\r\n"
> > > > > + "^NDISSTATQRY: 0,,,IPV6", TRUE, TRUE, FALSE, TRUE,
> > FALSE },
> > > > > + { "^RSSI: 16\r\n", FALSE, FALSE, FALSE, FALSE,
> > FALSE },
> > > > > + { "OK\r\n", FALSE, FALSE, FALSE, FALSE,
> > FALSE },
> > > > > + { "\r\n", FALSE, FALSE, FALSE, FALSE,
> > FALSE },
> > > > > + { "", FALSE, FALSE, FALSE, FALSE,
> > FALSE },
> > > > > + { NULL, FALSE, FALSE, FALSE, FALSE,
> > FALSE }
> > > > > };
> > > > >
> > > > > static void
> > > > > test_ndisstatqry (void)
> > > > > {
> > > > > - guint i;
> > > > > + size_t i;
> > > > >
> > > > > - for (i = 0; ndisstatqry_tests[i].str; i++) {
> > > > > + for (i = 0; i < sizeof (ndisstatqry_tests) / sizeof
> > > > (ndisstatqry_tests[0]); i++) {
> > > > > GError *error = NULL;
> > > > > gboolean ipv4_available;
> > > > > gboolean ipv4_connected;
> > > > > @@ -76,15 +89,18 @@ test_ndisstatqry (void)
> > > > > &ipv4_connected,
> > > > > &ipv6_available,
> > > > > &ipv6_connected,
> > > > > - &error) == TRUE);
> > > > > - g_assert_no_error (error);
> > > > > + &error) ==
> > ndisstatqry_tests[i].expected_result);
> > > > > + if (ndisstatqry_tests[i].expected_result) {
> > > > > + g_assert_no_error (error);
> > > > > + g_assert (ipv4_available ==
> > > > ndisstatqry_tests[i].expected_ipv4_available);
> > > > > + if (ipv4_available)
> > > > > + g_assert (ipv4_connected ==
> > > > ndisstatqry_tests[i].expected_ipv4_connected);
> > > > > + g_assert (ipv6_available ==
> > > > ndisstatqry_tests[i].expected_ipv6_available);
> > > > > + if (ipv6_available)
> > > > > + g_assert (ipv6_connected ==
> > > > ndisstatqry_tests[i].expected_ipv6_connected);
> > > > > + } else
> > > > > + g_assert_error (error, MM_CORE_ERROR,
> > MM_CORE_ERROR_FAILED);
> > > > >
> > > > > - g_assert (ipv4_available ==
> > > > ndisstatqry_tests[i].expected_ipv4_available);
> > > > > - if (ipv4_available)
> > > > > - g_assert (ipv4_connected ==
> > > > ndisstatqry_tests[i].expected_ipv4_connected);
> > > > > - g_assert (ipv6_available ==
> > > > ndisstatqry_tests[i].expected_ipv6_available);
> > > > > - if (ipv6_available)
> > > > > - g_assert (ipv6_connected ==
> > > > ndisstatqry_tests[i].expected_ipv6_connected);
> > > > > }
> > > > > }
> > > > >
> > > >
> > > >
> > > >
> > > _______________________________________________
> > > ModemManager-devel mailing list
> > > ModemManager-devel at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
> >
> >
> >
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
More information about the ModemManager-devel
mailing list