[PATCH 0/4] Add a driver for Quectel UC15 modules

[PATCH 0/4] Add a driver for Quectel UC15 modules

[Philip Paeps]

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

Not very exciting as drivers go. :)

Philip Paeps (4):
  atmodem: add vendor Quectel
  udevng: add setup logic for Quectel UC15 modules
  plugins: add a new driver for Quectel UC15 modules
  sim: query Quectel UC15 PIN retries with AT+QPINQ

 Makefile.am              |    3 +
 drivers/atmodem/sim.c    |   49 +++++++
 drivers/atmodem/vendor.h |    3 +-
 plugins/quectel.c        |  335 ++++++++++++++++++++++++++++++++++++++++++++++
 plugins/udevng.c         |   37 +++++
 5 files changed, 426 insertions(+), 1 deletion(-)
 create mode 100644 plugins/quectel.c

-- 
1.7.10.4

[PATCH 1/4] atmodem: add vendor Quectel

[Philip Paeps]

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

---
 drivers/atmodem/vendor.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index bf2b38a..05ec872 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -42,5 +42,6 @@ enum ofono_vendor {
 	OFONO_VENDOR_SIMCOM_SIM900,
 	OFONO_VENDOR_ICERA,
 	OFONO_VENDOR_WAVECOM_Q2XXX,
-	OFONO_VENDOR_ALCATEL
+	OFONO_VENDOR_ALCATEL,
+	OFONO_VENDOR_QUECTEL
 };
-- 
1.7.10.4

[PATCH 2/4] udevng: add setup logic for Quectel UC15 modules

[Philip Paeps]

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

---
 plugins/udevng.c |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index d41c6a8..de37a40 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -791,6 +791,41 @@ static gboolean setup_samsung(struct modem_info *modem)
 	return TRUE;
 }
 
