[RFC at-export 0/2] Export AT-modem functions to other drivers

[RFC at-export 0/2] Export AT-modem functions to other drivers

[Jonas Bonn]

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

Hi Denis,

I'm writing a custom network-registration driver for uBlox modems and
I'd like to know if you will choke on the following approach:

i)  export method implementations from atmodem driver
ii) set up uBlox netreg vtable with custom probe method and exported
atmodem implementations for rest

If you can't accept this approach, please let me know of an alternative
method that would be preferable that (hopefully) doesn't entail just
copying all the code from one driver to the other (1000+ lines to keep
in sync going forward).

Best regards,
Jonas

PS:  This is an RFC... patches are not quite done.  That said, feel free
to patch review if you've got a moment; the final result shouldn't
differ all that much from what's here.

Jonas Bonn (2):
  atmodem: export generic netreg funcs
  ublox: network-registration atom

 drivers/atmodem/network-registration.c    |  16 +-
 drivers/atmodem/network-registration.h    |  17 +
 drivers/ubloxmodem/network-registration.c | 413 ++++++++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c           |   2 +
 drivers/ubloxmodem/ubloxmodem.h           |   3 +
 5 files changed, 444 insertions(+), 7 deletions(-)
 create mode 100644 drivers/atmodem/network-registration.h
 create mode 100644 drivers/ubloxmodem/network-registration.c

-- 
2.20.1

[RFC at-export 1/2] atmodem: export generic netreg funcs

[Jonas Bonn]

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

An upcoming netreg driver for uBlox modems will need to override the
probe method in order to set itself up, but for further functionality
the "generic" AT implementations are sufficient.  The easiest way to do
this is to just set up a vtable with a custom probe implementation and
defer all other methods to the common/generic methods.

The problem is that the AT methods are not actually exported.  This
generic AT functionality was not intended to be hooked directly into
other drivers.

This patch exports all the methods of the atmodem network-registration
driver implementation so that they can be used as generic/common
implementations for other drivers.
---
 drivers/atmodem/network-registration.c | 16 +++++++++-------
 drivers/atmodem/network-registration.h | 17 +++++++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)
 create mode 100644 drivers/atmodem/network-registration.h

diff --git a/drivers/atmodem/network-registration.c b/drivers/atmodem/network-registration.c
index 67380b73..0f83977b 100644
--- a/drivers/atmodem/network-registration.c
+++ b/drivers/atmodem/network-registration.c
@@ -41,6 +41,8 @@
 #include "atmodem.h"
 #include "vendor.h"
 
+#include "network-registration.h"
+
 static const char *none_prefix[] = { NULL };
 static const char *creg_prefix[] = { "+CREG:", NULL };
 static const char *cops_prefix[] = { "+COPS:", NULL };
@@ -270,7 +272,7 @@ static void option_tech_cb(gboolean ok, GAtResult *result, gpointer user_data)
 		nd->tech = -1;
 }
 
