[RfC] Retry registering packet radio to home PLMN when available.

[RfC] Retry registering packet radio to home PLMN when available.

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 9429 bytes --]

Instead of setting DataConnectionManager.Powered to false after
failed attempts to register to home PLMN with packet radio when
RoamingAllowed is false, wait for the home PLMN to become available
retrying everytime the circuit switched radio manages to register
to home PLMN.  The downside is if the circuit switched radio is off,
we wait indefinitely.

Also don't emit a useless PropertyChanged signal when we attached
and are going to detach immediately because of RoamingAllowed.

Hopefully this makes the code easier to follow.
---
 src/gprs.c |  147 +++++++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 106 insertions(+), 41 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 2769c27..d3f3318 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -38,6 +38,7 @@
 #define DATA_CONTEXT_INTERFACE "org.ofono.PrimaryDataContext"
 
 #define GPRS_FLAG_ATTACHING 0x1
+#define GPRS_FLAG_DETACHED_AFTER_ROAMING 0x2
 
 static GSList *g_drivers = NULL;
 static GSList *g_context_drivers = NULL;
@@ -51,6 +52,7 @@ enum gprs_context_type {
 struct ofono_gprs {
 	GSList *contexts;
 	ofono_bool_t attached;
+	ofono_bool_t driver_attached;
 	ofono_bool_t roaming_allowed;
 	ofono_bool_t powered;
 	int status;
@@ -61,6 +63,10 @@ struct ofono_gprs {
 	int next_context_id;
 	int cid_min;
 	int cid_max;
+	int netreg_status;
+	struct ofono_netreg *netreg;
+	unsigned int netreg_watch;
+	unsigned int status_watch;
 	DBusMessage *pending;
 	struct ofono_gprs_context *context_driver;
 	const struct ofono_gprs_driver *driver;
@@ -506,23 +512,32 @@ static char **gprs_contexts_path_list(GSList *context_list)
 	return objlist;
 }
 
-static void gprs_attach_callback(const struct ofono_error *error, void *data)
+static void gprs_set_attached(struct ofono_gprs *gprs)
 {
-	struct ofono_gprs *gprs = data;
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path;
 	dbus_bool_t value;
+	ofono_bool_t attached = gprs->driver_attached &&
+		!(gprs->flags & GPRS_FLAG_DETACHED_AFTER_ROAMING);
 
-	if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
-			(gprs->flags & GPRS_FLAG_ATTACHING)) {
-		gprs->attached = !gprs->attached;
+	if (attached != gprs->attached) {
+		gprs->attached = attached;
 
 		path = __ofono_atom_get_path(gprs->atom);
-		value = gprs->attached;
+		value = attached;
 		ofono_dbus_signal_property_changed(conn, path,
 					DATA_CONNECTION_MANAGER_INTERFACE,
 					"Attached", DBUS_TYPE_BOOLEAN, &value);
 	}
+}
+
+static void gprs_attach_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_gprs *gprs = data;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
+			(gprs->flags & GPRS_FLAG_ATTACHING))
+		gprs->driver_attached = !gprs->driver_attached;
 
 	gprs->flags &= ~GPRS_FLAG_ATTACHING;
 
@@ -531,18 +546,21 @@ static void gprs_attach_callback(const struct ofono_error *error, void *data)
 
 static void gprs_netreg_update(struct ofono_gprs *gprs)
 {
-	DBusConnection *conn = ofono_dbus_get_connection();
 	int attach;
 	int operator_ok;
-	const char *path;
-	dbus_bool_t value = 0;
 
 	operator_ok = gprs->roaming_allowed ||
-		(gprs->status != NETWORK_REGISTRATION_STATUS_ROAMING);
+		(gprs->status != NETWORK_REGISTRATION_STATUS_ROAMING &&
+		 !(gprs->flags & GPRS_FLAG_DETACHED_AFTER_ROAMING));
+
+	if (gprs->powered && !operator_ok)
+		gprs->flags |= GPRS_FLAG_DETACHED_AFTER_ROAMING;
+
+	gprs_set_attached(gprs);
 
 	attach = gprs->powered && operator_ok;
 
-	if (gprs->attached != attach &&
+	if (gprs->driver_attached != attach &&
 			!(gprs->flags & GPRS_FLAG_ATTACHING) &&
 			!(attach && gprs->status !=
 				NETWORK_REGISTRATION_STATUS_SEARCHING)) {
@@ -550,17 +568,33 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
 
 		gprs->driver->set_attached(gprs, attach, gprs_attach_callback,
 						gprs);
+	}
+}
 
-		/* Prevent further attempts to attach */
-		if (!attach && gprs->powered) {
-			gprs->powered = 0;
+static void netreg_status_changed(int status, int lac, int ci, int tech,
+					const struct ofono_network_operator *op,
+					void *data)
+{
+	struct ofono_gprs *gprs = data;
 
-			path = __ofono_atom_get_path(gprs->atom);
-			ofono_dbus_signal_property_changed(conn, path,
-					DATA_CONNECTION_MANAGER_INTERFACE,
-					"Powered", DBUS_TYPE_BOOLEAN, &value);
-		}
-	}
+	DBG("%d, %d, %d, %d, %p", status, lac, ci, tech, op);
+
+	if (gprs->netreg_status == status)
+		return;
+	gprs->netreg_status = status;
+
+	if (!(gprs->flags & GPRS_FLAG_DETACHED_AFTER_ROAMING))
+		return;
+
+	if (status != NETWORK_REGISTRATION_STATUS_REGISTERED)
+		return;
+
+	/* If the circuit switched radio just registered to home PLMN then
+	 * we also make an attempt to attach.
+	 */
+	gprs->flags &= ~GPRS_FLAG_DETACHED_AFTER_ROAMING;
+
+	gprs_netreg_update(gprs);
 }
 
 static DBusMessage *gprs_get_properties(DBusConnection *conn,
@@ -667,6 +701,8 @@ static DBusMessage *gprs_set_property(DBusConnection *conn,
 			return dbus_message_new_method_return(msg);
 
 		gprs->roaming_allowed = value;
+		gprs->flags &= ~GPRS_FLAG_DETACHED_AFTER_ROAMING;
+
 		gprs_netreg_update(gprs);
 	} else if (!strcmp(property, "Powered")) {
 		if (!gprs->driver->set_attached)
@@ -681,6 +717,8 @@ static DBusMessage *gprs_set_property(DBusConnection *conn,
 			return dbus_message_new_method_return(msg);
 
 		gprs->powered = value;
+		gprs->flags &= ~GPRS_FLAG_DETACHED_AFTER_ROAMING;
+
 		gprs_netreg_update(gprs);
 	} else
 		return __ofono_error_invalid_args(msg);
@@ -854,17 +892,9 @@ static GDBusSignalTable manager_signals[] = {
 
 void ofono_gprs_attach_notify(struct ofono_gprs *gprs, int attached)
 {
-	DBusConnection *conn = ofono_dbus_get_connection();
-	const char *path;
-	dbus_bool_t value = 0;
-
-	if (gprs->attached && !(gprs->flags & GPRS_FLAG_ATTACHING)) {
-		gprs->attached = 0;
-
-		path = __ofono_atom_get_path(gprs->atom);
-		ofono_dbus_signal_property_changed(conn, path,
-					DATA_CONNECTION_MANAGER_INTERFACE,
-					"Attached", DBUS_TYPE_BOOLEAN, &value);
+	if (gprs->driver_attached != attached &&
+			!(gprs->flags & GPRS_FLAG_ATTACHING)) {
+		gprs->driver_attached = attached;
 
 		gprs_netreg_update(gprs);
 	}
@@ -875,7 +905,7 @@ static void set_registration_status(struct ofono_gprs *gprs, int status)
 	const char *str_status = registration_status_to_string(status);
 	const char *path = __ofono_atom_get_path(gprs->atom);
 	DBusConnection *conn = ofono_dbus_get_connection();
-	dbus_bool_t attached;
+	ofono_bool_t attached;
 
 	gprs->status = status;
 
@@ -886,15 +916,8 @@ static void set_registration_status(struct ofono_gprs *gprs, int 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);
-	}
+	if (!(gprs->flags & GPRS_FLAG_ATTACHING))
+		gprs->driver_attached = attached;
 
 	gprs_netreg_update(gprs);
 }
@@ -1135,6 +1158,18 @@ static void gprs_unregister(struct ofono_atom *atom)
 
 	g_slist_free(gprs->contexts);
 
+	if (gprs->netreg_watch) {
+		if (gprs->status_watch) {
+			__ofono_netreg_remove_status_watch(gprs->netreg,
+							gprs->status_watch);
+			gprs->status_watch = 0;
+		}
+
+		__ofono_modem_remove_atom_watch(modem, gprs->netreg_watch);
+		gprs->netreg_watch = 0;
+		gprs->netreg = NULL;
+	}
+
 	ofono_modem_remove_interface(modem, DATA_CONNECTION_MANAGER_INTERFACE);
 	g_dbus_unregister_interface(conn, path,
 					DATA_CONNECTION_MANAGER_INTERFACE);
@@ -1194,11 +1229,30 @@ struct ofono_gprs *ofono_gprs_create(struct ofono_modem *modem,
 	return gprs;
 }
 
+static void netreg_watch(struct ofono_atom *atom,
+				enum ofono_atom_watch_condition cond,
+				void *data)
+{
+	struct ofono_gprs *gprs = data;
+
+	if (cond != OFONO_ATOM_WATCH_CONDITION_REGISTERED) {
+		gprs->status_watch = 0;
+		gprs->netreg = NULL;
+		return;
+	}
+
+	gprs->netreg = __ofono_atom_get_data(atom);
+	gprs->netreg_status = ofono_netreg_get_status(gprs->netreg);
+	gprs->status_watch = __ofono_netreg_add_status_watch(gprs->netreg,
+					netreg_status_changed, gprs, NULL);
+}
+
 void ofono_gprs_register(struct ofono_gprs *gprs)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
 	const char *path = __ofono_atom_get_path(gprs->atom);
+	struct ofono_atom *netreg_atom;
 
 	if (!g_dbus_register_interface(conn, path,
 					DATA_CONNECTION_MANAGER_INTERFACE,
@@ -1212,6 +1266,17 @@ void ofono_gprs_register(struct ofono_gprs *gprs)
 
 	ofono_modem_add_interface(modem, DATA_CONNECTION_MANAGER_INTERFACE);
 
+	gprs->netreg_watch = __ofono_modem_add_atom_watch(modem,
+					OFONO_ATOM_TYPE_NETREG,
+					netreg_watch, gprs, NULL);
+
+	netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
+
+	if (netreg_atom && __ofono_atom_get_registered(netreg_atom))
+		netreg_watch(netreg_atom,
+				OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);
+
+
 	__ofono_atom_register(gprs->atom, gprs_unregister);
 }
 
-- 
1.6.1

Re: [RfC] Retry registering packet radio to home PLMN when available.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

Hi Andrew,

So I took this patch and the two earlier patches and applied them.  Thanks.

> Instead of setting DataConnectionManager.Powered to false after
> failed attempts to register to home PLMN with packet radio when
> RoamingAllowed is false, wait for the home PLMN to become available
> retrying everytime the circuit switched radio manages to register
> to home PLMN.  The downside is if the circuit switched radio is off,
> we wait indefinitely.

So I decided to simplify things even further.  Before we can attach we must be 
registered to the network (e.g. COPS issued), otherwise attach fails.  Since 
we need netreg for this anyway, we might as well always rely on netreg's 
registration status to base our attach / detach decisions on.

To me this seems to be the most sensible way to approach this.  However, this 
assumes that the modem can't be registered on netreg and roaming on gprs.

It also might break down in low-signal conditions where packet radio has 
signal but circuit-switched radio doesn't (which is interesting case where 
COPS reports an operator, CGREG shows registered, but CREG shows unregistered)

Comments?

Regards,
-Denis

Re: [RfC] Retry registering packet radio to home PLMN when available.

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1848 bytes --]

Hi,

2009/10/27 Denis Kenzior <denkenz@gmail.com>:
>> Instead of setting DataConnectionManager.Powered to false after
>> failed attempts to register to home PLMN with packet radio when
>> RoamingAllowed is false, wait for the home PLMN to become available
>> retrying everytime the circuit switched radio manages to register
>> to home PLMN.  The downside is if the circuit switched radio is off,
>> we wait indefinitely.
>
> So I decided to simplify things even further.  Before we can attach we must be
> registered to the network (e.g. COPS issued), otherwise attach fails.  Since
> we need netreg for this anyway, we might as well always rely on netreg's
> registration status to base our attach / detach decisions on.
>
> To me this seems to be the most sensible way to approach this.  However, this
> assumes that the modem can't be registered on netreg and roaming on gprs.
>
> It also might break down in low-signal conditions where packet radio has
> signal but circuit-switched radio doesn't (which is interesting case where
> COPS reports an operator, CGREG shows registered, but CREG shows unregistered)
>
> Comments?

One reason you might want to not register to network would be for power saving.

It seems like a small gain for adding this dependency (which will seem
especially strange to users not knowing about this quirk we're relying
on).  The current picture is that we want to have an indication of
when we might be able to register back to home PLMN with gprs.  We
keep the gprs radio disabled and enable the circuit-switched radio
possibly with the only aim being to have that (possibly weak)
indication.  Couldn't we instead use the gprs radio for the same
purpose?  We would simply prohibit activating contexts while roaming
and we would report Attached = false on D-Bus.

Regards

Re: [RfC] Retry registering packet radio to home PLMN when available.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1747 bytes --]

Hi Andrew,

> One reason you might want to not register to network would be for power
> saving.

Sure, but remember there are two cases here:

The device oFono is managing is a proper voice + data modem.  In which case 
the likely scenario is that we want to always be registered to the voice 
network and not register to GPRS network for power reasons.

The second case is we're managing a GPRS-only data device.  In which case the 
power considerations are flipped.

>
> It seems like a small gain for adding this dependency (which will seem
> especially strange to users not knowing about this quirk we're relying
> on).  The current picture is that we want to have an indication of
> when we might be able to register back to home PLMN with gprs.  We
> keep the gprs radio disabled and enable the circuit-switched radio
> possibly with the only aim being to have that (possibly weak)

For the devices that oFono is primarily targeting (e.g. voice + data devices) 
this behavior is exactly what we want, no?

> indication.  Couldn't we instead use the gprs radio for the same
> purpose?  We would simply prohibit activating contexts while roaming
> and we would report Attached = false on D-Bus.

I agree that we want to do this for data-only devices.  In which case I 
suspect we need to have the driver tell oFono that it is a data-only device.  
With implications that we always auto-attach, and either ignore RoamingAllowed 
setting completely or do as you suggest and refuse to activate the context. 

The tricky part is how to report the operator.  Remember that it is quite 
ambiguous what COPS=0 does.  On some devices it activates CSD radio.  On some 
GPRS, and both on others.

Regards.
=Demos