[PATCH 0/5] RFC: ubloxmodem add netmon interface

[PATCH 0/5] RFC: ubloxmodem add netmon interface

[djalal]

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

From: Djalal Harouni <djalal@endocode.com>

Hi,

These patches add netmon interface to ubloxmodem. This allows to query
and get data returned by +CESQ command through D-Bus.

The following types will be exposed as Network Monitor Property Types.
RSCP: Received Signal Code Power
ECN0: Received Energy Ratio
RSRQ: Reference Signal Received Quality
RSRP: Reference Signal Received Power
OPERATOR: the operator


The 5 patches are:

0001: add the ofono netmon types that we are interesting in
      OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}
      These will be collected from +COPS and +CESQ

0002: handle OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR} in D-Bus

0003: ubloxmodem: add the netmon driver
      This adds the actual netmon driver. It also contains the code to
      do +COPS and +CESQ and send back result.

0004: ubloxmodem: intialize and register the netmon driver
      This adds the code to intialize and register the driver that was
      added in patch 0003

0005: test: support OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}
      This makes get-serving-cell-info script return the all data that
      we did expose.


Questions:
How we should document these new fields in doc/networkmonitor-api.txt ?
Should we use the official values from the ublox AT manual ?

Thank you!


Djalal Harouni (5):
  netmon: add OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}
  netmon: handle OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR} in D-Bus
  ubloxmodem: add the netmon driver
  ubloxmodem: intialize and register the netmon driver
  test: support OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}

 Makefile.am                     |   1 +
 drivers/ubloxmodem/netmon.c     | 339 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/ubloxmodem/ubloxmodem.c |   2 +
 drivers/ubloxmodem/ubloxmodem.h |   3 ++
 include/netmon.h                |   5 ++
 plugins/ublox.c                 |   3 ++
 src/netmon.c                    |  40 +++++++++++++++-
 test/get-serving-cell-info      |  24 ++++++++++
 8 files changed, 416 insertions(+), 1 deletion(-)
 create mode 100644 drivers/ubloxmodem/netmon.c

-- 
2.5.5

[PATCH 1/5] netmon: add OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}

[djalal]

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

From: Djalal Harouni <djalal@endocode.com>

Add more ofono netmon info types that will be served through the netmon
interface. The main user of this now will be the ublox modem.

RSCP: Received Signal Code Power
ECN0: Received Energy Ratio
RSRQ: Reference Signal Received Quality
RSRP: Reference Signal Received Power
OPERATOR: the operator

Patches using these types to follow.
---
 include/netmon.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/netmon.h b/include/netmon.h
index ec8a2e1..abde225 100644
--- a/include/netmon.h
+++ b/include/netmon.h
@@ -59,6 +59,11 @@ enum ofono_netmon_info {
 	OFONO_NETMON_INFO_RSSI, /* int */
 	OFONO_NETMON_INFO_TIMING_ADVANCE, /* int */
 	OFONO_NETMON_INFO_PSC, /* int */
+	OFONO_NETMON_INFO_RSCP, /* int */
+	OFONO_NETMON_INFO_ECN0, /* int */
+	OFONO_NETMON_INFO_RSRQ, /* int */
+	OFONO_NETMON_INFO_RSRP, /* int */
+	OFONO_NETMON_INFO_OPERATOR, /* char *, up to 24 digits */
 	OFONO_NETMON_INFO_INVALID,
 };
 
-- 
2.5.5

[PATCH 2/5] netmon: handle OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR} in D-Bus

[djalal]

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

From: Djalal Harouni <djalal@endocode.com>

Handle the previously added types in D-Bus.
---
 src/netmon.c | 40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/src/netmon.c b/src/netmon.c
index eb18b9c..c87980a 100644
--- a/src/netmon.c
+++ b/src/netmon.c
@@ -78,7 +78,8 @@ void ofono_netmon_serving_cell_notify(struct ofono_netmon *netmon,
 	const char *technology = cell_type_to_tech_name(type);
 	char *mcc = NULL;
 	char *mnc = NULL;
-	int intval;
+	char *op = NULL;
+	int intval = -1;
 	netmon->reply = dbus_message_new_method_return(netmon->pending);
 
 	if (netmon->reply == NULL)
@@ -180,6 +181,43 @@ void ofono_netmon_serving_cell_notify(struct ofono_netmon *netmon,
 					intval, uint8_t, DBUS_TYPE_BYTE);
 			break;
 
+		case OFONO_NETMON_INFO_RSCP:
+			intval = va_arg(arglist, int);
+
+			CELL_INFO_DICT_APPEND(&dict, "ReceivedSignalCodePower",
+					intval, uint8_t, DBUS_TYPE_BYTE);
+			break;
+
+		case OFONO_NETMON_INFO_ECN0:
+			intval = va_arg(arglist, int);
+
+			CELL_INFO_DICT_APPEND(&dict, "ReceivedEnergyRatio",
+					intval, uint8_t, DBUS_TYPE_BYTE);
+			break;
+
+		case OFONO_NETMON_INFO_RSRQ:
+			intval = va_arg(arglist, int);
+
+			CELL_INFO_DICT_APPEND(&dict, "ReferenceSignalReceivedQuality",
+					intval, uint8_t, DBUS_TYPE_BYTE);
+			break;
+
+		case OFONO_NETMON_INFO_RSRP:
+			intval = va_arg(arglist, int);
+
+			CELL_INFO_DICT_APPEND(&dict, "ReferenceSignalReceivedPower",
+					intval, uint8_t, DBUS_TYPE_BYTE);
+			break;
+
+		case OFONO_NETMON_INFO_OPERATOR:
+			op = va_arg(arglist, char *);
+
+			if (op && strlen(op))
+				ofono_dbus_dict_append(&dict, "Operator",
+						DBUS_TYPE_STRING, &op);
+
+			break;
+
 		case OFONO_NETMON_INFO_INVALID:
 			break;
 		}
-- 
2.5.5

[PATCH 3/5] ubloxmodem: add the netmon driver

[djalal]

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

From: Djalal Harouni <djalal@endocode.com>

This adds a netmon driver for ublox. The driver support both +COPS and
+CESQ commands to return the previously added ofono netmon types:

RSCP: Received Signal Code Power
ECN0: Received Energy Ratio
RSRQ: Reference Signal Received Quality
RSRP: Reference Signal Received Power
OPERATOR: the operator
---
 Makefile.am                 |   1 +
 drivers/ubloxmodem/netmon.c | 339 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 340 insertions(+)
 create mode 100644 drivers/ubloxmodem/netmon.c

diff --git a/Makefile.am b/Makefile.am
index 3d7774b..07adeab 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -383,6 +383,7 @@ builtin_sources += drivers/atmodem/atutil.h \
 			drivers/ubloxmodem/ubloxmodem.h \
 			drivers/ubloxmodem/ubloxmodem.c \
 			drivers/ubloxmodem/gprs-context.c \
+			drivers/ubloxmodem/netmon.c \
 			drivers/ubloxmodem/lte.c
 
 
