[PATCH 0/6] Fast dormancy support

[PATCH 0/6] Fast dormancy support

[Mika Liljeberg]

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

Hi All,

Here's another go at fast dormancy.

Br,

	MikaL

[PATCH 1/6] radio settings: fix mode initializion
[PATCH 2/6] stemodem: add default case
[PATCH 3/6] radio settings: add FastDormancy property
[PATCH 4/6] test: add script to control fast dormancy
[PATCH 5/6] isimodem: add support for FastDormancy property
[PATCH 6/6] TODO: mark fast dormancy as done

 Makefile.am                       |    3 +-
 TODO                              |   20 ------
 doc/features.txt                  |    8 +++
 drivers/isimodem/radio-settings.c |   91 ++++++++++++++++++++++++++++-
 drivers/stemodem/radio-settings.c |    2 +-
 include/radio-settings.h          |   22 ++++++-
 src/radio-settings.c              |  116 ++++++++++++++++++++++++++++++++++--
 test/set-fast-dormancy            |   25 ++++++++
 8 files changed, 253 insertions(+), 34 deletions(-)

[PATCH 1/6] radio settings: fix mode initializion

[Mika Liljeberg]

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

---
 include/radio-settings.h |    9 +++++----
 src/radio-settings.c     |    2 +-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/radio-settings.h b/include/radio-settings.h
index d41ec0b..1b133e9 100644
--- a/include/radio-settings.h
+++ b/include/radio-settings.h
@@ -29,10 +29,11 @@ extern "C" {
 #include <ofono/types.h>
 
 enum ofono_radio_access_mode {
-	OFONO_RADIO_ACCESS_MODE_ANY	= 0,
-	OFONO_RADIO_ACCESS_MODE_GSM	= 1,
-	OFONO_RADIO_ACCESS_MODE_UMTS	= 2,
-	OFONO_RADIO_ACCESS_MODE_LTE	= 3,
+	OFONO_RADIO_ACCESS_MODE_UNKNOWN	= 0,
+	OFONO_RADIO_ACCESS_MODE_ANY	= 1,
+	OFONO_RADIO_ACCESS_MODE_GSM	= 2,
+	OFONO_RADIO_ACCESS_MODE_UMTS	= 3,
+	OFONO_RADIO_ACCESS_MODE_LTE	= 4,
 };
 
 struct ofono_radio_settings;
diff --git a/src/radio-settings.c b/src/radio-settings.c
index 3306be6..173e5bf 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -321,7 +321,7 @@ struct ofono_radio_settings *ofono_radio_settings_create(struct ofono_modem *mod
 	if (!rs)
 		return NULL;
 
-	rs->mode = -1;
+	rs->mode = OFONO_RADIO_ACCESS_MODE_UNKNOWN;
 
 	rs->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_RADIO_SETTINGS,
 						radio_settings_remove, rs);
-- 
1.7.0.4

[PATCH 2/6] stemodem: add default case

[Mika Liljeberg]

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

---
 drivers/stemodem/radio-settings.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/stemodem/radio-settings.c b/drivers/stemodem/radio-settings.c
index 5b16ec0..a345975 100644
--- a/drivers/stemodem/radio-settings.c
+++ b/drivers/stemodem/radio-settings.c
@@ -90,7 +90,7 @@ static gboolean ofono_mode_to_ste_mode(enum ofono_radio_access_mode mode,
 	case OFONO_RADIO_ACCESS_MODE_UMTS:
 		*stemode = STE_RADIO_WCDMA_ONLY;
 		return TRUE;
-	case OFONO_RADIO_ACCESS_MODE_LTE:
+	default:
 		break;
 	}
 
-- 
1.7.0.4

[PATCH 3/6] radio settings: add FastDormancy property

[Mika Liljeberg]

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

---
 include/radio-settings.h |   13 +++++
 src/radio-settings.c     |  114 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 121 insertions(+), 6 deletions(-)

diff --git a/include/radio-settings.h b/include/radio-settings.h
index 1b133e9..9a00772 100644
--- a/include/radio-settings.h
+++ b/include/radio-settings.h
@@ -44,6 +44,12 @@ typedef void (*ofono_radio_settings_rat_mode_query_cb_t)(const struct ofono_erro
 						enum ofono_radio_access_mode mode,
 						void *data);
 
+typedef void (*ofono_radio_settings_fast_dormancy_set_cb_t)(const struct ofono_error *error,
+							void *data);
+typedef void (*ofono_radio_settings_fast_dormancy_query_cb_t)(const struct ofono_error *error,
+							ofono_bool_t enable,
+							void *data);
+
 struct ofono_radio_settings_driver {
 	const char *name;
 	int (*probe)(struct ofono_radio_settings *rs, unsigned int vendor,
@@ -56,6 +62,13 @@ struct ofono_radio_settings_driver {
 				enum ofono_radio_access_mode mode,
 				ofono_radio_settings_rat_mode_set_cb_t cb,
 				void *data);
+	void (*query_fast_dormancy)(struct ofono_radio_settings *rs,
+			ofono_radio_settings_fast_dormancy_query_cb_t cb,
+			void *data);
+	void (*set_fast_dormancy)(struct ofono_radio_settings *rs,
+				int enable,
+				ofono_radio_settings_fast_dormancy_set_cb_t,
+				void *data);
 };
 
 int ofono_radio_settings_driver_register(const struct ofono_radio_settings_driver *d);
diff --git a/src/radio-settings.c b/src/radio-settings.c
index 173e5bf..223ee51 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -33,7 +33,7 @@
 #include "ofono.h"
 #include "common.h"
 
-#define RADIO_SETTINGS_MODE_CACHED 0x1
+#define RADIO_SETTINGS_FLAG_CACHED 0x1
 
 static GSList *g_drivers = NULL;
 
@@ -42,6 +42,8 @@ struct ofono_radio_settings {
 	int flags;
 	enum ofono_radio_access_mode mode;
 	enum ofono_radio_access_mode pending_mode;
+	ofono_bool_t fast_dormancy;
+	ofono_bool_t fast_dormancy_pending;
 	const struct ofono_radio_settings_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -106,11 +108,55 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 	ofono_dbus_dict_append(&dict, "TechnologyPreference",
 					DBUS_TYPE_STRING, &mode);
 
+	if (rs->driver->query_fast_dormancy) {
+		dbus_bool_t value = rs->fast_dormancy;
+		ofono_dbus_dict_append(&dict, "FastDormancy",
+					DBUS_TYPE_BOOLEAN, &value);
+	}
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
 }
 
+static void radio_set_fast_dormancy(struct ofono_radio_settings *rs,
+					ofono_bool_t enable)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(rs->atom);
+	dbus_bool_t value = enable;
+
+	if ((rs->flags & RADIO_SETTINGS_FLAG_CACHED) &&
+		rs->fast_dormancy == enable)
+		return;
+
+	ofono_dbus_signal_property_changed(conn, path,
+						OFONO_RADIO_SETTINGS_INTERFACE,
+						"FastDormancy",
+						DBUS_TYPE_BOOLEAN, &value);
+	rs->fast_dormancy = enable;
+}
+
+static void radio_fast_dormancy_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) {
+		DBG("Error setting fast dormancy");
+		rs->fast_dormancy_pending = rs->fast_dormancy;
+		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_fast_dormancy(rs, rs->fast_dormancy_pending);
+}
+
 static void radio_set_rat_mode(struct ofono_radio_settings *rs,
 				enum ofono_radio_access_mode mode)
 {
@@ -122,7 +168,6 @@ static void radio_set_rat_mode(struct ofono_radio_settings *rs,
 		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);
@@ -152,6 +197,43 @@ static void radio_mode_set_callback(const struct ofono_error *error, void *data)
 	radio_set_rat_mode(rs, rs->pending_mode);
 }
 
