<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">> Is this previous part really needed? Is there any case in which a LOCK</div>command would succeed but then UNLOCK command report a "not supported" error? I guess it doesn't harm to have this, though, your call.<div> </div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">Right, there is no real need for this, but just for completeness I prefer to keep it, if you don't mind</div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"></div></div><div>><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"> </div>Looks like csim_lock_support isn't set anywhere to FEATURE_SUPPORTED? It</div><div>should be set here I believe.<div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"></div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline"> </div></div><div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif;display:inline">Doh! I'll fix it :D</div></div></div><div class="gmail_extra"> <div class="gmail_quote">On 10 April 2017 at 13:25, Aleksander Morgado <<a href="mailto:aleksander@aleksander.es" target="_blank">aleksander@aleksander.es</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. <div><div class="h5"> > 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; > + } </div></div>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. <div><div class="h5"> > > 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{ > -- </div></div>Aleksander <a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a> </blockquote></div> </div>