[PATCH 1/4] radio settings: allow for more than one property

[PATCH 1/4] radio settings: allow for more than one property

[Mika Liljeberg]

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

Restructure code to allow other properties besides
TechnologyPreference to be returned.
---
 src/radio-settings.c |   60 +++++++++++++++++++++++++-------------------------
 1 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/src/radio-settings.c b/src/radio-settings.c
index f70d870..7cdd411 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -77,15 +77,16 @@ static int string_to_radio_access_mode(const char *mode)
 	return -1;
 }
 
-static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
-						struct ofono_radio_settings *rs)
+static void radio_rat_mode_query_callback(const struct ofono_error *error,
+					enum ofono_radio_access_mode mode,
+					void *data);
+
+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;
@@ -96,8 +97,17 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 					OFONO_PROPERTIES_ARRAY_SIGNATURE,
 					&dict);
 
-	ofono_dbus_dict_append(&dict, "TechnologyPreference", DBUS_TYPE_STRING,
-				&mode);
+	if (rs->flags & RADIO_SETTINGS_MODE_CACHED) {
+		const char *mode = radio_access_mode_to_string(rs->mode);
+		ofono_dbus_dict_append(&dict, "TechnologyPreference",
+				DBUS_TYPE_STRING, &mode);
+	} else if (rs->driver->query_rat_mode) {
+		rs->pending = dbus_message_ref(msg);
+		rs->driver->query_rat_mode(rs,
+					radio_rat_mode_query_callback, rs);
+		dbus_message_unref(reply);
+		return NULL;
+	}
 
 	dbus_message_iter_close_container(&iter, &dict);
 
@@ -107,23 +117,21 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 static void radio_set_rat_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->flags & RADIO_SETTINGS_MODE_CACHED) &&
+		rs->mode != (int)mode) {
 
-	if (rs->mode == (int)mode)
-		return;
+		DBusConnection *conn = ofono_dbus_get_connection();
+		const char *path = __ofono_atom_get_path(rs->atom);
+		const char *str_mode = radio_access_mode_to_string(rs->mode);
+
+		ofono_dbus_signal_property_changed(conn, path,
+						OFONO_RADIO_SETTINGS_INTERFACE,
+						"TechnologyPreference",
+						DBUS_TYPE_STRING, &str_mode);
+	}
 
 	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,
-					OFONO_RADIO_SETTINGS_INTERFACE,
-					"TechnologyPreference", DBUS_TYPE_STRING,
-					&str_mode);
 }
 
 static void radio_mode_set_callback(const struct ofono_error *error, void *data)
@@ -162,7 +170,8 @@ 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);
+	if (reply)
+		__ofono_dbus_pending_reply(&rs->pending, reply);
 }
 
 static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
@@ -170,19 +179,10 @@ static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
 {
 	struct ofono_radio_settings *rs = data;
 
-	if (rs->flags & RADIO_SETTINGS_MODE_CACHED)
-		return radio_get_properties_reply(msg, rs);
-
-	if (!rs->driver->query_rat_mode)
-		return __ofono_error_not_implemented(msg);
-
 	if (rs->pending)
 		return __ofono_error_busy(msg);
 
-	rs->pending = dbus_message_ref(msg);
-	rs->driver->query_rat_mode(rs, radio_rat_mode_query_callback, rs);
-
-	return NULL;
+	return radio_get_properties_reply(msg, rs);
 }
 
 static DBusMessage *radio_set_property(DBusConnection *conn, DBusMessage *msg,
-- 
1.7.0.4

[PATCH 0/4] Support for fast dormancy - take 2

[Mika Liljeberg]

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

Oops, a couple of checkpatch issues had crept in since last check.
Overlong lines removed.

Apologies,

	MikaL

[PATCH 1/4] radio settings: allow for more than one property
[PATCH 2/4] radio settings: add FastDormancy property
[PATCH 3/4] radio settings: document FastDormancy property
[PATCH 4/4] test: add scripts to enable and disable fast dormancy

[PATCH 1/4] radio settings: allow for more than one property

[Mika Liljeberg]

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

Restructure code to allow other properties besides
TechnologyPreference to be returned.
---
 src/radio-settings.c |   59 +++++++++++++++++++++++++------------------------
 1 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/src/radio-settings.c b/src/radio-settings.c
index f70d870..5b6ef9e 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -77,15 +77,17 @@ static int string_to_radio_access_mode(const char *mode)
 	return -1;
 }
 
+static void radio_rat_mode_query_callback(const struct ofono_error *error,
+					enum ofono_radio_access_mode mode,
+					void *data);
+
 static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
-						struct ofono_radio_settings *rs)
+					       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;
@@ -96,8 +98,17 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 					OFONO_PROPERTIES_ARRAY_SIGNATURE,
 					&dict);
 
-	ofono_dbus_dict_append(&dict, "TechnologyPreference", DBUS_TYPE_STRING,
-				&mode);
+	if (rs->flags & RADIO_SETTINGS_MODE_CACHED) {
+		const char *mode = radio_access_mode_to_string(rs->mode);
+		ofono_dbus_dict_append(&dict, "TechnologyPreference",
+				DBUS_TYPE_STRING, &mode);
+	} else if (rs->driver->query_rat_mode) {
+		rs->pending = dbus_message_ref(msg);
+		rs->driver->query_rat_mode(rs,
+					radio_rat_mode_query_callback, rs);
+		dbus_message_unref(reply);
+		return NULL;
+	}
 
 	dbus_message_iter_close_container(&iter, &dict);
 
