On Tue, Sep 8, 2015 at 10:37 PM, Stevens, Nick <nick.stev...@digi.com> wrote:
> Previously the enable unsolicited messages command (+CNMI) was only
> being sent on the primary. This patch adds support for sending the
> enable on the secondary as well. If the secondary doesn't exist, or if
> setting the enable causes an error, a warning is logged but no error is
> propogated up.
>
> This change is needed for proper SMS operation on the Telit HE910, which
> requires that +CNMI be sent to both primary and secondary. Since a
> failure to send the +CNMI command on the secondary is a non-fatal error,
> it is unlikely that this will cause issues with other modems.
>
> Signed-off-by: Nick Stevens <nick.stev...@digi.com>
Not sure I fully like the approach taken with how the callback is
stored in the context; the better way would be to use a
GSimpleAsyncResult to wrap all the steps in the same async operation.
The GSimpleAsyncResult will also take a reference to the 'self'
object, and that means that it will make sure the object stays valid
for as long as the async operation is running.
Also, you shouldn't store non-full references to the primary and
secondary port objects in the context. If the context is going to be
used asynchronously, better store full references (e.g. not peek() but
get() the ports), and then explicitly g_object_unref() them when
freeing the context. You could even fully skip the context and pass
the GSimpleAsyncResult object to the different steps, and use
g_async_result_get_source_object() to retrieve the 'self' pointer in
each sub-step (note g_async_result_get_source_object() returns a full
reference), and then peek() the ports before using them.
Also, I think we should first configure the primary port and then the
secondary if it exists, not the other way around.
From my POV, it would be more desirable to:
1) Generate a GSimpleAsyncResult right away in
modem_messaging_enable_unsolicited_events(), and store that one in the
context (not the callback and user data).
2) Run the enabling in the primary port, passing a new
modem_messaging_enable_unsolicited_events_primary_ready callback.
3) In modem_messaging_enable_unsolicited_events_secondary_ready:
3.1) if there is no secondary port, complete the
GSimpleAsyncResult from the context, and we're finished.
3.2) if there is a secondary port:
3.2.1. run the enabling in the secondary port, passing the new
modem_messaging_enable_unsolicited_events_secondary_ready callback.
3.2.2. in
modem_messaging_enable_unsolicited_events_secondary_ready, complete
the GSimpleAsyncResult from the context, and we're finished.
> ---
> src/mm-broadband-modem.c | 71
> ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c
> index f176b80..085d4d7 100644
> --- a/src/mm-broadband-modem.c
> +++ b/src/mm-broadband-modem.c
> @@ -5791,7 +5791,7 @@ set_messaging_unsolicited_events_handlers
> (MMIfaceModemMessaging *self,
> ports[0] = mm_base_modem_peek_port_primary (MM_BASE_MODEM (self));
> ports[1] = mm_base_modem_peek_port_secondary (MM_BASE_MODEM (self));
>
> - /* Enable unsolicited events in given port */
> + /* Add messaging unsolicited events handler for port primary and
> secondary */
> for (i = 0; i < 2; i++) {
> if (!ports[i])
> continue;
> @@ -5840,6 +5840,19 @@ modem_messaging_cleanup_unsolicited_events
> (MMIfaceModemMessaging *self,
>
> /*****************************************************************************/
> /* Enable unsolicited events (SMS indications) (Messaging interface) */
>
> +typedef struct {
> + GAsyncReadyCallback callback;
> + gpointer user_data;
> + MMPortSerialAt *primary;
> + MMPortSerialAt *secondary;
> +} MessagingEnableContext;
> +
> +static void
> +messaging_enable_context_free (MessagingEnableContext *ctx)
> +{
> + g_free (ctx);
> +}
> +
> static gboolean
> modem_messaging_enable_unsolicited_events_finish (MMIfaceModemMessaging
> *self,
> GAsyncResult *res,
> @@ -5847,7 +5860,7 @@ modem_messaging_enable_unsolicited_events_finish
> (MMIfaceModemMessaging *self,
> {
> GError *inner_error = NULL;
>
> - mm_base_modem_at_sequence_finish (MM_BASE_MODEM (self), res, NULL,
> &inner_error);
> + mm_base_modem_at_sequence_full_finish (MM_BASE_MODEM (self), res, NULL,
> &inner_error);
> if (inner_error) {
> g_propagate_error (error, inner_error);
> return FALSE;
> @@ -5895,17 +5908,65 @@ static const MMBaseModemAtCommand cnmi_sequence[] = {
> };
>
> static void
> +modem_messaging_enable_unsolicited_events_secondary_ready (MMBaseModem *self,
> + GAsyncResult *res,
> +
> MessagingEnableContext *ctx)
> +{
> + GError *inner_error = NULL;
> +
> + /* Since the secondary is not required, we don't propagate the error
> anywhere */
> + mm_base_modem_at_sequence_full_finish (MM_BASE_MODEM (self), res, NULL,
> &inner_error);
> + if (inner_error) {
> + mm_warn("(%s) Unable to enable messaging unsolicited events on modem
> secondary",
> + mm_port_get_device (MM_PORT (ctx->secondary)));
> + g_error_free(inner_error);
> + }
> +
> + /* Enable unsolicited events for primary port */
> + mm_dbg ("(%s) Enabling messaging unsolicited events on modem primary",
> + mm_port_get_device (MM_PORT (ctx->primary)));
> + mm_base_modem_at_sequence_full (
> + self,
> + ctx->primary,
> + cnmi_sequence,
> + NULL, /* response_processor_context */
> + NULL, /* response_processor_context_free */
> + NULL,
> + ctx->callback,
> + ctx->user_data);
> +
> + messaging_enable_context_free(ctx);
> +}
> +
> +static void
> modem_messaging_enable_unsolicited_events (MMIfaceModemMessaging *self,
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> - mm_base_modem_at_sequence (
> + MessagingEnableContext *ctx = NULL;
> +
> + ctx = g_new0(MessagingEnableContext, 1);
> + ctx->primary = mm_base_modem_peek_port_primary (MM_BASE_MODEM (self));
> + ctx->secondary = mm_base_modem_peek_port_secondary (MM_BASE_MODEM
> (self));
> + ctx->callback = callback;
> + ctx->user_data = user_data;
> +
> + /* Enable unsolicited events for secondary port if it exists, otherwise
> + * go straight to enabling the primary port */
> + mm_dbg ("(%s) Enabling messaging unsolicited events on %s",
> + mm_port_get_device (MM_PORT (ctx->secondary ? ctx->secondary :
> ctx->primary)),
> + ctx->secondary ? "secondary" : "primary");
> + mm_base_modem_at_sequence_full (
> MM_BASE_MODEM (self),
> + ctx->secondary ? ctx->secondary : ctx->primary,
> cnmi_sequence,
> NULL, /* response_processor_context */
> NULL, /* response_processor_context_free */
> - callback,
> - user_data);
> + NULL,
> + ctx->secondary ?
> +
> (GAsyncReadyCallback)modem_messaging_enable_unsolicited_events_secondary_ready
> :
> + callback,
> + ctx->secondary ? ctx : user_data);
> }
>
>
> /*****************************************************************************/
> --
> 2.5.0
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
_______________________________________________
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