-static void at_registration_status(struct ofono_netreg *netreg,
+void at_registration_status(struct ofono_netreg *netreg,
 					ofono_netreg_status_cb_t cb,
 					void *data)
 {
@@ -450,7 +452,7 @@ error:
 	g_free(cbd);
 }
 
-static void at_current_operator(struct ofono_netreg *netreg,
+void at_current_operator(struct ofono_netreg *netreg,
 				ofono_netreg_operator_cb_t cb, void *data)
 {
 	struct netreg_data *nd = ofono_netreg_get_data(netreg);
@@ -589,7 +591,7 @@ static void cops_list_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	g_free(list);
 }
 
-static void at_list_operators(struct ofono_netreg *netreg,
+void at_list_operators(struct ofono_netreg *netreg,
 				ofono_netreg_operator_list_cb_t cb, void *data)
 {
 	struct netreg_data *nd = ofono_netreg_get_data(netreg);
@@ -615,7 +617,7 @@ static void register_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	cb(&error, cbd->data);
 }
 
-static void at_register_auto(struct ofono_netreg *netreg,
+void at_register_auto(struct ofono_netreg *netreg,
 				ofono_netreg_register_cb_t cb, void *data)
 {
 	struct netreg_data *nd = ofono_netreg_get_data(netreg);
@@ -630,7 +632,7 @@ static void at_register_auto(struct ofono_netreg *netreg,
 	CALLBACK_WITH_FAILURE(cb, data);
 }
 
-static void at_register_manual(struct ofono_netreg *netreg,
+void at_register_manual(struct ofono_netreg *netreg,
 				const char *mcc, const char *mnc,
 				ofono_netreg_register_cb_t cb, void *data)
 {
@@ -1228,7 +1230,7 @@ static void csq_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	cb(&error, strength, cbd->data);
 }
 
-static void at_signal_strength(struct ofono_netreg *netreg,
+void at_signal_strength(struct ofono_netreg *netreg,
 				ofono_netreg_strength_cb_t cb, void *data)
 {
 	struct netreg_data *nd = ofono_netreg_get_data(netreg);
@@ -2144,7 +2146,7 @@ static int at_netreg_probe(struct ofono_netreg *netreg, unsigned int vendor,
 	return 0;
 }
 
-static void at_netreg_remove(struct ofono_netreg *netreg)
+void at_netreg_remove(struct ofono_netreg *netreg)
 {
 	struct netreg_data *nd = ofono_netreg_get_data(netreg);
 
diff --git a/drivers/atmodem/network-registration.h b/drivers/atmodem/network-registration.h
new file mode 100644
index 00000000..0f622411
--- /dev/null
+++ b/drivers/atmodem/network-registration.h
@@ -0,0 +1,17 @@
+#pragma once
+
+void at_registration_status(struct ofono_netreg *netreg,
+					ofono_netreg_status_cb_t cb,
+					void *data);
+void at_current_operator(struct ofono_netreg *netreg,
+				ofono_netreg_operator_cb_t cb, void *data);
+void at_list_operators(struct ofono_netreg *netreg,
+				ofono_netreg_operator_list_cb_t cb, void *data);
+void at_register_auto(struct ofono_netreg *netreg,
+				ofono_netreg_register_cb_t cb, void *data);
+void at_register_manual(struct ofono_netreg *netreg,
+				const char *mcc, const char *mnc,
+				ofono_netreg_register_cb_t cb, void *data);
+void at_signal_strength(struct ofono_netreg *netreg,
+				ofono_netreg_strength_cb_t cb, void *data);
+void at_netreg_remove(struct ofono_netreg *netreg);
-- 
2.20.1

[RFC at-export 2/2] ublox: network-registration atom

[Jonas Bonn]

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

For uBlox modems, a bit of custom setup is required, but after that the
generic "atmodem" (27.007-compatible) method implementations are
sufficient.  This driver, therefore, just puts the custom probe method
into place and defers remaining functionality to the recently exported
atmodem implementations.
---
 drivers/ubloxmodem/network-registration.c | 413 ++++++++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c           |   2 +
 drivers/ubloxmodem/ubloxmodem.h           |   3 +
 3 files changed, 418 insertions(+)
 create mode 100644 drivers/ubloxmodem/network-registration.c

diff --git a/drivers/ubloxmodem/network-registration.c b/drivers/ubloxmodem/network-registration.c
new file mode 100644
index 00000000..55adaff1
--- /dev/null
+++ b/drivers/ubloxmodem/network-registration.c
@@ -0,0 +1,413 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2010  ST-Ericsson AB.
+ *
+ *  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 <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/netreg.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "common.h"
+#include "ubloxmodem.h"
+#include "drivers/atmodem/vendor.h"
+
+#include "drivers/atmodem/network-registration.h"
+
+static const char *none_prefix[] = { NULL };
+static const char *cmer_prefix[] = { "+CMER:", NULL };
+static const char *ureg_prefix[] = { "+UREG:", NULL };
+
+
+/* This needs to be kept compatible with the version in
+ * atmodem/network-registration.  Add ublox-specific fields only to
+ * the end of the struct.
+ */
+struct netreg_data {
+	GAtChat *chat;
+	char mcc[OFONO_MAX_MCC_LENGTH + 1];
+	char mnc[OFONO_MAX_MNC_LENGTH + 1];
+	int signal_index; /* If strength is reported via CIND */
+	int signal_min; /* min strength reported via CIND */
+	int signal_max; /* max strength reported via CIND */
+	int signal_invalid; /* invalid strength reported via CIND */
+	int tech;
+	struct ofono_network_time time;
+	guint nitz_timeout;
+	unsigned int vendor;
+
+	const struct ublox_model *model;
+};
+
+struct tech_query {
+	int status;
+	int lac;
+	int ci;
+	struct ofono_netreg *netreg;
+};
+
+static void ciev_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+	int strength, ind;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CIEV:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &ind))
+		return;
+
+	if (ind != nd->signal_index)
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &strength))
+		return;
+
+	if (strength == nd->signal_invalid)
+		strength = -1;
+	else
+		strength = (strength * 100) / (nd->signal_max - nd->signal_min);
+
+	ofono_netreg_strength_notify(netreg, strength);
+}
+
+static gboolean notify_time(gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+
+	nd->nitz_timeout = 0;
+
+	ofono_netreg_time_notify(netreg, &nd->time);
+
+	return FALSE;
+}
+
+static void ctzdst_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+	int dst;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CTZDST:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &dst))
+		return;
+
+	DBG("dst %d", dst);
+
+	nd->time.dst = dst;
+
+	if (nd->nitz_timeout > 0) {
+		g_source_remove(nd->nitz_timeout);
+		nd->nitz_timeout = 0;
+	}
+
+	ofono_netreg_time_notify(netreg, &nd->time);
+}
+
+static void ctzv_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+	int year, mon, mday, hour, min, sec;
+	const char *tz, *time;
+	GAtResultIter iter;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CTZV:"))
+		return;
+
+	if (!g_at_result_iter_next_unquoted_string(&iter, &tz))
+		return;
+
+	if (!g_at_result_iter_next_string(&iter, &time))
+		return;
+
+	DBG("tz %s time %s", tz, time);
+
+	if (sscanf(time, "%u/%u/%u,%u:%u:%u", &year, &mon, &mday,
+						&hour, &min, &sec) != 6)
+		return;
+
+	nd->time.sec = sec;
+	nd->time.min = min;
+	nd->time.hour = hour;
+	nd->time.mday = mday;
+	nd->time.mon = mon;
+	nd->time.year = 2000 + year;
+
+	nd->time.utcoff = atoi(tz) * 15 * 60;
+
+	/* Delay notification in case there's a DST update coming */
+	if (nd->nitz_timeout > 0)
+		g_source_remove(nd->nitz_timeout);
+
+	nd->nitz_timeout = g_timeout_add_seconds(1, notify_time, user_data);
+}
+
+static void ublox_query_tech_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct tech_query *tq = user_data;
+	GAtResultIter iter;
+	gint enabled, state;
+	int tech = -1;
+
+	if (!ok)
+		goto error;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+UREG:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &enabled))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &state))
+		return;
+
+	switch (state) {
+	case 4:
+		tech = 5;
+		break;
+	case 5:
+		tech = 4;
+		break;
+	case 8:
+		tech = 1;
+		break;
+	case 9:
+		tech = 2;
+		break;
+	default:
+		tech = state;
+	}
+
+error:
+	ofono_netreg_status_notify(tq->netreg,
+			tq->status, tq->lac, tq->ci, tech);
+}
+
+static void creg_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	int status, lac, ci, tech;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+	struct tech_query *tq;
+
+	if (at_util_parse_reg_unsolicited(result, "+CREG:", &status,
+				&lac, &ci, &tech, nd->vendor) == FALSE)
+		return;
+
+	if (status != 1 && status != 5)
+		goto notify;
+
+	if (ublox_is_toby_l4(nd->model)) {
+		tq = g_new0(struct tech_query, 1);
+
+		tq->status = status;
+		tq->lac = lac;
+		tq->ci = ci;
+		tq->netreg = netreg;
+
+		if (g_at_chat_send(nd->chat, "AT+UREG?", ureg_prefix,
+				ublox_query_tech_cb, tq, g_free) > 0)
+			return;
+
+		g_free(tq);
+	}
+
+	if ((status == 1 || status == 5) && tech == -1)
+		tech = nd->tech;
+
+notify:
+	ofono_netreg_status_notify(netreg, status, lac, ci, tech);
+}
+
+static void at_cmer_not_supported(struct ofono_netreg *netreg)
+{
+	ofono_error("+CMER not supported by this modem.  If this is an error"
+			" please submit patches to support this hardware");
+
+	ofono_netreg_remove(netreg);
+}
+
+static void ublox_cmer_set_cb(gboolean ok,
+				GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+
+	if (!ok) {
+		at_cmer_not_supported(netreg);
+		return;
+	}
+
+	g_at_chat_register(nd->chat, "+CIEV:",
+			ciev_notify, FALSE, netreg, NULL);
+
+	g_at_chat_register(nd->chat, "+CREG:",
+				creg_notify, FALSE, netreg, NULL);
+
+	ofono_netreg_register(netreg);
+}
+
+static void ublox_creg_set_cb(gboolean ok,
+				GAtResult *result, gpointer user_data)
+{
+	struct ofono_netreg *netreg = user_data;
+	struct netreg_data *nd = ofono_netreg_get_data(netreg);
+
+	if (!ok) {
+		ofono_error("Unable to initialize Network Registration");
+		ofono_netreg_remove(netreg);
+		return;
+	}
+
+#if 0
+	if (ublox_is_toby_l4(nd->model)) {
+		/* FIXME */
+		ofono_error("TOBY L4 requires polling of ECSQ");
+		ofono_error("TOBY L4 wants UREQ notifications for tech updates");
+	}
+#endif
+
+	/* Register for network time update reports */
+	if (!ublox_is_toby_l2(nd->model)) {
+#if 0
+		/* TOBY L2 does not support CTZDST */
+		g_at_chat_register(nd->chat, "+CTZE:", ctze_notify,
+						FALSE, netreg, NULL);
+		g_at_chat_send(nd->chat, "AT+CTZR=2", none_prefix,
+						NULL, NULL, NULL);
+#endif
+	} else {
+		g_at_chat_register(nd->chat, "+CTZV:", ctzv_notify,
+						FALSE, netreg, NULL);
+		g_at_chat_register(nd->chat, "+CTZDST:", ctzdst_notify,
+						FALSE, netreg, NULL);
+		g_at_chat_send(nd->chat, "AT+CTZR=1", none_prefix,
+						NULL, NULL, NULL);
+	}
+#if 0
+	/* FIXME: relevant for L4... what about others? */
+	if (ublox_is_toby_l4(netreg->model)) {
+		/* The TOBY L4 does not support RAT notifications via CREG.
+		 * We need to use +UREG instead
+		 */
+		g_at_chat_register(gd->chat, "+UREG:", ublox_ureg_notify,
+						FALSE, netreg, NULL);
+		g_at_chat_send(gd->chat, "AT+UREG=1", none_prefix,
+						NULL, NULL, NULL);
+	}
+#endif
+
+	/* AT+CMER NOTES:
+	 * - For all u-blox models, mode 3 is equivalent to mode 1;
+	 * since some models do not support setting modes 2 nor 3
+	 * (see UBX-13002752), we prefer mode 1 for all models.
+	 * - The TOBY L4 does not support ind=2
+	 */
+	g_at_chat_send(nd->chat, "AT+CMER=1,0,0,1", cmer_prefix,
+			ublox_cmer_set_cb, netreg, NULL);
+}
+
+/*
+ * uBlox netreg atom probe.
+ * - takes uBlox model ID parameter instead of AT vendor ID
+ */
+static int ublox_netreg_probe(struct ofono_netreg *netreg,
+				unsigned int model_id,
+				void *data)
+{
+	GAtChat *chat = data;
+	struct netreg_data *nd;
+
+	nd = g_new0(struct netreg_data, 1);
+
+	nd->model = ublox_model_from_id(model_id);
+
+	/* There should be no uBlox-specific quirks in the 'generic'
+	 * AT driver
+	 */
+	nd->vendor = OFONO_VENDOR_GENERIC;
+
+	nd->chat = g_at_chat_clone(chat);
+	nd->tech = -1;
+	nd->time.sec = -1;
+	nd->time.min = -1;
+	nd->time.hour = -1;
+	nd->time.mday = -1;
+	nd->time.mon = -1;
+	nd->time.year = -1;
+	nd->time.dst = 0;
+	nd->time.utcoff = 0;
+	ofono_netreg_set_data(netreg, nd);
+
+	/* All uBlox devices support n=2 so no need to query this */
+	g_at_chat_send(nd->chat, "AT+CREG=2", none_prefix,
+			ublox_creg_set_cb, netreg, NULL);
+
+	return 0;
+}
+
+static const struct ofono_netreg_driver driver = {
+	.name				= "ubloxmodem",
+	.probe				= ublox_netreg_probe,
+	.remove				= at_netreg_remove,
+	.registration_status		= at_registration_status,
+	.current_operator		= at_current_operator,
+	.list_operators			= at_list_operators,
+	.register_auto			= at_register_auto,
+	.register_manual		= at_register_manual,
+	.strength			= at_signal_strength,
+};
+
+void ublox_netreg_init(void)
+{
+	ofono_netreg_driver_register(&driver);
+}
+
+void ublox_netreg_exit(void)
+{
+	ofono_netreg_driver_unregister(&driver);
+}
diff --git a/drivers/ubloxmodem/ubloxmodem.c b/drivers/ubloxmodem/ubloxmodem.c
index a52a67ea..719c77a0 100644
--- a/drivers/ubloxmodem/ubloxmodem.c
+++ b/drivers/ubloxmodem/ubloxmodem.c
@@ -115,6 +115,7 @@ int ublox_is_toby_l4(const struct ublox_model *model)
 static int ubloxmodem_init(void)
 {
 	ublox_gprs_context_init();
+	ublox_netreg_init();
 	ublox_netmon_init();
 	ublox_lte_init();
 
@@ -124,6 +125,7 @@ static int ubloxmodem_init(void)
 static void ubloxmodem_exit(void)
 {
 	ublox_gprs_context_exit();
+	ublox_netreg_exit();
 	ublox_netmon_exit();
 	ublox_lte_exit();
 }
diff --git a/drivers/ubloxmodem/ubloxmodem.h b/drivers/ubloxmodem/ubloxmodem.h
index 2c5b7433..81fe9481 100644
--- a/drivers/ubloxmodem/ubloxmodem.h
+++ b/drivers/ubloxmodem/ubloxmodem.h
@@ -43,6 +43,9 @@ int ublox_is_toby_l4(const struct ublox_model *model);
 extern void ublox_gprs_context_init(void);
 extern void ublox_gprs_context_exit(void);
 
+void ublox_netreg_init(void);
+void ublox_netreg_exit(void);
+
 extern void ublox_netmon_init(void);
 extern void ublox_netmon_exit(void);
 
-- 
2.20.1

Re: [RFC at-export 0/2] Export AT-modem functions to other drivers

[Denis Kenzior]

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

Hi Jonas,

On 04/22/2019 02:44 AM, Jonas Bonn wrote:
> Hi Denis,
> 
> I'm writing a custom network-registration driver for uBlox modems and
> I'd like to know if you will choke on the following approach:
> 
> i)  export method implementations from atmodem driver

