[PATCH 1/3] Add radio settings atom and driver API

[PATCH 1/3] Add radio settings atom and driver API

[Aki Niemi]

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

This interface exposes a rw property for radio access selection mode
setting.
---
 Makefile.am                |    6 +-
 doc/radio-settings-api.txt |   42 +++++
 include/radio-settings.h   |   74 +++++++++
 src/ofono.h                |    2 +
 src/radio-settings.c       |  372 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 494 insertions(+), 2 deletions(-)
 create mode 100644 doc/radio-settings-api.txt
 create mode 100644 include/radio-settings.h
 create mode 100644 src/radio-settings.c

diff --git a/Makefile.am b/Makefile.am
index 33339df..672ce0b 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -11,7 +11,8 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/sms.h include/sim.h include/message-waiting.h \
 			include/netreg.h include/voicecall.h include/devinfo.h \
 			include/cbs.h include/call-volume.h \
-			include/gprs.h include/gprs-context.h
+			include/gprs.h include/gprs-context.h \
+			include/radio-settings.h
 
 nodist_include_HEADERS = include/version.h
 
@@ -224,7 +225,8 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) \
 			src/phonebook.c src/history.c src/message-waiting.c \
 			src/simutil.h src/simutil.c src/storage.h \
 			src/storage.c src/cbs.c src/watch.c src/call-volume.c \
-			src/gprs.c src/idmap.h src/idmap.c
+			src/gprs.c src/idmap.h src/idmap.c \
+			src/radio-settings.c
 
 src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ -ldl
 
