Patch on unsupported AT command

Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

Hi all,
	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!

Regards,
-Yang


[-- Attachment #2: 0001-Add-terminator-for-unsupported-AT-command-of-Huawei.patch --]
[-- Type: application/octet-stream, Size: 784 bytes --]

From 6776f92d87afb56b11dc8b44a153eb46bf03f7da Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Fri, 13 Nov 2009 14:48:58 +0800
Subject: [PATCH] Add terminator for unsupported AT command of Huawei modem

---
 gatchat/gatchat.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..88811ba 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -375,7 +375,8 @@ static struct terminator_info terminator_table[] = {
 	{ "NO ANSWER", -1, FALSE },
 	{ "+CMS ERROR:", 11, FALSE },
 	{ "+CME ERROR:", 11, FALSE },
-	{ "+EXT ERROR:", 11, FALSE }
+	{ "+EXT ERROR:", 11, FALSE },
+	{ "COMMAND NOT SUPPORT", -1, FALSE},
 };
 
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
-- 
1.6.2.5

Re: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

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

Re: Patch on unsupported AT command

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Hi Marcel,

> 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.

Regards,
-Denis

Re: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

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

Re: Patch on unsupported AT command

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

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

RE: Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
>Denis Kenzior
>Sent: Saturday, November 14, 2009 1:06 AM
>To: ofono(a)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(a)ofono.org
>http://lists.ofono.org/listinfo/ofono

[-- Attachment #2: 0001-Support-vendor-specific-terminator.patch --]
[-- Type: application/octet-stream, Size: 3963 bytes --]

From adb87b83c3ccd6e5dbfafccfde39f5bc9f73722c Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Tue, 17 Nov 2009 17:30:17 +0800
Subject: [PATCH] Support vendor specific terminator

---
 gatchat/gatchat.c |   44 ++++++++++++++++++++++++++++----------------
 gatchat/gatchat.h |    3 +++
 plugins/huawei.c  |    1 +
 3 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..f9238f8 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -88,6 +88,7 @@ struct _GAtChat {
 	GTimer *wakeup_timer;			/* Keep track of elapsed time */
 	GAtSyntax *syntax;
 	gboolean destroyed;			/* Re-entrancy guard */
+	GSList *terminator_table;
 };
 
 static gint at_notify_node_compare_by_id(gconstpointer a, gconstpointer b)
@@ -251,6 +252,10 @@ static void g_at_chat_cleanup(GAtChat *chat)
 	chat->syntax = NULL;
 
 	chat->channel = NULL;
+
+	g_slist_foreach(chat->terminator_table, (GFunc)g_free, NULL);
+	g_slist_free(chat->terminator_table);
+	chat->terminator_table = NULL;
 }
 
 static void read_watcher_destroy_notify(GAtChat *chat)
@@ -365,29 +370,25 @@ struct terminator_info {
 	gboolean success;
 };
 
-static struct terminator_info terminator_table[] = {
-	{ "OK", -1, TRUE },
-	{ "ERROR", -1, FALSE },
-	{ "NO DIALTONE", -1, FALSE },
-	{ "BUSY", -1, FALSE },
-	{ "NO CARRIER", -1, FALSE },
-	{ "CONNECT", -1, TRUE },
-	{ "NO ANSWER", -1, FALSE },
-	{ "+CMS ERROR:", 11, FALSE },
-	{ "+CME ERROR:", 11, FALSE },
-	{ "+EXT ERROR:", 11, FALSE }
-};
+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);
+}
 
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
 							struct at_command *cmd,
 							char *line)
 {
-	int i;
-	int size = sizeof(terminator_table) / sizeof(struct terminator_info);
+	GSList *l;
 	int hint;
 
-	for (i = 0; i < size; i++) {
-		struct terminator_info *info = &terminator_table[i];
+	for (l = p->terminator_table; l; l = l->next) {
+		struct terminator_info *info = l->data;
 
 		if (info->len == -1 && !strcmp(line, info->terminator)) {
 			g_at_chat_finish_command(p, info->success, line);
@@ -951,6 +952,17 @@ GAtChat *g_at_chat_new(GIOChannel *channel, GAtSyntax *syntax)
 
 	chat->syntax = g_at_syntax_ref(syntax);
 
+	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);
+
 	return chat;
 
 error:
diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
index fe5b97b..b191f1b 100644
--- a/gatchat/gatchat.h
+++ b/gatchat/gatchat.h
@@ -128,6 +128,9 @@ gboolean g_at_chat_unregister(GAtChat *chat, guint id);
 gboolean g_at_chat_set_wakeup_command(GAtChat *chat, const char *cmd,
 					guint timeout, guint msec);
 
+void g_at_chat_add_terminator(GAtChat *chat, const char *terminator,
+				int len, gboolean success);
+
 
 #ifdef __cplusplus
 }
diff --git a/plugins/huawei.c b/plugins/huawei.c
index 28b7650..df8ef40 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,7 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	syntax = g_at_syntax_new_gsmv1();
 	data->chat = g_at_chat_new(channel, syntax);
+	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-- 
1.6.2.5

RE: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

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

RE: Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

Hi,

>> 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.

Thanks for the comments. I have split the patch to two separate ones. 

>
>Regards
>
>Marcel
>
>
>_______________________________________________
>ofono mailing list
>ofono(a)ofono.org
>http://lists.ofono.org/listinfo/ofono

Regards,
-Yang

[-- Attachment #2: 0001-Framework-to-support-standard-and-non-standard-termi.patch --]
[-- Type: application/octet-stream, Size: 3545 bytes --]

From 066402774c0a61cf64215dbf760d7b66d76ad498 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Wed, 18 Nov 2009 00:21:40 +0800
Subject: [PATCH 1/2] Framework to support standard and non-standard terminators

---
 gatchat/gatchat.c |   44 ++++++++++++++++++++++++++++----------------
 gatchat/gatchat.h |    3 +++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..f9238f8 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -88,6 +88,7 @@ struct _GAtChat {
 	GTimer *wakeup_timer;			/* Keep track of elapsed time */
 	GAtSyntax *syntax;
 	gboolean destroyed;			/* Re-entrancy guard */
+	GSList *terminator_table;
 };
 
 static gint at_notify_node_compare_by_id(gconstpointer a, gconstpointer b)
@@ -251,6 +252,10 @@ static void g_at_chat_cleanup(GAtChat *chat)
 	chat->syntax = NULL;
 
 	chat->channel = NULL;
+
+	g_slist_foreach(chat->terminator_table, (GFunc)g_free, NULL);
+	g_slist_free(chat->terminator_table);
+	chat->terminator_table = NULL;
 }
 
 static void read_watcher_destroy_notify(GAtChat *chat)
@@ -365,29 +370,25 @@ struct terminator_info {
 	gboolean success;
 };
 
-static struct terminator_info terminator_table[] = {
-	{ "OK", -1, TRUE },
-	{ "ERROR", -1, FALSE },
-	{ "NO DIALTONE", -1, FALSE },
-	{ "BUSY", -1, FALSE },
-	{ "NO CARRIER", -1, FALSE },
-	{ "CONNECT", -1, TRUE },
-	{ "NO ANSWER", -1, FALSE },
-	{ "+CMS ERROR:", 11, FALSE },
-	{ "+CME ERROR:", 11, FALSE },
-	{ "+EXT ERROR:", 11, FALSE }
-};
+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);
+}
 
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
 							struct at_command *cmd,
 							char *line)
 {
-	int i;
-	int size = sizeof(terminator_table) / sizeof(struct terminator_info);
+	GSList *l;
 	int hint;
 
-	for (i = 0; i < size; i++) {
-		struct terminator_info *info = &terminator_table[i];
+	for (l = p->terminator_table; l; l = l->next) {
+		struct terminator_info *info = l->data;
 
 		if (info->len == -1 && !strcmp(line, info->terminator)) {
 			g_at_chat_finish_command(p, info->success, line);
@@ -951,6 +952,17 @@ GAtChat *g_at_chat_new(GIOChannel *channel, GAtSyntax *syntax)
 
 	chat->syntax = g_at_syntax_ref(syntax);
 
+	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);
+
 	return chat;
 
 error:
diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
index fe5b97b..b191f1b 100644
--- a/gatchat/gatchat.h
+++ b/gatchat/gatchat.h
@@ -128,6 +128,9 @@ gboolean g_at_chat_unregister(GAtChat *chat, guint id);
 gboolean g_at_chat_set_wakeup_command(GAtChat *chat, const char *cmd,
 					guint timeout, guint msec);
 
+void g_at_chat_add_terminator(GAtChat *chat, const char *terminator,
+				int len, gboolean success);
+
 
 #ifdef __cplusplus
 }
-- 
1.6.2.5


[-- Attachment #3: 0002-Support-Huawei-specific-terminator.patch --]
[-- Type: application/octet-stream, Size: 713 bytes --]

From b2c955d5f0ee23bf553b84570bd919229f6c7228 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Wed, 18 Nov 2009 00:22:29 +0800
Subject: [PATCH 2/2] Support Huawei specific terminator

---
 plugins/huawei.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 28b7650..df8ef40 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,7 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	syntax = g_at_syntax_new_gsmv1();
 	data->chat = g_at_chat_new(channel, syntax);
+	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-- 
1.6.2.5

Re: Patch on unsupported AT command

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

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

Re: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

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

RE: Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]


>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
>Marcel Holtmann
>Sent: Wednesday, November 18, 2009 4:36 AM
>To: ofono(a)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(a)ofono.org
>http://lists.ofono.org/listinfo/ofono


Regards,
-Yang

[-- Attachment #2: 0001-Framework-to-support-non-standard-terminator.patch --]
[-- Type: application/octet-stream, Size: 4053 bytes --]

From 86f8526302b42e76908fa50c43c1419b6d42c570 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Fri, 20 Nov 2009 16:53:20 +0800
Subject: [PATCH 1/2] Framework to support non-standard terminator

---
 gatchat/gatchat.c |   68 +++++++++++++++++++++++++++++++++++++++++-----------
 gatchat/gatchat.h |    3 ++
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..17f9118 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -88,6 +88,13 @@ struct _GAtChat {
 	GTimer *wakeup_timer;			/* Keep track of elapsed time */
 	GAtSyntax *syntax;
 	gboolean destroyed;			/* Re-entrancy guard */
+	GSList *terminator_list;		/* Non-standard terminator */
+};
+
+struct terminator_info {
+	const char *terminator;
+	int len;
+	gboolean success;
 };
 
 static gint at_notify_node_compare_by_id(gconstpointer a, gconstpointer b)
@@ -204,6 +211,14 @@ static void at_command_destroy(struct at_command *cmd)
 	g_free(cmd);
 }
 
+static void free_terminator(struct terminator_info *ti)
+{
+	g_free((char *)ti->terminator);
+	ti->terminator = NULL;
+	g_free(ti);
+	ti = NULL;
+}
+
 static void g_at_chat_cleanup(GAtChat *chat)
 {
 	struct at_command *c;
@@ -251,6 +266,13 @@ static void g_at_chat_cleanup(GAtChat *chat)
 	chat->syntax = NULL;
 
 	chat->channel = NULL;
+
+	if (chat->terminator_list) {
+		g_slist_foreach(chat->terminator_list,
+					(GFunc)free_terminator, NULL);
+		g_slist_free(chat->terminator_list);
+		chat->terminator_list = NULL;
+	}
 }
 
 static void read_watcher_destroy_notify(GAtChat *chat)
@@ -359,12 +381,6 @@ static void g_at_chat_finish_command(GAtChat *p, gboolean ok,
 	at_command_destroy(cmd);
 }
 
-struct terminator_info {
-	const char *terminator;
-	int len;
-	gboolean success;
-};
-
 static struct terminator_info terminator_table[] = {
 	{ "OK", -1, TRUE },
 	{ "ERROR", -1, FALSE },
@@ -378,6 +394,32 @@ static struct terminator_info terminator_table[] = {
 	{ "+EXT ERROR:", 11, FALSE }
 };
 
+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 = strdup(terminator);
+	ti->len = len;
+	ti->success = success;
+	chat->terminator_list = g_slist_prepend(chat->terminator_list, ti);
+}
+
+static gboolean meet_terminator(GAtChat *chat,
+				struct terminator_info *info, char *line)
+{
+	if (info->len == -1 && !strcmp(line, info->terminator)) {
+		g_at_chat_finish_command(chat, info->success, line);
+		return TRUE;
+	}
+
+	if (info->len > 0 && !strncmp(line, info->terminator, info->len)) {
+		g_at_chat_finish_command(chat, info->success, line);
+		return TRUE;
+	}
+
+	return FALSE;
+}
+
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
 							struct at_command *cmd,
 							char *line)
@@ -385,20 +427,16 @@ static gboolean g_at_chat_handle_command_response(GAtChat *p,
 	int i;
 	int size = sizeof(terminator_table) / sizeof(struct terminator_info);
 	int hint;
+	GSList *l;
 
 	for (i = 0; i < size; i++) {
-		struct terminator_info *info = &terminator_table[i];
-
-		if (info->len == -1 && !strcmp(line, info->terminator)) {
-			g_at_chat_finish_command(p, info->success, line);
+		if (meet_terminator(p, &terminator_table[i], line))
 			return TRUE;
-		}
+	}
 
-		if (info->len > 0 &&
-			!strncmp(line, info->terminator, info->len)) {
-			g_at_chat_finish_command(p, info->success, line);
+	for (l = p->terminator_list; l; l = l->next) {
+		if (meet_terminator(p, l->data, line))
 			return TRUE;
-		}
 	}
 
 	if (cmd->prefixes) {
diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
index fe5b97b..b191f1b 100644
--- a/gatchat/gatchat.h
+++ b/gatchat/gatchat.h
@@ -128,6 +128,9 @@ gboolean g_at_chat_unregister(GAtChat *chat, guint id);
 gboolean g_at_chat_set_wakeup_command(GAtChat *chat, const char *cmd,
 					guint timeout, guint msec);
 
+void g_at_chat_add_terminator(GAtChat *chat, const char *terminator,
+				int len, gboolean success);
+
 
 #ifdef __cplusplus
 }
-- 
1.6.5.2


[-- Attachment #3: 0002-Support-Huawei-specific-terminator.patch --]
[-- Type: application/octet-stream, Size: 713 bytes --]

From 716bf45423fc25537072985dd3d5849462c0b12d Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Fri, 20 Nov 2009 16:54:05 +0800
Subject: [PATCH 2/2] Support Huawei specific terminator

---
 plugins/huawei.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 08f0e01..e83f9c1 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,7 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	syntax = g_at_syntax_new_gsmv1();
 	data->chat = g_at_chat_new(channel, syntax);
+	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-- 
1.6.5.2

RE: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

Hi Yang,

> >> >+	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!

please do casts with a space between. Like (char *) terminator etc. Also
why do you bother with making it const. Just leave that out. Since you
do actually copy the string anyway.

Regards

Marcel

RE: Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]


>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
>Marcel Holtmann
>Sent: Friday, November 20, 2009 9:31 PM
>To: ofono(a)ofono.org
>Subject: RE: Patch on unsupported AT command
>
>Hi Yang,
>
>> >> >+	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!
>
>please do casts with a space between. Like (char *) terminator etc. Also
>why do you bother with making it const. Just leave that out. Since you
>do actually copy the string anyway.

Fixed!

>
>Regards
>
>Marcel
>
>
>_______________________________________________
>ofono mailing list
>ofono(a)ofono.org
>http://lists.ofono.org/listinfo/ofono

[-- Attachment #2: 0001-Framework-to-support-non-standard-terminator.patch --]
[-- Type: application/octet-stream, Size: 4042 bytes --]

From 4143e866144b87cef1c301033ee0352a4e20be4c Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Sat, 21 Nov 2009 13:42:37 +0800
Subject: [PATCH 1/2] Framework to support non-standard terminator

---
 gatchat/gatchat.c |   68 +++++++++++++++++++++++++++++++++++++++++-----------
 gatchat/gatchat.h |    3 ++
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..b8aeae9 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -88,6 +88,13 @@ struct _GAtChat {
 	GTimer *wakeup_timer;			/* Keep track of elapsed time */
 	GAtSyntax *syntax;
 	gboolean destroyed;			/* Re-entrancy guard */
+	GSList *terminator_list;		/* Non-standard terminator */
+};
+
+struct terminator_info {
+	const char *terminator;
+	int len;
+	gboolean success;
 };
 
 static gint at_notify_node_compare_by_id(gconstpointer a, gconstpointer b)
@@ -204,6 +211,14 @@ static void at_command_destroy(struct at_command *cmd)
 	g_free(cmd);
 }
 
+static void free_terminator(struct terminator_info *ti)
+{
+	g_free((char *) ti->terminator);
+	ti->terminator = NULL;
+	g_free(ti);
+	ti = NULL;
+}
+
 static void g_at_chat_cleanup(GAtChat *chat)
 {
 	struct at_command *c;
@@ -251,6 +266,13 @@ static void g_at_chat_cleanup(GAtChat *chat)
 	chat->syntax = NULL;
 
 	chat->channel = NULL;
+
+	if (chat->terminator_list) {
+		g_slist_foreach(chat->terminator_list,
+					(GFunc)free_terminator, NULL);
+		g_slist_free(chat->terminator_list);
+		chat->terminator_list = NULL;
+	}
 }
 
 static void read_watcher_destroy_notify(GAtChat *chat)
@@ -359,12 +381,6 @@ static void g_at_chat_finish_command(GAtChat *p, gboolean ok,
 	at_command_destroy(cmd);
 }
 
-struct terminator_info {
-	const char *terminator;
-	int len;
-	gboolean success;
-};
-
 static struct terminator_info terminator_table[] = {
 	{ "OK", -1, TRUE },
 	{ "ERROR", -1, FALSE },
@@ -378,6 +394,32 @@ static struct terminator_info terminator_table[] = {
 	{ "+EXT ERROR:", 11, FALSE }
 };
 
+void g_at_chat_add_terminator(GAtChat *chat, char *terminator,
+					int len, gboolean success)
+{
+	struct terminator_info *ti = g_new0(struct terminator_info, 1);
+	ti->terminator = strdup(terminator);
+	ti->len = len;
+	ti->success = success;
+	chat->terminator_list = g_slist_prepend(chat->terminator_list, ti);
+}
+
+static gboolean meet_terminator(GAtChat *chat,
+				struct terminator_info *info, char *line)
+{
+	if (info->len == -1 && !strcmp(line, info->terminator)) {
+		g_at_chat_finish_command(chat, info->success, line);
+		return TRUE;
+	}
+
+	if (info->len > 0 && !strncmp(line, info->terminator, info->len)) {
+		g_at_chat_finish_command(chat, info->success, line);
+		return TRUE;
+	}
+
+	return FALSE;
+}
+
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
 							struct at_command *cmd,
 							char *line)
@@ -385,20 +427,16 @@ static gboolean g_at_chat_handle_command_response(GAtChat *p,
 	int i;
 	int size = sizeof(terminator_table) / sizeof(struct terminator_info);
 	int hint;
+	GSList *l;
 
 	for (i = 0; i < size; i++) {
-		struct terminator_info *info = &terminator_table[i];
-
-		if (info->len == -1 && !strcmp(line, info->terminator)) {
-			g_at_chat_finish_command(p, info->success, line);
+		if (meet_terminator(p, &terminator_table[i], line))
 			return TRUE;
-		}
+	}
 
-		if (info->len > 0 &&
-			!strncmp(line, info->terminator, info->len)) {
-			g_at_chat_finish_command(p, info->success, line);
+	for (l = p->terminator_list; l; l = l->next) {
+		if (meet_terminator(p, l->data, line))
 			return TRUE;
-		}
 	}
 
 	if (cmd->prefixes) {
diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
index fe5b97b..249c8cf 100644
--- a/gatchat/gatchat.h
+++ b/gatchat/gatchat.h
@@ -128,6 +128,9 @@ gboolean g_at_chat_unregister(GAtChat *chat, guint id);
 gboolean g_at_chat_set_wakeup_command(GAtChat *chat, const char *cmd,
 					guint timeout, guint msec);
 
+void g_at_chat_add_terminator(GAtChat *chat, char *terminator,
+				int len, gboolean success);
+
 
 #ifdef __cplusplus
 }
-- 
1.6.5.2


[-- Attachment #3: 0002-Support-Huawei-specific-terminator.patch --]
[-- Type: application/octet-stream, Size: 713 bytes --]

From 56e4f32df042f968a3334651a5556f01b84bca72 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Sat, 21 Nov 2009 13:43:14 +0800
Subject: [PATCH 2/2] Support Huawei specific terminator

---
 plugins/huawei.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 08f0e01..e83f9c1 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,7 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	syntax = g_at_syntax_new_gsmv1();
 	data->chat = g_at_chat_new(channel, syntax);
+	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-- 
1.6.5.2

RE: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

Hi Yang,

> >> >> >+	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!
> >
> >please do casts with a space between. Like (char *) terminator etc. Also
> >why do you bother with making it const. Just leave that out. Since you
> >do actually copy the string anyway.
> 
> Fixed!

I don't like this casting at all actually. Please just store the
terminator as char and not const char. That is internal code anyway and
you do allocate it. So no need to mark it as read-only. Also please use
g_strdup since you are using g_free to free it.

+static gboolean meet_terminator(GAtChat *chat,
+                               struct terminator_info *info, char *line)

About this function name, I don't really like it since it confuses the
hell out of me what it is meant do be doing. I do get what you are
trying to do here, but I would just name it check_terminator and then
leave the g_at_chat_finish_command() call in the original code.

Regards

Marcel

RE: Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]



>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
>Marcel Holtmann
>Sent: Saturday, November 21, 2009 6:42 PM
>To: ofono(a)ofono.org
>Subject: RE: Patch on unsupported AT command
>
>Hi Yang,
>
>> >> >> >+	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!
>> >
>> >please do casts with a space between. Like (char *) terminator etc. Also
>> >why do you bother with making it const. Just leave that out. Since you
>> >do actually copy the string anyway.
>>
>> Fixed!
>
>I don't like this casting at all actually. Please just store the
>terminator as char and not const char. That is internal code anyway and
>you do allocate it. So no need to mark it as read-only. Also please use
>g_strdup since you are using g_free to free it.
>
>+static gboolean meet_terminator(GAtChat *chat,
>+                               struct terminator_info *info, char *line)
>
>About this function name, I don't really like it since it confuses the
>hell out of me what it is meant do be doing. I do get what you are
>trying to do here, but I would just name it check_terminator and then
>leave the g_at_chat_finish_command() call in the original code.

Code is modified according to your comment. Please have a review!

>Regards
>
>Marcel
>
>
>_______________________________________________
>ofono mailing list
>ofono(a)ofono.org
>http://lists.ofono.org/listinfo/ofono

[-- Attachment #2: 0001-Framework-to-support-non-standard-terminator.patch --]
[-- Type: application/octet-stream, Size: 3892 bytes --]

From 301d6b7ec5c8a22e0aaf4be58bc39956677f67b4 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Mon, 23 Nov 2009 09:58:32 +0800
Subject: [PATCH 1/2] Framework to support non-standard terminator

---
 gatchat/gatchat.c |   57 +++++++++++++++++++++++++++++++++++++++++++---------
 gatchat/gatchat.h |    3 ++
 2 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..3218d02 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -88,6 +88,13 @@ struct _GAtChat {
 	GTimer *wakeup_timer;			/* Keep track of elapsed time */
 	GAtSyntax *syntax;
 	gboolean destroyed;			/* Re-entrancy guard */
+	GSList *terminator_list;		/* Non-standard terminator */
+};
+
+struct terminator_info {
+	char *terminator;
+	int len;
+	gboolean success;
 };
 
 static gint at_notify_node_compare_by_id(gconstpointer a, gconstpointer b)
@@ -204,6 +211,14 @@ static void at_command_destroy(struct at_command *cmd)
 	g_free(cmd);
 }
 
+static void free_terminator(struct terminator_info *info)
+{
+	g_free(info->terminator);
+	info->terminator = NULL;
+	g_free(info);
+	info = NULL;
+}
+
 static void g_at_chat_cleanup(GAtChat *chat)
 {
 	struct at_command *c;
@@ -251,6 +266,13 @@ static void g_at_chat_cleanup(GAtChat *chat)
 	chat->syntax = NULL;
 
 	chat->channel = NULL;
+
+	if (chat->terminator_list) {
+		g_slist_foreach(chat->terminator_list,
+					(GFunc)free_terminator, NULL);
+		g_slist_free(chat->terminator_list);
+		chat->terminator_list = NULL;
+	}
 }
 
 static void read_watcher_destroy_notify(GAtChat *chat)
@@ -359,12 +381,6 @@ static void g_at_chat_finish_command(GAtChat *p, gboolean ok,
 	at_command_destroy(cmd);
 }
 
-struct terminator_info {
-	const char *terminator;
-	int len;
-	gboolean success;
-};
-
 static struct terminator_info terminator_table[] = {
 	{ "OK", -1, TRUE },
 	{ "ERROR", -1, FALSE },
@@ -378,6 +394,25 @@ static struct terminator_info terminator_table[] = {
 	{ "+EXT ERROR:", 11, FALSE }
 };
 
+void g_at_chat_add_terminator(GAtChat *chat, char *terminator,
+					int len, gboolean success)
+{
+	struct terminator_info *info = g_new0(struct terminator_info, 1);
+	info->terminator = g_strdup(terminator);
+	info->len = len;
+	info->success = success;
+	chat->terminator_list = g_slist_prepend(chat->terminator_list, info);
+}
+
+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;
+}
+
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
 							struct at_command *cmd,
 							char *line)
@@ -385,17 +420,19 @@ static gboolean g_at_chat_handle_command_response(GAtChat *p,
 	int i;
 	int size = sizeof(terminator_table) / sizeof(struct terminator_info);
 	int hint;
+	GSList *l;
 
 	for (i = 0; i < size; i++) {
 		struct terminator_info *info = &terminator_table[i];
-
-		if (info->len == -1 && !strcmp(line, info->terminator)) {
+		if (check_terminator(info, line)) {
 			g_at_chat_finish_command(p, info->success, line);
 			return TRUE;
 		}
+	}
 
-		if (info->len > 0 &&
-			!strncmp(line, info->terminator, info->len)) {
+	for (l = p->terminator_list; l; l = l->next) {
+		struct terminator_info *info = l->data;
+		if (check_terminator(info, line)) {
 			g_at_chat_finish_command(p, info->success, line);
 			return TRUE;
 		}
diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
index fe5b97b..249c8cf 100644
--- a/gatchat/gatchat.h
+++ b/gatchat/gatchat.h
@@ -128,6 +128,9 @@ gboolean g_at_chat_unregister(GAtChat *chat, guint id);
 gboolean g_at_chat_set_wakeup_command(GAtChat *chat, const char *cmd,
 					guint timeout, guint msec);
 
+void g_at_chat_add_terminator(GAtChat *chat, char *terminator,
+				int len, gboolean success);
+
 
 #ifdef __cplusplus
 }
-- 
1.6.5.2


[-- Attachment #3: 0002-Support-Huawei-specific-terminator.patch --]
[-- Type: application/octet-stream, Size: 713 bytes --]

From a282f3df613d89d9ef72d439b3307a3d78640a9e Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Mon, 23 Nov 2009 09:58:54 +0800
Subject: [PATCH 2/2] Support Huawei specific terminator

---
 plugins/huawei.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 08f0e01..e83f9c1 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,7 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	syntax = g_at_syntax_new_gsmv1();
 	data->chat = g_at_chat_new(channel, syntax);
+	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-- 
1.6.5.2

RE: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

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

Re: Patch on unsupported AT command

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

Hi Marcel,

> 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.

Regards,
-Denis

Re: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

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

RE: Patch on unsupported AT command

[Gu, Yang]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]



>-----Original Message-----
>From: ofono-bounces(a)ofono.org [mailto:ofono-bounces(a)ofono.org] On Behalf Of
>Marcel Holtmann
>Sent: Monday, November 23, 2009 2:21 PM
>To: ofono(a)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(a)ofono.org
>http://lists.ofono.org/listinfo/ofono

[-- Attachment #2: 0001-Framework-to-support-non-standard-terminator.patch --]
[-- Type: application/octet-stream, Size: 3902 bytes --]

From 7832bd87beed3db190becc71ca14776328d67910 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Tue, 24 Nov 2009 23:55:45 +0800
Subject: [PATCH 1/2] Framework to support non-standard terminator

---
 gatchat/gatchat.c |   59 ++++++++++++++++++++++++++++++++++++++++++++---------
 gatchat/gatchat.h |    3 ++
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/gatchat/gatchat.c b/gatchat/gatchat.c
index 320150a..6e158b3 100644
--- a/gatchat/gatchat.c
+++ b/gatchat/gatchat.c
@@ -88,6 +88,13 @@ struct _GAtChat {
 	GTimer *wakeup_timer;			/* Keep track of elapsed time */
 	GAtSyntax *syntax;
 	gboolean destroyed;			/* Re-entrancy guard */
+	GSList *terminator_list;		/* Non-standard terminator */
+};
+
+struct terminator_info {
+	char *terminator;
+	int len;
+	gboolean success;
 };
 
 static gint at_notify_node_compare_by_id(gconstpointer a, gconstpointer b)
@@ -204,6 +211,14 @@ static void at_command_destroy(struct at_command *cmd)
 	g_free(cmd);
 }
 
+static void free_terminator(struct terminator_info *info)
+{
+	g_free(info->terminator);
+	info->terminator = NULL;
+	g_free(info);
+	info = NULL;
+}
+
 static void g_at_chat_cleanup(GAtChat *chat)
 {
 	struct at_command *c;
@@ -251,6 +266,13 @@ static void g_at_chat_cleanup(GAtChat *chat)
 	chat->syntax = NULL;
 
 	chat->channel = NULL;
+
+	if (chat->terminator_list) {
+		g_slist_foreach(chat->terminator_list,
+					(GFunc)free_terminator, NULL);
+		g_slist_free(chat->terminator_list);
+		chat->terminator_list = NULL;
+	}
 }
 
 static void read_watcher_destroy_notify(GAtChat *chat)
@@ -359,12 +381,6 @@ static void g_at_chat_finish_command(GAtChat *p, gboolean ok,
 	at_command_destroy(cmd);
 }
 
-struct terminator_info {
-	const char *terminator;
-	int len;
-	gboolean success;
-};
-
 static struct terminator_info terminator_table[] = {
 	{ "OK", -1, TRUE },
 	{ "ERROR", -1, FALSE },
@@ -378,6 +394,27 @@ static struct terminator_info terminator_table[] = {
 	{ "+EXT ERROR:", 11, FALSE }
 };
 
+void g_at_chat_add_terminator(GAtChat *chat, char *terminator,
+					int len, gboolean success)
+{
+	struct terminator_info *info = g_new0(struct terminator_info, 1);
+	info->terminator = g_strdup(terminator);
+	info->len = len;
+	info->success = success;
+	chat->terminator_list = g_slist_prepend(chat->terminator_list, info);
+}
+
+static gboolean check_terminator(struct terminator_info *info, char *line)
+{
+	if (info->len == -1 && !strcmp(line, info->terminator))
+		return TRUE;
+
+	if (info->len > 0 && !strncmp(line, info->terminator, info->len))
+		return TRUE;
+
+	return FALSE;
+}
+
 static gboolean g_at_chat_handle_command_response(GAtChat *p,
 							struct at_command *cmd,
 							char *line)
@@ -385,17 +422,19 @@ static gboolean g_at_chat_handle_command_response(GAtChat *p,
 	int i;
 	int size = sizeof(terminator_table) / sizeof(struct terminator_info);
 	int hint;
+	GSList *l;
 
 	for (i = 0; i < size; i++) {
 		struct terminator_info *info = &terminator_table[i];
-
-		if (info->len == -1 && !strcmp(line, info->terminator)) {
+		if (check_terminator(info, line)) {
 			g_at_chat_finish_command(p, info->success, line);
 			return TRUE;
 		}
+	}
 
-		if (info->len > 0 &&
-			!strncmp(line, info->terminator, info->len)) {
+	for (l = p->terminator_list; l; l = l->next) {
+		struct terminator_info *info = l->data;
+		if (check_terminator(info, line)) {
 			g_at_chat_finish_command(p, info->success, line);
 			return TRUE;
 		}
diff --git a/gatchat/gatchat.h b/gatchat/gatchat.h
index fe5b97b..249c8cf 100644
--- a/gatchat/gatchat.h
+++ b/gatchat/gatchat.h
@@ -128,6 +128,9 @@ gboolean g_at_chat_unregister(GAtChat *chat, guint id);
 gboolean g_at_chat_set_wakeup_command(GAtChat *chat, const char *cmd,
 					guint timeout, guint msec);
 
+void g_at_chat_add_terminator(GAtChat *chat, char *terminator,
+				int len, gboolean success);
+
 
 #ifdef __cplusplus
 }
-- 
1.6.5.2


[-- Attachment #3: 0002-Support-Huawei-specific-terminator.patch --]
[-- Type: application/octet-stream, Size: 713 bytes --]

From 878392149d24fc0d1bd596ccca51f173cf073c02 Mon Sep 17 00:00:00 2001
From: Yang Gu <yang.gu@intel.com>
Date: Tue, 24 Nov 2009 23:56:05 +0800
Subject: [PATCH 2/2] Support Huawei specific terminator

---
 plugins/huawei.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 08f0e01..e83f9c1 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -107,6 +107,7 @@ static int huawei_enable(struct ofono_modem *modem)
 
 	syntax = g_at_syntax_new_gsmv1();
 	data->chat = g_at_chat_new(channel, syntax);
+	g_at_chat_add_terminator(data->chat, "COMMAND NOT SUPPORT", -1, FALSE);
 	g_at_syntax_unref(syntax);
 	g_io_channel_unref(channel);
 
-- 
1.6.5.2

RE: Patch on unsupported AT command

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

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