@@ -107,23 +118,21 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 static void radio_set_rat_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->flags & RADIO_SETTINGS_MODE_CACHED) &&
+		rs->mode != (int)mode) {
 
-	if (rs->mode == (int)mode)
-		return;
+		DBusConnection *conn = ofono_dbus_get_connection();
+		const char *path = __ofono_atom_get_path(rs->atom);
+		const char *str_mode = radio_access_mode_to_string(rs->mode);
+
+		ofono_dbus_signal_property_changed(conn, path,
+						OFONO_RADIO_SETTINGS_INTERFACE,
+						"TechnologyPreference",
+						DBUS_TYPE_STRING, &str_mode);
+	}
 
 	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,
-					OFONO_RADIO_SETTINGS_INTERFACE,
-					"TechnologyPreference", DBUS_TYPE_STRING,
-					&str_mode);
 }
 
 static void radio_mode_set_callback(const struct ofono_error *error, void *data)
@@ -162,7 +171,8 @@ 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);
+	if (reply)
+		__ofono_dbus_pending_reply(&rs->pending, reply);
 }
 
 static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
@@ -170,19 +180,10 @@ static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
 {
 	struct ofono_radio_settings *rs = data;
 
-	if (rs->flags & RADIO_SETTINGS_MODE_CACHED)
-		return radio_get_properties_reply(msg, rs);
-
-	if (!rs->driver->query_rat_mode)
-		return __ofono_error_not_implemented(msg);
-
 	if (rs->pending)
 		return __ofono_error_busy(msg);
 
-	rs->pending = dbus_message_ref(msg);
-	rs->driver->query_rat_mode(rs, radio_rat_mode_query_callback, rs);
-
-	return NULL;
+	return radio_get_properties_reply(msg, rs);
 }
 
 static DBusMessage *radio_set_property(DBusConnection *conn, DBusMessage *msg,
-- 
1.7.0.4

[PATCH 2/4] radio settings: add FastDormancy property

[Mika Liljeberg]

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

---
 include/radio-settings.h |   14 ++++++
 src/radio-settings.c     |  104 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 117 insertions(+), 1 deletions(-)

diff --git a/include/radio-settings.h b/include/radio-settings.h
index d41ec0b..20c2a51 100644
--- a/include/radio-settings.h
+++ b/include/radio-settings.h
@@ -42,6 +42,13 @@ typedef void (*ofono_radio_settings_rat_mode_set_cb_t)(const struct ofono_error
 typedef void (*ofono_radio_settings_rat_mode_query_cb_t)(const struct ofono_error *error,
 						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;
@@ -55,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,
+				ofono_bool_t 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 5b6ef9e..e6f0980 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -33,7 +33,9 @@
 #include "ofono.h"
 #include "common.h"
 
-#define RADIO_SETTINGS_MODE_CACHED 0x1
+#define RADIO_SETTINGS_MODE_CACHED	0x01
+#define FAST_DORMANCY_SETTING_CACHED	0x02
+
 
 static GSList *g_drivers = NULL;
 
@@ -42,6 +44,8 @@ struct ofono_radio_settings {
 	int flags;
 	int mode;
 	int pending_mode;
+	ofono_bool_t fast_dormancy_current;
+	ofono_bool_t fast_dormancy_target;
 	const struct ofono_radio_settings_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -80,6 +84,9 @@ static int string_to_radio_access_mode(const char *mode)
 static void radio_rat_mode_query_callback(const struct ofono_error *error,
 					enum ofono_radio_access_mode mode,
 					void *data);
+static void fast_dormancy_query_callback(const struct ofono_error *error,
+					ofono_bool_t enable,
+					void *data);
 
 static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 					       struct ofono_radio_settings *rs)
@@ -110,6 +117,17 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
 		return NULL;
 	}
 
+	if (rs->flags & FAST_DORMANCY_SETTING_CACHED) {
+		dbus_bool_t value = rs->fast_dormancy_current;
+		ofono_dbus_dict_append(&dict, "FastDormancy",
+				DBUS_TYPE_BOOLEAN, &value);
+	} else if (rs->driver->query_fast_dormancy) {
+		rs->driver->query_fast_dormancy(rs,
+					fast_dormancy_query_callback, rs);
+		dbus_message_unref(reply);
+		return NULL;
+	}
+
 	dbus_message_iter_close_container(&iter, &dict);
 
 	return reply;
@@ -175,6 +193,67 @@ static void radio_rat_mode_query_callback(const struct ofono_error *error,
 		__ofono_dbus_pending_reply(&rs->pending, reply);
 }
 
+static void set_fast_dormancy(struct ofono_radio_settings *rs,
+			ofono_bool_t enable)
+{
+	if ((rs->flags & FAST_DORMANCY_SETTING_CACHED) &&
+		rs->fast_dormancy_current != enable) {
+
+		DBusConnection *conn = ofono_dbus_get_connection();
+		const char *path = __ofono_atom_get_path(rs->atom);
+		dbus_bool_t value = enable;
+
+		ofono_dbus_signal_property_changed(conn, path,
+						OFONO_RADIO_SETTINGS_INTERFACE,
+						"FastDormancy",
+						DBUS_TYPE_BOOLEAN, &value);
+	}
+
+	rs->fast_dormancy_current = enable;
+	rs->flags |= FAST_DORMANCY_SETTING_CACHED;
+}
+
+static void 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_target = rs->fast_dormancy_current;
+		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);
+
+	set_fast_dormancy(rs, rs->fast_dormancy_target);
+}
+
+static void 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;
+	}
+
+	set_fast_dormancy(rs, enable);
+
+	reply = radio_get_properties_reply(rs->pending, rs);
+	if (reply)
+		__ofono_dbus_pending_reply(&rs->pending, reply);
+}
+
 static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
 					void *data)
 {
@@ -236,6 +315,29 @@ 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;
+		ofono_bool_t 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_target == target)
+			return dbus_message_new_method_return(msg);
+
+		rs->pending = dbus_message_ref(msg);
+		rs->fast_dormancy_target = target;
+
+		rs->driver->set_fast_dormancy(rs, target,
+					fast_dormancy_set_callback, rs);
+
+		return NULL;
 	}
 
 	return __ofono_error_invalid_args(msg);
