[RFC PATCH 0/5 v2] Atom for Neighbour Cell Info

[RFC PATCH 0/5 v2] Atom for Neighbour Cell Info

[Antti Paila]

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

 This series of patches implements an interface for client
 application to fetch the ECID information of neighbouring
 cells using DBUS.

[RFC PATCH 1/5 v2] cell-info: Atom for ECID info of neighb cells

[Antti Paila]

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

---
 src/cell-info.c |  484 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 484 insertions(+), 0 deletions(-)
 create mode 100644 src/cell-info.c

diff --git a/src/cell-info.c b/src/cell-info.c
new file mode 100644
index 0000000..2289ce1
--- /dev/null
+++ b/src/cell-info.c
@@ -0,0 +1,484 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <glib.h>
+#include <gdbus.h>
+#include <sys/time.h>
+
+#include "ofono.h"
+
+#include "common.h"
+#include "util.h"
+#include "ofono/cell-info.h"
+
+
+struct ofono_cell_info {
+	DBusMessage *pending;
+	struct ofono_atom *atom;
+	const struct ofono_cell_info_driver *driver;
+	void *driver_data;
+};
+
+
+static DBusMessage *ci_get_cells(DBusConnection *, DBusMessage *, void *);
+
+static GSList *g_drivers = NULL;
+
+static GDBusMethodTable ci_methods[] = {
+	{ "GetProperties",	"",	"a{sv}aa{sv}",	ci_get_cells },
+	{ }
+};
+
+static GDBusSignalTable ci_signals[] = {
+	{ }
+};
+
+int ofono_cell_info_driver_register(struct ofono_cell_info_driver *driver)
+{
+	DBG("driver: %p, name: %s", driver, driver->name);
+
+	if (driver->probe == NULL)
+		return -EINVAL;
+
+	g_drivers = g_slist_prepend(g_drivers, (void *) driver);
+
+	return 0;
+}
+
+void ofono_cell_info_driver_unregister(struct ofono_cell_info_driver *driver)
+{
+	DBG("driver: %p, name: %s", driver, driver->name);
+
+	g_drivers = g_slist_remove(g_drivers, (void *) driver);
+}
+
+void ofono_cell_info_remove(struct ofono_cell_info *ci)
+{
+	__ofono_atom_free(ci->atom);
+}
+
+static void cell_info_unregister(struct ofono_atom *atom)
+{
+	struct ofono_cell_info *ci = __ofono_atom_get_data(atom);
+	const char *path = __ofono_atom_get_path(ci->atom);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(ci->atom);
+
+	ofono_modem_remove_interface(modem, OFONO_CELL_INFO_INTERFACE);
+
+	if (!g_dbus_unregister_interface(conn, path, OFONO_CELL_INFO_INTERFACE))
+		ofono_error("Failed to unregister interface %s",
+				OFONO_CELL_INFO_INTERFACE);
+}
+
+static void cell_info_remove(struct ofono_atom *atom)
+{
+	struct ofono_cell_info *ci = __ofono_atom_get_data(atom);
+	DBG("atom: %p", atom);
+
+	if (ci == NULL)
+		return;
+
+	if (ci->driver && ci->driver->remove)
+		ci->driver->remove(ci);
+
+	g_free(ci);
+}
+
+void ofono_cell_info_register(struct ofono_cell_info *ci)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(ci->atom);
+	const char *path = __ofono_atom_get_path(ci->atom);
+
+	if (!g_dbus_register_interface(conn, path,
+					OFONO_CELL_INFO_INTERFACE,
+					ci_methods, ci_signals, NULL,
+					ci, NULL)) {
+		ofono_error("Could not create %s interface",
+					OFONO_CELL_INFO_INTERFACE);
+
+		return;
+	}
+
+	ofono_modem_add_interface(modem, OFONO_CELL_INFO_INTERFACE);
+
+	__ofono_atom_register(ci->atom, cell_info_unregister);
+}
+
+struct ofono_cell_info *ofono_cell_info_create(struct ofono_modem *modem,
+					unsigned int vendor,
+					const char *driver,
+					void *data)
+{
+	struct ofono_cell_info *ci;
+	GSList *l;
+
+	if (driver == NULL)
+		return NULL;
+
+	ci = g_try_new0(struct ofono_cell_info, 1);
+	if (ci == NULL)
+		return NULL;
+
+	ci->atom = __ofono_modem_add_atom(modem,
+					OFONO_ATOM_TYPE_CELL_INFO,
+					cell_info_remove, ci);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_cell_info_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver))
+			continue;
+
+		if (drv->probe(ci, vendor, data) < 0)
+			continue;
+
+		ci->driver = drv;
+		break;
+	}
+
+	return ci;
+}
+
+void *ofono_cell_info_get_data(struct ofono_cell_info *ci)
+{
+	return ci->driver_data;
+}
+
+void ofono_cell_info_set_data(struct ofono_cell_info *ci, void *cid)
+{
+	ci->driver_data = cid;
+}
+
+static int append_geran_meta_data(DBusMessageIter *iter,
+		 struct ofono_cell_info_results *ci)
+{
+	DBusMessageIter iter_array;
+	const char *type = "GERAN";
+	dbus_message_iter_open_container(iter,
+					DBUS_TYPE_ARRAY,
+					"{sv}",
+					&iter_array);
+
+	ofono_dbus_dict_append(&iter_array,
+				"Type",
+				DBUS_TYPE_STRING,
+				&type);
+	ofono_dbus_dict_append(&iter_array,
+				"MNC",
+				DBUS_TYPE_UINT32,
+				&ci->mnc);
+	ofono_dbus_dict_append(&iter_array,
+				"MCC",
+				DBUS_TYPE_UINT32,
+				&ci->mcc);
+	ofono_dbus_dict_append(&iter_array,
+				"LAC",
+				DBUS_TYPE_UINT32,
+				&ci->gsm.lac);
+	ofono_dbus_dict_append(&iter_array,
+				"CI",
+				DBUS_TYPE_UINT32,
+				&ci->gsm.ci);
+
+	if (ci->gsm.ta != -1)
+		ofono_dbus_dict_append(&iter_array,
+					"TA",
+					DBUS_TYPE_UINT32,
+					&ci->gsm.ta);
+
+	dbus_message_iter_close_container(iter, &iter_array);
+
+	return 0;
+
+}
+
+static void add_geran_neighbor(DBusMessageIter *iter,
+		struct geran_neigh_cell *cell)
+{
+	DBusMessageIter iter_array;
+
+	dbus_message_iter_open_container(iter,
+				DBUS_TYPE_ARRAY,
+				"{sv}",
+				&iter_array);
+	ofono_dbus_dict_append(&iter_array,
+				"ARFCN",
+				DBUS_TYPE_UINT32,
+				&cell->arfcn);
+	ofono_dbus_dict_append(&iter_array,
+				"BSIC",
+				DBUS_TYPE_UINT32,
+				&cell->bsic);
+	ofono_dbus_dict_append(&iter_array,
+				"RXLEV",
+				DBUS_TYPE_UINT32,
+				&cell->rxlev);
+
+	dbus_message_iter_close_container(iter, &iter_array);
+}
+
+static int fill_geran_cell_info(DBusMessage *msg,
+		  struct ofono_cell_info_results *ci)
+{
+	DBusMessageIter iter, iter_array;
+	int i;
+
+	dbus_message_iter_init_append(msg, &iter);
+	append_geran_meta_data(&iter, ci);
+
+	dbus_message_iter_open_container(&iter,
+					DBUS_TYPE_ARRAY,
+					"a{sv}",
+					&iter_array);
+
+	for (i = 0; i < ci->gsm.no_cells; ++i)
+		add_geran_neighbor(&iter_array, &ci->gsm.nmr[i]);
+
+	dbus_message_iter_close_container(&iter, &iter_array);
+
+	return 0;
+}
+
+static int append_utra_cell_list(DBusMessageIter *iter,
+		struct cell_measured_results *cmr, int no_cells)
+{
+	int j;
+
+	for (j = 0; j < no_cells; ++j) {
+
+		ofono_dbus_dict_append(iter,
+					"SC",
+					DBUS_TYPE_UINT32,
+					&cmr[j].sc);
+
+		if (cmr[j].ucid != OFONO_CI_FIELD_UNDEFINED)
+			ofono_dbus_dict_append(iter,
+					"UCID",
+					DBUS_TYPE_UINT32,
+					&cmr[j].ucid);
+
+		if (cmr[j].ecn0 != OFONO_CI_FIELD_UNDEFINED)
+			ofono_dbus_dict_append(iter,
+					"ECN0",
+					DBUS_TYPE_UINT32,
+					&cmr[j].ecn0);
+
+		if (cmr[j].rscp != OFONO_CI_FIELD_UNDEFINED)
+			ofono_dbus_dict_append(iter,
+					"RSCP",
+					DBUS_TYPE_INT32,
+					&cmr[j].rscp);
+
+		if (cmr[j].pathloss != OFONO_CI_FIELD_UNDEFINED)
+			ofono_dbus_dict_append(iter,
+					"Pathloss",
+					DBUS_TYPE_UINT32,
+					&cmr[j].rscp);
+
+	}
+
+	return 0;
+}
+
+static int append_utra_freq_info(DBusMessageIter *iter,
+		struct measured_results_list *mrl)
+{
+	ofono_dbus_dict_append(iter,
+				"RSSI",
+				DBUS_TYPE_INT32,
+				&mrl->rssi);
+
+	ofono_dbus_dict_append(iter,
+				"UARFCN-DL",
+				DBUS_TYPE_UINT32,
+				&mrl->dl_freq);
+	if (mrl->ul_freq != OFONO_CI_FIELD_UNDEFINED)
+		ofono_dbus_dict_append(iter,
+				"UARFCN-UL",
+				DBUS_TYPE_UINT32,
+				&mrl->ul_freq);
+
+	return 0;
+}
+
+static int append_utra_neighbors(DBusMessageIter *iter,
+			struct wcdma *wcdma)
+{
+	DBusMessageIter iter_array, iter_array_array;
+	int i;
+
+	dbus_message_iter_open_container(iter,
+					DBUS_TYPE_ARRAY,
+					"a{sv}",
+					&iter_array);
+
+	for (i = 0; i < wcdma->no_freq; ++i) {
+		dbus_message_iter_open_container(&iter_array,
+					DBUS_TYPE_ARRAY,
+					"{sv}",
+					&iter_array_array);
+		append_utra_freq_info(&iter_array_array, &wcdma->mrl[i]);
+		append_utra_cell_list(&iter_array_array, wcdma->mrl[i].cmr,
+					wcdma->mrl[i].no_cells);
+
+		dbus_message_iter_close_container(&iter_array,
+				&iter_array_array);
+	}
+
+	dbus_message_iter_close_container(iter, &iter_array);
+
+	return 0;
+}
+
+static void append_utran_meta_data(DBusMessageIter *iter,
+		struct ofono_cell_info_results *ci)
+{
+	DBusMessageIter iter_array;
+	const char *type = "UTRA-FDD";
+
+	dbus_message_iter_open_container(iter,
+					DBUS_TYPE_ARRAY,
+					"{sv}",
+					&iter_array);
+
+	ofono_dbus_dict_append(&iter_array,
+				"Type",
+				DBUS_TYPE_STRING,
+				&type);
+	ofono_dbus_dict_append(&iter_array,
+				"MNC",
+				DBUS_TYPE_INT32,
+				&ci->mnc);
+	ofono_dbus_dict_append(&iter_array,
+				"MCC",
+				DBUS_TYPE_UINT32,
+				&ci->mcc);
+	ofono_dbus_dict_append(&iter_array,
+				"UCID",
+				DBUS_TYPE_UINT32,
+				&ci->wcdma.ucid);
+	ofono_dbus_dict_append(&iter_array,
+				"SC",
+				DBUS_TYPE_UINT32,
+				&ci->wcdma.sc);
+	ofono_dbus_dict_append(&iter_array,
+				"UARFCN-DL",
+				DBUS_TYPE_UINT32,
+				&ci->wcdma.dl_freq);
+
+	if (ci->wcdma.ul_freq != OFONO_CI_FIELD_UNDEFINED) {
+		ofono_dbus_dict_append(&iter_array,
+				"UARFCN-UL",
+				DBUS_TYPE_UINT32,
+				&ci->wcdma.ul_freq);
+	}
+
+	dbus_message_iter_close_container(iter, &iter_array);
+}
+
+static int fill_utra_cell_info(DBusMessage *msg,
+			struct ofono_cell_info_results *ci)
+{
+	DBusMessageIter iter;
+
+	dbus_message_iter_init_append(msg, &iter);
+	append_utran_meta_data(&iter, ci);
+	append_utra_neighbors(&iter, &ci->wcdma);
+
+	return 0;
+}
+
+void ofono_cell_info_query_cb(const struct ofono_error *error,
+		struct ofono_cell_info_results ci_results, void *data)
+{
+	struct ofono_cell_info *ci = data;
+	DBusMessage *msg = ci->pending;
+	DBusMessage *reply;
+	int status;
+
+	DBG("");
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_error("Neighbor cell info query failed");
+		goto error;
+	}
+
+	reply = dbus_message_new_method_return(msg);
+	if (reply == NULL) {
+		ofono_error("Failed to create response");
+		goto error;
+	}
+
+	if (ci_results.rat == OFONO_CELL_TYPE_GERAN) {
+		ofono_debug("GSM CELL INFO RECEIVED");
+
+		status = fill_geran_cell_info(msg, &ci_results);
+		if (status != 0) {
+			ofono_error("Failed to fill geran info");
+			goto error;
+		}
+
+	} else if (ci_results.rat == OFONO_CELL_TYPE_UTRA_FDD) {
+		ofono_debug("WCDMA CELL INFO RECEIVED");
+
+		status = fill_utra_cell_info(msg, &ci_results);
+		if (status != 0) {
+			ofono_error("Failed to fill utra info");
+			goto error;
+		}
+
+	} else {
+		ofono_error("Unrecognized cell type.");
+		goto error;
+	}
+
+	__ofono_dbus_pending_reply(&msg, reply);
+	return;
+
+error:
+	reply = __ofono_error_failed(msg);
+	__ofono_dbus_pending_reply(&msg, reply);
+	return;
+
+}
+
+static DBusMessage *ci_get_cells(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	struct ofono_cell_info *ci = data;
+	DBG("");
+
+	ci->pending = dbus_message_ref(msg);
+	ci->driver->query(ci, ofono_cell_info_query_cb, ci->driver_data);
+
+	return NULL;
+}
+
-- 
1.7.1

[RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Antti Paila]

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

---
 include/cell-info.h |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 include/cell-info.h

diff --git a/include/cell-info.h b/include/cell-info.h
new file mode 100644
index 0000000..3941e69
--- /dev/null
+++ b/include/cell-info.h
@@ -0,0 +1,133 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __OFONO_CELL_INFO_H
+#define __OFONO_CELL_INFO_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <ofono/types.h>
+
+struct ofono_cell_info;
+
+#define OFONO_CI_FIELD_UNDEFINED -1001
+#define OFONO_MAX_NMR_COUNT 15
+#define OFONO_MAX_MEASURED_CELL_COUNT 32
+#define OFONO_MAX_MEAS_RES_LIST_COUNT 8
+
+
+enum ofono_cell_type {
+	OFONO_CELL_TYPE_GERAN,
+	OFONO_CELL_TYPE_UTRA_FDD
+};
+
+enum UTRA_MEAS_TYPE {
+	UTRA_MEAS_TYPE_INTER_FREQ,
+	UTRA_MEAS_TYPE_INTRA_FREQ,
+};
+
+struct gsm {
+	int lac;
+	int ci;
+	int ta;
+	int no_cells;
+	struct geran_neigh_cell{
+		int arfcn;
+		int bsic;
+		int rxlev;
+
+	} nmr[OFONO_MAX_NMR_COUNT];
+};
+
+struct cell_measured_results {
+	int ucid;
+	int sc;
+	int ecn0;
+	int rscp;
+	int pathloss;
+};
+
+struct measured_results_list {
+	int dl_freq;
+	int ul_freq;
+	int rssi;
+	int no_cells;
+	struct cell_measured_results cmr[OFONO_MAX_MEASURED_CELL_COUNT];
+};
+
+struct wcdma {
+	int ucid;
+	int dl_freq;
+	int ul_freq;
+	int sc;
+	int no_freq;
+	struct measured_results_list mrl[OFONO_MAX_MEAS_RES_LIST_COUNT];
+
+};
+
+struct ofono_cell_info_results {
+	enum ofono_cell_type rat;
+	int mcc;
+	int mnc;
+	union {
+		struct gsm gsm;
+		struct wcdma wcdma;
+	};
+
+};
+
+
+typedef void (*ofono_cell_info_query_cb_t)(const struct ofono_error *error,
+		struct ofono_cell_info_results results, void *data);
+
+struct ofono_cell_info_driver {
+	const char *name;
+	int (*probe)(struct ofono_cell_info *ci,
+			unsigned int vendor,
+			void *data);
+	void (*remove)(struct ofono_cell_info *ci);
+	void (*query)(struct ofono_cell_info *ci,
+			ofono_cell_info_query_cb_t,
+			void *data);
+};
+
+struct ofono_cell_info *ofono_cell_info_create(struct ofono_modem *modem,
+					unsigned int vendor,
+					const char *driver,
+					void *data);
+
+void ofono_cell_info_register(struct ofono_cell_info *ci);
+void ofono_cell_info_remove(struct ofono_cell_info *ci);
+int ofono_cell_info_driver_register(struct ofono_cell_info_driver *driver);
+void ofono_cell_info_driver_unregister(struct ofono_cell_info_driver *driver);
+void *ofono_cell_info_get_data(struct ofono_cell_info *ci);
+void ofono_cell_info_set_data(struct ofono_cell_info *ci, void *cid);
+void ofono_cell_info_query_cb(const struct ofono_error *error,
+			struct ofono_cell_info_results results, void *data);
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_CELL_INFO_H */
-- 
1.7.1

