714413 - Sensor support for gonk

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is a bug for the common work needed to support the sensors exposed through the android sensors HAL module.  See hardware/libhardware/include/hardware/sensors.h.

Polling for new sensor events is synchronous, so this needs to happen on a separate thread.  The raw sensor HAL module API is reasonably nice, but there's a low-level helper SensorDevice in frameworks/base/services/sensorservice/SensorService.h that will help us a bit.  So here's the plan
 - add a hal/gonk/Sensors.cpp file where this code will live, implementing the Hal.h API created in bug 697641.
 - the first time a sensor listener registers, spawn off the sensor-poller thread.
 - create SensorDevice on the other thread
 - when the first listener for a particular type is registered, activate(..., true) that sensor device through SensorDevice
 - when the last listener unregisters for a particular type, activate(..., false) (deactivate) that sensor device
 - poll() for sensor events.  When they arrive, post them back for delivery to listeners

We can implement all the Hal.h sensor types here or do part of the work in followup bugs.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Hi Steven, this is the bug we discussed at the last b2g call.  If you're working on this, please assign this bug to yourself :).

Comment 2

[StevenLee[:slee]]

Hi Chris:
I found the include path of hal does not contain where SensorDevice.h locates in. Should I copy SensorDevice.cpp and SensorDeivce.h to gecko/hal/gonk or add an include path in the Makefile?
Thanks~

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Let's set up the include path correctly.  We also need to link against libsensorservice, by adding -lsensorservice here: http://mxr.mozilla.org/mozilla-central/source/toolkit/library/Makefile.in#429

mwu where should the added include path (frameworks/base/services/sensorservice/) go?  Is there a reason we shouldn't add it to widget/gonk/Makefile.in LOCAL_INCLUDES?

Comment 4