+static void radio_send_properties_reply(struct ofono_radio_settings *rs)
+{
+	DBusMessage *reply;
+
+	rs->flags |= RADIO_SETTINGS_FLAG_CACHED;
+	reply = radio_get_properties_reply(rs->pending, rs);
+	__ofono_dbus_pending_reply(&rs->pending, reply);
+}
+
+static void radio_fast_dormancy_query_callback(const struct ofono_error *error,
+						ofono_bool_t enable, void *data)
+{
+	struct ofono_radio_settings *rs = data;
+	DBusMessage *reply;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		DBG("Error during fast dormancy query");
+		reply = __ofono_error_failed(rs->pending);
+		__ofono_dbus_pending_reply(&rs->pending, reply);
+		return;
+	}
+
+	radio_set_fast_dormancy(rs, enable);
+	radio_send_properties_reply(rs);
+}
+
+static void radio_query_fast_dormancy(struct ofono_radio_settings *rs)
+{
+	if (!rs->driver->query_fast_dormancy) {
+		radio_send_properties_reply(rs);
+		return;
+	}
+
+	rs->driver->query_fast_dormancy(rs, radio_fast_dormancy_query_callback,
+					rs);
+}
+
 static void radio_rat_mode_query_callback(const struct ofono_error *error,
 					enum ofono_radio_access_mode mode,
 					void *data)
@@ -167,9 +249,7 @@ static void radio_rat_mode_query_callback(const struct ofono_error *error,
 	}
 
 	radio_set_rat_mode(rs, mode);