diff --git a/doc/radio-settings-api.txt b/doc/radio-settings-api.txt
new file mode 100644
index 0000000..c36b574
--- /dev/null
+++ b/doc/radio-settings-api.txt
@@ -0,0 +1,42 @@
+Radio Settings hierarchy
+========================
+
+Service		org.ofono
+Interface	org.ofono.RadioSettings
+Object path	[variable prefix]/{modem0,modem1,...}
+
+Methods		dict GetProperties()
+
+			Returns all radio access properties. See the
+			properties section for available properties.
+
+			Possible Errors: [service].Error.InvalidArguments
+
+		void SetProperty(string name, variant value)
+
+			Changes the value of the specified property. Only
+			properties that are listed as read-write are
+			changeable. On success a PropertyChanged signal
+			will be emitted.
+
+			Possible Errors: [service].Error.InvalidArguments
+					 [service].Error.DoesNotExist
+					 [service].Error.InProgress
+
+Signals		PropertyChanged(string property, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	string Mode [read-write]
+
+			The current radio access selection mode, also known
+			as network preference.
+
+			The possible values are:
+				"dual"     Radio access selection is done
+					   automatically, using either 2G
+					   or 3G, depending on reception.
+				"2g"       Only GSM used for radio access.
+				"3g"       Only UTRAN used for radio access.
+				"unknown"  Mode currently unknown.
diff --git a/include/radio-settings.h b/include/radio-settings.h
new file mode 100644
index 0000000..807d3da
--- /dev/null
+++ b/include/radio-settings.h
@@ -0,0 +1,74 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
+ *
+ *  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_RADIO_SETTINGS_H
+#define __OFONO_RADIO_SETTINGS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <ofono/types.h>
+
+enum ofono_radio_access_mode {
+	OFONO_RADIO_ACCESS_MODE_DUAL = 0,
+	OFONO_RADIO_ACCESS_MODE_2G = 1,
+	OFONO_RADIO_ACCESS_MODE_3G = 2,
+};
+
+struct ofono_radio_settings;
+
+typedef void (*ofono_radio_settings_mode_set_cb_t)(const struct ofono_error *error,
+							void *data);
+typedef void (*ofono_radio_settings_mode_query_cb_t)(const struct ofono_error *error,
+						enum ofono_radio_access_mode mode,
+						void *data);
+
+struct ofono_radio_settings_driver {
+	const char *name;
+	int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor, void *data);
+	void (*remove)(struct ofono_radio_settings *rs);
+	void (*query_mode)(struct ofono_radio_settings *rs,
+				ofono_radio_settings_mode_query_cb_t cb, void *data);
+	void (*set_mode)(struct ofono_radio_settings *rs,
+				enum ofono_radio_access_mode mode,
+				ofono_radio_settings_mode_set_cb_t cb, void *data);
+};
+
+int ofono_radio_settings_driver_register(const struct ofono_radio_settings_driver *d);
+void ofono_radio_settings_driver_unregister(const struct ofono_radio_settings_driver *d);
+
+struct ofono_radio_settings *ofono_radio_settings_create(struct ofono_modem *modem,
+								unsigned int vendor,
+								const char *driver,
+								void *data);
+
+void ofono_radio_settings_register(struct ofono_radio_settings *rs);
+void ofono_radio_settings_remove(struct ofono_radio_settings *rs);
+
+void ofono_radio_settings_set_data(struct ofono_radio_settings *rs, void *data);
+void *ofono_radio_settings_get_data(struct ofono_radio_settings *rs);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_RADIO_SETTINGS_H */
diff --git a/src/ofono.h b/src/ofono.h
index 379f413..ff67728 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -113,6 +113,7 @@ enum ofono_atom_type {
 	OFONO_ATOM_TYPES_CALL_VOLUME = 15,
 	OFONO_ATOM_TYPE_GPRS = 16,
 	OFONO_ATOM_TYPE_GPRS_CONTEXT = 17,
+	OFONO_ATOM_TYPE_RADIO_SETTINGS = 18,
 };
 
 enum ofono_atom_watch_condition {
@@ -168,6 +169,7 @@ void __ofono_atom_free(struct ofono_atom *atom);
 #include <ofono/voicecall.h>
 #include <ofono/gprs.h>
 #include <ofono/gprs-context.h>
+#include <ofono/radio-settings.h>
 
 #include <ofono/sim.h>
 
diff --git a/src/radio-settings.c b/src/radio-settings.c
new file mode 100644
index 0000000..6e41194
--- /dev/null
+++ b/src/radio-settings.c
@@ -0,0 +1,372 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
+ *
+ *  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 "ofono.h"
+#include "common.h"
+
+#define RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
+#define RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
+
+#define RADIO_SETTINGS_MODE_CACHED 0x1
+
+static GSList *g_drivers = NULL;
+
+struct ofono_radio_settings {
+	DBusMessage *pending;
+	int flags;
+	int mode;
+	int pending_mode;
+	const struct ofono_radio_settings_driver *driver;
+	void *driver_data;
+	struct ofono_atom *atom;
+};
+
+static const char *radio_access_mode_to_string(enum ofono_radio_access_mode mode)
+{
+	switch (mode) {
+	case OFONO_RADIO_ACCESS_MODE_DUAL:
+		return "dual";
+	case OFONO_RADIO_ACCESS_MODE_2G:
+		return "2g";
+	case OFONO_RADIO_ACCESS_MODE_3G:
+		return "3g";
+	default:
+		return "unknown";
+	}
+}
+
+static int string_to_radio_access_mode(const char *mode)
+
+{
+	if (g_strcmp0(mode, "dual") == 0)
+		return OFONO_RADIO_ACCESS_MODE_DUAL;
+	if (g_strcmp0(mode, "2g") == 0)
+		return OFONO_RADIO_ACCESS_MODE_2G;
+	if (g_strcmp0(mode, "3g") == 0)
+		return OFONO_RADIO_ACCESS_MODE_3G;
+	return -1;
+}
+
+static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
+						struct ofono_radio_settings *rs)
+{
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+
+	const char *mode = radio_access_mode_to_string(rs->mode);
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dict);
+
+	ofono_dbus_dict_append(&dict, "Mode", DBUS_TYPE_STRING, &mode);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static void radio_set_mode(struct ofono_radio_settings *rs,
+				enum ofono_radio_access_mode mode)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path;
+	const char *str_mode;
+
+	if (rs->mode == (int)mode)
+		return;
+
+	rs->mode = mode;
+	rs->flags |= RADIO_SETTINGS_MODE_CACHED;
+
+	path = __ofono_atom_get_path(rs->atom);
+	str_mode = radio_access_mode_to_string(rs->mode);
+
+	ofono_dbus_signal_property_changed(conn, path,
+						RADIO_SETTINGS_INTERFACE,
+						"Mode", DBUS_TYPE_STRING,
+						&str_mode);
+}
+
+static void radio_mode_set_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_radio_settings *rs = data;
+	DBusMessage *reply;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_debug("Error setting radio access mode");
+		rs->pending_mode = rs->mode;
+		reply = __ofono_error_failed(rs->pending);
+		__ofono_dbus_pending_reply(&rs->pending, reply);
+		return;
+	}
+
+	reply = dbus_message_new_method_return(rs->pending);
+	__ofono_dbus_pending_reply(&rs->pending, reply);
+
+	radio_set_mode(rs, rs->pending_mode);
+}
+
+static void radio_mode_query_callback(const struct ofono_error *error,
+					enum ofono_radio_access_mode mode,
+					void *data)
+{
+	struct ofono_radio_settings *rs = data;
+	DBusMessage *reply;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_debug("Error during radio access mode query");
+		reply = __ofono_error_failed(rs->pending);
+		__ofono_dbus_pending_reply(&rs->pending, reply);
+		return;
+	}
+
+	radio_set_mode(rs, mode);
+
+	reply = radio_get_properties_reply(rs->pending, rs);
+	__ofono_dbus_pending_reply(&rs->pending, reply);
+}
+
+static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	struct ofono_radio_settings *rs = data;
+
+	if (rs->flags & RADIO_SETTINGS_MODE_CACHED)
+		return radio_get_properties_reply(msg, rs);
+
+	if (!rs->driver->query_mode)
+		return __ofono_error_not_implemented(msg);
+
+	if (rs->pending)
+		return __ofono_error_busy(msg);
+
+	rs->pending = dbus_message_ref(msg);
+	rs->driver->query_mode(rs, radio_mode_query_callback, rs);
+
+	return NULL;
+}
+
+static DBusMessage *radio_set_property(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	struct ofono_radio_settings *rs = data;
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+
+	if (rs->pending)
+		return __ofono_error_busy(msg);
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &property);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_recurse(&iter, &var);
+
+	if (g_strcmp0(property, "Mode") == 0) {
+		const char *value;
+		int mode = -1;
+
+		if (!rs->driver->set_mode)
+			return __ofono_error_not_implemented(msg);
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_STRING)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &value);
+		mode = string_to_radio_access_mode(value);
+
+		if (mode == -1)
+			return __ofono_error_invalid_args(msg);
+
+		if (rs->mode == mode)
+			return dbus_message_new_method_return(msg);
+
+		rs->pending = dbus_message_ref(msg);
+		rs->pending_mode = mode;
+
+		rs->driver->set_mode(rs, mode, radio_mode_set_callback, rs);
+
+		return NULL;
+	}
+
+	return __ofono_error_invalid_args(msg);
+}
+
+static GDBusMethodTable radio_methods[] = {
+	{ "GetProperties",	"",	"a{sv}",	radio_get_properties,
+							G_DBUS_METHOD_FLAG_ASYNC },
+	{ "SetProperty",	"sv",	"",		radio_set_property,
+							G_DBUS_METHOD_FLAG_ASYNC },
+	{ }
+};
+
+static GDBusSignalTable radio_signals[] = {
+	{ "PropertyChanged",	"sv" },
+	{ }
+};
+
+int ofono_radio_settings_driver_register(const struct ofono_radio_settings_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (!d || !d->probe)
+		return -EINVAL;
+
+	g_drivers = g_slist_prepend(g_drivers, (void *)d);
+
+	return 0;
+}
+
+void ofono_radio_settings_driver_unregister(const struct ofono_radio_settings_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (!d)
+		return;
+
+	g_drivers = g_slist_remove(g_drivers, (void *)d);
+}
+
+static void radio_settings_unregister(struct ofono_atom *atom)
+{
+	struct ofono_radio_settings *rs = __ofono_atom_get_data(atom);
+	const char *path = __ofono_atom_get_path(rs->atom);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(rs->atom);
+
+	ofono_modem_remove_interface(modem, RADIO_SETTINGS_INTERFACE);
+	g_dbus_unregister_interface(conn, path, RADIO_SETTINGS_INTERFACE);
+}
+
+static void radio_settings_remove(struct ofono_atom *atom)
+{
+	struct ofono_radio_settings *rs = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (!rs)
+		return;
+
+	if (rs->driver && rs->driver->remove)
+		rs->driver->remove(rs);
+
+	g_free(rs);
+}
+
+struct ofono_radio_settings *ofono_radio_settings_create(struct ofono_modem *modem,
+							unsigned int vendor,
+							const char *driver,
+							void *data)
+{
+	struct ofono_radio_settings *rs;
+	GSList *l;
+
+	if (!driver)
+		return NULL;
+
+	rs = g_try_new0(struct ofono_radio_settings, 1);
+	if (!rs)
+		return NULL;
+
+	rs->mode = -1;
+
+	rs->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_RADIO_SETTINGS,
+						radio_settings_remove, rs);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_radio_settings_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver) != 0)
+			continue;
+
+		if (drv->probe(rs, vendor, data) < 0)
+			continue;
+
+		rs->driver = drv;
+		break;
+	}
+
+	return rs;
+}
+
+void ofono_radio_settings_register(struct ofono_radio_settings *rs)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(rs->atom);
+	const char *path = __ofono_atom_get_path(rs->atom);
+
+	if (!g_dbus_register_interface(conn, path,
+					RADIO_SETTINGS_INTERFACE,
+					radio_methods, radio_signals,
+					NULL, rs, NULL)) {
+		ofono_error("Could not create %s interface",
+				RADIO_SETTINGS_INTERFACE);
+
+		return;
+	}
+
+	ofono_modem_add_interface(modem, RADIO_SETTINGS_INTERFACE);
+	__ofono_atom_register(rs->atom, radio_settings_unregister);
+}
+
+void ofono_radio_settings_remove(struct ofono_radio_settings *rs)
+{
+	__ofono_atom_free(rs->atom);
+}
+
+void ofono_radio_settings_set_data(struct ofono_radio_settings *rs,
+					void *data)
+{
+	rs->driver_data = data;
+}
+
+void *ofono_radio_settings_get_data(struct ofono_radio_settings *rs)
+{
+	return rs->driver_data;
+}
-- 
1.6.3.3