+static gboolean setup_quectel(struct modem_info *modem)
+{
+	const char *aux = NULL, *mdm = NULL;
+	GSList *list;
+
+	DBG("%s", modem->syspath);
+
+	for (list = modem->devices; list; list = list->next) {
+		struct device_info *info = list->data;
+
+		DBG("%s %s %s %s", info->devnode, info->interface,
+						info->number, info->label);
+
+		if (g_strcmp0(info->label, "aux") == 0) {
+			aux = info->devnode;
+			if (mdm != NULL)
+				break;
+		} else if (g_strcmp0(info->label, "modem") == 0) {
+			mdm = info->devnode;
+			if (aux != NULL)
+				break;
+		}
+	}
+
+	if (aux == NULL || mdm == NULL)
+		return FALSE;
+
+	DBG("aux=%s modem=%s", aux, mdm);
+
+	ofono_modem_set_string(modem->modem, "Aux", aux);
+	ofono_modem_set_string(modem->modem, "Modem", mdm);
+
+	return TRUE;
+}
+
 static struct {
 	const char *name;
 	gboolean (*setup)(struct modem_info *modem);
@@ -815,6 +850,7 @@ static struct {
 	{ "zte",	setup_zte	},
 	{ "icera",	setup_icera	},
 	{ "samsung",	setup_samsung	},
+	{ "quectel",	setup_quectel	},
 	{ }
 };
 
@@ -1026,6 +1062,7 @@ static struct {
 	{ "nokia",	"option",	"0421", "0623"	},
 	{ "samsung",	"option",	"04e8", "6889"	},
 	{ "samsung",	"kalmia"			},
+	{ "quectel",	"option",	"05c6", "9090"	},
 	{ }
 };
 
-- 
1.7.10.4

[PATCH 3/4] plugins: add a new driver for Quectel UC15 modules

[Philip Paeps]

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

---
 Makefile.am       |    3 +
 plugins/quectel.c |  335 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 338 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index cd83ef4..aee28de 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -420,6 +420,9 @@ builtin_sources += plugins/connman.c
 builtin_modules += he910
 builtin_sources += plugins/he910.c
 
+builtin_modules += quectel
+builtin_sources += plugins/quectel.c
+
 if BLUETOOTH
 if BLUEZ4
 builtin_modules += bluez4
diff --git a/plugins/quectel.c b/plugins/quectel.c
new file mode 100644
index 0000000..d665cbe
--- /dev/null
+++ b/plugins/quectel.c
@@ -0,0 +1,335 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <stdlib.h>
+
+#include <glib.h>
+#include <gatchat.h>
+#include <gattty.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+#include <ofono/plugin.h>
+#include <ofono/modem.h>
+#include <ofono/devinfo.h>
+#include <ofono/netreg.h>
+#include <ofono/sim.h>
+#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
+#include <ofono/log.h>
+
+#include <drivers/atmodem/atutil.h>
+#include <drivers/atmodem/vendor.h>
+
+struct quectel_data {
+	GAtChat *modem;
+	GAtChat *aux;
+	guint enable_timer;
+	guint enable_tries;
+};
+
+#define RESET_DELAY 3 /* seconds */
+#define MAX_RETRIES 5 /* number of attempts */
+
+static void quectel_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	ofono_info("%s%s", prefix, str);
+}
+
+static int quectel_probe(struct ofono_modem *modem)
+{
+	struct quectel_data *data;
+
+	DBG("%p", modem);
+
+	data = g_try_new0(struct quectel_data, 1);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ofono_modem_set_data(modem, data);
+
+	return 0;
+}
+
+static void quectel_remove(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	if (data->enable_timer)
+		g_source_remove(data->enable_timer);
+
+	ofono_modem_set_data(modem, NULL);
+	g_at_chat_unref(data->aux);
+	g_free(data);
+}
+
+static GAtChat *open_device(struct ofono_modem *modem,
+				const char *key, char *debug)
+{
+	const char *device;
+	GAtSyntax *syntax;
+	GIOChannel *channel;
+	GAtChat *chat;
+
+	device = ofono_modem_get_string(modem, key);
+	if (device == NULL)
+		return NULL;
+
+	DBG("%s %s", key, device);
+
+	channel = g_at_tty_open(device, NULL);
+	if (channel == NULL)
+		return NULL;
+
+	syntax = g_at_syntax_new_gsm_permissive();
+	chat = g_at_chat_new(channel, syntax);
+	g_at_syntax_unref(syntax);
+
+	g_io_channel_unref(channel);
+
+	if (chat == NULL)
+		return NULL;
+
+	if (getenv("OFONO_AT_DEBUG"))
+		g_at_chat_set_debug(chat, quectel_debug, debug);
+
+	return chat;
+}
+
+static void verify_enable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	if (ok) {
+		ofono_modem_set_powered(modem, TRUE);
+		g_at_chat_send(data->modem, "AT&C0", NULL, NULL, NULL, NULL);
+		g_at_chat_send(data->aux, "AT&C0", NULL, NULL, NULL, NULL);
+	}
+}
+
+static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("ok %d (tried %d times)", (int)ok, data->enable_tries);
+
+	if (!ok) {
+		if (data->enable_tries > MAX_RETRIES) {
+			g_at_chat_unref(data->aux);
+			data->aux = NULL;
+			g_at_chat_unref(data->modem);
+			data->modem = NULL;
+			ofono_modem_set_powered(modem, FALSE);
+			DBG("modem won't enable - giving up");
+		}
+		goto retry;
+	}
+
+	g_at_chat_send(data->aux, "AT+CPIN?", NULL, verify_enable,
+					modem, NULL);
+retry:
+	data->enable_tries++;
+}
+
+static gboolean quectel_start(gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	if (ofono_modem_get_powered(modem) == FALSE) {
+		g_at_chat_send(data->aux, "AT+CFUN=1", NULL, cfun_enable,
+					modem, NULL);
+		return TRUE;
+	}
+
+	data->enable_timer = 0;
+
+	return FALSE;
+}
+
+static int quectel_enable(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	data->modem = open_device(modem, "Modem", "Modem: ");
+	if (data->modem == NULL)
+		return -EINVAL;
+
+	data->aux = open_device(modem, "Aux", "Aux: ");
+	if (data->aux == NULL) {
+		g_at_chat_unref(data->modem);
+		data->modem = NULL;
+		return -EIO;
+	}
+	g_at_chat_set_slave(data->modem, data->aux);
+
+	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(data->aux, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
+
+	/* Give the modem a bit more time to power up completely. */
+	data->enable_tries = 0;
+	data->enable_timer = g_timeout_add_seconds(RESET_DELAY,
+					quectel_start, modem);
+
+	return -EINPROGRESS;
+}
+
+static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	g_at_chat_unref(data->aux);
+	data->aux = NULL;
+
+	if (ok)
+		ofono_modem_set_powered(modem, FALSE);
+}
+
+static int quectel_disable(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	/* Prevent a race with cfun_start() retries. */
+	if (data->enable_timer)
+		g_source_remove(data->enable_timer);
+
+	g_at_chat_cancel_all(data->modem);
+	g_at_chat_unregister_all(data->modem);
+
+	g_at_chat_unref(data->modem);
+	data->modem = NULL;
+
+	g_at_chat_cancel_all(data->aux);
+	g_at_chat_unregister_all(data->aux);
+
+	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
+					cfun_disable, modem, NULL);
+
+	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 quectel_set_online(struct ofono_modem *modem, ofono_bool_t online,
+				ofono_modem_online_cb_t cb, void *user_data)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	struct cb_data *cbd = cb_data_new(cb, user_data);
+	char const *command = online ? "AT+CFUN=1" : "AT+CFUN=4";
+
+	DBG("modem %p %s", modem, online ? "online" : "offline");
+
+	if (g_at_chat_send(data->aux, command, NULL, set_online_cb,
+					cbd, g_free) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+	g_free(cbd);
+}
+
+static void quectel_pre_sim(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim;
+
+	DBG("%p", modem);
+
+	ofono_devinfo_create(modem, 0, "atmodem", data->aux);
+	sim = ofono_sim_create(modem, OFONO_VENDOR_QUECTEL, "atmodem",
+					data->aux);
+	if (sim)
+		ofono_sim_inserted_notify(sim, TRUE);
+}
+
+static void quectel_post_sim(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+	struct ofono_gprs *gprs;
+	struct ofono_gprs_context *gc;
+
+	DBG("%p", modem);
+
+	gprs = ofono_gprs_create(modem, OFONO_VENDOR_QUECTEL, "atmodem",
+					data->aux);
+	gc = ofono_gprs_context_create(modem, 0, "atmodem", data->modem);
+
+	if (gprs && gc)
+		ofono_gprs_add_context(gprs, gc);
+}
+
+static void quectel_post_online(struct ofono_modem *modem)
+{
+	struct quectel_data *data = ofono_modem_get_data(modem);
+
+	ofono_netreg_create(modem, OFONO_VENDOR_QUECTEL, "atmodem",
+					data->aux);
+}
+
+static struct ofono_modem_driver quectel_driver = {
+	.name			= "quectel",
+	.probe			= quectel_probe,
+	.remove			= quectel_remove,
+	.enable			= quectel_enable,
+	.disable		= quectel_disable,
+	.set_online		= quectel_set_online,
+	.pre_sim		= quectel_pre_sim,
+	.post_sim		= quectel_post_sim,
+	.post_online		= quectel_post_online,
+};
+
+static int quectel_init(void)
+{
+	return ofono_modem_driver_register(&quectel_driver);
+}
+
+static void quectel_exit(void)
+{
+	ofono_modem_driver_unregister(&quectel_driver);
+}
+
+OFONO_PLUGIN_DEFINE(quectel, "Quectel driver", VERSION,
+    OFONO_PLUGIN_PRIORITY_DEFAULT, quectel_init, quectel_exit)
-- 
1.7.10.4

