Hey Carlo,
On Wed, May 31, 2017 at 10:36 AM, Carlo Lobrano <c.lobr...@gmail.com> 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.
>
> ---
>
> Changes according to code review:
>
> - used GTask
> - added helper method for "#QSS?" parsing
> - added tests for "#QSS?" parser
> - enabled unsolicited on both ports
>
Thanks for the update! See some more comments below.
> ---
> plugins/telit/mm-broadband-modem-telit.c | 267
> +++++++++++++++-------
> plugins/telit/mm-modem-helpers-telit.c | 21 ++
> plugins/telit/mm-modem-helpers-telit.h | 11 +
> plugins/telit/tests/test-mm-modem-helpers-telit.c | 40 ++++
> 4 files changed, 261 insertions(+), 78 deletions(-)
>
> diff --git a/plugins/telit/mm-broadband-modem-telit.c
> b/plugins/telit/mm-broadband-modem-telit.c
> index 13ca4a5..b68ff6d 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -50,6 +50,7 @@ typedef enum {
>
> struct _MMBroadbandModemTelitPrivate {
> FeatureSupport csim_lock_support;
> + QssStatus qss_status;
> };
>
>
> /*****************************************************************************/
> @@ -90,49 +91,57 @@ 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_ENABLE_PRIMARY_PORT,
> + QSS_SETUP_STEP_ENABLE_SECONDARY_PORT,
> + QSS_SETUP_STEP_LAST
> +} QssSetupStep;
> +
> +static const gchar *qss_status_names [] = {
> + [QSS_STATUS_SIM_REMOVED] = "QSS_STATUS_SIM_REMOVED",
> + [QSS_STATUS_SIM_INSERTED] = "QSS_STATUS_SIM_INSERTED",
> + [QSS_STATUS_SIM_INSERTED_AND_UNLOCKED] =
> "QSS_STATUS_SIM_INSERTED_AND_UNLOCKED",
> + [QSS_STATUS_SIM_INSERTED_AND_READY] =
> "QSS_STATUS_SIM_INSERTED_AND_READY",
> +};
How about setting up glib-mkenums based generation for this enum
defined in the telit helpers file? We do that already for enums
defined in the u-blox plugin (see plugins/Makefile.am and look for the
mkenums generation), which you could use as reference. That would let
you do e.g. "mm_telit_qss_status_get_string()" and forget about
defining the array of strings yourself (and would also allow negative
enum values, e.g. for UNKNOWN as suggested later on).
> +
> +typedef struct {
> + MMBroadbandModemTelit *self;
This 'self' pointer here is not used anywhere.
> + QssSetupStep step;
> +} QssSetupContext;
> +
> +static void qss_setup_step (GTask *task);
> +
> 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) {
> - case 0:
> - mm_info ("QSS: SIM removed");
> - break;
> - case 1:
> - mm_info ("QSS: SIM inserted");
> - break;
> - case 2:
> - mm_info ("QSS: SIM inserted and PIN unlocked");
> - break;
> - case 3:
> - mm_info ("QSS: SIM inserted and PIN locked");
> - break;
> - default:
> - mm_warn ("QSS: unknown QSS value %d", qss);
> - break;
> - }
> + prev_qss_status = self->priv->qss_status;
> + self->priv->qss_status = cur_qss_status;
>
> - mm_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM
> (self));
> -}
> + mm_dbg ("QSS: got '%s' status (previously it was '%s')",
> + qss_status_names[cur_qss_status],
> + qss_status_names[prev_qss_status]);
How about renaming the message like:
if (cur_qss_status != prev_qss_status)
mm_dbg ("QSS: status changed '%s -> %s",
qss_status_names[prev_qss_status], qss_status_names[cur_qss_status]);
Also, qss_status_names[UNKNOWN] isn't defined, what would be printed
in that case, e.g. during the initial status change? If we do the
glib-mkenums change we wouldn't need to worry about that, as each enum
always has an associated string.
>
> -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_info ("QSS: SIM swap detected");
> + mm_broadband_modem_update_sim_hot_swap_detected (MM_BROADBAND_MODEM
> (self));
> + }
> +}
>
> static void
> -toggle_qss_unsolicited_context_complete_and_free
> (ToggleQssUnsolicitedContext *ctx)
> +qss_setup_context_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
> @@ -140,54 +149,104 @@ modem_setup_sim_hot_swap_finish (MMIfaceModem *self,
> GAsyncResult *res,
> GError **error)
> {
> - return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT
> (res), error);
> + return g_task_propagate_boolean (G_TASK (res), error);
> +}
> +
> +static void
> +telit_qss_query_ready (MMBaseModem *modem,
> + GAsyncResult *res,
> + GTask *task)
> +{
> + MMBroadbandModemTelit *self;
> + GError *error = NULL;
> + const gchar *response;
> + QssStatus qss_status;
> + QssSetupContext *ctx;
> +
> + self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));
> + ctx = g_task_get_task_data (task);
> +
> + response = mm_base_modem_at_command_finish (modem, res, &error);
> + if (error) {
> + mm_warn ("Could not get \"#QSS?\" reply.");
How about adding the actual error->message we get in the warning?
> + g_error_free (error);
> + goto next_step;
> + }
> +
> + qss_status = mm_telit_parse_qss_query (response, &error);
> + if (error) {
> + mm_err ("QSS query parse error: %s", error->message);
mm_warn()
> + g_error_free (error);
> + goto next_step;
> + }
> +
> + mm_info ("QSS: current status is '%s'", qss_status_names [qss_status]);
> + self->priv->qss_status = qss_status;
> +
> +next_step:
> + ctx->step++;
> + qss_setup_step (task);
> }
>
> static void
> -telit_qss_toggle_ready (MMBaseModem *self,
> +telit_qss_enable_ready (MMBaseModem *modem,
> GAsyncResult *res,
> - ToggleQssUnsolicitedContext *ctx)
> + GTask *task)
> {
> GError *error = NULL;
> + MMBroadbandModemTelit *self;
> + QssSetupContext *ctx;
> + MMPortSerialAt *primary;
> + MMPortSerialAt *secondary;
> + GRegex *pattern;
> +
> + self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));
> + ctx = g_task_get_task_data (task);
>
> - mm_base_modem_at_command_finish (self, res, &error);
> + mm_base_modem_at_command_finish (modem, 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");
> - } else {
> - MMPortSerialAt *primary;
> - MMPortSerialAt *secondary;
> - GRegex *pattern;
> + g_task_return_new_error (task,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Could not enable QSS unsolicited: %s",
> + error->message);
> + goto next_step;
There is no going to any other step after calling
"g_task_return_XXX()". If you return an error here, the whole async
method will be failed, and you should actually unref the task just
after the return_new_error() call.
If you want to go on even if you get an error, you can just mm_warn()
the error message, free the error and go on.
In this very specific case, where the telit_qss_enable_ready() method
is used for both the primary and the secondary port, I would
completely fail the async method only if an error was received in both
ports (if we have two ports) or if the error is received in the
primary port and there is no secondary port. To do this, you could
store in the Context e.g. a GError primary_error and a GError
secondary_error, and in QSS_SETUP_STEP_LAST decide whether you want to
return an error or otherwise TRUE if at least one port was configured
with QSS enabled. If you do this, don't forget to g_clear_error() both
GErrors in the context free method.
E.g.
if (error) {
if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_PORT)
ctx->primary_error = error;
else if (ctx->step == QSS_SETUP_STEP_ENABLE_SECONDARY_PORT)
ctx->secondary_error = error;
else
g_assert_not_reached();
}
> + }
>
> - pattern = g_regex_new ("#QSS:\\s*([0-3])\\r\\n", G_REGEX_RAW, 0,
> NULL);
> - g_assert (pattern);
> + pattern = g_regex_new ("#QSS:\\s*([0-3])\\r\\n", G_REGEX_RAW, 0, NULL);
> + g_assert (pattern);
>
> - primary = mm_base_modem_peek_port_primary (MM_BASE_MODEM
> (ctx->self));
> + if (ctx->step == QSS_SETUP_STEP_ENABLE_PRIMARY_PORT) {
> + mm_dbg ("QSS: setting handler at primary port");
> + primary = mm_base_modem_peek_port_primary (MM_BASE_MODEM (self));
> mm_port_serial_at_add_unsolicited_msg_handler (
> primary,
> pattern,
> (MMPortSerialAtUnsolicitedMsgFn)telit_qss_unsolicited_handler,
> - ctx->self,
> + self,
> NULL);
> + }
>
> - secondary = mm_base_modem_peek_port_secondary (MM_BASE_MODEM
> (ctx->self));
> - if (secondary)
> + if (ctx->step == QSS_SETUP_STEP_ENABLE_SECONDARY_PORT) {
> + mm_dbg ("QSS: setting handler at secondary port");
> + secondary = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (self));
> + if (secondary) {
> mm_port_serial_at_add_unsolicited_msg_handler (
> secondary,
> pattern,
>
> (MMPortSerialAtUnsolicitedMsgFn)telit_qss_unsolicited_handler,
> - ctx->self,
> + self,
> NULL);
> -
> - g_regex_unref (pattern);
> - g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> -
> + } else {
> + mm_warn ("QSS could not set handler hat secondary port: no
> secondary port found.");
> + }
> }
>
> - toggle_qss_unsolicited_context_complete_and_free (ctx);
> + g_regex_unref (pattern);
> +
> +next_step:
> + ctx->step++;
> + qss_setup_step (task);
> }
>
>
> @@ -196,31 +255,82 @@ modem_setup_sim_hot_swap (MMIfaceModem *self,
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> - ToggleQssUnsolicitedContext *ctx;
> - MMPortSerialAt *port;
> + QssSetupContext *ctx;
> + GTask *task;
>
> - mm_dbg ("Telit SIM hot swap: Enable QSS");
> + task = g_task_new (self, NULL, callback, user_data);
>
> - ctx = g_slice_new0 (ToggleQssUnsolicitedContext);
> - 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);
> -
> - 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);
> + ctx = g_slice_new0 (QssSetupContext);
> + ctx->step = QSS_SETUP_STEP_FIRST;
> +
> + g_task_set_task_data (task, ctx, (GDestroyNotify)
> qss_setup_context_free);
> + qss_setup_step (task);
> +}
> +
Just one white line here to separate methods, please.
> +
> +
> +static void
> +qss_setup_step (GTask *task)
> +{
Could you reorder this method and move it above
modem_setup_sim_hot_swap()? We usually do this to "read" the async
method flow from the bottom to the top of the source code.
> + QssSetupContext *ctx;
> + MMBroadbandModemTelit *self;
> +
> + self = MM_BROADBAND_MODEM_TELIT (g_task_get_source_object (task));
> + ctx = g_task_get_task_data (task);
> +
> + 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 (self),
> + "#QSS?",
> + 3,
> + FALSE,
> + (GAsyncReadyCallback)
> telit_qss_query_ready,
> + task);
> + return;
> + case QSS_SETUP_STEP_ENABLE_PRIMARY_PORT:
> + mm_base_modem_at_command_full (MM_BASE_MODEM (self),
> + mm_base_modem_peek_port_primary
> (MM_BASE_MODEM (self)),
> + "#QSS=1",
> + 3,
> + FALSE,
> + FALSE, /* raw */
> + NULL, /* cancellable */
> + (GAsyncReadyCallback)
> telit_qss_enable_ready,
> + task);
> + return;
> + case QSS_SETUP_STEP_ENABLE_SECONDARY_PORT:
> + {
The open braces should go at the end of the previous line.
> + MMPortSerialAt *port;
> +
> + port = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (self));
> + if (port) {
> + mm_base_modem_at_command_full (MM_BASE_MODEM (self),
> + port,
> + "#QSS=1",
> + 3,
> + FALSE,
> + FALSE, /* raw */
> + NULL, /* cancellable */
> + (GAsyncReadyCallback)
> telit_qss_enable_ready,
> + task);
> + return;
> + }
> +
> + /* Fall back to next step */
> + ctx->step++;
> + }
> + case QSS_SETUP_STEP_LAST:
> + g_task_return_boolean (task, TRUE);
> + g_object_unref (task);
> + break;
> + default:
> + g_assert_not_reached ();
> + break;
> +
The default block isn't even needed here, given that the switch()
covers all enum cases.
> + }
> }
>
>
> /*****************************************************************************/
> @@ -1283,6 +1393,7 @@ mm_broadband_modem_telit_init (MMBroadbandModemTelit
> *self)
> MMBroadbandModemTelitPrivate);
>
> self->priv->csim_lock_support = FEATURE_SUPPORT_UNKNOWN;
> + self->priv->qss_status = QSS_STATUS_UNKNOWN;
> }
>
> static void
> diff --git a/plugins/telit/mm-modem-helpers-telit.c
> b/plugins/telit/mm-modem-helpers-telit.c
> index 8353c45..95f783a 100644
> --- a/plugins/telit/mm-modem-helpers-telit.c
> +++ b/plugins/telit/mm-modem-helpers-telit.c
> @@ -587,3 +587,24 @@ mm_telit_get_band_flags_from_string (const gchar
> *flag_str,
>
> return TRUE;
> }
> +
> +/*****************************************************************************/
> +/* #QSS? response parser */
> +QssStatus
> +mm_telit_parse_qss_query (const gchar *response,
> + GError **error)
> +{
> + QssStatus qss_status;
> + guint qss_mode;
> +
> + qss_status = QSS_STATUS_UNKNOWN;
> +
> + if (!sscanf (response, "#QSS: %d,%d", &qss_mode, (guint*)&qss_status)) {
sscanf() returns "On success, these functions return the number of
input items successfully matched and assigned; this can be fewer than
provided for, or even zero, in the event of an early matching
failure."
You shouldn't check against a zero return value, because the parser
may also read the first value and return 1, and that would also be an
error. It isn't an error only if you read 2 values, so (sscanf() != 2)
Also, in this very specific case, I would make qss_status a gint
directly, and only convert it to an enum when returning it.
Plus, qss_mode should also be a gint (if you're using %d modifier in
sscanf() you're expecting a signed integer).
> + g_propagate_error (error,
> + g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED,
> + "Could not parse \"#QSS?\" response:
> %s",
> + response));
> + }
> +
> + return qss_status;
> +}
> diff --git a/plugins/telit/mm-modem-helpers-telit.h
> b/plugins/telit/mm-modem-helpers-telit.h
> index b693732..30fba62 100644
> --- a/plugins/telit/mm-modem-helpers-telit.h
> +++ b/plugins/telit/mm-modem-helpers-telit.h
> @@ -98,4 +98,15 @@ gboolean mm_telit_update_4g_bands(GArray** bands,
> GMatchInfo *match_info, GError
>
> void mm_telit_get_band_flag (GArray *bands_array, gint *flag_2g, gint
> *flag_3g, gint *flag_4g);
>
> +/* #QSS? response parser */
> +typedef enum {
> + QSS_STATUS_SIM_REMOVED,
> + QSS_STATUS_SIM_INSERTED,
> + QSS_STATUS_SIM_INSERTED_AND_UNLOCKED,
> + QSS_STATUS_SIM_INSERTED_AND_READY,
> + QSS_STATUS_UNKNOWN
I'd suggest UNKNOWN to be the first one in the enum. I see the
QssStatus enum values have a one to one relationship with the actual
values returned in #QSS, so you could make UNKNOWN=-1 for example. If
you do this, beware in the previous method because the 'qss_status'
variable shouldn't be initialized to UNKNOWN if it is a guint! (you
could do that if you make it a gint, or just return UNKNOWN on the
error detection block).
> +} QssStatus;
> +
Being an enum in a header, it should be "MMTelitQssStatus".
> +QssStatus mm_telit_parse_qss_query (const gchar *response, GError **error);
> +
> #endif /* MM_MODEM_HELPERS_TELIT_H */
> diff --git a/plugins/telit/tests/test-mm-modem-helpers-telit.c
> b/plugins/telit/tests/test-mm-modem-helpers-telit.c
> index 3008010..38b7baa 100644
> --- a/plugins/telit/tests/test-mm-modem-helpers-telit.c
> +++ b/plugins/telit/tests/test-mm-modem-helpers-telit.c
> @@ -502,6 +502,45 @@ test_telit_get_4g_bnd_flag (void)
> g_array_free (bands_array, TRUE);
> }
>
> +typedef struct {
> + const char* response;
> + QssStatus expected_qss;
> + const char* error_message;
const gchar *response;
(i.e. gchar for consistency and the asterisk next to the variable name)
> +} QssParseTest;
> +
> +static QssParseTest qss_parse_tests [] = {
> + {"#QSS: 0,0", QSS_STATUS_SIM_REMOVED, NULL},
> + {"#QSS: 0,0", QSS_STATUS_SIM_REMOVED, NULL},
Isn't this previous test a duplicate?
> + {"#QSS: 0,1", QSS_STATUS_SIM_INSERTED, NULL},
> + {"#QSS: 0,2", QSS_STATUS_SIM_INSERTED_AND_UNLOCKED, NULL},
> + {"#QSS: 0,3", QSS_STATUS_SIM_INSERTED_AND_READY, NULL},
> + {"#QSS:0,3", QSS_STATUS_SIM_INSERTED_AND_READY, NULL},
> + {"#QSS: 0, 3", QSS_STATUS_SIM_INSERTED_AND_READY, NULL},
> + {"#QSS: 0", QSS_STATUS_UNKNOWN, NULL},
> + {"QSS:0,1", QSS_STATUS_UNKNOWN, "Could not parse \"#QSS?\" response:
> QSS:0,1"},
> + {NULL, QSS_STATUS_UNKNOWN, NULL}
No need for this previous last item, use G_N_ELEMENTS() when
iterating, see below. G_N_ELEMENTS is a handy macro that does
sizeof(array)/sizeof(array[0]), effectively giving you the number of
items in the static array.
> +};
> +
> +static void
> +test_telit_parse_qss_query (void)
> +{
> + QssStatus actual_qss_status;
> + GError *error = NULL;
> + guint i;
> +
> + for (i = 0; qss_parse_tests[i].response != NULL; i++) {
for (i = 0; i < G_N_ELEMENTS (qss_parse_tests); i++) {
....
> + actual_qss_status = mm_telit_parse_qss_query
> (qss_parse_tests[i].response, &error);
> +
> + if (qss_parse_tests[i].error_message) {
> + g_assert_error (error, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
> + g_assert_cmpstr (error->message, ==,
> qss_parse_tests[i].error_message);
> + g_clear_error (&error);
> + } else {
> + g_assert_cmpint (actual_qss_status, ==,
> qss_parse_tests[i].expected_qss);
> + }
> + }
> +}
> +
> int main (int argc, char **argv)
> {
> setlocale (LC_ALL, "");
> @@ -516,5 +555,6 @@ int main (int argc, char **argv)
> g_test_add_func ("/MM/telit/bands/current/set_bands/2g",
> test_telit_get_2g_bnd_flag);
> g_test_add_func ("/MM/telit/bands/current/set_bands/3g",
> test_telit_get_3g_bnd_flag);
> g_test_add_func ("/MM/telit/bands/current/set_bands/4g",
> test_telit_get_4g_bnd_flag);
> + g_test_add_func ("/MM/telit/qss/query", test_telit_parse_qss_query);
> return g_test_run ();
> }
> --
> 2.9.3
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
