mbm plugin gps improvements
Tomas Jura
tomas.jura1 at gmail.com
Thu Jan 28 16:00:46 PST 2016
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);.
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.
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
>
>
More information about the ModemManager-devel
mailing list