This idea is really not new.  Every time I tried it the result was worse 
than maintaining a single netreg driver with the vendor logic.  netreg 
is by far the worst case scenario for this since 3GPP failed to 
standardize these commands (and voice management) until late in the game.

> ii) set up uBlox netreg vtable with custom probe method and exported
> atmodem implementations for rest

Except I hate your 'keep netreg data in sync' idea.  Not happening.  Use 
l_container_of instead and use composition by value for your driver 
specific structure.

> 
> If you can't accept this approach, please let me know of an alternative
> method that would be preferable that (hopefully) doesn't entail just
> copying all the code from one driver to the other (1000+ lines to keep
> in sync going forward).

What is actually different for uBlox that requires such a drastic 
approach?  Right now I'm seeing a duplication of CMER/CIEV and NITZ 
logic.  Not much else.

Regards,
-Denis

Re: [RFC at-export 0/2] Export AT-modem functions to other drivers

[Jonas Bonn]

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

Hi Denis,

On 22/04/2019 17:52, Denis Kenzior wrote:

> What is actually different for uBlox that requires such a drastic 
> approach?  Right now I'm seeing a duplication of CMER/CIEV and NITZ 
> logic.  Not much else.

For the TOBY L4 I hit this:
ofono_error("This driver is not setup with Signal Strength reporting"
             " via CIND indications, please write proper netreg"
             " handling for this device");

This device requires _polling_ ECSQ to get this info (no async signal 
strenght reporting).  Other uBlox models don't have this issue.

So we need to have UBLOX_TOBY_L4, UBLOX_TOBY_L2, etc. "vendor" types to 
differentiate not only the vendor but the model.  Is that ok?  Not 
enough to warrant a custom driver?

