492328 - Add GPSD as a geolocation provider

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, enhancement)

Description

[Martin McNickle]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3
Build Identifier: N/A

GPSD[1] is a daemon for linux/unix that provides gps data from any gps devices attached to a machine. It makes this data available in a vendor independent form via TCP port 2947.

This would make an excellent geolocation provider, as it has the ability to provide velocity, altitiude and heading information as well as latitude and longitude.

[1]http://gpsd.berlios.de/

This should be included by default for linux/unix builds.


Reproducible: Always

Comment 1

[Martin McNickle]

I've noticed that the Wifi provider was written in javascript, so maybe the GPSD provider doesn't need to be written in C after all.

I'm attacting my current js provider file as a base patch.

This patch needs robust error handling especially in regards to the socket code. It would also need some hooks to the preferences to set the GPSD IP address and port number.

Even still, could someone have a look at the code and see any potential problems with this approach?

Comment 2

[Martin McNickle]

Attachment: GPSD Geolocation Provider

Comment 3

[Doug Turner (:dougt)]

Hi Martin, this looks good.  I want to fix 454490 so that multiple geolocation providers can exists at the same time.  I am pretty sure that we will break nsIGeolocationProvider when we do make this change.  I would love to have your input on this bug.

Comment 5

[Jan-Benedict Glaw]

Jo Hermans attached my (obvious duplicate) bug to this one.

Attached to #502857, there's another patch going a different route. It is implemented in C++ and calls an external program via popen(). That program is ment to return position information. In case there's a well-working JS provider, my patch might still be useful for debugging, because you can simply place a script that outputs whatever you want it to.

Comment 6

[Jan-Benedict Glaw]

I'll attach that patch to this bug, too. It is not ment to exactly address gpsd-as-a-source problem, but to keep it in discussion as a generic provider for tests. (At least, I for one would love to see something like what my patch implements to be included. Would help me testing!)

Comment 7

[Jan-Benedict Glaw]

Attachment: Generic source reading location information from a popen()ed program.

Comment 8

[Doug Turner (:dougt)]

recently, i fixed 454490 that will allow many geolocation providers.

If you would like to create an addon and/or extension, AND if you would like GPSD to exist along side the default in Firefox, you can convert your registration to use a category instead of using the contract id.

Comment 9

[Martin McNickle]

Attachment: GPSD Geolocation Provider using Category Manager

I've modified the provider to add itself to the geolocation-provider category. I've tested and it does work in tandem with existing providers.

Have a look through and give any pointers to anything that needs some work.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 393793 [details] [diff] [review]
GPSD Geolocation Provider using Category Manager

Martin, first awesome job.  really cool stuff.  I tried it out and it more or less worked great.  I had to tweak a few things, but otherwise great.  Here are the review notes I took looking over your code.  I am minusing this, but it is really close.

Let me know if something isn't clear.

remove references to the nsIGeolocationPrompt,  you do not need to implement that interface.

There is a dash in front of the Components.utils.import which will cause problems.

You want to comment out the dump's, or follow what I am doing in the NetworkGeolocationProvider wrt to logging.

startup() is called on the main thread which can block all UI.  is it safe to create a synchronous socket there?  Maybe it is better to do this asynchronously?  Afterall, here it doesn't look like startup() doesn't throw so you might as well just return and not write "J=1".  (not sure what j=1 does).  Maybe you can get away with just going into watcher mode right in startup().

You probably do not need to hold a reference to "this.stream", as the this.inputStream will own the "this.stream" reference.

Could you add comments about why you are writing what you are writing?  For example, what does writing "J=1\n" to the output stream mean?  I am sure that anyone with gpsd experience will know what is going on, but for most mozilla hackers this will look like voodoo.

Using the word "channel" as a variable name in networking code that isn't a nsIChannel is probably a bad idea.  Could you rename "var channel" to something else?  Maybe introString or something....

You could move the check for RMC above the looking over the fields as a minor optimization.

You probably should use the map() function to assign stuff into cords similar to what I do in the other provider.

style nits.  use spaces not tabs.  two spaces works great.  similar to what the NetworkGelocationProvider does.

Comment 11

[Martin McNickle]

Attachment: GPSD Geolocation Provider

Comment 12

[Martin McNickle]

