[PATCH] Core logic to support SIM hot swap
Aleksander Morgado
aleksander at aleksander.es
Thu Jun 30 14:06:23 UTC 2016
Hey,
>
> sorry but I sent the previous email before completing it. This
> proposal is based on two patches.
> The first, in the previous e-mail, adds the core logic for modems that
> support SIM hot swap,
> The second in this e-mail, is the plugin side implementation for a Telit modem.
>
> Carlo
>
> ---
>
> MMBroadbandModemTelit:
> * added SIM_HOT_SWAP property and updated init function accordingly
> * added function to enable QSS unsolicited
> * added QSS unsolicited handler
> ---
> plugins/dell/mm-plugin-dell.c | 3 +-
> plugins/telit/mm-broadband-modem-telit.c | 185 ++++++++++++++++++++++++++++++-
> plugins/telit/mm-broadband-modem-telit.h | 7 +-
> plugins/telit/mm-plugin-telit.c | 3 +-
> 4 files changed, 191 insertions(+), 4 deletions(-)
>
> diff --git a/plugins/dell/mm-plugin-dell.c b/plugins/dell/mm-plugin-dell.c
> index 2ed4375..9275a04 100644
> --- a/plugins/dell/mm-plugin-dell.c
> +++ b/plugins/dell/mm-plugin-dell.c
> @@ -424,7 +424,8 @@ create_modem (MMPlugin *self,
> drivers,
>
> mm_plugin_get_name (self),
> vendor,
> - product));
> + product,
> + TRUE /*
> SIM hot swap supported */));
If SIM hotswap always supported, what's the purpose of having the
additional argument in mm_broadband_modem_telit_new()? I would just
remove the additional argument.
> }
>
> mm_dbg ("Dell-branded generic modem found...");
> diff --git a/plugins/telit/mm-broadband-modem-telit.c
> b/plugins/telit/mm-broadband-modem-telit.c
> index 8baf2cf..b8ca965 100644
> --- a/plugins/telit/mm-broadband-modem-telit.c
> +++ b/plugins/telit/mm-broadband-modem-telit.c
> @@ -42,6 +42,131 @@ 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));
>
> +
> +struct _MMBroadbandModemTelitPrivate {
> + gboolean is_sim_hot_swap_supported;
This gboolean is the one providing the value to the property; it
should go in MMBroadbandModem so that all modems have the boolean, not
just the Telit one.
You can then g_object_get() the property in the plugin to know the value.
> + GAsyncReadyCallback sim_swap_callback;
> +};
> +
> +enum {
> + PROP_FIRST,
> + PROP_IS_SIM_HOT_SWAP_SUPPORTED,
> + PROP_LAST
> +};
> +
> +/*****************************************************************************/
> +/* Setup SIM hot swap (Modem interface) */
> +
> +static void
> +telit_qss_unsolicited_handler (MMPortSerialAt *port,
> + GMatchInfo *match_info,
> + MMBroadbandModemTelit *self)
> +{
> + guint qss;
> + gboolean is_sim_missing;
> +
> + if (!mm_get_uint_from_match_info (match_info, 1, &qss))
> + return;
> +
> + switch(qss) {
Missing whitespaces before open-parenthesis; please review the whole
patch fixing those, they're in multiple places.
> + 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;
> + }
> +
> + is_sim_missing = (qss == 0) ? TRUE : FALSE;
> + mm_broadband_modem_update_sim_hot_swap_status (MM_BROADBAND_MODEM
> (self), is_sim_missing);
> +}
> +
> +typedef struct {
> + MMBroadbandModemTelit *self;
> + GSimpleAsyncResult *result;
> +} ToggleQssUnsolicitedContext;
> +
> +static void
> +toggle_qss_unsolicited_context_complete_and_free
> (ToggleQssUnsolicitedContext *ctx)
> +{
> + g_simple_async_result_complete (ctx->result);
> + g_object_unref (ctx->result);
> + g_object_unref (ctx->self);
> + g_slice_free (ToggleQssUnsolicitedContext, ctx);
> +}
> +
> +static gboolean
> +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);
> +}
> +
> +static void
> +telit_qss_toggle_ready (MMBaseModem *self,
> + GAsyncResult *res,
> + ToggleQssUnsolicitedContext *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");
error is leaking here.
> + } else {
> + mm_port_serial_at_add_unsolicited_msg_handler (
> + mm_base_modem_peek_port_secondary (MM_BASE_MODEM (ctx->self)),
> + g_regex_new ("#QSS:\\s*([0-3])\\r\\n", G_REGEX_RAW, 0, NULL),
> + (MMPortSerialAtUnsolicitedMsgFn)telit_qss_unsolicited_handler,
> + ctx->self,
> + NULL);
The mm_port_serial_at_add_unsolicited_msg_handler() takes its own
reference to the GRegex, so that would be leaking.
You should create the GRegex, pass it to
mm_port_serial_at_add_unsolicited_msg_handler(), then unref the
original reference.
> + g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
> + }
> +
> + toggle_qss_unsolicited_context_complete_and_free (ctx);
> +}
> +
> +
> +static void
> +modem_setup_sim_hot_swap (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + ToggleQssUnsolicitedContext *ctx;
> +
> + mm_dbg ("Telit: enabling QSS");
> +
> + 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);
> +
> + mm_base_modem_at_command_full (MM_BASE_MODEM (self),
> + mm_base_modem_peek_port_secondary
You're assuming there's a secondary port, we shouldn't do that. What
if there's only one port? What if for some reason MM is able to just
one even if there are more?
Can't this QSS command be sent to the primary port? We always have a
primary port.
> (MM_BASE_MODEM (self)),
> + "#QSS=1",
> + 3,
> + FALSE,
> + FALSE, /* raw */
> + NULL, /* cancellable */
> + (GAsyncReadyCallback)
> telit_qss_toggle_ready,
> + ctx);
> +}
> +
> /*****************************************************************************/
> /* Set current bands (Modem interface) */
>
> @@ -1009,7 +1134,8 @@ mm_broadband_modem_telit_new (const gchar *device,
> const gchar **drivers,
> const gchar *plugin,
> guint16 vendor_id,
> - guint16 product_id)
> + guint16 product_id,
> + gboolean is_sim_hot_swap_supported)
> {
> return g_object_new (MM_TYPE_BROADBAND_MODEM_TELIT,
> MM_BASE_MODEM_DEVICE, device,
> @@ -1017,12 +1143,17 @@ mm_broadband_modem_telit_new (const gchar *device,
> MM_BASE_MODEM_PLUGIN, plugin,
> MM_BASE_MODEM_VENDOR_ID, vendor_id,
> MM_BASE_MODEM_PRODUCT_ID, product_id,
> + IS_SIM_HOT_SWAP_SUPPORTED_PROPERTY,
> is_sim_hot_swap_supported,
> NULL);
As said before, I'd just remove the is_sim_hot_swap_supported extra
argument from the mm_broadband_modem_telit_new() method, and then
just):
IS_SIM_HOT_SWAP_SUPPORTED_PROPERTY, TRUE
> }
>
> 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);
> }
>
> static void
> @@ -1052,6 +1183,8 @@ iface_modem_init (MMIfaceModem *iface)
> iface->load_current_modes_finish = load_current_modes_finish;
> iface->set_current_modes = set_current_modes;
> iface->set_current_modes_finish = set_current_modes_finish;
> + iface->setup_sim_hot_swap = modem_setup_sim_hot_swap;
> + iface->setup_sim_hot_swap_finish = modem_setup_sim_hot_swap_finish;
> }
>
> static void
> @@ -1062,6 +1195,56 @@ iface_modem_3gpp_init (MMIfaceModem3gpp *iface)
> }
>
> static void
> +set_property (GObject *object,
> + guint prop_id,
> + const GValue *value,
> + GParamSpec *pspec)
> +{
> + MMBroadbandModemTelit *self = MM_BROADBAND_MODEM_TELIT (object);
> +
> + switch (prop_id) {
> + case PROP_IS_SIM_HOT_SWAP_SUPPORTED:
> + self->priv->is_sim_hot_swap_supported = g_value_get_boolean (value);
> + break;
> + default:
> + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> + break;
> + }
> +}
> +
> +static void
> +get_property (GObject *object,
> + guint prop_id,
> + GValue *value,
> + GParamSpec *pspec)
> +{
> + MMBroadbandModemTelit *self = MM_BROADBAND_MODEM_TELIT (object);
> +
> + switch (prop_id) {
> + case PROP_IS_SIM_HOT_SWAP_SUPPORTED:
> + g_value_set_boolean (value, self->priv->is_sim_hot_swap_supported);
> + break;
> + default:
> + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
> + break;
> + }
> +}
These setter and getter should go in MMBroadbandModem.
> +
> +static void
> mm_broadband_modem_telit_class_init (MMBroadbandModemTelitClass *klass)
> {
> + GObjectClass *object_class = G_OBJECT_CLASS (klass);
> +
> + g_type_class_add_private (object_class, sizeof
> (MMBroadbandModemTelitPrivate));
> +
> + object_class->set_property = set_property;
> + object_class->get_property = get_property;
> +
> + g_object_class_install_property (object_class,
> PROP_IS_SIM_HOT_SWAP_SUPPORTED,
> + g_param_spec_boolean (IS_SIM_HOT_SWAP_SUPPORTED_PROPERTY,
> + "IsSimHotSwapSupported",
> + "Whether the modem supports sim hot
> swap or not.",
> + TRUE,
> + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));
Same for the property itself, it should go in MMBroadbandModem.
> +
> }
> diff --git a/plugins/telit/mm-broadband-modem-telit.h
> b/plugins/telit/mm-broadband-modem-telit.h
> index 50e6365..37e2f21 100644
> --- a/plugins/telit/mm-broadband-modem-telit.h
> +++ b/plugins/telit/mm-broadband-modem-telit.h
> @@ -27,11 +27,15 @@
> #define MM_IS_BROADBAND_MODEM_TELIT_CLASS(klass)
> (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_BROADBAND_MODEM_TELIT))
> #define MM_BROADBAND_MODEM_TELIT_GET_CLASS(obj)
> (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_BROADBAND_MODEM_TELIT,
> MMBroadbandModemTelitClass))
>
> +#define IS_SIM_HOT_SWAP_SUPPORTED_PROPERTY "is-sim-hot-swap-supported"
> +
Why is this redefined here? Wasn't this defined in mm-iface-modem?
> typedef struct _MMBroadbandModemTelit MMBroadbandModemTelit;
> +typedef struct _MMBroadbandModemTelitPrivate MMBroadbandModemTelitPrivate;
> typedef struct _MMBroadbandModemTelitClass MMBroadbandModemTelitClass;
>
> struct _MMBroadbandModemTelit {
> MMBroadbandModem parent;
> + MMBroadbandModemTelitPrivate *priv;
> };
>
> struct _MMBroadbandModemTelitClass{
> @@ -44,6 +48,7 @@ MMBroadbandModemTelit *mm_broadband_modem_telit_new
> (const gchar *device,
> const gchar **drivers,
> const gchar *plugin,
> guint16 vendor_id,
> - guint16 product_id);
> + guint16 product_id,
> + gboolean
> is_sim_hot_swap_supported);
>
> #endif /* MM_BROADBAND_MODEM_TELIT_H */
> diff --git a/plugins/telit/mm-plugin-telit.c b/plugins/telit/mm-plugin-telit.c
> index 5a44ba6..b9506da 100644
> --- a/plugins/telit/mm-plugin-telit.c
> +++ b/plugins/telit/mm-plugin-telit.c
> @@ -48,7 +48,8 @@ create_modem (MMPlugin *self,
> drivers,
>
> mm_plugin_get_name (self),
> vendor,
> - product));
> + product,
> + TRUE /*
> support sim hot swap */));
As said before, I'd remove the extra gboolean arg from
mm_broadband_modem_telit_new(), if it's always TRUE.
> }
>
> /*****************************************************************************/
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list