Hey Tomas, See comments below.
On 02/02/16 15:30, [email protected] wrote: > From: Tomas Jura <[email protected]> > 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 _______________________________________________ ModemManager-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
