[PATCH] wavecom: Re-work modem handling to improve stability

[PATCH] wavecom: Re-work modem handling to improve stability

[Holger Hans Peter Freyther]

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

From: Holger Hans Peter Freyther <holger@moiji-mobile.com>

This is the result of heavily looking at other plugins to
decide when and which objects to create.

* Use +WIND to get to know when the modem is ready
* Enable AT+CFUN=1/AT+CFUN=4 for online/offline handling
* Deal with GAtChat surviving a disable/enable cycle
---
 plugins/wavecom.c | 248 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 209 insertions(+), 39 deletions(-)

diff --git a/plugins/wavecom.c b/plugins/wavecom.c
index 7f24eae..cda5da0 100644
--- a/plugins/wavecom.c
+++ b/plugins/wavecom.c
@@ -47,35 +47,148 @@
 #include <ofono/sms.h>
 #include <ofono/ussd.h>
 #include <ofono/voicecall.h>
+#include <ofono.h>
 
+#include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
+static const char *cfun_prefix[] = { "+CFUN:", NULL };
+static const char *wind_prefix[] = { "+WIND:", NULL };
+static const char *none_prefix[] = { NULL };
+
+struct wavecom_data {
+	GAtChat *chat;
+	int have_enabled;
+	gboolean have_sim;
+	gboolean have_sms;
+};
+
+static enum ofono_vendor vendor(struct ofono_modem *modem)
+{
+	const char *model;
+
+	model = ofono_modem_get_string(modem, "Model");
+
+	if (model && strcmp(model, "Q2XXX") == 0)
+		return OFONO_VENDOR_WAVECOM_Q2XXX;
+
+	return 0;
+}
+
+static void wavecom_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	DBG("%s%s", prefix, str);
+}
 
 static int wavecom_probe(struct ofono_modem *modem)
 {
+	struct wavecom_data *data;
+
+	DBG("%p: %s", modem, __func__);
+
+	data = g_try_new0(struct wavecom_data, 1);
+
+	if (data == NULL)
+		return -ENOMEM;
+
+	ofono_modem_set_data(modem, data);
+
 	return 0;
 }
 
 static void wavecom_remove(struct ofono_modem *modem)
 {
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p: %s", modem, __func__);
+
+	ofono_modem_set_data(modem, NULL);
+	g_at_chat_unref(data->chat);
+	g_free(data);
+}
+
+static void rf_off_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+
+	ofono_modem_set_powered(modem, TRUE);
 }
 
-static void wavecom_debug(const char *str, void *user_data)
+static void wind_notify(GAtResult *result, gpointer user_data)
 {
-	const char *prefix = user_data;
+	struct ofono_modem *modem = user_data;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
+	GAtResultIter iter;
+	int val;
+
+	DBG("%p: %s", modem, __func__);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+WIND:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &val))
+		return;
+
+	switch (val) {
+	case 3: /* ready to process AT commands */
+		if (!data->have_enabled) {
+			data->have_enabled = TRUE;
+			g_at_chat_send(data->chat, "AT+CFUN=4", none_prefix,
+					rf_off_cb, modem, NULL);
+		}
+		break;
+	case 0: /* sim removed */
+		data->have_sim = FALSE;
+		if (sim)
+			ofono_sim_inserted_notify(sim, FALSE);
+		break;
+	case 1: /* sim inserted */
+		data->have_sim = TRUE;
+		break;
+	case 16: /* SMS and SMS-CB services initialized */
+		if (sim)
+			ofono_sim_inserted_notify(sim, TRUE);
+		break;
+	case 7: /* service available for emergency call */
+		break;
+	case 8: /* network is lost */
+	case 10: /* reload status of SIM phonebook */
+		break;
+	case 11: /* checksum of SIM phoenbook */
+	case 13: /* rack has been detected closed */
+		break;
+	}
+}
 
