Comment on attachment 114562
mbm: add GPS location gathering support (v2)

Review of attachment 114562:
-----------------------------------------------------------------

Some minor additional things to fix.

Also, could you maybe also attach a debug log of MM to see how it
behaves with the GPS enabled/disabled? Just for reference, because I
don't have a GPS capable MBM modem around.

Thanks!

::: plugins/mbm/mm-broadband-modem-mbm.c
@@ +1252,5 @@
> +typedef struct {
> +    MMBroadbandModemMbm *self;
> +    GSimpleAsyncResult *result;
> +    MMModemLocationSource source;
> +    int idx;

Isn't this idx variable not needed?

@@ +1388,5 @@
> +                                                 MM_CORE_ERROR,
> +                                                 MM_CORE_ERROR_FAILED,
> +                                                 "Couldn't open raw GPS 
> serial port");
> +        } else {
> +            GByteArray *buf = g_byte_array_sized_new (13);

A whiteline needed after the variable declaration.

@@ +1389,5 @@
> +                                                 MM_CORE_ERROR_FAILED,
> +                                                 "Couldn't open raw GPS 
> serial port");
> +        } else {
> +            GByteArray *buf = g_byte_array_sized_new (13);
> +            g_byte_array_append(buf, (const guint8 *) "AT*E2GPSNPD\r\n", 12);

I'm counting 13 chars in the string to be written, not 12.

How about this?

{ 
    GByteArray *buf;
    const gchar *command = "AT*E2GPSNPD\r\n";

    buf = g_byte_array_new ();
    g_byte_array_append (buf, command, strlen (command));
    mm_port_serial_command (...);
    g_byte_array_unref (buf);
}

Also, the fact that we need an AT command to be fed to the GPS data port
is an unusual case (which is why you need to use the MMPortSerial API
and cannot use the MMPortSerialAt one. I'd explain that in a comment
message just around here, so that it's clear what is being done.

-- 
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/783220

Title:
  [needs-packaging] Missing packages for mbm-gpsd and mbm-gps-control

To manage notifications about this bug go to:
https://bugs.launchpad.net/modemmanager/+bug/783220/+subscriptions

-- 
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs

Reply via email to