diff --git a/drivers/ubloxmodem/netmon.c b/drivers/ubloxmodem/netmon.c
new file mode 100644
index 0000000..6b9d74f
--- /dev/null
+++ b/drivers/ubloxmodem/netmon.c
@@ -0,0 +1,339 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2016  EndoCode AG. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <string.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/netmon.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "common.h"
+#include "netreg.h"
+#include "ubloxmodem.h"
+#include "drivers/atmodem/vendor.h"
+
+static const char *cops_prefix[] = { "+COPS:", NULL };
+static const char *cesq_prefix[] = { "+CESQ:", NULL };
+
+struct netmon_driver_data {
+	GAtChat *modem;
+	GAtChat *aux;
+};
+
+struct req_cb_data {
+	struct ofono_netmon *netmon;
+
+	ofono_netmon_cb_t cb;
+	void *data;
+
+	struct ofono_network_operator op;
+
+	int rxlev;	/* CESQ: Received Signal Strength Indication */
+	int ber;	/* CESQ: Bit Error Rate */
+	int rscp;	/* CESQ: Received Signal Code Powe */
+	int rsrp;	/* CESQ: Reference Signal Received Power */
+	double ecn0;	/* CESQ: Received Energy Ratio */
+	double rsrq;	/* CESQ: Reference Signal Received Quality */
+};
+
+/*
+ * Returns the appropriate radio access technology.
+ *
+ * If we can not resolve to a specific radio access technolgy
+ * we return OFONO_NETMON_CELL_TYPE_GSM by default.
+ */
+static int ublox_map_radio_access_technology(int tech)
+{
+	switch (tech) {
+	case ACCESS_TECHNOLOGY_GSM:
+	case ACCESS_TECHNOLOGY_GSM_COMPACT:
+		return OFONO_NETMON_CELL_TYPE_GSM;
+	case ACCESS_TECHNOLOGY_UTRAN:
+	case ACCESS_TECHNOLOGY_UTRAN_HSDPA:
+	case ACCESS_TECHNOLOGY_UTRAN_HSUPA:
+	case ACCESS_TECHNOLOGY_UTRAN_HSDPA_HSUPA:
+		return OFONO_NETMON_CELL_TYPE_UMTS;
+	case ACCESS_TECHNOLOGY_EUTRAN:
+		return OFONO_NETMON_CELL_TYPE_LTE;
+	}
+
+	return OFONO_NETMON_CELL_TYPE_GSM;
+}
+
+static inline struct req_cb_data *req_cb_data_new0(void *cb, void *data, void *user)
+{
+	struct req_cb_data *ret = g_new0(struct req_cb_data, 1);
+	if (ret == NULL)
+		return NULL;
+
+
+	ret->cb = cb;
+	ret->data = data;
+	ret->netmon = user;
+	ret->rxlev = -1;
+	ret->ber = -1;
+	ret->rscp = -1;
+	ret->rsrp = -1;
+	ret->ecn0 = -1;
+	ret->rsrq = -1;
+
+	return ret;
+}
+
+static gboolean ublox_delayed_register(gpointer user_data)
+{
+	struct ofono_netmon *netmon = user_data;
+
+	ofono_netmon_register(netmon);
+
+	return FALSE;
+}
+
+static void cesq_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	enum cesq_ofono_netmon_info {
+		CESQ_RXLEV,
+		CESQ_BER,
+		CESQ_RSCP,
+		CESQ_ECN0,
+		CESQ_RSRQ,
+		CESQ_RSRP,
+		_MAX,
+	};
+
+	struct req_cb_data *cbd = user_data;
+	struct ofono_netmon *nm = cbd->netmon;
+	struct ofono_error error;
+	GAtResultIter iter;
+	int idx, number;
+
+	DBG("ok %d", ok);
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		goto error;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CESQ:"))
+		return;
+
+	for (idx = 0; idx < _MAX; idx++) {
+
+		ok = g_at_result_iter_next_number(&iter, &number);
+		if (!ok) {
+			DBG(" error@idx: %d", idx);
+			goto error;
+		}
+
+		switch (idx) {
+		case CESQ_RXLEV:
+			cbd->rxlev = number != 99 ? number:cbd->rxlev;
+			break;
+		case CESQ_BER:
+			cbd->ber = number != 99 ? number:cbd->ber;
+			break;
+		case CESQ_RSCP:
+			cbd->rscp= number != 255 ? number:cbd->rscp;
+			break;
+		case CESQ_ECN0:
+			cbd->ecn0= number != 255 ? number:cbd->ecn0;
+			break;
+		case CESQ_RSRQ:
+			cbd->rsrq= number != 255 ? number:cbd->rsrq;
+			break;
+		case CESQ_RSRP:
+			cbd->rsrp= number != 255 ? number:cbd->rsrp;
+			break;
+		}
+	}
+
+	DBG(" RXLEV	%d ", cbd->rxlev);
+	DBG(" BER	%d ", cbd->ber);
+	DBG(" RSCP	%d ", cbd->rscp);
+	DBG(" ECN0	%f ", cbd->ecn0);
+	DBG(" RSRQ	%f ", cbd->rsrq);
+	DBG(" RSRP	%d ", cbd->rsrp);
+
+	ofono_netmon_serving_cell_notify(nm,
+					 cbd->op.tech,
+					 OFONO_NETMON_INFO_OPERATOR, cbd->op.name,
+					 OFONO_NETMON_INFO_RXLEV, cbd->rxlev,
+					 OFONO_NETMON_INFO_BER, cbd->ber,
+					 OFONO_NETMON_INFO_RSCP, cbd->rscp,
+					 OFONO_NETMON_INFO_ECN0, cbd->ecn0,
+					 OFONO_NETMON_INFO_RSRQ, cbd->rsrq,
+					 OFONO_NETMON_INFO_RSRP, cbd->rsrp,
+					 OFONO_NETMON_INFO_INVALID);
+
+	CALLBACK_WITH_SUCCESS(cbd->cb, cbd->data);
+	g_free(cbd);
+	return;
+
+error:
+	CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
+	g_free(cbd);
+}
+
+static void cops_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct req_cb_data *cbd = user_data;
+	struct ofono_netmon *nm = cbd->netmon;
+	struct netmon_driver_data *nmd = ofono_netmon_get_data(nm);
+	struct ofono_error error;
+	GAtResultIter iter;
+	int tech;
+	const char *name;
+
+	DBG("ok %d", ok);
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		goto error;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+COPS:"))
+		return;
+
+	g_at_result_iter_skip_next(&iter);
+	g_at_result_iter_skip_next(&iter);
+
+	if (g_at_result_iter_next_string(&iter, &name) == FALSE)
+		goto error;
+
+	strncpy(cbd->op.name, name, OFONO_MAX_OPERATOR_NAME_LENGTH);
+	cbd->op.name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0';
+
+	/* Ignored for now. TODO: maybe read format but value
+	 * wouldn't be forwarder anywhere
+	 */
+	cbd->op.mcc[0] = '\0';
+	cbd->op.mnc[0] = '\0';
+
+	/* Default to GSM */
+	if (g_at_result_iter_next_number(&iter, &tech) == FALSE)
+		cbd->op.tech = ublox_map_radio_access_technology(ACCESS_TECHNOLOGY_GSM);
+	else
+		cbd->op.tech = ublox_map_radio_access_technology(tech);
+
+	if (g_at_chat_send(nmd->aux, "AT+CESQ", cesq_prefix,
+			   cesq_cb, cbd, NULL)) {
+		return;
+	}
+
+error:
+	CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
+	g_free(cbd);
+}
+
+
+static void ublox_netmon_request_update(struct ofono_netmon *netmon,
+					ofono_netmon_cb_t cb, void *data)
+{
+	struct netmon_driver_data *nmd = ofono_netmon_get_data(netmon);
+	struct req_cb_data *cbd;
+
+	DBG("ublox netmon request update");
+
+	cbd = req_cb_data_new0(cb, data, netmon);
+	if (!cbd) {
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	if (g_at_chat_send(nmd->aux, "AT+COPS?", cops_prefix,
+			   cops_cb, cbd, NULL) == 0) {
+		CALLBACK_WITH_FAILURE(cb, data);
+		g_free(cbd);
+	}
+}
+
+static int ublox_netmon_probe(struct ofono_netmon *netmon,
+			      unsigned int vendor, void *user)
+{
+	struct netmon_driver_data *n = user;
+	struct netmon_driver_data *nmd;
+
+	DBG("ublox netmon probe");
+
+	nmd = g_try_new0(struct netmon_driver_data, 1);
+	if (nmd == NULL)
+		return -ENOMEM;
+
+	nmd->modem = g_at_chat_clone(n->modem);
+	nmd->aux = g_at_chat_clone(n->aux);
+
+	ofono_netmon_set_data(netmon, nmd);
+
+	g_idle_add(ublox_delayed_register, netmon);
+
+	return 0;
+}
+
+static void ublox_netmon_remove(struct ofono_netmon *netmon)
+{
+	struct netmon_driver_data *nmd = ofono_netmon_get_data(netmon);
+
+	DBG("ublox netmon remove");
+
+	ofono_netmon_set_data(netmon, NULL);
+
+	g_at_chat_unref(nmd->modem);
+	g_at_chat_unref(nmd->aux);
+
+	g_free(nmd);
+}
+
+static struct ofono_netmon_driver driver = {
+	.name			= UBLOXMODEM,
+	.probe			= ublox_netmon_probe,
+	.remove			= ublox_netmon_remove,
+	.request_update		= ublox_netmon_request_update,
+};
+
+void ublox_netmon_init(void)
+{
+	ofono_netmon_driver_register(&driver);
+}
+
+void ublox_netmon_exit(void)
+{
+	ofono_netmon_driver_unregister(&driver);
+}
-- 
2.5.5

[PATCH 4/5] ubloxmodem: intialize and register the netmon driver

[djalal]

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

From: Djalal Harouni <djalal@endocode.com>

This patch allows to intialize and register the ublox netmon driver.
---
 drivers/ubloxmodem/ubloxmodem.c | 2 ++
 drivers/ubloxmodem/ubloxmodem.h | 3 +++
 plugins/ublox.c                 | 3 +++
 3 files changed, 8 insertions(+)

diff --git a/drivers/ubloxmodem/ubloxmodem.c b/drivers/ubloxmodem/ubloxmodem.c
index 93cb928..a325b1f 100644
--- a/drivers/ubloxmodem/ubloxmodem.c
+++ b/drivers/ubloxmodem/ubloxmodem.c
@@ -36,6 +36,7 @@
 static int ubloxmodem_init(void)
 {
 	ublox_gprs_context_init();
+	ublox_netmon_init();
 	ublox_lte_init();
 
 	return 0;
@@ -44,6 +45,7 @@ static int ubloxmodem_init(void)
 static void ubloxmodem_exit(void)
 {
 	ublox_gprs_context_exit();
+	ublox_netmon_exit();
 	ublox_lte_exit();
 }
 
diff --git a/drivers/ubloxmodem/ubloxmodem.h b/drivers/ubloxmodem/ubloxmodem.h
index cf66412..bfb0106 100644
--- a/drivers/ubloxmodem/ubloxmodem.h
+++ b/drivers/ubloxmodem/ubloxmodem.h
@@ -26,5 +26,8 @@
 extern void ublox_gprs_context_init(void);
 extern void ublox_gprs_context_exit(void);
 
+extern void ublox_netmon_init(void);
+extern void ublox_netmon_exit(void);
+
 extern void ublox_lte_init(void);
 extern void ublox_lte_exit(void);
diff --git a/plugins/ublox.c b/plugins/ublox.c
index 6d77df8..cc44de4 100644
--- a/plugins/ublox.c
+++ b/plugins/ublox.c
@@ -38,6 +38,7 @@
 #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>
@@ -323,6 +324,8 @@ 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->aux);
+
+	ofono_netmon_create(modem, data->vendor_family, "ubloxmodem", data);
 }
 
 static struct ofono_modem_driver ublox_driver = {
-- 
2.5.5

[PATCH 5/5] test: support OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}

