<HTML><HEAD></HEAD><BODY id="enrich@telit.com"> <table style="height: 80%;" border="0" cellspacing="0" cellpadding="0" width="95%" id="imp_template" class="enrich@telit.com"> <tbody> <tr height="80%"> <td id="etexttd" class="mceNonEditable mceNonResizeable" style="padding:10px;" width="100%" valign="top"> <div id="imp_emailtext" class="mceNonEditable mceNonResizeable">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. > > > } > > > > /*****************************************************************************/ > > > > > </div> </td> </tr> <tr height="20%"> <td width="100%"> <table id="row2" style="height: 100%; font-family: Arial,Helvetica,sans-serif;" border="0" cellspacing="0" cellpadding="2" width="100%"> <tbody> <tr> <td width="100%"> <table border="0"> <tbody> <tr> <td> <table id="autoSig" style="height: 100%; font-family: Arial,Helvetica,sans-serif;" border="0" cellspacing="0" cellpadding="2" width="100%"> <tbody> <tr> <td width="100%"> <table border="0" width="95%" align="left"> <tbody> <tr> <td colspan="2">Carlo Lobrano </td> </tr> <tr> <td colspan="2">Senior Software Engineer, R&D </td> </tr> <tr> <td rowspan="3" width="100"><img class="embedded_img" src="cid:imp_0.7706962700687823_autosig_Image_01" border="0" alt="Link" width="87" height="50" / style=" max-width:100%;"></td> <td id="optional" valign="top">T: +39 040 4192 560</td> </tr> <tr> <TD> </TD></tr> <tr> <td valign="top"><a style="text-decoration: none; color:inherit;" id="rt" href="mailto:carlo.lobrano@telit.com">carlo.lobrano@telit.com</a></td> </tr> <tr> <td colspan="2"><img class="embedded_img" src="cid:imp_0.4702790782620736_autosig_Image_02" border="0" alt="Link" width="291" height="7" / style=" max-width:100%;"></td> </tr> <tr> <td id="rt" style="font-family: arial,helvetica,sans-serif; color: #003366;" width="100"><a id="rt" href="http://www.telit.com">www.telit.com</a></td> <td> <table border="0"> <tbody> <tr> <td> </td> <td><a href="http://robustus.impactia.com/AnalyticServer/redirect?cid=15fbbe1bb7e142c9&mid=76961948&eurl=aHR0cDovL3d3dy55b3V0dWJlLmNvbS90ZWxpdGNvbW11bmljYXRpb25z&istmp=enrich@telit.com"><img class="embedded_img" src="cid:imp_0.5532723427003916_autosig_Image_03" border="0" alt="Link" / style=" max-width:100%;"></a></td> <td><a href="http://robustus.impactia.com/AnalyticServer/redirect?cid=15fbbe1bb7e142c9&mid=76961948&eurl=aHR0cHM6Ly9wbHVzLmdvb2dsZS5jb20vK3RlbGl0&istmp=enrich@telit.com"><img class="embedded_img" src="cid:imp_0.6861437723755275_autosig_Image_04" border="0" alt="Link" / style=" max-width:100%;"></a></td> <td><a href="http://robustus.impactia.com/AnalyticServer/redirect?cid=15fbbe1bb7e142c9&mid=76961948&eurl=aHR0cHM6Ly90d2l0dGVyLmNvbS9UZWxpdF9XU19jb3Jw&istmp=enrich@telit.com"><img class="embedded_img" src="cid:imp_0.8252852095764112_autosig_Image_05" border="0" alt="Link" / style=" max-width:100%;"></a></td> <td><a href="http://robustus.impactia.com/AnalyticServer/redirect?cid=15fbbe1bb7e142c9&mid=76961948&eurl=aHR0cDovL2ZhY2Vib29rLmNvbS90ZWxpdHdpcmVsZXNz&istmp=enrich@telit.com"><img class="embedded_img" src="cid:imp_0.017211039270454198_autosig_Image_06" border="0" alt="Link" / style=" max-width:100%;"></a></td> <td><a href="http://robustus.impactia.com/AnalyticServer/redirect?cid=15fbbe1bb7e142c9&mid=76961948&eurl=aHR0cDovL3d3dy5saW5rZWRpbi5jb20vY29tcGFueS90ZWxpdC13aXJlbGVzcy1zb2x1dGlvbnM=&istmp=enrich@telit.com"><img class="embedded_img" src="cid:imp_0.07153260227664593_autosig_Image_07" border="0" alt="Link" / style=" max-width:100%;"></a></td> </tr> </tbody> </table> </td> </tr> <tr> <td></td> <td></td> </tr> <tr> <td></td> <td></td> </tr> </tbody> </table> </td> </tr> </tbody> </table> </td> </tr> <tr> <td><a href="http://robustus.impactia.com/AnalyticServer/redirect?cid=15fbbe1bb7e142c9&mid=76961948&eurl=aHR0cDovL3d3dy5pb3QtZXhlY3V0aXZlLWhhbmRib29rLmNvbS9nZXQtdGhlLWJvb2sv&istmp=enrich@telit.com"><img class="embedded_img" src="cid:imp_0.9930939633280242_Image_01" border="0" alt="Link" / style=" max-width:100%;"></a> <table border="0" cellspacing="1" cellpadding="1"> <tbody> <tr> <td rowspan="6">This electronic message, including attachments, is intended only for the use of the individual or company named above or to which it is addressed. The information contained in this message shall be considered confidential and proprietary, and may include confidential work product. If you are not the intended recipient, please be aware that any unauthorized use, dissemination, distribution or copying of this message is strictly prohibited. If you have received this email in error, please notify the sender by replying to this message and deleting this email immediately. To the extent this email includes an offer to buy Telit products or an order confirmation, the offer or order confirmation is governed by Telit’s General Terms and Conditions of Sale, which are published <a id="rt" href="http://www.telit.com/terms-and-conditions/">here</a>. </td> </tr> </tbody> </table> </td> </tr> </tbody> </table> </td> </tr> </tbody> </table> </td> </tr> </tbody> </table> <div id="footer" class="mceNonEditable mceNonResizeable" dir="ltr"> <hr size="1" noshade="65535" /> </div> </BODY></HTML>