RE: Patch on unsupported AT command
-Original Message-
From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of
Marcel Holtmann
Sent: Monday, November 23, 2009 2:21 PM
To: ofono@ofono.org
Subject: RE: Patch on unsupported AT command
Hi Yang,
looks good so far, but ...
+static gboolean check_terminator(struct terminator_info *info, char
*line)
+{
+ if ((info-len == -1 !strcmp(line, info-terminator)) ||
+ (info-len 0 !strncmp(line, info-terminator,
info-len)))
+ return TRUE;
+ else
+ return FALSE;
+}
+
This is first of all violating the coding style with the indentation on
the second line of the if, but it is also way too complicated.
if (info-len == -1 !strcmp(line, info-terminator)
return TRUE;
if (info-len 0 !strncmp(line, info-terminator, ...))
return TRUE;
return FALSE;
Maybe it is too early in the morning to do code review, but this should
be doing the same, but be a lot simpler to read ;)
You're absolutely right. In this way, the code is more readable. Please review
again.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
0001-Framework-to-support-non-standard-terminator.patch
Description: 0001-Framework-to-support-non-standard-terminator.patch
0002-Support-Huawei-specific-terminator.patch
Description: 0002-Support-Huawei-specific-terminator.patch
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang, Maybe it is too early in the morning to do code review, but this should be doing the same, but be a lot simpler to read ;) You're absolutely right. In this way, the code is more readable. Please review again. both patches have been applied. Thanks. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Denis, This is first of all violating the coding style with the indentation on the second line of the if, but it is also way too complicated. There is actually a reason for this. if (info-len == -1 !strcmp(line, info-terminator) return TRUE; This part checks for static terminators, like OK or BUSY or ERROR. We do whole string comparison here. if (info-len 0 !strncmp(line, info-terminator, ...)) return TRUE; This part checks for variable terminators. E.g. +CMS ERROR: XXX. These are well defined by the standard so we only do a prefix comparison for these. and your point is? I was just going by the pure algorithmic of the if statement to make it actually readable without getting a headache ;) Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang,
looks good so far, but ...
+static gboolean check_terminator(struct terminator_info *info, char
*line)
+{
+ if ((info-len == -1 !strcmp(line, info-terminator)) ||
+ (info-len 0 !strncmp(line, info-terminator,
info-len)))
+ return TRUE;
+ else
+ return FALSE;
+}
+
This is first of all violating the coding style with the indentation on
the second line of the if, but it is also way too complicated.
if (info-len == -1 !strcmp(line, info-terminator)
return TRUE;
if (info-len 0 !strncmp(line, info-terminator, ...))
return TRUE;
return FALSE;
Maybe it is too early in the morning to do code review, but this should
be doing the same, but be a lot simpler to read ;)
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
-Original Message- From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of Marcel Holtmann Sent: Wednesday, November 18, 2009 4:36 AM To: ofono@ofono.org Subject: Re: Patch on unsupported AT command Hi Denis, + g_at_chat_add_terminator(chat, +EXT ERROR:, 11, FALSE); + g_at_chat_add_terminator(chat, +CME ERROR:, 11, FALSE); + g_at_chat_add_terminator(chat, +CMS ERROR:, 11, FALSE); + g_at_chat_add_terminator(chat, NO ANSWER, -1, FALSE); + g_at_chat_add_terminator(chat, CONNECT, -1, TRUE); + g_at_chat_add_terminator(chat, NO CARRIER, -1, FALSE); + g_at_chat_add_terminator(chat, BUSY, -1, FALSE); + g_at_chat_add_terminator(chat, NO DIALTONE, -1, FALSE); + g_at_chat_add_terminator(chat, ERROR, -1, FALSE); + g_at_chat_add_terminator(chat, OK, -1, TRUE); I really don't like this. Lets keep the non-standard terminators in a separate list. I don't want the vast majority of the drivers incurring the cost of multiple g_new/g_frees. I have to agree on this. We should keep the penalty for well behaving cards as small as possible. Thank you for the comments. Modified patches are attached! Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono Regards, -Yang 0001-Framework-to-support-non-standard-terminator.patch Description: 0001-Framework-to-support-non-standard-terminator.patch 0002-Support-Huawei-specific-terminator.patch Description: 0002-Support-Huawei-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
-Original Message- From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of Denis Kenzior Sent: Saturday, November 14, 2009 1:06 AM To: ofono@ofono.org Subject: Re: Patch on unsupported AT command Hi Marcel, Hi Denis, I think this might need a GAtChat quirk function where we can add extra terminator responses that will be recognized. And maybe even translated into something meaningful. Originally I had the terminators freely definable on the GAtChat + some hardcoded ones, but abandoned that approach since it didn't seem useful. Perhaps this needs to be resurrected. Denis, or do you want this quirked in every plugin or modem driver? I do prefer non-standard terminators to be setup in the plugin/driver. I meant adding something like g_at_chat_add_terminator() or so. Yes, that is what I meant as well. Attached is the patch to support plugin specific terminator, and it supports Huawei's special one COMMAND NOT SUPPORT. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono 0001-Support-vendor-specific-terminator.patch Description: 0001-Support-vendor-specific-terminator.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: Patch on unsupported AT command
Hi Yang, I think this might need a GAtChat quirk function where we can add extra terminator responses that will be recognized. And maybe even translated into something meaningful. Originally I had the terminators freely definable on the GAtChat + some hardcoded ones, but abandoned that approach since it didn't seem useful. Perhaps this needs to be resurrected. Denis, or do you want this quirked in every plugin or modem driver? I do prefer non-standard terminators to be setup in the plugin/driver. I meant adding something like g_at_chat_add_terminator() or so. Yes, that is what I meant as well. Attached is the patch to support plugin specific terminator, and it supports Huawei's special one COMMAND NOT SUPPORT. you need to split this into a GAtChat specific patch and oFono plugin patch. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Yang,
Thanks for the comments. I have split the patch to two separate ones.
Two problems:
+void g_at_chat_add_terminator(GAtChat *chat, const char *terminator,
+ int len, gboolean success)
+{
+ struct terminator_info *ti = g_new0(struct terminator_info, 1);
+ ti-terminator = terminator;
+ ti-len = len;
+ ti-success = success;
+ chat-terminator_table = g_slist_prepend(chat-terminator_table, ti);
+}
You're not strdup'ing the terminator string here. For safety reasons I
suggest this be done.
+ g_at_chat_add_terminator(chat, +EXT ERROR:, 11, FALSE);
+ g_at_chat_add_terminator(chat, +CME ERROR:, 11, FALSE);
+ g_at_chat_add_terminator(chat, +CMS ERROR:, 11, FALSE);
+ g_at_chat_add_terminator(chat, NO ANSWER, -1, FALSE);
+ g_at_chat_add_terminator(chat, CONNECT, -1, TRUE);
+ g_at_chat_add_terminator(chat, NO CARRIER, -1, FALSE);
+ g_at_chat_add_terminator(chat, BUSY, -1, FALSE);
+ g_at_chat_add_terminator(chat, NO DIALTONE, -1, FALSE);
+ g_at_chat_add_terminator(chat, ERROR, -1, FALSE);
+ g_at_chat_add_terminator(chat, OK, -1, TRUE);
I really don't like this. Lets keep the non-standard terminators in a
separate list. I don't want the vast majority of the drivers incurring the
cost of multiple g_new/g_frees.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Denis, +g_at_chat_add_terminator(chat, +EXT ERROR:, 11, FALSE); +g_at_chat_add_terminator(chat, +CME ERROR:, 11, FALSE); +g_at_chat_add_terminator(chat, +CMS ERROR:, 11, FALSE); +g_at_chat_add_terminator(chat, NO ANSWER, -1, FALSE); +g_at_chat_add_terminator(chat, CONNECT, -1, TRUE); +g_at_chat_add_terminator(chat, NO CARRIER, -1, FALSE); +g_at_chat_add_terminator(chat, BUSY, -1, FALSE); +g_at_chat_add_terminator(chat, NO DIALTONE, -1, FALSE); +g_at_chat_add_terminator(chat, ERROR, -1, FALSE); +g_at_chat_add_terminator(chat, OK, -1, TRUE); I really don't like this. Lets keep the non-standard terminators in a separate list. I don't want the vast majority of the drivers incurring the cost of multiple g_new/g_frees. I have to agree on this. We should keep the penalty for well behaving cards as small as possible. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Yang, If some unsupported AT command is issued, different modem may have their own response. Now at my hand is a Huawei modem (EM770W), and it returns COMMAND NOT SUPPORT. In my case, this modem doesn't support AT+CGAUTO=0 in atmodem/gprs.c. Current oFono will hang there for it's not a valid return. We may have some quirk to handle this problem, the same way as current code in network-registration.c with CALYPSO. But I wonder if it's better to add the response string into terminator table, so that we don't need this kind of quirk here and there. I'm not sure if this is the better/best way to handle this problem. After all, the table may become larger and larger is more and more specific terminator like this are added. Comments are welcome! this lovely broken Huawei modem where the firmware developers were incapable of reading the specification and just made up a new response. I think this might need a GAtChat quirk function where we can add extra terminator responses that will be recognized. And maybe even translated into something meaningful. Denis, or do you want this quirked in every plugin or modem driver? Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Denis, I think this might need a GAtChat quirk function where we can add extra terminator responses that will be recognized. And maybe even translated into something meaningful. Originally I had the terminators freely definable on the GAtChat + some hardcoded ones, but abandoned that approach since it didn't seem useful. Perhaps this needs to be resurrected. Denis, or do you want this quirked in every plugin or modem driver? I do prefer non-standard terminators to be setup in the plugin/driver. I meant adding something like g_at_chat_add_terminator() or so. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: Patch on unsupported AT command
Hi Marcel, Hi Denis, I think this might need a GAtChat quirk function where we can add extra terminator responses that will be recognized. And maybe even translated into something meaningful. Originally I had the terminators freely definable on the GAtChat + some hardcoded ones, but abandoned that approach since it didn't seem useful. Perhaps this needs to be resurrected. Denis, or do you want this quirked in every plugin or modem driver? I do prefer non-standard terminators to be setup in the plugin/driver. I meant adding something like g_at_chat_add_terminator() or so. Yes, that is what I meant as well. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