Is NITZ really a thing?  I interpreted it as "No immediate timezone"...

/Jonas

Re: [RFC at-export 0/2] Export AT-modem functions to other drivers

[Denis Kenzior]

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

Hi Jonas,

On 04/22/2019 12:33 PM, Jonas Bonn wrote:
> Hi Denis,
> 
> On 22/04/2019 17:52, Denis Kenzior wrote:
> 
>> What is actually different for uBlox that requires such a drastic 
>> approach?  Right now I'm seeing a duplication of CMER/CIEV and NITZ 
>> logic.  Not much else.
> 
> For the TOBY L4 I hit this:
> ofono_error("This driver is not setup with Signal Strength reporting"
>              " via CIND indications, please write proper netreg"
>              " handling for this device");
> 
> This device requires _polling_ ECSQ to get this info (no async signal 
> strenght reporting).  Other uBlox models don't have this issue.

You have to be kidding...  Sigh.

> 
> So we need to have UBLOX_TOBY_L4, UBLOX_TOBY_L2, etc. "vendor" types to 
> differentiate not only the vendor but the model.  Is that ok?  Not 
> enough to warrant a custom driver?

We used to have UBLOX_L2 prior to 
e70789534efc62d31112f54e42c0d249bed93b37.  So I don't really mind.  If 
you can do the l_container_of polymorphism going nicely, that's fine 
with me as well.

> 
> Is NITZ really a thing?  I interpreted it as "No immediate timezone"...

NITZ stands for: Network Identity & Time Zone.  And yes, it is a 'thing' 
and part of the 3GPP standards for at least 15 years.

> 
> /Jonas

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Hi,

Den mån 22 apr. 2019 kl 09:44 skrev Jonas Bonn <jonas@norrbonn.se>:
> +
> +static void creg_notify(GAtResult *result, gpointer user_data)
> +{
> +       struct ofono_netreg *netreg = user_data;
> +       int status, lac, ci, tech;
> +       struct netreg_data *nd = ofono_netreg_get_data(netreg);
> +       struct tech_query *tq;
> +
> +       if (at_util_parse_reg_unsolicited(result, "+CREG:", &status,
> +                               &lac, &ci, &tech, nd->vendor) == FALSE)
> +               return;
> +
> +       if (status != 1 && status != 5)
> +               goto notify;

Just wants to check your thoughts about CREG and LTE only...
If the modem only has 4g coverage it might set the status to 4 (unknown),
but ureg would indicate LTE(7).
I have done a local patch for this in gprs, do not go to de-attached
state in that case.

Just wondering if we should combine the CREG and UREG status
in some way, the problem is that UREG is not indicating roaming or not...

--Richard

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Richard,

On 05/04/2019 02:06 PM, Richard Röjfors wrote:
> Hi,
> 
> Den mån 22 apr. 2019 kl 09:44 skrev Jonas Bonn <jonas@norrbonn.se>:
>> +
>> +static void creg_notify(GAtResult *result, gpointer user_data)
>> +{
>> +       struct ofono_netreg *netreg = user_data;
>> +       int status, lac, ci, tech;
>> +       struct netreg_data *nd = ofono_netreg_get_data(netreg);
>> +       struct tech_query *tq;
>> +
>> +       if (at_util_parse_reg_unsolicited(result, "+CREG:", &status,
>> +                               &lac, &ci, &tech, nd->vendor) == FALSE)
>> +               return;
>> +
>> +       if (status != 1 && status != 5)
>> +               goto notify;
> 
> Just wants to check your thoughts about CREG and LTE only...
> If the modem only has 4g coverage it might set the status to 4 (unknown),
> but ureg would indicate LTE(7).
> I have done a local patch for this in gprs, do not go to de-attached
> state in that case.

It seems a bunch of devices have this problem.  By my reading of 27.007, 
CREG should not report status 4 unless there is no coverage whatsoever, 
e.g.:

"4	unknown (e.g. out of GERAN/UTRAN/E-UTRAN coverage)"

> 
> Just wondering if we should combine the CREG and UREG status
> in some way, the problem is that UREG is not indicating roaming or not...

So that sounds like a bug in the firmware.  Does ublox support +CEREG?

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Den ons 8 maj 2019 kl 16:37 skrev Denis Kenzior <denkenz@gmail.com>:
>
> Hi Richard,
> >
> > Just wants to check your thoughts about CREG and LTE only...
> > If the modem only has 4g coverage it might set the status to 4 (unknown),
> > but ureg would indicate LTE(7).
> > I have done a local patch for this in gprs, do not go to de-attached
> > state in that case.
>
> It seems a bunch of devices have this problem.  By my reading of 27.007,
> CREG should not report status 4 unless there is no coverage whatsoever,
> e.g.:
>
> "4      unknown (e.g. out of GERAN/UTRAN/E-UTRAN coverage)"
>
> >
> > Just wondering if we should combine the CREG and UREG status
> > in some way, the problem is that UREG is not indicating roaming or not...
>
> So that sounds like a bug in the firmware.  Does ublox support +CEREG?

Yes they support CEREG according to the AT spec. I have not looked
into the difference, do you have an executive, summary? ;)

It might be a firmware issue. I have seen multiple cases where it
reports unknown but UREG = 7... AND the LTE context still works.

--Richard

Re: [RFC at-export 2/2] ublox: network-registration atom

[Philippe De Swert]

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

Hi,

On 09/05/2019, Richard Röjfors <richard.rojfors@gmail.com> wrote:
> Den ons 8 maj 2019 kl 16:37 skrev Denis Kenzior <denkenz@gmail.com>:
>>
>> Hi Richard,
>> >
>> > Just wants to check your thoughts about CREG and LTE only...
>> > If the modem only has 4g coverage it might set the status to 4
>> > (unknown),
>> > but ureg would indicate LTE(7).
>> > I have done a local patch for this in gprs, do not go to de-attached
>> > state in that case.

Yes we ran into the same problem.

>> It seems a bunch of devices have this problem.  By my reading of 27.007,
>> CREG should not report status 4 unless there is no coverage whatsoever,
>> e.g.:
>>
>> "4      unknown (e.g. out of GERAN/UTRAN/E-UTRAN coverage)"
>>
>> >
>> > Just wondering if we should combine the CREG and UREG status
>> > in some way, the problem is that UREG is not indicating roaming or
>> > not...
>>
>> So that sounds like a bug in the firmware.  Does ublox support +CEREG?
>
> Yes they support CEREG according to the AT spec. I have not looked
> into the difference, do you have an executive, summary? ;)

We got it fixed through CEREG. This however has been a
long-standing/recurring issue
A few mails and threads in reference:

https://lists.ofono.org/pipermail/ofono/2018-September/018514.html
https://lists.ofono.org/pipermail/ofono/2019-February/019134.html
https://lists.ofono.org/pipermail/ofono/2019-February/019136.html

I'd like to see it fixed, and I have been spending some time on things
the last few days. but I have not understood yet where exactly this
now needs to be added and how.

> It might be a firmware issue. I have seen multiple cases where it
> reports unknown but UREG = 7... AND the LTE context still works.

We spotted the same thing.

