Closed Bug 584015 Opened 9 years ago Closed 9 years ago

gpsd has changed protocol, GPSDGeolocationProvider is broken

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla5

People

(Reporter: lultimouomo, Assigned: lultimouomo)

References

()

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.8) Gecko/20100729 Iceweasel/3.6.7 (like Firefox/3.6.7)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.8) Gecko/20100729 Iceweasel/3.6.7 (like Firefox/3.6.7)

gpsd has switched to a new incompatible protocol since version 2.90.
This completely breaks the gpsd interface in xulrunner.
The new protocol also offers a convenient JSON format; the attached patch uses that instead of parsing NMEA sentences.


Reproducible: Always

Steps to Reproduce:
1. Install a recent version of gpsd
2. try to use geolocation and see that firefox falls back on WiFi or IP
Status: UNCONFIRMED → NEW
Ever confirmed: true
You should code/create a Patch against Mozilla-Central - not a Branch (unless Branch-only is intended).
Then you need to request Review of you Patch from a Peer of that Module.
Since http://www.mozilla.org/about/owners.html does not mention Geolocation, i'd try asking Doug Turner, who fixed quite some Geolocation Bugs in the Past.
oh, cool.

what about older gpsd implementations that do not support WATCH
(In reply to comment #2)
> You should code/create a Patch against Mozilla-Central - not a Branch (unless
> Branch-only is intended).


Hi, GPSDGeolocationProvider.js is present only in mozilla1.9.2 (not even in mozilla2.0)
The patch against mozilla-central would be the whole file...

> Then you need to request Review of you Patch from a Peer of that Module.
> Since http://www.mozilla.org/about/owners.html does not mention Geolocation,
> i'd try asking Doug Turner, who fixed quite some Geolocation Bugs in the Past.

Fine, I see Doug is already CCed
(In reply to comment #3)
> oh, cool.
> 
> what about older gpsd implementations that do not support WATCH

From what I read here[0] it wasn't already working with the old protocol.
Anyway, immediately after opening the socket gpsd reports its version (new ones do at least), so it would be possible to differentiate the behaviour according to it.
But considering that:
- I can't find any documentation about the old protocol, so I don't know how we should deal with it
- The old protocol is deprecated and disappearing soon
- Probably gpsd geolocation is already broken anyway

IMHO it would be cleaner to drop the support for the old protocol.
For now I've attached a new patch that deals better with incomplete GPS data.

[0] http://old.nabble.com/Firefox-GPS-td27504867.html
Attachment #462511 - Flags: review?(doug.turner)
hey Luca!

Support for gpsd should be in the trunk (in mozilla 2 and will be in firefox 4). So, i am not really understand you when you say it "is only present in mozilla1.9.2".  Check it out:

http://mxr.mozilla.org/mozilla-central/find?text=&string=gpsd


Do you have any docs on the new protocol.  The patch looks more or less right (i'll have to review and test).  Important point is that it is alot less code!
(In reply to comment #7)
> hey Luca!
> 
> Support for gpsd should be in the trunk (in mozilla 2 and will be in firefox
> 4). So, i am not really understand you when you say it "is only present in
> mozilla1.9.2".  Check it out:
> 
> http://mxr.mozilla.org/mozilla-central/find?text=&string=gpsd

Ops, the file is just in a different directory...
I'll prepare a new patch with updated paths.

> Do you have any docs on the new protocol.  The patch looks more or less right

There is the GPSD client howto[0] and the gpsd man page[1] (mainly relevant are the TPV and WATCH object descriptions)

> (i'll have to review and test).  Important point is that it is alot less code!

The new one is a bit bigger, but conceptually very similar.
JSON is really handy here.

[0]http://gpsd.berlios.de/client-howto.html
[1]http://arndtroide.homelinux.org/cgi-bin/man/man2html?8+gpsd
I've been trying to test the patch on firefox 4.0b2, but even before patching I can't get GPSDGeolocationProvider to initialize (there is no "startup called" log message with geo.gpsd.loggin.enabled=true).
Anyway, I attached the refactored patch (which I was obviously not able to test).
Attachment #462373 - Attachment is obsolete: true
Attachment #462511 - Attachment is obsolete: true
Attachment #462545 - Flags: review?(doug.turner)
Attachment #462511 - Flags: review?(doug.turner)
Comment on attachment 462545 [details] [diff] [review]
gpsd patch against mozilla-central


+          response = JSON.parse(responseSentence);

you want to declare response outside.  so something like:

var response = null;
try {
  response = ....
} catch ...

+        var horizontalError = Math.max(response.epx,response.epy); // meters 

Sum or avg instead?  really not sure.

+        } else {return;}

Go ahead an move to a new line. 

+        // Altitude is optional, but if it's present, so must be vertical precision.
+        if (response.alt && response.epv) {
+          var altitude = response.alt; // meters
+          var verticalError = response.epv; // meters
+        } else {
+          var altitude = null;
+          var verticalError = null; 
         }

Do something like:

var verticalError = null;
var verticalError = null;

if (response.alt && response.epv) {
  var altitude      = response.alt; // meters
  var verticalError = response.epv; // meters
}

Do this for speed and course.

+          else { var speed = null; }

and of course, new lines here too.  Keep the style.


This looks really good and it is very close.  Could you clean up the nits I mentioned above and post a new patch?
Attachment #462545 - Flags: review?(doug.turner) → review-
(In reply to comment #11)

> +        var horizontalError = Math.max(response.epx,response.epy); // meters 
> 
> Sum or avg instead?  really not sure.

I really don't know either... maybe it's sqrt(epx^2+epy^2); though this would cover the whole circle that encloses the square formed by the one-dimensional intervals, so it's probably exceedingly conservative.
Errors are supposed to be 95% confidence interval, so this could also depend (I think) on the probability distribution.
The maximum seemed an easy and fairly conservative choice, is this really a big problem?

> This looks really good and it is very close.  Could you clean up the nits I
> mentioned above and post a new patch?

I'm waiting for some clues about the horizontal error point, then I'll prepare a new patch that receives your points.
I received you coding style suggestion and prepared a new patch.
I left the horizontal precision code as it was since I don't know how to calculate exactly the confidence range; but taking the maximum of the errors shouldn't be to far from the exact number and I guess it doesn't need to be precise to the one metre.
Attachment #462545 - Attachment is obsolete: true
Attachment #472371 - Flags: review?(doug.turner)
Comment on attachment 472371 [details] [diff] [review]
Improved gpsd patch

risk:

low.  updates to latest gpsd api.

tests:

manual test using fakegps.
Attachment #472371 - Flags: review?(doug.turner)
Attachment #472371 - Flags: review+
Attachment #472371 - Flags: approval2.0?
Assignee: nobody → lultimouomo
Attachment #472371 - Flags: approval2.0? → approval2.0+
Comment on attachment 472371 [details] [diff] [review]
Improved gpsd patch

Removing ancient approval. Feel free to renominate if you need it in the tree, explaining why it's worth the risk.
Attachment #472371 - Flags: approval2.0+ → approval2.0-
Without the patch GPSDGeolocationProvider is broken (i.e. it does not work at all) on any reasonably recent version of GPSD.
Either patch it or remove it completely.
Attachment #472371 - Flags: review?
Comment on attachment 472371 [details] [diff] [review]
Improved gpsd patch

An extra review? flag won't help here.  The patch was reviewed and not approved, so it can't land in its current state.
Attachment #472371 - Flags: review?
(In reply to comment #17)
> Comment on attachment 472371 [details] [diff] [review]
> Improved gpsd patch
> 
> An extra review? flag won't help here.  The patch was reviewed and not
> approved, so it can't land in its current state.


Actually the patch was reviewed and approved, but then never merged, I don't know why.

Mike Beltzner said:

> Removing ancient approval. Feel free to renominate if you need it in the tree,
> explaining why it's worth the risk.

So I explained why the patch is needed in the tree and nominated it again.
But then again, I don't really care for that functionality and I guess no one does, otherwise it would have already been fixed, so you might as well go ahead and remove GPSDGeolocationProvider; it really just doesn't make sense to ship code that can't possibly work.
Luca, that was a review? flag, not approval2.0? which is the nomination that matters.  Doug, your call if you want to renominate and actually commit to landing this time.
http://hg.mozilla.org/mozilla-central/rev/a058ba876d89
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Unable to reproduce this bug -- VERIFIED FIXED
Status: RESOLVED → VERIFIED
What is the status of the GPSD-based geolocation in FF4? The string of comments above lead me to think that it GPSDGeolocationProvider.js was dropped. 

Understand: I am not a FF developer but was using the capability in FF3. After upgrading to FF4, the capability doesn't appear operational.
You need to log in before you can comment on or make changes to this bug.