-- 
1.7.0.4

[PATCH 3/4] radio settings: document FastDormancy property

[Mika Liljeberg]

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

---
 doc/radio-settings-api.txt |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/doc/radio-settings-api.txt b/doc/radio-settings-api.txt
index 3372abd..f1b91ad 100644
--- a/doc/radio-settings-api.txt
+++ b/doc/radio-settings-api.txt
@@ -41,3 +41,25 @@ Properties	string TechnologyPreference [read-write]
 				"gsm"	Only GSM used for radio access.
 				"umts"	Only UMTS used for radio access.
 				"lte"	Only LTE used for radio acccess.
+
+		boolean	FastDormancy [read-write, optional]
+
+			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, for instance, when the 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.
-- 
1.7.0.4

[PATCH 4/4] test: add scripts to enable and disable fast dormancy

[Mika Liljeberg]

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

---
 Makefile.am                |    4 +++-
 test/disable-fast-dormancy |   20 ++++++++++++++++++++
 test/enable-fast-dormancy  |   20 ++++++++++++++++++++
 3 files changed, 43 insertions(+), 1 deletions(-)
 create mode 100755 test/disable-fast-dormancy
 create mode 100755 test/enable-fast-dormancy

diff --git a/Makefile.am b/Makefile.am
index 6c56459..7a56cb4 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -407,7 +407,9 @@ test_scripts = test/backtrace \
 		test/lock-pin \
 		test/unlock-pin \
 		test/enable-gprs \
-		test/disable-gprs
+		test/disable-gprs \
+		test/enable-fast-dormancy \
+		test/disable-fast-dormancy
 
 if TEST
 testdir = $(pkglibdir)/test
diff --git a/test/disable-fast-dormancy b/test/disable-fast-dormancy
new file mode 100755
index 0000000..ec326aa
--- /dev/null
+++ b/test/disable-fast-dormancy
@@ -0,0 +1,20 @@
+#!/usr/bin/python
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 2:
+	path = sys.argv[1]
+else:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+			'org.ofono.Manager')
+	modems = manager.GetModems()
+	path = modems[0][0]
+
+print "Disabling fast dormancy on modem %s..." % path
+cm = dbus.Interface(bus.get_object('org.ofono', path),
+		'org.ofono.RadioSettings')
+
+cm.SetProperty("FastDormancy", dbus.Boolean(0))
diff --git a/test/enable-fast-dormancy b/test/enable-fast-dormancy
new file mode 100755
index 0000000..93ec1d5
--- /dev/null
+++ b/test/enable-fast-dormancy
@@ -0,0 +1,20 @@
+#!/usr/bin/python
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 2:
+	path = sys.argv[1]
+else:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+			'org.ofono.Manager')
+	modems = manager.GetModems()
+	path = modems[0][0]
+
+print "Enabling fast dormancy on modem %s..." % path
+cm = dbus.Interface(bus.get_object('org.ofono', path),
+		'org.ofono.RadioSettings')
+
+cm.SetProperty("FastDormancy", dbus.Boolean(1))
-- 
1.7.0.4

Re: [PATCH 3/4] radio settings: document FastDormancy property

[Marcel Holtmann]

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

Hi Mika,

> diff --git a/doc/radio-settings-api.txt b/doc/radio-settings-api.txt
> index 3372abd..f1b91ad 100644
> --- a/doc/radio-settings-api.txt
> +++ b/doc/radio-settings-api.txt
> @@ -41,3 +41,25 @@ Properties	string TechnologyPreference [read-write]
>  				"gsm"	Only GSM used for radio access.
>  				"umts"	Only UMTS used for radio access.
>  				"lte"	Only LTE used for radio acccess.
> +
> +		boolean	FastDormancy [read-write, optional]
> +
> +			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, for instance, when the 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.

so how does the future prediction is suppose to work. Are we shipping a
time machine together with oFono ;)

Regards

Marcel

RE: [PATCH 3/4] radio settings: document FastDormancy property

[Mika.Liljeberg]

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

Hi,

> so how does the future prediction is suppose to work. Are we 
> shipping a
> time machine together with oFono ;)

I would say the heuristic is system dependent. Typical way would be to monitor events from different input devices. If no events arrive for a specified time the user is assumed to be inactive. The user might also have some explicit way to do this, such as locking the display and keys.

	MikaL

RE: [PATCH 3/4] radio settings: document FastDormancy property

[Marcel Holtmann]

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

Hi Mika,

> > so how does the future prediction is suppose to work. Are we 
> > shipping a
> > time machine together with oFono ;)
> 
> I would say the heuristic is system dependent. Typical way would be to monitor events from different input devices. If no events arrive for a specified time the user is assumed to be inactive. The user might also have some explicit way to do this, such as locking the display and keys.