Regards,

Philippe

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Richard,

<snip>

>>> Just wondering if we should combine the CREG and UREG status >>> in some way, the problem is that UREG is not indicating roaming or 
not...>>>> So that sounds like a bug in the firmware.  Does ublox 
support +CEREG?> > Yes they support CEREG according to the AT spec. I 
have not looked> into the difference, do you have an executive, summary? ;)
For whatever reason 3GPP decided to separate each technology status into 
a separate command.  So sort of similar to +CGREG for GPRS, they added 
CEREG for E-EUTRAN, and there's a 5G version, etc.  I think the intent 
of the spec was to keep CREG backwards compatible and applicable to all 
techs, but somehow the modem firmware guys got this wrong.

> 
> It might be a firmware issue. I have seen multiple cases where it
> reports unknown but UREG = 7... AND the LTE context still works.
> 

Ah wonderful.  I'd complain to the vendor.

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Philippe,

<snip>

> https://lists.ofono.org/pipermail/ofono/2018-September/018514.html
> https://lists.ofono.org/pipermail/ofono/2019-February/019134.html
> https://lists.ofono.org/pipermail/ofono/2019-February/019136.html

Yes, the thread from 2018 is a good summary.

> 
> I'd like to see it fixed, and I have been spending some time on things
> the last few days. but I have not understood yet where exactly this
> now needs to be added and how.

Essentially the netreg atom driver needs to query both CEREG and CREG, 
combine the results to give the final answer to the core.

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Hi,

I got this case again.

Killed ofono and checked the statuses manually. The output is below.
Its clear that creg and cereg both reports roaming, while cgreg reports unknown.

Denis if netreg would be updated, I still think we have an issue with
atmodem/gprs.c.
The problematic part is when at modem received the cgreg and calls
ofono_gprs_status_notify, the gprs core part will then detach.

The log when this happened:

May 13 05:33:37  ofonod[741]: Aux: < \r\n+CREG: 5,"0080","00000006",6\r\n
May 13 05:33:37  ofonod[741]:
../ofono-1.29/src/network.c:ofono_netreg_status_notify() /ublox_0
status 5 tech 6
May 13 05:33:37  ofonod[741]: ../ofono-1.29/src/gprs.c:netreg_status_changed() 5
May 13 05:33:37  ofonod[741]:
../ofono-1.29/src/gprs.c:gprs_netreg_update() attach: 1,
driver_attached: 1
May 13 05:33:37  ofonod[741]: Aux: > AT\r
May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
May 13 05:33:37  ofonod[741]: Aux: Finally woke up the modem
May 13 05:33:37  ofonod[741]: Aux: > AT+COPS=3,2\r
May 13 05:33:37  ofonod[741]: Aux: < \r\n+CIEV: 2,5\r\n
May 13 05:33:37  ofonod[741]:
../ofono-1.29/src/network.c:ofono_netreg_strength_notify() strength
100
May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
May 13 05:33:37  ofonod[741]: Aux: > AT+COPS?\r
May 13 05:33:37  ofonod[741]: Aux: < \r\n+COPS: 0,2,"24007",6\r\n
May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
May 13 05:33:37  ofonod[741]:
../ofono-1.29/drivers/atmodem/network-registration.c:cops_numeric_cb()
Cops numeric got mcc: 240, mnc: 07
May 13 05:33:37  ofonod[741]: Aux: > AT+CIND?\r
May 13 05:33:37  ofonod[741]: Aux: < \r\n+CIND: 5,5,0,0,0,0,1,0,2,0,0,1\r\n
May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
May 13 05:33:37  ofonod[741]: Aux: > AT+COPS=3,0\r
May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
May 13 05:33:37  ofonod[741]: Aux: > AT+COPS?\r
May 13 05:33:37  ofonod[741]: Aux: < \r\n+COPS: 0,0,"Tele2",6\r\n
May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
May 13 05:33:37  ofonod[741]:
../ofono-1.29/drivers/atmodem/network-registration.c:cops_cb()
cops_cb: Tele2, 240 07 6
May 13 05:33:37  ofonod[741]:
../ofono-1.29/src/network.c:current_operator_callback() 0x1fd2980,
0x1fe2c88
May 13 05:33:37  ofonod[741]: Aux: < \r\n+CGREG: 4\r\n
May 13 05:33:37  ofonod[741]:
../ofono-1.29/src/gprs.c:ofono_gprs_status_notify() /ublox_0 status
unknown (4)
May 13 05:33:37  ofonod[741]:
../ofono-1.29/src/gprs.c:gprs_set_attached_property() 1 -> 0

Private log above, we got dettached, and will not reattach.

May 13 05:33:38  ofonod[741]: Aux: < \r\n+UREG: 0\r\n\r\n+CIEV:
9,1\r\n\r\n+UREG: 7\r\n\r\n+CIEV: 9,2\r\n


I killed ofono and checked the *reg manually:

at+creg?
+CREG: 2,5,"2AFF","01B19A04",7

OK
at+cgreg?
+CGREG: 2,4

OK
at+cereg?
+CEREG: 0,5

OK

Den tors 9 maj 2019 kl 16:35 skrev Denis Kenzior <denkenz@gmail.com>:
>
> Hi Philippe,
>
> <snip>
>
> > https://lists.ofono.org/pipermail/ofono/2018-September/018514.html
> > https://lists.ofono.org/pipermail/ofono/2019-February/019134.html
> > https://lists.ofono.org/pipermail/ofono/2019-February/019136.html
>
> Yes, the thread from 2018 is a good summary.
>
> >
> > I'd like to see it fixed, and I have been spending some time on things
> > the last few days. but I have not understood yet where exactly this
> > now needs to be added and how.
>
> Essentially the netreg atom driver needs to query both CEREG and CREG,
> combine the results to give the final answer to the core.
>
> Regards,
> -Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Richard,

On 05/13/2019 01:20 AM, Richard Röjfors wrote:
> Hi,
> 
> I got this case again.
> 
> Killed ofono and checked the statuses manually. The output is below.
> Its clear that creg and cereg both reports roaming, while cgreg reports unknown.

This is a different case?  Your originally case had CREG reporting 
unknown while UREG reporting LTE.

> 
> Denis if netreg would be updated, I still think we have an issue with
> atmodem/gprs.c.
> The problematic part is when at modem received the cgreg and calls
> ofono_gprs_status_notify, the gprs core part will then detach.
> 
> The log when this happened:
> 
> May 13 05:33:37  ofonod[741]: Aux: < \r\n+CREG: 5,"0080","00000006",6\r\n

So we are roaming...

> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/src/network.c:ofono_netreg_status_notify() /ublox_0
> status 5 tech 6
> May 13 05:33:37  ofonod[741]: ../ofono-1.29/src/gprs.c:netreg_status_changed() 5
> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/src/gprs.c:gprs_netreg_update() attach: 1,
> driver_attached: 1
> May 13 05:33:37  ofonod[741]: Aux: > AT\r
> May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> May 13 05:33:37  ofonod[741]: Aux: Finally woke up the modem

Weird that you're using this magic.  I thought only the Freerunner modem 
had this issue.  But ok...

