mbm plugin gps improvements
Aleksander Morgado
aleksander at aleksander.es
Thu Jan 28 09:10:53 PST 2016
Hey Tomas,
Thanks for the patches! Next time, though, could you send the patches
in a way that we can pick them up independently and easily? The best
way is to use git send-email, which produces one email per patch (and
then we can just save the email and "git am" it). Also, please review
the coding style, it is waaay off w.r.t. the other code :)
>
> I created a set of patches for the mbm plugin. The target goal is to get mbm
> gps unmanaged mode to work cleanly. Basic problems which I was struggling
> with was the refresh time for the gps and termios setup.
>
Ok.
> The gps refresh time is configurable now using the environment variable by
> the first patch. A month ago there appears a patch in trunk
> (6c35878f12ab37604d85cb3a864e3859973bd195) which allows the the set up of
> gps refresh time using the dbus location interface. But as far as I was able
> to infer from code, the requested dbus refresh value is not propagated to
> location interface. ( This is my first meet with the gobject system, and I'm
> perplexed. ). Also I enabled the DGPS, using 3rd parameter of AT*E2GPSCTL.
>
The new method in the API to allow changing the frequency applies only
to the notifications sent through DBus. There are 2 different things
here: the rate at which the modem reports location updates, and the
rate at which the DBus interface reports location updates. The API
method was only for the second one.
Instead of using an envvar, the implementation configuring the modem
should for example request to report location updates with double
frequency (half period) of the rate configured in the DBus interface.
E.g. if the user sets to report location every 10s, the modem should
be configured to report location every 5s. That would be a good
compromise.
For the DGPS change, I think we should do it like we do for AGPS; with
another location source that may be enabled/disabled independently;
i.e. not always enabled by default. If I'm not mistaken, GPS receivers
will need to allocate channels to process DGPS signals, and that may
cause a slower initial acquisition, even if the precision afterwards
is much better; so may not always be desired.
I'd split these two things in different logical patches.
> The rest of the patches deals with the proper gps port setup. The biggest
> problem was echo on both sides ( modem and termios ) which created an
> infinite "echo loop" and a heavy mess in the NMEA messages. Solution of this
> problem lead to a (controversial ?) patch 6 which turns off the restore of
> termios during the port close to keep the port sane and usable. May there
> should be created a new method for port abandon without termios restore. The
> other problems were the freeing the gps port (unlocked) after the switch to
> unmanaged mode (patch 3) and a drain of of the input data buffer after the
> finish of GPS unmanaged mode (patch 4).
>
I'll try to comment on the patches inline below.
>
> From fecbbdca536226447370c1d267507c1310baaa5d Mon Sep 17 00:00:00 2001
> From: Tomas Jura <tomas_jura1 at gmail.com>
> Date: Wed, 2 Dec 2015 21:40:31 +0100
> Subject: [PATCH 1/6] GPS refresh is configurable using GPS_INTERVAL
> environment variable
>
> ---
> plugins/mbm/mm-broadband-modem-mbm.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/plugins/mbm/mm-broadband-modem-mbm.c
> b/plugins/mbm/mm-broadband-modem-mbm.c
> index dc480f8..b24f9a1 100644
> --- a/plugins/mbm/mm-broadband-modem-mbm.c
> +++ b/plugins/mbm/mm-broadband-modem-mbm.c
> @@ -45,7 +45,7 @@
> #include "mm-iface-modem-location.h"
>
> /* sets the interval in seconds on how often the card emits the NMEA
> sentences */
> -#define MBM_GPS_NMEA_INTERVAL "5"
> +#define MBM_GPS_NMEA_INTERVAL 5
>
> static void iface_modem_init (MMIfaceModem *iface);
> static void iface_modem_3gpp_init (MMIfaceModem3gpp *iface);
> @@ -1432,15 +1432,27 @@ parent_enable_location_gathering_ready
> (MMIfaceModemLocation *self,
> }
>
> if (start_gps) {
> + gchar *s_gps_interval = getenv("GPS_INTERVAL");
> + int gps_interval;
> + gchar *buf;
> + if ((s_gps_interval==NULL) || ((gps_interval=atoi(s_gps_interval))
> == 0)) {
> + gps_interval=MBM_GPS_NMEA_INTERVAL;
> + }
As said, the envvar is not an ideal solution. The interval should be
half the one set in the DBus interface.
> + else {
> + if (gps_interval > 60) { gps_interval=60; }
> + if (gps_interval < 1) { gps_interval=MBM_GPS_NMEA_INTERVAL; }
If less than 1, then 5? I guess it should be if less than 1, then 1,
which could be written as:
gps_interval = CLAMP (gps_interval, 1, 60);
> + }
> + buf = g_strdup_printf("AT*E2GPSCTL=1,%d,1",gps_interval);
So the last 1 is to request DGPS. As said earlier, this should go into
another patch, and if possible treated the same way as AGPS (i.e. with
the ability to enable/disable it independently).
> mm_base_modem_at_command_full (MM_BASE_MODEM (self),
> mm_base_modem_peek_port_primary (MM_BASE_MODEM (self)),
> - "AT*E2GPSCTL=1,"
> MBM_GPS_NMEA_INTERVAL ",0",
> + buf,
> 3,
> FALSE,
> FALSE, /* raw */
> NULL, /* cancellable */
> (GAsyncReadyCallback)gps_enabled_ready,
> ctx);
> + g_free(buf);
> return;
> }
>
> --
> 2.7.0
>
>
> From 51952eb611bf29a501f4cc9b48cda092c5f496c0 Mon Sep 17 00:00:00 2001
> From: Tomas Jura <tomas_jura1 at gmail.com>
> Date: Sun, 3 Jan 2016 22:01:42 +0100
> Subject: [PATCH 2/6] stop echos on gps port
>
> ---
> plugins/mbm/mm-broadband-modem-mbm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/plugins/mbm/mm-broadband-modem-mbm.c
> b/plugins/mbm/mm-broadband-modem-mbm.c
> index b24f9a1..778eb6d 100644
> --- a/plugins/mbm/mm-broadband-modem-mbm.c
> +++ b/plugins/mbm/mm-broadband-modem-mbm.c
> @@ -1377,7 +1377,7 @@ gps_enabled_ready (MMBaseModem *self,
> "Couldn't open raw GPS
> serial port");
> } else {
> GByteArray *buf;
> - const gchar *command = "AT*E2GPSNPD\r\n";
> + const gchar *command = "ATE0*E2GPSNPD\r\n";
>
This looks good to me.
> /* We need to send an AT command to the GPS data port to
> * toggle it into this data mode. This is a particularity of
> --
> 2.7.0
>
>
> From 19e4a15ab5f881f6fd4ed9da36e25cb34c3e0aca Mon Sep 17 00:00:00 2001
> From: Tomas Jura <tomas_jura1 at gmail.com>
> Date: Sat, 2 Jan 2016 17:51:41 +0100
> Subject: [PATCH 3/6] fully prepare GPS unmanaged port
>
> ---
> 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 778eb6d..d23c0be 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,
> + GAsyncResult *res,
> + LocationGatheringContext *ctx )
> +{
> + 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,
Ok, so in GPS_UNMANAGED we close the GPS data port once it's setup,
makes sense. But, what I would do is to always provide a
GAsyncReadyCallback, not only when it's UNMANAGED, e.g. a
"gps_enabled_ready" method as in mm-common-cinterion.c, and then only
close the port if UNMANAGED.
> --
> 2.7.0
>
>
> From 840c50cba2996cf97edcb3171787d0b8f8fc3f98 Mon Sep 17 00:00:00 2001
> From: Tomas Jura <tomas_jura1 at gmail.com>
> Date: Sat, 2 Jan 2016 20:22:05 +0100
> Subject: [PATCH 4/6] flush unmanaged GPS port after GPS is stoped
>
> ---
> plugins/mbm/mm-broadband-modem-mbm.c | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/plugins/mbm/mm-broadband-modem-mbm.c
> b/plugins/mbm/mm-broadband-modem-mbm.c
> index d23c0be..62db9d1 100644
> --- a/plugins/mbm/mm-broadband-modem-mbm.c
> +++ b/plugins/mbm/mm-broadband-modem-mbm.c
> @@ -1269,7 +1269,7 @@ gps_disabled_ready (MMBaseModem *self,
> GAsyncResult *res,
> LocationGatheringContext *ctx)
> {
> - MMPortSerialGps *gps_port;
> + MMPortSerialGps *gps_port = NULL;
> GError *error = NULL;
>
> if (!mm_base_modem_at_command_full_finish (self, res, &error))
> @@ -1277,15 +1277,26 @@ gps_disabled_ready (MMBaseModem *self,
> else
> g_simple_async_result_set_op_res_gboolean (ctx->result, TRUE);
>
> - /* Only use the GPS port in NMEA/RAW setups */
> - if (ctx->source & (MM_MODEM_LOCATION_SOURCE_GPS_NMEA |
> - MM_MODEM_LOCATION_SOURCE_GPS_RAW)) {
> - /* Even if we get an error here, we try to close the GPS port */
> + if ( ctx->source & MM_MODEM_LOCATION_SOURCE_GPS_UNMANAGED ) /*
> Unmanaged GPS port will be drained during close */ {
> + gps_port = mm_base_modem_peek_port_gps (self);
> + if (!gps_port || !mm_port_serial_open (MM_PORT_SERIAL (gps_port),
> &error)) {
> + if (error)
> + g_simple_async_result_take_error (ctx->result, error);
> + else
> + g_simple_async_result_set_error (ctx->result,
> + MM_CORE_ERROR,
> + MM_CORE_ERROR_FAILED,
> + "Couldn't open raw GPS
> serial port");
> + }
> + }
> + else if (ctx->source & (MM_MODEM_LOCATION_SOURCE_GPS_NMEA |
> + MM_MODEM_LOCATION_SOURCE_GPS_RAW )) /* need to
> close GPS port in NMEA/RAW setups */ {
> gps_port = mm_base_modem_peek_port_gps (self);
> - if (gps_port)
> - mm_port_serial_close (MM_PORT_SERIAL (gps_port));
> }
>
> + /* Even if we have an error here, we try to close the GPS port */
> + if (gps_port) mm_port_serial_close (MM_PORT_SERIAL (gps_port));
> +
> location_gathering_context_complete_and_free (ctx);
So... in UNMANAGED we re-open the port when disabling and we close it
right away? I don't think this makes much sense. And actually, don't
we need to send the AT*E2GPSCTL=0 command in the GPS data port as we
did with the enabling command?
> }
>
> --
> 2.7.0
>
>
> From f9323fe62fa616b0b770461a809fe6be78091ecc Mon Sep 17 00:00:00 2001
> From: Tomas Jura <tomas_jura1 at gmail.com>
> Date: Sun, 3 Jan 2016 16:29:26 +0100
> Subject: [PATCH 5/6] termios setup cleanup
>
> ---
> src/mm-port-serial.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/src/mm-port-serial.c b/src/mm-port-serial.c
> index 92ad481..fc5d59e 100644
> --- a/src/mm-port-serial.c
> +++ b/src/mm-port-serial.c
> @@ -456,7 +456,6 @@ real_config_fd (MMPortSerial *self, int fd, GError
> **error)
> stbuf.c_iflag &= ~(IGNCR | ICRNL | IUCLC | INPCK | IXON | IXANY );
> stbuf.c_oflag &= ~(OPOST | OLCUC | OCRNL | ONLCR | ONLRET);
> stbuf.c_lflag &= ~(ICANON | ECHO | ECHOE | ECHONL);
> - stbuf.c_lflag &= ~(ECHO | ECHOE);
> stbuf.c_cc[VMIN] = 1;
> stbuf.c_cc[VTIME] = 0;
> stbuf.c_cc[VEOF] = 1;
I'll need more context on this change. This setting is being changed
for every serial port out there, not just for the GPS data port of MBM
modems.
> --
> 2.7.0
>
>
> From f5efa4eeb32d8bf3e8bdeffa457c7671482da67c Mon Sep 17 00:00:00 2001
> From: Tomas Jura <tomas_jura1 at gmail.com>
> Date: Sun, 3 Jan 2016 18:21:07 +0100
> Subject: [PATCH 6/6] Do not restore termios on port close
>
> ---
> src/mm-port-serial.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/src/mm-port-serial.c b/src/mm-port-serial.c
> index fc5d59e..5b04bcd 100644
> --- a/src/mm-port-serial.c
> +++ b/src/mm-port-serial.c
> @@ -1349,7 +1349,6 @@ _close_internal (MMPortSerial *self, gboolean force)
> }
> }
>
> - tcsetattr (self->priv->fd, TCSANOW, &self->priv->old_t);
Same here, I need more context, as this is applying to every serial
port out there.
> tcflush (self->priv->fd, TCIOFLUSH);
> }
>
> --
> 2.7.0
>
>
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list