[djalal]

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

From: Djalal Harouni <djalal@endocode.com>

Display the following fields if they are returned.

RXLEV:  Received Signal Strength
RSCP:   Received Signal Code Power
ECN0:   Received Energy Ratio
RSRQ:   Reference Signal Received Quality
RSRP:   Reference Signal Received Power
OPERATOR: The operator
---
 test/get-serving-cell-info | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/get-serving-cell-info b/test/get-serving-cell-info
index 05dc9fe..e4c9efb 100755
--- a/test/get-serving-cell-info
+++ b/test/get-serving-cell-info
@@ -26,6 +26,12 @@ cid = 'CellId'
 psc = 'PrimaryScramblingCode'
 rssi = 'Strength'
 ber = 'BitErrorRate'
+rxlev = 'ReceivedSignalStrength'
+rscp = 'ReceivedSignalCodePower'
+ecn0 = 'ReceivedEnergyRatio'
+rsrq = 'ReferenceSignalReceivedQuality'
+rsrp = 'ReferenceSignalReceivedPower'
+op = 'Operator'
 
 print("Current serving cell information:")
 
@@ -50,7 +56,25 @@ if psc in servingcell:
 if rssi in servingcell:
 	print("    [ Signal Strength = %d]" % (servingcell[rssi]))
 
+if rxlev in servingcell:
+	print("    [ Received Signal Strength = %d]" % (servingcell[rxlev]))
+
 if ber in servingcell:
 	print("    [ Bit Error Rate = %d]" % (servingcell[ber]))
 
+if rscp in servingcell:
+	print("    [ Received Signal Code Power = %d]" % (servingcell[rscp]))
+
+if ecn0 in servingcell:
+	print("    [ Received Energy Ratio = %d]" % (servingcell[ecn0]))
+
+if rsrq in servingcell:
+	print("    [ Reference Signal Received Quality = %d]" % (servingcell[rsrq]))
+
+if rsrp in servingcell:
+	print("    [ Reference Signal Received Power = %d]" % (servingcell[rsrp]))
+
+if op in servingcell:
+	print("    [ Operator = %s ]" % (servingcell[op]))
+
 print('')
-- 
2.5.5

Re: [PATCH 1/5] netmon: add OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}

[Denis Kenzior]

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

Hi Djalal,

On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
> From: Djalal Harouni <djalal@endocode.com>
>
> Add more ofono netmon info types that will be served through the netmon
> interface. The main user of this now will be the ublox modem.
>
> RSCP: Received Signal Code Power
> ECN0: Received Energy Ratio
> RSRQ: Reference Signal Received Quality
> RSRP: Reference Signal Received Power
> OPERATOR: the operator
>
> Patches using these types to follow.
> ---
>   include/netmon.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/netmon.h b/include/netmon.h
> index ec8a2e1..abde225 100644
> --- a/include/netmon.h
> +++ b/include/netmon.h
> @@ -59,6 +59,11 @@ enum ofono_netmon_info {
>   	OFONO_NETMON_INFO_RSSI, /* int */
>   	OFONO_NETMON_INFO_TIMING_ADVANCE, /* int */
>   	OFONO_NETMON_INFO_PSC, /* int */
> +	OFONO_NETMON_INFO_RSCP, /* int */
> +	OFONO_NETMON_INFO_ECN0, /* int */
> +	OFONO_NETMON_INFO_RSRQ, /* int */
> +	OFONO_NETMON_INFO_RSRP, /* int */
> +	OFONO_NETMON_INFO_OPERATOR, /* char *, up to 24 digits */

MCC & MNC are not enough?  This API is intended for various location 
services which hardly care about the text representation of the 
operator.  They'd rather know the lac & ci, and possibly the mcc & mnc.

I'm not sure if its worth including this as it is likely just coming 
from an MCCMNC database in the modem and this info is available via 
NetworkRegistration anyway.

>   	OFONO_NETMON_INFO_INVALID,
>   };
>
>

Regards,
-Denis

Re: [PATCH 2/5] netmon: handle OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR} in D-Bus

[Denis Kenzior]

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

Hi Djalal,