[RFC PATCH 3/5 v2] cell-info: Interface declarations

[Antti Paila]

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

---
 include/dbus.h |    1 +
 src/ofono.h    |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/include/dbus.h b/include/dbus.h
index c527515..98839cc 100644
--- a/include/dbus.h
+++ b/include/dbus.h
@@ -45,6 +45,7 @@ extern "C" {
 #define OFONO_MESSAGE_WAITING_INTERFACE "org.ofono.MessageWaiting"
 #define OFONO_NETWORK_REGISTRATION_INTERFACE "org.ofono.NetworkRegistration"
 #define OFONO_NETWORK_OPERATOR_INTERFACE "org.ofono.NetworkOperator"
+#define OFONO_CELL_INFO_INTERFACE "org.ofono.CellInfo"
 #define OFONO_PHONEBOOK_INTERFACE "org.ofono.Phonebook"
 #define OFONO_RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
 #define OFONO_AUDIO_SETTINGS_INTERFACE "org.ofono.AudioSettings"
diff --git a/src/ofono.h b/src/ofono.h
index 792134b..cd89a8f 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -126,6 +126,7 @@ enum ofono_atom_type {
 	OFONO_ATOM_TYPE_STK = 20,
 	OFONO_ATOM_TYPE_NETTIME = 21,
 	OFONO_ATOM_TYPE_CTM = 22,
+	OFONO_ATOM_TYPE_CELL_INFO = 23,
 };
 
 enum ofono_atom_watch_condition {
-- 
1.7.1

[RFC PATCH 4/5 v2] cell-info: New files included for compilation

[Antti Paila]

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

---
 Makefile.am |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 4a919ab..e433861 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -14,7 +14,7 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/gprs.h include/gprs-context.h \
 			include/radio-settings.h include/stk.h \
 			include/audio-settings.h include/nettime.h \
-			include/ctm.h
+			include/ctm.h include/cell-info.h
 
 nodist_include_HEADERS = include/version.h
 
@@ -320,7 +320,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \
 			src/radio-settings.c src/stkutil.h src/stkutil.c \
 			src/nettime.c src/stkagent.c src/stkagent.h \
 			src/simfs.c src/simfs.h src/audio-settings.c \
-			src/smsagent.c src/smsagent.h src/ctm.c
+			src/smsagent.c src/smsagent.h src/ctm.c src/cell-info.c
 
 src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
 
-- 
1.7.1

[RFC PATCH 5/5 v2] cell-info: Documentation

[Antti Paila]

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

---
 doc/cell-info.txt |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 110 insertions(+), 0 deletions(-)
 create mode 100644 doc/cell-info.txt

diff --git a/doc/cell-info.txt b/doc/cell-info.txt
new file mode 100644
index 0000000..9144eb7
--- /dev/null
+++ b/doc/cell-info.txt
@@ -0,0 +1,110 @@
+Cell Info hierarchy
+===================
+
+Service		org.ofono
+Interface	org.ofono.CellInfo
+Object path	[variable prefix]/{modem0,modem1,...}
+
+Methods		a{sv}aa{sv} GetProperties()
+
+			Calling this procedure returns properties of serving
+			and neighbouring cells in GSM or WCDMA networks. This
+			information can be used to determine current location
+			using triangulation over neighbouring cell tower
+			locations and estimated distances.
+
+			The return value consists of two parts: 1) dictionary
+			of common information about network and serving cell 2)
+			array of dictionaries containing measurement results
+			of neighbouring cells. The contents of the dictionaries
+			follow the specification OMA-TS-ULP-V2_0-20100816-C for
+			user plane Location and is described in properties
+			section.
+
+Properties
+
+a{sv]		string Type
+
+			Radio access type.
+			Values: "GERAN", "UTRA-FDD
+
+		uint32 MCC
+			Mobile Country Code of serving cell. Possible values:
+			Values: 0...999"
+
+		uint32 MNC
+			Mobile Network Code of serving cell.
+			Values: 0...999"
+
+		uint32 LAC [GERAN]
+			Location area code of serving cell.
+			Values: 0...65535
+
+		uint32 CI [GERAN]
+			Cell Id of serving cell.
+			Values: 0...65535
+
+		uint32 TA [GERAN, OPTIONAL]
+			Timing advance.
+			Values: 0...63
+
+		uint32 UCID [UTRA-FDD]
+			Serving WCDMA unique cell ID.
+			Values: 0...268435455
+
+		uint32 SC [UTRA-FDD]
+			Primary scrambling code.
+			Values: 0...511
+
+		uint32 UARFCN-DL [UTRA-FDD]
+			Downlink UARFCN of serving cell.
+			Values: 0...16383
+
+		uint32 UARFCN-UL [UTRA-FDD, OPTIONAL]
+			Uplink UARFCN.
+			Values: 0...16383
+
+
+aa{sv}		uint32 ARFCN [GERAN]
+			Absolute radio frequency channel number.
+			 Values: 0...1023
+
+		uint32 BSIC [GERAN]
+			Base station identity code.
+			Values: 0...63.
+
+		uint32 RXLEV [GERAN]
+			Measured power of the channel.
+			Values: 0...63
+
+		int32 RSSI [UTRA-FDD]
+			RX power level in dBm.
+			Values: -47...-110
+
+		uint32 UARFCN-DL [UTRA-FDD]
+			Downlink UARFCN.
+			Values: 0...16383
+
+		uint32 UARFCN-UL [UTRA-FDD, OPTIONAL]
+			Uplink UARFCN.
+			Values: 0...16383
+
+		uint32 SC [UTRA-FDD]
+			Primary scrambling code.
+			Values: 0...511
+
+		uint32 UCID [UTRA-FDD, OPTIONAL]
+			Unique cell ID.
+			Values: 0...268435455
+
+		uint32 ECN0 [UTRA-FDD, OPTIONAL]
+			 CPICH RX energy per chip over noise density in dB.
+			 Values: 0...63
+
+		int32 RSCP [UTRA-FDD, OPTIONAL]
+			CPICH RX carrier power in dBm
+			Values: -4...127
+
+		uint32 Pathloss [UTRA-FDD, OPTIONAL]
+			Measured path loss in dB.
+			Values: 46...173
-- 
1.7.1

