Hi Guillaume,
> Change log from v4:
> - Check if modem name is sent by oFono
> - CDMA modem name is used as network name
> - Change modem_data property from is_cdma into has_cdma
>
> plugins/ofono.c | 156
> +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 151 insertions(+), 5 deletions(-)
>
> diff --git a/plugins/ofono.c b/plugins/ofono.c
> index 8650bfc..88a1781 100644
> --- a/plugins/ofono.c
> +++ b/plugins/ofono.c
> @@ -50,6 +50,7 @@
> #define OFONO_CONTEXT_INTERFACE OFONO_SERVICE
> ".ConnectionContext"
> #define OFONO_SIM_INTERFACE OFONO_SERVICE ".SimManager"
> #define OFONO_REGISTRATION_INTERFACE OFONO_SERVICE ".NetworkRegistration"
> +#define OFONO_CDMA_INTERFACE OFONO_SERVICE ".cdma.ConnectionManager"
>
> #define PROPERTY_CHANGED "PropertyChanged"
> #define GET_PROPERTIES "GetProperties"
> @@ -76,6 +77,7 @@ struct modem_data {
> gboolean has_sim;
> gboolean has_reg;
> gboolean has_gprs;
> + gboolean has_cdma;
> gboolean available;
> gboolean pending_online;
> dbus_bool_t requested_online;
> @@ -337,6 +339,7 @@ static struct connman_device_driver modem_driver = {
>
> static void remove_device_networks(struct connman_device *device)
> {
> + struct modem_data *modem = connman_device_get_data(device);
> GHashTableIter iter;
> gpointer key, value;
> GSList *info_list = NULL;
> @@ -362,6 +365,11 @@ static void remove_device_networks(struct connman_device
> *device)
> connman_device_remove_network(device, info->network);
> }
>
> + if (modem->has_cdma == TRUE) {
> + g_hash_table_remove(network_hash, "/context1");
> + modem->has_cdma = FALSE;
> + }
> +
> g_slist_free(info_list);
> }
I do not like this "/context1" stuff at all. What is wrong with just
using the modem object path that you get from oFono here. I do not
remember that devices and networks have to have distinct object paths. I
think that limitation is gone since a long time.
> @@ -541,25 +549,57 @@ done:
>
> static int set_network_active(struct connman_network *network)
> {
> + struct connman_device *device;
> + struct modem_data *modem;
> dbus_bool_t value = TRUE;
> const char *path = connman_network_get_string(network, "Path");
>
> DBG("network %p, path %s", network, path);
>
> - return set_property(path, OFONO_CONTEXT_INTERFACE,
> - "Active", DBUS_TYPE_BOOLEAN, &value,
> - set_active_reply, g_strdup(path), g_free);
> + device = connman_network_get_device(network);
> + if (device == NULL)
> + return -ENODEV;
> +
> + modem = connman_device_get_data(device);
> + if (modem == NULL)
> + return -ENODEV;
> +
> + if (modem->has_cdma == TRUE)
> + return set_property(modem->path, OFONO_CDMA_INTERFACE,
> + "Powered", DBUS_TYPE_BOOLEAN, &value,
> + set_active_reply, g_strdup(modem->path),
> + g_free);
> + else
> + return set_property(path, OFONO_CONTEXT_INTERFACE,
> + "Active", DBUS_TYPE_BOOLEAN, &value,
> + set_active_reply,
> + g_strdup(path), g_free);
> }
the Path should be set properly here. So I prefer that a CDMA network
object looks similar to a GSM network object.
Dragging the device object into this here seems wrong.
> static int set_network_inactive(struct connman_network *network)
> {
> + struct connman_device *device;
> + struct modem_data *modem;
> int err;
> dbus_bool_t value = FALSE;
> const char *path = connman_network_get_string(network, "Path");
>
> DBG("network %p, path %s", network, path);
>
> - err = set_property(path, OFONO_CONTEXT_INTERFACE,
> + device = connman_network_get_device(network);
> + if (device == NULL)
> + return -ENODEV;
> +
> + modem = connman_device_get_data(device);
> + if (modem == NULL)
> + return -ENODEV;
> +
> + if (modem->has_cdma == TRUE)
> + err = set_property(modem->path, OFONO_CDMA_INTERFACE,
> + "Powered", DBUS_TYPE_BOOLEAN, &value,
> + NULL, NULL, NULL);
> + else
> + err = set_property(path, OFONO_CONTEXT_INTERFACE,
> "Active", DBUS_TYPE_BOOLEAN, &value,
> NULL, NULL, NULL);
Same comments as above.
> @@ -584,6 +624,9 @@ static int network_connect(struct connman_network
> *network)
> if (modem == NULL)
> return -ENODEV;
>
> + if (modem->has_cdma == TRUE)
> + return set_network_active(network);
> +
> if (modem->registered == FALSE)
> return -ENOLINK;
>
> @@ -1464,6 +1507,12 @@ static gboolean modem_has_gprs(DBusMessageIter *array)
> return modem_has_interface(array, OFONO_GPRS_INTERFACE);
> }
>
> +static gboolean modem_has_cdma(DBusMessageIter *array)
> +{
> + return modem_has_interface(array,
> + OFONO_CDMA_INTERFACE);
> +}
> +
> static void add_modem(const char *path, DBusMessageIter *prop)
> {
> struct modem_data *modem;
> @@ -1473,6 +1522,7 @@ static void add_modem(const char *path, DBusMessageIter
> *prop)
> gboolean has_sim = FALSE;
> gboolean has_reg = FALSE;
> gboolean has_gprs = FALSE;
> + gboolean has_cdma = FALSE;
>
> modem = g_hash_table_lookup(modem_hash, path);
>
> @@ -1486,6 +1536,7 @@ static void add_modem(const char *path, DBusMessageIter
> *prop)
> modem->path = g_strdup(path);
> modem->device = NULL;
> modem->available = TRUE;
> + modem->operator = NULL;
>
> g_hash_table_insert(modem_hash, g_strdup(path), modem);
>
> @@ -1507,6 +1558,7 @@ static void add_modem(const char *path, DBusMessageIter
> *prop)
> has_sim = modem_has_sim(&value);
> has_reg = modem_has_reg(&value);
> has_gprs = modem_has_gprs(&value);
> + has_cdma = modem_has_cdma(&value);
> }
>
> dbus_message_iter_next(prop);
> @@ -1521,6 +1573,7 @@ static void add_modem(const char *path, DBusMessageIter
> *prop)
> modem->has_sim = has_sim;
> modem->has_reg = has_reg;
> modem->has_gprs = has_gprs;
> + modem->has_cdma = has_cdma;
>
> update_modem_online(modem, online);
>
> @@ -1658,10 +1711,27 @@ static gboolean modem_changed(DBusConnection
> *connection, DBusMessage *message,
> gboolean had_reg = modem->has_reg;
> gboolean has_gprs = modem_has_gprs(&value);
> gboolean had_gprs = modem->has_gprs;
> + gboolean has_cdma = modem_has_cdma(&value);
> + gboolean had_cdma = modem->has_cdma;
>
> modem->has_sim = has_sim;
> modem->has_reg = has_reg;
> modem->has_gprs = has_gprs;
> + modem->has_cdma = has_cdma;
> +
> + if (has_cdma) {
> + if (modem->device == NULL)
> + return TRUE;
> +
> + add_network(modem->device, "/context1", NULL);
> +
> + return TRUE;
> + } else {
> + if (has_cdma == FALSE && had_cdma == TRUE) {
> + modem->has_cdma = had_cdma;
> + modem_remove_device(modem);
> + }
> + }
This is horrible :(
First of all, the second has_cdma check is implied anyway. And second,
why so complicated. This can be done surely a lot simpler.
> if (modem->device == NULL) {
> if (has_sim)
> @@ -1679,6 +1749,25 @@ static gboolean modem_changed(DBusConnection
> *connection, DBusMessage *message,
> check_gprs(modem);
> }
> }
> + } else if (g_str_equal(key, "Serial") == TRUE) {
> + const char *hw_serial;
> +
> + dbus_message_iter_get_basic(&value, &hw_serial);
> +
> + add_device(modem->path, hw_serial);
This breaks GSM. Have you actually tested that this does not break
anything. Now in the GSM case, you just add device with the IMEI here.
> +
> + if (modem->has_cdma == TRUE)
> + add_network(modem->device, "/context1", NULL);
> + } else if (g_str_equal(key, "Model") == TRUE) {
> + const char *model;
> +
> + dbus_message_iter_get_basic(&value, &model);
> +
> + if (modem->has_cdma == TRUE)
> + modem->operator = g_strdup(model);
> +
> + if (modem->device != NULL && modem->has_cdma == TRUE)
> + add_network(modem->device, "/context1", NULL);
> }
So what happens if Model comes before Serial property?
> return TRUE;
> @@ -1888,6 +1977,55 @@ static gboolean context_changed(DBusConnection
> *connection,
> return TRUE;
> }
>
> +static gboolean cdma_changed(DBusConnection *connection,
> + DBusMessage *message, void *user_data)
> +{
> + struct network_info *info;
> + DBusMessageIter iter, value;
> + const char *key;
> +
> + DBG("");
> +
> + info = g_hash_table_lookup(network_hash, "/context1");
> + if (info == NULL)
> + return TRUE;
> +
> + if (!pending_network_is_available(info->network)) {
> + g_hash_table_remove(network_hash, "/context1");
> + return TRUE;
> + }
> +
> + connman_network_set_associating(info->network, FALSE);
> +
> + if (dbus_message_iter_init(message, &iter) == FALSE)
> + return TRUE;
> +
> + dbus_message_iter_get_basic(&iter, &key);
> +
> + dbus_message_iter_next(&iter);
> + dbus_message_iter_recurse(&iter, &value);
> +
> + DBG("key %s", key);
> +
> + if (g_str_equal(key, "Settings") == TRUE)
> + update_ipv4_settings(&value, info);
> + else if (g_str_equal(key, "Powered") == TRUE) {
> + dbus_bool_t active;
> +
> + dbus_message_iter_get_basic(&value, &active);
> +
> + if (active == FALSE)
> + set_connected(info, active);
> +
> + /* Connect only if requested to do so */
> + if (active == TRUE &&
> + connman_network_get_connecting(info->network) == TRUE)
> + set_connected(info, active);
Actually "... else if ..." statement exists for a reason ;)
> + }
> +
> + return TRUE;
> +}
> +
> static guint watch;
> static guint reg_watch;
> static guint sim_watch;
> @@ -1898,6 +2036,7 @@ static guint modem_watch;
> static guint modem_added_watch;
> static guint modem_removed_watch;
> static guint context_watch;
> +static guint cdma_watch;
>
> static int ofono_init(void)
> {
> @@ -1964,11 +2103,17 @@ static int ofono_init(void)
> context_changed,
> NULL, NULL);
>
> + cdma_watch = g_dbus_add_signal_watch(connection, NULL, NULL,
> + OFONO_CDMA_INTERFACE,
> + PROPERTY_CHANGED,
> + cdma_changed,
> + NULL, NULL);
> +
> if (watch == 0 || gprs_watch == 0 || context_added_watch == 0 ||
> context_removed_watch == 0 || modem_watch == 0 ||
> reg_watch == 0 || sim_watch == 0 ||
> modem_added_watch == 0 || modem_removed_watch == 0 ||
> - context_watch == 0) {
> + context_watch == 0 || cdma_watch == 0) {
> err = -EIO;
> goto remove;
> }
> @@ -1996,6 +2141,7 @@ remove:
> g_dbus_remove_watch(connection, modem_added_watch);
> g_dbus_remove_watch(connection, modem_removed_watch);
> g_dbus_remove_watch(connection, context_watch);
> + g_dbus_remove_watch(connection, cdma_watch);
>
> dbus_connection_unref(connection);
>
Regards
Marcel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
