[PATCH 0/8] [RFC] Support for serial u-blox NB-IoT modems

[PATCH 0/8] [RFC] Support for serial u-blox NB-IoT modems

[philippedeswert]

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

From: Philippe De Swert <philippe.deswert@digitalistgroup.com>

Hi,

Lately we have been trying to get a u-blox SARA-R410 and R412 to work with ofono.
We inspired ourselves on the existing u-blox driver and ran into some weird
issues (mostly issues with roaming sims). So it would be nice to get some feedback 
on what we might have done wrong. We did have issues with ATD99
and would be very interested in knowing what we missed or did wrong there as we
needed to force it. We also found a small bug related to ppp and disconnects.

Thanks!


Philippe De Swert (7):
  ublox-serial: add new plugin for u-blox
  Makefile.am : include new u-blox serial modem driver
  gprs-context : Force use of atd99
  common: Add new NB-IoT technologies

Tommi Rekosuo (1):
  gprs.h: Add ofono_gprs_get_netreg function
  gprs.c: Add ofono_gprs_get_netreg function
  atmodem/gprs.c: Add CEREG support
  Fix gprs_netreg_update behaviour if PPP connection active

 Makefile.am                    |   4 +
 drivers/atmodem/gprs-context.c |   5 +-
 drivers/atmodem/gprs.c         |  60 ++++-
 include/gprs.h                 |   1 +
 plugins/ublox-serial.c         | 393 +++++++++++++++++++++++++++++++++
 plugins/udevng.c               |   1 +
 src/common.c                   |   4 +
 src/common.h                   |   2 +
 src/gprs.c                     |  29 ++-
 9 files changed, 493 insertions(+), 6 deletions(-)
 create mode 100644 plugins/ublox-serial.c

-- 
2.20.1

[PATCH 1/8] ublox-serial: add new plugin for u-blox

[philippedeswert]

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

From: Philippe De Swert <philippe.deswert@digitalistgroup.com>

Add support for ublox modems that are used over the serial port.
(tested on SARA-R410M and R412M)
---
 plugins/ublox-serial.c | 393 +++++++++++++++++++++++++++++++++++++++++
 plugins/udevng.c       |   1 +
 2 files changed, 394 insertions(+)
 create mode 100644 plugins/ublox-serial.c