Re: [RFC PATCH 1/5 v2] cell-info: Atom for ECID info of neighb cells

[Aki Niemi]

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

Hi Antti,

On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> ---
>  src/cell-info.c |  484 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 484 insertions(+), 0 deletions(-)
>  create mode 100644 src/cell-info.c
> 
> diff --git a/src/cell-info.c b/src/cell-info.c
> new file mode 100644
> index 0000000..2289ce1
> --- /dev/null
> +++ b/src/cell-info.c
> @@ -0,0 +1,484 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.

Use Nokia copyright statement here.

> + *  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 <stdio.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +#include <sys/time.h>
> +
> +#include "ofono.h"
> +
> +#include "common.h"
> +#include "util.h"
> +#include "ofono/cell-info.h"

This can't be the first patch in the set since the file included here is
only added in the second patch.

Cheers,
Aki

Re: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Aki Niemi]

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

Hi Antti,

On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> ---
>  include/cell-info.h |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 133 insertions(+), 0 deletions(-)
>  create mode 100644 include/cell-info.h
> 
> diff --git a/include/cell-info.h b/include/cell-info.h
> new file mode 100644
> index 0000000..3941e69
> --- /dev/null
> +++ b/include/cell-info.h
> @@ -0,0 +1,133 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifndef __OFONO_CELL_INFO_H
> +#define __OFONO_CELL_INFO_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <ofono/types.h>
> +
> +struct ofono_cell_info;
> +
> +#define OFONO_CI_FIELD_UNDEFINED -1001
> +#define OFONO_MAX_NMR_COUNT 15
> +#define OFONO_MAX_MEASURED_CELL_COUNT 32
> +#define OFONO_MAX_MEAS_RES_LIST_COUNT 8
> +
> +
> +enum ofono_cell_type {
> +	OFONO_CELL_TYPE_GERAN,
> +	OFONO_CELL_TYPE_UTRA_FDD
> +};
> +
> +enum UTRA_MEAS_TYPE {
> +	UTRA_MEAS_TYPE_INTER_FREQ,
> +	UTRA_MEAS_TYPE_INTRA_FREQ,
> +};
> +
> +struct gsm {
> +	int lac;
> +	int ci;
> +	int ta;
> +	int no_cells;
> +	struct geran_neigh_cell{
> +		int arfcn;
> +		int bsic;
> +		int rxlev;
> +
> +	} nmr[OFONO_MAX_NMR_COUNT];
> +};
> +
> +struct cell_measured_results {
> +	int ucid;
> +	int sc;
> +	int ecn0;
> +	int rscp;
> +	int pathloss;
> +};
> +
> +struct measured_results_list {
> +	int dl_freq;
> +	int ul_freq;
> +	int rssi;
> +	int no_cells;
> +	struct cell_measured_results cmr[OFONO_MAX_MEASURED_CELL_COUNT];
> +};
> +
> +struct wcdma {
> +	int ucid;
> +	int dl_freq;
> +	int ul_freq;
> +	int sc;
> +	int no_freq;
> +	struct measured_results_list mrl[OFONO_MAX_MEAS_RES_LIST_COUNT];
> +
> +};

