Solidify OwnTracks protocol decoder #3249
Conversation
|
Please fix merge issues. I did some cleanup. |
|
I think you added the line which I've removed again because that would cause the code to fail in the (unlikely) event that the JSON doesn't contain |
| @@ -17,6 +17,8 @@ public void testDecode() throws Exception { | |||
| verifyPosition(decoder, request(HttpMethod.POST, "/", | |||
| buffer("{\"cog\":271,\"lon\":2.29513,\"acc\":5,\"vel\":61,\"vac\":21,\"lat\":48.85833,\"tst\":1497349316,\"alt\":167,\"_type\":\"location\",\"tid\":\"JJ\",\"t\":\"u\",\"batt\":67}"))); | |||
|
|
|||
| verifyPosition(decoder, request(HttpMethod.POST, "/", | |||
| buffer(text("{\"lat\":48.85,\"lon\":2.295,\"_type\":\"location\",\"tid\":\"JJ\"}")))); | |||
| } | ||
|
|
||
| long timestamp; | ||
| String deviceId = new String(); |
There was a problem hiding this comment.
Why is there a new String() here. That's just wrong. Also, the name should be uniqueId. We use deviceId for internal device identifier. Also, why are these variables at the top?
|
|
||
| if (root.containsKey("tst")) { | ||
| timestamp = root.getJsonNumber("tst").longValue(); | ||
| if (timestamp < Integer.MAX_VALUE) { |
There was a problem hiding this comment.
I have removed this code. I believe it's unnecessary.
| } | ||
| if (root.containsKey("topic")) { | ||
| deviceId = root.getString("topic"); | ||
| if (root.containsKey("tid")) { |
There was a problem hiding this comment.
We already do this check. You can move it to the code above.
There was a problem hiding this comment.
Not really. If topic exists, we want tid as an attribute, in addition to uniqueId set to topic.
There was a problem hiding this comment.
I'm sure there is a better way to write this code without checking twice.
|
I have, I hope, addressed all your comments. |
|
|
||
| position.setTime(new Date(root.getJsonNumber("tst").longValue() * 1000)); | ||
|
|
||
| DeviceSession deviceSession = getDeviceSession(channel, remoteAddress, root.getString("tid")); | ||
| Boolean haveTopic = root.containsKey("topic"); |
There was a problem hiding this comment.
You shouldn't use boxed primitives. Also, there is no need for variables here at all.
| DeviceSession deviceSession = getDeviceSession(channel, remoteAddress, root.getString("tid")); | ||
| Boolean haveTopic = root.containsKey("topic"); | ||
| Boolean haveTid = root.containsKey("tid"); | ||
| String uniqueId = null; |
There was a problem hiding this comment.
You don't need to assign null. here.
| DeviceSession deviceSession = getDeviceSession(channel, remoteAddress, root.getString("tid")); | ||
| Boolean haveTopic = root.containsKey("topic"); | ||
| Boolean haveTid = root.containsKey("tid"); | ||
| String uniqueId = null; |
There was a problem hiding this comment.
You don't need to assign null. here.
| @@ -80,16 +85,29 @@ protected Object decode( | |||
| if (root.containsKey("t")) { | |||
| position.set("t", root.getString("t")); | |||
| } | |||
| if (root.containsKey("p")) { | |||
| position.set("pb", root.getInt("p")); | |||
There was a problem hiding this comment.
Wait, I just noticed this. What is "pb"?
There was a problem hiding this comment.
pb was atmospheric pressure; it is reported by our iOS app on iPhone >= 6. I've removed it for the time being; it's not so important.
|
It seems like you haven't read my comments carefully enough. Why do you need variables for something that you are using once? |
|
Apologies: I thought the code looked clearer. |
|
Merged, thanks. |
|
Thank you. |
You kindly merged, the new OwnTracks Protocol Decoder, and this pull request attempts to address some things I omitted the first time around. In particular:
lat,lon, and_typeare mandatory."_type" : "location"and uses only those. (OwnTracks has other payload types, e.g. "transition" which probably shouldn't be used in Traccar.)tid). Starting now, if the payload contains ourtopic(e.g.owntracks/jpmens/iphone5s) that will be used in preference to avoid collisions.I hope this is OK.
We're updating our documentation to include Traccar, and that could be the landing for "device type OwnTracks" on your https://www.traccar.org/devices/ page, for example. Just a thought.