diff --git a/plugins/ublox-serial.c b/plugins/ublox-serial.c
new file mode 100644
index 00000000..4f0cf5f3
--- /dev/null
+++ b/plugins/ublox-serial.c
@@ -0,0 +1,393 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2014  Philip Paeps. All rights reserved.
+ *  Copyright (C) 2018-2019 Treon oy. All rights reserved.
+ *
+ *  Inspired on the Ublox driver by Philip Paeps
+ *
+ *  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 <string.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/netmon.h>
+#include <ofono/lte.h>
+
+#include <drivers/atmodem/atutil.h>
+#include <drivers/atmodem/vendor.h>
+
+static const char *none_prefix[] = { NULL };
+
+struct ublox_data {
+	GIOChannel *device;
+	GAtChat *modem;
+	GAtChat *dlc;
+	int model_id;
+	enum ofono_vendor vendor_family;
+	guint frame_size;
+};
+
+static void ublox_debug(const char *str, void *user_data)
+{
+	const char *prefix = user_data;
+
+	ofono_info("%s%s", prefix, str);
+}
+
+static int ublox_probe(struct ofono_modem *modem)
+{
+	struct ublox_data *data;
+
+	DBG("%p", modem);
+
+	data = g_try_new0(struct ublox_data, 1);
+	if (data == NULL)
+		return -ENOMEM;
+
+	ofono_modem_set_data(modem, data);
+
+	return 0;
+}
+
+static void ublox_remove(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	ofono_modem_set_data(modem, NULL);
+	g_at_chat_unref(data->modem);
+	g_free(data);
+}
+
+static GAtChat *open_device(struct ofono_modem *modem,
+				const char *key, char *debug)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+	const char *device;
+	GAtSyntax *syntax;
+	GIOChannel *channel;
+	GAtChat *chat;
+	GHashTable *options;
+
+	device = ofono_modem_get_string(modem, key);
+	if (device == NULL)
+	{
+		DBG("device = NULL");
+		return NULL;
+	}
+	DBG("%s %s", key, device);
+
+        options = g_hash_table_new(g_str_hash, g_str_equal);
+        if (options == NULL)
+		return NULL;
+
+        g_hash_table_insert(options, "Baud", "115200");
+        g_hash_table_insert(options, "Parity", "none");
+        g_hash_table_insert(options, "StopBits", "1");
+        g_hash_table_insert(options, "DataBits", "8");
+        g_hash_table_insert(options, "XonXoff", "on");
+        g_hash_table_insert(options, "Local", "on");
+        g_hash_table_insert(options, "RtsCts", "off");
+        g_hash_table_insert(options, "Read", "on");
+
+	channel = g_at_tty_open(device, options);
+        g_hash_table_destroy(options);
+
+	if (channel == NULL)
+		return NULL;
+
+	data->device = channel;
+	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, ublox_debug, debug);
+
+	return chat;
+}
+
+#if 0
+static GAtChat *create_chat(GIOChannel *channel, struct ofono_modem *modem,
+                                char *debug)
+{
+        GAtSyntax *syntax;
+        GAtChat *chat;
+
+        if (channel == NULL)
+                return NULL;
+
+        syntax = g_at_syntax_new_gsmv1();
+        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, ublox_debug, debug);
+
+        return chat;
+}
+
+static void shutdown_device(struct ublox_data *data)
+{
+        DBG("");
+
+        g_at_chat_unref(data->dlc);
+        data->dlc = NULL;
+
+        g_io_channel_unref(data->device);
+        data->device = NULL;
+}
+#endif
+
+static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct ublox_data * data = ofono_modem_get_data(modem);
+
+	DBG("ok %d", ok);
+
+	if (!ok) {
+		g_at_chat_unref(data->modem);
+		data->modem = NULL;
+		ofono_modem_set_powered(modem, FALSE);
+		DBG("CFUN failed");
+		return;
+	}
+        ofono_modem_set_powered(modem, TRUE);
+	DBG("Powered on!");
+}
+
+static int ublox_enable(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+	const char *model_str = NULL;
+
+	DBG("%p", modem);
+
+	model_str = ofono_modem_get_string(modem, "Model");
+	DBG("%p is model %s", modem, model_str);
+	if (model_str == NULL)
+		model_str = strdup("SARA_R410");
+		//return -EINVAL;
+
+	data->model_id = atoi(model_str);
+
+	switch (data->model_id) {
+	default:
+		DBG("unknown ublox model id %d", data->model_id);
+		//return -EINVAL;
+		data->vendor_family = OFONO_VENDOR_UBLOX;
+		break;
+	}
+
+	if (data->vendor_family == OFONO_VENDOR_UBLOX) {
+		data->dlc = open_device(modem, "Device", "Setup: ");
+		if (data->dlc == NULL) {
+			g_at_chat_unref(data->dlc);
+			data->dlc = NULL;
+			DBG("open_device failed!");
+			return -EIO;
+		}
+
+		g_at_chat_set_slave(data->modem, data->dlc);
+
+		g_at_chat_send(data->dlc, "AT+CFUN=0", none_prefix,
+						NULL, NULL, NULL);
+
+		g_at_chat_send(data->dlc, "AT+UMNOPROF=1", NULL, NULL, NULL, NULL);
+		//enable roaming in the modem
+		g_at_chat_send(data->dlc, "AT+UDCONF=76,1,0", NULL, NULL, NULL, NULL);
+	}
+
+	/* The modem can take a while to wake up if just powered on. */
+	//g_at_chat_set_wakeup_command(data->dlcs[SETUP_DLC], "AT\r", 1000, 11000);
+
+	g_at_chat_send(data->dlc, "ATE0 +CMEE=1", none_prefix,
+					NULL, NULL, NULL);
+
+	g_at_chat_send(data->dlc, "AT+CFUN=4", none_prefix,
+					cfun_enable, modem, NULL);
+
+	DBG("Return in progress");
+	return -EINPROGRESS;
+}
+
+static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_modem *modem = user_data;
+	struct ublox_data *data = ofono_modem_get_data(modem);
+
+	DBG("");
+
+	g_at_chat_unref(data->dlc);
+	data->dlc = NULL;
+
+	if (ok)
+		ofono_modem_set_powered(modem, FALSE);
+}
+
+static int ublox_disable(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+
+	DBG("%p", modem);
+
+	g_at_chat_cancel_all(data->dlc);
+	g_at_chat_unregister_all(data->dlc);
+
+	g_at_chat_send(data->dlc, "AT+CFUN=4", none_prefix,
+					cfun_disable, modem, NULL);
+
+	g_at_chat_unref(data->modem);
+	data->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);
+	DBG("online!?");
+}
+
+static void ublox_set_online(struct ofono_modem *modem, ofono_bool_t online,
+				ofono_modem_online_cb_t cb, void *user_data)
+{
+	struct ublox_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->dlc, command, none_prefix, set_online_cb,
+					cbd, g_free) > 0)
+		return;
+
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+	g_free(cbd);
+}
+
+static void ublox_pre_sim(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+	struct ofono_sim *sim;
+
+	DBG("%p", modem);
+
+	ofono_devinfo_create(modem, 0, "atmodem", data->dlc);
+	DBG("%p devinfo created", modem);
+	sim = ofono_sim_create(modem, data->vendor_family, "atmodem",
+					data->dlc);
+
+	DBG("%p sim created", modem);
+
+	if (sim)
+	{
+		DBG("%p sim notification", modem);
+		ofono_sim_inserted_notify(sim, TRUE);
+	}
+}
+
+static void ublox_post_sim(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+	struct ofono_gprs *gprs;
+	struct ofono_gprs_context *gc;
+	GAtChat *chat = data->dlc;
+	const char *driver = "atmodem";
+
+	DBG("%p", modem);
+
+	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
+					data->dlc);
+
+	gc = ofono_gprs_context_create(modem, data->vendor_family,
+						driver, chat);
+
+	if (gprs && gc)
+		ofono_gprs_add_context(gprs, gc);
+
+	ofono_lte_create(modem, 0, "atmodem", data->dlc);
+}
+
+static void ublox_post_online(struct ofono_modem *modem)
+{
+	struct ublox_data *data = ofono_modem_get_data(modem);
+
+	ofono_netreg_create(modem, data->vendor_family, "atmodem", data->dlc);
+
+	ofono_netmon_create(modem, data->vendor_family, "ubloxserial", data->modem);
+}
+
+static struct ofono_modem_driver ublox_driver = {
+	.name		= "ubloxserial",
+	.probe		= ublox_probe,
+	.remove		= ublox_remove,
+	.enable		= ublox_enable,
+	.disable	= ublox_disable,
+	.set_online	= ublox_set_online,
+	.pre_sim	= ublox_pre_sim,
+	.post_sim	= ublox_post_sim,
+	.post_online	= ublox_post_online,
+};
+
+static int ublox_init(void)
+{
+	return ofono_modem_driver_register(&ublox_driver);
+}
+
+static void ublox_exit(void)
+{
+	ofono_modem_driver_unregister(&ublox_driver);
+}
+
+OFONO_PLUGIN_DEFINE(ubloxserial, "u-blox serial modem driver", VERSION,
+		OFONO_PLUGIN_PRIORITY_DEFAULT, ublox_init, ublox_exit)
diff --git a/plugins/udevng.c b/plugins/udevng.c
index ce8cdee1..d95fc527 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1322,6 +1322,7 @@ static struct {
 	{ "cinterion",	setup_serial_modem	},
 	{ "nokiacdma",	setup_serial_modem	},
 	{ "sim900",	setup_serial_modem	},
+	{ "ubloxserial",setup_serial_modem	},
 	{ "wavecom",	setup_wavecom		},
 	{ "tc65",	setup_tc65		},
 	{ "ehs6",	setup_ehs6		},
-- 
2.20.1

Re: [PATCH 1/8] ublox-serial: add new plugin for u-blox

[Jonas Bonn]

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

Hi,

My spontaneous reaction to this is:  why does this need to be a separate 
plugin?  At first glance, it appears to be largely a copy of the ublox 
plugin and the seemingly small differences would have been easy to 
finesse into the existing driver.

Could you provide some more justification for requiring a completely new 
plugin here?

Thanks,
Jonas

(Just for info, I'm working on support for the TOBY L4 currently... it 
would be preferable to get as much u-blox support into one place as 
possible rather than spreading it out over multiple plugins/drivers. 
All these modems share common documentation, after all.)

On 15/02/2019 13:11, philippedeswert(a)gmail.com wrote:
> From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> 
> Add support for ublox modems that are used over the serial port.
> (tested on SARA-R410M and R412M)
> ---
>   plugins/ublox-serial.c | 393 +++++++++++++++++++++++++++++++++++++++++
>   plugins/udevng.c       |   1 +
>   2 files changed, 394 insertions(+)
>   create mode 100644 plugins/ublox-serial.c
> 
> diff --git a/plugins/ublox-serial.c b/plugins/ublox-serial.c
> new file mode 100644
> index 00000000..4f0cf5f3
> --- /dev/null
> +++ b/plugins/ublox-serial.c
> @@ -0,0 +1,393 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2014  Philip Paeps. All rights reserved.
> + *  Copyright (C) 2018-2019 Treon oy. All rights reserved.
> + *
> + *  Inspired on the Ublox driver by Philip Paeps
> + *
> + *  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 <string.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/netmon.h>
> +#include <ofono/lte.h>
> +
> +#include <drivers/atmodem/atutil.h>
> +#include <drivers/atmodem/vendor.h>
> +
> +static const char *none_prefix[] = { NULL };
> +
> +struct ublox_data {
> +	GIOChannel *device;
> +	GAtChat *modem;
> +	GAtChat *dlc;
> +	int model_id;
> +	enum ofono_vendor vendor_family;
> +	guint frame_size;
> +};
> +
> +static void ublox_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	ofono_info("%s%s", prefix, str);
> +}
> +
> +static int ublox_probe(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data;
> +
> +	DBG("%p", modem);
> +
> +	data = g_try_new0(struct ublox_data, 1);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ofono_modem_set_data(modem, data);
> +
> +	return 0;
> +}
> +
> +static void ublox_remove(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_unref(data->modem);
> +	g_free(data);
> +}
> +
> +static GAtChat *open_device(struct ofono_modem *modem,
> +				const char *key, char *debug)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	const char *device;
> +	GAtSyntax *syntax;
> +	GIOChannel *channel;
> +	GAtChat *chat;
> +	GHashTable *options;
> +
> +	device = ofono_modem_get_string(modem, key);
> +	if (device == NULL)
> +	{
> +		DBG("device = NULL");
> +		return NULL;
> +	}
> +	DBG("%s %s", key, device);
> +
> +        options = g_hash_table_new(g_str_hash, g_str_equal);
> +        if (options == NULL)
> +		return NULL;
> +
> +        g_hash_table_insert(options, "Baud", "115200");
> +        g_hash_table_insert(options, "Parity", "none");
> +        g_hash_table_insert(options, "StopBits", "1");
> +        g_hash_table_insert(options, "DataBits", "8");
> +        g_hash_table_insert(options, "XonXoff", "on");
> +        g_hash_table_insert(options, "Local", "on");
> +        g_hash_table_insert(options, "RtsCts", "off");
> +        g_hash_table_insert(options, "Read", "on");
> +
> +	channel = g_at_tty_open(device, options);
> +        g_hash_table_destroy(options);
> +
> +	if (channel == NULL)
> +		return NULL;
> +
> +	data->device = channel;
> +	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, ublox_debug, debug);
> +
> +	return chat;
> +}
> +
> +#if 0
> +static GAtChat *create_chat(GIOChannel *channel, struct ofono_modem *modem,
> +                                char *debug)
> +{
> +        GAtSyntax *syntax;
> +        GAtChat *chat;
> +
> +        if (channel == NULL)
> +                return NULL;
> +
> +        syntax = g_at_syntax_new_gsmv1();
> +        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, ublox_debug, debug);
> +
> +        return chat;
> +}
> +
> +static void shutdown_device(struct ublox_data *data)
> +{
> +        DBG("");
> +
> +        g_at_chat_unref(data->dlc);
> +        data->dlc = NULL;
> +
> +        g_io_channel_unref(data->device);
> +        data->device = NULL;
> +}
> +#endif
> +
> +static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ublox_data * data = ofono_modem_get_data(modem);
> +
> +	DBG("ok %d", ok);
> +
> +	if (!ok) {
> +		g_at_chat_unref(data->modem);
> +		data->modem = NULL;
> +		ofono_modem_set_powered(modem, FALSE);
> +		DBG("CFUN failed");
> +		return;
> +	}
> +        ofono_modem_set_powered(modem, TRUE);
> +	DBG("Powered on!");
> +}
> +
> +static int ublox_enable(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	const char *model_str = NULL;
> +
> +	DBG("%p", modem);
> +
> +	model_str = ofono_modem_get_string(modem, "Model");
> +	DBG("%p is model %s", modem, model_str);
> +	if (model_str == NULL)
> +		model_str = strdup("SARA_R410");
> +		//return -EINVAL;
> +
> +	data->model_id = atoi(model_str);
> +
> +	switch (data->model_id) {
> +	default:
> +		DBG("unknown ublox model id %d", data->model_id);
> +		//return -EINVAL;
> +		data->vendor_family = OFONO_VENDOR_UBLOX;
> +		break;
> +	}
> +
> +	if (data->vendor_family == OFONO_VENDOR_UBLOX) {
> +		data->dlc = open_device(modem, "Device", "Setup: ");
> +		if (data->dlc == NULL) {
> +			g_at_chat_unref(data->dlc);
> +			data->dlc = NULL;
> +			DBG("open_device failed!");
> +			return -EIO;
> +		}
> +
> +		g_at_chat_set_slave(data->modem, data->dlc);
> +
> +		g_at_chat_send(data->dlc, "AT+CFUN=0", none_prefix,
> +						NULL, NULL, NULL);
> +
> +		g_at_chat_send(data->dlc, "AT+UMNOPROF=1", NULL, NULL, NULL, NULL);
> +		//enable roaming in the modem
> +		g_at_chat_send(data->dlc, "AT+UDCONF=76,1,0", NULL, NULL, NULL, NULL);
> +	}
> +
> +	/* The modem can take a while to wake up if just powered on. */
> +	//g_at_chat_set_wakeup_command(data->dlcs[SETUP_DLC], "AT\r", 1000, 11000);
> +
> +	g_at_chat_send(data->dlc, "ATE0 +CMEE=1", none_prefix,
> +					NULL, NULL, NULL);
> +
> +	g_at_chat_send(data->dlc, "AT+CFUN=4", none_prefix,
> +					cfun_enable, modem, NULL);
> +
> +	DBG("Return in progress");
> +	return -EINPROGRESS;
> +}
> +
> +static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	g_at_chat_unref(data->dlc);
> +	data->dlc = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static int ublox_disable(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	g_at_chat_cancel_all(data->dlc);
> +	g_at_chat_unregister_all(data->dlc);
> +
> +	g_at_chat_send(data->dlc, "AT+CFUN=4", none_prefix,
> +					cfun_disable, modem, NULL);
> +
> +	g_at_chat_unref(data->modem);
> +	data->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);
> +	DBG("online!?");
> +}
> +
> +static void ublox_set_online(struct ofono_modem *modem, ofono_bool_t online,
> +				ofono_modem_online_cb_t cb, void *user_data)
> +{
> +	struct ublox_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->dlc, command, none_prefix, set_online_cb,
> +					cbd, g_free) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> +	g_free(cbd);
> +}
> +
> +static void ublox_pre_sim(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	struct ofono_sim *sim;
> +
> +	DBG("%p", modem);
> +
> +	ofono_devinfo_create(modem, 0, "atmodem", data->dlc);
> +	DBG("%p devinfo created", modem);
> +	sim = ofono_sim_create(modem, data->vendor_family, "atmodem",
> +					data->dlc);
> +
> +	DBG("%p sim created", modem);
> +
> +	if (sim)
> +	{
> +		DBG("%p sim notification", modem);
> +		ofono_sim_inserted_notify(sim, TRUE);
> +	}
> +}
> +
> +static void ublox_post_sim(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	struct ofono_gprs *gprs;
> +	struct ofono_gprs_context *gc;
> +	GAtChat *chat = data->dlc;
> +	const char *driver = "atmodem";
> +
> +	DBG("%p", modem);
> +
> +	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
> +					data->dlc);
> +
> +	gc = ofono_gprs_context_create(modem, data->vendor_family,
> +						driver, chat);
> +
> +	if (gprs && gc)
> +		ofono_gprs_add_context(gprs, gc);
> +
> +	ofono_lte_create(modem, 0, "atmodem", data->dlc);
> +}
> +
> +static void ublox_post_online(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	ofono_netreg_create(modem, data->vendor_family, "atmodem", data->dlc);
> +
> +	ofono_netmon_create(modem, data->vendor_family, "ubloxserial", data->modem);
> +}
> +
> +static struct ofono_modem_driver ublox_driver = {
> +	.name		= "ubloxserial",
> +	.probe		= ublox_probe,
> +	.remove		= ublox_remove,
> +	.enable		= ublox_enable,
> +	.disable	= ublox_disable,
> +	.set_online	= ublox_set_online,
> +	.pre_sim	= ublox_pre_sim,
> +	.post_sim	= ublox_post_sim,
> +	.post_online	= ublox_post_online,
> +};
> +
> +static int ublox_init(void)
> +{
> +	return ofono_modem_driver_register(&ublox_driver);
> +}
> +
> +static void ublox_exit(void)
> +{
> +	ofono_modem_driver_unregister(&ublox_driver);
> +}
> +
> +OFONO_PLUGIN_DEFINE(ubloxserial, "u-blox serial modem driver", VERSION,
> +		OFONO_PLUGIN_PRIORITY_DEFAULT, ublox_init, ublox_exit)
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index ce8cdee1..d95fc527 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1322,6 +1322,7 @@ static struct {
>   	{ "cinterion",	setup_serial_modem	},
>   	{ "nokiacdma",	setup_serial_modem	},
>   	{ "sim900",	setup_serial_modem	},
> +	{ "ubloxserial",setup_serial_modem	},
>   	{ "wavecom",	setup_wavecom		},
>   	{ "tc65",	setup_tc65		},
>   	{ "ehs6",	setup_ehs6		},
> 

Re: [PATCH 1/8] ublox-serial: add new plugin for u-blox

[Philippe De Swert]

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

Hi,

On 16/02/2019, Jonas Bonn <jonas@norrbonn.se> wrote:
> Hi,
>
> My spontaneous reaction to this is:  why does this need to be a separate
> plugin?  At first glance, it appears to be largely a copy of the ublox
> plugin and the seemingly small differences would have been easy to
> finesse into the existing driver.
> Could you provide some more justification for requiring a completely new
> plugin here?

True it looks very similar, however the current driver works for USB
only and also uses muxing which seems to be buggy on the serial port.
(it eats error messages for example).

> Thanks,
> Jonas
>
> (Just for info, I'm working on support for the TOBY L4 currently... it
> would be preferable to get as much u-blox support into one place as
> possible rather than spreading it out over multiple plugins/drivers.
> All these modems share common documentation, after all.)

I understand the sentiment but I did not immediately see a way to
combine the two, and giving the limited time to get it running making
it's own plugin turned out to be the easiest given the issues
(USB/broken mux) I ran into.

Regards,

Philippe

Re: [PATCH 1/8] ublox-serial: add new plugin for u-blox

[Jonas Bonn]

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

Hi,

On 16/02/2019 14:55, Philippe De Swert wrote:
> Hi,
> 
> On 16/02/2019, Jonas Bonn <jonas@norrbonn.se> wrote:
>> Hi,
>>
>> My spontaneous reaction to this is:  why does this need to be a separate
>> plugin?  At first glance, it appears to be largely a copy of the ublox
>> plugin and the seemingly small differences would have been easy to
>> finesse into the existing driver.
>> Could you provide some more justification for requiring a completely new
>> plugin here?
> 
> True it looks very similar, however the current driver works for USB
> only and also uses muxing which seems to be buggy on the serial port.
> (it eats error messages for example).
> 

Could you elaborate on this?  If you don't want the mux'ing, you just 
skip setting up the channel (that you don't have, anyway).  There's no 
other mux'ing in the driver.  If you skip that, I can't immediately see 
that you are actually doing anything particularly different that will 
cause any change in how error messages are propagated.



>> Thanks,
>> Jonas
>>
>> (Just for info, I'm working on support for the TOBY L4 currently... it
>> would be preferable to get as much u-blox support into one place as
>> possible rather than spreading it out over multiple plugins/drivers.
>> All these modems share common documentation, after all.)
> 
> I understand the sentiment but I did not immediately see a way to
> combine the two, and giving the limited time to get it running making
> it's own plugin turned out to be the easiest given the issues
> (USB/broken mux) I ran into.

I spent some time looking at this... I really don't see that this 
warrants it's own plugin.  Half of the diff is just data->aux --> 
data->dlc renaming... the rest is simple stuff that's easily hidden 
behind model checks.  This is better implemented as incremental changes 
to the existing ublox plugin; that's much easier to review and provides 
better insight into what's going on.

Or provide more justification as to why having this as a separate plugin 
is absolute necessary.

Thanks,
/Jonas


> 
> Regards,
> 
> Philippe
> 

Re: [PATCH 1/8] ublox-serial: add new plugin for u-blox

[Jonas Bonn]

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

Hi,

My overriding comment here is to try to combine this with the existing 
ublox plugin.  That said, there are some comments below.

/Jonas

On 15/02/2019 13:11, philippedeswert(a)gmail.com wrote:
> From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> 
> Add support for ublox modems that are used over the serial port.
> (tested on SARA-R410M and R412M)
> ---
>   plugins/ublox-serial.c | 393 +++++++++++++++++++++++++++++++++++++++++
>   plugins/udevng.c       |   1 +
>   2 files changed, 394 insertions(+)
>   create mode 100644 plugins/ublox-serial.c
> 
> diff --git a/plugins/ublox-serial.c b/plugins/ublox-serial.c
> new file mode 100644
> index 00000000..4f0cf5f3
> --- /dev/null
> +++ b/plugins/ublox-serial.c
> @@ -0,0 +1,393 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2014  Philip Paeps. All rights reserved.
> + *  Copyright (C) 2018-2019 Treon oy. All rights reserved.
> + *
> + *  Inspired on the Ublox driver by Philip Paeps
> + *
> + *  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 <string.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/netmon.h>
> +#include <ofono/lte.h>
> +
> +#include <drivers/atmodem/atutil.h>
> +#include <drivers/atmodem/vendor.h>
> +
> +static const char *none_prefix[] = { NULL };
> +
> +struct ublox_data {
> +	GIOChannel *device;
> +	GAtChat *modem;
> +	GAtChat *dlc;
> +	int model_id;
> +	enum ofono_vendor vendor_family;
> +	guint frame_size;
> +};
> +
> +static void ublox_debug(const char *str, void *user_data)
> +{
> +	const char *prefix = user_data;
> +
> +	ofono_info("%s%s", prefix, str);
> +}
> +
> +static int ublox_probe(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data;
> +
> +	DBG("%p", modem);
> +
> +	data = g_try_new0(struct ublox_data, 1);
> +	if (data == NULL)
> +		return -ENOMEM;
> +
> +	ofono_modem_set_data(modem, data);
> +
> +	return 0;
> +}
> +
> +static void ublox_remove(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_unref(data->modem);
> +	g_free(data);
> +}
> +

Your implementation of open_device is the same as the implementation in 
ublox.c with the addition of communication settings.  You could easily 
add this as open_serial_device in the ublox.c and call it for serial modems.

> +static GAtChat *open_device(struct ofono_modem *modem,
> +				const char *key, char *debug)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	const char *device;
> +	GAtSyntax *syntax;
> +	GIOChannel *channel;
> +	GAtChat *chat;
> +	GHashTable *options;
> +
> +	device = ofono_modem_get_string(modem, key);
> +	if (device == NULL)
> +	{
> +		DBG("device = NULL");
> +		return NULL;
> +	}
> +	DBG("%s %s", key, device);
> +
> +        options = g_hash_table_new(g_str_hash, g_str_equal);
> +        if (options == NULL)
> +		return NULL;
> +
> +        g_hash_table_insert(options, "Baud", "115200");
> +        g_hash_table_insert(options, "Parity", "none");
> +        g_hash_table_insert(options, "StopBits", "1");
> +        g_hash_table_insert(options, "DataBits", "8");
> +        g_hash_table_insert(options, "XonXoff", "on");
> +        g_hash_table_insert(options, "Local", "on");
> +        g_hash_table_insert(options, "RtsCts", "off");
> +        g_hash_table_insert(options, "Read", "on");
> +
> +	channel = g_at_tty_open(device, options);
> +        g_hash_table_destroy(options);
> +
> +	if (channel == NULL)
> +		return NULL;
> +
> +	data->device = channel;
> +	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, ublox_debug, debug);
> +
> +	return chat;
> +}
> +
> +#if 0