Re: [PATCH 1/3] Add radio settings atom and driver API

[Marcel Holtmann]

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

Hi Aki,

> +Properties	string Mode [read-write]
> +
> +			The current radio access selection mode, also known
> +			as network preference.
> +
> +			The possible values are:
> +				"dual"     Radio access selection is done
> +					   automatically, using either 2G
> +					   or 3G, depending on reception.
> +				"2g"       Only GSM used for radio access.
> +				"3g"       Only UTRAN used for radio access.
> +				"unknown"  Mode currently unknown.

how would we be dealing with LTE here. The word "dual" is kinda tricky
in that sense. Also what about chips like Qualcomm GOBI that can change
their firmware and start operating in CDMA. We do wanna extend oFono to
support CDMA at some point.

Regards

Marcel

Re: [PATCH 1/3] Add radio settings atom and driver API

[Denis Kenzior]

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

Hi Aki,

> This interface exposes a rw property for radio access selection mode
> setting.
> ---
>  Makefile.am                |    6 +-
>  doc/radio-settings-api.txt |   42 +++++
>  include/radio-settings.h   |   74 +++++++++
>  src/ofono.h                |    2 +
>  src/radio-settings.c       |  372
>  ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 494
>  insertions(+), 2 deletions(-)
>  create mode 100644 doc/radio-settings-api.txt
>  create mode 100644 include/radio-settings.h
>  create mode 100644 src/radio-settings.c