and by that extend we drain the battery since every single component has
to send keep-alive messages around. Or it has to accept that the wakeup
time for its GPRS connection is horrible.

I am not per-se against doing the interface like this, but we have
discussed this in the beginning of the year. And I like to see how we
are planning to use this API to support fast dormancy properly and most
efficient.

So lets take a MeeGo system running on a mobile phone, how would we use
this property for this. What other system components do we have to
monitor? Which are our input sources for this?

Help me to get the full picture of this and how it should be integrated
into a complete system. Just checking of the fast dormancy check box is
not gonna be enough here.

Regards

Marcel

Re: [PATCH 1/4] radio settings: allow for more than one property

[Denis Kenzior]

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

Hi Mika,

On 10/07/2010 08:37 AM, Mika Liljeberg wrote:
> Restructure code to allow other properties besides
> TechnologyPreference to be returned.
> ---
>  src/radio-settings.c |   59 +++++++++++++++++++++++++------------------------
>  1 files changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> index f70d870..5b6ef9e 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -77,15 +77,17 @@ static int string_to_radio_access_mode(const char *mode)
>  	return -1;
>  }
>  
> +static void radio_rat_mode_query_callback(const struct ofono_error *error,
> +					enum ofono_radio_access_mode mode,
> +					void *data);
> +
>  static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
> -						struct ofono_radio_settings *rs)
> +					       struct ofono_radio_settings *rs)

Please don't mix and match tabs and spaces for indentation

>  {
>  	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;
> @@ -96,8 +98,17 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>  					OFONO_PROPERTIES_ARRAY_SIGNATURE,
>  					&dict);
>  
> -	ofono_dbus_dict_append(&dict, "TechnologyPreference", DBUS_TYPE_STRING,
> -				&mode);
> +	if (rs->flags & RADIO_SETTINGS_MODE_CACHED) {
> +		const char *mode = radio_access_mode_to_string(rs->mode);
> +		ofono_dbus_dict_append(&dict, "TechnologyPreference",
> +				DBUS_TYPE_STRING, &mode);
> +	} else if (rs->driver->query_rat_mode) {
> +		rs->pending = dbus_message_ref(msg);
> +		rs->driver->query_rat_mode(rs,
> +					radio_rat_mode_query_callback, rs);
> +		dbus_message_unref(reply);
> +		return NULL;
> +	}
>  
>  	dbus_message_iter_close_container(&iter, &dict);
>  
> @@ -107,23 +118,21 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>  static void radio_set_rat_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->flags & RADIO_SETTINGS_MODE_CACHED) &&
> +		rs->mode != (int)mode) {
>  
> -	if (rs->mode == (int)mode)
> -		return;
> +		DBusConnection *conn = ofono_dbus_get_connection();
> +		const char *path = __ofono_atom_get_path(rs->atom);
> +		const char *str_mode = radio_access_mode_to_string(rs->mode);
> +
> +		ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_RADIO_SETTINGS_INTERFACE,
> +						"TechnologyPreference",
> +						DBUS_TYPE_STRING, &str_mode);
> +	}
>  
>  	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,
> -					OFONO_RADIO_SETTINGS_INTERFACE,
> -					"TechnologyPreference", DBUS_TYPE_STRING,
> -					&str_mode);
>  }
>  
>  static void radio_mode_set_callback(const struct ofono_error *error, void *data)
> @@ -162,7 +171,8 @@ 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);
> +	if (reply)
> +		__ofono_dbus_pending_reply(&rs->pending, reply);

Actually I would like to avoid this.  If two applications send
GetProperties() at the same time and the properties have not been cached
yet, we should tell the second one to bugger off.  It will receive the
properties as signals in a little while anyway.

Your proposal would generate queries for each client, which can
potentially spam the modem with unneeded commands.

