Add support for Nominatim-based per-road speed limits. #4255
Conversation
|
@tananaev I have a private nominatim instance if you require to use this for testing |
| @@ -40,7 +44,7 @@ public NominatimGeocoder(String url, String key, String language, int cacheSize, | |||
| @Override | |||
| public Address parseAddress(JsonObject json) { | |||
| JsonObject result = json.getJsonObject("address"); | |||
|
|
|||
| JsonObject extrasresult = json.getJsonObject("extratags"); | |||
There was a problem hiding this comment.
This can probably be moved to the place you actually use the variable.
There was a problem hiding this comment.
No problem, will be in next commit
| } | ||
| url += "?format=json&lat=%f&lon=%f&zoom=18&addressdetails=1"; | ||
| url += "reverse?format=json&lat=%f&lon=%f&zoom=16&addressdetails=1&extratags=1"; |
There was a problem hiding this comment.
Zoom 18 takes you to features such as gardens and parks, zoom 16 keeps you at street level.
There was a problem hiding this comment.
Does zoom 16 give you house numbers? That's the reason we are using 18.
There was a problem hiding this comment.
No, but in absence of house numbers and for devices with low accuracy, it will return inaccurate addresses, for example with my test device, was on a road but reported it was in a garden off of the road. Maybe the solution is to add a variable in the config to select this?
There was a problem hiding this comment.
Configuration sounds good to me.
There was a problem hiding this comment.
Will explore this at the weekend
|
|
||
| public class NominatimGeocoder extends JsonGeocoder { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(NominatimGeocoder.class); |
There was a problem hiding this comment.
It was for debugging, will remove.
| @@ -112,6 +145,44 @@ public void failed(Throwable throwable) { | |||
| return null; | |||
| } | |||
|
|
|||
| @Override | |||
| public double getSpeedLimit( | |||
There was a problem hiding this comment.
Why do we need a second method?
There was a problem hiding this comment.
Because we have a second cache for speeds and I couldn't find a good way to return the speed from the original function without changing the type of the callback (original returns a string), which involves changing all of the files that call it.
|
|
||
| void onFailure(Throwable e); | ||
|
|
||
| } | ||
|
|
||
| String getAddress(double latitude, double longitude, ReverseGeocoderCallback callback); | ||
| double getSpeedLimit(double latitude, double longitude, ReverseGeocoderCallback callback); |
There was a problem hiding this comment.
I think getAddress should just return all information.
There was a problem hiding this comment.
getAddress returns a string which is accessed by several other files as mentioned in previous comment. I like the idea of attaching speed limit to position. The other way, which is more caching-friendly is to store lat,lon against a place_id and store the speed limit against place_id. This will reduce netowrk calls overall but will require quite a few more changes to implement, plus changes to the way other files call the geocoder.
| @@ -143,6 +144,14 @@ public OverspeedEventHandler(Config config, DeviceManager deviceManager, Geofenc | |||
| speedLimit = geofenceSpeedLimit; | |||
| } | |||
|
|
|||
| if (Context.getGeocoder() != null) { | |||
There was a problem hiding this comment.
I suggest we store value in position attributes instead of doing network call twice.
There was a problem hiding this comment.
Could be a good way to handle this - how do we access the position object from the geocoder call however? It only passes latitude and longitude.
There was a problem hiding this comment.
You would need to pass more data back from geocoder.
There was a problem hiding this comment.
Okay, I'll look into refactoring at the weekend.
|
This is such a great feature, any idea when it could be merged into master? |
|
It would need a lot of work to merge now. I don't have the time to be able to spend on this at the moment unfortunately. |
|
Ok cool, we will work on implementing this as a web-hook triggered feature.
If we have capacity on our side we will try and see if we can contribute
|
These changes implement the reading of the extratags object in nominatim, which includes maxspeed (Speed limit).
We automatically detect kph or mph and convert to knots, which is checked in the overspeed event.