-	ofono_info("%s%s", prefix, str);
+static void wind_set(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	DBG("%s", __func__);
 }
 
 static int wavecom_enable(struct ofono_modem *modem)
 {
+	struct wavecom_data *data;
 	GAtChat *chat;
 	GIOChannel *channel;
 	GAtSyntax *syntax;
 	const char *device;
 	GHashTable *options;
 
-	DBG("%p", modem);
+	DBG("%p: %s", modem, __func__);
+
+	data = ofono_modem_get_data(modem);
+
+	if (data->chat) {
+		g_at_chat_cancel_all(data->chat);
+		g_at_chat_unregister_all(data->chat);
+		g_at_chat_unref(data->chat);
+		data->chat = NULL;
+	}
 
 	device = ofono_modem_get_string(modem, "Device");
 	if (device == NULL)
@@ -111,75 +224,130 @@ static int wavecom_enable(struct ofono_modem *modem)
 	if (chat == NULL)
 		return -ENOMEM;
 
+	g_at_chat_register(chat, "+WIND", wind_notify,
+				FALSE, modem, NULL);
+
 	g_at_chat_add_terminator(chat, "+CPIN:", 6, TRUE);
 
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(chat, wavecom_debug, "");
 
-	ofono_modem_set_data(modem, chat);
+	data->chat = chat;
 
-	return 0;
+	g_at_chat_send(chat, "AT+WIND=32767", wind_prefix,
+			wind_set, modem, NULL);
+
+	/* restart and wait for the +WIND */
+	data->have_enabled = FALSE;
+	data->have_sim = FALSE;
+	g_at_chat_send(chat, "AT+CFUN=1,1", none_prefix,
+			NULL, modem, NULL);
+	return -EINPROGRESS;
+}
+
+static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p: %s", modem, __func__);
+
+	g_at_chat_unref(data->chat);
+	data->chat = NULL;
+
+	if (ok)
+		ofono_modem_set_powered(modem, FALSE);
+	else
+		ofono_error("%p power down has failed!", modem);
 }
 
 static int wavecom_disable(struct ofono_modem *modem)
 {
-	GAtChat *chat = ofono_modem_get_data(modem);
+	struct wavecom_data *data = ofono_modem_get_data(modem);
 
-	DBG("%p", modem);
+	DBG("%p: %s", modem, __func__);
 
-	ofono_modem_set_data(modem, NULL);
+	g_at_chat_cancel_all(data->chat);
+	g_at_chat_unregister_all(data->chat);
 
-	g_at_chat_unref(chat);
+	g_at_chat_send(data->chat, "AT+CFUN=0", cfun_prefix,
+					cfun_disable, modem, NULL);
 
-	return 0;
+	return -EINPROGRESS;
+}
+
+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_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	cb(&error, cbd->data);
+}
+
+static void wavecom_set_online(struct ofono_modem *modem, ofono_bool_t online,
+				ofono_modem_online_cb_t cb, void *user_data)
+{
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+	char const *command = online ? "AT+CFUN=1,0" : "AT+CFUN=4";
+
+	DBG("%p: %s %d", modem, __func__, online);
+
+	if (g_at_chat_send(data->chat, command, cfun_prefix, set_online_cb,
+					cbd, g_free) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+	g_free(cbd);
 }
 
 static void wavecom_pre_sim(struct ofono_modem *modem)
 {
-	GAtChat *chat = ofono_modem_get_data(modem);
-	const char *model;
-	enum ofono_vendor vendor = 0;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
 	struct ofono_sim *sim;
 
-	DBG("%p", modem);
-
-	model = ofono_modem_get_string(modem, "Model");
-	if (model && strcmp(model, "Q2XXX") == 0)
-		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
+	DBG("%p: %s", modem, __func__);
 
-	ofono_devinfo_create(modem, 0, "atmodem", chat);
-	sim = ofono_sim_create(modem, vendor, "atmodem", chat);
-	ofono_voicecall_create(modem, 0, "atmodem", chat);
+	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
+	sim = ofono_sim_create(modem, vendor(modem), "atmodem", data->chat);
+	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
 
-	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
+	if (sim && data->have_sim == TRUE) {
+		data->have_sim = FALSE;
 		ofono_sim_inserted_notify(sim, TRUE);
+	}
 }
 
 static void wavecom_post_sim(struct ofono_modem *modem)
 {
-	GAtChat *chat = ofono_modem_get_data(modem);
-	struct ofono_message_waiting *mw;
-	const char *model;
-	enum ofono_vendor vendor = 0;
+	DBG("%p: %s", modem, __func__);
 
-	DBG("%p", modem);
+	/* TODO: Initialize GPRS support if we have an aux line */
+}
 
-	model = ofono_modem_get_string(modem, "Model");
-	if (model && strcmp(model, "Q2XXX") == 0)
-		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
+static void wavecom_post_online(struct ofono_modem *modem)
+{
+	struct ofono_message_waiting *mw;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
 
-	ofono_ussd_create(modem, 0, "atmodem", chat);
-	ofono_call_forwarding_create(modem, 0, "atmodem", chat);
-	ofono_call_settings_create(modem, 0, "atmodem", chat);
-	ofono_netreg_create(modem, 0, "atmodem", chat);
-	ofono_call_meter_create(modem, 0, "atmodem", chat);
-	ofono_call_barring_create(modem, 0, "atmodem", chat);
-	ofono_sms_create(modem, vendor, "atmodem", chat);
-	ofono_phonebook_create(modem, 0, "atmodem", chat);
+	DBG("%p: %s", modem, __func__);
+	ofono_ussd_create(modem, 0, "atmodem", data->chat);
+	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
+	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
+	ofono_netreg_create(modem, 0, "atmodem", data->chat);
+	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
+	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
 
 	mw = ofono_message_waiting_create(modem);
+
 	if (mw)
 		ofono_message_waiting_register(mw);
+
+	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
+	ofono_sms_create(modem, vendor(modem), "atmodem", data->chat);
 }
 
 static struct ofono_modem_driver wavecom_driver = {
@@ -188,8 +356,10 @@ static struct ofono_modem_driver wavecom_driver = {
 	.remove		= wavecom_remove,
 	.enable		= wavecom_enable,
 	.disable	= wavecom_disable,
+	.set_online	= wavecom_set_online,
 	.pre_sim	= wavecom_pre_sim,
 	.post_sim	= wavecom_post_sim,
+	.post_online	= wavecom_post_online,
 };
 
 static int wavecom_init(void)
-- 
2.6.3

Re: [PATCH] wavecom: Re-work modem handling to improve stability

[Denis Kenzior]

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

Hi Holger,

On 04/16/2016 10:36 AM, Holger Hans Peter Freyther wrote:
> From: Holger Hans Peter Freyther <holger@moiji-mobile.com>
>
> This is the result of heavily looking at other plugins to
> decide when and which objects to create.
>
> * Use +WIND to get to know when the modem is ready
> * Enable AT+CFUN=1/AT+CFUN=4 for online/offline handling
> * Deal with GAtChat surviving a disable/enable cycle
> ---
>   plugins/wavecom.c | 248 +++++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 209 insertions(+), 39 deletions(-)
>
> diff --git a/plugins/wavecom.c b/plugins/wavecom.c
> index 7f24eae..cda5da0 100644
> --- a/plugins/wavecom.c
> +++ b/plugins/wavecom.c
> @@ -47,35 +47,148 @@
>   #include <ofono/sms.h>
>   #include <ofono/ussd.h>
>   #include <ofono/voicecall.h>
> +#include <ofono.h>
>
> +#include <drivers/atmodem/atutil.h>
>   #include <drivers/atmodem/vendor.h>
>
> +static const char *cfun_prefix[] = { "+CFUN:", NULL };
> +static const char *wind_prefix[] = { "+WIND:", NULL };
> +static const char *none_prefix[] = { NULL };
> +
> +struct wavecom_data {
> +	GAtChat *chat;
> +	int have_enabled;
> +	gboolean have_sim;
> +	gboolean have_sms;
> +};
> +
> +static enum ofono_vendor vendor(struct ofono_modem *modem)
> +{
> +	const char *model;
> +
> +	model = ofono_modem_get_string(modem, "Model");
> +
> +	if (model && strcmp(model, "Q2XXX") == 0)
> +		return OFONO_VENDOR_WAVECOM_Q2XXX;
> +
> +	return 0;
> +}
> +
> +static void wavecom_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	DBG("%s%s", prefix, str);
> +}
>
>   static int wavecom_probe(struct ofono_modem *modem)
>   {
> +	struct wavecom_data *data;
> +
> +	DBG("%p: %s", modem, __func__);
> +
> +	data = g_try_new0(struct wavecom_data, 1);
> +
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ofono_modem_set_data(modem, data);
> +
>   	return 0;
>   }
>
>   static void wavecom_remove(struct ofono_modem *modem)
>   {
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p: %s", modem, __func__);
> +
> +	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_unref(data->chat);
> +	g_free(data);
> +}
> +
> +static void rf_off_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +
> +	ofono_modem_set_powered(modem, TRUE);
>   }
>
> -static void wavecom_debug(const char *str, void *user_data)
> +static void wind_notify(GAtResult *result, gpointer user_data)
>   {
> -	const char *prefix = user_data;
> +	struct ofono_modem *modem = user_data;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	GAtResultIter iter;
> +	int val;
> +
> +	DBG("%p: %s", modem, __func__);
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+WIND:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &val))
> +		return;
> +
> +	switch (val) {
> +	case 3: /* ready to process AT commands */
> +		if (!data->have_enabled) {
> +			data->have_enabled = TRUE;
> +			g_at_chat_send(data->chat, "AT+CFUN=4", none_prefix,
> +					rf_off_cb, modem, NULL);
> +		}
> +		break;
> +	case 0: /* sim removed */
> +		data->have_sim = FALSE;
> +		if (sim)
> +			ofono_sim_inserted_notify(sim, FALSE);
> +		break;
> +	case 1: /* sim inserted */
> +		data->have_sim = TRUE;
> +		break;
> +	case 16: /* SMS and SMS-CB services initialized */
> +		if (sim)
> +			ofono_sim_inserted_notify(sim, TRUE);
> +		break;
> +	case 7: /* service available for emergency call */
> +		break;
> +	case 8: /* network is lost */
> +	case 10: /* reload status of SIM phonebook */
> +		break;
> +	case 11: /* checksum of SIM phoenbook */
> +	case 13: /* rack has been detected closed */
> +		break;
> +	}
> +}
>
> -	ofono_info("%s%s", prefix, str);
> +static void wind_set(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	DBG("%s", __func__);

DBG already prints the file and function information, so this is redundant.

>   }
>
>   static int wavecom_enable(struct ofono_modem *modem)
>   {
> +	struct wavecom_data *data;
>   	GAtChat *chat;
>   	GIOChannel *channel;
>   	GAtSyntax *syntax;
>   	const char *device;
>   	GHashTable *options;
>
> -	DBG("%p", modem);
> +	DBG("%p: %s", modem, __func__);

as above

> +
> +	data = ofono_modem_get_data(modem);
> +
> +	if (data->chat) {
> +		g_at_chat_cancel_all(data->chat);
> +		g_at_chat_unregister_all(data->chat);
> +		g_at_chat_unref(data->chat);
> +		data->chat = NULL;
> +	}
>
>   	device = ofono_modem_get_string(modem, "Device");
>   	if (device == NULL)
> @@ -111,75 +224,130 @@ static int wavecom_enable(struct ofono_modem *modem)
>   	if (chat == NULL)
>   		return -ENOMEM;
>
> +	g_at_chat_register(chat, "+WIND", wind_notify,
> +				FALSE, modem, NULL);
> +
>   	g_at_chat_add_terminator(chat, "+CPIN:", 6, TRUE);
>
>   	if (getenv("OFONO_AT_DEBUG"))
>   		g_at_chat_set_debug(chat, wavecom_debug, "");
>
> -	ofono_modem_set_data(modem, chat);
> +	data->chat = chat;
>
> -	return 0;
> +	g_at_chat_send(chat, "AT+WIND=32767", wind_prefix,
> +			wind_set, modem, NULL);
> +
> +	/* restart and wait for the +WIND */
> +	data->have_enabled = FALSE;
> +	data->have_sim = FALSE;
> +	g_at_chat_send(chat, "AT+CFUN=1,1", none_prefix,
> +			NULL, modem, NULL);
> +	return -EINPROGRESS;
> +}
> +
> +static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p: %s", modem, __func__);
> +
> +	g_at_chat_unref(data->chat);
> +	data->chat = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +	else
> +		ofono_error("%p power down has failed!", modem);
>   }
>
>   static int wavecom_disable(struct ofono_modem *modem)
>   {
> -	GAtChat *chat = ofono_modem_get_data(modem);
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
>
> -	DBG("%p", modem);
> +	DBG("%p: %s", modem, __func__);
>
> -	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_cancel_all(data->chat);
> +	g_at_chat_unregister_all(data->chat);
>
> -	g_at_chat_unref(chat);
> +	g_at_chat_send(data->chat, "AT+CFUN=0", cfun_prefix,
> +					cfun_disable, modem, NULL);
>
> -	return 0;
> +	return -EINPROGRESS;
> +}
> +
> +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_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +	cb(&error, cbd->data);
> +}
> +
> +static void wavecom_set_online(struct ofono_modem *modem, ofono_bool_t online,
> +				ofono_modem_online_cb_t cb, void *user_data)
> +{
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	char const *command = online ? "AT+CFUN=1,0" : "AT+CFUN=4";
> +
> +	DBG("%p: %s %d", modem, __func__, online);
> +
> +	if (g_at_chat_send(data->chat, command, cfun_prefix, set_online_cb,
> +					cbd, g_free) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> +	g_free(cbd);
>   }
>
>   static void wavecom_pre_sim(struct ofono_modem *modem)
>   {
> -	GAtChat *chat = ofono_modem_get_data(modem);
> -	const char *model;
> -	enum ofono_vendor vendor = 0;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
>   	struct ofono_sim *sim;
>
> -	DBG("%p", modem);
> -
> -	model = ofono_modem_get_string(modem, "Model");
> -	if (model && strcmp(model, "Q2XXX") == 0)
> -		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
> +	DBG("%p: %s", modem, __func__);
>
> -	ofono_devinfo_create(modem, 0, "atmodem", chat);
> -	sim = ofono_sim_create(modem, vendor, "atmodem", chat);
> -	ofono_voicecall_create(modem, 0, "atmodem", chat);
> +	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
> +	sim = ofono_sim_create(modem, vendor(modem), "atmodem", data->chat);
> +	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
>
> -	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
> +	if (sim && data->have_sim == TRUE) {
> +		data->have_sim = FALSE;
>   		ofono_sim_inserted_notify(sim, TRUE);
> +	}
>   }
>
>   static void wavecom_post_sim(struct ofono_modem *modem)
>   {
> -	GAtChat *chat = ofono_modem_get_data(modem);
> -	struct ofono_message_waiting *mw;
> -	const char *model;
> -	enum ofono_vendor vendor = 0;
> +	DBG("%p: %s", modem, __func__);
>
> -	DBG("%p", modem);
> +	/* TODO: Initialize GPRS support if we have an aux line */
> +}
>
> -	model = ofono_modem_get_string(modem, "Model");
> -	if (model && strcmp(model, "Q2XXX") == 0)
> -		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
> +static void wavecom_post_online(struct ofono_modem *modem)
> +{
> +	struct ofono_message_waiting *mw;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
>
> -	ofono_ussd_create(modem, 0, "atmodem", chat);
> -	ofono_call_forwarding_create(modem, 0, "atmodem", chat);
> -	ofono_call_settings_create(modem, 0, "atmodem", chat);
> -	ofono_netreg_create(modem, 0, "atmodem", chat);
> -	ofono_call_meter_create(modem, 0, "atmodem", chat);
> -	ofono_call_barring_create(modem, 0, "atmodem", chat);
> -	ofono_sms_create(modem, vendor, "atmodem", chat);
> -	ofono_phonebook_create(modem, 0, "atmodem", chat);
> +	DBG("%p: %s", modem, __func__);
> +	ofono_ussd_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
> +	ofono_netreg_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
>
>   	mw = ofono_message_waiting_create(modem);
> +
>   	if (mw)
>   		ofono_message_waiting_register(mw);
> +
> +	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
> +	ofono_sms_create(modem, vendor(modem), "atmodem", data->chat);
>   }
>
>   static struct ofono_modem_driver wavecom_driver = {
> @@ -188,8 +356,10 @@ static struct ofono_modem_driver wavecom_driver = {
>   	.remove		= wavecom_remove,
>   	.enable		= wavecom_enable,
>   	.disable	= wavecom_disable,
> +	.set_online	= wavecom_set_online,
>   	.pre_sim	= wavecom_pre_sim,
>   	.post_sim	= wavecom_post_sim,
> +	.post_online	= wavecom_post_online,
>   };
>
>   static int wavecom_init(void)
>

All this looks fine except for the DBG statements ;)

Regards,
-Denis

[PATCH] wavecom: Re-work modem handling to improve stability

[Holger Hans Peter Freyther]

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

From: Holger Hans Peter Freyther <holger@moiji-mobile.com>

This is the result of heavily looking at other plugins to
decide when and which objects to create.

* Use +WIND to get to know when the modem is ready
* Enable AT+CFUN=1/AT+CFUN=4 for online/offline handling
* Deal with GAtChat surviving a disable/enable cycle
---
 plugins/wavecom.c | 248 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 209 insertions(+), 39 deletions(-)

diff --git a/plugins/wavecom.c b/plugins/wavecom.c
index 7f24eae..c3b0b52 100644
--- a/plugins/wavecom.c
+++ b/plugins/wavecom.c
@@ -47,35 +47,145 @@
 #include <ofono/sms.h>
 #include <ofono/ussd.h>
 #include <ofono/voicecall.h>
+#include <ofono.h>
 
+#include <drivers/atmodem/atutil.h>
 #include <drivers/atmodem/vendor.h>
 
+static const char *cfun_prefix[] = { "+CFUN:", NULL };
+static const char *wind_prefix[] = { "+WIND:", NULL };
+static const char *none_prefix[] = { NULL };
+
+struct wavecom_data {
+	GAtChat *chat;
+	int have_enabled;
+	gboolean have_sim;
+	gboolean have_sms;
+};
+
+static enum ofono_vendor vendor(struct ofono_modem *modem)
+{
+	const char *model;
+
+	model = ofono_modem_get_string(modem, "Model");
+	if (model && strcmp(model, "Q2XXX") == 0)
+		return OFONO_VENDOR_WAVECOM_Q2XXX;
+	return 0;
+}
+
+static void wavecom_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	ofono_info("%s%s", prefix, str);
+}
 
 static int wavecom_probe(struct ofono_modem *modem)
 {
+	struct wavecom_data *data;
+
+	ofono_info("%p: %s", modem, __func__);
+
+	data = g_try_new0(struct wavecom_data, 1);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ofono_modem_set_data(modem, data);
+
 	return 0;
 }
 
 static void wavecom_remove(struct ofono_modem *modem)
 {
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+
+	ofono_info("%p: %s", modem, __func__);
+
+	ofono_modem_set_data(modem, NULL);
+	g_at_chat_unref(data->chat);
+	g_free(data);
 }
 