[PATCH 4/4] sim: query Quectel UC15 PIN retries with AT+QPINQ

[Philip Paeps]

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

---
 drivers/atmodem/sim.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index f8e0472..4a91798 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -67,6 +67,7 @@ static const char *epin_prefix[] = { "*EPIN:", NULL };
 static const char *spic_prefix[] = { "+SPIC:", NULL };
 static const char *pct_prefix[] = { "#PCT:", NULL };
 static const char *pnnm_prefix[] = { "+PNNM:", NULL };
+static const char *qpinc_prefix[] = { "+QPINC:", NULL };
 static const char *none_prefix[] = { NULL };
 
 static void at_crsm_info_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -967,6 +968,49 @@ error:
 	CALLBACK_WITH_FAILURE(cb, NULL, cbd->data);
 }
 
+static void at_qpinc_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_sim_pin_retries_cb_t cb = cbd->cb;
+	const char *final = g_at_result_final_response(result);
+	GAtResultIter iter;
+	struct ofono_error error;
+	int retries[OFONO_SIM_PASSWORD_INVALID];
+	size_t i;
+
+	decode_at_error(&error, final);
+
+	if (!ok) {
+		cb(&error, NULL, cbd->data);
+		return;
+	}
+
+	for (i = 0; i < OFONO_SIM_PASSWORD_INVALID; i++)
+		retries[i] = -1;
+
+	g_at_result_iter_init(&iter, result);
+	while (g_at_result_iter_next(&iter, "+QPINC:")) {
+		const char *name;
+		int pin, puk;
+
+		if (!g_at_result_iter_next_string(&iter, &name))
+			continue;
+		if (!g_at_result_iter_next_number(&iter, &pin))
+			continue;
+		if (!g_at_result_iter_next_number(&iter, &puk))
+			continue;
+
+		if (!strcmp(name, "SC")) {
+			retries[OFONO_SIM_PASSWORD_SIM_PIN] = pin;
+			retries[OFONO_SIM_PASSWORD_SIM_PUK] = puk;
+		} else if (!strcmp(name, "P2")) {
+			retries[OFONO_SIM_PASSWORD_SIM_PIN2] = pin;
+			retries[OFONO_SIM_PASSWORD_SIM_PUK2] = puk;
+		}
+	}
+	cb(&error, retries, cbd->data);
+}
+
 static void at_pin_retries_query(struct ofono_sim *sim,
 					ofono_sim_pin_retries_cb_t cb,
 					void *data)
