Closed Bug 492328 Opened 11 years ago Closed 10 years ago

Add GPSD as a geolocation provider

Categories

(Core :: DOM: Geolocation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: mmcnicklebugs, Assigned: dougt)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 3 obsolete files)

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
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?
Attached file GPSD Geolocation Provider (obsolete) —
Depends on: 454490
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 502857
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.
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!)
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.
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.
Attachment #376684 - Attachment is obsolete: true
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.
Attachment #393793 - Flags: review-
Attached patch GPSD Geolocation Provider (obsolete) — Splinter Review
Attachment #393793 - Attachment is obsolete: true
Attachment #394817 - Flags: review?(doug.turner)
(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.
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.
@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.
OS: Linux → All
Hardware: x86 → All
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.
Attachment #394817 - Flags: review?(doug.turner) → review-
Avoids cluttering preferences by assuming default values for ipaddr and port.
Attachment #394817 - Attachment is obsolete: true
Attachment #395325 - Flags: review?(doug.turner)
Attachment #395325 - Flags: review?(doug.turner) → review+
http://hg.mozilla.org/mozilla-central/rev/44344ac8fe41

martin, can you work with Joel to help him create a litmus test?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 395325 [details] [diff] [review]
GPSD Geolocation Provider

safe support of GPSD.  we should consider for FF3.6.
Attachment #395325 - Flags: approval1.9.2?
Assignee: nobody → mmcnicklebugs
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
Attachment #395325 - Flags: approval1.9.2? → approval1.9.2-
Comment on attachment 395325 [details] [diff] [review]
GPSD Geolocation Provider

a- until you fix the packaging manifests.
Attached patch packaging fu.Splinter Review
Assignee: mmcnicklebugs → doug.turner
Attachment #395913 - Flags: review?
Attachment #395913 - Flags: review? → review?(ted.mielczarek)
Attachment #395913 - Flags: review?(ted.mielczarek) → review+
Attachment #395325 - Flags: approval1.9.2- → approval1.9.2?
Comment on attachment 395325 [details] [diff] [review]
GPSD Geolocation Provider

rerequesting now that packaging is correct.
Blocks: 512005
> 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.
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.
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?
Attachment #395325 - Flags: approval1.9.2? → approval1.9.2+
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
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?
(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.
(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.
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.
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.
Added a note mentioning this support here:

https://developer.mozilla.org/En/Using_geolocation
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
You need to log in before you can comment on or make changes to this bug.