-static void wavecom_debug(const char *str, void *user_data)
+static void rf_off_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
-	const char *prefix = user_data;
+	struct ofono_modem *modem = user_data;
 
-	ofono_info("%s%s", prefix, str);
+	ofono_modem_set_powered(modem, TRUE);
+}
+
+static void wind_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
+	GAtResultIter iter;
+	int val;
+
+	ofono_info("%p: %s", modem, __func__);
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+WIND:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &val))
+		return;
+
+	switch (val) {
+	case 3: /* ready to process AT commands */
+		if (!data->have_enabled) {
+			data->have_enabled = TRUE;
+			g_at_chat_send(data->chat, "AT+CFUN=4", none_prefix, rf_off_cb, modem, NULL);
+		}
+		break;
+	case 0: /* sim removed */
+		data->have_sim = FALSE;
+		if (sim)
+			ofono_sim_inserted_notify(sim, FALSE);
+		break;
+	case 1: /* sim inserted */
+		data->have_sim = TRUE;
+		break;
+	case 16: /* SMS and SMS-CB services initialized */
+		if (sim)
+			ofono_sim_inserted_notify(sim, TRUE);
+		break;
+	case 7: /* service available for emergency call */
+		break;
+	case 8: /* network is lost */
+	case 10: /* reload status of SIM phonebook */
+		break;
+	case 11: /* checksum of SIM phoenbook */
+	case 13: /* rack has been detected closed */
+		break;
+	}
+}
+
+
+static void wind_set(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	ofono_info("%s", __func__);
 }
 
 static int wavecom_enable(struct ofono_modem *modem)
 {
+	struct wavecom_data *data;
 	GAtChat *chat;
 	GIOChannel *channel;
 	GAtSyntax *syntax;
 	const char *device;
 	GHashTable *options;
 
-	DBG("%p", modem);
+	ofono_info("%p: %s", modem, __func__);
+
+	data = ofono_modem_get_data(modem);
+
+	if (data->chat) {
+		g_at_chat_cancel_all(data->chat);
+		g_at_chat_unregister_all(data->chat);
+		g_at_chat_unref(data->chat);
+		data->chat = NULL;
+	}
 
 	device = ofono_modem_get_string(modem, "Device");
 	if (device == NULL)
@@ -111,75 +221,133 @@ static int wavecom_enable(struct ofono_modem *modem)
 	if (chat == NULL)
 		return -ENOMEM;
 
+	g_at_chat_register(chat, "+WIND", wind_notify,
+			   FALSE, modem, NULL);
+
 	g_at_chat_add_terminator(chat, "+CPIN:", 6, TRUE);
 
 	if (getenv("OFONO_AT_DEBUG"))
 		g_at_chat_set_debug(chat, wavecom_debug, "");
 
-	ofono_modem_set_data(modem, chat);
+	data->chat = chat;
 
-	return 0;
+	g_at_chat_send(chat, "AT+WIND=32767", wind_prefix,
+			wind_set, modem, NULL);
+
+	/* restart and wait for the +WIND */
+	data->have_enabled = FALSE;
+	data->have_sim = FALSE;
+	g_at_chat_send(chat, "AT+CFUN=1,1", none_prefix,
+			NULL, modem, NULL);
+	return -EINPROGRESS;
+}
+
+static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+
+	ofono_info("%p: %s", modem, __func__);
+
+	g_at_chat_unref(data->chat);
+	data->chat = NULL;
+
+	if (ok)
+		ofono_modem_set_powered(modem, FALSE);
+	else
+		ofono_error("%p power down has failed!", modem);
 }
 
 static int wavecom_disable(struct ofono_modem *modem)
 {
-	GAtChat *chat = ofono_modem_get_data(modem);
+	struct wavecom_data *data = ofono_modem_get_data(modem);
 
-	DBG("%p", modem);
+	ofono_info("%p: %s", modem, __func__);
 
-	ofono_modem_set_data(modem, NULL);
+	g_at_chat_cancel_all(data->chat);
+	g_at_chat_unregister_all(data->chat);
 
-	g_at_chat_unref(chat);
+	g_at_chat_send(data->chat, "AT+CFUN=0", cfun_prefix,
+					cfun_disable, modem, NULL);
 
-	return 0;
+	return -EINPROGRESS;
+}
+
+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_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	cb(&error, cbd->data);
+}
+
+static void wavecom_set_online(struct ofono_modem *modem, ofono_bool_t online,
+				ofono_modem_online_cb_t cb, void *user_data)
+{
+	struct wavecom_data *data = ofono_modem_get_data(modem);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+	char const *command = online ? "AT+CFUN=1,0" : "AT+CFUN=4";
+
+	ofono_info("%p: %s %d", modem, __func__, online);
+
+	if (g_at_chat_send(data->chat, command, cfun_prefix, set_online_cb,
+					cbd, g_free) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+	g_free(cbd);
 }
 
 static void wavecom_pre_sim(struct ofono_modem *modem)
 {
-	GAtChat *chat = ofono_modem_get_data(modem);
-	const char *model;
-	enum ofono_vendor vendor = 0;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
 	struct ofono_sim *sim;
 
-	DBG("%p", modem);
-
-	model = ofono_modem_get_string(modem, "Model");
-	if (model && strcmp(model, "Q2XXX") == 0)
-		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
+	ofono_info("%p: %s", modem, __func__);
 
-	ofono_devinfo_create(modem, 0, "atmodem", chat);
-	sim = ofono_sim_create(modem, vendor, "atmodem", chat);
-	ofono_voicecall_create(modem, 0, "atmodem", chat);
+	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
+	sim = ofono_sim_create(modem, vendor(modem), "atmodem", data->chat);
+	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
 
-	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
+	if (sim && data->have_sim == TRUE) {
+		data->have_sim = FALSE;
 		ofono_sim_inserted_notify(sim, TRUE);
+	}
 }
 
 static void wavecom_post_sim(struct ofono_modem *modem)
 {
-	GAtChat *chat = ofono_modem_get_data(modem);
-	struct ofono_message_waiting *mw;
-	const char *model;
-	enum ofono_vendor vendor = 0;
+	ofono_info("%p: %s", modem, __func__);
 
-	DBG("%p", modem);
+	/* TODO: Initialize GPRS support if we have an aux line */
+	/*
+	 * FIXME: This only seems to be called when no immediate
+	 * enabled -> online change is called
+	 */
+}
 
-	model = ofono_modem_get_string(modem, "Model");
-	if (model && strcmp(model, "Q2XXX") == 0)
-		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
+static void wavecom_post_online(struct ofono_modem *modem)
+{
+	struct ofono_message_waiting *mw;
+	struct wavecom_data *data = ofono_modem_get_data(modem);
 
-	ofono_ussd_create(modem, 0, "atmodem", chat);
-	ofono_call_forwarding_create(modem, 0, "atmodem", chat);
-	ofono_call_settings_create(modem, 0, "atmodem", chat);
-	ofono_netreg_create(modem, 0, "atmodem", chat);
-	ofono_call_meter_create(modem, 0, "atmodem", chat);
-	ofono_call_barring_create(modem, 0, "atmodem", chat);
-	ofono_sms_create(modem, vendor, "atmodem", chat);
-	ofono_phonebook_create(modem, 0, "atmodem", chat);
+	ofono_info("%p: %s", modem, __func__);
+	ofono_ussd_create(modem, 0, "atmodem", data->chat);
+	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
+	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
+	ofono_netreg_create(modem, 0, "atmodem", data->chat);
+	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
+	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
 
 	mw = ofono_message_waiting_create(modem);
 	if (mw)
 		ofono_message_waiting_register(mw);
+
+	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
+	ofono_sms_create(modem, vendor(modem), "atmodem", data->chat);
 }
 
 static struct ofono_modem_driver wavecom_driver = {
@@ -188,8 +356,10 @@ static struct ofono_modem_driver wavecom_driver = {
 	.remove		= wavecom_remove,
 	.enable		= wavecom_enable,
 	.disable	= wavecom_disable,
+	.set_online	= wavecom_set_online,
 	.pre_sim	= wavecom_pre_sim,
 	.post_sim	= wavecom_post_sim,
+	.post_online	= wavecom_post_online,
 };
 
 static int wavecom_init(void)
-- 
2.6.3

Re: [PATCH] wavecom: Re-work modem handling to improve stability

[Denis Kenzior]

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

Hi Holger,

On 04/05/2016 02:25 PM, Holger Hans Peter Freyther wrote:
> From: Holger Hans Peter Freyther <holger@moiji-mobile.com>
>
> This is the result of heavily looking at other plugins to
> decide when and which objects to create.
>
> * Use +WIND to get to know when the modem is ready
> * Enable AT+CFUN=1/AT+CFUN=4 for online/offline handling
> * Deal with GAtChat surviving a disable/enable cycle
> ---
>   plugins/wavecom.c | 248 +++++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 209 insertions(+), 39 deletions(-)
>
> diff --git a/plugins/wavecom.c b/plugins/wavecom.c
> index 7f24eae..c3b0b52 100644
> --- a/plugins/wavecom.c
> +++ b/plugins/wavecom.c
> @@ -47,35 +47,145 @@
>   #include <ofono/sms.h>
>   #include <ofono/ussd.h>
>   #include <ofono/voicecall.h>
> +#include <ofono.h>
>
> +#include <drivers/atmodem/atutil.h>
>   #include <drivers/atmodem/vendor.h>
>
> +static const char *cfun_prefix[] = { "+CFUN:", NULL };
> +static const char *wind_prefix[] = { "+WIND:", NULL };
> +static const char *none_prefix[] = { NULL };
> +
> +struct wavecom_data {
> +	GAtChat *chat;
> +	int have_enabled;
> +	gboolean have_sim;
> +	gboolean have_sms;
> +};
> +
> +static enum ofono_vendor vendor(struct ofono_modem *modem)
> +{
> +	const char *model;
> +
> +	model = ofono_modem_get_string(modem, "Model");
> +	if (model && strcmp(model, "Q2XXX") == 0)
> +		return OFONO_VENDOR_WAVECOM_Q2XXX;
> +	return 0;

Just a minor nitpick here, but doc/coding-style.txt, item M1

> +}
> +
> +static void wavecom_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	ofono_info("%s%s", prefix, str);
> +}
>
>   static int wavecom_probe(struct ofono_modem *modem)
>   {
> +	struct wavecom_data *data;
> +
> +	ofono_info("%p: %s", modem, __func__);

This one should use the DBG macro, not ofono_info

> +
> +	data = g_try_new0(struct wavecom_data, 1);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ofono_modem_set_data(modem, data);
> +
>   	return 0;
>   }
>
>   static void wavecom_remove(struct ofono_modem *modem)
>   {
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +
> +	ofono_info("%p: %s", modem, __func__);

As above

> +
> +	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_unref(data->chat);
> +	g_free(data);
>   }
>
> -static void wavecom_debug(const char *str, void *user_data)
> +static void rf_off_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   {
> -	const char *prefix = user_data;
> +	struct ofono_modem *modem = user_data;
>
> -	ofono_info("%s%s", prefix, str);
> +	ofono_modem_set_powered(modem, TRUE);
> +}
> +
> +static void wind_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	GAtResultIter iter;
> +	int val;
> +
> +	ofono_info("%p: %s", modem, __func__);

DBG

> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+WIND:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &val))
> +		return;
> +
> +	switch (val) {
> +	case 3: /* ready to process AT commands */
> +		if (!data->have_enabled) {
> +			data->have_enabled = TRUE;
> +			g_at_chat_send(data->chat, "AT+CFUN=4", none_prefix, rf_off_cb, modem, NULL);

This line is over 80 characters, see doc/coding-style.txt

> +		}
> +		break;
> +	case 0: /* sim removed */
> +		data->have_sim = FALSE;
> +		if (sim)
> +			ofono_sim_inserted_notify(sim, FALSE);
> +		break;
> +	case 1: /* sim inserted */
> +		data->have_sim = TRUE;
> +		break;
> +	case 16: /* SMS and SMS-CB services initialized */
> +		if (sim)
> +			ofono_sim_inserted_notify(sim, TRUE);
> +		break;
> +	case 7: /* service available for emergency call */
> +		break;
> +	case 8: /* network is lost */
> +	case 10: /* reload status of SIM phonebook */
> +		break;
> +	case 11: /* checksum of SIM phoenbook */
> +	case 13: /* rack has been detected closed */
> +		break;
> +	}
> +}
> +
> +