On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
> From: Djalal Harouni <djalal@endocode.com>
>
> Handle the previously added types in D-Bus.
> ---
>   src/netmon.c | 40 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/src/netmon.c b/src/netmon.c
> index eb18b9c..c87980a 100644
> --- a/src/netmon.c
> +++ b/src/netmon.c
> @@ -78,7 +78,8 @@ void ofono_netmon_serving_cell_notify(struct ofono_netmon *netmon,
>   	const char *technology = cell_type_to_tech_name(type);
>   	char *mcc = NULL;
>   	char *mnc = NULL;
> -	int intval;
> +	char *op = NULL;
> +	int intval = -1;

Why the initializer?  Our rule of thumb is not to initialize in order to 
let the compiler (hopefully) or valgrind catch instances of 
uninitialized variable use (e.g. bugs).

>   	netmon->reply = dbus_message_new_method_return(netmon->pending);
>
>   	if (netmon->reply == NULL)
> @@ -180,6 +181,43 @@ void ofono_netmon_serving_cell_notify(struct ofono_netmon *netmon,
>   					intval, uint8_t, DBUS_TYPE_BYTE);
>   			break;
>
> +		case OFONO_NETMON_INFO_RSCP:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReceivedSignalCodePower",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +
> +		case OFONO_NETMON_INFO_ECN0:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReceivedEnergyRatio",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +
> +		case OFONO_NETMON_INFO_RSRQ:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReferenceSignalReceivedQuality",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +
> +		case OFONO_NETMON_INFO_RSRP:
> +			intval = va_arg(arglist, int);
> +
> +			CELL_INFO_DICT_APPEND(&dict, "ReferenceSignalReceivedPower",
> +					intval, uint8_t, DBUS_TYPE_BYTE);
> +			break;
> +

This looks fine, however there should be a patch documenting these and 
their respective value ranges inside doc/netmon-api.txt.

> +		case OFONO_NETMON_INFO_OPERATOR:
> +			op = va_arg(arglist, char *);
> +
> +			if (op && strlen(op))
> +				ofono_dbus_dict_append(&dict, "Operator",
> +						DBUS_TYPE_STRING, &op);
> +
> +			break;
> +
>   		case OFONO_NETMON_INFO_INVALID:
>   			break;
>   		}
>

Regards,
-Denis

Re: [PATCH 4/5] ubloxmodem: intialize and register the netmon driver

[Denis Kenzior]

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

Hi Djalal,

On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
> From: Djalal Harouni <djalal@endocode.com>
>
> This patch allows to intialize and register the ublox netmon driver.
> ---
>   drivers/ubloxmodem/ubloxmodem.c | 2 ++
>   drivers/ubloxmodem/ubloxmodem.h | 3 +++
>   plugins/ublox.c                 | 3 +++
>   3 files changed, 8 insertions(+)
>

This patch isn't broken down properly.  See HACKING, 'Submitting 
patches' section.  Each top-level directory change should be in a 
separate patch.  E.g. one patch for drivers/* and one patch for plugins/*.

It might be easier to simply squash drivers/ubloxmodem/ubloxmodem.[ch] 
changes into patch 3.

Regards,
-Denis

Re: [PATCH 3/5] ubloxmodem: add the netmon driver

[Denis Kenzior]

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

Hi Djalal,

On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
> From: Djalal Harouni <djalal@endocode.com>
>
> This adds a netmon driver for ublox. The driver support both +COPS and
> +CESQ commands to return the previously added ofono netmon types:
>
> RSCP: Received Signal Code Power
> ECN0: Received Energy Ratio
> RSRQ: Reference Signal Received Quality
> RSRP: Reference Signal Received Power
> OPERATOR: the operator
> ---
>   Makefile.am                 |   1 +
>   drivers/ubloxmodem/netmon.c | 339 ++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 340 insertions(+)
>   create mode 100644 drivers/ubloxmodem/netmon.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 3d7774b..07adeab 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -383,6 +383,7 @@ builtin_sources += drivers/atmodem/atutil.h \
>   			drivers/ubloxmodem/ubloxmodem.h \
>   			drivers/ubloxmodem/ubloxmodem.c \
>   			drivers/ubloxmodem/gprs-context.c \
> +			drivers/ubloxmodem/netmon.c \
>   			drivers/ubloxmodem/lte.c
>
>
> diff --git a/drivers/ubloxmodem/netmon.c b/drivers/ubloxmodem/netmon.c
> new file mode 100644
> index 0000000..6b9d74f
> --- /dev/null
> +++ b/drivers/ubloxmodem/netmon.c
> @@ -0,0 +1,339 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2016  EndoCode AG. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/netmon.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "common.h"
> +#include "netreg.h"
> +#include "ubloxmodem.h"
> +#include "drivers/atmodem/vendor.h"
> +
> +static const char *cops_prefix[] = { "+COPS:", NULL };
> +static const char *cesq_prefix[] = { "+CESQ:", NULL };
> +
> +struct netmon_driver_data {
> +	GAtChat *modem;
> +	GAtChat *aux;
> +};

Why do you need both the modem and the aux port?  I only see you using 
the aux port.

> +
> +struct req_cb_data {
> +	struct ofono_netmon *netmon;
> +
> +	ofono_netmon_cb_t cb;
> +	void *data;
> +
> +	struct ofono_network_operator op;
> +
> +	int rxlev;	/* CESQ: Received Signal Strength Indication */
> +	int ber;	/* CESQ: Bit Error Rate */
> +	int rscp;	/* CESQ: Received Signal Code Powe */
> +	int rsrp;	/* CESQ: Reference Signal Received Power */
> +	double ecn0;	/* CESQ: Received Energy Ratio */
> +	double rsrq;	/* CESQ: Reference Signal Received Quality */

Why are these doubles?

> +};
> +
> +/*
> + * Returns the appropriate radio access technology.
> + *
> + * If we can not resolve to a specific radio access technolgy
> + * we return OFONO_NETMON_CELL_TYPE_GSM by default.
> + */
> +static int ublox_map_radio_access_technology(int tech)
> +{
> +	switch (tech) {
> +	case ACCESS_TECHNOLOGY_GSM:
> +	case ACCESS_TECHNOLOGY_GSM_COMPACT:
> +		return OFONO_NETMON_CELL_TYPE_GSM;
> +	case ACCESS_TECHNOLOGY_UTRAN:
> +	case ACCESS_TECHNOLOGY_UTRAN_HSDPA:
> +	case ACCESS_TECHNOLOGY_UTRAN_HSUPA:
> +	case ACCESS_TECHNOLOGY_UTRAN_HSDPA_HSUPA:
> +		return OFONO_NETMON_CELL_TYPE_UMTS;
> +	case ACCESS_TECHNOLOGY_EUTRAN:
> +		return OFONO_NETMON_CELL_TYPE_LTE;
> +	}
> +
> +	return OFONO_NETMON_CELL_TYPE_GSM;
> +}
> +
> +static inline struct req_cb_data *req_cb_data_new0(void *cb, void *data, void *user)
> +{
> +	struct req_cb_data *ret = g_new0(struct req_cb_data, 1);
> +	if (ret == NULL)
> +		return NULL;
> +
> +
> +	ret->cb = cb;
> +	ret->data = data;
> +	ret->netmon = user;
> +	ret->rxlev = -1;
> +	ret->ber = -1;
> +	ret->rscp = -1;
> +	ret->rsrp = -1;
> +	ret->ecn0 = -1;
> +	ret->rsrq = -1;
> +
> +	return ret;
> +}
> +
> +static gboolean ublox_delayed_register(gpointer user_data)
> +{
> +	struct ofono_netmon *netmon = user_data;
> +
> +	ofono_netmon_register(netmon);
> +
> +	return FALSE;
> +}
> +
> +static void cesq_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	enum cesq_ofono_netmon_info {
> +		CESQ_RXLEV,
> +		CESQ_BER,
> +		CESQ_RSCP,
> +		CESQ_ECN0,
> +		CESQ_RSRQ,
> +		CESQ_RSRP,
> +		_MAX,
> +	};
> +
> +	struct req_cb_data *cbd = user_data;
> +	struct ofono_netmon *nm = cbd->netmon;
> +	struct ofono_error error;
> +	GAtResultIter iter;
> +	int idx, number;
> +
> +	DBG("ok %d", ok);
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok) {
> +		goto error;
> +	}