Here's a big block of commented out code.  This isn't from the ublox 
driver...

> +static GAtChat *create_chat(GIOChannel *channel, struct ofono_modem *modem,
> +                                char *debug)
> +{
> +        GAtSyntax *syntax;
> +        GAtChat *chat;
> +
> +        if (channel == NULL)
> +                return NULL;
> +
> +        syntax = g_at_syntax_new_gsmv1();
> +        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, ublox_debug, debug);
> +
> +        return chat;
> +}
> +
> +static void shutdown_device(struct ublox_data *data)
> +{
> +        DBG("");
> +
> +        g_at_chat_unref(data->dlc);
> +        data->dlc = NULL;
> +
> +        g_io_channel_unref(data->device);
> +        data->device = NULL;
> +}
> +#endif

... that ends here.  Drop all of this.

> +
> +static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ublox_data * data = ofono_modem_get_data(modem);
> +
> +	DBG("ok %d", ok);
> +
> +	if (!ok) {
> +		g_at_chat_unref(data->modem);
> +		data->modem = NULL;
> +		ofono_modem_set_powered(modem, FALSE);
> +		DBG("CFUN failed");
> +		return;
> +	}
> +        ofono_modem_set_powered(modem, TRUE);
> +	DBG("Powered on!");
> +}
> +
> +static int ublox_enable(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	const char *model_str = NULL;
> +
> +	DBG("%p", modem);
> +
> +	model_str = ofono_modem_get_string(modem, "Model");
> +	DBG("%p is model %s", modem, model_str);
> +	if (model_str == NULL)
> +		model_str = strdup("SARA_R410");
> +		//return -EINVAL;
> +

As you are going via add_serial_device() in udevng.c, the Model string 
will not be set so you don't even need to check for this.  That said, 
you aren't using the model_str value for anything anyway, so just drop it.

> +	data->model_id = atoi(model_str);

atoi("SARA_R410") looks interesting... does this work?

> +
> +	switch (data->model_id) {

Drop the switch as you have no options here.

> +	default:
> +		DBG("unknown ublox model id %d", data->model_id);

Unknown?

> +		//return -EINVAL;
> +		data->vendor_family = OFONO_VENDOR_UBLOX;



> +		break;
> +	}
> +
> +	if (data->vendor_family == OFONO_VENDOR_UBLOX) {

You've asked for this above... no need for the if() statement.


> +		data->dlc = open_device(modem, "Device", "Setup: ");

> +		if (data->dlc == NULL) {
> +			g_at_chat_unref(data->dlc);

_unref(NULL) ... not necessary.

> +			data->dlc = NULL;

You already tested for this above.  This is a no-op.

> +			DBG("open_device failed!");
> +			return -EIO;

> +		}
> +

If you stuck to the name data->aux above and opened either "Device" or 
"Aux" depending on existence of the string or on device model then your 
diff against ublox.c is automatically 50% as big as now.


> +		g_at_chat_set_slave(data->modem, data->dlc);
> +
> +		g_at_chat_send(data->dlc, "AT+CFUN=0", none_prefix,
> +						NULL, NULL, NULL);
> +
> +		g_at_chat_send(data->dlc, "AT+UMNOPROF=1", NULL, NULL, NULL, NULL);
> +		//enable roaming in the modem
> +		g_at_chat_send(data->dlc, "AT+UDCONF=76,1,0", NULL, NULL, NULL, NULL);
> +	}
> +
> +	/* The modem can take a while to wake up if just powered on. */
> +	//g_at_chat_set_wakeup_command(data->dlcs[SETUP_DLC], "AT\r", 1000, 11000);

->dlcs does not exist... you don't enable mux'ing anywhere in the 
driver.  Yes, I realize this is commented out, but there's half of a 
thought here that I'd like to reconcile since you've left the commented 
code in place.  Did you intend on using mux'ing later?  Either way, I 
think you pretty much just end up with MUX on channel 0, AUX on channel 
1, and MODEM on channel2 and if you set up the chats accordingly then 
everything becomes independent of underlying device... (or something 
like that, I think).

> +
> +	g_at_chat_send(data->dlc, "ATE0 +CMEE=1", none_prefix,
> +					NULL, NULL, NULL);
> +
> +	g_at_chat_send(data->dlc, "AT+CFUN=4", none_prefix,
> +					cfun_enable, modem, NULL);
> +
> +	DBG("Return in progress");
> +	return -EINPROGRESS;
> +}
> +
> +static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	g_at_chat_unref(data->dlc);
> +	data->dlc = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static int ublox_disable(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	g_at_chat_cancel_all(data->dlc);
> +	g_at_chat_unregister_all(data->dlc);
> +
> +	g_at_chat_send(data->dlc, "AT+CFUN=4", none_prefix,
> +					cfun_disable, modem, NULL);
> +
> +	g_at_chat_unref(data->modem);
> +	data->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);
> +	DBG("online!?");
> +}
> +
> +static void ublox_set_online(struct ofono_modem *modem, ofono_bool_t online,
> +				ofono_modem_online_cb_t cb, void *user_data)
> +{
> +	struct ublox_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->dlc, command, none_prefix, set_online_cb,
> +					cbd, g_free) > 0)
> +		return;
> +
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> +	g_free(cbd);
> +}
> +
> +static void ublox_pre_sim(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	struct ofono_sim *sim;
> +
> +	DBG("%p", modem);
> +
> +	ofono_devinfo_create(modem, 0, "atmodem", data->dlc);
> +	DBG("%p devinfo created", modem);
> +	sim = ofono_sim_create(modem, data->vendor_family, "atmodem",
> +					data->dlc);
> +
> +	DBG("%p sim created", modem);
> +
> +	if (sim)
> +	{
> +		DBG("%p sim notification", modem);
> +		ofono_sim_inserted_notify(sim, TRUE);
> +	}
> +}
> +
> +static void ublox_post_sim(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +	struct ofono_gprs *gprs;
> +	struct ofono_gprs_context *gc;
> +	GAtChat *chat = data->dlc;
> +	const char *driver = "atmodem";
> +
> +	DBG("%p", modem);
> +
> +	gprs = ofono_gprs_create(modem, data->vendor_family, "atmodem",
> +					data->dlc);
> +
> +	gc = ofono_gprs_context_create(modem, data->vendor_family,
> +						driver, chat);
> +
> +	if (gprs && gc)
> +		ofono_gprs_add_context(gprs, gc);
> +
> +	ofono_lte_create(modem, 0, "atmodem", data->dlc);

The 'atmodem' variant of the lte atom uses CGAUTH for setting up the 
default bearer configuration.  This command isn't available on the 
SARA-R4, as far as I can see.

The current u-blox LTE driver uses CGDFLT, but that isn't available on 
your modem either (only on the TOBY L2).  You probably want UAUTHREQ here...

> +}
> +
> +static void ublox_post_online(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	ofono_netreg_create(modem, data->vendor_family, "atmodem", data->dlc);
> +
> +	ofono_netmon_create(modem, data->vendor_family, "ubloxserial", data->modem);
> +}
> +
> +static struct ofono_modem_driver ublox_driver = {
> +	.name		= "ubloxserial",
> +	.probe		= ublox_probe,
> +	.remove		= ublox_remove,
> +	.enable		= ublox_enable,
> +	.disable	= ublox_disable,
> +	.set_online	= ublox_set_online,
> +	.pre_sim	= ublox_pre_sim,
> +	.post_sim	= ublox_post_sim,
> +	.post_online	= ublox_post_online,
> +};
> +
> +static int ublox_init(void)
> +{
> +	return ofono_modem_driver_register(&ublox_driver);
> +}
> +
> +static void ublox_exit(void)
> +{
> +	ofono_modem_driver_unregister(&ublox_driver);
> +}
> +
> +OFONO_PLUGIN_DEFINE(ubloxserial, "u-blox serial modem driver", VERSION,
> +		OFONO_PLUGIN_PRIORITY_DEFAULT, ublox_init, ublox_exit)
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index ce8cdee1..d95fc527 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -1322,6 +1322,7 @@ static struct {
>   	{ "cinterion",	setup_serial_modem	},
>   	{ "nokiacdma",	setup_serial_modem	},
>   	{ "sim900",	setup_serial_modem	},
> +	{ "ubloxserial",setup_serial_modem	},
>   	{ "wavecom",	setup_wavecom		},
>   	{ "tc65",	setup_tc65		},
>   	{ "ehs6",	setup_ehs6		},
> 

[PATCH 2/8] Makefile.am : include new u-blox serial modem driver

[philippedeswert]

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

From: Philippe De Swert <philippe.deswert@digitalistgroup.com>

Make sure the new u-blox serial modem driver gets built.
---
 Makefile.am | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Makefile.am b/Makefile.am
index 6aa8f8fe..df7827d4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -596,6 +596,10 @@ builtin_sources += plugins/quectel.c
 builtin_modules += ublox
 builtin_sources += plugins/ublox.c
 
+
+builtin_modules += ubloxserial
+builtin_sources += plugins/ublox-serial.c
+
 builtin_modules += xmm7xxx
 builtin_sources += plugins/xmm7xxx.c
 
-- 
2.20.1

[PATCH 3/8] gprs.h: Add ofono_gprs_get_netreg function

[philippedeswert]

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

From: Tommi Rekosuo <tommi.rekosuo@treon.fi>

Add function to get registration technology.
---
 include/gprs.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/gprs.h b/include/gprs.h
index 988d6102..eaf9f2de 100644
--- a/include/gprs.h
+++ b/include/gprs.h
@@ -62,6 +62,7 @@ void ofono_gprs_resume_notify(struct ofono_gprs *gprs);
 void ofono_gprs_bearer_notify(struct ofono_gprs *gprs, int bearer);
 
 struct ofono_modem *ofono_gprs_get_modem(struct ofono_gprs *gprs);
+struct ofono_netreg *ofono_gprs_get_netreg(struct ofono_gprs *gprs);
 
 int ofono_gprs_driver_register(const struct ofono_gprs_driver *d);
 void ofono_gprs_driver_unregister(const struct ofono_gprs_driver *d);
-- 
2.20.1

[PATCH 4/8] gprs.c: Add ofono_gprs_get_netreg function

[philippedeswert]

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

From: Tommi Rekosuo <tommi.rekosuo@treon.fi>

Add function to get registration technology.
---
 src/gprs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/gprs.c b/src/gprs.c
index 3dce001b..f1146d06 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -3392,3 +3392,8 @@ void *ofono_gprs_get_data(struct ofono_gprs *gprs)
 {
 	return gprs->driver_data;
 }
+
+struct ofono_netreg *ofono_gprs_get_netreg(struct ofono_gprs *gprs)
+{
+	return gprs->netreg;
+}
-- 
2.20.1

[PATCH 5/8] atmodem/gprs.c: Add CEREG support

[philippedeswert]

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

From: Tommi Rekosuo <tommi.rekosuo@treon.fi>

For NB-IoT modems we need to use AT+CEREG instead of AT+CGREG
when in LTE network and detecting network registration in initial boot.
Enable +CEREG URC and use that also to detect registration state.
---
 drivers/atmodem/gprs.c | 60 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index de7c7864..d6128a42 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -34,6 +34,7 @@
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/gprs.h>
+#include <ofono/netreg.h>
 
 #include "gatchat.h"
 #include "gatresult.h"
@@ -42,6 +43,7 @@
 #include "vendor.h"
 
 static const char *cgreg_prefix[] = { "+CGREG:", NULL };
+static const char *cereg_prefix[] = { "+CEREG:", NULL };
 static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
 static const char *none_prefix[] = { NULL };
 
@@ -108,12 +110,38 @@ static void at_cgreg_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	cb(&error, status, cbd->data);
 }
 
