[PATCH] garmin_gpi: fix proximity alerts for large numbers of isolated waypoints

classic Classic list List threaded Threaded
3 messages Options
maz
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] garmin_gpi: fix proximity alerts for large numbers of isolated waypoints

maz
Large waypoint lists are organized into blocks of at most
WAYPOINTS_PER_BLOCK (128) POIs in generated GPI files for performance
reasons.

It appears that at least on older Garmin Nuvi devices, proximity alerts
often do not work (they do not show up as Custom POIs) and POI lists get
mangled (even to the point of crashing the unit) when there are several
such blocks, particularly with large lists of isolated waypoints.

This problem is the result of wdata_check() which may leave empty
nodes in the tree after sorting waypoints. These nodes show up as empty
blocks in the output file and are not well handled by some devices.

Preventing wdata_compute_size() and wdata_write() from issuing empty
blocks solves the issue.

Fixes: 26840523d411 ("Add support for multiple POI lists to writer code.")

Signed-off-by: maz <[hidden email]>
---
 gpsbabel/garmin_gpi.cc | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gpsbabel/garmin_gpi.cc b/gpsbabel/garmin_gpi.cc
index 17e18f7..cae6f1d 100644
--- a/gpsbabel/garmin_gpi.cc
+++ b/gpsbabel/garmin_gpi.cc
@@ -883,7 +883,10 @@ static int
 wdata_compute_size(writer_data_t* data)
 {
   queue* elem, *tmp;
-  int res;
+  int res = 0;
+
+  if (QUEUE_EMPTY(&data->Q))
+    goto skip_empty_block; /* do not issue an empty block */
 
   res = 23; /* bounds, ... of tag 0x80008 */
 
@@ -1009,6 +1012,8 @@ wdata_compute_size(writer_data_t* data)
     }
   }
 
+skip_empty_block:
+
   if (data->top_left) {
     res += wdata_compute_size(data->top_left);
   }
@@ -1024,6 +1029,9 @@ wdata_compute_size(writer_data_t* data)
 
   data->sz = res;
 
+  if (QUEUE_EMPTY(&data->Q))
+    return res;
+
   return res + 12; /* + 12 = caller needs info about tag header size */
 }
 
@@ -1033,6 +1041,9 @@ wdata_write(const writer_data_t* data)
 {
   queue* elem, *tmp;
 
+  if (QUEUE_EMPTY(&data->Q))
+    goto skip_empty_block; /* do not issue an empty block */
+
   gbfputint32(0x80008, fout);
   gbfputint32(data->sz, fout);
   gbfputint32(23, fout); /* bounds + three bytes */
@@ -1155,6 +1166,8 @@ wdata_write(const writer_data_t* data)
     }
   }
 
+skip_empty_block:
+
   if (data->top_left) {
     wdata_write(data->top_left);
   }
--
1.8.4


------------------------------------------------------------------------------
_______________________________________________
Gpsbabel-code mailing list  http://www.gpsbabel.org
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] garmin_gpi: fix proximity alerts for large numbers of isolated waypoints

Robert Lipe-4
Welcome, maz.

Great find, fix, and explanation.  I'm going back on the road, so I may not get to apply this immediately, but it seems legit.  

Did you run ./testo after applying this?  It seems possible that we may need to regenerate some of our reference POI files, but that may not be the case.

To what does your "fixes" field apply?

On Sat, Aug 15, 2015 at 6:58 AM, maz <[hidden email]> wrote:
Large waypoint lists are organized into blocks of at most
WAYPOINTS_PER_BLOCK (128) POIs in generated GPI files for performance
reasons.

It appears that at least on older Garmin Nuvi devices, proximity alerts
often do not work (they do not show up as Custom POIs) and POI lists get
mangled (even to the point of crashing the unit) when there are several
such blocks, particularly with large lists of isolated waypoints.

This problem is the result of wdata_check() which may leave empty
nodes in the tree after sorting waypoints. These nodes show up as empty
blocks in the output file and are not well handled by some devices.

Preventing wdata_compute_size() and wdata_write() from issuing empty
blocks solves the issue.

Fixes: 26840523d411 ("Add support for multiple POI lists to writer code.")

Signed-off-by: maz <[hidden email]>
---
 gpsbabel/garmin_gpi.cc | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gpsbabel/garmin_gpi.cc b/gpsbabel/garmin_gpi.cc
index 17e18f7..cae6f1d 100644
--- a/gpsbabel/garmin_gpi.cc
+++ b/gpsbabel/garmin_gpi.cc
@@ -883,7 +883,10 @@ static int
 wdata_compute_size(writer_data_t* data)
 {
   queue* elem, *tmp;
-  int res;
+  int res = 0;
+
+  if (QUEUE_EMPTY(&data->Q))
+    goto skip_empty_block; /* do not issue an empty block */

   res = 23;    /* bounds, ... of tag 0x80008 */

@@ -1009,6 +1012,8 @@ wdata_compute_size(writer_data_t* data)
     }
   }

+skip_empty_block:
+
   if (data->top_left) {
     res += wdata_compute_size(data->top_left);
   }
@@ -1024,6 +1029,9 @@ wdata_compute_size(writer_data_t* data)

   data->sz = res;

+  if (QUEUE_EMPTY(&data->Q))
+    return res;
+
   return res + 12;     /* + 12 = caller needs info about tag header size */
 }

@@ -1033,6 +1041,9 @@ wdata_write(const writer_data_t* data)
 {
   queue* elem, *tmp;

+  if (QUEUE_EMPTY(&data->Q))
+    goto skip_empty_block; /* do not issue an empty block */
+
   gbfputint32(0x80008, fout);
   gbfputint32(data->sz, fout);
   gbfputint32(23, fout);       /* bounds + three bytes */
@@ -1155,6 +1166,8 @@ wdata_write(const writer_data_t* data)
     }
   }