@@ -1028,6 +1072,11 @@ static void at_pin_retries_query(struct ofono_sim *sim,
 					at_pnnm_cb, cbd, g_free) > 0)
 			return;
 		break;
+	case OFONO_VENDOR_QUECTEL:
+		if (g_at_chat_send(sd->chat, "AT+QPINC?", qpinc_prefix,
+					at_qpinc_cb, cbd, g_free) > 0)
+			return;
+		break;
 	default:
 		if (g_at_chat_send(sd->chat, "AT+CPINR", cpinr_prefixes,
 					at_cpinr_cb, cbd, g_free) > 0)
-- 
1.7.10.4

Re: [PATCH 3/4] plugins: add a new driver for Quectel UC15 modules

[Philip Paeps]

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

On 2014-06-24 12:08:41 (+0200), Philip Paeps <philip@paeps.cx> wrote:
> + *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.

So focused on trying to make sure I didn't violate any local coding
style rules, I forgot I copy/pasted the copyright header!  This should
read "2014  Philip Paeps".

Would you like me to submit an updated patchset?

Philip

-- 
Philip Paeps
Senior Reality Engineer
Ministry of Information

Re: [PATCH 3/4] plugins: add a new driver for Quectel UC15 modules

[Denis Kenzior]

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

Hi Philip,

<snip>

> +
> +static void verify_enable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	if (ok) {
> +		ofono_modem_set_powered(modem, TRUE);
> +		g_at_chat_send(data->modem, "AT&C0", NULL, NULL, NULL, NULL);
> +		g_at_chat_send(data->aux, "AT&C0", NULL, NULL, NULL, NULL);

Why do you do send AT&C0 here?  Can it be done earlier?

Any reason for checking that CFUN succeeded?  If so, then it might be a
good idea to document that in comments / commit description.

What if it failed?  Are you relying on the core timeout mechanisms?

> +	}
> +}
> +
> +static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("ok %d (tried %d times)", (int)ok, data->enable_tries);

This cast seems unnecessary