> May 13 05:33:37  ofonod[741]: Aux: > AT+COPS=3,2\r
> May 13 05:33:37  ofonod[741]: Aux: < \r\n+CIEV: 2,5\r\n
> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/src/network.c:ofono_netreg_strength_notify() strength
> 100
> May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> May 13 05:33:37  ofonod[741]: Aux: > AT+COPS?\r
> May 13 05:33:37  ofonod[741]: Aux: < \r\n+COPS: 0,2,"24007",6\r\n
> May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/drivers/atmodem/network-registration.c:cops_numeric_cb()
> Cops numeric got mcc: 240, mnc: 07
> May 13 05:33:37  ofonod[741]: Aux: > AT+CIND?\r
> May 13 05:33:37  ofonod[741]: Aux: < \r\n+CIND: 5,5,0,0,0,0,1,0,2,0,0,1\r\n
> May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> May 13 05:33:37  ofonod[741]: Aux: > AT+COPS=3,0\r
> May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> May 13 05:33:37  ofonod[741]: Aux: > AT+COPS?\r
> May 13 05:33:37  ofonod[741]: Aux: < \r\n+COPS: 0,0,"Tele2",6\r\n
> May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/drivers/atmodem/network-registration.c:cops_cb()
> cops_cb: Tele2, 240 07 6
> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/src/network.c:current_operator_callback() 0x1fd2980,
> 0x1fe2c88
> May 13 05:33:37  ofonod[741]: Aux: < \r\n+CGREG: 4\r\n

So for whatever reason the modem (or network?) decides to detach GPRS...

> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/src/gprs.c:ofono_gprs_status_notify() /ublox_0 status
> unknown (4)
> May 13 05:33:37  ofonod[741]:
> ../ofono-1.29/src/gprs.c:gprs_set_attached_property() 1 -> 0
> 

My memory is fuzzy now, but I'm not sure if we ever put in the logic to 
attempt a re-attach in such a situation (e.g. we didn't request a 
detach).  For all devices we cared about the modem re-attached for us. 
It looks like there's still a TODO for this in src/gprs.c

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Hi Denis,

Den mån 13 maj 2019 kl 23:57 skrev Denis Kenzior <denkenz@gmail.com>:
>
> Hi Richard,
>
> On 05/13/2019 01:20 AM, Richard Röjfors wrote:
> > Hi,
> >
> > I got this case again.
> >
> > Killed ofono and checked the statuses manually. The output is below.
> > Its clear that creg and cereg both reports roaming, while cgreg reports unknown.
>
> This is a different case?  Your originally case had CREG reporting
> unknown while UREG reporting LTE.

I really hope I didn't confuse CREG and CGREG.

>
> >
> > Denis if netreg would be updated, I still think we have an issue with
> > atmodem/gprs.c.
> > The problematic part is when at modem received the cgreg and calls
> > ofono_gprs_status_notify, the gprs core part will then detach.
> >
> > The log when this happened:
> >
> > May 13 05:33:37  ofonod[741]: Aux: < \r\n+CREG: 5,"0080","00000006",6\r\n
>
> So we are roaming...
>
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/src/network.c:ofono_netreg_status_notify() /ublox_0
> > status 5 tech 6
> > May 13 05:33:37  ofonod[741]: ../ofono-1.29/src/gprs.c:netreg_status_changed() 5
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/src/gprs.c:gprs_netreg_update() attach: 1,
> > driver_attached: 1
> > May 13 05:33:37  ofonod[741]: Aux: > AT\r
> > May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> > May 13 05:33:37  ofonod[741]: Aux: Finally woke up the modem
>
> Weird that you're using this magic.  I thought only the Freerunner modem
> had this issue.  But ok...

Not sure we need to ;-) It seems to me like gatchat always does this
is the command list is empty, putting a single AT first..I didn't know
it was "optional".

I have seen the ublox to lock up and not reply to AT commands, but
that was after the wake anyway so that did not help.

>
> > May 13 05:33:37  ofonod[741]: Aux: > AT+COPS=3,2\r
> > May 13 05:33:37  ofonod[741]: Aux: < \r\n+CIEV: 2,5\r\n
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/src/network.c:ofono_netreg_strength_notify() strength
> > 100
> > May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> > May 13 05:33:37  ofonod[741]: Aux: > AT+COPS?\r
> > May 13 05:33:37  ofonod[741]: Aux: < \r\n+COPS: 0,2,"24007",6\r\n
> > May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/drivers/atmodem/network-registration.c:cops_numeric_cb()
> > Cops numeric got mcc: 240, mnc: 07
> > May 13 05:33:37  ofonod[741]: Aux: > AT+CIND?\r
> > May 13 05:33:37  ofonod[741]: Aux: < \r\n+CIND: 5,5,0,0,0,0,1,0,2,0,0,1\r\n
> > May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> > May 13 05:33:37  ofonod[741]: Aux: > AT+COPS=3,0\r
> > May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> > May 13 05:33:37  ofonod[741]: Aux: > AT+COPS?\r
> > May 13 05:33:37  ofonod[741]: Aux: < \r\n+COPS: 0,0,"Tele2",6\r\n
> > May 13 05:33:37  ofonod[741]: Aux: < \r\nOK\r\n
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/drivers/atmodem/network-registration.c:cops_cb()
> > cops_cb: Tele2, 240 07 6
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/src/network.c:current_operator_callback() 0x1fd2980,
> > 0x1fe2c88
> > May 13 05:33:37  ofonod[741]: Aux: < \r\n+CGREG: 4\r\n
>
> So for whatever reason the modem (or network?) decides to detach GPRS...
>
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/src/gprs.c:ofono_gprs_status_notify() /ublox_0 status
> > unknown (4)
> > May 13 05:33:37  ofonod[741]:
> > ../ofono-1.29/src/gprs.c:gprs_set_attached_property() 1 -> 0
> >
>
> My memory is fuzzy now, but I'm not sure if we ever put in the logic to
> attempt a re-attach in such a situation (e.g. we didn't request a
> detach).  For all devices we cared about the modem re-attached for us.
> It looks like there's still a TODO for this in src/gprs.c

I guess the thing is that the modem has switched to 4G. Do we actually
need to care about re-attaching to gprs? I mean the modem
automatically hand over to 4G and the context continues to work.
Wouldn't it make sense if gprs kept the "combined packet status"?
(combine CGREG, CEREG and whatever for 5G).
The name GPRS would be confusing but it would keep the complete packet
switched status in one place.
In this case that could would not consider us being roaming instead of
unknown, since that whats CEREG reports.

--Richard

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Richard,

> Not sure we need to ;-) It seems to me like gatchat always does this
> is the command list is empty, putting a single AT first..I didn't know
> it was "optional".

It is enabled only if you use g_at_chat_set_wakeup_command.  Looks like 
commit ae74d4ccd added this.  I no longer remember the details why.  It 
is a bit of a waste to keep this on after the modem is active though. 
The freerunner had an overly aggressive sleep mode where the TTY would 
sleep after 5 seconds of inactivity and eat the first command sent to it.

