Last Comment Bug 711300 - Add GPS support for gonk
: Add GPS support for gonk
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla12
Assigned To: Kan-Ru Chen [:kanru] (UTC+8)
:
Mentors:
Depends on: 752649 777983
Blocks: b2g-demo-phone 715788
  Show dependency treegraph
 
Reported: 2011-12-15 17:30 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-26 16:22 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add GPS support for gonk (10.60 KB, patch)
2012-01-12 01:14 PST, Kan-Ru Chen [:kanru] (UTC+8)
josh: review-
Details | Diff | Review
Add GPS support for gonk, v2 (11.85 KB, patch)
2012-01-12 21:00 PST, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Add GPS support for gonk, v2 (11.88 KB, patch)
2012-01-12 22:56 PST, Kan-Ru Chen [:kanru] (UTC+8)
josh: review+
Details | Diff | Review
Bug 711300 - Add GPS support for gonk, v3 (12.05 KB, patch)
2012-01-15 22:50 PST, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review
Bug 711300 - Add GPS support for gonk, v4 (12.05 KB, patch)
2012-01-16 01:03 PST, Kan-Ru Chen [:kanru] (UTC+8)
no flags Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-15 17:30:34 PST
We of course want geolocation support in gonk.  To get that, we need to add (at least) a backend for geolocation that talks to the gps HW.

There's a reasonable and usable GPS HAL in android (hardware/libhardware/include/hardware/gps.h), but I haven't investigated how it's used in higher levels of the system.  We might be able to use a simpler wrapper around the HAL.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-30 21:10:29 PST
The logic is spread across frameworks/base/services/jni/com_android_server_location_GpsLocationProvider.cpp and frameworks/base/services/java/com/android/server/location/GpsLocationProvider.java.  This depends on fairly accurate (ntp) time support, and full a-gps and ni-gps support have other dependencies.

Let's focus this bug on just GPS support.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-30 21:12:01 PST
(Possible dep on bug 714355.)
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-06 13:39:05 PST
Kan-Ru, if you're currently working on this, can you assign it to yourself?
Comment 4 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-12 01:14:35 PST
Created attachment 587968 [details] [diff] [review]
Add GPS support for gonk
Comment 5 Josh Matthews [:jdm] 2012-01-12 10:09:07 PST
Comment on attachment 587968 [details] [diff] [review]
Add GPS support for gonk

Review of attachment 587968 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fairly sensible, but I'd like to see another version with my comments addressed.

::: dom/src/geolocation/nsGeolocation.cpp
@@ -87,4 +87,5 @@
> >  #ifdef MOZ_WIDGET_ANDROID
> >  #include "AndroidLocationProvider.h"
> >  #endif
> >  
> > +#include "GonkGPSGeolocationProvider.h"

We'll want #ifdef goop here, presumably.

@@ -577,3 +579,5 @@
> >    if (provider)
> >      mProviders.AppendObject(provider);
> >  #endif
> > +
> > +  provider = new GonkGPSGeolocationProvider();

And here as well.

::: dom/system/b2g/GonkGPSGeolocationProvider.cpp
@@ +45,5 @@
> +
> +NS_IMPL_ISUPPORTS1(GonkGPSGeolocationProvider, nsIGeolocationProvider)
> +
> +static nsIGeolocationUpdate* gLocationCallback = NULL;
> +static const GpsInterface* gGpsInterface = NULL;

There's no particular reason these need to be static globals. Let's move them inside the provider and add a public getter for the callback. The provider can be the only static global in this file.

@@ +71,5 @@
> +  pthread_t thread;
> +  pthread_attr_t attr;
> +
> +  pthread_attr_init(&attr);
> +  pthread_create(&thread, &attr, (void*(*)(void*))start, arg);

This cast weirds me out. Why is it necessary?