No need for parentheses

> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CESQ:"))
> +		return;

What happens to memory used by cbd? :)

> +
> +	for (idx = 0; idx < _MAX; idx++) {
> +
> +		ok = g_at_result_iter_next_number(&iter, &number);
> +		if (!ok) {
> +			DBG(" error at idx: %d", idx);
> +			goto error;
> +		}
> +
> +		switch (idx) {
> +		case CESQ_RXLEV:
> +			cbd->rxlev = number != 99 ? number:cbd->rxlev;
> +			break;
> +		case CESQ_BER:
> +			cbd->ber = number != 99 ? number:cbd->ber;
> +			break;
> +		case CESQ_RSCP:
> +			cbd->rscp= number != 255 ? number:cbd->rscp;
> +			break;
> +		case CESQ_ECN0:
> +			cbd->ecn0= number != 255 ? number:cbd->ecn0;
> +			break;
> +		case CESQ_RSRQ:
> +			cbd->rsrq= number != 255 ? number:cbd->rsrq;
> +			break;
> +		case CESQ_RSRP:
> +			cbd->rsrp= number != 255 ? number:cbd->rsrp;
> +			break;

space before / after equal sign.  See doc/coding-style.txt

> +		}
> +	}
> +
> +	DBG(" RXLEV	%d ", cbd->rxlev);
> +	DBG(" BER	%d ", cbd->ber);
> +	DBG(" RSCP	%d ", cbd->rscp);
> +	DBG(" ECN0	%f ", cbd->ecn0);
> +	DBG(" RSRQ	%f ", cbd->rsrq);
> +	DBG(" RSRP	%d ", cbd->rsrp);
> +
> +	ofono_netmon_serving_cell_notify(nm,
> +					 cbd->op.tech,
> +					 OFONO_NETMON_INFO_OPERATOR, cbd->op.name,
> +					 OFONO_NETMON_INFO_RXLEV, cbd->rxlev,
> +					 OFONO_NETMON_INFO_BER, cbd->ber,
> +					 OFONO_NETMON_INFO_RSCP, cbd->rscp,
> +					 OFONO_NETMON_INFO_ECN0, cbd->ecn0,
> +					 OFONO_NETMON_INFO_RSRQ, cbd->rsrq,
> +					 OFONO_NETMON_INFO_RSRP, cbd->rsrp,
> +					 OFONO_NETMON_INFO_INVALID);
> +
> +	CALLBACK_WITH_SUCCESS(cbd->cb, cbd->data);
> +	g_free(cbd);
> +	return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
> +	g_free(cbd);
> +}
> +
> +static void cops_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct req_cb_data *cbd = user_data;
> +	struct ofono_netmon *nm = cbd->netmon;
> +	struct netmon_driver_data *nmd = ofono_netmon_get_data(nm);
> +	struct ofono_error error;
> +	GAtResultIter iter;
> +	int tech;
> +	const char *name;
> +
> +	DBG("ok %d", ok);
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok) {
> +		goto error;
> +	}

No need for parentheses

> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+COPS:"))
> +		return;

What happens to memory used by cbd?

> +
> +	g_at_result_iter_skip_next(&iter);
> +	g_at_result_iter_skip_next(&iter);
> +
> +	if (g_at_result_iter_next_string(&iter, &name) == FALSE)
> +		goto error;
> +
> +	strncpy(cbd->op.name, name, OFONO_MAX_OPERATOR_NAME_LENGTH);
> +	cbd->op.name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0';
> +

As mentioned before, not sure string representation of the operator is 
useful...

> +	/* Ignored for now. TODO: maybe read format but value
> +	 * wouldn't be forwarder anywhere
> +	 */
> +	cbd->op.mcc[0] = '\0';
> +	cbd->op.mnc[0] = '\0';
> +
> +	/* Default to GSM */
> +	if (g_at_result_iter_next_number(&iter, &tech) == FALSE)
> +		cbd->op.tech = ublox_map_radio_access_technology(ACCESS_TECHNOLOGY_GSM);
> +	else
> +		cbd->op.tech = ublox_map_radio_access_technology(tech);
> +
> +	if (g_at_chat_send(nmd->aux, "AT+CESQ", cesq_prefix,
> +			   cesq_cb, cbd, NULL)) {
> +		return;
> +	}
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
> +	g_free(cbd);
> +}
> +
> +

double empty line

> +static void ublox_netmon_request_update(struct ofono_netmon *netmon,
> +					ofono_netmon_cb_t cb, void *data)
> +{
> +	struct netmon_driver_data *nmd = ofono_netmon_get_data(netmon);
> +	struct req_cb_data *cbd;
> +
> +	DBG("ublox netmon request update");
> +
> +	cbd = req_cb_data_new0(cb, data, netmon);
> +	if (!cbd) {
> +		CALLBACK_WITH_FAILURE(cb, data);
> +		return;
> +	}
> +
> +	if (g_at_chat_send(nmd->aux, "AT+COPS?", cops_prefix,
> +			   cops_cb, cbd, NULL) == 0) {

You should be providing a destructor for cbd.  E.g. last argument to 
g_at_chat_send.  Avoids memory leaks if the modem gets hot-unplugged for 
example.  I think some of the drivers still get this wrong when 
implementing multiple-command transactions.

Might be safer to simply issue AT+COPS;+CESQ and write a single callback 
that parses both responses.

> +		CALLBACK_WITH_FAILURE(cb, data);
> +		g_free(cbd);
> +	}
> +}
> +
> +static int ublox_netmon_probe(struct ofono_netmon *netmon,
> +			      unsigned int vendor, void *user)
> +{
> +	struct netmon_driver_data *n = user;

This is evil and is going to bite you at some point.  In 
ofono_netmon_create inside ublox.c you pass in ublox_data.  So if 
someone changes this structure or ublox_data, things will explode.

> +	struct netmon_driver_data *nmd;
> +
> +	DBG("ublox netmon probe");
> +
> +	nmd = g_try_new0(struct netmon_driver_data, 1);
> +	if (nmd == NULL)
> +		return -ENOMEM;
> +
> +	nmd->modem = g_at_chat_clone(n->modem);
> +	nmd->aux = g_at_chat_clone(n->aux);

If you truly need both ports, use the g_at_chat_set_slave functionality.

> +
> +	ofono_netmon_set_data(netmon, nmd);
> +
> +	g_idle_add(ublox_delayed_register, netmon);
> +
> +	return 0;
> +}
> +
> +static void ublox_netmon_remove(struct ofono_netmon *netmon)
> +{
> +	struct netmon_driver_data *nmd = ofono_netmon_get_data(netmon);
> +
> +	DBG("ublox netmon remove");
> +
> +	ofono_netmon_set_data(netmon, NULL);
> +
> +	g_at_chat_unref(nmd->modem);
> +	g_at_chat_unref(nmd->aux);
> +
> +	g_free(nmd);
> +}
> +
> +static struct ofono_netmon_driver driver = {
> +	.name			= UBLOXMODEM,
> +	.probe			= ublox_netmon_probe,
> +	.remove			= ublox_netmon_remove,
> +	.request_update		= ublox_netmon_request_update,
> +};
> +
> +void ublox_netmon_init(void)
> +{
> +	ofono_netmon_driver_register(&driver);
> +}
> +
> +void ublox_netmon_exit(void)
> +{
> +	ofono_netmon_driver_unregister(&driver);
> +}
>

