<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Hi, </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">thank you for the quick and useful reply.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"> </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">I will submit a new patch with the changes you suggested</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"> </div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Best regards, Carlo</div></div><div class="gmail_extra"> <div class="gmail_quote">On 14 December 2015 at 15:19, 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">Hey! See comments below. On Mon, Dec 14, 2015 at 12:51 PM, Carlo Lobrano <<a href="mailto:c.lobrano@gmail.com">c.lobrano@gmail.com</a>> wrote: > --- > plugins/telit/mm-broadband-modem-telit.c | 169 +++++++++++++++++++++++++++++++ > plugins/telit/mm-broadband-modem-telit.h | 2 + > 2 files changed, 171 insertions(+) > > diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c > index 0f77231..b34503d 100644 > --- a/plugins/telit/mm-broadband-modem-telit.c > +++ b/plugins/telit/mm-broadband-modem-telit.c > @@ -39,6 +39,165 @@ 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 struct { > + gint pin; > + gint pin2; > + gint puk; > + gint puk2; > +} ModemLockRetries; > + > +struct _MMBroadbandModemTelitPrivate { > + ModemLockRetries mm_lock_retries; > +}; > + I would say that the context of a single operation shouldn't be stored in private info, which is common to all operations. I see that what you want to do is to be able to build the MMUnlockRetries object in the finish() function, using the values you cached in the private info. Instead of doing that, I'd instead create a context struct for the action like this: typedef struct { MMBaseModemTelit *self; GSimpleAsyncResult *result; ModemLockRetries *retries; } LoadUnlockRetriesContext; You would do this in the modem_load_unlock_retries() and pass it to the sequence handler method (as response_processor_context, so that you can use it in the response processor methods). Then, for each of the responses, you could update ctx->retries directly, and get it returned in finish() afterwards. Another option would be to create the ModemLockRetries object and pass it directly in response_processor_context, while passing the GSimpleAsyncResult in the sequence method user_data (e.g. see modem_3gpp_disable_unsolicited_events() in mm-broadband-modem-mtk.c for an example). <div><div class="h5"> > + > +/*****************************************************************************/ > +/* Load unlock retries (Modem interface) */ > + > +#define CSIM_QUERY_PIN_RETRIES_STR "+CSIM=10,0020000100" > +#define CSIM_QUERY_PUK_RETRIES_STR "+CSIM=10,002C000100" > +#define CSIM_QUERY_PIN2_RETRIES_STR "+CSIM=10,0020008100" > +#define CSIM_QUERY_PUK2_RETRIES_STR "+CSIM=10,002C008100" > + > +static gboolean > +csim_query_response_processor (MMBaseModem *_self, > + gpointer none, > + const gchar *command, > + const gchar *response, > + gboolean last_command, > + const GError *error, > + GVariant **result, > + GError **result_error) > +{ > + GRegex *r = NULL; > + GMatchInfo *match_info = NULL; > + > + r = g_regex_new ("\\+CSIM:\\s*.*63C(.*)\"", G_REGEX_RAW, 0, NULL); > + g_assert (NULL != r); > + </div></div>For the regex parsers we try to write them in a separate -helpers.[c|h] set of files, and write unit tests for them, so that if someone updates the parsers in a the future we can validate that they don't break the previous strings. Could you do that as well? (see e.g. Huawei plugin) > + if (!g_regex_match (r, response, 0, &match_info)) { > + mm_err ("Could not parse +CSIM response '%s'", response); > + goto end; > + } > + > + if (!g_match_info_matches (match_info)) { > + mm_err ("Could not get SIM unlock retries for query '%s'. Response was '%s'", > + command, > + response); > + goto end; > + } else { > + MMBroadbandModemTelit *self = MM_BROADBAND_MODEM_TELIT (_self); > + gchar* retries_hex_str; > + guint retries; > + > + retries_hex_str = mm_get_string_unquoted_from_match_info (match_info, 1); > + g_assert (NULL != retries_hex_str); > + > + // convert hex value to uint We follow GNOME coding style, so we usually put comments /* like this */, no one-line comments with //. ;) <div><div class="h5"> > + sscanf (retries_hex_str, "%x", &retries); > + mm_dbg ("%s: CSIM retries 0x%s => %d", command, retries_hex_str, retries); > + > + g_free (retries_hex_str); > + > + if(strstr(CSIM_QUERY_PIN_RETRIES_STR, command)) { > + self->priv->mm_lock_retries.pin = retries; > + } else if(strstr(CSIM_QUERY_PIN2_RETRIES_STR, command)) { > + self->priv->mm_lock_retries.pin2 = retries; > + } else if(strstr(CSIM_QUERY_PUK_RETRIES_STR, command)) { > + self->priv->mm_lock_retries.puk = retries; > + } else if(strstr(CSIM_QUERY_PUK2_RETRIES_STR, command)) { > + self->priv->mm_lock_retries.puk2 = retries; > + } else { > + mm_err ("Could not assign SIM unlock retry value: unrecognized command '%s'", > + command); > + } > + } > + > +end: > + g_match_info_free (match_info); > + g_regex_unref (r); > + > + return FALSE; > +} > + > +static const MMBaseModemAtCommand load_unlock_retries_sequence[] = { > + { CSIM_QUERY_PIN_RETRIES_STR, 10, FALSE, csim_query_response_processor }, > + { CSIM_QUERY_PIN2_RETRIES_STR, 10, FALSE, csim_query_response_processor }, > + { CSIM_QUERY_PUK_RETRIES_STR, 10, FALSE, csim_query_response_processor }, > + { CSIM_QUERY_PUK2_RETRIES_STR, 10, FALSE, csim_query_response_processor }, > + { NULL } > +}; > + > +static MMUnlockRetries * > +modem_load_unlock_retries_finish (MMIfaceModem *_self, > + GAsyncResult *res, > + GError **error) > +{ > + MMBroadbandModemTelit *self = MM_BROADBAND_MODEM_TELIT (_self); > + MMUnlockRetries* unlock_retries; > + > + mm_dbg("%s", __FUNCTION__); > + </div></div>No mm_dbg() with the function name please. Also, Looks like you're not handling errors in this finish() call. What if the response processor returned an error? This logic is also mixing two things. When you call mm_base_modem_at_sequence_full() there *must* be a call to mm_base_modem_at_sequence_full_finish() to get the action completed. If you pass the original callback and user_data in modem_load_unlock_retries() it means that you must call mm_base_modem_at_sequence_full_finish() in this modem_load_unlock_retries_finish() method to finish that operation. NOTE that you can only do this (passing original callback directly) because both mm_base_modem_at_sequence_full() and modem_load_unlock_retries_finish() take the modem object as first parameter (i.e. they are both actions on the same modem object). In other words, you must never do this if you're using SIM or Bearer objects (this is just some context, nothing to do with your patch). I'm personally not very happy with the at_sequence methods, they're quick to write but the logic behind is a bit hard to get... If you're more comfortable with some other logic not using the sequence, e.g. using a Context and a step enum (see e.g. load_access_technologies() in mm-broadband-modem-option.c), then that would be also good to have. > + > + unlock_retries = mm_unlock_retries_new (); > + if (self->priv->mm_lock_retries.pin > 0) { > + mm_dbg("SIM PIN retries left:%d",self->priv->mm_lock_retries.pin); > + mm_unlock_retries_set (unlock_retries, MM_MODEM_LOCK_SIM_PIN, > + self->priv->mm_lock_retries.pin); > + } else { If retries.pin == 0, shouldn't we better say SIM PIN retries left: 0? why UNKNOWN? i.e. shouldn't it be >= 0? <div><div class="h5"> > + mm_dbg("SIM PIN retries left: UNKNOWN"); > + } > + > + if (self->priv->mm_lock_retries.pin2 > 0) { > + mm_dbg("SIM PIN2 retries left:%d",self->priv->mm_lock_retries.pin2); > + mm_unlock_retries_set (unlock_retries, MM_MODEM_LOCK_SIM_PIN2, > + self->priv->mm_lock_retries.pin2); > + } else { > + mm_dbg("SIM PIN2 retries left: UNKNOWN"); > + } > + > + if (self->priv->mm_lock_retries.puk > 0) { > + mm_dbg("SIM PUK retries left:%d",self->priv->mm_lock_retries.puk); > + mm_unlock_retries_set (unlock_retries, MM_MODEM_LOCK_SIM_PUK, > + self->priv->mm_lock_retries.puk); > + } else { > + mm_dbg("SIM PUK retries left: UNKNOWN"); > + } > + > + if (self->priv->mm_lock_retries.puk2 > 0) { > + mm_dbg("SIM PUK2 retries left:%d",self->priv->mm_lock_retries.puk2); > + mm_unlock_retries_set (unlock_retries, MM_MODEM_LOCK_SIM_PUK2, > + self->priv->mm_lock_retries.puk2); > + } else { > + mm_dbg("SIM PUK2 retries left: UNKNOWN"); > + } > + > + return unlock_retries; > +} > + > +static void > +modem_load_unlock_retries (MMIfaceModem *self, > + GAsyncReadyCallback callback, > + gpointer user_data) > +{ > + GSimpleAsyncResult *result; > + > + mm_dbg("%s", __FUNCTION__); > + > + result = g_simple_async_result_new (G_OBJECT (self), > + callback, > + user_data, > + modem_load_unlock_retries); > + > + mm_base_modem_at_sequence ( > + MM_BASE_MODEM (self), > + load_unlock_retries_sequence, > + NULL, /* response_processor_context */ > + NULL, /* response_processor_context_free */ > + callback, > + user_data); > +} > + > /*****************************************************************************/ > /* Modem power down (Modem interface) */ > > @@ -331,11 +490,21 @@ mm_broadband_modem_telit_new (const gchar *device, > static void > mm_broadband_modem_telit_init (MMBroadbandModemTelit *self) > { > + /* Initialize private data */ > + self->priv = G_TYPE_INSTANCE_GET_PRIVATE (self, > + MM_TYPE_BROADBAND_MODEM_TELIT, > + MMBroadbandModemTelitPrivate); > + self->priv->mm_lock_retries.pin = -1; > + self->priv->mm_lock_retries.pin2 = -1; > + self->priv->mm_lock_retries.puk = -1; > + self->priv->mm_lock_retries.puk2 = -1; > } > > static void > iface_modem_init (MMIfaceModem *iface) > { > + iface->load_unlock_retries_finish = modem_load_unlock_retries_finish; > + iface->load_unlock_retries = modem_load_unlock_retries; > iface->reset = modem_reset; > iface->reset_finish = modem_reset_finish; > iface->modem_power_down = modem_power_down; > diff --git a/plugins/telit/mm-broadband-modem-telit.h b/plugins/telit/mm-broadband-modem-telit.h > index 50e6365..9b32ec6 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{ > -- > 2.1.4 > </div></div>> _______________________________________________ > ModemManager-devel mailing list > <a href="mailto:ModemManager-devel@lists.freedesktop.org">ModemManager-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel</a> -- Aleksander <a href="https://aleksander.es" rel="noreferrer" target="_blank">https://aleksander.es</a> </blockquote></div> </div>