>  }
>  
>  static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
> @@ -170,19 +180,10 @@ static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
>  {
>  	struct ofono_radio_settings *rs = data;
>  
> -	if (rs->flags & RADIO_SETTINGS_MODE_CACHED)
> -		return radio_get_properties_reply(msg, rs);
> -
> -	if (!rs->driver->query_rat_mode)
> -		return __ofono_error_not_implemented(msg);
> -
>  	if (rs->pending)
>  		return __ofono_error_busy(msg);
>  
> -	rs->pending = dbus_message_ref(msg);
> -	rs->driver->query_rat_mode(rs, radio_rat_mode_query_callback, rs);
> -
> -	return NULL;
> +	return radio_get_properties_reply(msg, rs);

I prefer you keep the current implementation as it is consistent with
the rest of ofono core.  The way the rest of the core works is that you
query each in sequence, then set the the CACHED flag (only one is
actually required).

We use a single cached flag to keep the logic simpler and optimize for
the general case (e.g. UI will query the properties first before
allowing the user to set them)  While this does lead to us querying some
attributes unnecessarily in certain extremely unlikely corner cases, I
believe it is an acceptable compromise.

>  }
>  
>  static DBusMessage *radio_set_property(DBusConnection *conn, DBusMessage *msg,

Regards,
-Denis

Re: [PATCH 2/4] radio settings: add FastDormancy property

[Denis Kenzior]

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

Hi Mika,

On 10/07/2010 08:37 AM, Mika Liljeberg wrote:
> ---
>  include/radio-settings.h |   14 ++++++
>  src/radio-settings.c     |  104 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 117 insertions(+), 1 deletions(-)
> 
> diff --git a/include/radio-settings.h b/include/radio-settings.h
> index d41ec0b..20c2a51 100644
> --- a/include/radio-settings.h
> +++ b/include/radio-settings.h
> @@ -42,6 +42,13 @@ typedef void (*ofono_radio_settings_rat_mode_set_cb_t)(const struct ofono_error
>  typedef void (*ofono_radio_settings_rat_mode_query_cb_t)(const struct ofono_error *error,
>  						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;
> @@ -55,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,
> +				ofono_bool_t 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 5b6ef9e..e6f0980 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -33,7 +33,9 @@
>  #include "ofono.h"
>  #include "common.h"
>  
> -#define RADIO_SETTINGS_MODE_CACHED 0x1
> +#define RADIO_SETTINGS_MODE_CACHED	0x01
> +#define FAST_DORMANCY_SETTING_CACHED	0x02
> +

As mentioned in the previous email, a single cached flag is enough.

>  
>  static GSList *g_drivers = NULL;
>  
> @@ -42,6 +44,8 @@ struct ofono_radio_settings {
>  	int flags;
>  	int mode;
>  	int pending_mode;
> +	ofono_bool_t fast_dormancy_current;
> +	ofono_bool_t fast_dormancy_target;

can you rename this into fast_dormancy and fast_dormancy_pending?

>  	const struct ofono_radio_settings_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> @@ -80,6 +84,9 @@ static int string_to_radio_access_mode(const char *mode)
>  static void radio_rat_mode_query_callback(const struct ofono_error *error,
>  					enum ofono_radio_access_mode mode,
>  					void *data);
> +static void fast_dormancy_query_callback(const struct ofono_error *error,
> +					ofono_bool_t enable,
> +					void *data);
>  
>  static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>  					       struct ofono_radio_settings *rs)
> @@ -110,6 +117,17 @@ static DBusMessage *radio_get_properties_reply(DBusMessage *msg,
>  		return NULL;
>  	}
>  
> +	if (rs->flags & FAST_DORMANCY_SETTING_CACHED) {
> +		dbus_bool_t value = rs->fast_dormancy_current;
> +		ofono_dbus_dict_append(&dict, "FastDormancy",
> +				DBUS_TYPE_BOOLEAN, &value);

In patch 3 you note that this property is optional.  Am I missing
something or it is always included in the dictionary?

> +	} else if (rs->driver->query_fast_dormancy) {
> +		rs->driver->query_fast_dormancy(rs,
> +					fast_dormancy_query_callback, rs);
> +		dbus_message_unref(reply);
> +		return NULL;
> +	}
> +
>  	dbus_message_iter_close_container(&iter, &dict);
>  
>  	return reply;
> @@ -175,6 +193,67 @@ static void radio_rat_mode_query_callback(const struct ofono_error *error,
>  		__ofono_dbus_pending_reply(&rs->pending, reply);
>  }
>  
> +static void set_fast_dormancy(struct ofono_radio_settings *rs,
> +			ofono_bool_t enable)
> +{
> +	if ((rs->flags & FAST_DORMANCY_SETTING_CACHED) &&
> +		rs->fast_dormancy_current != enable) {
> +
> +		DBusConnection *conn = ofono_dbus_get_connection();
> +		const char *path = __ofono_atom_get_path(rs->atom);
> +		dbus_bool_t value = enable;
> +
> +		ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_RADIO_SETTINGS_INTERFACE,
> +						"FastDormancy",
> +						DBUS_TYPE_BOOLEAN, &value);
> +	}
> +
> +	rs->fast_dormancy_current = enable;
> +	rs->flags |= FAST_DORMANCY_SETTING_CACHED;
> +}
> +
> +static void 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_target = rs->fast_dormancy_current;
> +		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);
> +
> +	set_fast_dormancy(rs, rs->fast_dormancy_target);
> +}
> +
> +static void 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;
> +	}
> +
> +	set_fast_dormancy(rs, enable);
> +
> +	reply = radio_get_properties_reply(rs->pending, rs);
> +	if (reply)
> +		__ofono_dbus_pending_reply(&rs->pending, reply);
> +}
> +

Can you restructure the rest of the code according to comments in the
previous email?  E.g. on a get_properties: query rat, then query
dormancy and set cached flag.  Skip any queries which are not supported
by the driver.

>  static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
>  					void *data)
>  {
> @@ -236,6 +315,29 @@ 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;
> +		ofono_bool_t 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_target == target)
> +			return dbus_message_new_method_return(msg);
> +
> +		rs->pending = dbus_message_ref(msg);
> +		rs->fast_dormancy_target = target;
> +
> +		rs->driver->set_fast_dormancy(rs, target,
> +					fast_dormancy_set_callback, rs);
> +
> +		return NULL;

This part looks good

>  	}
>  
>  	return __ofono_error_invalid_args(msg);

Regards,
-Denis

Re: [PATCH 4/4] test: add scripts to enable and disable fast dormancy

[Denis Kenzior]

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

Hi Mika,

