Thanks Aleksander. Submitted patch v3.
On Thu, Feb 14, 2013 at 2:01 AM, Aleksander Morgado <[email protected]>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_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; > > For the sake of clarity, I would also clear the deferred state value here: > ctx->deferred_new_state = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN; > > > > + } > > + > > /* 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); > > - > > /* If already reloading operator, skip it */ > > if (ctx->reloading_operator) > > return; > > @@ -1056,27 +1122,29 @@ update_registration_state (MMIfaceModem3gpp > *self, > > return; > > } > > > > + if ((old_state == MM_MODEM_3GPP_REGISTRATION_STATE_HOME || > > + old_state == MM_MODEM_3GPP_REGISTRATION_STATE_ROAMING) && > > + (new_state == MM_MODEM_3GPP_REGISTRATION_STATE_SEARCHING || > > + new_state == MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN)) { > > I believe there may be an issue here if we ever get multiple > notifications of registration states during our wait. So imagine the > case that we get this flow: > > We get unregistered: > HOME --> UNKNOWN > we setup the 15s timeout > > After some seconds: > UNKNOWN --> SEARCHING > we clear the previous timeout and we add a new one of 15s > > After some seconds: > SEARCHING --> UNKNOWN > we clear the previous timeout and we add a new one of 15s > > During all this time, we still notified in the DBus interface that we > were registered, while we were not, and it may take much more than 15s. > > A fix for this would be to don't clear the previous timeout if there is > one and if the new state is UNKNOWN or SEARCHING. We could just update > ctx->deferred_new_state to the new value if it changed. > > > > > + mm_info ("Modem %s: 3GPP Registration state changed (%s -> %s), > update deferred", > > + 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)); > > + > > + ctx->deferred_new_state = new_state; > > + ctx->deferred_update_id = g_timeout_add_seconds ( > > + DEFERRED_REGISTRATION_STATE_UPDATE_TIMEOUT_SEC, > > + (GSourceFunc)run_deferred_registration_state_update, > > + self); > > + return; > > + } > > + > > mm_info ("Modem %s: 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)); > > > > - /* 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); > > + update_non_registered_state (self, old_state, new_state); > > } > > > > void > > > > > -- > Aleksander >
_______________________________________________ networkmanager-list mailing list [email protected] https://mail.gnome.org/mailman/listinfo/networkmanager-list