[Michael Wu [:mwu]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> mwu where should the added include path
> (frameworks/base/services/sensorservice/) go?  Is there a reason we
> shouldn't add it to widget/gonk/Makefile.in LOCAL_INCLUDES?

That should be fine if it works. I don't think we have an easy to use makefile var for finding the gonk base directory though.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

OK, I understand.

So it looks like the other option is http://mxr.mozilla.org/mozilla-central/source/configure.in#316 .

Comment 6

[Mozilla RelEng Bot]

Try run for 30de184919c4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=30de184919c4
Results (out of 127 total builds):
    success: 97
    warnings: 21
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-30de184919c4

Comment 7

[Mozilla RelEng Bot]

Try run for 61bab4fd528c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=61bab4fd528c
Results (out of 206 total builds):
    success: 153
    warnings: 19
    other: 34
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-61bab4fd528c

Comment 8

[StevenLee[:slee]]

Attachment: SensorManager in hal on gonk

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 593355 [details] [diff] [review]
SensorManager in hal on gonk

Hi Steven,

There are a few high-level style issues here.

First, please move SensorManager.h/.cpp into GonkSensor.cpp.  These aren't part of the public API, but only part of the gonk implementation of the public API.  Having the implementation spread across several files makes it harder for me to follow.

Second, (a minor issue), the coding style we use for functions is 

void
EnableSensorNotifications(SensorType aSensor)
{

like that.  The style is inconsistent in your patch.

Third, the indentation style is 2-space indents.  Tabs aren't allowed in Mozilla files.

Fourth, please use the new MPL license header in the new files you're adding here.  The first few lines of the file should be,

/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this file,
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

Much shorter! :)

I'll leave more technical review comments in a bit.  These are just style issues.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 593355 [details] [diff] [review]
SensorManager in hal on gonk

>diff --git a/hal/gonk/GonkSensor.cpp b/hal/gonk/GonkSensor.cpp

>+using namespace mozilla::hal;
>+
>+namespace mozilla {
>+namespace hal_impl {
>+
>+SensorManager sSensorManager;

Just so you know, SensorManager has a nontrivial constructor, and
nontrivial constructors slow down the startup time of Gecko.  We
usually try to avoid them.  In this case though, I'm OK with it.

Just for your information ;).  Nothing needs to change for this patch.

>diff --git a/hal/gonk/SensorManager.cpp b/hal/gonk/SensorManager.cpp

>+#include "Hal.h"
>+#include "SensorDevice.h"
>+#include "SensorManager.h"
>+#include <stdio.h>
>+#include "mozilla/hal_sandbox/PHal.h"

Why do you need to include this directly?  You shouldn't need to.

>+
>+#include <pthread.h>
>+

Another general style issue: please include headers in the following
order

// C headers like
#include <stdio.h>
#include <unistd.h>

// then C++ headers like
#include <vector>

// and finally, Gecko headers
#include "Hal.h"
#include "SensorDevice.h"
#include "SensorManager.h"

Please try to keep each in alphabetical order.

The reason for this separation is that platform-specific headers tend
to always be included in this way, and on occasion some things break
unexpectedly when they're not included in this order.

>+inline SensorType hardwareSensor2halSensor(int type)

Please remove the |inline| annotation from this.  Modern compilers
completely ignore it (except one case that's not important here).

Please rename this to "HardwareSensorToHalSensor".

This should be a |static| function.  Style for static functions is
like this

static SensorType
HardwareSensorToHalSensor(int aType)
{

>+{
>+	SensorType aSensor;

Please remove this.  See below.

>+	case SENSOR_TYPE_ORIENTATION:
>+		aSensor = SENSOR_ORIENTATION;
>+		break;

Instead,

  case SENSOR_TYPE_ORIENTATION:
    return SENSOR_ORIENTATION;

>+inline int halSensor2hardwareSensor(SensorType type)

Please apply the same changes to this function too.

>+inline bool Sensorsevent2SensorData(const sensors_event_t& data, SensorData &sensorData)

Similar changes to the declaration of this function.

>+{
>+        sensorData.sensor() = hardwareSensor2halSensor(data.type);
>+	
>+	if (sensorData.sensor() == SENSOR_UNKNOWN) return false;

Please put the |return false;| on its own line.

  if (sensorData.sensor() == SENSOR_UNKNOWN)
    return false;

>+void onSensorChanged(const sensors_event_t& data, SensorData& aSensorData)

Please make the SensorData parameter a bare pointer, to make its
outparam semantics clear at callsites

  SensorData* aSensorData

Style change here too:

void
OnSensorChanged(const sensors_event_t& aData, SensorData* aSensorData)
{

I'm going to stop noting these :).  I think you've got the pattern by
now :).

>+{
>+	if (!Sensorsevent2SensorData(data, aSensorData)) return;

In what situation could we get an event for an unrecognized sensor?
If it can't happen unless something goes wrong, please convert the
code to the following

  DebugOnly<bool> convertedData = SensorEventToSensorData(data, &aSensorData));
  MOZ_ASSERT(convertedData);
  NotifySensorChange(aSensorData);

>+class SensorDeviceGetter {

Why don't we call SensorDevice::getInstance() directly and remove this
class?

>+bool SensorManager::mContinue = true;
>+int SensorManager::mDeviceCount = 0;

Please keep this a |size_t|.

>+Mutex SensorManager::mMutex("MozSensorManager");

We need to allocate this dynamically; in DEBUG builds, Mutex can't be
allocated from a static context.  Let's allocate this just before we
initialize the sensor thread.

>+void* SensorManager::Runnable(void *arg)

Since you don't use the argument to this function, please make it

  |void* /*unused*/|

This avoids compiler warnings in some cases.

>+                count = mDeviceGetter->getDevice().poll(buffer, numEventMax);
>+		if (count < 0) {
>+			//HAL_LOG("poll fail %d\n", count);
>+			break;
>+		}

Why can the poll() call fail here?  If there are "normal" errors that
we can ignore, we should just continue looping, and not |break| here.

>+} // hal_impl
>+} // mozilla
>diff --git a/hal/gonk/SensorManager.h b/hal/gonk/SensorManager.h
>new file mode 100755
>--- /dev/null
>+++ b/hal/gonk/SensorManager.h
>@@ -0,0 +1,58 @@
>+#ifndef _MY_SENSOR_MANAGER_H_
>+#define _MY_SENSOR_MANAGER_H_

It won't matter for this patch because this header file will go away,
but the style we normally use for include guards is

#ifndef mozilla_hal_impl_SensorManager_h
#define mozilla_hal_impl_SensorManager_h

For future reference :).

>+class SensordataHolder

>+	SensordataHolder():mRefCount(0) {};
>+	int mRefCount;

"refcount" has a normal usage which is, "the number of references to
the instance of this class".  Normally, when the refcount reaches 0,
the instance of the class is deleted.

That's not quite what we're doing here, so I think changing this name
to |mActivationCount| would make things a bit clearer.

>+class SensorManager

As things stand, this class is just a singleton being used as a
namespace.  That is, everything inside it is class-static, basically.

A cleaner pattern in C++ is to make a file-static implementation:
file-static data that's manipulated by file-static functions.  In
addition to being cleaner and less verbose, that style of
implementation also allows compilers to better optimize the code.

Please convert the SensorManager fields and methods into file-static
variables and functions in GonkSensor.cpp.

>+{
>+public:
>+	SensorManager();
>+	~SensorManager();
>+	
>+	void enableSensor(mozilla::hal::SensorType type);
>+	void disableSensor(mozilla::hal::SensorType type);
>+	void getCurrentSensorDataInformation(mozilla::hal::SensorType type, mozilla::hal::SensorData* aSensorData);
>+

Gecko style for C++ methods is

  void EnableSensor(mozilla::hal::SensorType aType);

like that.  When you're not sure what the style is, look at code nearby.

But like I said above, I think this code should become statics in
GonkSensor.cpp.

>+private:
>+	
>+	static Mutex					mMutex;

Please document what data |mMutex| protects, and on which threads the
data is accessed from.

Also, please document which functions run on each thread.  A style
that I like is

  // These functions run on the main thread
  void Foo() { /*...*/ }
  void Bar() { /*...*/ }

  // These functions run on the sensor-polling thread.
  void Baz() { /*...*/ }

In addition, adding assertions to each function that check whether the
function's code is running on the expected thread helps catch bugs in
the future.  For example,

MainThreadFunction()
{
  MOZ_ASSERT(NS_IsMainThread());

SensorThreadFunction()
{
  MOZ_ASSERT(pthread_self() == sSensorThread);

OK!  I left a lot of comments here, but the basic approach of this
patch is good.  Consider this an introduction to Gecko coding style
:).

If you have any questions, please feel free to ask.

I cleared the review request on this patch to mean, "the approach of
this patch is OK, but I would like to see second version with my
commented fixed."

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Additional note: for the data that will become file-static in GonkSensor.cpp, please initialize it lazily the first time a sensor is enabled.  This includes spawning the pthread.

Thanks!

Comment 12

[Mozilla RelEng Bot]

Try run for 61bab4fd528c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=61bab4fd528c
Results (out of 225 total builds):
    success: 171
    warnings: 20
    other: 34
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-61bab4fd528c

Comment 13

[Mozilla RelEng Bot]

Try run for eac0db9c7db2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eac0db9c7db2
Results (out of 206 total builds):
    exception: 1
    success: 168
    warnings: 36
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-eac0db9c7db2

Comment 14

[StevenLee[:slee]]

Attachment: Sensor support for gonk V2

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 594065 [details] [diff] [review]
Sensor support for gonk V2

Very nice!!  So much smaller and simpler :).

>diff --git a/hal/Makefile.in b/hal/Makefile.in
>-CXXFLAGS        += $(MOZ_DBUS_GLIB_CFLAGS)
>+CXXFLAGS        += $(MOZ_DBUS_GLIB_CFLAGS) -DHAVE_PTHREADS

Hmm ... I thought we were defining this for Gonk already.  mwu?

Either way, this is harmless.

>diff --git a/hal/gonk/GonkSensor.cpp b/hal/gonk/GonkSensor.cpp

>+static SensorType
>+HardwareSensorToHalSensor(int type)

Nit: Let's put these three helper functions inside the |namespace
mozilla| below, so that ...

>+  mozilla::DebugOnly<bool> convertedData = SensorseventToSensorData(data, aSensorData);

we don't need the 'mozilla::' here.

>+static bool sInitialized = false;
>+static bool sContinue = false;
>+static int sActivatedSensors = 0;

Nit: this is the default value these are initialized with anyway, so
no need to write it explicitly.

>+static void 
>+InitialResources()

"InitializeResources"

Very nice patch!  r=me with the nits above fixed.

Comment 16

[[:fabrice] Fabrice Desré]

Steven, can you attach the updated patch addressing Chris's nits?

Comment 17

[StevenLee[:slee]]

Attachment: Sensor support for gonk V3 - fixed Chris's nits

Comment 18

[[:fabrice] Fabrice Desré]

pushed after minor rebasing of Makefile.in-s

https://hg.mozilla.org/integration/mozilla-inbound/rev/85b4eb39ba84

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/85b4eb39ba84

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Congratulations, Steven!

Comment 21

[StevenLee[:slee]]

Chris and Fabrice, thanks for your help.