Feature: Start/stop drawing of the track (Patch included)

[Florian Schafferhans]

Hello there!

In the current version 1.1.1 FoxtrotGPS will draw a track on the map. I'm not speaking about the track logging, but the red line that is drawn even when track logging is off.

I'd like to have the possibility to turn this behaviour off from time to time. So far I didn't find a way to do so in the original version. So I patched it as included.

For my needs I somehow figured , that this is closely related to the features of the trip meter. So, with the patch applied FoxtrotGPS will stop drawing the track when the "Stop" button in the "Trip Meter" pane is pressed. It will resume drawing the track when the "Resume" button in the "Trip Meter" pane is pressed. The part of the track between the point where the drawing was stopped and the point where it was resumed will not be drawn. And pressing the "Reset" button in the "Trip Meter" will clear all these drawings off the map.

As already mentioned the patch is against version 1.1.1 (the tar ball downloadable on the main page). I compiled and run it on an OpenBSD 5.3 (including some OpenBSD specific patches [1]), but I hope it should work anywhere else (possibly some other platform specific patches may be needed of course).

I've been testing this now for quite some time and so far it works well for me. So I thought, maybe you folks may like the idea, why not sharing it with you.

Cheers!

[1] http://www.openbsd.org/cgi-bin/cvsweb/ports/geo/foxtrotgps/patches/

[Joshua Judson Rosen (rozzin)]

It's interesting--I've occasionally gone and hit that button myself and been surprised that it didn't already do exactly what you implemented. I wonder if anyone thinks differently :)

[Joshua Judson Rosen (rozzin)]

Rebasing on top of trunk, and doing some trivial whitespace cleanup to make the diff a little more readable....

Shouldn't we be calling g_queue_free_full rather than g_queue_free to avoid leaking the g_new0'd trackpoint_t objects? There's not a race or anything that we're avoiding by not freeing them, is there?

Also, I don't quite understand what you're doing with the NULL sentinels in tracks.c and callbacks.c--can you provide some comments?

[Florian Schafferhans (fsos)]

Hi and thanks for your feedback!

I guess you're right about g_queue_free_full vs. g_queue_free. I must admit, that I missed that one as I was working with apparently outdated documentation not mentioning the existence of g_queue_free_full. But of course we should free the track point objects as well. I changed that in the attached patch, please check it.
Or should we probably even only free them and shrink the list rather than completely free the list and reallocate memory for list? Would that be any faster or had other advantages? I just chose it as an easy and quick way...

About the possible races and stuff... Well, to be honest I didn't think about that, good point of yours!
Now, as you mentioned it, I realised that the parts accessing the trip meter variables apparently aren't synchronised but maybe really should be!? Without the access on the list track point this may have only produced weird displays. (What happens if, in the middle of resetting the four trip meter variables in on_button8_clicked(), say after setting the first two of them to zero, cb_gps_timer() changes them? If that scenario was possible, I assume, the first two have non-zero values the last two have zero values, thus overall an inconsistent state.) But it can then, involving the track point list, in deed possibly even lead to access on freed memory or things like that, I guess. At least some thoughts on synchronising that cannot hurt, can it?
As far as I understand it, apart from any initialisation code, we have five functions to worry about conflicts accessing the trip meter variables (as they are trip_counter_on, trip_maxspeed, trip_time, trip_starttime, trip_distance and trackpoint_list):
(1) on_button8_clicked() in callbacks.c,
(2) on_button15_clicked() in callbacks.c,
(3) cb_gps_timer() in gps_fucntions.c,
(4) set_label() in gps_functions.c
and (5) paint_track() in tracks.c.
I thought about introducing a mutex to have them synchronised. However, that might need some thoughts on possible deadlocks. As far as I understand, that will at least require to reorder the code in cb_gps_timer() to get the critical section(s) together and eliminate some unhandy interleaving -- (a) do the calculations, then (b) update the variables and (c) do all the repainting stuff (like possibly calling set_mapcenter(), which will call paint_track(), calling set_label(), ...); lock the mutex before (b) and unlock it afterwards as parts of (c) might also need to lock it for themselves (e.g. paint_track(), which could also be called from elsewhere hence needs its own lock and gets called in cb_gps_timer() via calling set_mapcenter()). Could that be a feasible way in your opinion? I could try to get my hands on that then...

I use the NULLs in the list of track points as a stop marker. Hence, whenever there's a NULL, everything left of it belongs to one section of the track, everything right of it to another section. As there we're no sectioning of the track in the code before all the NULL stuff is an extension to handle them now (plus one sort of fix also affecting the handling with only one section as it is now in the release). I inserted some comments in the attached patch hopefully giv...

[Florian Schafferhans (fsos)]

Here's how I think this could be solved in a synchronising fashion with a mutex.

In cb_gps_timer() the section centring the map had to go down, as set_mapcenter() calls paint_track() which needs to lock the mutex on its own.