> +
> +	if (!ok) {
> +		if (data->enable_tries > MAX_RETRIES) {
> +			g_at_chat_unref(data->aux);
> +			data->aux = NULL;
> +			g_at_chat_unref(data->modem);
> +			data->modem = NULL;
> +			ofono_modem_set_powered(modem, FALSE);
> +			DBG("modem won't enable - giving up");
> +		}
> +		goto retry;

Is the timer still running in case MAX_RETRIES is reached?  In general
we prefer using single-shot timers and re-starting them if the command
fails.  Certain commands might take several seconds to return on some
hardware.

> +	}
> +
> +	g_at_chat_send(data->aux, "AT+CPIN?", NULL, verify_enable,
> +					modem, NULL);

Using prefixes is a good idea, otherwise any unsolicited notifications
will be missed.  git show 35feae07e50f89ea3c42566c765f501ec768bd44 for
an example of what might happen.

> +retry:
> +	data->enable_tries++;
> +}
> +
> +static gboolean quectel_start(gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	if (ofono_modem_get_powered(modem) == FALSE) {
> +		g_at_chat_send(data->aux, "AT+CFUN=1", NULL, cfun_enable,
> +					modem, NULL);
> +		return TRUE;
> +	}
> +
> +	data->enable_timer = 0;
> +
> +	return FALSE;
> +}
> +
> +static int quectel_enable(struct ofono_modem *modem)
> +{
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	data->modem = open_device(modem, "Modem", "Modem: ");
> +	if (data->modem == NULL)
> +		return -EINVAL;
> +
> +	data->aux = open_device(modem, "Aux", "Aux: ");
> +	if (data->aux == NULL) {
> +		g_at_chat_unref(data->modem);
> +		data->modem = NULL;
> +		return -EIO;
> +	}
> +	g_at_chat_set_slave(data->modem, data->aux);
> +
> +	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
> +	g_at_chat_send(data->aux, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
> +
> +	/* Give the modem a bit more time to power up completely. */
> +	data->enable_tries = 0;
> +	data->enable_timer = g_timeout_add_seconds(RESET_DELAY,
> +					quectel_start, modem);

Is this delay always necessary?  enable() is called in other situations
besides a hot-plugging...

> +
> +	return -EINPROGRESS;
> +}
> +
> +static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	g_at_chat_unref(data->aux);
> +	data->aux = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static int quectel_disable(struct ofono_modem *modem)
> +{
> +	struct quectel_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	/* Prevent a race with cfun_start() retries. */
> +	if (data->enable_timer)
> +		g_source_remove(data->enable_timer);
> +
> +	g_at_chat_cancel_all(data->modem);
> +	g_at_chat_unregister_all(data->modem);
> +
> +	g_at_chat_unref(data->modem);
> +	data->modem = NULL;
> +
> +	g_at_chat_cancel_all(data->aux);
> +	g_at_chat_unregister_all(data->aux);
> +
> +	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
> +					cfun_disable, modem, NULL);

What behavior does this modem exhibit when using CFUN=0?  Many devices
jump off the USB bus when this is sent.  Hence why most drivers use CFUN=4.

> +
> +	return -EINPROGRESS;
> +}
> +

Regards,
-Denis

Re: [PATCH 3/4] plugins: add a new driver for Quectel UC15 modules

[Philip Paeps]

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

On 2014-06-24 12:41:45 (-0500), Denis Kenzior <denkenz@gmail.com> wrote:
> Philip Paeps <philip@paeps.cx> wrote:

Thanks for the review, Denis!

