> Is this previous part really needed? Is there any case in which a LOCK command would succeed but then UNLOCK command report a "not supported" error? I guess it doesn't harm to have this, though, your call.
Right, there is no real need for this, but just for completeness I prefer to keep it, if you don't mind > Looks like csim_lock_support isn't set anywhere to FEATURE_SUPPORTED? It should be set here I believe. Doh! I'll fix it :D On 10 April 2017 at 13:25, Aleksander Morgado <[email protected]> wrote: > On 10/04/17 12:25, Carlo Lobrano wrote: > > Some modems do not support CSIM lock/unlock, but they do support > > querying SIM unlock retries through +CSIM command. > > > > If CSIM lock returns with "unsupported command" do not propagate > > the error and continue with the other CSIM queries instead, moreover the > > CSIM lock feature is signed as FEATURE_UNSUPPORTED in Telit modem > > private structure to prevent being sent again (e.g. calling CSIM > > unlock AT command). > > --- > > > > Hi Aleksander, > > > > I added private structure to store whether csim lock is supported or not > (and so > > skip the command). Let me know if this is ok now. > > > > See below. > > > Carlo > > > > --- > > plugins/telit/mm-broadband-modem-telit.c | 86 > ++++++++++++++++++++++++++------ > > plugins/telit/mm-broadband-modem-telit.h | 2 + > > 2 files changed, 72 insertions(+), 16 deletions(-) > > > > diff --git a/plugins/telit/mm-broadband-modem-telit.c > b/plugins/telit/mm-broadband-modem-telit.c > > index cce0229..841a48b 100644 > > --- a/plugins/telit/mm-broadband-modem-telit.c > > +++ b/plugins/telit/mm-broadband-modem-telit.c > > @@ -42,6 +42,15 @@ G_DEFINE_TYPE_EXTENDED (MMBroadbandModemTelit, > mm_broadband_modem_telit, MM_TYPE > > G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM, > iface_modem_init) > > G_IMPLEMENT_INTERFACE > (MM_TYPE_IFACE_MODEM_3GPP, iface_modem_3gpp_init)); > > > > +typedef enum { > > + FEATURE_SUPPORT_UNKNOWN, > > + FEATURE_NOT_SUPPORTED, > > + FEATURE_SUPPORTED > > +} FeatureSupport; > > + > > +struct _MMBroadbandModemTelitPrivate { > > + FeatureSupport csim_lock_support; > > +}; > > > > /*********************************************************** > ******************/ > > /* After Sim Unlock (Modem interface) */ > > @@ -521,6 +530,11 @@ csim_unlock_ready (MMBaseModem *self, > > /* Ignore errors */ > > response = mm_base_modem_at_command_finish (self, res, &error); > > if (!response) { > > + if (g_error_matches (error, > > + MM_MOBILE_EQUIPMENT_ERROR, > > + MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) > { > > + ctx->self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED; > > + } > > Is this previous part really needed? Is there any case in which a LOCK > command would succeed but then UNLOCK command report a "not supported" > error? I guess it doesn't harm to have this, though, your call. > > > mm_warn ("Couldn't unlock SIM card: %s", error->message); > > g_error_free (error); > > } > > @@ -591,10 +605,18 @@ csim_lock_ready (MMBaseModem *self, > > > > response = mm_base_modem_at_command_finish (self, res, &error); > > if (!response) { > > - g_prefix_error (&error, "Couldn't lock SIM card: "); > > - g_simple_async_result_take_error (ctx->result, error); > > - load_unlock_retries_context_complete_and_free (ctx); > > - return; > > + if (g_error_matches (error, > > + MM_MOBILE_EQUIPMENT_ERROR, > > + MM_MOBILE_EQUIPMENT_ERROR_NOT_SUPPORTED)) > { > > + ctx->self->priv->csim_lock_support = FEATURE_NOT_SUPPORTED; > > + mm_warn ("Couldn't lock SIM card: %s. Continuing without > CSIM lock.", error->message); > > + g_error_free (error); > > + } else { > > + g_prefix_error (&error, "Couldn't lock SIM card: "); > > + g_simple_async_result_take_error (ctx->result, error); > > + load_unlock_retries_context_complete_and_free (ctx); > > + return; > > + } > > } > > > Looks like csim_lock_support isn't set anywhere to FEATURE_SUPPORTED? It > should be set here I believe. > > > > > ctx->step++; > > @@ -602,6 +624,40 @@ csim_lock_ready (MMBaseModem *self, > > } > > > > static void > > +handle_csim_locking (LoadUnlockRetriesContext *ctx, gboolean is_lock) > > +{ > > + switch (ctx->self->priv->csim_lock_support) { > > + case FEATURE_SUPPORT_UNKNOWN: > > + case FEATURE_SUPPORTED: > > + if (is_lock) { > > + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self), > > + CSIM_LOCK_STR, > > + CSIM_QUERY_TIMEOUT, > > + FALSE, > > + (GAsyncReadyCallback) > csim_lock_ready, > > + ctx); > > + } else { > > + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self), > > + CSIM_UNLOCK_STR, > > + CSIM_QUERY_TIMEOUT, > > + FALSE, > > + (GAsyncReadyCallback) > csim_unlock_ready, > > + ctx); > > + } > > + break; > > + case FEATURE_NOT_SUPPORTED: > > + mm_dbg ("CSIM lock not supported by this modem. Skipping %s > command", > > + is_lock ? "lock" : "unlock"); > > + ctx->step++; > > + load_unlock_retries_step (ctx); > > + break; > > + default: > > + g_assert_not_reached (); > > + break; > > + } > > +} > > + > > +static void > > load_unlock_retries_step (LoadUnlockRetriesContext *ctx) > > { > > switch (ctx->step) { > > @@ -609,12 +665,7 @@ load_unlock_retries_step (LoadUnlockRetriesContext > *ctx) > > /* Fall back on next step */ > > ctx->step++; > > case LOAD_UNLOCK_RETRIES_STEP_LOCK: > > - mm_base_modem_at_command (MM_BASE_MODEM (ctx->self), > > - CSIM_LOCK_STR, > > - CSIM_QUERY_TIMEOUT, > > - FALSE, > > - (GAsyncReadyCallback) > csim_lock_ready, > > - ctx); > > + handle_csim_locking (ctx, TRUE); > > break; > > case LOAD_UNLOCK_RETRIES_STEP_PIN: > > mm_base_modem_at_command (MM_BASE_MODEM (ctx->self), > > @@ -649,12 +700,7 @@ load_unlock_retries_step (LoadUnlockRetriesContext > *ctx) > > ctx); > > break; > > case LOAD_UNLOCK_RETRIES_STEP_UNLOCK: > > - mm_base_modem_at_command (MM_BASE_MODEM (ctx->self), > > - CSIM_UNLOCK_STR, > > - CSIM_QUERY_TIMEOUT, > > - FALSE, > > - (GAsyncReadyCallback) > csim_unlock_ready, > > - ctx); > > + handle_csim_locking (ctx, FALSE); > > break; > > case LOAD_UNLOCK_RETRIES_STEP_LAST: > > if (ctx->succeded_requests == 0) { > > @@ -1220,6 +1266,11 @@ mm_broadband_modem_telit_new (const gchar *device, > > static void > > mm_broadband_modem_telit_init (MMBroadbandModemTelit *self) > > { > > + self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, > > + > MM_TYPE_BROADBAND_MODEM_TELIT, > > + > MMBroadbandModemTelitPrivate); > > + > > + self->priv->csim_lock_support = FEATURE_SUPPORT_UNKNOWN; > > } > > > > static void > > @@ -1263,4 +1314,7 @@ iface_modem_3gpp_init (MMIfaceModem3gpp *iface) > > static void > > mm_broadband_modem_telit_class_init (MMBroadbandModemTelitClass *klass) > > { > > + GObjectClass *object_class = G_OBJECT_CLASS (klass); > > + > > + g_type_class_add_private (object_class, sizeof > (MMBroadbandModemTelitPrivate)); > > } > > diff --git a/plugins/telit/mm-broadband-modem-telit.h > b/plugins/telit/mm-broadband-modem-telit.h > > index 50e6365..f68465e 100644 > > --- a/plugins/telit/mm-broadband-modem-telit.h > > +++ b/plugins/telit/mm-broadband-modem-telit.h > > @@ -29,9 +29,11 @@ > > > > typedef struct _MMBroadbandModemTelit MMBroadbandModemTelit; > > typedef struct _MMBroadbandModemTelitClass MMBroadbandModemTelitClass; > > +typedef struct _MMBroadbandModemTelitPrivate > MMBroadbandModemTelitPrivate; > > > > struct _MMBroadbandModemTelit { > > MMBroadbandModem parent; > > + MMBroadbandModemTelitPrivate *priv; > > }; > > > > struct _MMBroadbandModemTelitClass{ > > > > > -- > Aleksander > https://aleksander.es >
_______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