Double empty lines are not allowed

> +static void wind_set(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	ofono_info("%s", __func__);
>   }
>
>   static int wavecom_enable(struct ofono_modem *modem)
>   {
> +	struct wavecom_data *data;
>   	GAtChat *chat;
>   	GIOChannel *channel;
>   	GAtSyntax *syntax;
>   	const char *device;
>   	GHashTable *options;
>
> -	DBG("%p", modem);
> +	ofono_info("%p: %s", modem, __func__);

DBG please

> +
> +	data = ofono_modem_get_data(modem);
> +
> +	if (data->chat) {
> +		g_at_chat_cancel_all(data->chat);
> +		g_at_chat_unregister_all(data->chat);
> +		g_at_chat_unref(data->chat);
> +		data->chat = NULL;
> +	}
>
>   	device = ofono_modem_get_string(modem, "Device");
>   	if (device == NULL)
> @@ -111,75 +221,133 @@ static int wavecom_enable(struct ofono_modem *modem)
>   	if (chat == NULL)
>   		return -ENOMEM;
>
> +	g_at_chat_register(chat, "+WIND", wind_notify,
> +			   FALSE, modem, NULL);
> +
>   	g_at_chat_add_terminator(chat, "+CPIN:", 6, TRUE);
>
>   	if (getenv("OFONO_AT_DEBUG"))
>   		g_at_chat_set_debug(chat, wavecom_debug, "");
>
> -	ofono_modem_set_data(modem, chat);
> +	data->chat = chat;
>
> -	return 0;
> +	g_at_chat_send(chat, "AT+WIND=32767", wind_prefix,
> +			wind_set, modem, NULL);
> +
> +	/* restart and wait for the +WIND */
> +	data->have_enabled = FALSE;
> +	data->have_sim = FALSE;
> +	g_at_chat_send(chat, "AT+CFUN=1,1", none_prefix,
> +			NULL, modem, NULL);

Please always use tabs for indentation.  Our coding style does not allow 
mixed spaces / tabs.

> +	return -EINPROGRESS;
> +}
> +
> +static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +
> +	ofono_info("%p: %s", modem, __func__);
> +