+static void at_cereg_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_gprs_status_cb_t cb = cbd->cb;
+	struct ofono_error error;
+	int status;
+	struct gprs_data *gd = cbd->user;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		cb(&error, -1, cbd->data);
+		return;
+	}
+
+	if (at_util_parse_reg(result, "+CEREG:", NULL, &status,
+                        NULL, NULL, NULL, gd->vendor) == FALSE) {
+		CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+		return;
+	}
+
+	cb(&error, status, cbd->data);
+}
+
 static void at_gprs_registration_status(struct ofono_gprs *gprs,
 					ofono_gprs_status_cb_t cb,
 					void *data)
 {
 	struct gprs_data *gd = ofono_gprs_get_data(gprs);
 	struct cb_data *cbd = cb_data_new(cb, data);
+	struct ofono_netreg *netreg = ofono_gprs_get_netreg(gprs);
+	int ret = 0;
 
 	cbd->user = gd;
 
@@ -136,9 +164,16 @@ static void at_gprs_registration_status(struct ofono_gprs *gprs,
 		break;
 	}
 
-	if (g_at_chat_send(gd->chat, "AT+CGREG?", cgreg_prefix,
-				at_cgreg_cb, cbd, g_free) > 0)
-		return;
+	if (ofono_netreg_get_technology(netreg) >= 7) {
+		ret = g_at_chat_send(gd->chat, "AT+CEREG?", cereg_prefix,
+				     at_cereg_cb, cbd, g_free);
+	} else {
+		ret = g_at_chat_send(gd->chat, "AT+CGREG?", cgreg_prefix,
+				     at_cgreg_cb, cbd, g_free);
+	}
+
+	if (ret > 0)
+	  return;
 
 	g_free(cbd);
 
@@ -418,6 +453,19 @@ static void ublox_ureg_notify(GAtResult *result, gpointer user_data)
 	ofono_gprs_bearer_notify(gprs, bearer);
 }
 