Regards,
-Denis

Re: [PATCH 1/5] netmon: add OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR}

[Djalal Harouni]

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

Hi Denis,

On 29 November 2016 at 17:43, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Djalal,
>
>
> On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
>>
>> From: Djalal Harouni <djalal@endocode.com>
>>
>> Add more ofono netmon info types that will be served through the netmon
>> interface. The main user of this now will be the ublox modem.
>>
>> RSCP: Received Signal Code Power
>> ECN0: Received Energy Ratio
>> RSRQ: Reference Signal Received Quality
>> RSRP: Reference Signal Received Power
>> OPERATOR: the operator
>>
>> Patches using these types to follow.
>> ---
>>   include/netmon.h | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/include/netmon.h b/include/netmon.h
>> index ec8a2e1..abde225 100644
>> --- a/include/netmon.h
>> +++ b/include/netmon.h
>> @@ -59,6 +59,11 @@ enum ofono_netmon_info {
>>         OFONO_NETMON_INFO_RSSI, /* int */
>>         OFONO_NETMON_INFO_TIMING_ADVANCE, /* int */
>>         OFONO_NETMON_INFO_PSC, /* int */
>> +       OFONO_NETMON_INFO_RSCP, /* int */
>> +       OFONO_NETMON_INFO_ECN0, /* int */
>> +       OFONO_NETMON_INFO_RSRQ, /* int */
>> +       OFONO_NETMON_INFO_RSRP, /* int */
>> +       OFONO_NETMON_INFO_OPERATOR, /* char *, up to 24 digits */
>
>
> MCC & MNC are not enough?  This API is intended for various location
> services which hardly care about the text representation of the operator.
> They'd rather know the lac & ci, and possibly the mcc & mnc.
>
> I'm not sure if its worth including this as it is likely just coming from an
> MCCMNC database in the modem and this info is available via
> NetworkRegistration anyway.

Ok I dropped that one and will try to see how we can fix this at our end.

>>         OFONO_NETMON_INFO_INVALID,
>>   };
>>
>>
>
> Regards,
> -Denis

Re: [PATCH 2/5] netmon: handle OFONO_NETMON_INFO_{RSCP|ECN0|RSRQ|RSRP|OPERATOR} in D-Bus

[Djalal Harouni]

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

Hi Denis,

On 29 November 2016 at 17:49, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Djalal,
>
> On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
>>
>> From: Djalal Harouni <djalal@endocode.com>
>>
>> Handle the previously added types in D-Bus.
>> ---
>>   src/netmon.c | 40 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/netmon.c b/src/netmon.c
>> index eb18b9c..c87980a 100644
>> --- a/src/netmon.c
>> +++ b/src/netmon.c
>> @@ -78,7 +78,8 @@ void ofono_netmon_serving_cell_notify(struct
>> ofono_netmon *netmon,
>>         const char *technology = cell_type_to_tech_name(type);
>>         char *mcc = NULL;
>>         char *mnc = NULL;
>> -       int intval;
>> +       char *op = NULL;
>> +       int intval = -1;
>
>
> Why the initializer?  Our rule of thumb is not to initialize in order to let
> the compiler (hopefully) or valgrind catch instances of uninitialized
> variable use (e.g. bugs).

Ok dropped that one too, but in this case it does not matter, va_arg()
is undefined any way...

>
>>         netmon->reply = dbus_message_new_method_return(netmon->pending);
>>
>>         if (netmon->reply == NULL)
>> @@ -180,6 +181,43 @@ void ofono_netmon_serving_cell_notify(struct
>> ofono_netmon *netmon,
>>                                         intval, uint8_t, DBUS_TYPE_BYTE);
>>                         break;
>>
>> +               case OFONO_NETMON_INFO_RSCP:
>> +                       intval = va_arg(arglist, int);
>> +
>> +                       CELL_INFO_DICT_APPEND(&dict,
>> "ReceivedSignalCodePower",
>> +                                       intval, uint8_t, DBUS_TYPE_BYTE);
>> +                       break;
>> +
>> +               case OFONO_NETMON_INFO_ECN0:
>> +                       intval = va_arg(arglist, int);
>> +
>> +                       CELL_INFO_DICT_APPEND(&dict,
>> "ReceivedEnergyRatio",
>> +                                       intval, uint8_t, DBUS_TYPE_BYTE);
>> +                       break;
>> +
>> +               case OFONO_NETMON_INFO_RSRQ:
>> +                       intval = va_arg(arglist, int);
>> +
>> +                       CELL_INFO_DICT_APPEND(&dict,
>> "ReferenceSignalReceivedQuality",
>> +                                       intval, uint8_t, DBUS_TYPE_BYTE);
>> +                       break;
>> +
>> +               case OFONO_NETMON_INFO_RSRP:
>> +                       intval = va_arg(arglist, int);
>> +
>> +                       CELL_INFO_DICT_APPEND(&dict,
>> "ReferenceSignalReceivedPower",
>> +                                       intval, uint8_t, DBUS_TYPE_BYTE);
>> +                       break;
>> +
>
>
> This looks fine, however there should be a patch documenting these and their
> respective value ranges inside doc/netmon-api.txt.

OK documented in netmonmonitor-api.txt of v2 of the patches. Thank you!

>> +               case OFONO_NETMON_INFO_OPERATOR:
>> +                       op = va_arg(arglist, char *);
>> +
>> +                       if (op && strlen(op))
>> +                               ofono_dbus_dict_append(&dict, "Operator",
>> +                                               DBUS_TYPE_STRING, &op);
>> +
>> +                       break;
>> +
>>                 case OFONO_NETMON_INFO_INVALID:
>>                         break;
>>                 }
>>
>
> Regards,
> -Denis

Re: [PATCH 4/5] ubloxmodem: intialize and register the netmon driver

[Djalal Harouni]

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