-
-	reply = radio_get_properties_reply(rs->pending, rs);
-	__ofono_dbus_pending_reply(&rs->pending, reply);
+	radio_query_fast_dormancy(rs);
 }
 
 static DBusMessage *radio_get_properties(DBusConnection *conn,
@@ -177,7 +257,7 @@ static DBusMessage *radio_get_properties(DBusConnection *conn,
 {
 	struct ofono_radio_settings *rs = data;
 
-	if (rs->flags & RADIO_SETTINGS_MODE_CACHED)
+	if (rs->flags & RADIO_SETTINGS_FLAG_CACHED)
 		return radio_get_properties_reply(msg, rs);
 
 	if (!rs->driver->query_rat_mode)
@@ -240,6 +320,28 @@ static DBusMessage *radio_set_property(DBusConnection *conn, DBusMessage *msg,
 		rs->driver->set_rat_mode(rs, mode, radio_mode_set_callback, rs);
 
 		return NULL;
+	} else if (g_strcmp0(property, "FastDormancy") == 0) {
+		dbus_bool_t value;
+		int target;
+
+		if (!rs->driver->set_fast_dormancy)
+			return __ofono_error_not_implemented(msg);
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &value);
+		target = value;
+
+		if (rs->fast_dormancy_pending == target)
+			return dbus_message_new_method_return(msg);
+
+		rs->pending = dbus_message_ref(msg);
+		rs->fast_dormancy_pending = target;
+
+		rs->driver->set_fast_dormancy(rs, target,
+					radio_fast_dormancy_set_callback, rs);
+		return NULL;
 	}
 
 	return __ofono_error_invalid_args(msg);
-- 
1.7.0.4

[PATCH 4/6] test: add script to control fast dormancy

[Mika Liljeberg]

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

---
 Makefile.am            |    3 ++-
 test/set-fast-dormancy |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletions(-)
 create mode 100755 test/set-fast-dormancy

diff --git a/Makefile.am b/Makefile.am
index 17797ae..5a541c7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -410,7 +410,8 @@ test_scripts = test/backtrace \
 		test/enable-gprs \
 		test/disable-gprs \
 		test/get-icon \
-		test/deactivate-all
+		test/deactivate-all \
+		test/set-fast-dormancy
 
 if TEST
 testdir = $(pkglibdir)/test
diff --git a/test/set-fast-dormancy b/test/set-fast-dormancy
new file mode 100755
index 0000000..6951813
--- /dev/null
+++ b/test/set-fast-dormancy
@@ -0,0 +1,25 @@
+#!/usr/bin/python
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 3:
+	path = sys.argv[1]
+	enable = int(sys.argv[2])
+elif len(sys.argv) == 2:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+						'org.ofono.Manager')
+	modems = manager.GetModems()
+	path = modems[0][0]
+	enable = int(sys.argv[1])
+else:
+	print "%s [PATH] {0|1}" % (sys.argv[0])
+	exit(1)
+
+print "Setting fast dormancy for modem %s..." % path
+radiosettings = dbus.Interface(bus.get_object('org.ofono', path),
+						'org.ofono.RadioSettings')
+
+radiosettings.SetProperty("FastDormancy", dbus.Boolean(enable));
-- 
1.7.0.4

[PATCH 5/6] isimodem: add support for FastDormancy property

[Mika Liljeberg]

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

---
 drivers/isimodem/radio-settings.c |   91 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 90 insertions(+), 1 deletions(-)

diff --git a/drivers/isimodem/radio-settings.c b/drivers/isimodem/radio-settings.c
index d220476..558e032 100644
--- a/drivers/isimodem/radio-settings.c
+++ b/drivers/isimodem/radio-settings.c
@@ -41,11 +41,16 @@
 #include "isimodem.h"
 #include "isiutil.h"
 #include "debug.h"
+#include "gpds.h"
 #include "gss.h"
 #include "network.h"
 
+#define PN_WRAN 0xb4
+
 struct radio_data {
 	GIsiClient *client;
+	uint16_t wran_object;
+	uint16_t quick_release:1;
 };
 
 static enum ofono_radio_access_mode isi_mode_to_ofono_mode(guint8 mode)
@@ -236,6 +241,65 @@ error:
 	g_free(cbd);
 }
 
+static void update_fast_dormancy(struct radio_data *rd)
+{
+	struct sockaddr_pn dst = {
+		.spn_family = AF_PHONET,
+		.spn_resource = 0x3a,
+		.spn_dev = rd->wran_object >> 8,
+		.spn_obj = rd->wran_object & 0xff,
+	};
+
+	if (!rd->wran_object)
+		return;
+
+	if (rd->quick_release) {
+		const unsigned char msg[] = {
+			0x1f, 0x00, 0x01, 0x01, 0x01, 0x00
+		};
+
+		g_isi_sendto(rd->client, &dst, msg, sizeof(msg), 0,
+				NULL, NULL, NULL);
+	} else {
+		const unsigned char msg[] = {
+			0x1f, 0x00, 0x01, 0x01, 0x02, 0x0a
+		};
+
+		g_isi_sendto(rd->client, &dst, msg, sizeof(msg), 0,
+				NULL, NULL, NULL);
+	}
+
+	DBG("3G PS quick release %s",
+		rd->quick_release ? "enabled" : "disabled");
+}
+
+static void gpds_context_activating_ind_cb(GIsiClient *client,
+					const void *restrict data, size_t len,
+					uint16_t object, void *opaque)
+{
+	struct radio_data *rd = opaque;
+	update_fast_dormancy(rd);
+}
+
+static void isi_query_fast_dormancy(struct ofono_radio_settings *rs,
+			ofono_radio_settings_fast_dormancy_query_cb_t cb,
+			void *data)
+{
+	struct radio_data *rd = ofono_radio_settings_get_data(rs);
+	CALLBACK_WITH_SUCCESS(cb, rd->quick_release, data);
+}
+
+static void isi_set_fast_dormancy(struct ofono_radio_settings *rs,
+				int enable,
+				ofono_radio_settings_fast_dormancy_set_cb_t cb,
+				void *data)
+{
+	struct radio_data *rd = ofono_radio_settings_get_data(rs);
+	rd->quick_release = enable;
+	update_fast_dormancy(rd);
+	CALLBACK_WITH_SUCCESS(cb, data);
+}
+
 static gboolean isi_radio_settings_register(gpointer user)
 {
 	struct ofono_radio_settings *rs = user;
@@ -249,9 +313,31 @@ static gboolean isi_radio_settings_register(gpointer user)
 
 	ofono_radio_settings_register(rs);
 
+	g_isi_add_subscription(rd->client,
+				PN_GPDS, GPDS_CONTEXT_ACTIVATING_IND,
+				gpds_context_activating_ind_cb, rd);
+	g_isi_commit_subscriptions(rd->client);
+
 	return FALSE;
 }
 
