On 02/12/2013 02:18 AM, Ben Chan wrote:I tried to test this with a QMI modem and an external antenna, but
> 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.
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.
Why do we bail out if UNKNOWN here? If we end up 15s without getting
> ---
> 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;
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.
For the sake of clarity, I would also clear the deferred state value here:
> +
> + 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;
ctx->deferred_new_state = MM_MODEM_3GPP_REGISTRATION_STATE_UNKNOWN;
I believe there may be an issue here if we ever get multiple
> + }
> +
> /* 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)) {
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.
Aleksander
> + 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
>
--