Bug 85008

Created attachment 114525 [details] [review]
mbm: add GPS location gathering support

This patch add gps support for mbm devices, mainly inspired by the huawai similar plugin. The interval between two batches of NMEA sentences is set arbitrarily to 5 secs with a #define (ModemManager updates the gps location every 30 seconds on dbus, so it seems to me there's no need to request nmea data too frequently). The mbm device particularity is that the gps nmea device ttyACM2 must receive the command "AT*E2GPSNPD" before starting to emit data.

Created attachment 114526 [details]
mbm: factorize the udev rules

Created attachment 114527 [details] [review]
mbm: add GPS support for F5521gw

I tested this patch on my thinkpad X220, with this card:
Bus 004 Device 013: ID 0bdb:1911 Ericsson Business Mobile Networks BV

Created attachment 114528 [details] [review]
mbm: add GPS support for more cards

This patch imports the udev rule knowledge from mbm-gpsd. The mbm port usage is based on its bInterfaceNumber number (01 -> ttyACM0 for AT commands, 03 -> ttyACM1 for ppp data, 09 -> ttyACM2 for gps nmea stream).

(In reply to Fabrice Bellet from comment #2)
> Created attachment 114526 [details]
> mbm: factorize the udev rules

I don't think we want to do this. Adding new VID/PIDs in future patches will be much easier to read and review if the new PIDs add new lines, instead of modifying lines.

(In reply to Fabrice Bellet from comment #1)
> Created attachment 114525 [details] [review] [review]
> mbm: add GPS location gathering support
> 
> This patch add gps support for mbm devices, mainly inspired by the huawai
> similar plugin. The interval between two batches of NMEA sentences is set
> arbitrarily to 5 secs with a #define (ModemManager updates the gps location
> every 30 seconds on dbus, so it seems to me there's no need to request nmea
> data too frequently). The mbm device particularity is that the gps nmea
> device ttyACM2 must receive the command "AT*E2GPSNPD" before starting to
> emit data.

The ID_MM_ERICSSON_MBM_AT_PORT and ID_MM_ERICSSON_MBM_AT_PPP flags should not be added. MBM uses by default the "MMBroadbandBearerMbm" object, and that object requires the presence of the WWAN net port. i.e. MBM modems will always use a WWAN port and never PPP, at least when managed by ModemManager.

Could you update the patch so that only the GPS bits gets included?

Created attachment 114549 [details] [review]
Add support for H5321gw

Hi,

I have a similar device, H5321gw, I've made an attempt to add it to the rules file.

It shows up as a GPS now, but I've yet to get a fix, and even the 3GPP location is wildly inaccurate, so I'm not really sure what's going on.

Anyway, thank you so much for working on this!

Created attachment 114562 [details] [review]
mbm: add GPS location gathering support (v2)

Thanks for the review. This updated patch removes the AT_PORT and PPP_PORT flags, and leaves the udev rules file with one vid/pid per line. The port settings with my modem are :

  -------------------------
  Hardware |   manufacturer: 'Lenovo'
           |          model: 'F5521gw'
           |       revision: 'R3B01'
           |      supported: 'gsm-umts'
           |        current: 'gsm-umts'
           |   equipment id: 'xxxxxxxxxxxxxxx'
  -------------------------
  System   |         device: '/sys/devices/pci0000:00/0000:00:1d.0/usb4/4-1/4-1.4'
           |        drivers: 'cdc_acm, cdc_ncm, cdc_wdm'
           |         plugin: 'Ericsson MBM'
           |   primary port: 'ttyACM0'
           |          ports: 'ttyACM0 (at), cdc-wdm0 (at), cdc-wdm1 (at), ttyACM1 (at), wwp0s29u1u4i6 (net), ttyACM2 (gps)'
  -------------------------

A sidenote about the *E2GPSCTL parameters. According to thinkwiki, the 3rd parameter may be used to activate the DGPS feature. However, when I set this value (AT*E2GPSCTL=1,5,1 for example), I notice that :

1/ the stream is refreshed every second, and not every 5 seconds (as it works when just setting AT*E2GPSCTL=1,5)

2/ some spurious or blank lines, not starting with $GP are interleaved in the regular NMEA stream (from /dev/ttyACM2). These lines look like the characters of the end of previous NMEA sentences, quite strange. Firmware bug?

3/ no DGPS information appears in the NMEA $GPGGA sentences, even after a 3D fix is successfully obtained. According to http://aprs.gids.nl/nmea/#gga the DGPS information should appear in the last two fields of the $GPGGA string. But I may be in a location where DGPS stations are out-of-reach.

So, at the difference of the wiki that suggests to setup the GPS using "AT*E2GPSCTL=1,5,1", I prefer to use the command "AT*E2GPSCTL=1,5" instead.

Comment on attachment 114562 [details] [review]
mbm: add GPS location gathering support (v2)

Review of attachment 114562 [details] [review]:
-----------------------------------------------------------------

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.

Created attachment 114965 [details] [review]
mbm: add GPS location gathering support (v3)

New patch with suggestions from comment #10 applied

Created attachment 114966 [details]
ModemManager --debug log

Here are the related mmcli commands that produced this log file (some location details edited).

[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-status

/org/freedesktop/ModemManager1/Modem/0
  ----------------------------
  Location | capabilities: '3gpp-lac-ci, gps-raw, gps-nmea, gps-unmanaged'
           |      enabled: '3gpp-lac-ci'
           |      signals: 'no'
[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-enable-gps-nmea
successfully setup location gathering
[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-status

/org/freedesktop/ModemManager1/Modem/0
  ----------------------------
  Location | capabilities: '3gpp-lac-ci, gps-raw, gps-nmea, gps-unmanaged'
           |      enabled: '3gpp-lac-ci, gps-nmea'
           |      signals: 'no'
[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-get

/org/freedesktop/ModemManager1/Modem/0
  -------------------------
  3GPP location   | Mobile country code: '208'
                  | Mobile network code: '20'
                  |  Location area code: 'xxxx'
                  |             Cell ID: 'xxxxxx'
  -------------------------
  GPS NMEA traces | $GPGGA,,,,,,0,0,,,M,,,,*1B
                  | $GPGLL,,,,,235959.000,V,N*7B
                  | $GPGST,235959.000,,,,0,,,*78
                  | $GPGSV,1,1,01,14,,,,,,,,,,,,,,,*7D
                  | $GPGSA,A,1,,,,,,,,,,,,,,,*1E
                  | $GPRMC,235959.000,V,,,,,,,,,,N*4C
  -------------------------
  Raw GPS         | Not available
  -------------------------
  CDMA BS         | Not available
[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-disable-gps-nmea
successfully setup location gathering
[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-status

/org/freedesktop/ModemManager1/Modem/0
  ----------------------------
  Location | capabilities: '3gpp-lac-ci, gps-raw, gps-nmea, gps-unmanaged'
           |      enabled: '3gpp-lac-ci'
           |      signals: 'no'
[bellet@gandi-2 ModemManager (mm-1-4 %)]$ mmcli -m 0 --location-get

/org/freedesktop/ModemManager1/Modem/0
  -------------------------
  3GPP location   | Mobile country code: '208'
                  | Mobile network code: '20'
                  |  Location area code: 'xxxx'
                  |             Cell ID: 'xxxxxx'
  -------------------------
  GPS NMEA traces | Not available
  -------------------------
  Raw GPS         | Not available
  -------------------------
  CDMA BS         | Not available

Works on my F3607gw and F5521gw.

For my 5321 (non-MBIM firmware):

 # HP hs2350 Mobile Broadband Module
 ATTRS{idVendor}=="03f0", ATTRS{idProduct}=="3d1d", ENV{ID_MM_ERICSSON_MBM}="1"
+ATTRS{idVendor}=="03f0", ATTRS{idProduct}=="3d1d", ENV{.MM_USBIFNUM}=="09", ENV{ID_MM_ERICSSON_MBM_GPS_PORT}="1"

makes it work too; but I have no idea what the MBIM firmware does with USB ports as I don't have a 5321 with MBIM firmware anywhere.

I have no idea where my F3507g is, so I can't test that but I'll assume it works.

(In reply to Dan Williams from comment #13)

> I have no idea where my F3507g is, so I can't test that but I'll assume it
> works.

I tested on that hardware and it does work.

I have now merged to git master Fabrice's patch v3, and then pushed 2 other patches including Sven's and Dan's suggested additional changes.

Thanks to everyone!