Fwd: cinterion: modification to fetching unlock retries

[Aleksander Morgado]

On 30/12/17 15:23, Colin Helliwell wrote:
> plugins/cinterion/mm-broadband-modem-cinterion.c | 92 ++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 15 deletions(-)
> 
> diff --git a/plugins/cinterion/mm-broadband-modem-cinterion.c b/plugins/cinterion/mm-broadband-modem-cinterion.c
> index 64c5e08..aa23bb1 100644
> --- a/plugins/cinterion/mm-broadband-modem-cinterion.c
> +++ b/plugins/cinterion/mm-broadband-modem-cinterion.c
> @@ -83,6 +83,7 @@ struct _MMBroadbandModemCinterionPrivate {
>      /* Flags for feature support checks */
>      FeatureSupport swwan_support;
>      FeatureSupport sind_psinfo_support;
> +    FeatureSupport spic_pincounter_support;
>  };
>  
>  /*****************************************************************************/
> @@ -1442,6 +1443,7 @@ typedef struct {
>  } UnlockRetriesMap;
>  
>  static const UnlockRetriesMap unlock_retries_map [] = {
> +    { MM_MODEM_LOCK_NONE,        "^SPIC=?"   },
>      { MM_MODEM_LOCK_SIM_PIN,     "^SPIC=\"SC\""   },
>      { MM_MODEM_LOCK_SIM_PUK,     "^SPIC=\"SC\",1" },
>      { MM_MODEM_LOCK_SIM_PIN2,    "^SPIC=\"P2\""   },
> @@ -1470,6 +1472,25 @@ load_unlock_retries_finish (MMIfaceModem  *self,
>  static void load_unlock_retries_context_step (GTask *task);
>  
>  static void
> +parent_load_unlock_retries_ready (MMIfaceModem *self,
> +                                  GAsyncResult *res,
> +                                  GTask *task)
> +{
> +    GError *error = NULL;
> +    LoadUnlockRetriesContext *ctx;
> +
> +    ctx = g_task_get_task_data (task);
> +
> +    ctx->retries = iface_modem_parent->load_unlock_retries_finish (self, res, &error);
> +    if (error)
> +        g_task_return_error (task, error);
> +    else
> +        g_task_return_pointer (task, g_object_ref (ctx->retries), g_object_unref);
> +
> +    g_object_unref (task);
> +}
> +
> +static void
>  spic_ready (MMBaseModem  *self,
>              GAsyncResult *res,
>              GTask        *task)
> @@ -1481,20 +1502,38 @@ spic_ready (MMBaseModem  *self,
>      ctx = g_task_get_task_data (task);
>  
>      response = mm_base_modem_at_command_finish (self, res, &error);
> -    if (!response) {
> -        mm_dbg ("Couldn't load retry count for lock '%s': %s",
> -                mm_modem_lock_get_string (unlock_retries_map[ctx->i].lock),
> -                error->message);
> -        g_error_free (error);
> +    if (!ctx->i)
> +    {

I believe this is too complex, and it doesn't avoid re-calling SPIC=? check on further tests later on if it's found as UNSUPPORTED. Ideally, the logic should be something like:
  * Run load_unlock_retries
  * Is SPIC support unknown? (only during the first run will happen) Run SPIC=? and check result. If supported, flag as SUPPORTED, if not supported flag as UNSUPPORTED.
  * If SPIC is supported, run SPIC map.
  * If SPIC is unsupported, run parent logic.

The initial SPIC=? check should be completely dissasociated from the SPIC map so that we run it only once and change the support flag only once.

> +        /* The first step is just a check for support of ^SPIC */
> +        MMBroadbandModemCinterion *self_cint = MM_BROADBAND_MODEM_CINTERION (self);
> +        if (!response) {
> +            self_cint->priv->spic_pincounter_support = FEATURE_NOT_SUPPORTED;
> +            mm_warn ("^SPIC not supported: %s", error->message);
> +            g_error_free (error);
> +            /* Fallback to parent's method */
> +            iface_modem_parent->load_unlock_retries (
> +                MM_IFACE_MODEM (self),
> +                (GAsyncReadyCallback)parent_load_unlock_retries_ready,
> +                task);
> +            return;
> +        } else
> +            self_cint->priv->spic_pincounter_support = FEATURE_SUPPORTED;
>      } else {
> -        guint val;
> -
> -        response = mm_strip_tag (response, "^SPIC:");
> -        if (!mm_get_uint_from_str (response, &val))
> -            mm_dbg ("Couldn't parse retry count value for lock '%s'",
> -                    mm_modem_lock_get_string (unlock_retries_map[ctx->i].lock));
> -        else
> -            mm_unlock_retries_set (ctx->retries, unlock_retries_map[ctx->i].lock, val);
> +        if (!response) {
> +            mm_dbg ("Couldn't load retry count for lock '%s': %s",
> +                    mm_modem_lock_get_string (unlock_retries_map[ctx->i].lock),
> +                    error->message);
> +            g_error_free (error);
> +        } else {
> +            guint val;
> +
> +            response = mm_strip_tag (response, "^SPIC:");
> +            if (!mm_get_uint_from_str (response, &val))
> +                mm_dbg ("Couldn't parse retry count value for lock '%s'",
> +                        mm_modem_lock_get_string (unlock_retries_map[ctx->i].lock));
> +            else
> +                mm_unlock_retries_set (ctx->retries, unlock_retries_map[ctx->i].lock, val);
> +        }
>      }
>  
>      /* Go to next lock value */
> @@ -1812,8 +1871,9 @@ mm_broadband_modem_cinterion_init (MMBroadbandModemCinterion *self)
>                                                MMBroadbandModemCinterionPrivate);
>  
>      /* Initialize private variables */
> -    self->priv->sind_psinfo_support = FEATURE_SUPPORT_UNKNOWN;
> -    self->priv->swwan_support       = FEATURE_SUPPORT_UNKNOWN;
> +    self->priv->sind_psinfo_support     = FEATURE_SUPPORT_UNKNOWN;
> +    self->priv->spic_pincounter_support = FEATURE_SUPPORT_UNKNOWN;
> +    self->priv->swwan_support           = FEATURE_SUPPORT_UNKNOWN;
>  
>      self->priv->ciev_psinfo_regex = g_regex_new ("\\r\\n\\+CIEV: psinfo,(\\d+)\\r\\n",
>                                                   G_REGEX_RAW | G_REGEX_OPTIMIZE, 0, NULL);


-- 
Aleksander
https://aleksander.es