+static void ublox_cereg_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_gprs *gprs = user_data;
+	int status;
+	struct gprs_data *gd = ofono_gprs_get_data(gprs);
+
+	if (at_util_parse_reg_unsolicited(result, "+CEREG:", &status,
+					  NULL, NULL, NULL, gd->vendor) == FALSE)
+	  return;
+
+	ofono_gprs_status_notify(gprs, status);
+}
+
 static void cpsb_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_gprs *gprs = user_data;
@@ -458,6 +506,8 @@ static void gprs_initialized(gboolean ok, GAtResult *result, gpointer user_data)
 	case OFONO_VENDOR_UBLOX_TOBY_L2:
 		g_at_chat_register(gd->chat, "+UREG:", ublox_ureg_notify,
 						FALSE, gprs, NULL);
+		g_at_chat_register(gd->chat, "+CEREG:", ublox_cereg_notify,
+				   FALSE, gprs, NULL);
 		g_at_chat_send(gd->chat, "AT+UREG=1", none_prefix,
 						NULL, NULL, NULL);
 		break;
@@ -528,6 +578,10 @@ retry:
 		goto error;
 
 	g_at_chat_send(gd->chat, cmd, none_prefix, NULL, NULL, NULL);
+	/* Enable +CEREG URC for UBLOX */
+	if (gd->vendor == OFONO_VENDOR_UBLOX) {
+		g_at_chat_send(gd->chat, "AT+CEREG=2", none_prefix, NULL, NULL, NULL);
+	}
 	g_at_chat_send(gd->chat, "AT+CGAUTO=0", none_prefix, NULL, NULL, NULL);
 
 	switch (gd->vendor) {
-- 
2.20.1

Re: [PATCH 5/8] atmodem/gprs.c: Add CEREG support

[Denis Kenzior]

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

Hi Tommi,

On 02/15/2019 06:11 AM, philippedeswert(a)gmail.com wrote:
> From: Tommi Rekosuo <tommi.rekosuo@treon.fi>
> 
> For NB-IoT modems we need to use AT+CEREG instead of AT+CGREG
> when in LTE network and detecting network registration in initial boot.
> Enable +CEREG URC and use that also to detect registration state.

Read the thread I referred to in one of my other replies.  gprs should 
really be taught to understand that it is on LTE and not even bother 
asking for registration_status from gprs driver in those cases.  That 
would remove any need to add the code in this proposal.

Regards,
-Denis

[PATCH 6/8] gprs-context : Force use of atd99

[philippedeswert]

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

From: Philippe De Swert <philippe.deswert@digitalistgroup.com>

QUIRCK: For some reason I really need to force the use of the atd99 command here.
---
 drivers/atmodem/gprs-context.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 93894efd..a0776f2a 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -214,7 +214,7 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
 		return;
 	}
 