On 29 November 2016 at 17:53, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Djalal,
>
> On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
>>
>> From: Djalal Harouni <djalal@endocode.com>
>>
>> This patch allows to intialize and register the ublox netmon driver.
>> ---
>>   drivers/ubloxmodem/ubloxmodem.c | 2 ++
>>   drivers/ubloxmodem/ubloxmodem.h | 3 +++
>>   plugins/ublox.c                 | 3 +++
>>   3 files changed, 8 insertions(+)
>>
>
> This patch isn't broken down properly.  See HACKING, 'Submitting patches'
> section.  Each top-level directory change should be in a separate patch.
> E.g. one patch for drivers/* and one patch for plugins/*.
>
> It might be easier to simply squash drivers/ubloxmodem/ubloxmodem.[ch]
> changes into patch 3.

Oups sorry about that. Hopefully fixed in v2.

> Regards,
> -Denis

Re: [PATCH 3/5] ubloxmodem: add the netmon driver

[Djalal Harouni]

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

Hi Denis,

On 29 November 2016 at 18:32, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Djalal,
>
>
> On 11/25/2016 08:00 AM, djalal(a)endocode.com wrote:
>>
>> From: Djalal Harouni <djalal@endocode.com>
>>
>> This adds a netmon driver for ublox. The driver support both +COPS and
>> +CESQ commands to return the previously added ofono netmon types:
>>
>> RSCP: Received Signal Code Power
>> ECN0: Received Energy Ratio
>> RSRQ: Reference Signal Received Quality
>> RSRP: Reference Signal Received Power
>> OPERATOR: the operator
>> ---
>>   Makefile.am                 |   1 +
>>   drivers/ubloxmodem/netmon.c | 339
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 340 insertions(+)
>>   create mode 100644 drivers/ubloxmodem/netmon.c
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 3d7774b..07adeab 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -383,6 +383,7 @@ builtin_sources += drivers/atmodem/atutil.h \
>>                         drivers/ubloxmodem/ubloxmodem.h \
>>                         drivers/ubloxmodem/ubloxmodem.c \
>>                         drivers/ubloxmodem/gprs-context.c \
>> +                       drivers/ubloxmodem/netmon.c \
>>                         drivers/ubloxmodem/lte.c
>>
>>
>> diff --git a/drivers/ubloxmodem/netmon.c b/drivers/ubloxmodem/netmon.c
>> new file mode 100644
>> index 0000000..6b9d74f
>> --- /dev/null
>> +++ b/drivers/ubloxmodem/netmon.c
>> @@ -0,0 +1,339 @@
>> +/*
>> + *
>> + *  oFono - Open Source Telephony
>> + *
>> + *  Copyright (C) 2016  EndoCode AG. All rights reserved.
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License version 2 as
>> + *  published by the Free Software Foundation.
>> + *
>> + *  This program is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with this program; if not, write to the Free Software
>> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301
>> USA
>> + *
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#define _GNU_SOURCE
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +#include <string.h>
>> +#include <errno.h>
>> +
>> +#include <glib.h>
>> +
>> +#include <ofono/log.h>
>> +#include <ofono/modem.h>
>> +#include <ofono/netmon.h>
>> +
>> +#include "gatchat.h"
>> +#include "gatresult.h"
>> +
>> +#include "common.h"
>> +#include "netreg.h"
>> +#include "ubloxmodem.h"
>> +#include "drivers/atmodem/vendor.h"
>> +
>> +static const char *cops_prefix[] = { "+COPS:", NULL };
>> +static const char *cesq_prefix[] = { "+CESQ:", NULL };
>> +
>> +struct netmon_driver_data {
>> +       GAtChat *modem;
>> +       GAtChat *aux;
>> +};
>
>
> Why do you need both the modem and the aux port?  I only see you using the
> aux port.

Ok fixed in v2. we need only one channel.

>> +
>> +struct req_cb_data {
>> +       struct ofono_netmon *netmon;
>> +
>> +       ofono_netmon_cb_t cb;
>> +       void *data;
>> +
>> +       struct ofono_network_operator op;
>> +
>> +       int rxlev;      /* CESQ: Received Signal Strength Indication */
>> +       int ber;        /* CESQ: Bit Error Rate */
>> +       int rscp;       /* CESQ: Received Signal Code Powe */
>> +       int rsrp;       /* CESQ: Reference Signal Received Power */
>> +       double ecn0;    /* CESQ: Received Energy Ratio */
>> +       double rsrq;    /* CESQ: Reference Signal Received Quality */
>
>
> Why are these doubles?

Fixed.


>
>> +};
>> +
>> +/*
>> + * Returns the appropriate radio access technology.
>> + *
>> + * If we can not resolve to a specific radio access technolgy
>> + * we return OFONO_NETMON_CELL_TYPE_GSM by default.
>> + */
>> +static int ublox_map_radio_access_technology(int tech)
>> +{
>> +       switch (tech) {
>> +       case ACCESS_TECHNOLOGY_GSM:
>> +       case ACCESS_TECHNOLOGY_GSM_COMPACT:
>> +               return OFONO_NETMON_CELL_TYPE_GSM;
>> +       case ACCESS_TECHNOLOGY_UTRAN:
>> +       case ACCESS_TECHNOLOGY_UTRAN_HSDPA:
>> +       case ACCESS_TECHNOLOGY_UTRAN_HSUPA:
>> +       case ACCESS_TECHNOLOGY_UTRAN_HSDPA_HSUPA:
>> +               return OFONO_NETMON_CELL_TYPE_UMTS;
>> +       case ACCESS_TECHNOLOGY_EUTRAN:
>> +               return OFONO_NETMON_CELL_TYPE_LTE;
>> +       }
>> +
>> +       return OFONO_NETMON_CELL_TYPE_GSM;
>> +}
>> +
>> +static inline struct req_cb_data *req_cb_data_new0(void *cb, void *data,
>> void *user)
>> +{
>> +       struct req_cb_data *ret = g_new0(struct req_cb_data, 1);
>> +       if (ret == NULL)
>> +               return NULL;
>> +
>> +
>> +       ret->cb = cb;
>> +       ret->data = data;
>> +       ret->netmon = user;
>> +       ret->rxlev = -1;
>> +       ret->ber = -1;
>> +       ret->rscp = -1;
>> +       ret->rsrp = -1;
>> +       ret->ecn0 = -1;
>> +       ret->rsrq = -1;
>> +
>> +       return ret;
>> +}
>> +
>> +static gboolean ublox_delayed_register(gpointer user_data)
>> +{
>> +       struct ofono_netmon *netmon = user_data;
>> +
>> +       ofono_netmon_register(netmon);
>> +
>> +       return FALSE;
>> +}
>> +
>> +static void cesq_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +       enum cesq_ofono_netmon_info {
>> +               CESQ_RXLEV,
>> +               CESQ_BER,
>> +               CESQ_RSCP,
>> +               CESQ_ECN0,
>> +               CESQ_RSRQ,
>> +               CESQ_RSRP,
>> +               _MAX,
>> +       };
>> +
>> +       struct req_cb_data *cbd = user_data;
>> +       struct ofono_netmon *nm = cbd->netmon;
>> +       struct ofono_error error;
>> +       GAtResultIter iter;
>> +       int idx, number;
>> +
>> +       DBG("ok %d", ok);
>> +
>> +       decode_at_error(&error, g_at_result_final_response(result));
>> +
>> +       if (!ok) {
>> +               goto error;
>> +       }
>
>
> No need for parentheses

Ok, fixed.

>> +
>> +       g_at_result_iter_init(&iter, result);
>> +
>> +       if (!g_at_result_iter_next(&iter, "+CESQ:"))
>> +               return;
>
>
> What happens to memory used by cbd? :)

Oups nice catch. Please see below about how we are planning to free
memory and report since later I will send another set of patches to
support +UCGED which has more statistics and corner cases to fail.

