* [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status @ 2010-12-03 15:20 Tonny Tzeng 2010-12-07 3:41 ` Denis Kenzior 0 siblings, 1 reply; 5+ messages in thread From: Tonny Tzeng @ 2010-12-03 15:20 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2752 bytes --] This patch is to skip unsolicited CGREG while checking the GPRS attach status, especially for Huawei modem which sends lac and ci strings in unquoted format, and casues at_util_parse_reg() return lac value as status. --- drivers/atmodem/atutil.c | 12 ++++++++---- gatchat/gatresult.c | 5 +++++ gatchat/gatresult.h | 1 + 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c index b6f0d92..c190aa2 100644 --- a/drivers/atmodem/atutil.c +++ b/drivers/atmodem/atutil.c @@ -243,13 +243,11 @@ gboolean at_util_parse_reg(GAtResult *result, const char *prefix, if (g_at_result_iter_next_number(&iter, &s) == FALSE) continue; - /* Some firmware will report bogus lac/ci when unregistered */ - if (s != 1 && s != 5) - goto out; - switch (vendor) { case OFONO_VENDOR_HUAWEI: case OFONO_VENDOR_NOVATEL: + if (g_at_result_iter_eol(&iter)) + continue; r = g_at_result_iter_next_unquoted_string(&iter, &str); if (r == TRUE) @@ -257,6 +255,8 @@ gboolean at_util_parse_reg(GAtResult *result, const char *prefix, else goto out; + if (g_at_result_iter_eol(&iter)) + continue; r = g_at_result_iter_next_unquoted_string(&iter, &str); if (r == TRUE) @@ -266,11 +266,15 @@ gboolean at_util_parse_reg(GAtResult *result, const char *prefix, break; default: + if (g_at_result_iter_eol(&iter)) + continue; if (g_at_result_iter_next_string(&iter, &str) == TRUE) l = strtol(str, NULL, 16); else goto out; + if (g_at_result_iter_eol(&iter)) + continue; if (g_at_result_iter_next_string(&iter, &str) == TRUE) c = strtol(str, NULL, 16); else diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c index 8a6dfae..c919305 100644 --- a/gatchat/gatresult.c +++ b/gatchat/gatresult.c @@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result) iter->line_pos = 0; } +gboolean g_at_result_iter_eol(GAtResultIter *iter) +{ + return iter->line_pos >= strlen(iter->l->data); +} + gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix) { char *line; diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h index a74741f..33c0b2c 100644 --- a/gatchat/gatresult.h +++ b/gatchat/gatresult.h @@ -46,6 +46,7 @@ struct _GAtResultIter { typedef struct _GAtResultIter GAtResultIter; void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result); +gboolean g_at_result_iter_eol(GAtResultIter *iter); gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix); gboolean g_at_result_iter_open_list(GAtResultIter *iter); -- 1.7.2.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status 2010-12-03 15:20 [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status Tonny Tzeng @ 2010-12-07 3:41 ` Denis Kenzior 2010-12-11 14:09 ` Tonny Tzeng 0 siblings, 1 reply; 5+ messages in thread From: Denis Kenzior @ 2010-12-07 3:41 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 3477 bytes --] On 12/03/2010 09:20 AM, Tonny Tzeng wrote: > This patch is to skip unsolicited CGREG while checking the GPRS attach > status, especially for Huawei modem which sends lac and ci strings in > unquoted format, and casues at_util_parse_reg() return lac value as > status. > > --- > drivers/atmodem/atutil.c | 12 ++++++++---- > gatchat/gatresult.c | 5 +++++ > gatchat/gatresult.h | 1 + We like to see patches split on a top level directory boundary. Please see the 'Submitting patches' section in the HACKING document. > 3 files changed, 14 insertions(+), 4 deletions(-) > > diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c > index b6f0d92..c190aa2 100644 > --- a/drivers/atmodem/atutil.c > +++ b/drivers/atmodem/atutil.c > @@ -243,13 +243,11 @@ gboolean at_util_parse_reg(GAtResult *result, > const char *prefix, > if (g_at_result_iter_next_number(&iter, &s) == FALSE) > continue; > > - /* Some firmware will report bogus lac/ci when unregistered */ > - if (s != 1 && s != 5) > - goto out; > - Skipping this check is still a bad idea. > switch (vendor) { > case OFONO_VENDOR_HUAWEI: > case OFONO_VENDOR_NOVATEL: > + if (g_at_result_iter_eol(&iter)) > + continue; Coding style item M1. > r = g_at_result_iter_next_unquoted_string(&iter, &str); > > if (r == TRUE) > @@ -257,6 +255,8 @@ gboolean at_util_parse_reg(GAtResult *result, const > char *prefix, > else > goto out; > > + if (g_at_result_iter_eol(&iter)) > + continue; Coding style item M1. > r = g_at_result_iter_next_unquoted_string(&iter, &str); > > if (r == TRUE) > @@ -266,11 +266,15 @@ gboolean at_util_parse_reg(GAtResult *result, > const char *prefix, > > break; > default: > + if (g_at_result_iter_eol(&iter)) > + continue; Coding style item M1. And continuing is not the right thing to do here > if (g_at_result_iter_next_string(&iter, &str) == TRUE) > l = strtol(str, NULL, 16); > else > goto out; > > + if (g_at_result_iter_eol(&iter)) > + continue; Ditto > if (g_at_result_iter_next_string(&iter, &str) == TRUE) > c = strtol(str, NULL, 16); > else > diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c > index 8a6dfae..c919305 100644 > --- a/gatchat/gatresult.c > +++ b/gatchat/gatresult.c > @@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter, > GAtResult *result) > iter->line_pos = 0; > } > > +gboolean g_at_result_iter_eol(GAtResultIter *iter) > +{ > + return iter->line_pos >= strlen(iter->l->data); > +} > + And I pretty much don't see the need for this. It doesn't really help you here. > gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix) > { > char *line; > diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h > index a74741f..33c0b2c 100644 > --- a/gatchat/gatresult.h > +++ b/gatchat/gatresult.h > @@ -46,6 +46,7 @@ struct _GAtResultIter { > typedef struct _GAtResultIter GAtResultIter; > > void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result); > +gboolean g_at_result_iter_eol(GAtResultIter *iter); > > gboolean g_at_result_iter_next(GAtResultIter *iter, const char > *prefix); > gboolean g_at_result_iter_open_list(GAtResultIter *iter); Anyhow, can you please try the attached patch and see if it solves your problems. Regards, -Denis [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-atutil-Fix-parsing-of-un-quoted-CREG-CGREG.patch --] [-- Type: text/x-patch, Size: 1462 bytes --] >From f5473c398a2e29a98c74ad4c98a427564e25d9d0 Mon Sep 17 00:00:00 2001 From: Denis Kenzior <denkenz@gmail.com> Date: Mon, 6 Dec 2010 21:36:54 -0600 Subject: [PATCH] atutil: Fix parsing of un-quoted CREG / CGREG On broken hardware like the Huawei, it is possible to receive both an unsolicited and a solicited version of the CREG / CGREG within within the same response set. Skipping of the unsolicited version was not handled correctly. This attempts to fix this issue. --- drivers/atmodem/atutil.c | 19 +++++++++++++++++-- 1 files changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c index b6f0d92..427b098 100644 --- a/drivers/atmodem/atutil.c +++ b/drivers/atmodem/atutil.c @@ -240,8 +240,23 @@ gboolean at_util_parse_reg(GAtResult *result, const char *prefix, g_at_result_iter_next_number(&iter, &m); /* Sometimes we get an unsolicited CREG/CGREG here, skip it */ - if (g_at_result_iter_next_number(&iter, &s) == FALSE) - continue; + switch (vendor) { + case OFONO_VENDOR_HUAWEI: + case OFONO_VENDOR_NOVATEL: + r = g_at_result_iter_next_unquoted_string(&iter, &str); + + if (r == FALSE || strlen(str) != 1) + continue; + + s = strtol(str, NULL, 10); + + break; + default: + if (g_at_result_iter_next_number(&iter, &s) == FALSE) + continue; + + break; + } /* Some firmware will report bogus lac/ci when unregistered */ if (s != 1 && s != 5) -- 1.7.0.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status 2010-12-07 3:41 ` Denis Kenzior @ 2010-12-11 14:09 ` Tonny Tzeng 2010-12-11 14:52 ` Denis Kenzior 0 siblings, 1 reply; 5+ messages in thread From: Tonny Tzeng @ 2010-12-11 14:09 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2252 bytes --] > > > diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c > > index 8a6dfae..c919305 100644 > > --- a/gatchat/gatresult.c > > +++ b/gatchat/gatresult.c > > @@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter, > > GAtResult *result) > > iter->line_pos = 0; > > } > > > > +gboolean g_at_result_iter_eol(GAtResultIter *iter) > > +{ > > + return iter->line_pos >= strlen(iter->l->data); > > +} > > + > > And I pretty much don't see the need for this. It doesn't really help > you here. No, this function can help to ignore unsolicited response properly. I think the difference between solicited and unsolicited CGREG is the number of parameters after +CGREG:, so it would be nice if we have a utility function to know we are at the end of AT return string. This is what we want to parse the state info from the solicited response: +CGREG: <n>, <state>, <lac>, <ci> but in a race condition, we received this unsolicited response from Huawei modem: +CGREG: <state>, <lac>, <ci> In your proposed patch, it bypass the unsolicited response only if the 2nd number after +CGREG: is not single digit, however if the lac is single digit, we will still get the wrong state number. So I still believe we should check whether we are at the end of the return string before parsing the unquoted lac and ci. If we are at the end of the string, then this is a unsolicited code and we should ignore it. How do you think? > > gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix) > > { > > char *line; > > diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h > > index a74741f..33c0b2c 100644 > > --- a/gatchat/gatresult.h > > +++ b/gatchat/gatresult.h > > @@ -46,6 +46,7 @@ struct _GAtResultIter { > > typedef struct _GAtResultIter GAtResultIter; > > > > void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result); > > +gboolean g_at_result_iter_eol(GAtResultIter *iter); > > > > gboolean g_at_result_iter_next(GAtResultIter *iter, const char > > *prefix); > > gboolean g_at_result_iter_open_list(GAtResultIter *iter); > > Anyhow, can you please try the attached patch and see if it solves your > problems. > > Regards, > -Denis > [-- Attachment #2: attachment.html --] [-- Type: text/html, Size: 2906 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status 2010-12-11 14:09 ` Tonny Tzeng @ 2010-12-11 14:52 ` Denis Kenzior 2010-12-11 15:08 ` Tonny Tzeng 0 siblings, 1 reply; 5+ messages in thread From: Denis Kenzior @ 2010-12-11 14:52 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1023 bytes --] Hi Tonny, > In your proposed patch, it bypass the unsolicited response only if the > 2nd number after +CGREG: is not single digit, however if the lac is > single digit, we will still get the wrong state number. The lac is given as a 2 byte number and should always be printed as four hexadecimal letters. If this is not the case, then you should have a serious talk with your vendor. > > So I still believe we should check whether we are at the end of the > return string before parsing the unquoted lac and ci. If we are at the > end of the string, then this is a unsolicited code and we should ignore > it. How do you think? > I'm against this, if you really want to guard against this, then use the approach similar to comprehension_tlv_iter_copy in src/simutil.[ch]. You can use that to 'peek' ahead and figure out whether the response contains 3 or 4 elements. But again, you would not be having this issue in the first place if your vendor followed standards ;) Regards, -Denis ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status 2010-12-11 14:52 ` Denis Kenzior @ 2010-12-11 15:08 ` Tonny Tzeng 0 siblings, 0 replies; 5+ messages in thread From: Tonny Tzeng @ 2010-12-11 15:08 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 743 bytes --] Hi Denis, On Sat, Dec 11, 2010 at 10:52 PM, Denis Kenzior <denkenz@gmail.com> wrote: > Hi Tonny, > > > In your proposed patch, it bypass the unsolicited response only if the > > 2nd number after +CGREG: is not single digit, however if the lac is > > single digit, we will still get the wrong state number. > > The lac is given as a 2 byte number and should always be printed as four > hexadecimal letters. If this is not the case, then you should have a > serious talk with your vendor. > Well, if this (i.e. lac are 4 hex-digits) is the case, then I totally agree with you that it could ignore unsolicited response properly in at_util_parse_reg() for my case. Thank you so much for the coach :) Best Regards, Tonny [-- Attachment #2: attachment.html --] [-- Type: text/html, Size: 1054 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-12-11 15:08 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-03 15:20 [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status Tonny Tzeng 2010-12-07 3:41 ` Denis Kenzior 2010-12-11 14:09 ` Tonny Tzeng 2010-12-11 14:52 ` Denis Kenzior 2010-12-11 15:08 ` Tonny Tzeng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.