Hi Pekka,
On Mon, Aug 16, 2010 at 05:41:00PM +0300, [email protected] wrote:
> From: Pekka Pessi <[email protected]>
>
> Try to get IMSI immediately after SimManager interface is available.
>
> Listen to SIM events in Ofono plugin, add device when IMSI
I suppose you want to do that in order for ConnMan to be able to control GSM
devices through the Online property ? That is to say have it controlling both
GPRS enabled and GPRS less modems ? If that's so I'd really appreciate to get
this sort of details from either the log message above or from a 0/6 patch
explaining in details where you're trying to go.
Some additional comments:
> (SubscriberIdentity) becomes available, remove device if SIM is removed.
> ---
> plugins/ofono.c | 137 ++++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 101 insertions(+), 36 deletions(-)
>
> diff --git a/plugins/ofono.c b/plugins/ofono.c
> index faf82b9..1a4b8e6 100644
> --- a/plugins/ofono.c
> +++ b/plugins/ofono.c
> @@ -195,10 +195,35 @@ static struct connman_device_driver modem_driver = {
> .disable = modem_disable,
> };
>
> +static void modem_remove_device(struct modem_data *modem)
> +{
> + if (modem->device == NULL)
> + return;
> +
> + connman_device_unregister(modem->device);
> + connman_device_unref(modem->device);
> +
> + modem->device = NULL;
> +}
> +
> +static void remove_modem(gpointer data)
> +{
> + struct modem_data *modem = data;
> +
> + g_free(modem->path);
> +
> + modem_remove_device(modem);
> +
> + g_free(modem);
> +}
> +
> static char *get_ident(const char *path)
> {
> char *pos;
>
> + if (path == NULL)
> + return NULL;
> +
In which case is that needed ?
> if (*path != '/')
> return NULL;
>
> @@ -880,7 +905,7 @@ static void add_device(const char *path, const char *imsi,
> struct modem_data *modem;
> struct connman_device *device;
>
> - DBG("path %s imsi %s", path, imsi);
> + DBG("path %s imsi %s", path, imsi ? imsi : "<missing>");
That's not needed, you'll get a "nil" string here.
>
> if (path == NULL)
> return;
> @@ -892,6 +917,16 @@ static void add_device(const char *path, const char
> *imsi,
> if (modem == NULL)
> return;
>
> + if (modem->device) {
> + char const *ident;
> +
> + ident = connman_device_get_string(modem->device, "IMSI");
> + if (g_str_equal(ident, imsi))
> + return;
> +
> + modem_remove_device(modem);
> + }
> +
> device = connman_device_create(imsi, CONNMAN_DEVICE_TYPE_CELLULAR);
> if (device == NULL)
> return;
> @@ -901,6 +936,7 @@ static void add_device(const char *path, const char *imsi,
> connman_device_set_mode(device, CONNMAN_DEVICE_MODE_NETWORK_MULTIPLE);
>
> connman_device_set_string(device, "Path", path);
> + connman_device_set_string(device, "IMSI", imsi); /* XXX */
What's the XXX for ?
> if (mcc != NULL)
> connman_device_set_string(device, "MCC", mcc);
> if (mnc != NULL)
> @@ -919,7 +955,7 @@ static void add_device(const char *path, const char *imsi,
> static void sim_properties_reply(DBusPendingCall *call, void *user_data)
> {
> const char *path = user_data;
> - const char *imsi;
> + const char *imsi = NULL;
> char *mcc = NULL;
> char *mnc = NULL;
> /* If MobileNetworkCodeLength is not provided, mnc_length is 0 */
> @@ -1049,7 +1085,7 @@ static struct modem_data *add_modem(const char *path)
> return modem;
> }
>
> -static gboolean modem_has_gprs(DBusMessageIter *array)
> +static gboolean modem_has_sim(DBusMessageIter *array)
> {
> DBusMessageIter entry;
>
> @@ -1060,7 +1096,7 @@ static gboolean modem_has_gprs(DBusMessageIter *array)
>
> dbus_message_iter_get_basic(&entry, &interface);
>
> - if (g_strcmp0(OFONO_GPRS_INTERFACE, interface) == 0)
> + if (g_strcmp0(OFONO_SIM_INTERFACE, interface) == 0)
> return TRUE;
>
> dbus_message_iter_next(&entry);
> @@ -1076,7 +1112,7 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
> DBusMessageIter array, dict;
> const char *path = user_data;
> dbus_bool_t powered = FALSE;
> - gboolean has_gprs = FALSE;
> + gboolean has_sim = FALSE;
> struct modem_data *new_modem;
>
> DBG("path %s", path);
> @@ -1114,7 +1150,7 @@ 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, "Interfaces") == TRUE)
> - has_gprs = modem_has_gprs(&value);
> + has_sim = modem_has_sim(&value);
>
> dbus_message_iter_next(&dict);
> }
> @@ -1126,7 +1162,7 @@ static void modem_properties_reply(DBusPendingCall
> *call, void *user_data)
> if (!powered)
> modem_change_powered(path, TRUE);
>
> - if (has_gprs)
> + if (has_sim)
> get_imsi(path);
So here get_imsi will end up creating a device which then will query the GPRS
interface and try to build networks out of that. Would it make sense to remove
check_networks() from add_device() then ? Networks would only be built once
the GPRS interface is up. I guess that would work with the current code and if
not we should add the networks once we get the GPRS interface from
modem_changed().
>
> done:
> @@ -1277,28 +1313,6 @@ done:
> dbus_pending_call_unref(call);
> }
>
> -static void modem_remove_device(struct modem_data *modem)
> -{
> - if (modem->device == NULL)
> - return;
> -
> - connman_device_unregister(modem->device);
> - connman_device_unref(modem->device);
> -
> - modem->device = NULL;
> -}
> -
> -static void remove_modem(gpointer data)
> -{
> - struct modem_data *modem = data;
> -
> - g_free(modem->path);
> -
> - modem_remove_device(modem);
> -
> - g_free(modem);
> -}
> -
> static void ofono_connect(DBusConnection *connection, void *user_data)
> {
> DBusMessage *message;
> @@ -1373,15 +1387,56 @@ static gboolean modem_changed(DBusConnection
> *connection, DBusMessage *message,
> dbus_bool_t powered;
>
> dbus_message_iter_get_basic(&value, &powered);
> - if (powered == TRUE)
> - return TRUE;
> -
> - modem_remove_device(modem);
> + if (powered == FALSE)
> + modem_remove_device(modem);
You're not changing the logic here, please avoid those changes.
> } else if (g_str_equal(key, "Interfaces") == TRUE) {
> - if (modem_has_gprs(&value) == TRUE) {
> + if (modem_has_sim(&value)) {
> if (modem->device == NULL)
> - get_imsi(modem->path);
> - } else if (modem->device != NULL)
> + get_imsi(path);
> + } else {
> + if (modem->device != NULL)
Please keep the } else if (modem->device != NULL) line here.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
