Specify vendor ID for Huawei modem while creating GPRS context

PATCH In review Tonny Tzeng 4 participants 8 messages Dec 3, 2010 – Dec 11, 2010

Share & tools

Tonny Tzeng

author

2010-12-03 15:22 UTC #0 lore raw

PATCH 1/1 Specify vendor ID for Huawei modem while creating GPRS context

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

plugins/huawei.c +1 -1

index 25dfaca..32cf70d 100644
@@ -454,7 +454,7 @@ static void huawei_disconnect(gpointer user_data)
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
 		ofono_info("Reopened GPRS context channel");
 
-		data->gc = ofono_gprs_context_create(modem, 0, "atmodem",
+		data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,

"atmodem",
data->modem);

if (data->gprs && data->gc)
@@ -631,12 +631,12 @@ static void huawei_post_online(struct ofono_modem *modem)

if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
- data->gprs = ofono_gprs_create(modem, 0, "atmodem", data->pcui);
+ data->gprs = ofono_gprs_create(modem, OFONO_VENDOR_HUAWEI, "atmodem",
data->pcui);
if (data->ndis == TRUE)
- data->gc = ofono_gprs_context_create(modem, 0,
+ data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
"huaweimodem", data->pcui);
else
- data->gc = ofono_gprs_context_create(modem, 0,
+ data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
"atmodem", data->modem);

if (data->gprs && data->gc)

Signature · 1 line

1.7.2.2

Denis Kenzior

2010-12-07 03:21 UTC #1 lore raw

Hi Tonny,

Quoted 9 lines · Tonny Tzeng

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

plugins/huawei.c +0 -0

index 25dfaca..32cf70d 100644

plugins/huawei.c static void huawei_disconnect(gpointer user_data) · line 459

Quoted 7 lines

plugins/huawei.c +1 -1

@@ -454,7 +454,7 @@ static void huawei_disconnect(gpointer user_data)
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
 		ofono_info("Reopened GPRS context channel");
 
-		data->gc = ofono_gprs_context_create(modem, 0, "atmodem",
+		data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
 "atmodem",
 								data->modem);

There's no need to pass the vendor flag here. The atmodem gprs_context driver does not have any vendor handling code inside it.

Quoted 4 lines

plugins/huawei.c +0 -0

 		if (data->gprs && data->gc)

@@ -631,12 +631,12 @@ static void huawei_post_online(struct ofono_modem
 *modem)

