Re: [PATCH 1/6] radio settings: add FastDormancy property
Hi Mika,
On 10/25/2010 08:28 AM, Mika Liljeberg wrote:
> ---
> include/radio-settings.h | 11
> src/radio-settings.c | 134 +
> 2 files changed, 133 insertions(+), 12 deletions(-)
>
> diff --git a/include/radio-settings.h b/include/radio-settings.h
> index d41ec0b..a6b19d0 100644
> --- a/include/radio-settings.h
> +++ b/include/radio-settings.h
> @@ -42,6 +42,10 @@ 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,
> + int enable, void *data);
>
> struct ofono_radio_settings_driver {
> const char *name;
> @@ -55,6 +59,13 @@ struct ofono_radio_settings_driver {
> enum ofono_radio_access_mode mode,
> ofono_radio_settings_rat_mode_set_cb_t cb,
> void *data);
> + void (*query_fast_dormancy)(struct ofono_radio_settings *rs,
> + ofono_radio_settings_fast_dormancy_query_cb_t cb,
> + void *data);
> + void (*set_fast_dormancy)(struct ofono_radio_settings *rs,
> + int enable,
> + ofono_radio_settings_fast_dormancy_set_cb_t,
> + void *data);
> };
>
> int ofono_radio_settings_driver_register(const struct
> ofono_radio_settings_driver *d);
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> index 3306be6..5441481 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -33,7 +33,7 @@
> #include "ofono.h"
> #include "common.h"
>
> -#define RADIO_SETTINGS_MODE_CACHED 0x1
> +#define RADIO_SETTINGS_FLAG_CACHED 0x1
>
> static GSList *g_drivers = NULL;
>
> @@ -42,6 +42,8 @@ struct ofono_radio_settings {
> int flags;
> enum ofono_radio_access_mode mode;
> enum ofono_radio_access_mode pending_mode;
> + int fast_dormancy;
> + int fast_dormancy_pending;
I suggest simply using ofono_bool_t here.
> const struct ofono_radio_settings_driver *driver;
> void *driver_data;
> struct ofono_atom *atom;
> @@ -91,8 +93,6 @@ static DBusMessage *radio_get_properties_reply(DBusMessage
> *msg,
> 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;
> @@ -103,14 +103,60 @@ static DBusMessage
> *radio_get_properties_reply(DBusMessage *msg,
> OFONO_PROPERTIES_ARRAY_SIGNATURE,
> &dict);
>
> - ofono_dbus_dict_append(&dict, "TechnologyPreference",
> + if ((int)rs->mode != -1) {
> + const char *mode = radio_access_mode_to_string(rs->mode);
> + ofono_dbus_dict_append(&dict, "TechnologyPreference",
> DBUS_TYPE_STRING, &mode);
> + }
> +
I suggest always showing this property, otherwise RadioSettings
interface is pretty pointless.
> + if (rs->fast_dormancy != -1) {
> + dbus_bool_t value = (rs->fast_dormancy != 0);
> + ofono_dbus_dict_append(&dict, "FastDormancy",
> + DBUS_TYPE_BOOLEAN, &value);
> + }
This should be guarded by the query_fast_dormancy implementation
availability.
>
> dbus_message_iter_close_container(&iter, &dict);
>
> return reply;
> }
>
> +static void radio_set_fast_dormancy(struct ofono_radio_settings *rs, int
> enable)
> +{
I really suggest something like:
if (rs->fast_dormancy == enable)
return;
...
> + if (rs->fast_dormancy != enable) {
> +
In general, please don't add empty lines like this
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path = __ofono_atom_get_path(rs->atom);
> + dbus_bool_t value = (enable != 0);
> +
> + ofono_dbus_signal_property_changed(conn, path,
> + OFONO_RADIO_SETTINGS_INTERFACE,
> + "FastDormancy",
> + DBUS_TYPE_BOOLEAN, &value);
> + }
> +
> + rs->fast_dormancy = enable;
> +}
> +
> +static void radio_fast_dormancy_set_callback(const struct ofono_error *error,
> + void *data
Re: [PATCH 1/6] radio settings: add FastDormancy property
Hi Mika,
On 10/25/2010 10:05 AM, mika.liljeb...@nokia.com wrote:
> Hi Marcel,
>
>>> @@ -103,14 +103,60 @@ static DBusMessage
>> *radio_get_properties_reply(DBusMessage *msg,
>>>
>> OFONO_PROPERTIES_ARRAY_SIGNATURE,
>>> &dict);
>>>
>>> - ofono_dbus_dict_append(&dict, "TechnologyPreference",
>>> + if ((int)rs->mode != -1) {
>>> + const char *mode =
>> radio_access_mode_to_string(rs->mode);
>>> + ofono_dbus_dict_append(&dict, "TechnologyPreference",
>>> DBUS_TYPE_STRING, &mode);
>>
>> what is up with this (int) rs->mode cast here. That looks highly wrong
>> to me. The mode is an enum so please don't hack around it like this.
>>
>> If mode can be invalid or not present then we need to extend this enum
>> with an initial value of OFONO_RADIO_ACCESS_MODE_UNKNOWN, but not hack
>> some cast magic into it.
>
> Yes, it's fishy. Denis introduced the enum in commit
> 81bc8884b414e6c2d511789d2e183cdad55182f0 but left mode initialized as -1. I'm
> not sure what's up with that but I did not want to start fixing it. I suppose
> the initializer could be added to the enum, as you say, or the whole patch
> could be reverted. Not my call, though.
I must have missed the -1 initialization. In general the preference is
as follows:
- If the property is queried at the network, then set to a value that
means "unknown". Otherwise set to a default sane value.
- Only set to the new value if the query succeeds
- If the query fails (a bizarre case if querying the modem), don't reset
the sane value and don't set cached. The next GetProperties will try to
re-query the setting.
- Don't show the attribute if the query_ method is not provided by the
driver.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/6] radio settings: add FastDormancy property
Hi Marcel,
> > @@ -103,14 +103,60 @@ static DBusMessage
> *radio_get_properties_reply(DBusMessage *msg,
> >
> OFONO_PROPERTIES_ARRAY_SIGNATURE,
> > &dict);
> >
> > - ofono_dbus_dict_append(&dict, "TechnologyPreference",
> > + if ((int)rs->mode != -1) {
> > + const char *mode =
> radio_access_mode_to_string(rs->mode);
> > + ofono_dbus_dict_append(&dict, "TechnologyPreference",
> > DBUS_TYPE_STRING, &mode);
>
> what is up with this (int) rs->mode cast here. That looks highly wrong
> to me. The mode is an enum so please don't hack around it like this.
>
> If mode can be invalid or not present then we need to extend this enum
> with an initial value of OFONO_RADIO_ACCESS_MODE_UNKNOWN, but not hack
> some cast magic into it.
Yes, it's fishy. Denis introduced the enum in commit
81bc8884b414e6c2d511789d2e183cdad55182f0 but left mode initialized as -1. I'm
not sure what's up with that but I did not want to start fixing it. I suppose
the initializer could be added to the enum, as you say, or the whole patch
could be reverted. Not my call, though.
> Or you use some flags like for the cached value.
>
> And I would propose the same for fast dormancy value. Lets
> store it as a
> boolean and have a flag if it is present or not.
That's what I did in the previous patch but Denis wanted a single CACHED flag.
Guys, please try to agree on this.
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 1/6] radio settings: add FastDormancy property
Hi Mika,
> include/radio-settings.h | 11
> src/radio-settings.c | 134 +
> 2 files changed, 133 insertions(+), 12 deletions(-)
>
> diff --git a/include/radio-settings.h b/include/radio-settings.h
> index d41ec0b..a6b19d0 100644
> --- a/include/radio-settings.h
> +++ b/include/radio-settings.h
> @@ -42,6 +42,10 @@ 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,
> + int enable, void *data);
>
> struct ofono_radio_settings_driver {
> const char *name;
> @@ -55,6 +59,13 @@ struct ofono_radio_settings_driver {
> enum ofono_radio_access_mode mode,
> ofono_radio_settings_rat_mode_set_cb_t cb,
> void *data);
> + void (*query_fast_dormancy)(struct ofono_radio_settings *rs,
> + ofono_radio_settings_fast_dormancy_query_cb_t cb,
> + void *data);
> + void (*set_fast_dormancy)(struct ofono_radio_settings *rs,
> + int enable,
> + ofono_radio_settings_fast_dormancy_set_cb_t,
> + void *data);
> };
>
> int ofono_radio_settings_driver_register(const struct
> ofono_radio_settings_driver *d);
> diff --git a/src/radio-settings.c b/src/radio-settings.c
> index 3306be6..5441481 100644
> --- a/src/radio-settings.c
> +++ b/src/radio-settings.c
> @@ -33,7 +33,7 @@
> #include "ofono.h"
> #include "common.h"
>
> -#define RADIO_SETTINGS_MODE_CACHED 0x1
> +#define RADIO_SETTINGS_FLAG_CACHED 0x1
>
> static GSList *g_drivers = NULL;
>
> @@ -42,6 +42,8 @@ struct ofono_radio_settings {
> int flags;
> enum ofono_radio_access_mode mode;
> enum ofono_radio_access_mode pending_mode;
> + int fast_dormancy;
> + int fast_dormancy_pending;
> const struct ofono_radio_settings_driver *driver;
> void *driver_data;
> struct ofono_atom *atom;
> @@ -91,8 +93,6 @@ static DBusMessage *radio_get_properties_reply(DBusMessage
> *msg,
> 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;
> @@ -103,14 +103,60 @@ static DBusMessage
> *radio_get_properties_reply(DBusMessage *msg,
> OFONO_PROPERTIES_ARRAY_SIGNATURE,
> &dict);
>
> - ofono_dbus_dict_append(&dict, "TechnologyPreference",
> + if ((int)rs->mode != -1) {
> + const char *mode = radio_access_mode_to_string(rs->mode);
> + ofono_dbus_dict_append(&dict, "TechnologyPreference",
> DBUS_TYPE_STRING, &mode);
what is up with this (int) rs->mode cast here. That looks highly wrong
to me. The mode is an enum so please don't hack around it like this.
If mode can be invalid or not present then we need to extend this enum
with an initial value of OFONO_RADIO_ACCESS_MODE_UNKNOWN, but not hack
some cast magic into it.
Or you use some flags like for the cached value.
And I would propose the same for fast dormancy value. Lets store it as a
boolean and have a flag if it is present or not.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
