mbm plugin gps improvements
Dan Williams
dcbw at redhat.com
Fri Jan 29 07:25:20 PST 2016
On Fri, 2016-01-29 at 01:00 +0100, Tomas Jura wrote:
> Hi Alex
>
> Thanks for the feedback. The patches layout got broken due to my
> email
> client. Before I incorporate your comments I and send it in regular
> for
> review, I created a temporary git repo to see the patches in clear
> format: https://github.com/tomasjura/ModemManager/commits/master
>
> There are two case which probably require to extend the internal
> interface _MMIfaceModemLocation. It is "DGPS" and "GPS refresh time".
> Should be the interface MMIfaceModemLocation extended by
> getters/setters
> for those two variables? I can do write such extension for mbm, but
> what
> about the other plugins? I have no clue if the gobject system
> supports
> an implicit implementation for interface methods. If not then such
> task
> is beyond scope that I could cover. The setup through environment
> variables is can be easily kept on local mbm level without impact to
> other plugins.
>
> For commit "termios setup cleanup" see the line preceding the
> removed
> line. There is the same setup ~(ECHO | ECHOE);.
Maybe change the commit shortlog to read "remove duplicated ECHO &
ECHOE" or something to make it clear exactly what is being done.
> The quick open/close action after the gps unmanaged end (patch #4
> "flush
> unmanaged GPS port after GPS is stopped") serves for draining the
> remaining GPS sentences from the gps port. Without the drain some gps
> sentences can remain in the input buffer and those debris are offered
> as
> an input during the next gps read (for example after few hours).
> Reading
> such trash confuses the reader of GPS sentences. The modes NMEA and
> RAW
> do the drain of the port (during close). My patch just introduce the
> "drain and close" scenario to the unmanaged mode.
>
> The #6 - "Do not restore termios on port close" is a controversial
> patch. It has the wide impact and definitely can introduce nasty
> regressions. It is possible that some code could depend on the
> restore
> of the term setup. But such dependency is IMHO danger itself. In
> other
> words I a piece of code needs a port setup should do it itself and
> not
> blindly rely that the port setup that was somewhat preserved between
> port openings.
Yes, code that uses the serial port should set the serial port up
correctly by itself. And if the other code does that, then it
shouldn't matter whether the termios are left modified or reset back to
previous values. Was the termios restore causing problems somewhere?
Thanks for the patches, some good cleanups!
Dan
> Tomas
>
>
> On 28.1.2016 18:10, Aleksander Morgado wrote:
> > 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
> >
> >
>
> _______________________________________________
> ModemManager-devel mailing list
> ModemManager-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
More information about the ModemManager-devel
mailing list