[RFC PATCH v1 0/2] huawei: fix cold start

[RFC PATCH v1 0/2] huawei: fix cold start

[Kalle Valo]

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

I finally was able to start look at Huawei again. Like reported multiple
times, huawei currently works in only in the "warm start" case (ie. plug in 
modem and restart ofono). This is a regression, this worked ok a month ago.

Here's currently what I have, but pin locked sims still don't work. I'll
send the patches as RFC anyway. Please test and give feedback.

---

Kalle Valo (2):
      huawei: poll sim state
      huawei: fix online logic


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

[RFC PATCH v1 1/2] huawei: poll sim state

[Kalle Valo]

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

On my Huawei E1552 when I plug in the modem (ie. cold start) with PIN locked
SIM, the sim state is 255 (HUAWEI_SIM_STATE_NOT_EXISTENT). As the modem
doesn't send ^SIMST notifications, poll the sim state until it's ready.

In theory it might be possible to do this better, for example follow
^BOOT notifications or something, but it's unknown what parameter we
should check for.
---
 plugins/huawei.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/plugins/huawei.c b/plugins/huawei.c
index ccb5290..ad1c3ae 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -76,8 +76,14 @@ struct huawei_data {
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
 	gboolean voice;
+	guint query_sim_state;
+	guint sim_poll_count;
 };
 
+#define MAX_SIM_POLL_COUNT 5
+
+static gboolean query_sim_state(gpointer user_data);
+
 static int huawei_probe(struct ofono_modem *modem)
 {
 	struct huawei_data *data;
@@ -157,22 +163,32 @@ static void ussdmode_support_cb(gboolean ok, GAtResult *result,
 					ussdmode_query_cb, data, NULL);
 }
 
-static void notify_sim_state(struct ofono_modem *modem,
+static gboolean notify_sim_state(struct ofono_modem *modem,
 				enum huawei_sim_state sim_state)
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
 
-	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT)
+	DBG("%d", sim_state);
+
+	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT) {
 		ofono_sim_inserted_notify(data->sim, FALSE);
+
+		/* SIM is not ready, try again a bit later */
+		return TRUE;
+	}
 	else
 		ofono_sim_inserted_notify(data->sim, TRUE);
 
 	data->sim_state = sim_state;
+
+	return FALSE;
 }
 
 static void sysinfo_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct ofono_modem *modem = user_data;
+	struct huawei_data *data = ofono_modem_get_data(modem);
+	gboolean rerun;
 	gint sim_state;
 	GAtResultIter iter;
 
@@ -199,7 +215,29 @@ static void sysinfo_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	if (!g_at_result_iter_next_number(&iter, &sim_state))
 		return;
 
-	notify_sim_state(modem, (enum huawei_sim_state) sim_state);
+	rerun = notify_sim_state(modem, (enum huawei_sim_state) sim_state);
+
+	if (rerun && data->sim_poll_count < MAX_SIM_POLL_COUNT) {
+		data->sim_poll_count++;
+		data->query_sim_state = g_timeout_add_seconds(2,
+								query_sim_state,
+								modem);
+	}
+}
+
+static gboolean query_sim_state(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct huawei_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	data->query_sim_state = 0;
+
+	g_at_chat_send(data->pcui, "AT^SYSINFO", sysinfo_prefix,
+			sysinfo_cb, modem, NULL);
+
+	return FALSE;
 }
 
 static void simst_notify(GAtResult *result, gpointer user_data)
@@ -414,6 +452,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 	/* check for voice support */
 	g_at_chat_send(data->pcui, "AT^CVOICE=?", cvoice_prefix,
 					cvoice_support_cb, modem, NULL);
+
+	/* query_sim_state(modem); */
 }
 
 static GAtChat *create_port(const char *device)