>> My memory is fuzzy now, but I'm not sure if we ever put in the logic to
>> attempt a re-attach in such a situation (e.g. we didn't request a
>> detach).  For all devices we cared about the modem re-attached for us.
>> It looks like there's still a TODO for this in src/gprs.c
> 
> I guess the thing is that the modem has switched to 4G. Do we actually
> need to care about re-attaching to gprs? I mean the modem

Ah I think I see now....

Btw, what does 4G mean? That is a marketing term, please use the 
technical terms ;)

So you mean it switched to LTE/EUTRAN and GPRS state went to state 
'unknown'.  On EUTRAN we are always attached, so, no, we do not need to 
reattach.

> automatically hand over to 4G and the context continues to work.
> Wouldn't it make sense if gprs kept the "combined packet status"?
> (combine CGREG, CEREG and whatever for 5G).

Yes, it should.

> The name GPRS would be confusing but it would keep the complete packet
> switched status in one place.
> In this case that could would not consider us being roaming instead of
> unknown, since that whats CEREG reports.

So I think there's a bug where we erroneously set the Attached property 
to False in this situation.  It doesn't look like we trigger an actual 
detach, but Attached should remain 'True' while CREG shows that we're on 
EUTRAN or any related tech.  Perhaps the logic in gprs_attached_update() 
needs to be updated.

I do agree that gprs is no longer an optimal name, but then this project 
got started before LTE existed.

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Hi Denis,

Den tis 14 maj 2019 kl 21:44 skrev Denis Kenzior <denkenz@gmail.com>:
>
> Hi Richard,
>
> > Not sure we need to ;-) It seems to me like gatchat always does this
> > is the command list is empty, putting a single AT first..I didn't know
> > it was "optional".
>
> It is enabled only if you use g_at_chat_set_wakeup_command.  Looks like
> commit ae74d4ccd added this.  I no longer remember the details why.  It
> is a bit of a waste to keep this on after the modem is active though.
> The freerunner had an overly aggressive sleep mode where the TTY would
> sleep after 5 seconds of inactivity and eat the first command sent to it.
>
> >> My memory is fuzzy now, but I'm not sure if we ever put in the logic to
> >> attempt a re-attach in such a situation (e.g. we didn't request a
> >> detach).  For all devices we cared about the modem re-attached for us.
> >> It looks like there's still a TODO for this in src/gprs.c
> >
> > I guess the thing is that the modem has switched to 4G. Do we actually
> > need to care about re-attaching to gprs? I mean the modem
>
> Ah I think I see now....
>
> Btw, what does 4G mean? That is a marketing term, please use the
> technical terms ;)
>

Sorry about this :) I guess I mean LTE/EUTRAN

> So you mean it switched to LTE/EUTRAN and GPRS state went to state
> 'unknown'.  On EUTRAN we are always attached, so, no, we do not need to
> reattach.
>
> > automatically hand over to 4G and the context continues to work.
> > Wouldn't it make sense if gprs kept the "combined packet status"?
> > (combine CGREG, CEREG and whatever for 5G).
>
> Yes, it should.

So should we update gprs.c to hande c*reg?

>
> > The name GPRS would be confusing but it would keep the complete packet
> > switched status in one place.
> > In this case that could would not consider us being roaming instead of
> > unknown, since that whats CEREG reports.
>
> So I think there's a bug where we erroneously set the Attached property
> to False in this situation.  It doesn't look like we trigger an actual
> detach, but Attached should remain 'True' while CREG shows that we're on
> EUTRAN or any related tech.  Perhaps the logic in gprs_attached_update()
> needs to be updated.

We need to consider the "tricky" part that we consider us attached
first when the
 context is activated in EUTRAN. And we might be in a transition
(attaching flag).

I'm thinking if the "combined" state in gprs wouldn't be the least
intrusive update for this.

I think I saw a patch for using c*reg in atmodem/gprs.c, can we wake that up?
Otherwise I can put some energy into this, if we decide which
direction to go first.

--Richard

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Richard,

>> So I think there's a bug where we erroneously set the Attached property
>> to False in this situation.  It doesn't look like we trigger an actual
>> detach, but Attached should remain 'True' while CREG shows that we're on
>> EUTRAN or any related tech.  Perhaps the logic in gprs_attached_update()
>> needs to be updated.
> 
> We need to consider the "tricky" part that we consider us attached
> first when the
>   context is activated in EUTRAN. And we might be in a transition
> (attaching flag).

Which is why the whole 'separate context state into a bazillion 
commands' is just inherently a bad idea on 3GPP's part.  It makes it 
hard to figure out wtf just happened in an atomic way.

I think we already have some magic where we only signal Attached once 
the default bearer settings are read, e.g. after .read_settings 
completes successfully.  The second part will be tricky...

> 
> I'm thinking if the "combined" state in gprs wouldn't be the least
> intrusive update for this.
> 
> I think I saw a patch for using c*reg in atmodem/gprs.c, can we wake that up?
> Otherwise I can put some energy into this, if we decide which
> direction to go first.
> 

Since 3GPP decided to go with EREG being separate from CGREG, I think it 
makes sense to support that natively in lte.c or gprs.c.  Our philosophy 
has always been to base the driver APIs off of 27.007.  So from that 
perspective, introducing ofono_lte_status_notify would make sense. 
Whether something like this makes things easier or not is a different 
question :)

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Giacinto Cifelli]

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

Hi Denis,

On Thu, May 16, 2019 at 6:01 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Richard,
>
> >> So I think there's a bug where we erroneously set the Attached property
> >> to False in this situation.  It doesn't look like we trigger an actual
> >> detach, but Attached should remain 'True' while CREG shows that we're on
> >> EUTRAN or any related tech.  Perhaps the logic in gprs_attached_update()
> >> needs to be updated.
> >
> > We need to consider the "tricky" part that we consider us attached
> > first when the
> >   context is activated in EUTRAN. And we might be in a transition
> > (attaching flag).
>
> Which is why the whole 'separate context state into a bazillion
> commands' is just inherently a bad idea on 3GPP's part.  It makes it
> hard to figure out wtf just happened in an atomic way.
>

I find it a bit pathetic blaming the 3GPP for the limitations of an
old design (I did not say obsolete).

3GPP stakeholders (mainly MNOs) wanted to uncouple the technologies,
to avoid the mega
spaghetti soup of GSM+GPRS+UTRAN, and also to allow to launch the
technologies without carrying the bag of the legacy.
It is the same with the NR, that has a C5GREG indicator.
Also the chipset vendors could take advantage of this, implementing
independent stacks, and so easier-to-maintain firmwares.
This is done on purpose, and the transaction period shall be handled
by flexible enough software.

> I think we already have some magic where we only signal Attached once
> the default bearer settings are read, e.g. after .read_settings
> completes successfully.  The second part will be tricky...
>
> >
> > I'm thinking if the "combined" state in gprs wouldn't be the least
> > intrusive update for this.
> >
> > I think I saw a patch for using c*reg in atmodem/gprs.c, can we wake that up?
> > Otherwise I can put some energy into this, if we decide which
> > direction to go first.
> >
>
> Since 3GPP decided to go with EREG being separate from CGREG, I think it
> makes sense to support that natively in lte.c or gprs.c.  Our philosophy
> has always been to base the driver APIs off of 27.007.  So from that
> perspective, introducing ofono_lte_status_notify would make sense.
> Whether something like this makes things easier or not is a different
> question :)

 I did propose something long time ago (in gprs.c), and was more or
