Hi Guillaume,
On Thu, Jul 28, 2011 at 02:07:20PM +0200, Guillaume Zajac wrote:
> --- 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_CONNECTION_MANAGER_INTERFACE OFONO_SERVICE
> ".cdma.ConnectionManager"
>
Could we just call this one OFONO_CDMA_INTERFACE ?
> +static int set_cdma_network_inactive(struct connman_network *network,
> + const char *path)
> +{
> + int err;
> + dbus_bool_t value = FALSE;
> +
> + DBG("network %p, path %s", network, path);
> +
> + err = set_property(path, OFONO_CDMA_CONNECTION_MANAGER_INTERFACE,
> + "Powered", DBUS_TYPE_BOOLEAN, &value,
> + NULL, NULL, NULL);
> +
> + if (err == -EINPROGRESS)
> + err = 0;
> +
> + return err;
> +}
So I'd rather use the set_network_active|inactive() routine and have them
support cdma. You would get the modem from network -> device -> modem.
Then you'd set the interface and the property paths depending on the modem
being a cdma one or not.
> @@ -584,6 +616,9 @@ static int network_connect(struct connman_network
> *network)
> if (modem == NULL)
> return -ENODEV;
>
> + if (modem->is_cdma)
> + return set_cdma_network_active(network, modem->path);
> +
> if (modem->registered == FALSE)
> return -ENOLINK;
>
> @@ -598,13 +633,27 @@ static int network_connect(struct connman_network
> *network)
>
> static int network_disconnect(struct connman_network *network)
> {
> + struct connman_device *device;
> + struct modem_data *modem;
> +
> DBG("network %p", network);
>
> + device = connman_network_get_device(network);
> + if (device == NULL)
> + return -ENODEV;
> +
> + modem = connman_device_get_data(device);
> + if (modem == NULL)
> + return -ENODEV;
> +
> if (connman_network_get_index(network) < 0)
> return -ENOTCONN;
>
> connman_network_set_associating(network, FALSE);
>
> + if (modem->is_cdma)
> + return set_cdma_network_inactive(network, modem->path);
> +
> return set_network_inactive(network);
> }
With my above proposition, those 2 chunks are removed.
> +static int add_cdma_network(struct connman_device *device, const char *path)
> +{
> + struct modem_data *modem = connman_device_get_data(device);
> + struct connman_network *network;
> + struct network_info *info;
> + char *ident;
> + const char *hash_path;
> +
> + DBG("modem %p device %p path %s", modem, device, path);
> +
> + ident = get_ident(path);
> +
> + network = connman_device_get_network(device, ident);
> + if (network != NULL)
> + return -EALREADY;
> +
> + info = g_hash_table_lookup(network_hash, path);
> + if (info != NULL) {
> + DBG("path %p already exists with device %p", path,
> + connman_network_get_device(info->network));
> + if (connman_network_get_device(info->network))
> + return -EALREADY;
> + g_hash_table_remove(network_hash, path);
> + }
> +
> + network = connman_network_create(ident, CONNMAN_NETWORK_TYPE_CELLULAR);
> + if (network == NULL)
> + return -ENOMEM;
> +
> + info = g_try_new0(struct network_info, 1);
> + if (info == NULL) {
> + connman_network_unref(network);
> + return -ENOMEM;
> + }
> +
> + connman_ipaddress_clear(&info->ipv4_address);
> + connman_ipaddress_clear(&info->ipv6_address);
> + info->network = network;
> +
> + connman_network_set_string(network, "Path", path);
> + hash_path = connman_network_get_string(network, "Path");
> + if (hash_path == NULL) {
> + connman_network_unref(network);
> + g_free(info);
> + return -EIO;
> + }
> +
> + create_service(network);
> +
> + connman_network_ref(network);
> + g_hash_table_insert(network_hash, (char *) hash_path, info);
> +
> + connman_network_set_available(network, TRUE);
> + connman_network_set_index(network, -1);
> +
> + connman_network_set_name(network, "CDMA Network");
> +
> + if (modem->has_strength)
> + connman_network_set_strength(network, modem->strength);
> +
> + if (connman_device_add_network(device, network) != 0)
> + goto error;
> +
> + return 0;
> +
> +error:
> + connman_network_unref(network);
> + g_hash_table_remove(network_hash, hash_path);
> + return -EIO;
> +}
So my first question would be: Why do we need a specific network addition
routine for CDMA ? The only addition to the existing add_network() routine
would be for the network name setting since oFono doesn't have a CDMA netreg
support. By the way, setting all CDMA network names to "CDMA network" is not
good, you need to find a unique name.
My second question is: Why do you need that ?:
+ connman_network_set_string(network, "Path", path);
+ hash_path = connman_network_get_string(network, "Path");
> @@ -1656,10 +1786,21 @@ 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 is_cdma = modem_is_cdma(&value);
>
> modem->has_sim = has_sim;
> modem->has_reg = has_reg;
> modem->has_gprs = has_gprs;
> + modem->is_cdma = is_cdma;
> +
> + if (is_cdma) {
> + if (modem->device == NULL)
> + return TRUE;
> +
> + add_cdma_network(modem->device, "/context1");
With CDMA supportting only one context, I suppose the hardcoded path here is
ok.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
