[PATCH] telit: manage QSS transistions
Aleksander Morgado
aleksander at aleksander.es
Sat May 27 17:09:22 UTC 2017
On 26/05/17 11:36, Carlo Lobrano wrote:
> Currently, Telit's SIM swap implementation is stateless and
> based on #QSS unsolicited messages 0/1 (SIM_REMOVED/SIM_INSERTED).
>
> However, the user might have configured the modem in order to provide a
> more detailed information, with #QSS values 2/3 (SIM UNLOCKED/SIM READY).
>
> In this case and with current implementation, even receiving "#QSS: 3" will
> trigger the "SIM swap" logic. The same issue might occur in other use cases
> too, i.e. with SIM locked or when the message is received from both USB
> ports.
>
> This patch makes SIM swap implementation stateful, and it considers as an
> actual SIM swap, only transitions from #QSS: 0 to #QSS: 1/2/3 and vice
> versa.
Nice set of changes, totally make sense. See some minor things below.
> ---
> plugins/telit/mm-broadband-modem-telit.c | 162 ++++++++++++++++++++++++-------
> 1 file changed, 125 insertions(+), 37 deletions(-)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c b/plugins/telit/mm-broadband-modem-telit.c
> index 13ca4a5..4dde862 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -48,8 +48,16 @@ typedef enum {
> FEATURE_SUPPORTED
> } FeatureSupport;
>
> +typedef enum {
> + QSS_STATUS_SIM_REMOVED,
> + QSS_STATUS_SIM_INSERTED,
> + QSS_STATUS_SIM_INSERTED_AND_UNLOCKED,
> + QSS_STATUS_SIM_INSERTED_AND_READY
> +} QssStatus;
> +
> struct _MMBroadbandModemTelitPrivate {
> FeatureSupport csim_lock_support;
> + QssStatus qss_status;
> };
>
> /*****************************************************************************/
> @@ -90,17 +98,34 @@ modem_after_sim_unlock (MMIfaceModem *self,
> /*****************************************************************************/
> /* Setup SIM hot swap (Modem interface) */
>
> +typedef enum {
> + QSS_SETUP_STEP_FIRST,
> + QSS_SETUP_STEP_QUERY,
> + QSS_SETUP_STEP_SET_HANDLER,
> + QSS_SETUP_STEP_LAST
> +} QssSetupStep;
> +
> +typedef struct {
> + MMBroadbandModemTelit *self;
> + GSimpleAsyncResult *result;
> + QssSetupStep step;
> +} QssSetupContext;
> +
We're trying to move away from GSimpleAsyncResult, how about using
GTask? If you have any doubt or not sure how to do it, leave it with
GSimpleAsyncResult and I'll do the port to GTask as a follow up patch,
so that you can see what the changes were; it really is very easy, you
can check Ben's last emails doing lots of these GTask ports.
> +
> +static void qss_setup_step (QssSetupContext *ctx);
> +
> static void
> telit_qss_unsolicited_handler (MMPortSerialAt *port,
> GMatchInfo *match_info,
> MMBroadbandModemTelit *self)
> {
> - guint qss;
> + QssStatus cur_qss_status;
> + QssStatus prev_qss_status;
>
> - if (!mm_get_uint_from_match_info (match_info, 1, &qss))
> + if (!mm_get_uint_from_match_info (match_info, 1, &cur_qss_status))
> return;
>
> - switch (qss) {
> + switch (cur_qss_status) {
> case 0:
> mm_info ("QSS: SIM removed");
> break;
> @@ -111,28 +136,30 @@ telit_qss_unsolicited_handler (MMPortSerialAt *port,
> mm_info ("QSS: SIM inserted and PIN unlocked");
> break;
> case 3:
> - mm_info ("QSS: SIM inserted and PIN locked");
> + mm_info ("QSS: SIM inserted and ready");
> break;
> default:
> - mm_warn ("QSS: unknown QSS value %d", qss);
> + mm_warn ("QSS: unknown QSS value %d", cur_qss_status);
> break;
> }
>
> - mm_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM (self));
> -}
> + prev_qss_status = self->priv->qss_status;
> + self->priv->qss_status = cur_qss_status;
>
> -typedef struct {
> - MMBroadbandModemTelit *self;
> - GSimpleAsyncResult *result;
> -} ToggleQssUnsolicitedContext;
> + if ((prev_qss_status == QSS_STATUS_SIM_REMOVED && cur_qss_status != QSS_STATUS_SIM_REMOVED) ||
> + (prev_qss_status > QSS_STATUS_SIM_REMOVED && cur_qss_status == QSS_STATUS_SIM_REMOVED)) {
> + mm_dbg ("QSS: SIM swapped (QSS status changed %d -> %d)", prev_qss_status, cur_qss_status);
> + mm_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM (self));
> + }
> +}
>
> static void
> -toggle_qss_unsolicited_context_complete_and_free (ToggleQssUnsolicitedContext *ctx)
> +toggle_qss_unsolicited_context_complete_and_free (QssSetupContext *ctx)
> {
> g_simple_async_result_complete (ctx->result);
> g_object_unref (ctx->result);
> g_object_unref (ctx->self);
> - g_slice_free (ToggleQssUnsolicitedContext, ctx);
> + g_slice_free (QssSetupContext, ctx);
> }
>
> static gboolean
> @@ -144,19 +171,50 @@ modem_setup_sim_hot_swap_finish (MMIfaceModem *self,
> }
>
> static void
> -telit_qss_toggle_ready (MMBaseModem *self,
> +telit_qss_query_ready (MMBaseModem *self,
> + GAsyncResult *res,
> + QssSetupContext *ctx)
> +{
> + GError *error = NULL;
> + const gchar *response;
> + const gchar *str;
> + QssStatus qss_status;
> + guint qss_mode;
> +
> + response = mm_base_modem_at_command_finish (self, res, &error);
> + if (error) {
> + mm_warn ("Could not get \"#QSS?\" reply.");
> + g_error_free (error);
> + goto next_step;
> + }
> +
> + str = mm_strip_tag (response, "#QSS: ");
> + if (!sscanf (str, "%d,%d", &qss_mode, (guint*)&qss_status)) {
> + mm_err ("Could not parse \"#QSS?\" response: %s", response);
I'd suggest to have a helper method that does the parsing, and provide a
unit test for the parsing with the strings you're currently getting as
response. It will be easier to catch errors in the future if we ever
have to update the parser to because the format changed for some newer
modems. Maybe even with a regex, even if this one seems every simple.
> + goto next_step;
> + }
> +
> + ctx->self->priv->qss_status = qss_status;
> +
> +next_step:
> + ctx->step++;
> + qss_setup_step (ctx);
> +}
> +
> +static void
> +telit_qss_enable_ready (MMBaseModem *self,
> GAsyncResult *res,
> - ToggleQssUnsolicitedContext *ctx)
> + QssSetupContext *ctx)
> {
> GError *error = NULL;
>
> mm_base_modem_at_command_finish (self, res, &error);
> if (error) {
> - mm_warn ("Enable QSS failed: %s", error->message);
> g_simple_async_result_set_error (ctx->result,
> MM_CORE_ERROR,
> MM_CORE_ERROR_FAILED,
> - "Could not enable QSS");
> + "Could not enable QSS unsolicited: %s",
> + error->message);
> } else {
> MMPortSerialAt *primary;
> MMPortSerialAt *secondary;
> @@ -186,8 +244,8 @@ telit_qss_toggle_ready (MMBaseModem *self,
> g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
>
> }
> -
> - toggle_qss_unsolicited_context_complete_and_free (ctx);
> + ctx->step++;
> + qss_setup_step (ctx);
> }
>
>
> @@ -196,31 +254,60 @@ modem_setup_sim_hot_swap (MMIfaceModem *self,
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> - ToggleQssUnsolicitedContext *ctx;
> - MMPortSerialAt *port;
> -
> - mm_dbg ("Telit SIM hot swap: Enable QSS");
> + QssSetupContext *ctx;
>
> - ctx = g_slice_new0 (ToggleQssUnsolicitedContext);
> + ctx = g_slice_new0 (QssSetupContext);
> ctx->self = g_object_ref (MM_BROADBAND_MODEM_TELIT (self));
> ctx->result = g_simple_async_result_new (G_OBJECT (self),
> callback,
> user_data,
> modem_setup_sim_hot_swap);
> + ctx->step = QSS_SETUP_STEP_FIRST;
> + qss_setup_step (ctx);
> +}
> +
>
> - port = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (self));
> - if (!port)
> - port = mm_base_modem_peek_port_primary (MM_BASE_MODEM (self));
> -
> - mm_base_modem_at_command_full (MM_BASE_MODEM (self),
> - port,
> - "#QSS=1",
> - 3,
> - FALSE,
> - FALSE, /* raw */
> - NULL, /* cancellable */
> - (GAsyncReadyCallback) telit_qss_toggle_ready,
> - ctx);
> +
> +static void
> +qss_setup_step (QssSetupContext *ctx)
> +{
> + switch (ctx->step) {
> + case QSS_SETUP_STEP_FIRST:
> + /* Fall back on next step */
> + ctx->step++;
> + case QSS_SETUP_STEP_QUERY:
> + mm_base_modem_at_command (MM_BASE_MODEM (ctx->self),
> + "#QSS?",
> + 3,
> + FALSE,
> + (GAsyncReadyCallback) telit_qss_query_ready,
> + ctx);
> + break;
> + case QSS_SETUP_STEP_SET_HANDLER:
> + {
> + MMPortSerialAt *port;
Whiteline here, always after the variable definitions.
> + port = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (ctx->self));
> + if (!port)
> + port = mm_base_modem_peek_port_primary (MM_BASE_MODEM (ctx->self));
> +
We usually set unsolicited messages in both primary and secondary
always, and given that now you've implemented stateful transitions for
QSS, we could have them in both. What do you think?
> + mm_base_modem_at_command_full (MM_BASE_MODEM (ctx->self),
> + port,
> + "#QSS=1",
> + 3,
> + FALSE,
> + FALSE, /* raw */
> + NULL, /* cancellable */
> + (GAsyncReadyCallback) telit_qss_enable_ready,
> + ctx);
> + break;
> + }
> + case QSS_SETUP_STEP_LAST:
> + toggle_qss_unsolicited_context_complete_and_free (ctx);
> + break;
> + default:
> + break;
> +
> + }
> }
>
> /*****************************************************************************/
> @@ -1283,6 +1370,7 @@ mm_broadband_modem_telit_init (MMBroadbandModemTelit *self)
> MMBroadbandModemTelitPrivate);
>
> self->priv->csim_lock_support = FEATURE_SUPPORT_UNKNOWN;
> + self->priv->qss_status = QSS_STATUS_SIM_INSERTED;
I'd suggest to have an extra enum value saying QSS_STATUS_SIM_UNKNOWN
and initialize the variable to that one. We'll populate the correct
value during QSS_SETUP_STEP_QUERY anyway.
> }
>
> static void
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list