-	if (gcd->use_atd99)
+	if (gcd->use_atd99 || gcd->vendor == OFONO_VENDOR_UBLOX)
 		sprintf(buf, "ATD*99***%u#", gcd->active_context);
 	else
 		sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
@@ -457,6 +457,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
 	case OFONO_VENDOR_SIMCOM_SIM900:
 		gcd->use_atd99 = FALSE;
 		break;
+	case OFONO_VENDOR_UBLOX:
+		gcd->use_atd99 = TRUE;
+		break;
 	default:
 		g_at_chat_send(chat, "AT+CGDATA=?", cgdata_prefix,
 						at_cgdata_test_cb, gc, NULL);
-- 
2.20.1

Re: [PATCH 6/8] gprs-context : Force use of atd99

[Denis Kenzior]

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

Hi Philippe,

On 02/15/2019 06:11 AM, philippedeswert(a)gmail.com wrote:
> From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> 
> QUIRCK: For some reason I really need to force the use of the atd99 command here.
> ---
>   drivers/atmodem/gprs-context.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index 93894efd..a0776f2a 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -214,7 +214,7 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   		return;
>   	}
>   
> -	if (gcd->use_atd99)
> +	if (gcd->use_atd99 || gcd->vendor == OFONO_VENDOR_UBLOX)
>   		sprintf(buf, "ATD*99***%u#", gcd->active_context);
>   	else
>   		sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
> @@ -457,6 +457,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
>   	case OFONO_VENDOR_SIMCOM_SIM900:
>   		gcd->use_atd99 = FALSE;
>   		break;
> +	case OFONO_VENDOR_UBLOX:
> +		gcd->use_atd99 = TRUE;
> +		break;