<snip>

> +Properties	string Mode [read-write]

Per mailing list thread with Marcel, "Technology" or "TechnologySelection" or 
"TechnologyPreference" will be better here.

> +
> +			The current radio access selection mode, also known
> +			as network preference.
> +
> +			The possible values are:
> +				"dual"     Radio access selection is done
> +					   automatically, using either 2G
> +					   or 3G, depending on reception.

As discussed in later threads, I do like "auto" or "any" here.

> +				"2g"       Only GSM used for radio access.
> +				"3g"       Only UTRAN used for radio access.
> +				"unknown"  Mode currently unknown.

Lets get rid of the "unknown" part, we always know the setting, even if it 
takes a little time to query it.

> diff --git a/include/radio-settings.h b/include/radio-settings.h
> new file mode 100644
> index 0000000..807d3da
> +typedef void (*ofono_radio_settings_mode_query_cb_t)(const struct
>  ofono_error *error, +						enum ofono_radio_access_mode mode,
> +						void *data);

I still say this part is not required.

> +
> +struct ofono_radio_settings_driver {
> +	const char *name;
> +	int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor, void
>  *data); +	void (*remove)(struct ofono_radio_settings *rs);
> +	void (*query_mode)(struct ofono_radio_settings *rs,
> +				ofono_radio_settings_mode_query_cb_t cb, void *data);