+skip_empty_block:
+
   if (data->top_left) {
     wdata_write(data->top_left);
   }
--
1.8.4


------------------------------------------------------------------------------
_______________________________________________
Gpsbabel-code mailing list  http://www.gpsbabel.org
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code


------------------------------------------------------------------------------

_______________________________________________
Gpsbabel-code mailing list  http://www.gpsbabel.org
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
maz
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] garmin_gpi: fix proximity alerts for large numbers of isolated waypoints

maz
Hi Robert,

On Sat, Aug 15, 2015 at 08:06:52PM -0500, Robert Lipe wrote:
> Welcome, maz.
>
> Great find, fix, and explanation.  I'm going back on the road, so I may not
> get to apply this immediately, but it seems legit.

Thanks.

> Did you run ./testo after applying this?  It seems possible that we may
> need to regenerate some of our reference POI files, but that may not be the
> case.

I forgot to check before submitting this patch but it still runs fine. I
think adding a specific test for this case would be a good idea.

> To what does your "fixes" field apply?

It refers to the first 12 digits of the git commit causing the issue and its
title (first line). This is commonly used by projects based on git to keep
track of bugs and their fixes (often seen in Linux kernel commits for
instance).

I didn't know gpsbabel was somewhat new to git (thanks for not switching to
CVS instead!), so more detailed info below.

Usually a user who finds a bug will run git bisect between a known working
version (say, v4.38) and a bad one (v5.42) or even the master's HEAD if the
bug is still present there. This command greatly helps to identify commits
breaking some functionality and can sometimes be automated.

Once the culprit is identified, it's possible to know which versions are
affected (assuming the project is properly tagged - I think you should do it
for gpsbabel) with the following command:

 git describe --contains 0123456789ab

Will return something like "v4.59" so you know all versions between v4.59
and at least v5.42 are affected.

Basically, a "Fixes" line keeps track of the work done to find out the
origin of a bug, and may come with appropriate credits (see
Documentation/SubmittingPatches in the Linux kernel), such as:

 Reported-by: User who found the issue and usually ran git bisect but did
              not work on a patch.
 Tested-by: Third party who was able to test and validated the proposed fix.

Unrelated, you probably already know this but my previous mail has been
formatted with git format-patch and can be applied as is using git am.

> On Sat, Aug 15, 2015 at 6:58 AM, maz <[hidden email]> wrote:
>
> > Large waypoint lists are organized into blocks of at most
> > WAYPOINTS_PER_BLOCK (128) POIs in generated GPI files for performance
> > reasons.
> >
> > It appears that at least on older Garmin Nuvi devices, proximity alerts
> > often do not work (they do not show up as Custom POIs) and POI lists get
> > mangled (even to the point of crashing the unit) when there are several
> > such blocks, particularly with large lists of isolated waypoints.
> >
> > This problem is the result of wdata_check() which may leave empty
> > nodes in the tree after sorting waypoints. These nodes show up as empty
> > blocks in the output file and are not well handled by some devices.
> >
> > Preventing wdata_compute_size() and wdata_write() from issuing empty
> > blocks solves the issue.
> >
> > Fixes: 26840523d411 ("Add support for multiple POI lists to writer code.")
> >
> > Signed-off-by: maz <[hidden email]>
> > ---
> >  gpsbabel/garmin_gpi.cc | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/gpsbabel/garmin_gpi.cc b/gpsbabel/garmin_gpi.cc
> > index 17e18f7..cae6f1d 100644
> > --- a/gpsbabel/garmin_gpi.cc
> > +++ b/gpsbabel/garmin_gpi.cc
> > @@ -883,7 +883,10 @@ static int
> >  wdata_compute_size(writer_data_t* data)
> >  {
> >    queue* elem, *tmp;
> > -  int res;
> > +  int res = 0;
> > +
> > +  if (QUEUE_EMPTY(&data->Q))
> > +    goto skip_empty_block; /* do not issue an empty block */
> >
> >    res = 23;    /* bounds, ... of tag 0x80008 */
> >
> > @@ -1009,6 +1012,8 @@ wdata_compute_size(writer_data_t* data)
> >      }
> >    }
> >
> > +skip_empty_block:
> > +
> >    if (data->top_left) {
> >      res += wdata_compute_size(data->top_left);
> >    }
> > @@ -1024,6 +1029,9 @@ wdata_compute_size(writer_data_t* data)
> >
> >    data->sz = res;
> >
> > +  if (QUEUE_EMPTY(&data->Q))
> > +    return res;
> > +
> >    return res + 12;     /* + 12 = caller needs info about tag header size
> > */
> >  }
> >
> > @@ -1033,6 +1041,9 @@ wdata_write(const writer_data_t* data)
> >  {
> >    queue* elem, *tmp;
> >
> > +  if (QUEUE_EMPTY(&data->Q))
> > +    goto skip_empty_block; /* do not issue an empty block */
> > +
> >    gbfputint32(0x80008, fout);
> >    gbfputint32(data->sz, fout);
> >    gbfputint32(23, fout);       /* bounds + three bytes */
> > @@ -1155,6 +1166,8 @@ wdata_write(const writer_data_t* data)
> >      }
> >    }
> >
> > +skip_empty_block:
> > +
> >    if (data->top_left) {
> >      wdata_write(data->top_left);
> >    }
> > --
> > 1.8.4
> >
> >
> >
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > Gpsbabel-code mailing list  http://www.gpsbabel.org
> > [hidden email]
> > https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
> >

--
Maz

------------------------------------------------------------------------------
_______________________________________________
Gpsbabel-code mailing list  http://www.gpsbabel.org
[hidden email]
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code
Loading...