@@ -546,6 +586,9 @@ static int huawei_disable(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
+	if (data->query_sim_state)
+		g_source_remove(data->query_sim_state);
+
 	if (data->modem) {
 		g_at_chat_cancel_all(data->modem);
 		g_at_chat_unregister_all(data->modem);
@@ -610,6 +653,9 @@ static void huawei_pre_sim(struct ofono_modem *modem)
 	if (data->voice == TRUE)
 		ofono_voicecall_create(modem, OFONO_VENDOR_HUAWEI,
 						"atmodem", data->pcui);
+
+	data->sim_poll_count = 0;
+	query_sim_state(modem);
 }
 
 static void huawei_post_sim(struct ofono_modem *modem)

[RFC PATCH v1 2/2] huawei: fix online logic

[Kalle Valo]

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


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

diff --git a/plugins/huawei.c b/plugins/huawei.c
index ad1c3ae..0b4e65e 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -78,6 +78,7 @@ struct huawei_data {
 	gboolean voice;
 	guint query_sim_state;
 	guint sim_poll_count;
+	struct cb_data *online_cbd;
 };
 
 #define MAX_SIM_POLL_COUNT 5
@@ -167,19 +168,51 @@ static gboolean notify_sim_state(struct ofono_modem *modem,
 				enum huawei_sim_state sim_state)
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
+	ofono_modem_online_cb_t cb;
 
 	DBG("%d", sim_state);
 
-	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT) {
-		ofono_sim_inserted_notify(data->sim, FALSE);
+	/*
+	 * this needs to be set before calling the online callback, other
+	 * post_online() will use the wrong value
+	 */
+	data->sim_state = sim_state;
 
+	switch (sim_state) {
+	case HUAWEI_SIM_STATE_NOT_EXISTENT:
 		/* SIM is not ready, try again a bit later */
 		return TRUE;
+	case HUAWEI_SIM_STATE_INVALID_OR_LOCKED:
+		ofono_modem_set_powered(modem, TRUE);
+
+		if (data->sim)
+			ofono_sim_inserted_notify(data->sim, TRUE);
+		break;
+	case HUAWEI_SIM_STATE_VALID:
+	case HUAWEI_SIM_STATE_INVALID_CS:
+	case HUAWEI_SIM_STATE_INVALID_PS:
+	case HUAWEI_SIM_STATE_INVALID_PS_AND_CS:
+		/*
+		 * In the "warm start" case the modem skips
+		 * HUAWEI_SIM_STATE_INVALID_OR_LOCKED altogether, so need
+		 * to set power and sim state also here
+		 */
+		ofono_modem_set_powered(modem, TRUE);
+
+		if (data->sim)
+			ofono_sim_inserted_notify(data->sim, TRUE);
+
+		if (data->online_cbd) {
+			cb = data->online_cbd->cb;
+			CALLBACK_WITH_SUCCESS(cb, data->online_cbd->data);
+
+			/* FIXME: how to handle failure? */
+			/* CALLBACK_WITH_FAILURE(cb, cbd->data); */
+
+			data->online_cbd = NULL;
+		}
+		break;
 	}
-	else
-		ofono_sim_inserted_notify(data->sim, TRUE);
-
-	data->sim_state = sim_state;
 
 	return FALSE;
 }
@@ -385,8 +418,6 @@ static void cvoice_query_cb(gboolean ok, GAtResult *result,
 						NULL, NULL, NULL);
 
 done:
-	ofono_modem_set_powered(modem, TRUE);
-
 	/* query current sim state */
         g_at_chat_send(data->pcui, "AT^SYSINFO", sysinfo_prefix,
 						sysinfo_cb, modem, NULL);
@@ -414,8 +445,6 @@ static void cvoice_support_cb(gboolean ok, GAtResult *result,
 	return;
 
 done:
-	ofono_modem_set_powered(modem, TRUE);
-
 	/* query current sim state */
 	g_at_chat_send(data->pcui, "AT^SYSINFO", sysinfo_prefix,
 						sysinfo_cb, modem, NULL);
@@ -452,8 +481,6 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
 	/* check for voice support */
 	g_at_chat_send(data->pcui, "AT^CVOICE=?", cvoice_prefix,
 					cvoice_support_cb, modem, NULL);
-
-	/* query_sim_state(modem); */
 }
 
 static GAtChat *create_port(const char *device)
@@ -557,6 +584,7 @@ static int huawei_enable(struct ofono_modem *modem)
 		data->voice = TRUE;
 
 	data->sim_state = 0;