plugins/huawei.c static void huawei_post_online(struct ofono_modem · line 639

Quoted 10 lines

plugins/huawei.c +2 -2

 
 	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
-		data->gprs = ofono_gprs_create(modem, 0, "atmodem", data->pcui);
+		data->gprs = ofono_gprs_create(modem, OFONO_VENDOR_HUAWEI, "atmodem",
 data->pcui);
 		if (data->ndis == TRUE)
-			data->gc = ofono_gprs_context_create(modem, 0,
+			data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
 						"huaweimodem", data->pcui);

Same comment as above

plugins/huawei.c static void huawei_post_online(struct ofono_modem · line 642

Quoted 4 lines

plugins/huawei.c +1 -1

 		else
-			data->gc = ofono_gprs_context_create(modem, 0,
+			data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
 						"atmodem", data->modem);

ditto.

Quoted 2 lines

if (data->gprs && data->gc)

Regards,
-Denis

Tonny Tzeng

author

2010-12-11 14:48 UTC #2 lore raw

Hi Denis,

Quoted 12 lines · Denis Kenzior

plugins/huawei.c static void huawei_disconnect(gpointer user_data) · line 457

Quoted 11 lines

plugins/huawei.c +1 -1

index 25dfaca..32cf70d 100644
@@ -454,7 +454,7 @@ static void huawei_disconnect(gpointer user_data)
                      data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
              ofono_info("Reopened GPRS context channel");
 
-             data->gc = ofono_gprs_context_create(modem, 0, "atmodem",
+             data->gc = ofono_gprs_context_create(modem,

OFONO_VENDOR_HUAWEI,

Quoted 7 lines

plugins/huawei.c static void huawei_disconnect(gpointer user_data) · line 459
Quoted 2 lines
plugins/huawei.c +0 -0
 "atmodem",
 
data->modem);
There's no need to pass the vendor flag here. The atmodem gprs_context driver does not have any vendor handling code inside it.

But it seems to me the gprs context will be pass to modem plugins, so we need to set the vendor code for Huawei modems, because it sends unquoted strings.

For example, please correct me if wrong, when the network registration status changed, ofono will invoke modem's set_attached() to handle the modem attach/detach. Then, in gprs_attach_callback() in src/gprs.c, it will invokes modem's attached_status() to check the modem state, which has vendor dependent code in it. Most importantly, after ofono sending AT+CGREG? to query the solicited response, the at_cgreg_cb() requires the correct vendor
ID so that at_util_parse_reg() knows whether the lac and ci are quoted
strings or not? Did I misunderstand? Thanks.

Best Regards,
Tonny

Denis Kenzior

2010-12-11 14:55 UTC #3 lore raw

Hi Tonny,

Quoted 3 lines

But it seems to me the gprs context will be pass to modem plugins, so we need to set the vendor code for Huawei modems, because it sends unquoted strings.

No, you don't. You are confusing gprs atom and the gprs-context atom. Grep for VENDOR in drivers/atmodem/gprs-context.c if you do not believe me. There is no vendor specific code in the atmodem gprs-context driver.

Quoted 9 lines

For example, please correct me if wrong, when the network registration status changed, ofono will invoke modem's set_attached() to handle the modem attach/detach. Then, in gprs_attach_callback() in src/gprs.c, it will invokes modem's attached_status() to check the modem state, which has vendor dependent code in it. Most importantly, after ofono sending AT+CGREG? to query the solicited response, the at_cgreg_cb() requires the correct vendor ID so that at_util_parse_reg() knows whether the lac and ci are quoted strings or not? Did I misunderstand? Thanks.

As I mentioned before, this is needed for the gprs atom driver which handles the CGREG parsing. Not for gprs-context driver.

Regards,
-Denis

Tonny Tzeng

author

2010-12-11 16:37 UTC #4 lore raw

Hi Denis,

No, you don't. You are confusing gprs atom and the gprs-context atom.

Quoted 3 lines

Grep for VENDOR in drivers/atmodem/gprs-context.c if you do not believe me. There is no vendor specific code in the atmodem gprs-context driver.

Yes, my bad, I misunderstood the program flow...
So, is huawei_post_online() in plugins/huawei.c the right place to pass the vendor ID while calling to ofono_gprs_create()?

Together with the patch to fix parsing of unquoted CGREG, now I can get the CGREG status correctly from the Huawei modem. :) Thanks!

Best Regards,
Tonny

Denis Kenzior

2010-12-11 17:40 UTC #5 lore raw

Hi Tonny,

Quoted 11 lines · Tonny Tzeng

Hi Denis,
No, you don't. You are confusing gprs atom and the gprs-context atom. Grep for VENDOR in drivers/atmodem/gprs-context.c if you do not believe me. There is no vendor specific code in the atmodem gprs-context driver.
Yes, my bad, I misunderstood the program flow...
So, is huawei_post_online() in plugins/huawei.c the right place to pass the vendor ID while calling to ofono_gprs_create()?

Yes, I fixed this now as well.

Quoted 3 lines

Together with the patch to fix parsing of unquoted CGREG, now I can get the CGREG status correctly from the Huawei modem. :) Thanks!

My earlier patch has been applied, so please test the upstream version and report back any problems.

Regards,
-Denis

Kalle Valo

2010-12-07 08:43 UTC #6 lore raw

In reply to Tonny Tzeng

Hi Tonny,

Quoted 3 lines · Tonny Tzeng

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

Your commit log is empty. The most important part of the commit log is to answer the question "Why?" and this patch really would need that answer.

What's the bug are you fixing? Logs etc would help here. Also what is the exact model of the Huawei modem with you see the bug?

Signature · 1 line

Kalle Valo

Marcel Holtmann

2010-12-07 10:29 UTC #7 lore raw

In reply to Tonny Tzeng

Hi Tonny,

Quoted 8 lines

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

plugins/huawei.c +0 -0

index 25dfaca..32cf70d 100644

plugins/huawei.c static void huawei_disconnect(gpointer user_data) · line 459

Quoted 7 lines

plugins/huawei.c +1 -1

@@ -454,7 +454,7 @@ static void huawei_disconnect(gpointer user_data)
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
 		ofono_info("Reopened GPRS context channel");
 
-		data->gc = ofono_gprs_context_create(modem, 0, "atmodem",
+		data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
 "atmodem",
 								data->modem);

this not doing anything. Behavior is still the same.

plugins/huawei.c static void huawei_post_online(struct ofono_modem · line 639

Quoted 13 lines

plugins/huawei.c +2 -2

 		if (data->gprs && data->gc)

@@ -631,12 +631,12 @@ static void huawei_post_online(struct ofono_modem
 *modem)
 
 	if (data->sim_state == HUAWEI_SIM_STATE_VALID ||
 			data->sim_state == HUAWEI_SIM_STATE_INVALID_CS) {
-		data->gprs = ofono_gprs_create(modem, 0, "atmodem", data->pcui);
+		data->gprs = ofono_gprs_create(modem, OFONO_VENDOR_HUAWEI, "atmodem",
 data->pcui);
 		if (data->ndis == TRUE)
-			data->gc = ofono_gprs_context_create(modem, 0,
+			data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
 						"huaweimodem", data->pcui);

This one makes no sense. It is Huawei specific already.

plugins/huawei.c static void huawei_post_online(struct ofono_modem · line 642

Quoted 5 lines

plugins/huawei.c +1 -1

 		else
-			data->gc = ofono_gprs_context_create(modem, 0,
+			data->gc = ofono_gprs_context_create(modem, OFONO_VENDOR_HUAWEI,
 						"atmodem", data->modem);

This is also not doing anything either actually.

Regards

Marcel