[PATCH 4/7] fully prepare GPS unmanaged port

[Tomas Jura]

Hi Aleksander

Thanks for the feedback. I'll try adjust the patch according to your 
comments.

I know is that there is no Hayes "OK" response after sending E2GPSNPD 
command. The response is the NMEA stream. When the MMPortSerial port is 
opened then there is the installed an listener which swallows the 
incoming messages. Does the unexpected NMEA sentence raises an error 
when the mm_port_serial_command_finish is called? I will have to test 
it. It will takes me some time. ( Currently working on question which 
Dan W. placed at 4.2.2016 17:04 CET, if the DGPS increases the startup 
time. Answer can simplify the DGPS patch. )

Tomas

On 11.2.2016 10:31, Aleksander Morgado wrote:
> 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);
>
>