+	data->online_cbd = NULL;
 
 	g_at_chat_send(data->pcui, "ATE0", none_prefix, NULL, NULL, NULL);
 
@@ -609,13 +637,9 @@ static int huawei_disable(struct ofono_modem *modem)
 
 static void set_online_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
-	struct cb_data *cbd = user_data;
-	ofono_modem_online_cb_t cb = cbd->cb;
+	struct ofono_modem *modem = user_data;
 
-	if (ok)
-		CALLBACK_WITH_SUCCESS(cb, cbd->data);
-	else
-		CALLBACK_WITH_FAILURE(cb, cbd->data);
+	query_sim_state(modem);
 }
 
 static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
@@ -623,21 +647,23 @@ static void huawei_set_online(struct ofono_modem *modem, ofono_bool_t online,
 {
 	struct huawei_data *data = ofono_modem_get_data(modem);
 	GAtChat *chat = data->pcui;
-	struct cb_data *cbd = cb_data_new(cb, user_data);
 	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=5";
 
 	DBG("modem %p %s", modem, online ? "online" : "offline");
 
-	if (!cbd)
+	data->online_cbd = cb_data_new(cb, user_data);
+
+	if (!data->online_cbd)
 		goto error;
 
-	if (g_at_chat_send(chat, command, NULL, set_online_cb, cbd, g_free))
+	if (g_at_chat_send(chat, command, NULL, set_online_cb, modem, NULL))
 		return;
 
 error:
-	g_free(cbd);
+	g_free(data->online_cbd);
+	data->online_cbd = NULL;
 
-	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	CALLBACK_WITH_FAILURE(cb, user_data);
 }
 
 static void huawei_pre_sim(struct ofono_modem *modem)
@@ -673,8 +699,13 @@ static void huawei_post_online(struct ofono_modem *modem)
 	struct ofono_netreg *netreg;
 	struct ofono_message_waiting *mw;
 
-	if (data->sim_state == HUAWEI_SIM_STATE_INVALID_PS_AND_CS)
+	if (data->sim_state != HUAWEI_SIM_STATE_VALID &&
+	    data->sim_state != HUAWEI_SIM_STATE_INVALID_CS &&
+	    data->sim_state != HUAWEI_SIM_STATE_INVALID_PS) {
+		ofono_info("huawei: invalid sim state in post online (%d)",
+				data->sim_state);
 		return;
+	}
 
 	netreg = ofono_netreg_create(modem, OFONO_VENDOR_HUAWEI, "atmodem",
 								data->pcui);

Re: [RFC PATCH v1 1/2] huawei: poll sim state

[Marcel Holtmann]

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

Hi Kalle,

> On my Huawei E1552 when I plug in the modem (ie. cold start) with PIN locked
> SIM, the sim state is 255 (HUAWEI_SIM_STATE_NOT_EXISTENT). As the modem
> doesn't send ^SIMST notifications, poll the sim state until it's ready.
> 
> In theory it might be possible to do this better, for example follow
> ^BOOT notifications or something, but it's unknown what parameter we
> should check for.

I thought that I read the meaning of ^BOOT somewhere, but now I can't
find it anymore :(

> ---
>  plugins/huawei.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 files changed, 49 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index ccb5290..ad1c3ae 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -76,8 +76,14 @@ struct huawei_data {
>  	struct ofono_gprs *gprs;
>  	struct ofono_gprs_context *gc;
>  	gboolean voice;
> +	guint query_sim_state;
> +	guint sim_poll_count;
>  };
>  
> +#define MAX_SIM_POLL_COUNT 5
> +
> +static gboolean query_sim_state(gpointer user_data);
> +
>  static int huawei_probe(struct ofono_modem *modem)
>  {
>  	struct huawei_data *data;
> @@ -157,22 +163,32 @@ static void ussdmode_support_cb(gboolean ok, GAtResult *result,
>  					ussdmode_query_cb, data, NULL);
>  }
>  
> -static void notify_sim_state(struct ofono_modem *modem,
> +static gboolean notify_sim_state(struct ofono_modem *modem,
>  				enum huawei_sim_state sim_state)
>  {
>  	struct huawei_data *data = ofono_modem_get_data(modem);
>  
> -	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT)
> +	DBG("%d", sim_state);
> +
> +	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT) {
>  		ofono_sim_inserted_notify(data->sim, FALSE);
> +
> +		/* SIM is not ready, try again a bit later */
> +		return TRUE;
> +	}
>  	else
>  		ofono_sim_inserted_notify(data->sim, TRUE);

Coding style. } else on the same line. You should know this by now ;)