(In reply to comment #10)
> remove references to the nsIGeolocationPrompt,  you do not need to implement
> that interface.
> 
> There is a dash in front of the Components.utils.import which will cause
> problems.
> 
> You want to comment out the dump's, or follow what I am doing in the
> NetworkGeolocationProvider wrt to logging.

Done.

> startup() is called on the main thread which can block all UI.  is it safe to
> create a synchronous socket there?  Maybe it is better to do this
> asynchronously?  Afterall, here it doesn't look like startup() doesn't throw so
> you might as well just return and not write "J=1".  (not sure what j=1 does). 
> Maybe you can get away with just going into watcher mode right in startup().

The streams that are opened there are non-blocking so it should be safe to open there.

I've moved the "J=1" (set gpsd buffer on) to watch() now as well.

> You probably do not need to hold a reference to "this.stream", as the
> this.inputStream will own the "this.stream" reference.

I was getting mixed up here. Now holds a reference to inputStream and outputStream only. I create a scriptableInputStream inside watch() now.

> Could you add comments about why you are writing what you are writing?  For
> example, what does writing "J=1\n" to the output stream mean?  I am sure that
> anyone with gpsd experience will know what is going on, but for most mozilla
> hackers this will look like voodoo.
> 
> Using the word "channel" as a variable name in networking code that isn't a
> nsIChannel is probably a bad idea.  Could you rename "var channel" to something
> else?  Maybe introString or something....
> 
> You could move the check for RMC above the looking over the fields as a minor
> optimization.

Done.

> You probably should use the map() function to assign stuff into cords similar
> to what I do in the other provider.

I think I've addressed this, but let me know if I've misunderstood.

> style nits.  use spaces not tabs.  two spaces works great.  similar to what the
> NetworkGelocationProvider does.

Should've caught this myself. Fixed.

This should probably get minused because:

a) I need help with adding the "include for LINUX/UNIX only" directives, and the patch should be included without it.
b) Not sure if I should go cluttering up the preferences with geo.gpsd.*

I'd appreciate a pointer on these last 2 things.

Comment 13

[Jan-Benedict Glaw]

Is a GPSD geolocation provider really *only* good for Linux? You'd connect from a Windows box to a GPSD running on a different machine. The other Unix derivates might want it, too. And I seem to remember that there's a GPSDalike running natively on windows with a compatible protocol.

Comment 14

[Martin McNickle]

@Jan

I didn't consider that case; but you are correct. There is nothing Linux/Unix specific in the code so it should be included for all OSes.

Thanks for pointing that out.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 394817 [details] [diff] [review]
GPSD Geolocation Provider

lets make default prefs so that you do not have to add anything to the preferences file.  That is:


+    var hostIPAddr = this.prefService.getCharPref("geo.gpsd.host.ipaddr");

becomes:

var hostIPAddr = "localhost";

try {
  hostIPAddr  = this.prefService.getCharPref("geo.gpsd.host.ipaddr");
}
catch (e) {}


Otherwise, i think this is fine.  Throw me one more patch.

Comment 16

[Martin McNickle]

Attachment: GPSD Geolocation Provider

Avoids cluttering preferences by assuming default values for ipaddr and port.

Comment 17

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/44344ac8fe41

martin, can you work with Joel to help him create a litmus test?

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 395325 [details] [diff] [review]
GPSD Geolocation Provider

safe support of GPSD.  we should consider for FF3.6.

Comment 19

[(not currently active) Ted Mielczarek]

You didn't add GPSDGeolocationProvider.js to any of the packaging manifests, so this file is only shipping in our Mac builds, currently:
http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/packages-static
http://mxr.mozilla.org/mozilla-central/source/browser/installer/unix/packages-static

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 395325 [details] [diff] [review]
GPSD Geolocation Provider

a- until you fix the packaging manifests.

Comment 21

[Doug Turner (:dougt)]

Attachment: packaging fu.

Comment 22

[Doug Turner (:dougt)]

packaging fu:

http://hg.mozilla.org/mozilla-central/rev/c0e002e1d5e9

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 395325 [details] [diff] [review]
GPSD Geolocation Provider

rerequesting now that packaging is correct.

Comment 24

[Martin McNickle]

> martin, can you work with Joel to help him create a litmus test?

Sure. Although I'm not sure how to create a test to test individual providers as they are all exposed through the same interface. Any ideas?

Joel, if you need any questions answered please contact me directly.

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

Martin, I can easily create a case (or set of cases) that will cover this.  I need to know how to access a GPSD provider?  Is there a specific website that will do this?  

Do we need to install software on a local computer to interact with GPSD?  If you can give me a set of steps that will allow me to test this, I can get it added to litmus so we run it for each release.

