Hi Pekka,
On Mon, Aug 16, 2010 at 05:41:01PM +0300, [email protected] wrote:
> From: Pekka Pessi <[email protected]>
>
> The Modem Online property is tied with the cellular device Powered property.
Here again, I would appreciate more detailed explanations as to why we want to
use the Online property instead of the current one.
More comments below:
> ---
> plugins/ofono.c | 82
> ++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 69 insertions(+), 13 deletions(-)
>
> diff --git a/plugins/ofono.c b/plugins/ofono.c
> index 1a4b8e6..43d2410 100644
> --- a/plugins/ofono.c
> +++ b/plugins/ofono.c
> @@ -65,6 +65,8 @@ struct modem_data {
> char *path;
> struct connman_device *device;
> gboolean available;
> + dbus_bool_t online;
> + gboolean online_is_valid;
> };
>
> static int modem_probe(struct connman_device *device)
> @@ -159,13 +161,61 @@ static int set_property(const char *path,
> return -EINPROGRESS;
> }
>
> -static int gprs_change_powered(const char *path, dbus_bool_t powered)
> +static void update_online(struct modem_data *modem, connman_bool_t online)
> {
> - DBG("path %s powered %d", path, powered);
> + DBG("modem %p, path %s, online %d", modem, modem->path, online);
>
> - return set_property(path, OFONO_GPRS_INTERFACE, "Powered",
> - DBUS_TYPE_BOOLEAN, &powered,
> - NULL, NULL, NULL);
> + modem->online = online;
> + modem->online_is_valid = TRUE;
> +
> + if (modem->device)
> + connman_device_set_powered(modem->device, online);
> +}
> +
> +static void set_online_reply(DBusPendingCall *call, void *user_data)
> +{
> + DBusMessage *reply;
> + DBusError error;
> + struct modem_data *modem;
> +
> + DBG("");
> +
> + dbus_error_init(&error);
> +
> + reply = dbus_pending_call_steal_reply(call);
> +
> + if (dbus_set_error_from_message(&error, reply)) {
> + connman_error("%s(\"%s\"): %s: %s",
> + "SetProperty", "Online",
> + error.name, error.message);
I'm picky again, but this error string is not consistent with the rest of the
ConnMan code.
> + dbus_error_free(&error);
> + modem = g_hash_table_lookup(modem_hash, user_data);
> + if (modem && modem->device)
> + connman_device_set_powered(modem->device,
> + modem->online);
So Marcel and Denis told me that currently none of the oFono drivers support
the Online setting currently. So I'd like to see us falling back on the GPRS
interface if here we get a NotImplemented answer from oFono.
> + }
> +
> + dbus_message_unref(reply);
> +
> + dbus_pending_call_unref(call);
> +}
> +
> +static int modem_change_online(const char *path, dbus_bool_t online)
> +{
> + struct modem_data *modem = g_hash_table_lookup(modem_hash, path);
> +
> + DBG("path %s, modem %p", path, modem);
> +
> + if (!modem)
> + return -ENODEV;
> +
> + if (modem->online == online && modem->online_is_valid)
> + return 0;
> +
> + return set_property(path, OFONO_MODEM_INTERFACE, "Online",
> + DBUS_TYPE_BOOLEAN, &online,
> + set_online_reply,
> + g_strdup(path), g_free);
> }
>
> static int modem_enable(struct connman_device *device)
> @@ -174,7 +224,7 @@ static int modem_enable(struct connman_device *device)
>
> DBG("device %p, path, %s", device, path);
>
> - return gprs_change_powered(path, TRUE);
> + return modem_change_online(path, TRUE);
> }
>
> static int modem_disable(struct connman_device *device)
> @@ -183,7 +233,7 @@ static int modem_disable(struct connman_device *device)
>
> DBG("device %p, path %s", device, path);
>
> - return gprs_change_powered(path, FALSE);
> + return modem_change_online(path, FALSE);
> }
>
> static struct connman_device_driver modem_driver = {
> @@ -839,12 +889,6 @@ static void check_networks_reply(DBusPendingCall *call,
> void *user_data)
> contexts = value;
> add_default_context(&contexts, path,
> CONTEXT_NAME, CONTEXT_TYPE);
> - } else if (g_str_equal(key, "Powered") == TRUE) {
> - dbus_bool_t powered;
> -
> - dbus_message_iter_get_basic(&value, &powered);
> -
> - connman_device_set_powered(device, powered);
So you're sure we don't need this one anymore, right ?
> }
>
> dbus_message_iter_next(&dict);
> @@ -1112,6 +1156,8 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
> DBusMessageIter array, dict;
> const char *path = user_data;
> dbus_bool_t powered = FALSE;
> + /* Online default is TRUE for backward compatibility reasons */
> + dbus_bool_t online = TRUE;
Not sure we need this one as the oFono drivers not implementing the Online
property will set it to TRUE by default (and reply NotImplemented when trying
to set the property).
> gboolean has_sim = FALSE;
> struct modem_data *new_modem;
>
> @@ -1149,6 +1195,8 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
>
> if (g_str_equal(key, "Powered") == TRUE)
> dbus_message_iter_get_basic(&value, &powered);
> + else if (g_str_equal(key, "Online") == TRUE)
> + dbus_message_iter_get_basic(&value, &online);
> else if (g_str_equal(key, "Interfaces") == TRUE)
> has_sim = modem_has_sim(&value);
>
> @@ -1162,6 +1210,8 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
> if (!powered)
> modem_change_powered(path, TRUE);
>
> + update_online(new_modem, online);
> +
Ok, so this one will be called only once when creating the modem, right ?
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
