[PATCH] Core logic to support SIM hot swap
Carlo Lobrano
carlo.lobrano at telit.com
Fri Jul 1 08:47:20 UTC 2016
Hi Aleksander,
thanks for the review. I agree with all the changes, but regarding the usage of the secondary port, the reason is that I couldn't get unsolicited messages from the primary port when the modem is connected to a network. The same problem does not occur when not connected, namely I get all the unsolicited.
I also checked with a USB sniffer and the modem is always sending the unsolicited, just I can't see it on MM. Instead, using the secondary port this issue does not occur.
Consider that the modem sends QSS only from the port where it's been set, so I can't set it on the primary and listen on the secondary.
On giovedì 30 giugno 2016 16:06:23 CEST Aleksander Morgado wrote:
> 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.
>
> > }
> >
> > /*****************************************************************************/
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_01.jpg
Type: image/jpg
Size: 2771 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0002.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_07.gif
Type: image/gif
Size: 1089 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0006.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_04.gif
Type: image/gif
Size: 1059 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0007.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_05.gif
Type: image/gif
Size: 643 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0008.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Image_01.gif
Type: image/gif
Size: 26358 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0009.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_06.gif
Type: image/gif
Size: 1091 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0010.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_02.JPG
Type: image/jpg
Size: 8465 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0003.jpg>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: autosig_Image_03.gif
Type: image/gif
Size: 1111 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/modemmanager-devel/attachments/20160701/78b74886/attachment-0011.gif>
More information about the ModemManager-devel
mailing list