12:37 a.m.
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
12:37 a.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
9:57 a.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
7:55 p.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
1:03 a.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
6:08 p.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
12:49 a.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
1:21 a.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
1:55 a.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
5:59 p.m.
New subject: [PATCH 1/4] radio settings: allow for more than one property
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
12:37 a.m.
New subject: [PATCH 2/4] radio settings: add FastDormancy property
---
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
10:07 a.m.
New subject: [PATCH 2/4] radio settings: add FastDormancy property
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
12:37 a.m.
New subject: [PATCH 3/4] radio settings: document FastDormancy property
---
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
2:34 a.m.
New subject: [PATCH 3/4] radio settings: document FastDormancy property
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
5:56 p.m.
New subject: [PATCH 3/4] radio settings: document FastDormancy property
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
7:55 p.m.
New subject: [PATCH 3/4] radio settings: document FastDormancy property
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
10:21 a.m.
New subject: [PATCH 3/4] radio settings: document FastDormancy property
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
12:37 a.m.
New subject: [PATCH 4/4] test: add scripts to enable and disable fast dormancy
---
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
10:09 a.m.
New subject: [PATCH 4/4] test: add scripts to enable and disable fast dormancy
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
4346
days inactive
4364
days old
18 comments
4 participants
participants (4)
-
Denis Kenzior -
Marcel Holtmann -
Mika Liljeberg -
Mika.Liljeberg＠nokia.com