Comment 26

[Martin McNickle]

Joel,

There is software to install in order to test the provider. However this doesn't have to be installed locally. You could set up a remote computer to run GPSD and have the test machine connect to it:

Say that the remote machine has an IP address of 123.123.123.123 then the test would look like the following.

1) Navigate to 'about:config'
2) Create a new string preference, name 'geo.gpsd.host.ipaddr' value '123.123.123.123'
3) Create a new boolean value, name 'geo.gpsd.logging.enabled' value 'true'
4) Open the error console, select 'messages'
5) Navigate to 'http://people.mozilla.org/~dougt/geo.html'
6) Accept request to acquire position
7) Verify that there are messages in the error console starting with '*** GPSD GEO: Got info: ...'

This test will ensure that the provider is working and reporting position.

The different geoproviders compete to report position; the one that has the "best" position wins. There may be cases where the Network provider will "win" and as a result the position reported by the site will NOT have come from the GPSD provider. It should be enough to ensure that it is generating the '*** GPSD GEO: Got info: ...' messages, it DOES NOT need to match the coordindates displayed on the site.

Setting up GPSD to run on a linux box is trivial, and you can get it to report fake data which is good for testing purposes. The problem may be finding a machine to host it reliably. Any ideas?

Comment 27

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6d700cea1785
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/39cd8bd62eaf

Comment 28

[Marc Pfister]

As of rev:39cd8bd62eaf it seems to be parsing the RMC NMEA sentence, but the field names in the code don't match the NMEA specification for RMC - for example there's no horizontal and vertical error. 
To calculate the error you'd probably have to make a guess from the PDOP value. Or you'd want to use GPSD over wifi positioning as long as the GPS has a valid fix?

Comment 29

[Martin McNickle]

(In reply to comment #28)
> As of rev:39cd8bd62eaf it seems to be parsing the RMC NMEA sentence, but the
> field names in the code don't match the NMEA specification for RMC.

The code isn't really parsing the RMC sentence. GPSD provides a layer of abstraction from the NMEA sentences, so what is being parsed is actually just a GPSD output. With caching on, GPSD will combine various sentences to provide a complete time/position/velocity report as one output[1]. We just happen to update the position when an new RMC sentence is recieved.

[1] http://gpsd.berlios.de/gpsd.html see option 'o' under the 'OLD PROTOCOL' section.

Comment 30

[Marc Pfister]

(In reply to comment #29)

> The code isn't really parsing the RMC sentence. GPSD provides a layer of
> abstraction from the NMEA sentences, so what is being parsed is actually just a
> GPSD output. With caching on, GPSD will combine various sentences to provide a
> complete time/position/velocity report as one output[1]. We just happen to
> update the position when an new RMC sentence is recieved.

Ah, I see - I was misinterpreting line 239. Since GPSD is only reporting the last NMEA setenced parsed, is it even needed to check for the RMC sentence, as all GPSD "O" messages should have the same contents?

I ask because my GPSD server does not output RMC, only GGA, GSA, and GSV.

I figure that you select one sentence instead of parsing the same "O" output multiple times per GPS position update? Maybe the code should grab the first valid NMEA tag it sees and then just keep using that one?

This is very nice work and exactly what I need for a project right now. If any assistance is needed I have time available to devote to this.

Comment 31

[Martin McNickle]

Hi Marc, thanks for the kind comments.

I have only the one device that I can test with, so I was not aware of this problem. Would it be possible for you to open a bug about it? It would also be really useful if you uploaded an NMEA dump along with it for testing.

Comment 32

[Marc Pfister]

Martin, I opened a bug with sample data:

https://bugzilla.mozilla.org/show_bug.cgi?id=524722

I can also make our GPSD test server public if needed for testing.

Delving further into this it appears for some reason GPSD is not outputting the RMC tag in my test data in 'watcher' mode.

Comment 33

[Eric Shepherd [:sheppy]]

Added a note mentioning this support here:

https://developer.mozilla.org/En/Using_geolocation

Comment 34

[bhaveshp]

Any ideas why the support for gpsd as a geolocation provider is only on Linux.
There are already ports of gpsd on Windows and I feel that the support for gpsd should be turned on by default for Windows also.

GPSd support on Windows
http://home.arcor.de/ulf.lamping/gpsd/gpsd.html
http://www.renderlab.net/projects/wrt54g/gpsdonwindows.html