Since these things are called GERAN and UTRAN over D-Bus, it would make
sense to use the same naming here instead of struct gsm and struct
wcdma.

> +struct ofono_cell_info_results {
> +	enum ofono_cell_type rat;
> +	int mcc;
> +	int mnc;
> +	union {
> +		struct gsm gsm;
> +		struct wcdma wcdma;
> +	};
> +
> +};
> +
> +
> +typedef void (*ofono_cell_info_query_cb_t)(const struct ofono_error *error,
> +		struct ofono_cell_info_results results, void *data);

The results is a pointer.

> +struct ofono_cell_info_driver {
> +	const char *name;
> +	int (*probe)(struct ofono_cell_info *ci,
> +			unsigned int vendor,
> +			void *data);
> +	void (*remove)(struct ofono_cell_info *ci);
> +	void (*query)(struct ofono_cell_info *ci,
> +			ofono_cell_info_query_cb_t,
> +			void *data);
> +};
> +
> +struct ofono_cell_info *ofono_cell_info_create(struct ofono_modem *modem,
> +					unsigned int vendor,
> +					const char *driver,
> +					void *data);
> +
> +void ofono_cell_info_register(struct ofono_cell_info *ci);
> +void ofono_cell_info_remove(struct ofono_cell_info *ci);
> +int ofono_cell_info_driver_register(struct ofono_cell_info_driver *driver);
> +void ofono_cell_info_driver_unregister(struct ofono_cell_info_driver *driver);
> +void *ofono_cell_info_get_data(struct ofono_cell_info *ci);
> +void ofono_cell_info_set_data(struct ofono_cell_info *ci, void *cid);
> +void ofono_cell_info_query_cb(const struct ofono_error *error,
> +			struct ofono_cell_info_results results, void *data);

Why is this last one public?

Cheers,
Aki

Re: [RFC PATCH 5/5 v2] cell-info: Documentation

[Aki Niemi]

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

Hi Antti,

On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> ---
>  doc/cell-info.txt |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 110 insertions(+), 0 deletions(-)
>  create mode 100644 doc/cell-info.txt
> 
> diff --git a/doc/cell-info.txt b/doc/cell-info.txt
> new file mode 100644
> index 0000000..9144eb7
> --- /dev/null
> +++ b/doc/cell-info.txt
> @@ -0,0 +1,110 @@
> +Cell Info hierarchy
> +===================
> +
> +Service		org.ofono
> +Interface	org.ofono.CellInfo
> +Object path	[variable prefix]/{modem0,modem1,...}
> +
> +Methods		a{sv}aa{sv} GetProperties()
> +
> +			Calling this procedure returns properties of serving
> +			and neighbouring cells in GSM or WCDMA networks. This
> +			information can be used to determine current location
> +			using triangulation over neighbouring cell tower
> +			locations and estimated distances.
> +
> +			The return value consists of two parts: 1) dictionary
> +			of common information about network and serving cell 2)
> +			array of dictionaries containing measurement results
> +			of neighbouring cells. The contents of the dictionaries
> +			follow the specification OMA-TS-ULP-V2_0-20100816-C for
> +			user plane Location and is described in properties
> +			section.
> +
> +Properties
> +
> +a{sv]		string Type
> +
> +			Radio access type.
> +			Values: "GERAN", "UTRA-FDD