On 10/07/2010 08:37 AM, Mika Liljeberg wrote:
> ---
>  Makefile.am                |    4 +++-
>  test/disable-fast-dormancy |   20 ++++++++++++++++++++
>  test/enable-fast-dormancy  |   20 ++++++++++++++++++++
>  3 files changed, 43 insertions(+), 1 deletions(-)
>  create mode 100755 test/disable-fast-dormancy
>  create mode 100755 test/enable-fast-dormancy
> 
> diff --git a/Makefile.am b/Makefile.am
> index 6c56459..7a56cb4 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -407,7 +407,9 @@ test_scripts = test/backtrace \
>  		test/lock-pin \
>  		test/unlock-pin \
>  		test/enable-gprs \
> -		test/disable-gprs
> +		test/disable-gprs \
> +		test/enable-fast-dormancy \
> +		test/disable-fast-dormancy
>  
>  if TEST
>  testdir = $(pkglibdir)/test
> diff --git a/test/disable-fast-dormancy b/test/disable-fast-dormancy
> new file mode 100755
> index 0000000..ec326aa
> --- /dev/null
> +++ b/test/disable-fast-dormancy
> @@ -0,0 +1,20 @@
> +#!/usr/bin/python
> +
> +import dbus
> +import sys
> +
> +bus = dbus.SystemBus()
> +
> +if len(sys.argv) == 2:
> +	path = sys.argv[1]
> +else:
> +	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
> +			'org.ofono.Manager')
> +	modems = manager.GetModems()
> +	path = modems[0][0]
> +
> +print "Disabling fast dormancy on modem %s..." % path
> +cm = dbus.Interface(bus.get_object('org.ofono', path),
> +		'org.ofono.RadioSettings')
> +
> +cm.SetProperty("FastDormancy", dbus.Boolean(0))

Just a minor nitpick, but renaming cm into rs might be a bit more
consistent ;)

> diff --git a/test/enable-fast-dormancy b/test/enable-fast-dormancy
> new file mode 100755
> index 0000000..93ec1d5
> --- /dev/null
> +++ b/test/enable-fast-dormancy
> @@ -0,0 +1,20 @@
> +#!/usr/bin/python
> +
> +import dbus
> +import sys
> +
> +bus = dbus.SystemBus()
> +
> +if len(sys.argv) == 2:
> +	path = sys.argv[1]
> +else:
> +	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
> +			'org.ofono.Manager')
> +	modems = manager.GetModems()
> +	path = modems[0][0]
> +
> +print "Enabling fast dormancy on modem %s..." % path
> +cm = dbus.Interface(bus.get_object('org.ofono', path),
> +		'org.ofono.RadioSettings')
> +
> +cm.SetProperty("FastDormancy", dbus.Boolean(1))

Same comment as above.

Otherwise looks good.

Regards,
-Denis

Re: [PATCH 3/4] radio settings: document FastDormancy property

[Denis Kenzior]

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

Hi Mika,

On 10/07/2010 08:37 AM, Mika Liljeberg wrote:
> ---
>  doc/radio-settings-api.txt |   22 ++++++++++++++++++++++
>  1 files changed, 22 insertions(+), 0 deletions(-)
> 

This patch has been applied.  Can you include a short blurb of what is
expected from the modem driver in the patch covering the interface
definition?

Regards,
-Denis

RE: [PATCH 1/4] radio settings: allow for more than one property

[Mika.Liljeberg]

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

Hi,

> Please don't mix and match tabs and spaces for indentation

Oops, will fix. For some reason my checkpatch script did not complain about this.

You must be using a script as well. To avoid unnecessary fuss like this, would it be possible to integrate it to the oFono tree so everyone would check their patches using the same thing?

> I prefer you keep the current implementation as it is consistent with
> the rest of ofono core.  The way the rest of the core works 
> is that you
> query each in sequence, then set the the CACHED flag (only one is
> actually required).
> 
> We use a single cached flag to keep the logic simpler and optimize for
> the general case (e.g. UI will query the properties first before
> allowing the user to set them)  While this does lead to us 
> querying some
> attributes unnecessarily in certain extremely unlikely corner cases, I
> believe it is an acceptable compromise.

This was my intention as well but I did it in a different way, as I didn't realize there was an existing pattern for it.

My code does actually query all the properties up front but I serialized the driver queries in a different way. I.e., I used a separate CACHED flag for each property to keep track of whether the value is already valid. Each driver callback attempts to create the pending reply. The attempt to create the reply either succeeds or triggers a new driver query for another missing property value. This goes on until all the properties are cached and the pending reply can be sent.

I now see that the other services use a separate state machine to fetch the properties. That introduces more code than my solution but I guess I can do it that way if you feel it is easier to read.

	MikaL

Re: [PATCH 1/4] radio settings: allow for more than one property

[Denis Kenzior]

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

Hi Mika,

On 10/21/2010 03:55 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi,
> 
>> Please don't mix and match tabs and spaces for indentation
> 
> Oops, will fix. For some reason my checkpatch script did not complain about this.
> 
> You must be using a script as well. To avoid unnecessary fuss like this, would it be possible to integrate it to the oFono tree so everyone would check their patches using the same thing?

Not a script, but Johan's magic vim macro.  I paste it here for reference:
highlight RedundantWhitespace ctermbg=red guibg=red
match RedundantWhitespace /\s\+$\|\t\+\ze \+[^*]\| \+\ze\t/

Between checkpatch, git am and this one we catch most issues.

> 
>> I prefer you keep the current implementation as it is consistent with
>> the rest of ofono core.  The way the rest of the core works 
>> is that you
>> query each in sequence, then set the the CACHED flag (only one is
>> actually required).
>>
>> We use a single cached flag to keep the logic simpler and optimize for
>> the general case (e.g. UI will query the properties first before
>> allowing the user to set them)  While this does lead to us 
>> querying some
>> attributes unnecessarily in certain extremely unlikely corner cases, I
>> believe it is an acceptable compromise.
> 
> This was my intention as well but I did it in a different way, as I didn't realize there was an existing pattern for it.
> 
> My code does actually query all the properties up front but I serialized the driver queries in a different way. I.e., I used a separate CACHED flag for each property to keep track of whether the value is already valid. Each driver callback attempts to create the pending reply. The attempt to create the reply either succeeds or triggers a new driver query for another missing property value. This goes on until all the properties are cached and the pending reply can be sent.
> 
> I now see that the other services use a separate state machine to fetch the properties. That introduces more code than my solution but I guess I can do it that way if you feel it is easier to read.
> 

