[PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Tonny Tzeng]

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

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

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 25dfaca..32cf70d 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -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)
-- 
1.7.2.2

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Denis Kenzior]

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

Hi Tonny,

On 12/03/2010 09:22 AM, Tonny Tzeng wrote:
> ---
>  plugins/huawei.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index 25dfaca..32cf70d 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -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.

>  
>  		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);

Same comment as above

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

ditto.

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

Regards,
-Denis

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Tonny Tzeng]

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

Hi Denis,

On Tue, Dec 7, 2010 at 11:21 AM, Denis Kenzior <denkenz@gmail.com> wrote:
>
> > diff --git a/plugins/huawei.c b/plugins/huawei.c
> > index 25dfaca..32cf70d 100644
> > --- a/plugins/huawei.c
> > +++ b/plugins/huawei.c
> > @@ -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.
>

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

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 2024 bytes --]

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Denis Kenzior]

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

Hi Tonny,

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

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

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Tonny Tzeng]

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

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()?

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

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 840 bytes --]

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Denis Kenzior]

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

Hi Tonny,

On 12/11/2010 10:37 AM, Tonny Tzeng wrote:
> 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.

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

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Kalle Valo]

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

Hi Tonny,

Tonny Tzeng <tonny.tzeng@gmail.com> writes:

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

-- 
Kalle Valo

Re: [PATCH] Specify vendor ID for Huawei modem while creating GPRS context

[Marcel Holtmann]

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

Hi Tonny,

>  plugins/huawei.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index 25dfaca..32cf70d 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -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.

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

>  		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