[PATCH] Telit plugin: add load_unlock_retries interface
Carlo Lobrano
c.lobrano at gmail.com
Mon Dec 14 07:09:36 PST 2015
Hi,
thank you for the quick and useful reply.
I will submit a new patch with the changes you suggested
Best regards,
Carlo
On 14 December 2015 at 15:19, Aleksander Morgado <aleksander at aleksander.es>
wrote:
> Hey!
>
> See comments below.
>
> On Mon, Dec 14, 2015 at 12:51 PM, Carlo Lobrano <c.lobrano at gmail.com>
> 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).
>
>
> > +
> >
> +/*****************************************************************************/
> > +/* 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);
> > +
>
> 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 //. ;)
>
> > + 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__);
> > +
>
> 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?
>
> > + 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
> >
> > _______________________________________________
> > ModemManager-devel mailing list
> > ModemManager-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
>
>
>
> --
> Aleksander
> https://aleksander.es
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/modemmanager-devel/attachments/20151214/cf671987/attachment-0001.html>
More information about the ModemManager-devel
mailing list