The speed is computed by using previous and current location in function gclue_location_set_speed_from_prev_location(). However this may cause a division by zero error when both locations have the same timestamp.
Created attachment 114787 [details] [review] location: More sanity checks when computing speed This patch adds a timestamp value test before computing the speed.
Also, it may be more interesting to use data from $GPRMC data http://aprs.gids.nl/nmea/#rmc to report speed (speed over ground, knots). NMEA updates from ModemManager over DBUS are scheduled every 30 seconds, even if the NMEA stream from hardware is updated more frequently. Processing info from the previous location will provide an average speed over the last 30 secs, while $GPRMC data may provide an instantaneous speed every 30 secs.
Comment on attachment 114787 [details] [review] location: More sanity checks when computing speed Review of attachment 114787 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location.c @@ +267,5 @@ > > timestamp = geocode_location_get_timestamp (gloc); > prev_timestamp = geocode_location_get_timestamp (prev_gloc); > + if (prev_timestamp == timestamp) { > + location->priv->speed = GCLUE_LOCATION_SPEED_UNKNOWN; If the timestamp is the same, that likely means a more accurate location (another patch from you already takes care of the duplicate locations?) so I don't think we should declare it unknown but rather in Locator, we should check for this and re-calculate speed based on previous_location (we currently don't keep that around but i guess we should).
(In reply to Fabrice Bellet from comment #2) > Also, it may be more interesting to use data from $GPRMC data > http://aprs.gids.nl/nmea/#rmc to report speed (speed over ground, knots). > > NMEA updates from ModemManager over DBUS are scheduled every 30 seconds, > even if the NMEA stream from hardware is updated more frequently. Processing > info from the previous location will provide an average speed over the last > 30 secs, while $GPRMC data may provide an instantaneous speed every 30 secs. Good idea. Could you please open a bug for that so we don't forget about it?
Created attachment 114873 [details] log file #1 generating a division by zero This is a first condition that generates a division by zero error. I added some debugging output to display some objects and interesting values. The problem, as I understand it, is that the timestamp is _only_ set when the object is initialized, never after. Strictly speaking, this timestamp tells when the object has been created, not when the coordinates values were considered. So in function gclue_location_source_set_location(), when we copy location information from the GClueLocation object to priv->location, all information is updated, except timestamp (its a readonly prop). The consequence, on each new GGA (distinct) sentence, is that the same timestamp values are always compared again and again in gclue_location_set_speed_from_prev_location() I could work around this behaviour, by deleting priv->location before recreating a new one in gclue_location_source_set_location().
Created attachment 114874 [details] log file #2 generating a division by zero This second log file (after the previous workaround has been applied) still shows a devision by zero error. The case corresponds to what you describe in your comment, where two different sources (GClue3G and GClueWifi) update the location in the same second.
Moreover, the *meaning* of a speed based on two consecutive locations coming from two different sources (or just with two different accuracies) should be considered with great care at best.
Created attachment 114924 [details] [review] location-source: Always create new location instance The timestamp on location is only set on it during creation so if we re-use the same location object, we'll have the same timestamp on every new location and hence we won't be able to calculate speed by comparing the new and old locations. Besides the timestamp is supposed to represent the time when location was acquired so keeping it the same throughout the lifetime of geoclue, is just wrong.
Created attachment 114926 [details] [review] location: Avoid a division by zero Don't calculate (and therefore update) speed if timestamp on both locations is the same.
These patches I attached, alone won't do the trick but I'm on this now. More patches coming soon..
Created attachment 114958 [details] [review] locator: Move speed/heading setting to source Move calculation and setting of speed and heading to gclue_location_source_set_location(). We ensure that speed and heading are only calculated if they are not already set on the new location object so that they are not re-calculated each time location is set from one source to another (keeping in mind that Locator is also a LocationSource subclass). This change would mean that speed and heading are only set by the source it originates from.
Created attachment 114959 [details] [review] location-source: No speed calculation if same timestamps If timestamps on new and existing location are the same, it doesn't make sense to calculate speed. Also if it wasn't for the guard in gclue_location_set_speed_from_prev_location() for such situation, it will end up with a "division by zero".
Please test/review these patches I have attached.
Created attachment 114963 [details] [review] locator: Move speed/heading setting to source Move calculation and setting of speed and heading to gclue_location_source_set_location(). We ensure that speed and heading are only calculated if they are not already set on the new location object so that they are not re-calculated each time location is set from one source to another (keeping in mind that Locator is also a LocationSource subclass). This change would mean that speed and heading are only set by the source it originates from. i-e speed and heading are not calculated by comparing location from two different sources, which would typically be wrong as different locations from different sources just means the same location but with different accuracies. For example, WiFi source will put me at Crystal Palace but soon after GPS will put me at Westow House (a pub in Crystal Palace) but I'm not moving so deriving speed and heading from these two locations would be wrong. As a positive side-effect, this would make it possible for sources to provide the speed and accuracy themselves. I'm not sure that is possible even with GPS but you never know what the future holds.
Created attachment 114964 [details] [review] location-source: No speed calculation if same timestamps If timestamps on new and existing location are the same, it doesn't make sense to calculate speed. Also if it wasn't for the guard in gclue_location_set_speed_from_prev_location() for such situation, it will end up with a "division by zero".
As a new GClueLocation object is created in priv->location each time we execute gclue_location_source_set_location(), I think cur_location probably should be unref()ed at the end of this function to avoid this memory leak ? I made a test with both a GClueWifi and a GClue3G source, and speed/heading seems to be computed by the GClueLocator object anyway ? Here is a debugging log, with a breakpoint at the end of gclue_location_source_set_location(), before the final g_object_notify, I print the following variables : print source print *priv->location->priv print *priv->location->parent_instance->priv if cur_location != 0 print *cur_location->priv print *cur_location->parent_instance->priv else print "cur_location = 0" end I also edited the lat/lon coordinates for privacy reason, the 3G coordinates have an accuracy of 5000m, and the wifi coordinates have an accuracy of 100m. The where-am-i client output : [bellet@gandi-2 geoclue (master *%)]$ demo/where-am-i -a 8 -t 3600 Client object: /org/freedesktop/GeoClue2/Client/1 New location: Latitude: xx.xxxxxx Longitude: x.xxxxxx Accuracy (in meters): 5000.000000 New location: Latitude: yy.yyyyyy Longitude: y.yyyyyy Accuracy (in meters): 100.000000 Speed (in meters per second): 100.946811 Heading (in degrees): 63.842112 ^C The gdb session of the geoclue server : (geoclue:31709): Geoclue-DEBUG: WiFi AP 'FreeWifi_secure' added. (geoclue:31709): Geoclue-DEBUG: WiFi AP 'FreeWifi_secure' added. [Thread 0x7fffed7b6700 (LWP 31723) exited](geoclue:31709): Geoclue-DEBUG: Got following response from 'https://location.services.mozilla.com/v1/geolocate?key=geoclue': {"location": {"lat": xx.xxxxxxx, "lng": x.xxxxx}, "accuracy": 5000.0} Breakpoint 6, gclue_location_source_set_location (source=0x6568c0 [GClue3G], location=0x719cf0 [GClueLocation]) at gclue-location-source.c:307 $44 = 0x6568c0 [GClue3G] $45 = { speed = -1, heading = -1 } $46 = { longitude = x.xxxxxx, latitude = xx.xxxxxxx, altitude = -1.7976931348623157e+308, accuracy = 5000, timestamp = 1428519895, description = 0x0, crs = GEOCODE_LOCATION_CRS_WGS84 } $47 = "cur_location = 0" (geoclue:31709): Geoclue-DEBUG: New location available Breakpoint 6, gclue_location_source_set_location (source=0x68ea20 [GClueLocator], location=0x719d70 [GClueLocation]) at gclue-location-source.c:307 $48 = 0x68ea20 [GClueLocator] $49 = { speed = -1, heading = -1 } $50 = { longitude = x.xxxxxx, latitude = xx.xxxxxxx, altitude = -1.7976931348623157e+308, accuracy = 5000, timestamp = 1428519895, description = 0x0, crs = GEOCODE_LOCATION_CRS_WGS84 } $51 = "cur_location = 0" (geoclue:31709): Geoclue-DEBUG: New 3GPP location is same as last one (geoclue:31709): Geoclue-DEBUG: No GGA trace [Thread 0x7fffdffff700 (LWP 31725) exited] [Thread 0x7fffecfb5700 (LWP 31724) exited] [Thread 0x7fffdf7fe700 (LWP 31726) exited](geoclue:31709): Geoclue-DEBUG: Refreshing location.. (geoclue:31709): Geoclue-DEBUG: Network available (geoclue:31709): Geoclue-DEBUG: Sending following request to 'https://location.services.mozilla.com/v1/geolocate?key=geoclue': <<<some wifi macaddresses here>>> {"location": {"lat": yy.yyyyyyy, "lng": y.yyyyyyy}, "accuracy": 100.0} Breakpoint 6, gclue_location_source_set_location (source=0x654de0 [GClueWifi], location=0xb045e0 [GC lueLocation]) at gclue-location-source.c:307 $52 = 0x654de0 [GClueWifi] $53 = { speed = -1, heading = -1 } $54 = { longitude = y.yyyyyyy, latitude = yy.yyyyyyy, altitude = -1.7976931348623157e+308, accuracy = 100, timestamp = 1428519896, description = 0x0, crs = GEOCODE_LOCATION_CRS_WGS84 } $55 = "cur_location = 0" (geoclue:31709): Geoclue-DEBUG: New location available Breakpoint 6, gclue_location_source_set_location (source=0x68ea20 [GClueLocator], location=0x90d780 [GClueLocation]) at gclue-location-source.c:307 $56 = 0x68ea20 [GClueLocator] $57 = { speed = 100.94681057838022, heading = 63.842111938757654 } $58 = { longitude = y.yyyyyyy, latitude = yy.yyyyyyy, altitude = -1.7976931348623157e+308, accuracy = 100, timestamp = 1428519896, description = 0x0, crs = GEOCODE_LOCATION_CRS_WGS84 } $59 = { speed = -1, heading = -1 } $60 = { longitude = x.xxxxxxx, latitude = xx.xxxxxxx, altitude = -1.7976931348623157e+308, accuracy = 5000, timestamp = 1428519895, description = 0x0, crs = GEOCODE_LOCATION_CRS_WGS84 } Program received signal SIGINT, Interrupt. 0x00007ffff6762c8d in poll () at ../sysdeps/unix/syscall-template.S:81 When called by the GClueLocator, it seems to me that the speed/heading is calculated even when it concerns two different sources, and this is what is returned to the client ?
(In reply to Fabrice Bellet from comment #16) > As a new GClueLocation object is created in priv->location each time we > execute gclue_location_source_set_location(), I think cur_location probably > should be unref()ed at the end of this function to avoid this memory leak ? Yeah. Thanks. > I made a test with both a GClueWifi and a GClue3G source, and speed/heading > seems to be computed by the GClueLocator object anyway ? Here is a debugging > log, with a breakpoint at the end of gclue_location_source_set_location(), > before the final g_object_notify, I print the following variables : Thanks for your very thorough testing and yeah it seems that since speed/heading are unknown to sources themselves for the first location, locator takes the opportunity to compute that. I'm not exactly sure if this really is an issue though. We already added the division by zero checks in place and its not to bad to have a wrong heading/speed on just the first location (at least it'll be better than the current state of being always wrong if last two locations came from two different sources). Maybe instead of these props being initially unknown, they could just be defined to some sane defaults (i-e 0.0) instead? The log would need fixing anyway. BTW, you might want to take advantage of inline (on the patch, through the Review link) comments feature in bugzilla. :)
(In reply to Zeeshan Ali from comment #17) > (In reply to Fabrice Bellet from comment #16) > > Maybe instead > of these props being initially unknown, they could just be defined to some > sane defaults (i-e 0.0) instead? Oh never mind this suggestion. If we do this, gclue_location_source_set_location() will have no way of deciding whether or not to compute these properties. I think we need to add a boolean property 'compute-movement' (or a better name) that dictates whether a source should compute these props or not. Locator will set this to FALSE on itself.
Created attachment 114981 [details] [review] location-source: Always create new location instance The timestamp on location is only set on it during creation so if we re-use the same location object, we'll have the same timestamp on every new location and hence we won't be able to calculate speed by comparing the new and old locations. Besides the timestamp is supposed to represent the time when location was acquired so keeping it the same throughout the lifetime of geoclue, is just wrong.
Created attachment 114982 [details] [review] location: Avoid a division by zero Don't calculate (and therefore update) speed if timestamp on both locations is the same.
Created attachment 114983 [details] [review] locator: Move speed/heading setting to source Move calculation and setting of speed and heading to gclue_location_source_set_location(). We ensure that speed and heading are only calculated if they are not already set on the new location object so that they are not re-calculated each time location is set from one source to another (keeping in mind that Locator is also a LocationSource subclass). This change would mean that speed and heading are only set by the source it originates from. i-e speed and heading are not calculated by comparing location from two different sources, which would typically be wrong as different locations from different sources just means the same location but with different accuracies. For example, WiFi source will put me at Crystal Palace but soon after GPS will put me at Westow House (a pub in Crystal Palace) but I'm not moving so deriving speed and heading from these two locations would be wrong. As a positive side-effect, this would make it possible for sources to provide the speed and accuracy themselves. This would be at least possible for GPS (see bug#89907). Known issue: We still end up with these properties being computed by Locator by comparison of locations from two different sources if the latest location is the first location from a source. We solve this issue in a following patch.
Created attachment 114984 [details] [review] location-source: No speed calculation if same timestamps If timestamps on new and existing location are the same, it doesn't make sense to calculate speed. Also if it wasn't for the guard in gclue_location_set_speed_from_prev_location() for such situation, it will end up with a "division by zero".
Created attachment 114985 [details] [review] location-source: Add 'compute-movement' property Add a boolean property, 'compute-movement' that dictates whether or not heading and speed are automatically computed and set on new locations by the source.
Created attachment 114986 [details] [review] locator: Don't auto-compute speed & heading Now that the actual sources themselves ensure that speed and heading are computed and set on new locations whenever possible, there is no reason for Locator to do this at all anymore. Otherwise, we still end up with these properties being computed by comparison of locations from two different sources if the latest location is the first location from a source.
Comment on attachment 114981 [details] [review] location-source: Always create new location instance Review of attachment 114981 [details] [review]: ----------------------------------------------------------------- It works for me. The speed and heading is now computed between locations coming from the same source, and the case of the initial location per-source is taken into account. And of course the initial division-by-zero problem is resolved. That's great, thanks ! ::: src/gclue-location-source.c @@ +280,5 @@ > "heading", gclue_location_get_heading (location), > NULL); > > g_object_notify (G_OBJECT (source), "location"); > + g_clear_object (&cur_location); The cur_location variable is introduced in a another later patch.
Comment on attachment 114981 [details] [review] location-source: Always create new location instance Review of attachment 114981 [details] [review]: ----------------------------------------------------------------- Thanks for all your testing! ::: src/gclue-location-source.c @@ +280,5 @@ > "heading", gclue_location_get_heading (location), > NULL); > > g_object_notify (G_OBJECT (source), "location"); > + g_clear_object (&cur_location); Oh! Nice catch. Thanks!
Created attachment 114993 [details] [review] location-source: Always create new location instance The timestamp on location is only set on it during creation so if we re-use the same location object, we'll have the same timestamp on every new location and hence we won't be able to calculate speed by comparing the new and old locations. Besides the timestamp is supposed to represent the time when location was acquired so keeping it the same throughout the lifetime of geoclue, is just wrong.
Created attachment 114994 [details] [review] location: Avoid a division by zero Don't calculate (and therefore update) speed if timestamp on both locations is the same.
Created attachment 114995 [details] [review] locator: Move speed/heading setting to source Move calculation and setting of speed and heading to gclue_location_source_set_location(). We ensure that speed and heading are only calculated if they are not already set on the new location object so that they are not re-calculated each time location is set from one source to another (keeping in mind that Locator is also a LocationSource subclass). This change would mean that speed and heading are only set by the source it originates from. i-e speed and heading are not calculated by comparing location from two different sources, which would typically be wrong as different locations from different sources just means the same location but with different accuracies. For example, WiFi source will put me at Crystal Palace but soon after GPS will put me at Westow House (a pub in Crystal Palace) but I'm not moving so deriving speed and heading from these two locations would be wrong. As a positive side-effect, this would make it possible for sources to provide the speed and accuracy themselves. This would be at least possible for GPS (see bug#89907). Known issue: We still end up with these properties being computed by Locator by comparison of locations from two different sources if the latest location is the first location from a source. We solve this issue in a following patch.
Created attachment 114996 [details] [review] location-source: No speed calculation if same timestamps If timestamps on new and existing location are the same, it doesn't make sense to calculate speed. Also if it wasn't for the guard in gclue_location_set_speed_from_prev_location() for such situation, it will end up with a "division by zero".
Created attachment 114997 [details] [review] location-source: Add 'compute-movement' property Add a boolean property, 'compute-movement' that dictates whether or not heading and speed are automatically computed and set on new locations by the source.
Created attachment 114998 [details] [review] locator: Don't auto-compute speed & heading Now that the actual sources themselves ensure that speed and heading are computed and set on new locations whenever possible, there is no reason for Locator to do this at all anymore. Otherwise, we still end up with these properties being computed by comparison of locations from two different sources if the latest location is the first location from a source.
Comment on attachment 114981 [details] [review] location-source: Always create new location instance Review of attachment 114981 [details] [review]: ----------------------------------------------------------------- ::: src/gclue-location-source.c @@ +280,5 @@ > "heading", gclue_location_get_heading (location), > NULL); > > g_object_notify (G_OBJECT (source), "location"); > + g_clear_object (&cur_location); Fixed this now. Could you please quickly check if i didn't mess things up with that? I'd like to push this so I can roll out a release soon.
Yes, it looks good to me with this new set of patches.
Attachment 114993 [details] pushed as 29bd580 - location-source: Always create new location instance Attachment 114994 [details] pushed as b3455a5 - location: Avoid a division by zero Attachment 114995 [details] pushed as a5596dd - locator: Move speed/heading setting to source Attachment 114996 [details] pushed as b97401c - location-source: No speed calculation if same timestamps Attachment 114997 [details] pushed as 0f24abe - location-source: Add 'compute-movement' property Attachment 114998 [details] pushed as cd84f82 - locator: Don't auto-compute speed & heading
(In reply to Fabrice Bellet from comment #34) > Yes, it looks good to me with this new set of patches. Thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.