Also in this case since you do return TRUE anyway from the if block, why
bother with else. Just don't.
 
>  	data->sim_state = sim_state;
> +
> +	return FALSE;
>  }
>  
>  static void sysinfo_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  {
>  	struct ofono_modem *modem = user_data;
> +	struct huawei_data *data = ofono_modem_get_data(modem);
> +	gboolean rerun;
>  	gint sim_state;
>  	GAtResultIter iter;
>  
> @@ -199,7 +215,29 @@ static void sysinfo_cb(gboolean ok, GAtResult *result, gpointer user_data)
>  	if (!g_at_result_iter_next_number(&iter, &sim_state))
>  		return;
>  
> -	notify_sim_state(modem, (enum huawei_sim_state) sim_state);
> +	rerun = notify_sim_state(modem, (enum huawei_sim_state) sim_state);
> +
> +	if (rerun && data->sim_poll_count < MAX_SIM_POLL_COUNT) {
> +		data->sim_poll_count++;
> +		data->query_sim_state = g_timeout_add_seconds(2,
> +								query_sim_state,
> +								modem);

Now I don't like the query_sim_state name. It should sim_poll_timeout or
something similar. At least make it clear that it is timeout.

> +	}
> +}
> +
> +static gboolean query_sim_state(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct huawei_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	data->query_sim_state = 0;
> +
> +	g_at_chat_send(data->pcui, "AT^SYSINFO", sysinfo_prefix,
> +			sysinfo_cb, modem, NULL);
> +
> +	return FALSE;
>  }
>  
>  static void simst_notify(GAtResult *result, gpointer user_data)
> @@ -414,6 +452,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
>  	/* check for voice support */
>  	g_at_chat_send(data->pcui, "AT^CVOICE=?", cvoice_prefix,
>  					cvoice_support_cb, modem, NULL);
> +
> +	/* query_sim_state(modem); */
>  }

What is this ???

>  
>  static GAtChat *create_port(const char *device)
> @@ -546,6 +586,9 @@ static int huawei_disable(struct ofono_modem *modem)
>  
>  	DBG("%p", modem);
>  
> +	if (data->query_sim_state)
> +		g_source_remove(data->query_sim_state);
> +

You need to set the timeout variable also to 0. Otherwise you have some
funny side effect then next time enabling the modem.

Also I prefer check for the timeout variables with > 0.

>  	if (data->modem) {
>  		g_at_chat_cancel_all(data->modem);
>  		g_at_chat_unregister_all(data->modem);
> @@ -610,6 +653,9 @@ static void huawei_pre_sim(struct ofono_modem *modem)
>  	if (data->voice == TRUE)
>  		ofono_voicecall_create(modem, OFONO_VENDOR_HUAWEI,
>  						"atmodem", data->pcui);
> +
> +	data->sim_poll_count = 0;
> +	query_sim_state(modem);
>  }
>  
>  static void huawei_post_sim(struct ofono_modem *modem)

Regards

Marcel

Re: [RFC PATCH v1 2/2] huawei: fix online logic

[Marcel Holtmann]

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

Hi Kalle,

>  plugins/huawei.c |   79 ++++++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 55 insertions(+), 24 deletions(-)
> 
> diff --git a/plugins/huawei.c b/plugins/huawei.c
> index ad1c3ae..0b4e65e 100644
> --- a/plugins/huawei.c
> +++ b/plugins/huawei.c
> @@ -78,6 +78,7 @@ struct huawei_data {
>  	gboolean voice;
>  	guint query_sim_state;
>  	guint sim_poll_count;
> +	struct cb_data *online_cbd;
>  };

doing it like this make me suspisious that this is the right approach.
No other modem plugin has to do it this way. Why does Huawei?

Regards

Marcel

Re: [RFC PATCH v1 0/2] huawei: fix cold start