>
>> +
>> +       for (idx = 0; idx < _MAX; idx++) {
>> +
>> +               ok = g_at_result_iter_next_number(&iter, &number);
>> +               if (!ok) {
>> +                       DBG(" error at idx: %d", idx);
>> +                       goto error;
>> +               }
>> +
>> +               switch (idx) {
>> +               case CESQ_RXLEV:
>> +                       cbd->rxlev = number != 99 ? number:cbd->rxlev;
>> +                       break;
>> +               case CESQ_BER:
>> +                       cbd->ber = number != 99 ? number:cbd->ber;
>> +                       break;
>> +               case CESQ_RSCP:
>> +                       cbd->rscp= number != 255 ? number:cbd->rscp;
>> +                       break;
>> +               case CESQ_ECN0:
>> +                       cbd->ecn0= number != 255 ? number:cbd->ecn0;
>> +                       break;
>> +               case CESQ_RSRQ:
>> +                       cbd->rsrq= number != 255 ? number:cbd->rsrq;
>> +                       break;
>> +               case CESQ_RSRP:
>> +                       cbd->rsrp= number != 255 ? number:cbd->rsrp;
>> +                       break;
>
>
> space before / after equal sign.  See doc/coding-style.txt

Oups :-) fixed.

>
>> +               }
>> +       }
>> +
>> +       DBG(" RXLEV     %d ", cbd->rxlev);
>> +       DBG(" BER       %d ", cbd->ber);
>> +       DBG(" RSCP      %d ", cbd->rscp);
>> +       DBG(" ECN0      %f ", cbd->ecn0);
>> +       DBG(" RSRQ      %f ", cbd->rsrq);
>> +       DBG(" RSRP      %d ", cbd->rsrp);
>> +
>> +       ofono_netmon_serving_cell_notify(nm,
>> +                                        cbd->op.tech,
>> +                                        OFONO_NETMON_INFO_OPERATOR,
>> cbd->op.name,
>> +                                        OFONO_NETMON_INFO_RXLEV,
>> cbd->rxlev,
>> +                                        OFONO_NETMON_INFO_BER, cbd->ber,
>> +                                        OFONO_NETMON_INFO_RSCP,
>> cbd->rscp,
>> +                                        OFONO_NETMON_INFO_ECN0,
>> cbd->ecn0,
>> +                                        OFONO_NETMON_INFO_RSRQ,
>> cbd->rsrq,
>> +                                        OFONO_NETMON_INFO_RSRP,
>> cbd->rsrp,
>> +                                        OFONO_NETMON_INFO_INVALID);
>> +
>> +       CALLBACK_WITH_SUCCESS(cbd->cb, cbd->data);
>> +       g_free(cbd);
>> +       return;
>> +
>> +error:
>> +       CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
>> +       g_free(cbd);
>> +}
>> +
>> +static void cops_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +       struct req_cb_data *cbd = user_data;
>> +       struct ofono_netmon *nm = cbd->netmon;
>> +       struct netmon_driver_data *nmd = ofono_netmon_get_data(nm);
>> +       struct ofono_error error;
>> +       GAtResultIter iter;
>> +       int tech;
>> +       const char *name;
>> +
>> +       DBG("ok %d", ok);
>> +
>> +       decode_at_error(&error, g_at_result_final_response(result));
>> +
>> +       if (!ok) {
>> +               goto error;
>> +       }
>
>
> No need for parentheses
>
>> +
>> +       g_at_result_iter_init(&iter, result);
>> +
>> +       if (!g_at_result_iter_next(&iter, "+COPS:"))
>> +               return;
>
>
> What happens to memory used by cbd?

Please see below.

>> +
>> +       g_at_result_iter_skip_next(&iter);
>> +       g_at_result_iter_skip_next(&iter);
>> +
>> +       if (g_at_result_iter_next_string(&iter, &name) == FALSE)
>> +               goto error;
>> +
>> +       strncpy(cbd->op.name, name, OFONO_MAX_OPERATOR_NAME_LENGTH);
>> +       cbd->op.name[OFONO_MAX_OPERATOR_NAME_LENGTH] = '\0';
>> +
>
>
> As mentioned before, not sure string representation of the operator is
> useful...

Dropped.

>> +       /* Ignored for now. TODO: maybe read format but value
>> +        * wouldn't be forwarder anywhere
>> +        */
>> +       cbd->op.mcc[0] = '\0';
>> +       cbd->op.mnc[0] = '\0';
>> +
>> +       /* Default to GSM */
>> +       if (g_at_result_iter_next_number(&iter, &tech) == FALSE)
>> +               cbd->op.tech =
>> ublox_map_radio_access_technology(ACCESS_TECHNOLOGY_GSM);
>> +       else
>> +               cbd->op.tech = ublox_map_radio_access_technology(tech);
>> +
>> +       if (g_at_chat_send(nmd->aux, "AT+CESQ", cesq_prefix,
>> +                          cesq_cb, cbd, NULL)) {
>> +               return;
>> +       }
>> +
>> +error:
>> +       CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
>> +       g_free(cbd);
>> +}
>> +
>> +
>
>
> double empty line

Fixed.

>> +static void ublox_netmon_request_update(struct ofono_netmon *netmon,
>> +                                       ofono_netmon_cb_t cb, void *data)
>> +{
>> +       struct netmon_driver_data *nmd = ofono_netmon_get_data(netmon);
>> +       struct req_cb_data *cbd;
>> +
>> +       DBG("ublox netmon request update");
>> +
>> +       cbd = req_cb_data_new0(cb, data, netmon);
>> +       if (!cbd) {
>> +               CALLBACK_WITH_FAILURE(cb, data);
>> +               return;
>> +       }
>> +
>> +       if (g_at_chat_send(nmd->aux, "AT+COPS?", cops_prefix,
>> +                          cops_cb, cbd, NULL) == 0) {
>
>
> You should be providing a destructor for cbd.  E.g. last argument to
> g_at_chat_send.  Avoids memory leaks if the modem gets hot-unplugged for
> example.  I think some of the drivers still get this wrong when implementing
> multiple-command transactions.
>
> Might be safer to simply issue AT+COPS;+CESQ and write a single callback
> that parses both responses.

Ok you are right. So the plan is to issue COPS, +CESQ then follow up
with +UCGED which has different modes depending on the firmware of the
modem and may return different values according to that mode. We
prefer to not chain them but have callbacks for each command and later
report what ever we managed to get. Also there is the case where it is
hard to parse the results of +UCGED.

()So the plan is to always try to report what we collected and to not
fail. So I backported in v2 what we have with +UCGED. By having a
finish function that does:
1) ofono_netmon_serving_cell_notify()
2) CALLBACK_WITH_SUCCESS(cbd->cb, cbd->data);
3) g_free(cbd);

This function will always be executed if sending +CESQ succeeded. This
means it will always be executed if we are not parsing +CESQ result or
failed to parse some responses of +CESQ to whatever reason. We try to
be conservative and not fail and do our best to report anything.
However if we fail in +COPS or before +CESQ result then we notify with
an error and free(cbd);

This logic is implemented in v2.


>> +               CALLBACK_WITH_FAILURE(cb, data);
>> +               g_free(cbd);
>> +       }
>> +}
>> +
>> +static int ublox_netmon_probe(struct ofono_netmon *netmon,
>> +                             unsigned int vendor, void *user)
>> +{
>> +       struct netmon_driver_data *n = user;
>
>
> This is evil and is going to bite you at some point.  In ofono_netmon_create
> inside ublox.c you pass in ublox_data.  So if someone changes this structure
> or ublox_data, things will explode.

Indeed, thanks for the note, fixed in v2.

>
>> +       struct netmon_driver_data *nmd;
>> +
>> +       DBG("ublox netmon probe");
>> +
>> +       nmd = g_try_new0(struct netmon_driver_data, 1);
>> +       if (nmd == NULL)
>> +               return -ENOMEM;
>> +
>> +       nmd->modem = g_at_chat_clone(n->modem);
>> +       nmd->aux = g_at_chat_clone(n->aux);
>
>
> If you truly need both ports, use the g_at_chat_set_slave functionality.
>
>
>> +
>> +       ofono_netmon_set_data(netmon, nmd);
>> +
>> +       g_idle_add(ublox_delayed_register, netmon);
>> +
>> +       return 0;
>> +}
>> +
>> +static void ublox_netmon_remove(struct ofono_netmon *netmon)
>> +{
>> +       struct netmon_driver_data *nmd = ofono_netmon_get_data(netmon);
>> +
>> +       DBG("ublox netmon remove");
>> +
>> +       ofono_netmon_set_data(netmon, NULL);
>> +
>> +       g_at_chat_unref(nmd->modem);
>> +       g_at_chat_unref(nmd->aux);
>> +
>> +       g_free(nmd);
>> +}
>> +
>> +static struct ofono_netmon_driver driver = {
>> +       .name                   = UBLOXMODEM,
>> +       .probe                  = ublox_netmon_probe,
>> +       .remove                 = ublox_netmon_remove,
>> +       .request_update         = ublox_netmon_request_update,
>> +};
>> +
>> +void ublox_netmon_init(void)
>> +{
>> +       ofono_netmon_driver_register(&driver);
>> +}
>> +
>> +void ublox_netmon_exit(void)
>> +{
>> +       ofono_netmon_driver_unregister(&driver);
>> +}
>>
>
> Regards,
> -Denis