[PATCH] Change in logic while aplying gps fixes to dives

Dirk Hohndel dirk at hohndel.org
Tue Jul 15 03:55:08 PDT 2014


Since insomnia is rearing its ugly head again I'm actually having time to
look at this...

On Mon, Jul 14, 2014 at 10:11:47AM +0200, Salvador Cuñat wrote:
> We were actually searching dives which match the dowloaded position
> fixes. So we're also trying to take into account if the fix is automatic
> or no based on a limited amount of predefined strings (bad idea, as the
> user can change in companion app settings the predefined string).

Yeah, I wish it wasn't configurable... but maybe your solution will make
that a moot point...

> This way, in actual implementation, if program concludes that a fix has
> been manually got or, simply, the user is unlucky enough to have all the
> position fixes out of the dive time, find_dive_n_near() function will pair
> fix and dive in an ordered way (1st fix -> 1st dive; 2nd fix -> 2nd dive ...)
> which is probably erroneous, except for manual position fixes.

Definitely.

> BTW actual implementation is unable to pair the same gps fix with more than
> one dive (which is most likely to happen).

Can you phrase that differently? I'm not sure I understand.

> The patch changes the logic:
> 
> - Search positions for defined dives (instead of dives for defined positions)
>   without care if position has manually or  automatically been set.
> - It makes two assumptions:
>    a.- If the position fix has been taken during the dive time, is correct.

But it shouldn't replace an existing position in the dive

>    b.- If not during diving time, the correct one is the nearest fix
>       before the dive begins (also the usual case if manually fixed from the
>       smartphone just before jump into the water). But will work too if there
>       is only one fix *in range* after the dive (another usual case).

This does have a relatively short "range", though. Taking a GPS fix from
an hour ago (or yesterday) makes no sense.

> - Finally, as copy_gps_location() in dive.h is used only here, let it take
>   care of naming the dive if user hasn't named it yet.

that will give "automated" names to people - but hey, I always set the
names first and then import...

> diff --git a/dive.h b/dive.h
> index 09a4dda..bb22a3f 100644
> --- a/dive.h
> +++ b/dive.h
> @@ -322,7 +322,11 @@ static inline void copy_gps_location(struct dive *from, struct dive *to)
>  	if (from && to) {
>  		to->latitude.udeg = from->latitude.udeg;
>  		to->longitude.udeg = from->longitude.udeg;
> +		if (!to->location) {
> +			to->location = strdup(from->location);
> +		}
>  	}
> +
>  }

Whitespace... no reason for a new empty line here (I bet you had debugging
output there :-) )

>  static inline int get_surface_pressure_in_mbar(const struct dive *dive, bool non_null)
> diff --git a/qt-ui/subsurfacewebservices.cpp b/qt-ui/subsurfacewebservices.cpp
> index 27a56ec..fe1a5cb 100644
> --- a/qt-ui/subsurfacewebservices.cpp
> +++ b/qt-ui/subsurfacewebservices.cpp
> @@ -31,69 +31,54 @@
[...]
> +	int i, j;
> +	struct dive *gpsfix, *nextgpsfix, *dive;
> +
> +	sort_table(&dive_table);

Umm, why? The table is always sorted. And at this point you haven't made
any changes, right?
> +
> +	for_each_dive (i, dive) {
> +		if (!dive_has_gps_location(dive)) {

*** -> see below

> +			sort_table(&gps_location_table);

Oh no - now you are resorting the gps_location_table for each dive. I have
300+ dives and currently about 1000 fixes in my gps_location_table (as I
kept the companion app running for a week long liveaboard). While on a
recent system that still won't take too long it's just silly - please sort
the gps_location_table just once - up there, instead of sorting the
dive_table :-)

> +			for_each_gps_location (j, gpsfix) {
> +				if (dive_within_time_range (dive, gpsfix->when, SAME_GROUP)) {
> +					/*
> +					 * If position is fixed during dive. This is the good one.
> +					 * Asign and end for_each_gps loop
> +					 */
> +					if ((dive->when <= gpsfix->when && gpsfix->when <= dive->when + dive->duration.seconds)) {
> +						if (!dive_has_gps_location(dive)) {

you only go here if the dive has no location, right? See *** above
so this check is redundant

> +							copy_gps_location(gpsfix,dive);
> +							break;
> +						}
> +					} else {
> +						/*
> +						 * If it is not, check if there are more position fixes.
> +						 * If none this is the last (or the only one) and so the good one.
> +						 * If there is at least one but later than end of dive, the actual fix
> +						 * is OK, asign it and end for_each_gps loop.
> +						 * If next fix is closer or into the dive, it will be evaluated in next
> +						 * loop iteration
> +						 */
> +						if (nextgpsfix = get_gps_location(j+1,&gps_location_table)) {
> +							if ((dive->when + dive->duration.seconds - gpsfix->when) < (nextgpsfix->when - gpsfix->when)) {

I don't understand this condition. So if the distance of this fix from the
end of the dive is less than the distance of the next fix from the
beginning of the dive? Hmm?
Oh, and what if the distance (in time) is two years? Should you still take
the fix? Hint: NO!!!

> +								copy_gps_location(gpsfix,dive);
> +								break;
> +							}
> +						} else {

See above. This needs a time cutoff. Max 30 minutes before or after a
dive.

> +							copy_gps_location(gpsfix,dive);
> +							break;
> +						}
> +					}
>  				}
> -			} else {
> -				struct tm tm;
> -				utc_mkdate(gpsfix->when, &tm);
> -#if DEBUG_WEBSERVICE
> -				printf("didn't find dive matching gps fix named %s @ %04d-%02d-%02d %02d:%02d:%02d\n",
> -				       gpsfix->location,
> -				       tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
> -				       tm.tm_hour, tm.tm_min, tm.tm_sec);
> -#endif
>  			}
>  		}
>  	}
> -	return changed > 0;
>  }

Did I miss something? Did this turn from a bool function to a void
function?

Overall, I think you have a point with your algorithm. Can you work on the
feedback and resubmit? I will definitely consider it.

/D


More information about the subsurface mailing list