[PATCH 4/7] fully prepare GPS unmanaged port

[Aleksander Morgado]

Hey Tomas,

See comments below.

On 02/02/16 15:30, tomas.jura1 at gmail.com wrote:
> From: Tomas Jura <tomas_jura1 at gmail.com>
> 

Looks like that previous email address doesn't exist? Earlier messages
sent there bounced with error.

> ---
>  plugins/mbm/mm-broadband-modem-mbm.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/plugins/mbm/mm-broadband-modem-mbm.c b/plugins/mbm/mm-broadband-modem-mbm.c
> index ec4563c..2f0a014 100644
> --- a/plugins/mbm/mm-broadband-modem-mbm.c
> +++ b/plugins/mbm/mm-broadband-modem-mbm.c
> @@ -1348,6 +1348,16 @@ enable_location_gathering_finish (MMIfaceModemLocation *self,
>      return !g_simple_async_result_propagate_error (G_SIMPLE_ASYNC_RESULT (res), error);
>  }
>  
> +static void close_unmanaged_port ( MMPortSerialGps  *self,

"static void" should go in its own line.

Also, "self" in this context should be only used for the modem object;
I'd suggest you use "port" or something like that instead.

> +                                   GAsyncResult *res,
> +                                   LocationGatheringContext *ctx )
> +{

Now that I see it, a call to mm_port_serial_command_finish () is missing
here. Every time a callback is provided to an async method (instead of
just NULL) we should call the corresponding _finish() method. I don't
think it is strictly required in this specific case, because the
internal GSimpleAsyncResult owns the response buffer, but for
consistency I think we should do it.

> +    mm_port_serial_close(MM_PORT_SERIAL(self));
> +    g_simple_async_result_set_op_res_gboolean (G_SIMPLE_ASYNC_RESULT(res), TRUE);
> +    location_gathering_context_complete_and_free (ctx);
> +}
> +
> +
>  static void
>  gps_enabled_ready (MMBaseModem *self,
>                     GAsyncResult *res,
> @@ -1364,7 +1374,8 @@ gps_enabled_ready (MMBaseModem *self,
>  
>      /* Only use the GPS port in NMEA/RAW setups */
>      if (ctx->source & (MM_MODEM_LOCATION_SOURCE_GPS_NMEA |
> -                       MM_MODEM_LOCATION_SOURCE_GPS_RAW)) {
> +                       MM_MODEM_LOCATION_SOURCE_GPS_RAW |
> +                       MM_MODEM_LOCATION_SOURCE_GPS_UNMANAGED )) {
>          gps_port = mm_base_modem_peek_port_gps (self);
>          if (!gps_port ||
>              !mm_port_serial_open (MM_PORT_SERIAL (gps_port), &error)) {
> @@ -1386,6 +1397,17 @@ gps_enabled_ready (MMBaseModem *self,
>               */
>              buf = g_byte_array_new ();
>              g_byte_array_append (buf, (const guint8 *) command, strlen (command));
> +            if (ctx->source & MM_MODEM_LOCATION_SOURCE_GPS_UNMANAGED ) {
> +                mm_port_serial_command (MM_PORT_SERIAL (gps_port),
> +                                        buf,
> +                                        3,
> +                                        FALSE,
> +                                        NULL,
> +                                        (GAsyncReadyCallback) close_unmanaged_port,
> +                                        ctx );
> +                g_byte_array_unref (buf);
> +                return;
> +            }
>              mm_port_serial_command (MM_PORT_SERIAL (gps_port),
>                                      buf,
>                                      3,
> 

As said in the original patch review, I think we should always provide a
GAsyncReadyCallback, not only when it's UNMANAGED, e.g.:

static void
e2gpsnpd_ready (MMPortSerialGps *port,
                GAsyncResult *res,
                LocationGatheringContext *ctx)
{
    GError *error = NULL;
    GByteArray *response;

    response = mm_port_serial_command_finish (port, res, &error);

    /* We ignore errors, just debug log them */
    if (error) {
        mm_dbg ("GPS enabling failed: %s", error->message);
        g_error_free (error);
    } else {
        mm_dbg ("GPS enabling succeeded");
        g_byte_array_unref (response);
    }

    if (ctx->source & MM_MODEM_LOCATION_SOURCE_GPS_UNMANAGED)
        mm_port_serial_close (MM_PORT_SERIAL (self));

    g_simple_async_result_set_op_res_gboolean
(G_SIMPLE_ASYNC_RESULT(res), TRUE);
    location_gathering_context_complete_and_free (ctx);
}

...

mm_port_serial_command (MM_PORT_SERIAL (gps_port),
                        buf,
                        3,
                        FALSE,
                        NULL,
                        (GAsyncReadyCallback) e2gpsnpd_ready,
                        ctx);


-- 
Aleksander
https://aleksander.es