> > +static void verify_enable(gboolean ok, GAtResult *result, gpointer user_data)
> > +{
> > +	struct ofono_modem *modem = user_data;
> > +	struct quectel_data *data = ofono_modem_get_data(modem);
> > +
> > +	if (ok) {
> > +		ofono_modem_set_powered(modem, TRUE);
> > +		g_at_chat_send(data->modem, "AT&C0", NULL, NULL, NULL, NULL);
> > +		g_at_chat_send(data->aux, "AT&C0", NULL, NULL, NULL, NULL);
> 
> Why do you do send AT&C0 here?  Can it be done earlier?

The module can be connected to USB or a regular UART.  In the case of a
regular UART, AT&C0 will keep DCD on regardless of whether the modem is
connected.

I don't think there's any reason this needs to be there specifically.  I
probably put it there when I was debugging a USB-related reset problem.
It can probably either go away (I've not done more than cursory testing
with the modem connected to a UART - many things may be broken) or move
to quectel_enable() (in which case the modem will behave a little bit
more like one expects a modem to behave when connected to a UART).

> Any reason for checking that CFUN succeeded?  If so, then it might be a
> good idea to document that in comments / commit description.

It turns out that AT+CFUN=0 -> AT+CFUN=1 transitions can return OK
before the SIM is completely out of reset (temperature related).  In
that case, AT+CPIN will return any number of bizarre error codes (from
SIM not inserted to memory failure).  Trying again a bit later just
works.

> What if it failed?  Are you relying on the core timeout mechanisms?

It it doesn't behave as expected after retrying as often and waiting as
long as this, it's unlikely to ever work.  The core will timeout waiting
for the driver to indicate the modem is powered.  I would expect higher
layers to see this as a hint that something may be wrong with powering
up the modem.

Can you suggest a better way to fail here?

> > +	if (!ok) {
> > +		if (data->enable_tries > MAX_RETRIES) {
> > +			g_at_chat_unref(data->aux);
> > +			data->aux = NULL;
> > +			g_at_chat_unref(data->modem);
> > +			data->modem = NULL;
> > +			ofono_modem_set_powered(modem, FALSE);
> > +			DBG("modem won't enable - giving up");
> > +		}
> > +		goto retry;
> 
> Is the timer still running in case MAX_RETRIES is reached?  In general
> we prefer using single-shot timers and re-starting them if the command
> fails.  Certain commands might take several seconds to return on some
> hardware.

I am fairly confident it's safe: I ran into this failure case quite
often during development and testing.  I schedule quectel_start() in
quectel_enable() and quectel_start() keeps rescheduling cfun_enable()
until it succeeds or runs out of retries.  I carefully check if the
timer is still running everywhere I try to tear things down.

It's probably not too hard to rework this to use a single-shot timer.

> > +	g_at_chat_send(data->aux, "AT+CPIN?", NULL, verify_enable,
> > +					modem, NULL);
> 
> Using prefixes is a good idea, otherwise any unsolicited notifications
> will be missed.  git show 35feae07e50f89ea3c42566c765f501ec768bd44 for
> an example of what might happen.

Thanks for the pointer.  I'll take a look and rework.

> > +	/* Give the modem a bit more time to power up completely. */
> > +	data->enable_tries = 0;
> > +	data->enable_timer = g_timeout_add_seconds(RESET_DELAY,
> > +					quectel_start, modem);
> 
> Is this delay always necessary?  enable() is called in other situations
> besides a hot-plugging...

I mainly tested this with hot-plugging and not-so-hot-plugging (keeping
the module powered but kicking its reset line) and it seems necessary in
both cases.  I haven't tested toggling enable in software much, but just
doing AT+CFUN=0 ; AT+CFUN=1 in a loop shows a fair amount of variance
even without touching power or reset.  May be a hardware issue, but I've
not talked to Quectel about it.
 
> > +	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
> > +					cfun_disable, modem, NULL);
> 
> What behavior does this modem exhibit when using CFUN=0?  Many devices
> jump off the USB bus when this is sent.  Hence why most drivers use CFUN=4.

It stays on the bus but turns off the radio and goes into lower power
mode.  I have no strong feelings about using CFUN=4 instead, but
intuitively that feels more like "offline" than "disabled".  Am I
missing some other distinction between set_online and enable/disable?

Philip

-- 
Philip Paeps
Senior Reality Engineer
Ministry of Information

Re: [PATCH 3/4] plugins: add a new driver for Quectel UC15 modules

[Denis Kenzior]

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

Hi Philip,

>
> I don't think there's any reason this needs to be there specifically.  I
> probably put it there when I was debugging a USB-related reset problem.
> It can probably either go away (I've not done more than cursory testing
> with the modem connected to a UART - many things may be broken) or move
> to quectel_enable() (in which case the modem will behave a little bit
> more like one expects a modem to behave when connected to a UART).

Just wondering.  We usually send &C0 fairly early in the process.

> 
>> Any reason for checking that CFUN succeeded?  If so, then it might be a
>> good idea to document that in comments / commit description.
> 
> It turns out that AT+CFUN=0 -> AT+CFUN=1 transitions can return OK
> before the SIM is completely out of reset (temperature related).  In
> that case, AT+CPIN will return any number of bizarre error codes (from
> SIM not inserted to memory failure).  Trying again a bit later just
> works.

Should you be using at_util_sim_state_query_new for this?  Or does
CFUN=1 needs to be sent again?

> 
>> What if it failed?  Are you relying on the core timeout mechanisms?
> 
> It it doesn't behave as expected after retrying as often and waiting as
> long as this, it's unlikely to ever work.  The core will timeout waiting
> for the driver to indicate the modem is powered.  I would expect higher
> layers to see this as a hint that something may be wrong with powering
> up the modem.
> 
> Can you suggest a better way to fail here?

I was thinking that the driver should notify the core as soon as it
determines that the hardware is inoperable.  The core timeout handling
is really just a failsafe.  But either way works...

> 
>>> +	if (!ok) {
>>> +		if (data->enable_tries > MAX_RETRIES) {
>>> +			g_at_chat_unref(data->aux);
>>> +			data->aux = NULL;
>>> +			g_at_chat_unref(data->modem);
>>> +			data->modem = NULL;
>>> +			ofono_modem_set_powered(modem, FALSE);
>>> +			DBG("modem won't enable - giving up");
>>> +		}
>>> +		goto retry;
>>
>> Is the timer still running in case MAX_RETRIES is reached?  In general
>> we prefer using single-shot timers and re-starting them if the command
>> fails.  Certain commands might take several seconds to return on some
>> hardware.
> 
> I am fairly confident it's safe: I ran into this failure case quite
> often during development and testing.  I schedule quectel_start() in
> quectel_enable() and quectel_start() keeps rescheduling cfun_enable()
> until it succeeds or runs out of retries.  I carefully check if the
> timer is still running everywhere I try to tear things down.

The thing is that running out of retries does not stop the timer.  At
least I don't see g_source_remove for it here.  Neither does the
quectel_start() function check for max retries.

> 
> It's probably not too hard to rework this to use a single-shot timer.
> 

That might make things a bit more reliable and readable as well.

>>> +	/* Give the modem a bit more time to power up completely. */
>>> +	data->enable_tries = 0;
>>> +	data->enable_timer = g_timeout_add_seconds(RESET_DELAY,
>>> +					quectel_start, modem);
>>
>> Is this delay always necessary?  enable() is called in other situations
>> besides a hot-plugging...
> 
> I mainly tested this with hot-plugging and not-so-hot-plugging (keeping
> the module powered but kicking its reset line) and it seems necessary in
> both cases.  I haven't tested toggling enable in software much, but just
> doing AT+CFUN=0 ; AT+CFUN=1 in a loop shows a fair amount of variance
> even without touching power or reset.  May be a hardware issue, but I've
> not talked to Quectel about it.
>  

Just wondering whether upping the max retries and sending this command
right away might be 'better' for the non-hotplug case.

>>> +	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
>>> +					cfun_disable, modem, NULL);
>>
>> What behavior does this modem exhibit when using CFUN=0?  Many devices
>> jump off the USB bus when this is sent.  Hence why most drivers use CFUN=4.
> 
> It stays on the bus but turns off the radio and goes into lower power
> mode.  I have no strong feelings about using CFUN=4 instead, but
> intuitively that feels more like "offline" than "disabled".  Am I
> missing some other distinction between set_online and enable/disable?

If the modem stays on the bus, using CFUN=0 is just fine.  As I said,
most others jump off the bus, which makes using CFUN=0 not a good idea :)

Regards,
-Denis