Neither is query_mode.  This is a local modem setting, not a network setting.  
There's simply no reason to query it when oFono can store the settings.

Lets just add ability to read the mode setting from storage and set it at 
interface startup.

> +	void (*set_mode)(struct ofono_radio_settings *rs,
> +				enum ofono_radio_access_mode mode,
> +				ofono_radio_settings_mode_set_cb_t cb, void *data);

Should this be called something else? E.g. set_technology or set_rat_mode.

> +};
> +
> diff --git a/src/ofono.h b/src/ofono.h
> index 379f413..ff67728 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -113,6 +113,7 @@ enum ofono_atom_type {
>  	OFONO_ATOM_TYPES_CALL_VOLUME = 15,
>  	OFONO_ATOM_TYPE_GPRS = 16,
>  	OFONO_ATOM_TYPE_GPRS_CONTEXT = 17,
> +	OFONO_ATOM_TYPE_RADIO_SETTINGS = 18,
>  };
> 
>  enum ofono_atom_watch_condition {
> @@ -168,6 +169,7 @@ void __ofono_atom_free(struct ofono_atom *atom);
>  #include <ofono/voicecall.h>
>  #include <ofono/gprs.h>
>  #include <ofono/gprs-context.h>
> +#include <ofono/radio-settings.h>
> 
>  #include <ofono/sim.h>
> 
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> new file mode 100644
> index 0000000..6e41194
> --- /dev/null
> +++ b/src/radio-settings.c

<snip>

> +#define RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
> +#define RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
> +

Double define?

Otherwise it looks good to me.

Regards,
-Denis

Re: [PATCH 1/3] Add radio settings atom and driver API

[Aki Niemi]

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

Hi Denis,

2010/2/4 Denis Kenzior <denkenz@gmail.com>:
>> +typedef void (*ofono_radio_settings_mode_query_cb_t)(const struct
>>  ofono_error *error, +                                                enum ofono_radio_access_mode mode,
>> +                                             void *data);
>
> I still say this part is not required.
>
>> +
>> +struct ofono_radio_settings_driver {
>> +     const char *name;
>> +     int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor, void
>>  *data); +    void (*remove)(struct ofono_radio_settings *rs);
>> +     void (*query_mode)(struct ofono_radio_settings *rs,
>> +                             ofono_radio_settings_mode_query_cb_t cb, void *data);
>
> Neither is query_mode.  This is a local modem setting, not a network setting.
> There's simply no reason to query it when oFono can store the settings.

Even with storage, it's needed the very first time a new SIM card is
inserted, no?

> Lets just add ability to read the mode setting from storage and set it at
> interface startup.

I can agreee to storing SIM data, but for other settings that are
anyway saved on the modem's non-volatile memory, this seems wrong.
oFono is aimed to be usable also on the desktop using tethering, which
means most of the time these settings are touched on the phone.

(As an aside, I have right now two N900s connected to my laptop via
USB, and was just making calls between them using oFono and d-feet.)

>> +     void (*set_mode)(struct ofono_radio_settings *rs,
>> +                             enum ofono_radio_access_mode mode,
>> +                             ofono_radio_settings_mode_set_cb_t cb, void *data);
>
> Should this be called something else? E.g. set_technology or set_rat_mode.

Makes sense.

>> +#define RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
>> +#define RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
>> +
>
> Double define?

Too much coffee.

> Otherwise it looks good to me.

Cool. I will make the changes and push in a bit.

Cheers,
Aki

Re: [PATCH 1/3] Add radio settings atom and driver API

[Denis Kenzior]

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

Hi Aki,

> Hi Denis,
> 
> 2010/2/4 Denis Kenzior <denkenz@gmail.com>:
> >> +typedef void (*ofono_radio_settings_mode_query_cb_t)(const struct
> >>  ofono_error *error, +                                              
> >>  enum ofono_radio_access_mode mode, +                                  
> >>           void *data);
> >
> > I still say this part is not required.
> >
> >> +
> >> +struct ofono_radio_settings_driver {
> >> +     const char *name;
> >> +     int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor,
> >> void *data); +    void (*remove)(struct ofono_radio_settings *rs);
> >> +     void (*query_mode)(struct ofono_radio_settings *rs,
> >> +                             ofono_radio_settings_mode_query_cb_t cb,
> >> void *data);
> >
> > Neither is query_mode.  This is a local modem setting, not a network
> > setting. There's simply no reason to query it when oFono can store the
> > settings.
> 
> Even with storage, it's needed the very first time a new SIM card is
> inserted, no?
> 

Not really, we can default to a safe setting (e.g. any) if no previous setting 
has been saved.  We do this for GPRS, Cell Broadcasts, etc.

> > Lets just add ability to read the mode setting from storage and set it at
> > interface startup.
> 
> I can agreee to storing SIM data, but for other settings that are
> anyway saved on the modem's non-volatile memory, this seems wrong.
> oFono is aimed to be usable also on the desktop using tethering, which
> means most of the time these settings are touched on the phone.

But also remember I want to keep all my settings when I switch SIMs.  On the 
desktop that is a much likelier scenario (e.g. GPRS data cards), travel 
between countries, etc. The radio access setting is likely to be specific to a 
network provider (e.g. poor 3G coverage) rather than the device.

I agree tethering is a usecase we haven't considered properly yet, but 
honestly I'm not sure the PC should be messing with radio preference settings 
while tethered...

> 
> (As an aside, I have right now two N900s connected to my laptop via
> USB, and was just making calls between them using oFono and d-feet.)
> 

Nice :)

