[PATCH] atmodem: Do some polling on at_pin_query

[PATCH] atmodem: Do some polling on at_pin_query

[Guillaume Zajac]

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

For some modem like ZTE MF180/190, we need to do some
polling to check SIM state when it returns +CME ERROR: 14 busy.
---
 drivers/atmodem/sim.c |   59 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index c51b1d4..a23738b 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -51,6 +51,8 @@ struct sim_data {
 	GAtChat *chat;
 	unsigned int vendor;
 	guint ready_id;
+	guint poll_source;
+	guint poll_count;
 };
 
 static const char *crsm_prefix[] = { "+CRSM:", NULL };
@@ -856,6 +858,8 @@ static void at_pin_retries_query(struct ofono_sim *sim,
 	CALLBACK_WITH_FAILURE(cb, NULL, data);
 }
 
+static gboolean sim_state_check(gpointer user_data);
+
 static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -874,9 +878,22 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	else
 		decode_at_error(&error, final);
 
-	if (!ok) {
+	switch (error.type) {
+	case OFONO_ERROR_TYPE_NO_ERROR:
+		break;
+	case OFONO_ERROR_TYPE_CME:
+		/* Check for SIM busy - try again later */
+		if (error.error == 14) {
+			if (sd->poll_count++ < 12) {
+				sd->poll_source = g_timeout_add_seconds(2,
+						sim_state_check, cbd);
+				return;
+			}
+		}
+		/* fall through */
+	default:
 		cb(&error, -1, cbd->data);
-		return;
+		goto done;
 	}
 
 	if (sd->vendor == OFONO_VENDOR_WAVECOM) {
@@ -887,7 +904,7 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 		if (!g_at_result_iter_next(&iter, "+CPIN:")) {
 			CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-			return;
+			goto done;
 		}
 
 		g_at_result_iter_next_unquoted_string(&iter, &pin_required);
@@ -903,12 +920,34 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 	if (pin_type == OFONO_SIM_PASSWORD_INVALID) {
 		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
-		return;
+		goto done;
 	}
 
 	DBG("crsm_pin_cb: %s", pin_required);
 
 	cb(&error, pin_type, cbd->data);
+
+done:
+	g_free(cbd);
+}
+
+static gboolean sim_state_check(gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct sim_data *sd = ofono_sim_get_data(cbd->user);
+	ofono_sim_passwd_cb_t cb = cbd->cb;
+
+	sd->poll_source = 0;
+
+	if (g_at_chat_send(sd->chat, "AT+CPIN?", cpin_prefix,
+				at_cpin_cb, cbd, NULL) > 0)
+		return FALSE;
+
+	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+
+	g_free(cbd);
+
+	return FALSE;
 }
 
 static void at_pin_query(struct ofono_sim *sim, ofono_sim_passwd_cb_t cb,
@@ -919,13 +958,8 @@ static void at_pin_query(struct ofono_sim *sim, ofono_sim_passwd_cb_t cb,
 
 	cbd->user = sim;
 
-	if (g_at_chat_send(sd->chat, "AT+CPIN?", cpin_prefix,
-				at_cpin_cb, cbd, g_free) > 0)
-		return;
-
-	g_free(cbd);
-
-	CALLBACK_WITH_FAILURE(cb, -1, data);
+	sd->poll_count = 0;
+	sim_state_check(cbd);
 }
 
 static void at_xsim_notify(GAtResult *result, gpointer user_data)
@@ -1246,6 +1280,9 @@ static void at_sim_remove(struct ofono_sim *sim)
 {
 	struct sim_data *sd = ofono_sim_get_data(sim);
 
+	if (sd->poll_source > 0)
+		g_source_remove(sd->poll_source);
+
 	ofono_sim_set_data(sim, NULL);
 
 	g_at_chat_unref(sd->chat);
-- 
1.7.4.1

Re: [PATCH] atmodem: Do some polling on at_pin_query

[Denis Kenzior]

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

Hi Guillaume,

On 04/12/2012 09:28 AM, Guillaume Zajac wrote:
> For some modem like ZTE MF180/190, we need to do some
> polling to check SIM state when it returns +CME ERROR: 14 busy.
> ---
>  drivers/atmodem/sim.c |   59 +++++++++++++++++++++++++++++++++++++++---------
>  1 files changed, 48 insertions(+), 11 deletions(-)
> 

<snip>

> @@ -874,9 +878,22 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	else
>  		decode_at_error(&error, final);
>  
> -	if (!ok) {
> +	switch (error.type) {
> +	case OFONO_ERROR_TYPE_NO_ERROR:
> +		break;
> +	case OFONO_ERROR_TYPE_CME:
> +		/* Check for SIM busy - try again later */
> +		if (error.error == 14) {
> +			if (sd->poll_count++ < 12) {
> +				sd->poll_source = g_timeout_add_seconds(2,
> +						sim_state_check, cbd);
> +				return;
> +			}
> +		}
> +		/* fall through */
> +	default:
>  		cb(&error, -1, cbd->data);
> -		return;
> +		goto done;
>  	}
>  
>  	if (sd->vendor == OFONO_VENDOR_WAVECOM) {

Is there a reason we are not using at_util_sim_state_query_new?

<snip>

Regards,
-Denis

Re: [PATCH] atmodem: Do some polling on at_pin_query

[Guillaume Zajac]

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

Hi Denis,

On 12/04/2012 21:36, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 04/12/2012 09:28 AM, Guillaume Zajac wrote:
>> For some modem like ZTE MF180/190, we need to do some
>> polling to check SIM state when it returns +CME ERROR: 14 busy.
>> ---
>>   drivers/atmodem/sim.c |   59 +++++++++++++++++++++++++++++++++++++++---------
>>   1 files changed, 48 insertions(+), 11 deletions(-)
>>
> <snip>
>
>> @@ -874,9 +878,22 @@ static void at_cpin_cb(gboolean ok, GAtResult *result, gpointer user_data)
>>   	else
>>   		decode_at_error(&error, final);
>>
>> -	if (!ok) {
>> +	switch (error.type) {
>> +	case OFONO_ERROR_TYPE_NO_ERROR:
>> +		break;
>> +	case OFONO_ERROR_TYPE_CME:
>> +		/* Check for SIM busy - try again later */
>> +		if (error.error == 14) {
>> +			if (sd->poll_count++<  12) {
>> +				sd->poll_source = g_timeout_add_seconds(2,
>> +						sim_state_check, cbd);
>> +				return;
>> +			}
>> +		}
>> +		/* fall through */
>> +	default:
>>   		cb(&error, -1, cbd->data);
>> -		return;
>> +		goto done;
>>   	}
>>
>>   	if (sd->vendor == OFONO_VENDOR_WAVECOM) {
> Is there a reason we are not using at_util_sim_state_query_new?

This function is only giving sim present information through its 
callback, however I also need to parse the answer from AT+CPIN?
So it means we would send once the AT+CPIN? using atutil helper and once 
to parse the answer.
I thought about implementing a new atutil helper function or a new 
g_at_chat_send_recursive() function but I saw in 
drivers/atmodem/phonebook.c on AT+CPBS=? command that polling mechanism 
is integrated into the driver itself.
That's why I have chosen this solution.

> <snip>
>
> Regards,
> -Denis

Kind regards,
Guillaume

Re: [PATCH] atmodem: Do some polling on at_pin_query

[Denis Kenzior]

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

Hi Guillaume,

On 04/16/2012 03:06 AM, Guillaume Zajac wrote:
> Hi Denis,
> 
> On 12/04/2012 21:36, Denis Kenzior wrote:
>> Hi Guillaume,
>>
>> On 04/12/2012 09:28 AM, Guillaume Zajac wrote:
>>> For some modem like ZTE MF180/190, we need to do some
>>> polling to check SIM state when it returns +CME ERROR: 14 busy.
>>> ---
>>>   drivers/atmodem/sim.c |   59
>>> +++++++++++++++++++++++++++++++++++++++---------
>>>   1 files changed, 48 insertions(+), 11 deletions(-)
>>>
>> <snip>
>>
>>> @@ -874,9 +878,22 @@ static void at_cpin_cb(gboolean ok, GAtResult
>>> *result, gpointer user_data)
>>>       else
>>>           decode_at_error(&error, final);
>>>
>>> -    if (!ok) {
>>> +    switch (error.type) {
>>> +    case OFONO_ERROR_TYPE_NO_ERROR:
>>> +        break;
>>> +    case OFONO_ERROR_TYPE_CME:
>>> +        /* Check for SIM busy - try again later */
>>> +        if (error.error == 14) {
>>> +            if (sd->poll_count++<  12) {
>>> +                sd->poll_source = g_timeout_add_seconds(2,
>>> +                        sim_state_check, cbd);
>>> +                return;
>>> +            }
>>> +        }
>>> +        /* fall through */
>>> +    default:
>>>           cb(&error, -1, cbd->data);
>>> -        return;
>>> +        goto done;
>>>       }
>>>
>>>       if (sd->vendor == OFONO_VENDOR_WAVECOM) {
>> Is there a reason we are not using at_util_sim_state_query_new?
> 
> This function is only giving sim present information through its
> callback, however I also need to parse the answer from AT+CPIN?
> So it means we would send once the AT+CPIN? using atutil helper and once
> to parse the answer.
> I thought about implementing a new atutil helper function or a new
> g_at_chat_send_recursive() function but I saw in
> drivers/atmodem/phonebook.c on AT+CPBS=? command that polling mechanism
> is integrated into the driver itself.
> That's why I have chosen this solution.
> 

Then I'm a bit lost what you're trying to solve.  The ZTE modem driver
is already running CPIN query repeatedly to figure out the SIM state.
So the only reason why we might be receiving a CME ERROR 14 is if we
(successfully) entered the PIN but the modem is busy initializing the
SIM.  In this case polling until CPIN becomes ready from within
at_pin_send_cb seems good enough.  Or is there something I am missing?

Regards,
-Denis

Re: [PATCH] atmodem: Do some polling on at_pin_query

[Guillaume Zajac]

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

Hi Denis,

On 16/04/2012 16:15, Denis Kenzior wrote:
> Hi Guillaume,
>
> On 04/16/2012 03:06 AM, Guillaume Zajac wrote:
>> Hi Denis,
>>
>> On 12/04/2012 21:36, Denis Kenzior wrote:
>>> Hi Guillaume,
>>>
>>> On 04/12/2012 09:28 AM, Guillaume Zajac wrote:
>>>> For some modem like ZTE MF180/190, we need to do some
>>>> polling to check SIM state when it returns +CME ERROR: 14 busy.
>>>> ---
>>>>    drivers/atmodem/sim.c |   59
>>>> +++++++++++++++++++++++++++++++++++++++---------
>>>>    1 files changed, 48 insertions(+), 11 deletions(-)
>>>>
>>> <snip>
>>>
>>>> @@ -874,9 +878,22 @@ static void at_cpin_cb(gboolean ok, GAtResult
>>>> *result, gpointer user_data)
>>>>        else
>>>>            decode_at_error(&error, final);
>>>>
>>>> -    if (!ok) {
>>>> +    switch (error.type) {
>>>> +    case OFONO_ERROR_TYPE_NO_ERROR:
>>>> +        break;
>>>> +    case OFONO_ERROR_TYPE_CME:
>>>> +        /* Check for SIM busy - try again later */
>>>> +        if (error.error == 14) {
>>>> +            if (sd->poll_count++<   12) {
>>>> +                sd->poll_source = g_timeout_add_seconds(2,
>>>> +                        sim_state_check, cbd);
>>>> +                return;
>>>> +            }
>>>> +        }
>>>> +        /* fall through */
>>>> +    default:
>>>>            cb(&error, -1, cbd->data);
>>>> -        return;
>>>> +        goto done;
>>>>        }
>>>>
>>>>        if (sd->vendor == OFONO_VENDOR_WAVECOM) {
>>> Is there a reason we are not using at_util_sim_state_query_new?
>> This function is only giving sim present information through its
>> callback, however I also need to parse the answer from AT+CPIN?
>> So it means we would send once the AT+CPIN? using atutil helper and once
>> to parse the answer.
>> I thought about implementing a new atutil helper function or a new
>> g_at_chat_send_recursive() function but I saw in
>> drivers/atmodem/phonebook.c on AT+CPBS=? command that polling mechanism
>> is integrated into the driver itself.
>> That's why I have chosen this solution.
>>
> Then I'm a bit lost what you're trying to solve.  The ZTE modem driver
> is already running CPIN query repeatedly to figure out the SIM state.
> So the only reason why we might be receiving a CME ERROR 14 is if we
> (successfully) entered the PIN but the modem is busy initializing the
> SIM.  In this case polling until CPIN becomes ready from within
> at_pin_send_cb seems good enough.  Or is there something I am missing

According to our discussion on IRC, I will send a v2 for which the 
polling is done after PIN is entered.

Kind regards,
Guillaume