How about some brief explanation of what these mean? Also, missing end
quotes on UTRA-FDD, and the naming convention has been to use lower case
for string values.

Perhaps along these lines:

string Type

	Radio access network type of neighbor cell information.

	The possible values are:
		"geran"   Measurement results are for the GSM EDGE Radio
			  Access Network
		"utran"   Measurement results are for UMTS Radio Access
			  Network

> +		uint32 MCC
> +			Mobile Country Code of serving cell. Possible values:
> +			Values: 0...999"
> +
> +		uint32 MNC
> +			Mobile Network Code of serving cell.
> +			Values: 0...999"

These need to be MobileCountryCode and MobileNetworkCode, as elsewhere
in the API. Also, why not string like in the network API?

> +		uint32 LAC [GERAN]
> +			Location area code of serving cell.
> +			Values: 0...65535
> +
> +		uint32 CI [GERAN]
> +			Cell Id of serving cell.
> +			Values: 0...65535
> +
> +		uint32 TA [GERAN, OPTIONAL]
> +			Timing advance.
> +			Values: 0...63
> +
> +		uint32 UCID [UTRA-FDD]
> +			Serving WCDMA unique cell ID.
> +			Values: 0...268435455

For the LAC and CI, naming could be borrowed from the network API here
as well. Though I understand the need to conform to the SUPL specs here.

Cheers,
Aki

Re: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Antti Paila]

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

On Wed, 2010-12-22 at 20:45 +0200, ext Aki Niemi wrote:
> Hi Antti,
> 
> On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> > ---
> >  include/cell-info.h |  133 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 133 insertions(+), 0 deletions(-)
> >  create mode 100644 include/cell-info.h
> > 
> > diff --git a/include/cell-info.h b/include/cell-info.h
> > new file mode 100644
> > index 0000000..3941e69
> > --- /dev/null
> > +++ b/include/cell-info.h
> > @@ -0,0 +1,133 @@
> > +/*
> > + *
> > + *  oFono - Open Source Telephony
> > + *
> > + *  Copyright (C) 2008-2010  Intel Corporation. All rights reserved.
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License version 2 as
> > + *  published by the Free Software Foundation.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with this program; if not, write to the Free Software
> > + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> > + *
> > + */
> > +
> > +#ifndef __OFONO_CELL_INFO_H
> > +#define __OFONO_CELL_INFO_H
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +
> > +#include <ofono/types.h>
> > +
> > +struct ofono_cell_info;
> > +
> > +#define OFONO_CI_FIELD_UNDEFINED -1001
> > +#define OFONO_MAX_NMR_COUNT 15
> > +#define OFONO_MAX_MEASURED_CELL_COUNT 32
> > +#define OFONO_MAX_MEAS_RES_LIST_COUNT 8
> > +
> > +
> > +enum ofono_cell_type {
> > +	OFONO_CELL_TYPE_GERAN,
> > +	OFONO_CELL_TYPE_UTRA_FDD
> > +};
> > +
> > +enum UTRA_MEAS_TYPE {
> > +	UTRA_MEAS_TYPE_INTER_FREQ,
> > +	UTRA_MEAS_TYPE_INTRA_FREQ,
> > +};
> > +
> > +struct gsm {
> > +	int lac;
> > +	int ci;
> > +	int ta;
> > +	int no_cells;
> > +	struct geran_neigh_cell{
> > +		int arfcn;
> > +		int bsic;
> > +		int rxlev;
> > +
> > +	} nmr[OFONO_MAX_NMR_COUNT];
> > +};
> > +
> > +struct cell_measured_results {
> > +	int ucid;
> > +	int sc;
> > +	int ecn0;
> > +	int rscp;
> > +	int pathloss;
> > +};
> > +
> > +struct measured_results_list {
> > +	int dl_freq;
> > +	int ul_freq;
> > +	int rssi;
> > +	int no_cells;
> > +	struct cell_measured_results cmr[OFONO_MAX_MEASURED_CELL_COUNT];
> > +};
> > +
> > +struct wcdma {
> > +	int ucid;
> > +	int dl_freq;
> > +	int ul_freq;
> > +	int sc;
> > +	int no_freq;
> > +	struct measured_results_list mrl[OFONO_MAX_MEAS_RES_LIST_COUNT];
> > +
> > +};
> 
> Since these things are called GERAN and UTRAN over D-Bus, it would make
> sense to use the same naming here instead of struct gsm and struct
> wcdma.
> 
> > +struct ofono_cell_info_results {
> > +	enum ofono_cell_type rat;
> > +	int mcc;
> > +	int mnc;
> > +	union {
> > +		struct gsm gsm;
> > +		struct wcdma wcdma;
> > +	};
> > +
> > +};
> > +
> > +
> > +typedef void (*ofono_cell_info_query_cb_t)(const struct ofono_error *error,
> > +		struct ofono_cell_info_results results, void *data);
> 
> The results is a pointer.

Should be.

> > +struct ofono_cell_info_driver {
> > +	const char *name;
> > +	int (*probe)(struct ofono_cell_info *ci,
> > +			unsigned int vendor,
> > +			void *data);
> > +	void (*remove)(struct ofono_cell_info *ci);
> > +	void (*query)(struct ofono_cell_info *ci,
> > +			ofono_cell_info_query_cb_t,
> > +			void *data);
> > +};
> > +
> > +struct ofono_cell_info *ofono_cell_info_create(struct ofono_modem *modem,
> > +					unsigned int vendor,
> > +					const char *driver,
> > +					void *data);
> > +
> > +void ofono_cell_info_register(struct ofono_cell_info *ci);
> > +void ofono_cell_info_remove(struct ofono_cell_info *ci);
> > +int ofono_cell_info_driver_register(struct ofono_cell_info_driver *driver);
> > +void ofono_cell_info_driver_unregister(struct ofono_cell_info_driver *driver);
> > +void *ofono_cell_info_get_data(struct ofono_cell_info *ci);
> > +void ofono_cell_info_set_data(struct ofono_cell_info *ci, void *cid);
> > +void ofono_cell_info_query_cb(const struct ofono_error *error,
> > +			struct ofono_cell_info_results results, void *data);
> 
> Why is this last one public?

You are right. It doesn't need to be public. Thanks for the comments.

Best Regards,
 Antti