[Kalle Valo]

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

Kalle Valo <kalle.valo@canonical.com> writes:

> I finally was able to start look at Huawei again. Like reported multiple
> times, huawei currently works in only in the "warm start" case (ie. plug in 
> modem and restart ofono). This is a regression, this worked ok a month ago.
>
> Here's currently what I have, but pin locked sims still don't work. I'll
> send the patches as RFC anyway. Please test and give feedback.

Actually my locked sim problem was just user error, I was using
unlock-pin instead of enter-pin. Oops :) So pin locked sim is now
working with huawei.

So now I need to just figure out how handle the online failure case:

+			/* FIXME: how to handle failure? */
+			/* CALLBACK_WITH_FAILURE(cb, cbd->data); */

-- 
Kalle Valo

Re: [RFC PATCH v1 2/2] huawei: fix online logic

[Kalle Valo]

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

Marcel Holtmann <marcel@holtmann.org> writes:

> Hi Kalle,

Hi Marcel,

>>  plugins/huawei.c |   79 ++++++++++++++++++++++++++++++++++++++----------------
>>  1 files changed, 55 insertions(+), 24 deletions(-)
>> 
>> diff --git a/plugins/huawei.c b/plugins/huawei.c
>> index ad1c3ae..0b4e65e 100644
>> --- a/plugins/huawei.c
>> +++ b/plugins/huawei.c
>> @@ -78,6 +78,7 @@ struct huawei_data {
>>  	gboolean voice;
>>  	guint query_sim_state;
>>  	guint sim_poll_count;
>> +	struct cb_data *online_cbd;
>>  };
>
> doing it like this make me suspisious that this is the right approach.
> No other modem plugin has to do it this way. Why does Huawei?

I have no idea, it may very well be that doing the initialisation wrong.
But the problem at hand I have it is that huawei_post_online() makes
assumptions about sim_state:

	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->gc = ofono_gprs_context_create(modem, 0, "atmodem",
								data->modem);

		if (data->gprs && data->gc)
			ofono_gprs_add_context(data->gprs, data->gc);
	}

And currently (with my setup) data->sim_state will be
HUAWEI_SIM_STATE_INVALID_OR_LOCKED when post_online() is called and
hence gprs context is not initialised. I didn't add the sim_state checks
so I can't comment why they are needed. But this is the commit which
added them:

  commit f01de438bd4f834a27f9c36e74e6a1067f778a21
  Author: Jo.o Paulo Rechi Vita <jprvita@profusion.mobi>
  Date:   Thu Aug 5 16:13:08 2010 -0300

    huawei: Fix SIM state logic
    
    Add support for voice-only SIM cards and enable phonebook atom for
    non-voice modems.

I don't have that much time to work on this, I just want to get huawei
working again. But I'll try to do something.

I'm guessing ofono_sim_inserted_notify() has something to do this. When
I did some quick tests, IIRC I was able to see HUAWEI_SIM_STATE_VALID
only after I had called ofono_sim_inserted_notify(). 

I think the way you are after is that we would first receive
HUAWEI_SIM_STATE_VALID (or the three other valid states) and only then
call ofono_sim_inserted_notify(). At least to me that would make sense.
I just don't know how to do that because I see only later in the
initialisation phase.

Thoughts?

-- 
Kalle Valo

Re: [RFC PATCH v1 2/2] huawei: fix online logic

[Kalle Valo]

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

Kalle Valo <kalle.valo@canonical.com> writes:

> I'm guessing ofono_sim_inserted_notify() has something to do this. When
> I did some quick tests, IIRC I was able to see HUAWEI_SIM_STATE_VALID
> only after I had called ofono_sim_inserted_notify(). 
>
> I think the way you are after is that we would first receive
> HUAWEI_SIM_STATE_VALID (or the three other valid states) and only then
> call ofono_sim_inserted_notify(). At least to me that would make sense.
> I just don't know how to do that because I see only later in the
> initialisation phase.

Just to show my problem. I did this on top of my two huawei v1 patches:

diff --git a/plugins/huawei.c b/plugins/huawei.c
index 0b4e65e..ac26a02 100644
--- a/plugins/huawei.c
+++ b/plugins/huawei.c
@@ -185,9 +185,9 @@ static gboolean notify_sim_state(struct ofono_modem *modem,
        case HUAWEI_SIM_STATE_INVALID_OR_LOCKED:
                ofono_modem_set_powered(modem, TRUE);
 
-               if (data->sim)
-                       ofono_sim_inserted_notify(data->sim, TRUE);
-               break;
+               /* if (data->sim) */
+               /*      ofono_sim_inserted_notify(data->sim, TRUE); */
+               return TRUE;
        case HUAWEI_SIM_STATE_VALID:
        case HUAWEI_SIM_STATE_INVALID_CS:
        case HUAWEI_SIM_STATE_INVALID_PS:
@@ -199,8 +199,8 @@ static gboolean notify_sim_state(struct ofono_modem *modem,
                 */
                ofono_modem_set_powered(modem, TRUE);
 
-               if (data->sim)
-                       ofono_sim_inserted_notify(data->sim, TRUE);
+               /* if (data->sim) */
+               /*      ofono_sim_inserted_notify(data->sim, TRUE); */
 
                if (data->online_cbd) {
                        cb = data->online_cbd->cb;

So we are not calling sim_inserted() at all and poll for sim states for
a while. And the output in cold start case is this:

Sep 17 17:14:29 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 255
Sep 17 17:14:32 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0
Sep 17 17:14:32 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0
Sep 17 17:14:34 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0
Sep 17 17:14:34 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0
Sep 17 17:14:36 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0
Sep 17 17:14:36 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0
Sep 17 17:14:38 dell-m520 ofonod[2436]: plugins/huawei.c:notify_sim_state() 0

My modem always stays in HUAWEI_SIM_STATE_INVALID_OR_LOCKED (0) case
forever.

-- 
Kalle Valo

Re: [RFC PATCH v1 1/2] huawei: poll sim state

[Kalle Valo]

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

Marcel Holtmann <marcel@holtmann.org> writes:

> Hi Kalle,
>
>> On my Huawei E1552 when I plug in the modem (ie. cold start) with PIN locked
>> SIM, the sim state is 255 (HUAWEI_SIM_STATE_NOT_EXISTENT). As the modem
>> doesn't send ^SIMST notifications, poll the sim state until it's ready.
>> 
>> In theory it might be possible to do this better, for example follow
>> ^BOOT notifications or something, but it's unknown what parameter we
>> should check for.
>

[...]

>> +	if (sim_state == HUAWEI_SIM_STATE_NOT_EXISTENT) {
>>  		ofono_sim_inserted_notify(data->sim, FALSE);
>> +
>> +		/* SIM is not ready, try again a bit later */
>> +		return TRUE;
>> +	}
>>  	else
>>  		ofono_sim_inserted_notify(data->sim, TRUE);
>
> Coding style. } else on the same line. You should know this by now ;)

Oops :)

> Also in this case since you do return TRUE anyway from the if block, why
> bother with else. Just don't.

Good point. I changed that.
>> +	if (rerun && data->sim_poll_count < MAX_SIM_POLL_COUNT) {
>> +		data->sim_poll_count++;
>> +		data->query_sim_state = g_timeout_add_seconds(2,
>> +								query_sim_state,
>> +								modem);
>
> Now I don't like the query_sim_state name. It should sim_poll_timeout or
> something similar. At least make it clear that it is timeout.

I didn't know if you talked only about about the name of structure field
or also about the function name? I only changed the structure name.

>> +
>> +	/* query_sim_state(modem); */
>>  }
>
> What is this ???

A leftover from my tests. Removed

>>  static GAtChat *create_port(const char *device)
>> @@ -546,6 +586,9 @@ static int huawei_disable(struct ofono_modem *modem)
>>  
>>  	DBG("%p", modem);
>>  
>> +	if (data->query_sim_state)
>> +		g_source_remove(data->query_sim_state);
>> +
>
> You need to set the timeout variable also to 0. Otherwise you have some
> funny side effect then next time enabling the modem.
>
> Also I prefer check for the timeout variables with > 0.

Fixed.

I'll send v2 shortly.

-- 
Kalle Valo