In the beginning we tried many approaches, including ones similar to the
path you took.  We found that the current approach used in the core,
while while by no means perfect in every situation, was the best
compromise.  It is quite scalable as the number of attributes / queries
grows and it blocks multiple (potentially malicious) applications from
DDoSing the modem when the information is not cached.

So while it might be a bit more code, I think it is the right approach
here as well; both for consistency and code readability reasons.

Regards,
-Denis

RE: [PATCH 1/4] radio settings: allow for more than one property

[Mika.Liljeberg]

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

Hi Denis, 

> > Oops, will fix. For some reason my checkpatch script did 
> not complain about this.
> > 
> > You must be using a script as well. To avoid unnecessary 
> fuss like this, would it be possible to integrate it to the 
> oFono tree so everyone would check their patches using the same thing?
> 
> Not a script, but Johan's magic vim macro.  I paste it here 
> for reference:
> highlight RedundantWhitespace ctermbg=red guibg=red
> match RedundantWhitespace /\s\+$\|\t\+\ze \+[^*]\| \+\ze\t/
> 
> Between checkpatch, git am and this one we catch most issues.

Thanks. Still, I'd vote for a common checkpatch script in the oFono tree. ;)

> 
> > 
> >> I prefer you keep the current implementation as it is 
> consistent with
> >> the rest of ofono core.  The way the rest of the core works 
> >> is that you
> >> query each in sequence, then set the the CACHED flag (only one is
> >> actually required).
> >>
> >> We use a single cached flag to keep the logic simpler and 
> optimize for
> >> the general case (e.g. UI will query the properties first before
> >> allowing the user to set them)  While this does lead to us 
> >> querying some
> >> attributes unnecessarily in certain extremely unlikely 
> corner cases, I
> >> believe it is an acceptable compromise.
> > 
> > This was my intention as well but I did it in a different 
> way, as I didn't realize there was an existing pattern for it.
> > 
> > My code does actually query all the properties up front but 
> I serialized the driver queries in a different way. I.e., I 
> used a separate CACHED flag for each property to keep track 
> of whether the value is already valid. Each driver callback 
> attempts to create the pending reply. The attempt to create 
> the reply either succeeds or triggers a new driver query for 
> another missing property value. This goes on until all the 
> properties are cached and the pending reply can be sent.
> > 
> > I now see that the other services use a separate state 
> machine to fetch the properties. That introduces more code 
> than my solution but I guess I can do it that way if you feel 
> it is easier to read.
> > 
> 
> In the beginning we tried many approaches, including ones 
> similar to the
> path you took.  We found that the current approach used in the core,
> while while by no means perfect in every situation, was the best
> compromise.  It is quite scalable as the number of attributes 
> / queries
> grows and it blocks multiple (potentially malicious) applications from
> DDoSing the modem when the information is not cached.

Well, my solution does exactly what you're asking, only with less code. You seem to think that it allows parallel property queries from multiple clients. That is not the case. See here:

static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
                                        void *data)
{
        struct ofono_radio_settings *rs = data;

        if (rs->pending)
                return __ofono_error_busy(msg);

        return radio_get_properties_reply(msg, rs);
}

A busy error is returned if a previous query is in progress.

I don't mind rewriting the stuff if need be but I'd like it to be for the right reason.

	MikaL

Re: [PATCH 1/4] radio settings: allow for more than one property

[Denis Kenzior]

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

Hi Mika,

On 10/22/2010 02:08 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi Denis, 
> 
>>> Oops, will fix. For some reason my checkpatch script did 
>> not complain about this.
>>>
>>> You must be using a script as well. To avoid unnecessary 
>> fuss like this, would it be possible to integrate it to the 
>> oFono tree so everyone would check their patches using the same thing?
>>
>> Not a script, but Johan's magic vim macro.  I paste it here 
>> for reference:
>> highlight RedundantWhitespace ctermbg=red guibg=red
>> match RedundantWhitespace /\s\+$\|\t\+\ze \+[^*]\| \+\ze\t/
>>
>> Between checkpatch, git am and this one we catch most issues.
> 
> Thanks. Still, I'd vote for a common checkpatch script in the oFono tree. ;)
>

Are you volunteering to maintain such a beast and make sure it is up to
date with the kernel upstream version?

>> In the beginning we tried many approaches, including ones 
>> similar to the
>> path you took.  We found that the current approach used in the core,
>> while while by no means perfect in every situation, was the best
>> compromise.  It is quite scalable as the number of attributes 
>> / queries
>> grows and it blocks multiple (potentially malicious) applications from
>> DDoSing the modem when the information is not cached.
> 
> Well, my solution does exactly what you're asking, only with less code. You seem to think that it allows parallel property queries from multiple clients. That is not the case. See here:

Yes, so let me give you a case where your approach breaks:

Set the RAT property, then run two GetProperties at the same time.  You
end up querying FastDormancy twice.

The current convention has also been proven to work in situations way
crazier than what radio settings has right now.  If it ain't broke,
please don't try to fix it.

However, all of this is besides the point.

