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

Discussion:

maz

2015-08-15 11:58:06 UTC

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 <***@p0d.org>
---
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

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

Robert Lipe

2015-08-16 01:06:52 UTC

Permalink

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?

Post by 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.")
---
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)
}
}
+
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)
}
}
+
if (data->top_left) {
wdata_write(data->top_left);
}
--
1.8.4
------------------------------------------------------------------------------
_______________________________________________
Gpsbabel-code mailing list http://www.gpsbabel.org
https://lists.sourceforge.net/lists/listinfo/gpsbabel-code

maz

2015-08-16 11:11:12 UTC

Permalink

Hi Robert,

Post by Robert Lipe
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.

Post by Robert Lipe
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.

Post by Robert Lipe
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.

Post by Robert Lipe

--
Maz

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