+static void wran_reachable_cb(GIsiClient *client, gboolean alive,
+				uint16_t object, void *opaque)
+{
+	struct radio_data *rd = opaque;
+
+	if (!alive) {
+		DBG("fast dormancy support disabled");
+		return;
+	}
+
+	rd->wran_object = object;
+
+	DBG("PN_WRAN reachable, object=0x%04x", object);
+
+	update_fast_dormancy(rd);
+}
+
 static void reachable_cb(GIsiClient *client, gboolean alive, uint16_t object,
 				void *opaque)
 {
@@ -289,6 +375,7 @@ static int isi_radio_settings_probe(struct ofono_radio_settings *rs,
 	ofono_radio_settings_set_data(rs, rd);
 
 	g_isi_verify(rd->client, reachable_cb, rs);
+	g_isi_verify_resource(rd->client, PN_WRAN, wran_reachable_cb, rd);
 
 	return 0;
 }
@@ -310,7 +397,9 @@ static struct ofono_radio_settings_driver driver = {
 	.probe			= isi_radio_settings_probe,
 	.remove			= isi_radio_settings_remove,
 	.query_rat_mode		= isi_query_rat_mode,
-	.set_rat_mode		= isi_set_rat_mode
+	.set_rat_mode		= isi_set_rat_mode,
+	.query_fast_dormancy	= isi_query_fast_dormancy,
+	.set_fast_dormancy	= isi_set_fast_dormancy,
 };
 
 void isi_radio_settings_init()
-- 
1.7.0.4

[PATCH 6/6] TODO: mark fast dormancy as done

[Mika Liljeberg]

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

---
 TODO             |   20 --------------------
 doc/features.txt |    8 ++++++++
 2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/TODO b/TODO
index 239bab0..6ac9cea 100644
--- a/TODO
+++ b/TODO
@@ -504,26 +504,6 @@ Miscellaneous
   Priority: Medium
   Complexity: C2
 
-- Add a property for Fast Dormancy in the RadioSettings atom. This property
-  will enable or disable Fast Dormancy. Fast Dormancy refers to UE initiated
-  release of radio resources quickly after a burst of data transfer has ended.
-  Normally, radio resources are released by the network after a timeout
-  configured by the network operator. Fast Dormancy allows the modem to release
-  radio resources more quickly. Typically, fast dormancy would be enabled
-  if no data transfer is predicted to occur in the near future (e.g. end user
-  is not actively using the device). This is a major power-saving feature for
-  mobile devices, but can be ignored for USB sticks or PCI devices.
-
-  If the modem does not support such a feature the property should never be
-  exposed to the user.
-
-  This feature is not discussed in 27.007, thus manufacturer specific commands
-  are required.
-
-  Priority: High
-  Complexity: C1
-  Owner: Mika Liljeberg <mika.liljeberg@nokia.com>
-
 - TTY (hearing impaired) support.  Add a new oFono atom type that will enable
   the user to enable or disable the TTY support on the modem.  Support for
   automatic detection of TTY (signaled by the driver) is also desired.
diff --git a/doc/features.txt b/doc/features.txt
index 2acd352..d6365a2 100644
--- a/doc/features.txt
+++ b/doc/features.txt
@@ -147,3 +147,11 @@ SIM
   check if BDN support is allocated and enabled in the SIM.  If enabled,
   oFono halts the SIM initialization procedure and the modem remains in the
   PRESIM state.  In this state oFono will only allow emergency calls.
+
+Radio settings
+==============
+
+- Fast dormancy support. A fast dormancy feature can be enabled in the
+  cellular modem to conserve power when the end user is not actively
+  using the device but some networking applications are online using
+  packet data.
-- 
1.7.0.4

Re: [PATCH 2/6] stemodem: add default case

[Marcel Holtmann]

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

Hi Mika,

> ---
>  drivers/stemodem/radio-settings.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/stemodem/radio-settings.c b/drivers/stemodem/radio-settings.c
> index 5b16ec0..a345975 100644
> --- a/drivers/stemodem/radio-settings.c
> +++ b/drivers/stemodem/radio-settings.c
> @@ -90,7 +90,7 @@ static gboolean ofono_mode_to_ste_mode(enum ofono_radio_access_mode mode,
>  	case OFONO_RADIO_ACCESS_MODE_UMTS:
>  		*stemode = STE_RADIO_WCDMA_ONLY;
>  		return TRUE;
> -	case OFONO_RADIO_ACCESS_MODE_LTE:
> +	default:
>  		break;

no default for enums please. I want the compiler to warn us about not
handled switch statements.

Regards

Marcel

RE: [PATCH 2/6] stemodem: add default case

[Mika.Liljeberg]

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

Hi Marcel,

> > diff --git a/drivers/stemodem/radio-settings.c 
> b/drivers/stemodem/radio-settings.c
> > index 5b16ec0..a345975 100644
> > --- a/drivers/stemodem/radio-settings.c
> > +++ b/drivers/stemodem/radio-settings.c
> > @@ -90,7 +90,7 @@ static gboolean 
> ofono_mode_to_ste_mode(enum ofono_radio_access_mode mode,
> >  	case OFONO_RADIO_ACCESS_MODE_UMTS:
> >  		*stemode = STE_RADIO_WCDMA_ONLY;
> >  		return TRUE;
> > -	case OFONO_RADIO_ACCESS_MODE_LTE:
> > +	default:
> >  		break;
> 
> no default for enums please. I want the compiler to warn us about not
> handled switch statements.

That must be a new policy then, considering that stemodem is the only one that failed compilation. Feel free to fix this one. The first two patches in the set are unrelated to fast dormancy anyway.

Br,

	MikaL

RE: [PATCH 2/6] stemodem: add default case

[Marcel Holtmann]

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

Hi Mika,

> > > diff --git a/drivers/stemodem/radio-settings.c 
> > b/drivers/stemodem/radio-settings.c
> > > index 5b16ec0..a345975 100644
> > > --- a/drivers/stemodem/radio-settings.c
> > > +++ b/drivers/stemodem/radio-settings.c
> > > @@ -90,7 +90,7 @@ static gboolean 
> > ofono_mode_to_ste_mode(enum ofono_radio_access_mode mode,
> > >  	case OFONO_RADIO_ACCESS_MODE_UMTS:
> > >  		*stemode = STE_RADIO_WCDMA_ONLY;
> > >  		return TRUE;
> > > -	case OFONO_RADIO_ACCESS_MODE_LTE:
> > > +	default:
> > >  		break;
> > 
> > no default for enums please. I want the compiler to warn us about not
> > handled switch statements.
> 
> That must be a new policy then, considering that stemodem is the only one that failed compilation. Feel free to fix this one. The first two patches in the set are unrelated to fast dormancy anyway.

that is the whole point here. You modified the enum and the compilation
should fail unless you add a statement for that new item.

Let me make this clear, I do want the compilation to fail here and the
STE driver is doing the right thing.

There might be other cases where this is not consistent. I would prefer
if that never happens, but somethings things slip through even close
code review. If you know other cases, please let me know and we fix them
as well here.

Regards

Marcel

RE: [PATCH 2/6] stemodem: add default case

[Mika.Liljeberg]

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

Hi Marcel,

> > That must be a new policy then, considering that stemodem 
> is the only one that failed compilation. Feel free to fix 
> this one. The first two patches in the set are unrelated to 
> fast dormancy anyway.
> 
> that is the whole point here. You modified the enum and the 
> compilation
> should fail unless you add a statement for that new item.
> 
> Let me make this clear, I do want the compilation to fail here and the
> STE driver is doing the right thing.

I understand that. As I said, feel free to fix. Fast dormancy patches 3-6 should apply just fine without the first two courtesy patches.

> There might be other cases where this is not consistent. I 
> would prefer
> if that never happens, but somethings things slip through even close
> code review. If you know other cases, please let me know and 
> we fix them
> as well here.

All the other drivers implementing radio settings. Here's a couple of examples:

ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 134)        switch (mode) {
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 135)        case OFONO_RADIO_ACCESS_MODE_ANY:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 136)                value = 1;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 137)                break;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 138)        case OFONO_RADIO_ACCESS_MODE_GSM:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 139)                value = 0;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 140)                break;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 141)        case OFONO_RADIO_ACCESS_MODE_UMTS:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 142)                value = 2;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 143)                break;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 144)        default:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 145)                CALLBACK_WITH_FAILURE(cb, data);
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 146)                g_free(cbd);
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 147)                return;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 148)        }