less ok, except that the logic should
have been in the core atom, because it is reusable and definitely complex.

Then I didn't go further because anyway your small 80 chars screen
couldn't allow for AT+MBIM driver,
effectively cutting off most of Gemalto (now Thales) new generation of modules.
But the proposal and discussion is still in the history.

>
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

Regards,
Giacinto

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Giacinto,

> I find it a bit pathetic blaming the 3GPP for the limitations of an
> old design (I did not say obsolete).
> 
> 3GPP stakeholders (mainly MNOs) wanted to uncouple the technologies,
> to avoid the mega
> spaghetti soup of GSM+GPRS+UTRAN, and also to allow to launch the
> technologies without carrying the bag of the legacy.
> It is the same with the NR, that has a C5GREG indicator.
> Also the chipset vendors could take advantage of this, implementing
> independent stacks, and so easier-to-maintain firmwares.
> This is done on purpose, and the transaction period shall be handled
> by flexible enough software.

The simple fact is that a state change resulting in multiple events 
being emitted, with no defined order mandated by 3GPP is just bad API 
design.  You have status events coming at least 5 different sources 
(+CGEV, +CGREG, +CREG, +CEREG, +C5REG, etc), with some implying 
something on one firmware implementation vs not implying it on another. 
Add to this the fact that many firmware implementations out there don't 
bother sending +CGEV events properly and you have a mess for the upper 
layers.  But I do agree it makes writing the firmware easier ;)

>> Since 3GPP decided to go with EREG being separate from CGREG, I think it
>> makes sense to support that natively in lte.c or gprs.c.  Our philosophy
>> has always been to base the driver APIs off of 27.007.  So from that
>> perspective, introducing ofono_lte_status_notify would make sense.
>> Whether something like this makes things easier or not is a different
>> question :)
> 
>   I did propose something long time ago (in gprs.c), and was more or
> less ok, except that the logic should
> have been in the core atom, because it is reusable and definitely complex.
> 

And I pointed this last part out as part of my comments back then if I 
recall correctly.

> Then I didn't go further because anyway your small 80 chars screen
> couldn't allow for AT+MBIM driver,
> effectively cutting off most of Gemalto (now Thales) new generation of modules.
> But the proposal and discussion is still in the history.
> 

If you're not going to be offering anything constructive, please don't 
say anything.  Insults will not be tolerated on this list.    This is 
your only warning, we've never had to ban anyone before, but there's a 
first time for everything.

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Richard =?unknown-8bit?q?R=C3=B6jfors?=]

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

Hi Denis,

Den tors 16 maj 2019 kl 18:01 skrev Denis Kenzior <denkenz@gmail.com>:
> >
> > I think I saw a patch for using c*reg in atmodem/gprs.c, can we wake that up?
> > Otherwise I can put some energy into this, if we decide which
> > direction to go first.
> >
>
> Since 3GPP decided to go with EREG being separate from CGREG, I think it
> makes sense to support that natively in lte.c or gprs.c.  Our philosophy
> has always been to base the driver APIs off of 27.007.  So from that
> perspective, introducing ofono_lte_status_notify would make sense.
> Whether something like this makes things easier or not is a different
> question :)

...and we have the issue that the indications seems to be sent in random order.

So what about when a c?reg indication is received from the modem.
Start a "poll" of all the supported c?reg and set a flag, so multiple
polls are not started in case of an additional indication is received.
When the poll of all the supported c?reg statuses returns -> update
the gprs "core" with the combined status?

.. and the attached_status callback should also poll all supported c?reg...

BR
Richard

Re: [RFC at-export 2/2] ublox: network-registration atom

[Denis Kenzior]

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

Hi Richard,

> ...and we have the issue that the indications seems to be sent in random order.

Figured..

Try looking at the strategy we employed when dealing with the 27.007 
voice calling (another nightmare) commands.  E.g. +CLIP, +CNAP, +CDIP, 
+CRING/RING, +CCWA, +CLCC, etc.  It might possibly give you some ideas. 
The general strategy is to start a timer, but not perform any queries 
right away and give the modem some time to send in all the unsolicited 
notifications.  Most of the time the modem will send us everything we 
need in one quick (temporal) burst.  But this is not guaranteed of course.

If the timer expires with no additional info received, then perform the 
query/poll, which might end up being redundant due to timing, etc.  In 
general using timers like this is quite racy and can lead to quite ugly 
code.

> 
> So what about when a c?reg indication is received from the modem.
> Start a "poll" of all the supported c?reg and set a flag, so multiple
> polls are not started in case of an additional indication is received.
> When the poll of all the supported c?reg statuses returns -> update
> the gprs "core" with the combined status?

Are you talking about doing this in the gprs driver?  I would bite the 
bullet and do this in the core.  I would also assume the core has to 
perform its own 'combining of the status'.

> 
> .. and the attached_status callback should also poll all supported c?reg...

But essentially yes, you're stuck doing that whenever gprs_status has 
changed to searching/unknown.

Regards,
-Denis

Re: [RFC at-export 2/2] ublox: network-registration atom

[Giacinto Cifelli]

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

Hi Denis,

On Tue, May 21, 2019 at 5:43 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Richard,
>
> > ...and we have the issue that the indications seems to be sent in random order.
>
> Figured..
>
> Try looking at the strategy we employed when dealing with the 27.007
> voice calling (another nightmare) commands.  E.g. +CLIP, +CNAP, +CDIP,
> +CRING/RING, +CCWA, +CLCC, etc.  It might possibly give you some ideas.
> The general strategy is to start a timer, but not perform any queries
> right away and give the modem some time to send in all the unsolicited
> notifications.  Most of the time the modem will send us everything we
> need in one quick (temporal) burst.  But this is not guaranteed of course.
>
> If the timer expires with no additional info received, then perform the
> query/poll, which might end up being redundant due to timing, etc.  In
> general using timers like this is quite racy and can lead to quite ugly
> code.
>

the timer might not work well, because several stacks report a
detach/out-of-service only after an internal timeout. This happens
when another part of the stack, uncoordinated, has registered to
another technology.


> >
> > So what about when a c?reg indication is received from the modem.
> > Start a "poll" of all the supported c?reg and set a flag, so multiple
> > polls are not started in case of an additional indication is received.
> > When the poll of all the supported c?reg statuses returns -> update
> > the gprs "core" with the combined status?
>
> Are you talking about doing this in the gprs driver?  I would bite the
> bullet and do this in the core.  I would also assume the core has to
> perform its own 'combining of the status'.
>
> >
> > .. and the attached_status callback should also poll all supported c?reg...
>
> But essentially yes, you're stuck doing that whenever gprs_status has
> changed to searching/unknown.
>
> Regards,
> -Denis

Regards,
Giacinto

> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono