[PATCH] Telit plugin: add load_unlock_retries interface
Aleksander Morgado
aleksander at aleksander.es
Mon Dec 14 06:19:01 PST 2015
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
More information about the ModemManager-devel
mailing list