30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 133)        switch (mode) {
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 134)        case OFONO_RADIO_ACCESS_MODE_ANY:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 135)                value = 5;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 136)                break;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 137)        case OFONO_RADIO_ACCESS_MODE_GSM:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 138)                value = 0;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 139)                break;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 140)        case OFONO_RADIO_ACCESS_MODE_UMTS:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 141)                value = 1;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 142)                break;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 143)        default:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 144)                CALLBACK_WITH_FAILURE(cb, data);
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 145)                g_free(cbd);
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 146)                return;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 147)        }

Sorry, couldn't resist. ;-)

Br,

	MikaL

RE: [PATCH 2/6] stemodem: add default case

[Marcel Holtmann]

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

Hi Mika,

> > > That must be a new policy then, considering that stemodem 
> > is the only one that failed compilation. Feel free to fix 
> > this one. The first two patches in the set are unrelated to 
> > fast dormancy anyway.
> > 
> > that is the whole point here. You modified the enum and the 
> > compilation
> > should fail unless you add a statement for that new item.
> > 
> > Let me make this clear, I do want the compilation to fail here and the
> > STE driver is doing the right thing.
> 
> I understand that. As I said, feel free to fix. Fast dormancy patches 3-6 should apply just fine without the first two courtesy patches.

I am waiting for Denis review since he was looking at them initially.
Once these are applied, we can take care of the fallout.

> > There might be other cases where this is not consistent. I 
> > would prefer
> > if that never happens, but somethings things slip through even close
> > code review. If you know other cases, please let me know and 
> > we fix them
> > as well here.
> 
> All the other drivers implementing radio settings. Here's a couple of examples:
> 
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 134)        switch (mode) {
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 135)        case OFONO_RADIO_ACCESS_MODE_ANY:
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 136)                value = 1;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 137)                break;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 138)        case OFONO_RADIO_ACCESS_MODE_GSM:
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 139)                value = 0;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 140)                break;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 141)        case OFONO_RADIO_ACCESS_MODE_UMTS:
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 142)                value = 2;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 143)                break;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 144)        default:
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 145)                CALLBACK_WITH_FAILURE(cb, data);
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 146)                g_free(cbd);
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 147)                return;
> ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 148)        }
> 
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 133)        switch (mode) {
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 134)        case OFONO_RADIO_ACCESS_MODE_ANY:
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 135)                value = 5;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 136)                break;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 137)        case OFONO_RADIO_ACCESS_MODE_GSM:
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 138)                value = 0;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 139)                break;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 140)        case OFONO_RADIO_ACCESS_MODE_UMTS:
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 141)                value = 1;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 142)                break;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 143)        default:
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 144)                CALLBACK_WITH_FAILURE(cb, data);
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 145)                g_free(cbd);
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 146)                return;
> 30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 147)        }
> 
> Sorry, couldn't resist. ;-)

I have no problem with that being pointing out. I actually encourage
people pointing this out. This is clearly myself being stupid. And good
thing is that this is open source, so I fixed it.

With a code base of 100k lines of code and more, such things happen. And
even with Denis and myself cross-reviewing each other this will still
happen. Sometimes things just slip through the cracks. Part of
development life. So once noticed this will be dealt with.

Regards

Marcel

Re: [PATCH 1/6] radio settings: fix mode initializion

[Denis Kenzior]

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

Hi Mika,

On 10/26/2010 10:31 AM, Mika Liljeberg wrote:
> ---
>  include/radio-settings.h |    9 +++++----
>  src/radio-settings.c     |    2 +-
>  2 files changed, 6 insertions(+), 5 deletions(-)

I decided to skip this patch.

Regards,
-Denis

Re: [PATCH 3/6] radio settings: add FastDormancy property

[Denis Kenzior]

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

Hi Mika,

On 10/26/2010 10:31 AM, Mika Liljeberg wrote:
> ---
>  include/radio-settings.h |   13 +++++
>  src/radio-settings.c     |  114 +++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 121 insertions(+), 6 deletions(-)
> 

Patch has been applied, thanks.  I did make a couple of minor tweaks
afterwards.

Regards,
-Denis

Re: [PATCH 4/6] test: add script to control fast dormancy

[Denis Kenzior]

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

Hi Mika,

On 10/26/2010 10:31 AM, Mika Liljeberg wrote:
> ---
>  Makefile.am            |    3 ++-
>  test/set-fast-dormancy |   25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletions(-)
>  create mode 100755 test/set-fast-dormancy

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 5/6] isimodem: add support for FastDormancy property

[Denis Kenzior]

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

Hi Mika,

On 10/26/2010 10:31 AM, Mika Liljeberg wrote:
> ---
>  drivers/isimodem/radio-settings.c |   91 ++++++++++++++++++++++++++++++++++++-
>  1 files changed, 90 insertions(+), 1 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 6/6] TODO: mark fast dormancy as done

[Denis Kenzior]

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

Hi Mika,

On 10/26/2010 10:31 AM, Mika Liljeberg wrote:
> ---
>  TODO             |   20 --------------------
>  doc/features.txt |    8 ++++++++
>  2 files changed, 8 insertions(+), 20 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

RE: [PATCH 3/6] radio settings: add FastDormancy property

[Mika.Liljeberg]

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

Hi Denis,

> Patch has been applied, thanks.  I did make a couple of minor tweaks
> afterwards.

Thanks. A question regarding this particular tweak:

diff --git a/src/radio-settings.c b/src/radio-settings.c
index cb0a598..eb2a42d 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -126,8 +126,7 @@ static void radio_set_fast_dormancy(struct ofono_radio_settings *rs,
        const char *path = __ofono_atom_get_path(rs->atom);
        dbus_bool_t value = enable;
 
-       if ((rs->flags & RADIO_SETTINGS_FLAG_CACHED) &&
-               rs->fast_dormancy == enable)
+       if (rs->fast_dormancy == enable)
                return;
 
        ofono_dbus_signal_property_changed(conn, path,

The fundamental problem here is that there is only a single CACHED flag for multiple properties, which may be modified individually. So, either you get extra signals or you get too few. I checked the CACHED flag here because otherwise the following might happen:

1. client tries to GetProperties() and gets the "Operation already in progress" error.
2. client waits for PropertyChanged signal to get the FastDormancy value
3. signal never comes because the default value happens to match the one returned by the driver and the signal is suppressed

I do agree that sending extra signals is bad but I think that not sending a signal is even worse. If the client cannot rely on getting a PropertyChanged signal after a busy error, all it can do is resort to polling. I.e., every client has to implement a polling pattern for GetProperties:

while (GetProperties() == BUSY)
	sleep(FOR_A_WHILE);

Having a separate CACHED flag for each value would solve this optimally. Failing that, I don't think a few extra signals is so bad. Forcing clients to poll is just ugly.

Am I missing something?

	MikaL

Re: [PATCH 3/6] radio settings: add FastDormancy property

[Denis Kenzior]

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

Hi Mika,

On 10/28/2010 08:32 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi Denis,
> 
>> Patch has been applied, thanks.  I did make a couple of minor tweaks
>> afterwards.
> 
> Thanks. A question regarding this particular tweak:
> 
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> index cb0a598..eb2a42d 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -126,8 +126,7 @@ static void radio_set_fast_dormancy(struct ofono_radio_settings *rs,
>         const char *path = __ofono_atom_get_path(rs->atom);
>         dbus_bool_t value = enable;
>  
> -       if ((rs->flags & RADIO_SETTINGS_FLAG_CACHED) &&
> -               rs->fast_dormancy == enable)
> +       if (rs->fast_dormancy == enable)
>                 return;
>  
>         ofono_dbus_signal_property_changed(conn, path,
> 
> The fundamental problem here is that there is only a single CACHED flag for multiple properties, which may be modified individually. So, either you get extra signals or you get too few. I checked the CACHED flag here because otherwise the following might happen:

Yes, I know.  But this problem is present in every single atom.  oFono
does not guarantee that every attribute is signaled when the atom is
initialized.

> 
> 1. client tries to GetProperties() and gets the "Operation already in progress" error.
> 2. client waits for PropertyChanged signal to get the FastDormancy value
> 3. signal never comes because the default value happens to match the one returned by the driver and the signal is suppressed
>

In general I think that for interfaces where this can happen, the
likelihood is very low that it actually will in the real world.

Do note that I have had the same argument with myself off and on for the
past two years.  So far this was never raised as an issue.  If this ever
becomes a problem, we can fix it properly using an appropriate idiom.

> I do agree that sending extra signals is bad but I think that not sending a signal is even worse. If the client cannot rely on getting a PropertyChanged signal after a busy error, all it can do is resort to polling. I.e., every client has to implement a polling pattern for GetProperties:
> 
> while (GetProperties() == BUSY)
> 	sleep(FOR_A_WHILE);
> 
> Having a separate CACHED flag for each value would solve this optimally. Failing that, I don't think a few extra signals is so bad. Forcing clients to poll is just ugly.
> 

Honestly, if you expect multiple applications to battle over the
FastDormancy property, then it should be modeled differently.  Perhaps
with application registration and lifetime tracking over D-Bus, similar
to how agents work.

> Am I missing something?
> 
> 	MikaL

Regards,
-Denis

RE: [PATCH 3/6] radio settings: add FastDormancy property

[Mika.Liljeberg]

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

Hi Denis,

> > The fundamental problem here is that there is only a single 
> CACHED flag for multiple properties, which may be modified 
> individually. So, either you get extra signals or you get too 
> few. I checked the CACHED flag here because otherwise the 
> following might happen:
> 
> Yes, I know.  But this problem is present in every single atom.  oFono
> does not guarantee that every attribute is signaled when the atom is
> initialized.

So I gathered. To me this looks like a wider issue, though. InProgress errors are returned in many other contexts as well, and they are not that well documented in the API documentation. All this makes it a bit painful to write a robust oFono client. Probably this could be at least partially rectified by improving the documentation.

> > 1. client tries to GetProperties() and gets the "Operation 
> already in progress" error.
> > 2. client waits for PropertyChanged signal to get the 
> FastDormancy value
> > 3. signal never comes because the default value happens to 
> match the one returned by the driver and the signal is suppressed
> >
> 
> In general I think that for interfaces where this can happen, the
> likelihood is very low that it actually will in the real world.

Perhaps so. The network and SIM interfaces, which are most likely to be bombared by multiple UI components, seem to be doing the right thing at least.

> Do note that I have had the same argument with myself off and 
> on for the
> past two years.  So far this was never raised as an issue.  
> If this ever
> becomes a problem, we can fix it properly using an appropriate idiom.

If this becomes a problem, it won't necessarily be visible to upstream. More likely this will be noticed in product maturization phase, and the fixes made to a product specific stable branches might never trickle back to upstream.

> > I do agree that sending extra signals is bad but I think 
> that not sending a signal is even worse. If the client cannot 
> rely on getting a PropertyChanged signal after a busy error, 
> all it can do is resort to polling. I.e., every client has to 
> implement a polling pattern for GetProperties:
> > 
> > while (GetProperties() == BUSY)
> > 	sleep(FOR_A_WHILE);
> > 
> > Having a separate CACHED flag for each value would solve 
> this optimally. Failing that, I don't think a few extra 
> signals is so bad. Forcing clients to poll is just ugly.
> > 
> 
> Honestly, if you expect multiple applications to battle over the
> FastDormancy property, then it should be modeled differently.  Perhaps
> with application registration and lifetime tracking over 
> D-Bus, similar
> to how agents work.

Hardly that, FastDormancy is unlikely to be a problem. I was merely curious whether there is a general design rule underneath, or if these things are decided on a case by case basis. Based on your comments and looking at the code, I guess it's more case by case. I just feel uneasy about an API that returns transient errors by design, on the (even if informed) assumption that it will probably be ok. I also dislike the fact that a generic InProgress error pretty much forces a client to just retry each operation until it succeeds. If there are problems like this, they are most likely discovered only in the product maturization phase and then it's generally too late to fix the upstream. Too late for that particular product, that is.

Br,

	MikaL

Re: [PATCH 3/6] radio settings: add FastDormancy property

[Denis Kenzior]

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

Hi Mika,

<snip>

> 
> So I gathered. To me this looks like a wider issue, though. InProgress errors are returned in many other contexts as well, and they are not that well documented in the API documentation. All this makes it a bit painful to write a robust oFono client. Probably this could be at least partially rectified by improving the documentation.
> 

Of course, and patches are always welcome.

<snip>

>> Do note that I have had the same argument with myself off and 
>> on for the
>> past two years.  So far this was never raised as an issue.  
>> If this ever
>> becomes a problem, we can fix it properly using an appropriate idiom.
> 
> If this becomes a problem, it won't necessarily be visible to upstream. More likely this will be noticed in product maturization phase, and the fixes made to a product specific stable branches might never trickle back to upstream.
>

I disagree.  I think upstream will be notified and we will improve the
situation as we progress.  But I do agree it might be too late for that
particular product.  Of course that is the case with all software.

<snip>

>> Honestly, if you expect multiple applications to battle over the
>> FastDormancy property, then it should be modeled differently.  Perhaps
>> with application registration and lifetime tracking over 
>> D-Bus, similar
>> to how agents work.
> 
> Hardly that, FastDormancy is unlikely to be a problem. I was merely curious whether there is a general design rule underneath, or if these things are decided on a case by case basis. Based on your comments and looking at the code, I guess it's more case by case. I just feel uneasy about an API that returns transient errors by design, on the (even if informed) assumption that it will probably be ok. I also dislike the fact that a generic InProgress error pretty much forces a client to just retry each operation until it succeeds. If there are problems like this, they are most likely discovered only in the product maturization phase and then it's generally too late to fix the upstream. Too late for that particular product, that is.
> 

One of the biggest principles in oFono is not to perform premature
optimization.  Sure there is a potential issue, but nobody knows whether
it will actually manifest itself outside of malicious code (which we
tell to bugger off) or what the most common manifestation pattern will be.

If / once we know for sure this is a problem, then we can solve it
properly.  So far this approach has been working very nicely for us.
There are countless occasions where taking the wait-and-see approach and
gathering more information allowed us to devise a much better solution
than we would have originally.

So the general rule is: Do the simplest thing that is likely to work.
If it doesn't work, improve it.

> Br,
> 
> 	MikaL

Regards,
-Denis

RE: [PATCH 3/6] radio settings: add FastDormancy property

[Mika.Liljeberg]

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

Hi Denis,

> One of the biggest principles in oFono is not to perform premature
> optimization.  Sure there is a potential issue, but nobody 
> knows whether
> it will actually manifest itself outside of malicious code (which we
> tell to bugger off) or what the most common manifestation 
> pattern will be.
> 
> If / once we know for sure this is a problem, then we can solve it
> properly.  So far this approach has been working very nicely for us.
> There are countless occasions where taking the wait-and-see 
> approach and
> gathering more information allowed us to devise a much better solution
> than we would have originally.
> 
> So the general rule is: Do the simplest thing that is likely to work.
> If it doesn't work, improve it.

Can't fault the approach, it's generally a good one. That said, I see this more as an API quality issue rather than an optimization issue. Anyway, I've raised the concern. Let's hope my worries are unfounded.

Br,

	MikaL