Hi,
2009/10/24 Denis Kenzior <[email protected]>:
> Ok, so I took the patch and hacked on it for a while. I disagreed with how
> you split up responsibilities, so much of the logic got moved into the core
> and the driver was simplified.
Yes, there were a couple of possible choices, I chose to align with
how the list of voice calls was managed in voicecall.c .
>
> I also decided to split up GPRS into two atoms to ease integration of hardware
> that supports dedicated network interfaces. This way the attach / GPRS
> network registration parameters can be reused from the generic 'atmodem'
> driver, while specific context handling can be customized.
That's a good idea.
>
> I'm happy to report that this actually sort of works with my MBM card. I can
> even define a GPRS context and activate it. Of course the card crashes as
> soon
> as I do :)
I'm happy to report it still works on my Nokia phone, too, except for
one test: when you have active contexts and either tell ofono to power
gprs down or are detached by the network for whatever reason,
".active" is never reset and no event emitted, meaning that you can't
remove the context, can't re-activate it or deactivate it.
>
> Some parts I still don't understand:
>
> in set_registration_status:
> attached = (status != NETWORK_REGISTRATION_STATUS_REGISTERED &&
> status != NETWORK_REGISTRATION_STATUS_ROAMING);
> if (gprs->attached != (int) attached &&
> !(gprs->flags & GPRS_FLAG_ATTACHING)) {
> gprs->attached = (int) attached;
>
> ofono_dbus_signal_property_changed(conn, path,
> DATA_CONNECTION_MANAGER_INTERFACE,
> "Attached", DBUS_TYPE_BOOLEAN,
> &attached);
>
> gprs_netreg_update(gprs);
> }
>
> I do not understand this logic at all. Can't we always call
> gprs_netreg_update here?
Sure, but it won't do anything, if either "attached" state hasn't
changed at all or it's already busy executing +CGATT.
> In my opinion Attached should only be emitted once
> it really has changed (e.g. in the callback)
Often there's no other notification telling us that we were detached,
so we must take any sign of it into account (it might be broken modems
but I've seen such situations on both modems that I've tested it with)
>
> gprs_netreg_update:
> /* Prevent further attempts to attach */
> if (!attach && gprs->powered) {
> gprs->powered = 0;
>
> path = __ofono_atom_get_path(gprs->atom);
> ofono_dbus_signal_property_changed(conn, path,
> DATA_CONNECTION_MANAGER_INTERFACE,
> "Powered", DBUS_TYPE_BOOLEAN, &value);
> }
>
> This is just too evil. Can't we set another flag that attached conditions
> have
> changed while we were attaching/detaching and that we should recheck those
> conditions when we return from attach/detach?
Is it evil because we change a property that is writable?
This is only for the case of RoamingAllowed = 0. In my understanding
"Powered" being set means that we're attached or attempting to attach,
which is not the case after we detached because of roaming, so the
D-Bus client would be misled.
>
> In gprs_set_property:
>
> You should really ignore set property requests that set the value to the
> already set value. Simply return success and don't do anything else.
Good point, the attached patch checks for those cases.
Best regards
From 6d4439fc701ca49ad88149833ac2f4b6d68ba4f3 Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <[email protected]>
Date: Sat, 24 Oct 2009 11:27:23 +0200
Subject: [PATCH 1/2] Just return success when value already set in SetProperty.
---
src/gprs.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/gprs.c b/src/gprs.c
index fb08d9a..1827ceb 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -661,6 +661,9 @@ static DBusMessage *gprs_set_property(DBusConnection *conn,
dbus_message_iter_get_basic(&var, &value);
+ if (gprs->roaming_allowed == (ofono_bool_t) value)
+ return dbus_message_new_method_return(msg);
+
gprs->roaming_allowed = value;
gprs_netreg_update(gprs);
} else if (!strcmp(property, "Powered")) {
@@ -672,6 +675,9 @@ static DBusMessage *gprs_set_property(DBusConnection *conn,
dbus_message_iter_get_basic(&var, &value);
+ if (gprs->powered == (ofono_bool_t) value)
+ return dbus_message_new_method_return(msg);
+
gprs->powered = value;
gprs_netreg_update(gprs);
} else
--
1.6.1
From e9498d070f3593c1ef5c27b91f59ca8e2be971ad Mon Sep 17 00:00:00 2001
From: Andrzej Zaborowski <[email protected]>
Date: Sat, 24 Oct 2009 11:28:07 +0200
Subject: [PATCH 2/2] Register gprs-context on calypso modem/phonesim.
---
plugins/phonesim.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/plugins/phonesim.c b/plugins/phonesim.c
index 1151da4..9463265 100644
--- a/plugins/phonesim.c
+++ b/plugins/phonesim.c
@@ -55,6 +55,7 @@
#include <ofono/ussd.h>
#include <ofono/voicecall.h>
#include <ofono/gprs.h>
+#include <ofono/gprs-context.h>
#include <drivers/atmodem/vendor.h>
@@ -289,6 +290,8 @@ static void phonesim_post_sim(struct ofono_modem *modem)
{
struct phonesim_data *data = ofono_modem_get_data(modem);
struct ofono_message_waiting *mw;
+ struct ofono_gprs *gprs;
+ struct ofono_gprs_context *gc;
DBG("%p", modem);
@@ -313,7 +316,11 @@ static void phonesim_post_sim(struct ofono_modem *modem)
ofono_cbs_create(modem, 0, "atmodem", data->chat);
}
- ofono_gprs_create(modem, 0, "atmodem", data->chat);
+ gprs = ofono_gprs_create(modem, 0, "atmodem", data->chat);
+ gc = ofono_gprs_context_create(modem, 0, "atmodem", data->chat);
+
+ if (gprs && gc)
+ ofono_gprs_add_context(gprs, gc);
mw = ofono_message_waiting_create(modem);
if (mw)
--
1.6.1
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