Regards,
-Denis

Re: [PATCH 1/3] Add radio settings atom and driver API

[Marcel Holtmann]

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

Hi Aki,

> > > +Properties    string Mode [read-write]
> > > +
> > > +            The current radio access selection mode, also known
> > > +            as network preference.
> > > +
> > > +            The possible values are:
> > > +                "dual"        Radio access selection is done
> > > +                        automatically, using either 2G
> > > +                        or 3G, depending on reception.
> > > +                "2g"            Only GSM used for radio access.
> > > +                "3g"            Only UTRAN used for radio access.
> > > +                "unknown"  Mode currently unknown.
> >
> > how would we be dealing with LTE here. The word "dual" is kinda tricky
> > in that sense. Also what about chips like Qualcomm GOBI that can change
> > their firmware and start operating in CDMA. We do wanna extend oFono to
> > support CDMA at some point.
> 
> Yeah, perhaps the property should be called "TechnologyPreference" with values "any", "2G", "3G", "4G". For now at least.

so what about the magic CDMA + UMTS combo chips. We basically have to
reload the firmware with these. Would be nice to figure out on how that
can be done. Not that we know much about how Qualcomm intends to use
that in a phone case scenario, but would be nice to at least think about
it a bit.

Also I wanted you to ask about the data roaming. Does the Nokia modem
has an option to NOT data roam that we can set. The problem with doing
it in the host is that a bit of data goes through if you start roaming
and have a PDP context active. So it costs the user. At least some
reports indicate this. Does your modem forces a detach or does it just
happily data roam and just give us a cell update?

