Re: [MM] [PATCH] iface-modem-3gpp: defer registration state update when registration is lost
Thanks Aleksander. Submitted patch v3.
On Thu, Feb 14, 2013 at 2:01 AM, Aleksander Morgado
wrote:
> On 02/12/2013 02:18 AM, Ben Chan wrote:
> > This patch defers the update of 3GPP registration state by 15 seconds
> > when the registration state changes from 'registered' (home / roaming)
> > to 'searching'. This allows a temporary loss of 3GPP registration to
> > recover itself when relying on ModemManager to explicitly disconnect and
> > reconnect to the network.
>
>
> I tried to test this with a QMI modem and an external antenna, but
> disconnecting the antenna while connected just gets me to signal quality
> 0, it doesn't get neither unregistered nor disconnected. Are you able to
> grab me some debug logs of a test case where this happens?
>
> See some additional comments inline below.
>
>
> > ---
> > src/mm-iface-modem-3gpp.c | 108
> +-
> > 1 file changed, 88 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/mm-iface-modem-3gpp.c b/src/mm-iface-modem-3gpp.c
> > index 7a88b2f..f5d703d 100644
> > --- a/src/mm-iface-modem-3gpp.c
> > +++ b/src/mm-iface-modem-3gpp.c
> > @@ -29,6 +29,7 @@
> > #include "mm-log.h"
> >
> > #define REGISTRATION_CHECK_TIMEOUT_SEC 30
> > +#define DEFERRED_REGISTRATION_STATE_UPDATE_TIMEOUT_SEC 15
> >
> > #define SUBSYSTEM_3GPP "3gpp"
> >
> > @@ -72,6 +73,8 @@ mm_iface_modem_3gpp_bind_simple_status
> (MMIfaceModem3gpp *self,
> > typedef struct {
> > MMModem3gppRegistrationState cs;
> > MMModem3gppRegistrationState ps;
> > +MMModem3gppRegistrationState deferred_new_state;
> > +guint deferred_update_id;
> > gboolean manual_registration;
> > GCancellable *pending_registration_cancellable;
> > gboolean reloading_operator;
> > @@ -84,6 +87,9 @@ registration_state_context_free
> (RegistrationStateContext *ctx)
> > g_cancellable_cancel (ctx->pending_registration_cancellable);
> > g_object_unref (ctx->pending_registration_cancellable);
> > }
> > +if (ctx->deferred_update_id) {
> > +g_source_remove (ctx->deferred_update_id);
> > +}
> > g_slice_free (RegistrationStateContext, ctx);
> > }
> >
> > @@ -102,6 +108,7 @@ get_registration_state_context (MMIfaceModem3gpp
> *self)
> > ctx = g_slice_new0 (RegistrationStateContext);
> > ctx->cs = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> > ctx->ps = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> > +ctx->deferred_new_state =
> MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> >
> > g_object_set_qdata_full (
> > G_OBJECT (self),
> > @@ -1019,25 +1026,84 @@
> update_registration_reload_current_operator_ready (MMIfaceModem3gpp *self,
> > }
> >
> > static void
> > +update_non_registered_state (MMIfaceModem3gpp *self,
> > + MMModem3gppRegistrationState old_state,
> > + MMModem3gppRegistrationState new_state)
> > +{
> > +/* Not registered neither in home nor roaming network */
> > +mm_iface_modem_3gpp_clear_current_operator (self);
> > +
> > +/* The property in the interface is bound to the property
> > + * in the skeleton, so just updating here is enough */
> > +g_object_set (self,
> > + MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, new_state,
> > + NULL);
> > +
> > +mm_iface_modem_update_subsystem_state (
> > +MM_IFACE_MODEM (self),
> > +SUBSYSTEM_3GPP,
> > +(new_state == MM_MODEM_3GPP_REGISTRATION_STATE_SEARCHING ?
> > + MM_MODEM_STATE_SEARCHING :
> > + MM_MODEM_STATE_ENABLED),
> > +MM_MODEM_STATE_CHANGE_REASON_UNKNOWN);
> > +}
> > +
> > +static gboolean
> > +run_deferred_registration_state_update (MMIfaceModem3gpp *self)
> > +{
> > +MMModem3gppRegistrationState old_state =
> MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> > +MMModem3gppRegistrationState new_state;
> > +RegistrationStateContext *ctx;
> > +
> > +ctx = get_registration_state_context (self);
> > +ctx->deferred_update_id = 0;
> > +
> > +g_object_get (self,
> > + MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, &old_state,
> > + NULL);
> > +new_state = ctx->deferred_new_state;
> > +
> > +/* Only set new state if it is known and different from the old one
> */
> > +if (new_state == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN ||
> > +new_state == old_state)
> > +return FALSE;
>
> Why do we bail out if UNKNOWN here? If we end up 15s without getting
> re-registered and the original new state that we got was UNKNOWN, we
> would not be doing anything, so the state would still say 'registered'
> in the interface, while we are not.
>
> > +
> > +mm_info ("Modem %s: (deferred) 3GPP Registration state changed (%s
> -> %s)",
> > + g_dbus_object_get_object_path (G_DBUS_OBJECT (self)),
> > + mm_modem_3gpp_registration_state_get_string (old_state),
> > + mm_modem_
Re: [MM] [PATCH] iface-modem-3gpp: defer registration state update when registration is lost
On 02/12/2013 02:18 AM, Ben Chan wrote:
> This patch defers the update of 3GPP registration state by 15 seconds
> when the registration state changes from 'registered' (home / roaming)
> to 'searching'. This allows a temporary loss of 3GPP registration to
> recover itself when relying on ModemManager to explicitly disconnect and
> reconnect to the network.
I tried to test this with a QMI modem and an external antenna, but
disconnecting the antenna while connected just gets me to signal quality
0, it doesn't get neither unregistered nor disconnected. Are you able to
grab me some debug logs of a test case where this happens?
See some additional comments inline below.
> ---
> src/mm-iface-modem-3gpp.c | 108
> +-
> 1 file changed, 88 insertions(+), 20 deletions(-)
>
> diff --git a/src/mm-iface-modem-3gpp.c b/src/mm-iface-modem-3gpp.c
> index 7a88b2f..f5d703d 100644
> --- a/src/mm-iface-modem-3gpp.c
> +++ b/src/mm-iface-modem-3gpp.c
> @@ -29,6 +29,7 @@
> #include "mm-log.h"
>
> #define REGISTRATION_CHECK_TIMEOUT_SEC 30
> +#define DEFERRED_REGISTRATION_STATE_UPDATE_TIMEOUT_SEC 15
>
> #define SUBSYSTEM_3GPP "3gpp"
>
> @@ -72,6 +73,8 @@ mm_iface_modem_3gpp_bind_simple_status (MMIfaceModem3gpp
> *self,
> typedef struct {
> MMModem3gppRegistrationState cs;
> MMModem3gppRegistrationState ps;
> +MMModem3gppRegistrationState deferred_new_state;
> +guint deferred_update_id;
> gboolean manual_registration;
> GCancellable *pending_registration_cancellable;
> gboolean reloading_operator;
> @@ -84,6 +87,9 @@ registration_state_context_free (RegistrationStateContext
> *ctx)
> g_cancellable_cancel (ctx->pending_registration_cancellable);
> g_object_unref (ctx->pending_registration_cancellable);
> }
> +if (ctx->deferred_update_id) {
> +g_source_remove (ctx->deferred_update_id);
> +}
> g_slice_free (RegistrationStateContext, ctx);
> }
>
> @@ -102,6 +108,7 @@ get_registration_state_context (MMIfaceModem3gpp *self)
> ctx = g_slice_new0 (RegistrationStateContext);
> ctx->cs = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> ctx->ps = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> +ctx->deferred_new_state = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
>
> g_object_set_qdata_full (
> G_OBJECT (self),
> @@ -1019,25 +1026,84 @@ update_registration_reload_current_operator_ready
> (MMIfaceModem3gpp *self,
> }
>
> static void
> +update_non_registered_state (MMIfaceModem3gpp *self,
> + MMModem3gppRegistrationState old_state,
> + MMModem3gppRegistrationState new_state)
> +{
> +/* Not registered neither in home nor roaming network */
> +mm_iface_modem_3gpp_clear_current_operator (self);
> +
> +/* The property in the interface is bound to the property
> + * in the skeleton, so just updating here is enough */
> +g_object_set (self,
> + MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, new_state,
> + NULL);
> +
> +mm_iface_modem_update_subsystem_state (
> +MM_IFACE_MODEM (self),
> +SUBSYSTEM_3GPP,
> +(new_state == MM_MODEM_3GPP_REGISTRATION_STATE_SEARCHING ?
> + MM_MODEM_STATE_SEARCHING :
> + MM_MODEM_STATE_ENABLED),
> +MM_MODEM_STATE_CHANGE_REASON_UNKNOWN);
> +}
> +
> +static gboolean
> +run_deferred_registration_state_update (MMIfaceModem3gpp *self)
> +{
> +MMModem3gppRegistrationState old_state =
> MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
> +MMModem3gppRegistrationState new_state;
> +RegistrationStateContext *ctx;
> +
> +ctx = get_registration_state_context (self);
> +ctx->deferred_update_id = 0;
> +
> +g_object_get (self,
> + MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, &old_state,
> + NULL);
> +new_state = ctx->deferred_new_state;
> +
> +/* Only set new state if it is known and different from the old one */
> +if (new_state == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN ||
> +new_state == old_state)
> +return FALSE;
Why do we bail out if UNKNOWN here? If we end up 15s without getting
re-registered and the original new state that we got was UNKNOWN, we
would not be doing anything, so the state would still say 'registered'
in the interface, while we are not.
> +
> +mm_info ("Modem %s: (deferred) 3GPP Registration state changed (%s ->
> %s)",
> + g_dbus_object_get_object_path (G_DBUS_OBJECT (self)),
> + mm_modem_3gpp_registration_state_get_string (old_state),
> + mm_modem_3gpp_registration_state_get_string (new_state));
> +
> +update_non_registered_state (self, old_state, new_state);
> +
> +return FALSE;
> +}
> +
> +static void
> update_registration_state (MMIfaceModem3gpp *self,
> MMModem3gppRegistrationState new_state)
> {
> MMModem3
[MM] [PATCH] iface-modem-3gpp: defer registration state update when registration is lost
This patch defers the update of 3GPP registration state by 15 seconds
when the registration state changes from 'registered' (home / roaming)
to 'searching'. This allows a temporary loss of 3GPP registration to
recover itself when relying on ModemManager to explicitly disconnect and
reconnect to the network.
---
src/mm-iface-modem-3gpp.c | 108 +-
1 file changed, 88 insertions(+), 20 deletions(-)
diff --git a/src/mm-iface-modem-3gpp.c b/src/mm-iface-modem-3gpp.c
index 7a88b2f..f5d703d 100644
--- a/src/mm-iface-modem-3gpp.c
+++ b/src/mm-iface-modem-3gpp.c
@@ -29,6 +29,7 @@
#include "mm-log.h"
#define REGISTRATION_CHECK_TIMEOUT_SEC 30
+#define DEFERRED_REGISTRATION_STATE_UPDATE_TIMEOUT_SEC 15
#define SUBSYSTEM_3GPP "3gpp"
@@ -72,6 +73,8 @@ mm_iface_modem_3gpp_bind_simple_status (MMIfaceModem3gpp
*self,
typedef struct {
MMModem3gppRegistrationState cs;
MMModem3gppRegistrationState ps;
+MMModem3gppRegistrationState deferred_new_state;
+guint deferred_update_id;
gboolean manual_registration;
GCancellable *pending_registration_cancellable;
gboolean reloading_operator;
@@ -84,6 +87,9 @@ registration_state_context_free (RegistrationStateContext
*ctx)
g_cancellable_cancel (ctx->pending_registration_cancellable);
g_object_unref (ctx->pending_registration_cancellable);
}
+if (ctx->deferred_update_id) {
+g_source_remove (ctx->deferred_update_id);
+}
g_slice_free (RegistrationStateContext, ctx);
}
@@ -102,6 +108,7 @@ get_registration_state_context (MMIfaceModem3gpp *self)
ctx = g_slice_new0 (RegistrationStateContext);
ctx->cs = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
ctx->ps = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
+ctx->deferred_new_state = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
g_object_set_qdata_full (
G_OBJECT (self),
@@ -1019,25 +1026,84 @@ update_registration_reload_current_operator_ready
(MMIfaceModem3gpp *self,
}
static void
+update_non_registered_state (MMIfaceModem3gpp *self,
+ MMModem3gppRegistrationState old_state,
+ MMModem3gppRegistrationState new_state)
+{
+/* Not registered neither in home nor roaming network */
+mm_iface_modem_3gpp_clear_current_operator (self);
+
+/* The property in the interface is bound to the property
+ * in the skeleton, so just updating here is enough */
+g_object_set (self,
+ MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, new_state,
+ NULL);
+
+mm_iface_modem_update_subsystem_state (
+MM_IFACE_MODEM (self),
+SUBSYSTEM_3GPP,
+(new_state == MM_MODEM_3GPP_REGISTRATION_STATE_SEARCHING ?
+ MM_MODEM_STATE_SEARCHING :
+ MM_MODEM_STATE_ENABLED),
+MM_MODEM_STATE_CHANGE_REASON_UNKNOWN);
+}
+
+static gboolean
+run_deferred_registration_state_update (MMIfaceModem3gpp *self)
+{
+MMModem3gppRegistrationState old_state =
MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
+MMModem3gppRegistrationState new_state;
+RegistrationStateContext *ctx;
+
+ctx = get_registration_state_context (self);
+ctx->deferred_update_id = 0;
+
+g_object_get (self,
+ MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, &old_state,
+ NULL);
+new_state = ctx->deferred_new_state;
+
+/* Only set new state if it is known and different from the old one */
+if (new_state == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN ||
+new_state == old_state)
+return FALSE;
+
+mm_info ("Modem %s: (deferred) 3GPP Registration state changed (%s -> %s)",
+ g_dbus_object_get_object_path (G_DBUS_OBJECT (self)),
+ mm_modem_3gpp_registration_state_get_string (old_state),
+ mm_modem_3gpp_registration_state_get_string (new_state));
+
+update_non_registered_state (self, old_state, new_state);
+
+return FALSE;
+}
+
+static void
update_registration_state (MMIfaceModem3gpp *self,
MMModem3gppRegistrationState new_state)
{
MMModem3gppRegistrationState old_state =
MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
+RegistrationStateContext *ctx;
g_object_get (self,
MM_IFACE_MODEM_3GPP_REGISTRATION_STATE, &old_state,
NULL);
+ctx = get_registration_state_context (self);
+g_assert (ctx);
+
+/* Cancel any deferred registration state update */
+if (ctx->deferred_update_id != 0) {
+g_source_remove (ctx->deferred_update_id);
+ctx->deferred_update_id = 0;
+}
+
/* Only set new state if different */
if (new_state == old_state)
return;
if (new_state == MM_MODEM_3GPP_REGISTRATION_STATE_HOME ||
new_state == MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING) {
-RegistrationStateContext *ctx;
-
-ctx = get_registration_state_context (self);
-