RE: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Wei, Jun]

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

Hi Antti,

On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> ---
>  include/cell-info.h |  133 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 133 insertions(+), 0 deletions(-)  create mode 
> 100644 include/cell-info.h
> 
> diff --git a/include/cell-info.h b/include/cell-info.h new file mode 
> 100644 index 0000000..3941e69
> --- /dev/null
> +++ b/include/cell-info.h
> @@ -0,0 +1,133 @@
> +/*
>  ...
>  ...
>  ...
> +struct ofono_cell_info_driver {
> +	const char *name;
> +	int (*probe)(struct ofono_cell_info *ci,
> +			unsigned int vendor,
> +			void *data);
> +	void (*remove)(struct ofono_cell_info *ci);
> +	void (*query)(struct ofono_cell_info *ci,
> +			ofono_cell_info_query_cb_t,
> +			void *data);
> +};
> +

To be aligning with the other header files, do we need to add one 'cb' after 'ofono_cell_info_query_cb_t' here?


Regards,

Wei,Jun

Re: [RFC PATCH 5/5 v2] cell-info: Documentation

[Antti Paila]

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

Hi Aki,

On Wed, 2010-12-22 at 20:46 +0200, ext Aki Niemi wrote:
> Hi Antti,
> 
> On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> > ---
> >  doc/cell-info.txt |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 110 insertions(+), 0 deletions(-)
> >  create mode 100644 doc/cell-info.txt
> > 
> > diff --git a/doc/cell-info.txt b/doc/cell-info.txt
> > new file mode 100644
> > index 0000000..9144eb7
> > --- /dev/null
> > +++ b/doc/cell-info.txt
> > @@ -0,0 +1,110 @@
> > +Cell Info hierarchy
> > +===================
> > +
> > +Service		org.ofono
> > +Interface	org.ofono.CellInfo
> > +Object path	[variable prefix]/{modem0,modem1,...}
> > +
> > +Methods		a{sv}aa{sv} GetProperties()
> > +
> > +			Calling this procedure returns properties of serving
> > +			and neighbouring cells in GSM or WCDMA networks. This
> > +			information can be used to determine current location
> > +			using triangulation over neighbouring cell tower
> > +			locations and estimated distances.
> > +
> > +			The return value consists of two parts: 1) dictionary
> > +			of common information about network and serving cell 2)
> > +			array of dictionaries containing measurement results
> > +			of neighbouring cells. The contents of the dictionaries
> > +			follow the specification OMA-TS-ULP-V2_0-20100816-C for
> > +			user plane Location and is described in properties
> > +			section.
> > +
> > +Properties
> > +
> > +a{sv]		string Type
> > +
> > +			Radio access type.
> > +			Values: "GERAN", "UTRA-FDD
> 
> How about some brief explanation of what these mean? Also, missing end
> quotes on UTRA-FDD, and the naming convention has been to use lower case
> for string values.
> 
> Perhaps along these lines:
> 
> string Type
> 
> 	Radio access network type of neighbor cell information.
> 
> 	The possible values are:
> 		"geran"   Measurement results are for the GSM EDGE Radio
> 			  Access Network
> 		"utran"   Measurement results are for UMTS Radio Access
> 			  Network
> 

I'll include this to doc.

> > +		uint32 MCC
> > +			Mobile Country Code of serving cell. Possible values:
> > +			Values: 0...999"
> > +
> > +		uint32 MNC
> > +			Mobile Network Code of serving cell.
> > +			Values: 0...999"
> 
> These need to be MobileCountryCode and MobileNetworkCode, as elsewhere
> in the API. Also, why not string like in the network API?

If convention is to use full names, then so be it. However, MNC and MCC
are so widely used acronyms in 3GPP specifications and generally in
telecommunication that chances for confusing is minimal. The three
letter form is also more compact and more convenient to use.

What is the benefit of representing MNC and MCC as strings? Both codes
consist of digits so to me a natural representation is integer of some
sort.   

> > +		uint32 LAC [GERAN]
> > +			Location area code of serving cell.
> > +			Values: 0...65535
> > +
> > +		uint32 CI [GERAN]
> > +			Cell Id of serving cell.
> > +			Values: 0...65535
> > +
> > +		uint32 TA [GERAN, OPTIONAL]
> > +			Timing advance.
> > +			Values: 0...63
> > +
> > +		uint32 UCID [UTRA-FDD]
> > +			Serving WCDMA unique cell ID.
> > +			Values: 0...268435455
> 
> For the LAC and CI, naming could be borrowed from the network API here
> as well. Though I understand the need to conform to the SUPL specs here.

This is up to discussion. Good argument for the use of acronyms is SUPL
specs and, for example, this doc that explains the content of each
field. Argument against is on the other hand ofono conventions that
should also be respected. Of course, conventions can be changed if seen
necessary.

Thanks for the comments.

Best Regards,
  Antti

RE: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Antti Paila]

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

Hi Jun,

On Thu, 2010-12-23 at 14:31 +0800, ext Wei, Jun wrote:
> Hi Antti,
> 
> On Wed, 2010-12-22 at 11:35 +0200, ext Antti Paila wrote:
> > ---
> >  include/cell-info.h |  133 
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 133 insertions(+), 0 deletions(-)  create mode 
> > 100644 include/cell-info.h
> > 
> > diff --git a/include/cell-info.h b/include/cell-info.h new file mode 
> > 100644 index 0000000..3941e69
> > --- /dev/null
> > +++ b/include/cell-info.h
> > @@ -0,0 +1,133 @@
> > +/*
> >  ...
> >  ...
> >  ...
> > +struct ofono_cell_info_driver {
> > +	const char *name;
> > +	int (*probe)(struct ofono_cell_info *ci,
> > +			unsigned int vendor,
> > +			void *data);
> > +	void (*remove)(struct ofono_cell_info *ci);
> > +	void (*query)(struct ofono_cell_info *ci,
> > +			ofono_cell_info_query_cb_t,
> > +			void *data);
> > +};
> > +
> 
> To be aligning with the other header files, do we need to add one 'cb' after 'ofono_cell_info_query_cb_t' here?

This seems to be a matter with no clear convention. In "sms.h" and
"netreg.h" some callbacks have 'cb' and some not. I agree that if within
a function's parameter list some parameters have name declared, so
should all have.

Thanks for the comment.

Best Regards,
  Antti  

RE: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Marcel Holtmann]

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

Hi Antti,

> > > ---
> > >  include/cell-info.h |  133 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 files changed, 133 insertions(+), 0 deletions(-)  create mode 
> > > 100644 include/cell-info.h
> > > 
> > > diff --git a/include/cell-info.h b/include/cell-info.h new file mode 
> > > 100644 index 0000000..3941e69
> > > --- /dev/null
> > > +++ b/include/cell-info.h
> > > @@ -0,0 +1,133 @@
> > > +/*
> > >  ...
> > >  ...
> > >  ...
> > > +struct ofono_cell_info_driver {
> > > +	const char *name;
> > > +	int (*probe)(struct ofono_cell_info *ci,
> > > +			unsigned int vendor,
> > > +			void *data);
> > > +	void (*remove)(struct ofono_cell_info *ci);
> > > +	void (*query)(struct ofono_cell_info *ci,
> > > +			ofono_cell_info_query_cb_t,
> > > +			void *data);
> > > +};
> > > +
> > 
> > To be aligning with the other header files, do we need to add one 'cb' after 'ofono_cell_info_query_cb_t' here?
> 
> This seems to be a matter with no clear convention. In "sms.h" and
> "netreg.h" some callbacks have 'cb' and some not. I agree that if within
> a function's parameter list some parameters have name declared, so
> should all have.

we are trying to be consistent, but some of these slipped through in the
initial development phase of oFono.

Once we find them, we document them in doc/coding-style.txt and then
slowly start fixing these up. This is an ongoing process and pretty hard
if your codebase reaches over 100k lines of code.

Regards

Marcel

RE: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Bastian, Waldo]

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

> +struct gsm {
> +	int lac;
> +	int ci;
> +	int ta;
> +	int no_cells;
> +	struct geran_neigh_cell{
> +		int arfcn;
> +		int bsic;
> +		int rxlev;
> +
> +	} nmr[OFONO_MAX_NMR_COUNT];
> +};

What about the frequency of the serving cell? I was under the impression that
there was a need for that one as well.

Even though the serving cell may be included already in the nmr, do you see a need 
to explicitly provide the arfcn, bsic and rxlev of the serving cell as well?

What about LAC and CI for the neighbouring cells?

Cheers,
Waldo

RE: [RFC PATCH 2/5 v2] cell-info: Header file for cell-info atom

[Antti Paila]

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

Hi Waldo,

On Fri, 2010-12-24 at 10:54 -0800, ext Bastian, Waldo wrote:
> > +struct gsm {
> > +	int lac;
> > +	int ci;
> > +	int ta;
> > +	int no_cells;
> > +	struct geran_neigh_cell{
> > +		int arfcn;
> > +		int bsic;
> > +		int rxlev;
> > +
> > +	} nmr[OFONO_MAX_NMR_COUNT];
> > +};
> 
> What about the frequency of the serving cell? I was under the impression that
> there was a need for that one as well.
> 
> Even though the serving cell may be included already in the nmr, do you see a need 
> to explicitly provide the arfcn, bsic and rxlev of the serving cell as well?

Serving cell frequency is, or should be, included in the nmr list in the
arfcn variable. I believe the arfcn, bsic and rxlev are needed for all
cells since this information is used for location determination.

> What about LAC and CI for the neighbouring cells?

As far as I know, the neighbor cell info in the above-mentioned form is
used for location specific purposes. Hence, bsic suffices to identify
base station and rxlev gives the unscaled distance from it.

If there is a reason for including LAC and CI for neighboring cells, we
can add them to the list. In that case we should probably modify the
DBUS method list to include one method for requesting location data and
another method for general identity data.

Best Regards,
  Antti 

RE: [RFC PATCH 1/5 v2] cell-info: Atom for ECID info of neighb cells

[Wei, Jun]

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

Hi Antti,

>+static GSList *g_drivers = NULL;
>+
>+static GDBusMethodTable ci_methods[] = {
>+	{ "GetProperties",	"",	"a{sv}aa{sv}",	ci_get_cells },
>+	{ }
+};
+

Do we need to add 'G_DBUS_METHOD_FLAG_ASYNC' after ci_get_cells? 


>+static DBusMessage *ci_get_cells(DBusConnection *conn, DBusMessage *msg,
>+					void *data)
>+{
>+	struct ofono_cell_info *ci = data;
>+	DBG("");
>+
>+	ci->pending = dbus_message_ref(msg);
>+	ci->driver->query(ci, ofono_cell_info_query_cb, ci->driver_data);

The 3rd parameter should be 'ci' instead of 'ci->driver_data', right?



Regards,
Wei, Jun 韦峻
Email: jun.wei(a)intel.com

RE: [RFC PATCH 1/5 v2] cell-info: Atom for ECID info of neighb cells

[Antti Paila]

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

Hi Jun,

On Tue, 2010-12-28 at 18:00 +0800, ext Wei, Jun wrote:
> Hi Antti,
> 
> >+static GSList *g_drivers = NULL;
> >+
> >+static GDBusMethodTable ci_methods[] = {
> >+	{ "GetProperties",	"",	"a{sv}aa{sv}",	ci_get_cells },
> >+	{ }
> +};
> +
> 
> Do we need to add 'G_DBUS_METHOD_FLAG_ASYNC' after ci_get_cells? 

Yes, we need. Already noticed it myself while testing :). I'll post the
next version of the patch soon which contains all sort of fixes plus
refactoring. 

> 
> >+static DBusMessage *ci_get_cells(DBusConnection *conn, DBusMessage *msg,
> >+					void *data)
> >+{
> >+	struct ofono_cell_info *ci = data;
> >+	DBG("");
> >+
> >+	ci->pending = dbus_message_ref(msg);
> >+	ci->driver->query(ci, ofono_cell_info_query_cb, ci->driver_data);
> 
> The 3rd parameter should be 'ci' instead of 'ci->driver_data', right?

Well, the first parameter is already 'ci', so maybe repetition is not
needed. The third parameter is kind of useless here anyway.

Thanks for the comments.

Best Regards,
  Antti