The AT command specification obviously didn't care about this at all :(

Regards

Marcel

Re: [PATCH 1/3] Add radio settings atom and driver API

[aki.niemi]

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


----- Alkuperäinen viesti -----
> > Yeah, perhaps the property should be called "TechnologyPreference" with values
> > "any", "2G", "3G", "4G". For now at least.
>
> so what about the magic CDMA + UMTS combo chips. We basically have to
> reload the firmware with these. Would be nice to figure out on how that
> can be done. Not that we know much about how Qualcomm intends to use
> that in a phone case scenario, but would be nice to at least think about
> it a bit.

I'm not at all sure toggling between CDMA and UMTS is going to work smoothly enough for it to be done at this interface level. It sounds more like disabling one modem and enabling another. And I'm sure there are other areas in the API that are going to need tweaking for CDMA. I would rather cross that bridge when we come to it.

> Also I wanted you to ask about the data roaming. Does the Nokia modem
> has an option to NOT data roam that we can set. The problem with doing
> it in the host is that a bit of data goes through if you start roaming
> and have a PDP context active. So it costs the user. At least some
> reports indicate this. Does your modem forces a detach or does it just
> happily data roam and just give us a cell update?

You mean PDP contexts survive a network change? I don't think they do. Sounds like a bug in the application.

The Nokia modems have a setting to automatically attach, but that doesn't yet move any data.

Cheers,
Aki

-- 
Sent from Nokia N900 running Maemo 5

Re: [PATCH 1/3] Add radio settings atom and driver API

[Marcel Holtmann]

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

Hi Aki,

> > > Yeah, perhaps the property should be called "TechnologyPreference" with values
> > > "any", "2G", "3G", "4G". For now at least.
> >
> > so what about the magic CDMA + UMTS combo chips. We basically have to
> > reload the firmware with these. Would be nice to figure out on how that
> > can be done. Not that we know much about how Qualcomm intends to use
> > that in a phone case scenario, but would be nice to at least think about
> > it a bit.
> 
> I'm not at all sure toggling between CDMA and UMTS is going to work smoothly enough for it to be done at this interface level. It sounds more like disabling one modem and enabling another. And I'm sure there are other areas in the API that are going to need tweaking for CDMA. I would rather cross that bridge when we come to it.

fair enough from my point of view, but lets keep that in mind. The
hardware for that is out there.

> > Also I wanted you to ask about the data roaming. Does the Nokia modem
> > has an option to NOT data roam that we can set. The problem with doing
> > it in the host is that a bit of data goes through if you start roaming
> > and have a PDP context active. So it costs the user. At least some
> > reports indicate this. Does your modem forces a detach or does it just
> > happily data roam and just give us a cell update?
> 
> You mean PDP contexts survive a network change? I don't think they do. Sounds like a bug in the application.
> 
> The Nokia modems have a setting to automatically attach, but that doesn't yet move any data.

That is what T-Mobile promises me when crossing borders into Netherlands
or Austria with my German SIM card. It should have smooth data roaming.
To be honest, I have never really tried it. Either I was on a plane
anymore or too cheap to pay for data roaming ;)

Regards

Marcel