@@ +85,5 @@
> +  const GpsInterface* interface = NULL;
> +
> +  err = hw_get_module(GPS_HARDWARE_MODULE_ID, (hw_module_t const**)&module);
> +
> +  if (!err) {

Invert the control flow and return NULL if hw_get_module fails.

@@ +88,5 @@
> +
> +  if (!err) {
> +    hw_device_t* device;
> +    err = module->methods->open(module, GPS_HARDWARE_MODULE_ID, &device);
> +    if (!err) {

Same thing here.

@@ +114,5 @@
> +}
> +
> +GonkGPSGeolocationProvider::~GonkGPSGeolocationProvider()
> +{
> +  NS_IF_RELEASE(gLocationCallback);

If gLocationCallback becomes an nsCOMPtr member, this can go away.

@@ +121,5 @@
> +NS_IMETHODIMP
> +GonkGPSGeolocationProvider::Startup()
> +{
> +  if (!gGpsInterface) {
> +    gGpsInterface = GetGPSInterface();

Why not move this into the constructor?

@@ +140,5 @@
> +GonkGPSGeolocationProvider::Watch(nsIGeolocationUpdate* aCallback)
> +{
> +  NS_IF_RELEASE(gLocationCallback);
> +  gLocationCallback = aCallback;
> +  NS_IF_ADDREF(gLocationCallback);

If the callback becomes an nsCOMPtr, the NS_IF goop can go away.
Comment 6 Michael Wu [:mwu] 2012-01-12 10:18:10 PST
Comment on attachment 587968 [details] [diff] [review]
Add GPS support for gonk

>+static void
>+location_callback(GpsLocation* location)
>+{
>+  nsRefPtr<nsGeoPosition> somewhere = new nsGeoPosition(location->latitude,
>+                                                        location->longitude,
>+                                                        location->altitude,
>+                                                        location->accuracy,
>+                                                        location->accuracy,
>+                                                        location->bearing,
>+                                                        location->speed,
>+                                                        location->timestamp);
>+  if (gLocationCallback)
>+    gLocationCallback->Update(somewhere);

Something like this at the top of the function would be slightly more efficient in the cases where there is no callback:
if (!gLocationCallback)
  return;
Comment 7 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-12 19:03:51 PST
(In reply to Josh Matthews [:jdm] from comment #5)
> @@ +71,5 @@
> > +  pthread_t thread;
> > +  pthread_attr_t attr;
> > +
> > +  pthread_attr_init(&attr);
> > +  pthread_create(&thread, &attr, (void*(*)(void*))start, arg);
> 
> This cast weirds me out. Why is it necessary?

Unfortunately pthread_create and the callback disagreed on what start function should return. I can typdef pthread_func and add a comment to it.

The create_thread_callback thing was for Android to be able to call java runtime from the callback, that we don't need. I guess we can call start directly, but I still create a pthread, the engine might assume a pthread_t would be returned and do something to it.
Comment 8 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-12 21:00:26 PST
Created attachment 588301 [details] [diff] [review]
Add GPS support for gonk, v2

--
Attachment #587968 [details] [diff] - Attachment is obsolete: true
Comment 9 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-12 22:56:46 PST
Created attachment 588324 [details] [diff] [review]
Add GPS support for gonk, v2

--
Attachment #588301 [details] [diff] - Attachment is obsolete: true
Comment 10 Josh Matthews [:jdm] 2012-01-14 13:03:58 PST
Comment on attachment 588324 [details] [diff] [review]
Add GPS support for gonk, v2

Review of attachment 588324 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, modulo my comments. If the answer about mStartup is that the check is just for safety, then I'm fine with leaving it in.

::: dom/system/b2g/GonkGPSGeolocationProvider.cpp
@@ +107,5 @@
> +}
> +
> +GonkGPSGeolocationProvider::~GonkGPSGeolocationProvider()
> +{
> +  this->Shutdown();

No need for this->.

@@ +111,5 @@
> +  this->Shutdown();
> +  sSingleton = NULL;
> +}
> +
> +/* static */ GonkGPSGeolocationProvider*

already_addrefed<GonkGPSGeolocationProvider>

@@ +117,5 @@
> +{
> +  if (!sSingleton)
> +    sSingleton = new GonkGPSGeolocationProvider();
> +
> +  NS_IF_ADDREF(sSingleton);

NS_ADDREF, since new is infallible.

@@ +124,5 @@
> +
> +nsCOMPtr<nsIGeolocationUpdate>
> +GonkGPSGeolocationProvider::GetLocationCallback()
> +{
> +  return mLocationCallback;

This function is better expressed as
already_addrefed<nsIGeolocationUpdate>
GonkGPSGeolocationProvider::GetLocationCallback()
{
  nsCOMPtr<nsIGeolocationUpdate> callback = mLocationCallback;
  return callback.forget();
}

@@ +146,5 @@
> +
> +NS_IMETHODIMP
> +GonkGPSGeolocationProvider::Startup()
> +{
> +  if (mStarted)

Why is this needed now? When do we call Startup multiple times without calling Shutdown first?

::: dom/system/b2g/GonkGPSGeolocationProvider.h
@@ +36,5 @@
> + * ***** END LICENSE BLOCK ***** */
> +#ifndef GonkGPSGeolocationProvider_h
> +#define GonkGPSGeolocationProvider_h
> +
> +#include <hardware/gps.h> // for GpsInterface

GpsInterface can just be a forward declaration, can't it?

@@ +51,5 @@
> +  nsCOMPtr<nsIGeolocationUpdate> GetLocationCallback();
> +
> +private:
> +
> +  /* Client shoud use GetSingleton() to get the provider instance. */

s/shoud/should/
Comment 11 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-15 19:51:16 PST
(In reply to Josh Matthews [:jdm] from comment #10)
> This looks good, modulo my comments. If the answer about mStartup is that
> the check is just for safety, then I'm fine with leaving it in.

Just for safety. If initialized multiple times, it will be locked up.

> ::: dom/system/b2g/GonkGPSGeolocationProvider.h
> @@ +36,5 @@
> > + * ***** END LICENSE BLOCK ***** */
> > +#ifndef GonkGPSGeolocationProvider_h
> > +#define GonkGPSGeolocationProvider_h
> > +
> > +#include <hardware/gps.h> // for GpsInterface
> 
> GpsInterface can just be a forward declaration, can't it?

Nope. It's a anonymous typedef struct.
Comment 12 Josh Matthews [:jdm] 2012-01-15 20:47:50 PST
Can't you just use |struct GpsInterface;| in the header, though?
Comment 13 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-15 20:55:12 PST
(In reply to Josh Matthews [:jdm] from comment #12)
> Can't you just use |struct GpsInterface;| in the header, though?

Sadly I can't. GpsInterface is defined as

  typedef struct {
     /* ... */
  } GpsInterface;

So |struct GpsInterface| is a different type.
Comment 14 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-15 22:50:31 PST
Created attachment 588812 [details] [diff] [review]
Bug 711300 - Add GPS support for gonk, v3
Comment 15 Josh Matthews [:jdm] 2012-01-15 23:22:52 PST
Comment on attachment 588812 [details] [diff] [review]
Bug 711300 - Add GPS support for gonk, v3

>+  nsCOMPtr<GonkGPSGeolocationProvider> provider =
>+    GonkGPSGeolocationProvider::GetSingleton();

This should be nsRefPtr. nsCOMPtr is for abstract interface classes.
Comment 16 Kan-Ru Chen [:kanru] (UTC+8) 2012-01-16 01:03:45 PST
Created attachment 588825 [details] [diff] [review]
Bug 711300 - Add GPS support for gonk, v4
Comment 18 Ed Morley [:emorley] 2012-01-19 17:49:09 PST
https://hg.mozilla.org/mozilla-central/rev/173c06d3dd71

Note You need to log in before you can comment on or make changes to this bug.