Silly problem with dive sites and GPS downloading
Linus Torvalds
torvalds at linux-foundation.org
Sat Sep 22 11:17:12 PDT 2018
So here's a patch that fixes a problem with the Garmin GPS downloading,
but I'm not entirely sure what the _real_ fix is.
Let me explain the background first.
Each dive has a dive site, and the dive site is identified with an uuid.
Now, I happen to think that's a horrible design, but it does mean that you
can just pick a dive site for a dive, and then if you have ten different
dives on the same dive site, you can edit the dive site, and the dive site
information will be changed for all ten dives, because you're editing not
the dive, but the information about the site.
In contrast, what we *used* to do was to just put the dive site data into
the dive itself, and if you edited the dive site, it would only change for
that one dive. You could still pick the same dive site for another dive,
but all that would do was to copy the dive site data at the time you
picked it - if you fixed the GPS coordinates (or the name) later, it would
change only for the particular dive you edited.
So there's a very real reason we do that indirection, and the uuid has all
kinds of theoretical advantages (if we actually had a better model of
managing it), but the uuid has actually caused a huge amount of problems,
and in my opinion it has created more problems than it ever was supposed
to solve.
As an example of one of the problems it created (long ago) was that if you
imported the same dive data twice, you'd get different uuid's for the dive
site data, and then you'd get really nasty conflicts when you merged
things. For example, maybe in one case you just had the name, but then in
the other import you did a better job and added GPS location too, but now
both copies of your dive has a dive site, and it's entirely unclear which
is the better data.
Our merge strategy (when you have dives on two different machines, because
you use a laptop _and_ a cellphone, for example) is *not* semantically
aware, because that would be a nightmare, it's just a line-by-line git
merge. Which completely breaks down when you have two lines that are
different just because they have two different uuid's on them.
So things like uuids are fundamentally bad for things like that.
The way we fixed the nastiest of the merge problem was to just say "ok,
the uuid isn't actually random when we create it, it's calculated as a
hash of the dive site name and the time of the dive". That didn't "fix"
the problem, but at least it meant that if you imported the same dive data
twice from an external source, you at least got the exact same uuids and
didn't get nasty merge conflicts just because you happened to import it
twice.
HOWEVER.
We now have a new problem exactly *because* we don't actually use a random
uuid, but generate it based on divetime and name. It's related to the fact
that the libdivecomputer download can now create new dive sites
automatically if the dive computer supports GPS data for the dive. Right
now that only affects the Garmin Descent Mk1, but I really hope other dive
computers will follow suit in the future, because the GPS location really
is very very convenient, and it's by far my favorite feature of the
Descent Mk1 (which is otherwise a fairly "Meh" dive computer).
What happens is that you download your dives for the day, and you are
reall yhappy to just get the location data automatically. But the Garmin
doesn't know the _name_ of the dive site, obviously, it just has the GPS,
so you edit the name, and you're all done. Great. No problem.
Now, you're on a dive vacation, and the end of the *next* day, you
download the new dives for _that_ day, and you get all the GPS data for
the new dives too. Everything is fine, right?
No, not everything is fine. Because what happens is that the
libdivecomputer download doesn't just download the new dives, it starts
downloading _all_ the dives from the Garmin Descent, and parses them, and
in the process eventually notices "Oh, I already had this dive", and
that's when it stops downloading.
That still sounds fine, right? You never see the old dive, because we
noticed it was old, so it doesn't matter.
Wrong again. As part of parsing the dive download, it obviously parses the
GPS data, and it generates the dive site information for the GPS data.
And this happens REGARDLESS of whether the downloaded dive is actually
used or not.
And, in fact, because we use the same name, and the same dive time, we are
guaranteed to create the same dive site uuid when we do this. See above
about *why* we very intentionally do this. So when we download a dive -
whether we will actually *use* that dive later or not - we will be filling
in the dive site information with the data we got from the dive computer.
... and in the process we will be overwriting the data that we filled in
manually yesterday. The name of the dive site, but also possibly even the
GPS of the dive site (maybe the user decided to edit that using the map,
because while the automatically downloaded GPS data was "correct", maybe
the user wanted to change it to be the actual under-water location using
the satellite data, rather than the place where you started the dive or
where you surfaced.
There are a few obvious solutions to this mess:
a) the uuid approach and indirection just isn't worth it.
b) just make the libdivecomputer download not use the dive time, but
"time of download" of something for the dive site time, and thus
effectively generate a new uuid for every download.
c) notice when we already have a pre-existing matching uuid, and just use
the old information for the newly downloaded dive.
Honestly, (a) has been my opinion for years now. It's a pain. We've never
had a good dive site editing model. The indirection has theoretical
advantages that we simply don't take advantage of, and it has real
disadvantages that have shown up many times.
However, while I'd personally prefer (a), it's not actually practical. We
have a lot of UI code built up around it that I'm not going to touch, and
maybe some day we can actually make use of the dive site indirection.
There is *some* small existing advantage too (that whole "shared dive site
description" thing), and people probably use it.
So (a) is out of the picture, I just wanted to mention it because it would
be my "in a perfect world" solution.
(b) is what the patch below actually implements. It will cause merge
conflicts if you download the same dive on two different machines without
having done a cloud sync in between, but honestly, you'll probably get
those merge conflicts anyway, and it shouldn't be fatal.
(c) is the other approach that would make sense, but the way the dive site
code is organized, it's just simply a more painful patch. It might be the
better approach, though.
I've added my sign-off to the patch, although the above explanation should
be made into a commit log entry to explain it.
Comments?
Linus
PS. We have another pending problem with the dive site situaiton: the
Garmin Descent Mk1 gives us both entry and exit coordinates, and having
done four drift dives with it, I actually really *would* like to have our
mapping to show it as not a flag, but as a line between two points. But
right now we only associate a single GPS coordinate with the dive, and
because the Garmin reliably gives an exit coordinate but not an entry one
(if you jump into the water before it gets a GPS fix), the downloader will
just use the single exit point for the automatic dive site.
But that pending problem is an enhancement, not a bug.
---
Signed-off-by: Linus Torvalds <torvalds at linux-foundation.org>
core/libdivecomputer.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/core/libdivecomputer.c b/core/libdivecomputer.c
index 2b535dfaf..fcc774425 100644
--- a/core/libdivecomputer.c
+++ b/core/libdivecomputer.c
@@ -12,6 +12,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
+#include <time.h>
#include "gettext.h"
#include "dive.h"
#include "subsurface-string.h"
@@ -615,7 +616,7 @@ static void parse_string_field(struct dive *dive, dc_field_string_t *str)
longitude = parse_degrees(line, &line);
if (latitude.udeg && longitude.udeg)
- dive->dive_site_uuid = create_dive_site_with_gps(str->value, latitude, longitude, dive->when);
+ dive->dive_site_uuid = create_dive_site_with_gps(str->value, latitude, longitude, time(NULL));
}
}
#endif
More information about the subsurface
mailing list