So why use both use_atd99 and a vendor quirk above?  Also, you're 
affecting other uBlox users, so do you want to introduce a different 
VENDOR quirk instead?

>   	default:
>   		g_at_chat_send(chat, "AT+CGDATA=?", cgdata_prefix,
>   						at_cgdata_test_cb, gc, NULL);
> 

Regards,
-Denis

Re: [PATCH 6/8] gprs-context : Force use of atd99

[Reinhard Speyerer]

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

On Sun, Feb 17, 2019 at 09:08:12PM -0600, Denis Kenzior wrote:
> Hi Philippe,
> 
> On 02/15/2019 06:11 AM, philippedeswert(a)gmail.com wrote:
> > From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> > 
> > QUIRCK: For some reason I really need to force the use of the atd99 command here.
> > ---
> >   drivers/atmodem/gprs-context.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> > index 93894efd..a0776f2a 100644
> > --- a/drivers/atmodem/gprs-context.c
> > +++ b/drivers/atmodem/gprs-context.c
> > @@ -214,7 +214,7 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
> >   		return;
> >   	}
> > -	if (gcd->use_atd99)
> > +	if (gcd->use_atd99 || gcd->vendor == OFONO_VENDOR_UBLOX)
> >   		sprintf(buf, "ATD*99***%u#", gcd->active_context);
> >   	else
> >   		sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
> > @@ -457,6 +457,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
> >   	case OFONO_VENDOR_SIMCOM_SIM900:
> >   		gcd->use_atd99 = FALSE;
> >   		break;
> > +	case OFONO_VENDOR_UBLOX:
> > +		gcd->use_atd99 = TRUE;
> > +		break;
> 
> So why use both use_atd99 and a vendor quirk above?  Also, you're affecting
> other uBlox users, so do you want to introduce a different VENDOR quirk
> instead?

Hi Denis,

the effect observed by Philippe is caused by the fact that Qualcomm
firmwares typically do not support the use of AT+CGDATA="PPP",<cid>
for contexts which have already been activated with AT+CGACT although
they respond with +CGDATA: ("PPP") to AT+CGDATA=?.

To minimize changes for other uBlox users it might make sense to limit
the use of ATD*99# to Qualcomm-based devices like their SARA R4 series.

Regards,
Reinhard

Re: [PATCH 6/8] gprs-context : Force use of atd99

[Jonas Bonn]

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

Hi,

Could I get a clarification on the behaviour you see here?

As I understand it, the SARA-R4 doesn't support the CGDATA command. 
That's fine as ofono will detect this and set use_atd99 in order to 
force the use of the ATD command as a fallback.

Why do you need to force this setting in the below patch?  Does the 
CGDATA command not return ERROR?  What does it return?

Thanks,
Jonas

On 15/02/2019 13:11, philippedeswert(a)gmail.com wrote:
> From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> 
> QUIRCK: For some reason I really need to force the use of the atd99 command here.
> ---
>   drivers/atmodem/gprs-context.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index 93894efd..a0776f2a 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -214,7 +214,7 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   		return;
>   	}
>   
> -	if (gcd->use_atd99)
> +	if (gcd->use_atd99 || gcd->vendor == OFONO_VENDOR_UBLOX)
>   		sprintf(buf, "ATD*99***%u#", gcd->active_context);
>   	else
>   		sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
> @@ -457,6 +457,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
>   	case OFONO_VENDOR_SIMCOM_SIM900:
>   		gcd->use_atd99 = FALSE;
>   		break;
> +	case OFONO_VENDOR_UBLOX:
> +		gcd->use_atd99 = TRUE;
> +		break;
>   	default:
>   		g_at_chat_send(chat, "AT+CGDATA=?", cgdata_prefix,
>   						at_cgdata_test_cb, gc, NULL);
> 

Re: [PATCH 6/8] gprs-context : Force use of atd99

[Reinhard Speyerer]

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

On Wed, Mar 13, 2019 at 09:19:13AM +0100, Jonas Bonn wrote:
> Hi,
> 
> Could I get a clarification on the behaviour you see here?
> 
> As I understand it, the SARA-R4 doesn't support the CGDATA command. That's
> fine as ofono will detect this and set use_atd99 in order to force the use
> of the ATD command as a fallback.
> 
> Why do you need to force this setting in the below patch?  Does the CGDATA
> command not return ERROR?  What does it return?
> 
> Thanks,
> Jonas

Hi Jonas,

please see my explanation given here
https://lists.ofono.org/pipermail/ofono/2019-February/019154.html
on why Philippe had to implement ATD99 forcing for the SARA-R4 series.

My (limited) tests indicate that another AT feature used by Ofono
(the "+CGEV: NW DEACT" URC to detect network-initiated PDP context
deactivation) also no longer seems to be reliable starting with
Qualcomm MDM9x15 firmwares (e.g. Gemalto PLS8).

Regards,
Reinhard

> 
> On 15/02/2019 13:11, philippedeswert(a)gmail.com wrote:
> > From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> > 
> > QUIRCK: For some reason I really need to force the use of the atd99 command here.
> > ---
> >   drivers/atmodem/gprs-context.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> > index 93894efd..a0776f2a 100644
> > --- a/drivers/atmodem/gprs-context.c
> > +++ b/drivers/atmodem/gprs-context.c
> > @@ -214,7 +214,7 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
> >   		return;
> >   	}
> > -	if (gcd->use_atd99)
> > +	if (gcd->use_atd99 || gcd->vendor == OFONO_VENDOR_UBLOX)
> >   		sprintf(buf, "ATD*99***%u#", gcd->active_context);
> >   	else
> >   		sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
> > @@ -457,6 +457,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
> >   	case OFONO_VENDOR_SIMCOM_SIM900:
> >   		gcd->use_atd99 = FALSE;
> >   		break;
> > +	case OFONO_VENDOR_UBLOX:
> > +		gcd->use_atd99 = TRUE;
> > +		break;
> >   	default:
> >   		g_at_chat_send(chat, "AT+CGDATA=?", cgdata_prefix,
> >   						at_cgdata_test_cb, gc, NULL);
> > 
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 6/8] gprs-context : Force use of atd99

[Philippe De Swert]

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

Hi Jonas,

On 13/03/2019, Jonas Bonn <jonas@norrbonn.se> wrote:
> Hi,
>
> Could I get a clarification on the behaviour you see here?
>
> As I understand it, the SARA-R4 doesn't support the CGDATA command.
> That's fine as ofono will detect this and set use_atd99 in order to
> force the use of the ATD command as a fallback.

For some reason it does not detect it.

> Why do you need to force this setting in the below patch?  Does the
> CGDATA command not return ERROR?  What does it return?

Yes it throws an error and then ofono just gives up. I could check the
detailed error but I don't think it is of any use.

Regards,

Philippe