> 
> static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
>                                         void *data)
> {
>         struct ofono_radio_settings *rs = data;
> 
>         if (rs->pending)
>                 return __ofono_error_busy(msg);
> 
>         return radio_get_properties_reply(msg, rs);
> }
> 
> A busy error is returned if a previous query is in progress.
> 
> I don't mind rewriting the stuff if need be but I'd like it to be for the right reason.
> 

So let me try to make it pretty clear.  I maintain all of the core and I
review just about every single patch.  For me code readability and
consistency of implementation is very important.  This is also the case
for anyone new starting with the codebase.  If I allow every atom to do
things just a little bit differently for the sake of saving 20 lines of
code or because the original author likes his approach better, I will go
crazy and the code will become unmaintainable.

I'd like to keep my sanity for a while longer ;)  So please modify your
patch to follow the oFono conventions.

Regards,
-Denis

RE: [PATCH 1/4] radio settings: allow for more than one property

[Mika.Liljeberg]

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

Hi Denis,

> > Thanks. Still, I'd vote for a common checkpatch script in 
> the oFono tree. ;)
> >
> 
> Are you volunteering to maintain such a beast and make sure 
> it is up to
> date with the kernel upstream version?

Only if I get to decide on the coding style. Otherwise, I'm proposing you guys should do it. Anyway, this is just a friendly suggestion in with the o sav e everyone's time.

> Yes, so let me give you a case where your approach breaks:
> 
> Set the RAT property, then run two GetProperties at the same 
> time.  You
> end up querying FastDormancy twice.

No you won't, because the second GetProperties query will be terminated right off the bat with ofono busy error. Again, see here:

> > static DBusMessage *radio_get_properties(DBusConnection 
> *conn, DBusMessage *msg,
> >                                         void *data)
> > {
> >         struct ofono_radio_settings *rs = data;
> > 
> >         if (rs->pending)
> >                 return __ofono_error_busy(msg);
> > 
> >         return radio_get_properties_reply(msg, rs);
> > }
> > 
> > A busy error is returned if a previous query is in progress.

Only one API call can be active at one time.

> So let me try to make it pretty clear.  I maintain all of the 
> core and I
> review just about every single patch.  For me code readability and
> consistency of implementation is very important.  This is 
> also the case
> for anyone new starting with the codebase.  If I allow every 
> atom to do
> things just a little bit differently for the sake of saving 
> 20 lines of
> code or because the original author likes his approach 
> better, I will go
> crazy and the code will become unmaintainable.

Understandable. Makes it difficult to improve on design patterns that have already been replicated, though.

> I'd like to keep my sanity for a while longer ;)  So please 
> modify your
> patch to follow the oFono conventions.

Sure. My personal cost optimization function tells me that at this point it's faster to rewrite than to argue further.

Br,

	MikaL

Re: [PATCH 1/4] radio settings: allow for more than one property

[Denis Kenzior]

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

Hi Mika,

On 10/22/2010 09:21 AM, Mika.Liljeberg(a)nokia.com wrote:
> Hi Denis,
> 
>>> Thanks. Still, I'd vote for a common checkpatch script in 
>> the oFono tree. ;)
>>>
>>
>> Are you volunteering to maintain such a beast and make sure 
>> it is up to
>> date with the kernel upstream version?
> 
> Only if I get to decide on the coding style. Otherwise, I'm proposing you guys should do it. Anyway, this is just a friendly suggestion in with the o sav e everyone's time.
> 

Nice try ;)

>> Yes, so let me give you a case where your approach breaks:
>>
>> Set the RAT property, then run two GetProperties at the same 
>> time.  You
>> end up querying FastDormancy twice.
> 
> No you won't, because the second GetProperties query will be terminated right off the bat with ofono busy error. Again, see here:
>

I really don't want to argue about this.

SetProperty("TechnologyPreference", ..)
-> Sets RADIO_SETTINGS_MODE_CACHED

GetProperties() -> queries Fast dormancy
GetProperties() -> queries Fast dormancy
->

+	} else if (rs->driver->query_fast_dormancy) {
+		rs->driver->query_fast_dormancy(rs,
+					fast_dormancy_query_callback, rs);
+		dbus_message_unref(reply);
+		return NULL;
+	}
+

You never set pending in the FastDormancy case.

> 
> Understandable. Makes it difficult to improve on design patterns that have already been replicated, though.
> 

Nobody is against improving things, but please try to do it in the
appropriate forum.  E.g. and RFC to the mailing list or an IRC discussion.

Regards,
-Denis

RE: [PATCH 1/4] radio settings: allow for more than one property

[Mika.Liljeberg]

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

Hi Denis,

> I really don't want to argue about this.
> 
> SetProperty("TechnologyPreference", ..)
> -> Sets RADIO_SETTINGS_MODE_CACHED
> 
> GetProperties() -> queries Fast dormancy
> GetProperties() -> queries Fast dormancy
> ->
> 
> +	} else if (rs->driver->query_fast_dormancy) {
> +		rs->driver->query_fast_dormancy(rs,
> +					
> fast_dormancy_query_callback, rs);
> +		dbus_message_unref(reply);
> +		return NULL;
> +	}
> +
> 
> You never set pending in the FastDormancy case.

Good catch. That's a bug, of course. Doesn't invalidate the approach, though.

> > Understandable. Makes it difficult to improve on design 
> patterns that have already been replicated, though.
> > 
> 
> Nobody is against improving things, but please try to do it in the
> appropriate forum.  E.g. and RFC to the mailing list or an 
> IRC discussion.

Of course. This time I wasn't conciously trying to improve things, though. Anyway, enough yakking. I'll do the rewrite sicne you're adamant.

Br,

	MikaL