DBG

> +	g_at_chat_unref(data->chat);
> +	data->chat = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +	else
> +		ofono_error("%p power down has failed!", modem);
>   }
>
>   static int wavecom_disable(struct ofono_modem *modem)
>   {
> -	GAtChat *chat = ofono_modem_get_data(modem);
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
>
> -	DBG("%p", modem);
> +	ofono_info("%p: %s", modem, __func__);

DBG

>
> -	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_cancel_all(data->chat);
> +	g_at_chat_unregister_all(data->chat);
>
> -	g_at_chat_unref(chat);
> +	g_at_chat_send(data->chat, "AT+CFUN=0", cfun_prefix,
> +					cfun_disable, modem, NULL);
>
> -	return 0;
> +	return -EINPROGRESS;
> +}
> +
> +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_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +	cb(&error, cbd->data);
> +}
> +
> +static void wavecom_set_online(struct ofono_modem *modem, ofono_bool_t online,
> +				ofono_modem_online_cb_t cb, void *user_data)
> +{
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
> +	struct cb_data *cbd = cb_data_new(cb, user_data);
> +	char const *command = online ? "AT+CFUN=1,0" : "AT+CFUN=4";
> +
> +	ofono_info("%p: %s %d", modem, __func__, online);
> +
> +	if (g_at_chat_send(data->chat, command, cfun_prefix, set_online_cb,
> +					cbd, g_free) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> +	g_free(cbd);
>   }
>
>   static void wavecom_pre_sim(struct ofono_modem *modem)
>   {
> -	GAtChat *chat = ofono_modem_get_data(modem);
> -	const char *model;
> -	enum ofono_vendor vendor = 0;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
>   	struct ofono_sim *sim;
>
> -	DBG("%p", modem);
> -
> -	model = ofono_modem_get_string(modem, "Model");
> -	if (model && strcmp(model, "Q2XXX") == 0)
> -		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
> +	ofono_info("%p: %s", modem, __func__);
>
> -	ofono_devinfo_create(modem, 0, "atmodem", chat);
> -	sim = ofono_sim_create(modem, vendor, "atmodem", chat);
> -	ofono_voicecall_create(modem, 0, "atmodem", chat);
> +	ofono_devinfo_create(modem, 0, "atmodem", data->chat);
> +	sim = ofono_sim_create(modem, vendor(modem), "atmodem", data->chat);
> +	ofono_voicecall_create(modem, 0, "atmodem", data->chat);
>
> -	if (vendor == OFONO_VENDOR_WAVECOM_Q2XXX)
> +	if (sim && data->have_sim == TRUE) {
> +		data->have_sim = FALSE;
>   		ofono_sim_inserted_notify(sim, TRUE);
> +	}
>   }
>
>   static void wavecom_post_sim(struct ofono_modem *modem)
>   {
> -	GAtChat *chat = ofono_modem_get_data(modem);
> -	struct ofono_message_waiting *mw;
> -	const char *model;
> -	enum ofono_vendor vendor = 0;
> +	ofono_info("%p: %s", modem, __func__);
>
> -	DBG("%p", modem);
> +	/* TODO: Initialize GPRS support if we have an aux line */
> +	/*
> +	 * FIXME: This only seems to be called when no immediate
> +	 * enabled -> online change is called
> +	 */

??

> +}
>
> -	model = ofono_modem_get_string(modem, "Model");
> -	if (model && strcmp(model, "Q2XXX") == 0)
> -		vendor = OFONO_VENDOR_WAVECOM_Q2XXX;
> +static void wavecom_post_online(struct ofono_modem *modem)
> +{
> +	struct ofono_message_waiting *mw;
> +	struct wavecom_data *data = ofono_modem_get_data(modem);
>
> -	ofono_ussd_create(modem, 0, "atmodem", chat);
> -	ofono_call_forwarding_create(modem, 0, "atmodem", chat);
> -	ofono_call_settings_create(modem, 0, "atmodem", chat);
> -	ofono_netreg_create(modem, 0, "atmodem", chat);
> -	ofono_call_meter_create(modem, 0, "atmodem", chat);
> -	ofono_call_barring_create(modem, 0, "atmodem", chat);
> -	ofono_sms_create(modem, vendor, "atmodem", chat);
> -	ofono_phonebook_create(modem, 0, "atmodem", chat);
> +	ofono_info("%p: %s", modem, __func__);
> +	ofono_ussd_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_forwarding_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_settings_create(modem, 0, "atmodem", data->chat);
> +	ofono_netreg_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_meter_create(modem, 0, "atmodem", data->chat);
> +	ofono_call_barring_create(modem, 0, "atmodem", data->chat);
>
>   	mw = ofono_message_waiting_create(modem);
>   	if (mw)
>   		ofono_message_waiting_register(mw);
> +
> +	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
> +	ofono_sms_create(modem, vendor(modem), "atmodem", data->chat);
>   }
>
>   static struct ofono_modem_driver wavecom_driver = {
> @@ -188,8 +356,10 @@ static struct ofono_modem_driver wavecom_driver = {
>   	.remove		= wavecom_remove,
>   	.enable		= wavecom_enable,
>   	.disable	= wavecom_disable,
> +	.set_online	= wavecom_set_online,
>   	.pre_sim	= wavecom_pre_sim,
>   	.post_sim	= wavecom_post_sim,
> +	.post_online	= wavecom_post_online,
>   };
>
>   static int wavecom_init(void)
>

Please note that you can debug the wavecom plugin specifically by 
starting ofonod with -d '*wavecom*'

Regards,
-Denis

Re: [PATCH] wavecom: Re-work modem handling to improve stability

[Holger Freyther]

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

Denis Kenzior <denkenz@...> writes:

> 
> Hi Holger,

Dear Denis,


> 
> Just a minor nitpick here, but doc/coding-style.txt, item M1

sorry about that. I was not sure about. I should have addressed
everything now.



> > -	DBG("%p", modem);
> > +	/* TODO: Initialize GPRS support if we have an aux line */
> > +	/*
> > +	 * FIXME: This only seems to be called when no immediate
> > +	 * enabled -> online change is called
> > +	 */
> 
> ??

I had sent a separate mail about that. For some reason (well, we actually
patched the state machine in modem.c and I have to re-test), this post
sim cb was never called.

I will try to re-test when I am back to the office next week.


> 
> Please note that you can debug the wavecom plugin specifically by 
> starting ofonod with -d '*wavecom*'

thanks, I assumed it would be possible but sed -i s,DBG,o..error, was
easier when developing. :)

thank you for your fast review

  holger