[PATCH 7/8] common: Add new NB-IoT technologies

[philippedeswert]

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

From: Philippe De Swert <philippe.deswert@digitalistgroup.com>

Add lte-cat-m1 and lte-cat-nb1 technology identifiers.
---
 src/common.c | 4 ++++
 src/common.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/src/common.c b/src/common.c
index ea7842cc..4dcbc835 100644
--- a/src/common.c
+++ b/src/common.c
@@ -697,6 +697,10 @@ const char *registration_tech_to_string(int tech)
 		return "hspa";
 	case ACCESS_TECHNOLOGY_EUTRAN:
 		return "lte";
+	case ACCESS_TECHNOLOGY_NB_IOT_M1:
+		return "lte-cat-m1";
+	case ACCESS_TECHNOLOGY_NB_IOT_NB1:
+		return "lte-cat-nb1";
 	default:
 		return "";
 	}
diff --git a/src/common.h b/src/common.h
index dc618942..8fef4432 100644
--- a/src/common.h
+++ b/src/common.h
@@ -33,6 +33,8 @@ enum access_technology {
 	ACCESS_TECHNOLOGY_UTRAN_HSUPA =		5,
 	ACCESS_TECHNOLOGY_UTRAN_HSDPA_HSUPA =	6,
 	ACCESS_TECHNOLOGY_EUTRAN =		7,
+	ACCESS_TECHNOLOGY_NB_IOT_M1 =		8,
+	ACCESS_TECHNOLOGY_NB_IOT_NB1 =		9,
 };
 
 /* 27.007 Section 7.2 <stat> */
-- 
2.20.1

Re: [PATCH 7/8] common: Add new NB-IoT technologies

[Denis Kenzior]

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

Hi Philippe,

On 02/15/2019 06:11 AM, philippedeswert(a)gmail.com wrote:
> From: Philippe De Swert <philippe.deswert@digitalistgroup.com>
> 
> Add lte-cat-m1 and lte-cat-nb1 technology identifiers.
> ---
>   src/common.c | 4 ++++
>   src/common.h | 2 ++
>   2 files changed, 6 insertions(+)

Applied, thanks.

Regards,
-Denis

[PATCH 8/8] Fix gprs_netreg_update behaviour if PPP connection active

[philippedeswert]

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

From: Tommi Rekosuo <tommi.rekosuo@treon.fi>

If a PPP connection is active, the modem can't detach before
the PPP connection has been disabled. This can happen if
RoamingAllowed is changed when connection is active.
This leaves connman in undefined state as the context
'Active' state is not propagated properly and connman
still thinks we are connected.
---
 src/gprs.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index f1146d06..9df273b1 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1547,18 +1547,26 @@ static void release_active_contexts(struct ofono_gprs *gprs)
 {
 	GSList *l;
 	struct pri_context *ctx;
+	DBusConnection *conn;
+	dbus_bool_t value;
 
 	for (l = gprs->contexts; l; l = l->next) {
 		struct ofono_gprs_context *gc;
 
 		ctx = l->data;
 
-		if (ctx->active == FALSE)
+		DBG("Context id: %d", ctx->context.cid);
+
+		if (ctx->active == FALSE) {
+			DBG("Context not active");
 			continue;
+		}
 
 		/* This context is already being messed with */
-		if (ctx->pending)
+		if (ctx->pending) {
+			DBG("Context in pending state");
 			continue;
+		}
 
 		gc = ctx->context_driver;
 
@@ -1567,6 +1575,11 @@ static void release_active_contexts(struct ofono_gprs *gprs)
 
 		/* Make sure the context is properly cleared */
 		release_context(ctx);
+		value = ctx->active;
+		conn = ofono_dbus_get_connection();
+		ofono_dbus_signal_property_changed(conn, ctx->path,
+					OFONO_CONNECTION_CONTEXT_INTERFACE,
+					"Active", DBUS_TYPE_BOOLEAN, &value);
 	}
 }
 
@@ -1688,6 +1701,13 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
 		return;
 	}
 
+	/* If PPP session if active it needs to be disconnected
+	 * before the driver can send at commands */
+	if (!attach) {
+		DBG("Release active contexts");
+		release_active_contexts(gprs);
+	}
+
 	gprs->flags |= GPRS_FLAG_ATTACHING;
 
 	gprs->driver_attached = attach;
-- 
2.20.1

Re: [PATCH 8/8] Fix gprs_netreg_update behaviour if PPP connection active

[Denis Kenzior]

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

Hi Tommi,

On 02/15/2019 06:11 AM, philippedeswert(a)gmail.com wrote:
> From: Tommi Rekosuo <tommi.rekosuo@treon.fi>
> 
> If a PPP connection is active, the modem can't detach before
> the PPP connection has been disabled. This can happen if

I don't believe this is true for every modem.  What exactly is your 
modem doing here?  A log might be a good start.

> RoamingAllowed is changed when connection is active.

Note that on LTE, oFono shouldn't really issue a detach when 
RoamingAllowed is toggled.  Roaming on LTE cannot work in the same way 
as on 3G.

27.007 still sort of mandates that a +CGATT on LTE should trigger a 
detach from PS, and that would obviously trigger a full disconnect and 
reconnect.  So oFono core should be fixed not to do that if you want to 
support roaming on LTE properly.

This was discussed earlier here (and maybe in a few other threads)

https://lists.ofono.org/pipermail/ofono/2018-September/018514.html
https://lists.ofono.org/pipermail/ofono/2018-September/018535.html

> This leaves connman in undefined state as the context
> 'Active' state is not propagated properly and connman
> still thinks we are connected. > ---
>   src/gprs.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index f1146d06..9df273b1 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -1547,18 +1547,26 @@ static void release_active_contexts(struct ofono_gprs *gprs)
>   {
>   	GSList *l;
>   	struct pri_context *ctx;
> +	DBusConnection *conn;
> +	dbus_bool_t value;
>   
>   	for (l = gprs->contexts; l; l = l->next) {
>   		struct ofono_gprs_context *gc;
>   
>   		ctx = l->data;
>   
> -		if (ctx->active == FALSE)
> +		DBG("Context id: %d", ctx->context.cid);
> +
> +		if (ctx->active == FALSE) {
> +			DBG("Context not active");
>   			continue;
> +		}

This seems to be debug code and is extraneous.  If you really want this 
for some reason, then use a separate commit

>   
>   		/* This context is already being messed with */
> -		if (ctx->pending)
> +		if (ctx->pending) {
> +			DBG("Context in pending state");
>   			continue;
> +		}

as above

>   
>   		gc = ctx->context_driver;
>   
> @@ -1567,6 +1575,11 @@ static void release_active_contexts(struct ofono_gprs *gprs)
>   
>   		/* Make sure the context is properly cleared */
>   		release_context(ctx);
> +		value = ctx->active;
> +		conn = ofono_dbus_get_connection();
> +		ofono_dbus_signal_property_changed(conn, ctx->path,
> +					OFONO_CONNECTION_CONTEXT_INTERFACE,
> +					"Active", DBUS_TYPE_BOOLEAN, &value);

release_active_contexts isn't really meant to send any signals.  It is 
meant to clean up and syncronize state between gprs and gprs_context 
drivers.

I suspect you need to give a bit more information about what is actually 
going on.  As mentioned earlier, RoamingAllowed on LTE is a bit of a mess.

>   	}
>   }
>   
> @@ -1688,6 +1701,13 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
>   		return;
>   	}
>   
> +	/* If PPP session if active it needs to be disconnected
> +	 * before the driver can send at commands */
> +	if (!attach) {
> +		DBG("Release active contexts");
> +		release_active_contexts(gprs);
> +	}
> +
>   	gprs->flags |= GPRS_FLAG_ATTACHING;
>   
>   	gprs->driver_attached = attach;
> 

Regards,
-Denis