fix for +CMER parser of AT driver (fixes registration)

Hi All,

i recently noticed that the at driver did not reach the state "registered". marcel pointer me to the message

+CMER not supported by this modem. If this is an error please submit patches to support this hardware

and i started digging. problem was parsing of the response

	+CMER: (0,3),(0,2),0,(0-1),0

that expresses the allowed options of CMER. neither (a list of) single integer "(0,3)" (in opposite to range "(0-1)") nor missing brackets "0" were supported by ofono so i added this (see patch).

i'm not sure if missing brackets are allowed or espress something different, maybe a AT-guru can tell. for now "(0)" behaves like "0".

i assume that also other response-parsing flows in ofono have that problem so either a generic aproach should be implemented or other places be reviewed.

marcel already pointer out that a generic aproach may be unnecessarily complex where i agree if it comes to strings and nested brackets but for plain integer cases it would be quite useful to get this done in one place for all. if interested i could implement such stuff into gatchat/gatresult.c which then can be used for the integer-only cases. what do you think?

regards,
michael

M. Dietrich

From: "M. Dietrich" <[email protected]>
Date: Sun, 30 Dec 2012 21:26:33 +0100
Subject: [PATCH] enhance parser for CMER response

---
 drivers/atmodem/network-registration.c |   39 ++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 10 deletions(-)

index 19b19b2..3128d5f 100644
@@ -1615,26 +1615,45 @@ static void at_cmer_query_cb(ofono_bool_t ok, GAtResult *result,
 
 	g_at_result_iter_init(&iter, result);
 
-	if (!g_at_result_iter_next(&iter, "+CMER:"))
+	if (!g_at_result_iter_next(&iter, "+CMER:")) {
+		DBG("parsing error, probably begin tag not found");
 		goto error;
+	}
 
 	for (opt = 0; opt < cmer_opts_cnt; opt++) {
 		int min, max;
 
-		if (!g_at_result_iter_open_list(&iter))
-			goto error;
-
-		while (g_at_result_iter_next_range(&iter, &min, &max)) {
-			for (mode = min; mode <= max; mode++)
-				cmer_opts[opt] |= 1 << mode;
+		if (!g_at_result_iter_open_list(&iter)) {
+			max = -1;
+			while (g_at_result_iter_next_number(&iter, &min)) {
+				if (max < 0)
+					max = min;
+				for (mode = min; mode <= max; mode++)
+					cmer_opts[opt] |= 1 << mode;
+			}
 		}
+		else
+		{
+			max = -1;
+			while (g_at_result_iter_next_range(&iter, &min, &max)
+			|| g_at_result_iter_next_number(&iter, &min)) {
+				if (max < 0)
+					max = min;
+				for (mode = min; mode <= max; mode++)
+					cmer_opts[opt] |= 1 << mode;
+			}
 
-		if (!g_at_result_iter_close_list(&iter))
-			goto error;
+			if (!g_at_result_iter_close_list(&iter)) {
+				DBG("parsing error: end not found");
+				goto error;
+			}
+		}
 	}
 
-	if (build_cmer_string(buf, cmer_opts, nd) == FALSE)
+	if (build_cmer_string(buf, cmer_opts, nd) == FALSE) {
+		DBG("could not build cmer");
 		goto error;
+	}
 
 	g_at_chat_send(nd->chat, buf, cmer_prefix,
 			at_cmer_set_cb, netreg, NULL);

1.7.10.4

Hi Michael,

This is quite obviously wrong. Quoting v.250:

"When the action accepts a single numeric subparameter, or the parameter accepts only one numeric value, the set of supported values may be presented in the information text as an ordered list of values. The list shall be preceded by a left parenthesis ("(", IA5 2/8), and is followed by a right parenthesis (")", IA5 2/9). If only a single value is supported, it shall appear between the parentheses. If more than one value is supported, then the values may be listed individually, separated by comma characters (IA5 2/12), or, when a continuous range of values is supported, by the first value in the range, followed by a hyphen character (IA5 2/13), followed by the last value in the range. The specification of single values and ranges of values may be intermixed within a single information text. In all cases, the supported values shall be indicated in ascending order."

I'm pretty sure that this is supported by GAtChat.

It is only a problem if the modem is broken, in which case you need to use a quirk and parsing the result of CMER=? is likely unnecessary. For broken devices it is likely easier to simply skip CMER probing and just set the wanted CMER settings.

Not sure what you're asking here, can you elaborate?

Regards,
-Denis

Hi,

i assume more or less all modems are in one or the other way "broken". even the spec is (in my opinion) broken.

so if only the output of an option list just does not follow the spec but most other stuff of the modem works fine why not be more tolerant parsing the option list?

if you instead have a quirk for this and only this modem you fixed only a single one. i assume that also other modems are broken in this way and fixing it in the parser would enable more modems to work.

the AT parser code for the option list is redundant, a re-use of a single implementation would make some sense and i offered a more generic way of parsing an option list. marcel pointed out that a completly generic parser would be unnecessarily complex and i agree. so i suggest a re-usable code for integer option lists.

anyway i would like to see the patch in ofono. on the one side it does not break any existing modem, on the other side it makes some modem work.

regards,
michael

M. Dietrich

Hi,

Actually we do not want to be tolerant. If something is broken, we want to make sure that any work-around code-path is explicitly called out. There are many reasons for this, one of the main ones being code efficiency. The other code readability. Over time such 'workarounds' pollute the code base and make it ugly; and the reasons for why it is that way are either lost or become murky. Case in point your patch, which doesn't even contain a code comment block on what it is trying to accomplish. (And by the way your coding style is still wrong in several places...)

I do not accept your assumption. We have been using almost the exact same structure for parsing parameter lists in the SMS driver for over 3 years. So far you are the first to report a modem that behaves in this manner when it comes to parameter lists.

Regards,
-Denis

Hi Denis,

i understood the response of the modem follows the spec so i thought this would be standard. but you are deeper in the specs and i won't tell otherwise.

i just tried to intuitively follow the code standard i found in ofono, sorry if i got that wrong.

yes, because i am probably the first that investigated the problem in depth. i saw alot of complains that the registration state is not reached from many ofono users. cause for that is exactly the +CMER answer of those modems.

as the modem in question is used in many thinkpads and other notebooks i assume that the many people can't use ofono out of this reason.

so what do you suggest to support this modem? how would a quirk as you suggested look like?

regards,
michael

M. Dietrich

Hi Michael,

Please understand, I'm not denying there's a problem, just that it is the first I've heard of it. Please mention these reports on IRC or the Mailing List to me in the future so we can get them addressed quicker.

Can you actually tell me what this modem is? Maybe I can test it on my end?

The CMER parser was introduced in commit b87619. It was essentially a straight forward port from the SMS driver and meant to fix the particular behaviors of the Telit devices. This was early September, surprising that no one complained in 4 months...

The way I suggest to fix it would be to skip probing CMER if a quirk is enabled for your particular device and simply send the appropriate CMER command. Prior to September oFono simply used to send 'AT+CMER=3,0,0,1'

Regards,
-Denis