512345 - Implement accelerometer support for Linux and Maemo

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Michael Ventnor]

Attachment: Patch

Bug 485943 implemented accelerometer support for OSX and some WinCE devices. This adds support for Maemo 5, as well as the laptop models that Linux supports (applesmc or hdaps).

Comment 1

[Doug Turner (:dougt)]

Comment on attachment 396308 [details] [diff] [review]
Patch

Nice.  Awesome.  Great.  A few small nits, but otherwise fine:

+#define NORMALIZE_FACTOR 255.0
+#define MAEMO_NORMALIZE_FACTOR -1000.0


Lets just have one #define set based on #ifdef's:

#ifdef NS_MAEMO
#define NORMALIZE_FACTOR 255.0
#else
...


nsAccelerometerGtk::Startup


could this be data driven so that we don't have to add more code when another linux variate is added?

For example, something like:

typedef struct {
char * fileName;
int type;
} Accelerometers;

Accelerometers gAccelerometers[] = {
{"/sys/devices/platform/applesmc.768/position", eAppleSensor},
...
{}
}


for (x in gAccelerometers) // <-- don't you wish C had that?
{
   if (access(x.fileName ...)
      mType = x.type
      mPositionFile = 
      mCalibrateFile =

...




   
Also, check out bug 512237.  Smaug mentioned two thing we should change with the cocoa port that must apply to all implementations.

One more patch, and I think we are there.

Comment 2

[Michael Ventnor]

(In reply to comment #1)
> (From update of attachment 396308 [details] [diff] [review])
> Nice.  Awesome.  Great.  A few small nits, but otherwise fine:
> 
> +#define NORMALIZE_FACTOR 255.0
> +#define MAEMO_NORMALIZE_FACTOR -1000.0
> 
> 
> Lets just have one #define set based on #ifdef's:
> 
> #ifdef NS_MAEMO
> #define NORMALIZE_FACTOR 255.0
> #else
> ...

Does NS_MAEMO exist? I don't know about making this conditional on Maemo (despite the variable names) since this depends on the presence of a certain device and Linux driver, not just for Maemo (even though Maemo is the only major user so far). This is why I check the sysfs rather than use ifdefs.

Comment 3

[Doug Turner (:dougt)]

#ifdef NS_OSSO works.  It is defined on any nokia device running maemo.

Comment 4

[Doug Turner (:dougt)]

(we define it)

Comment 5

[Michael Ventnor]

Attachment: Patch 2

Addresses review comments.

Comment 6

[Michael Ventnor]

Attachment: Patch 2.1

There's actually a quirk in the applesmc driver we need to deal with, to do with calibration.

Comment 7

[Michael Ventnor]

OK, I really really REALLY don't want to make that third accelerometer interface dependent on Maemo or OSSO. I've seen videos of "desktop" Linuxes on some other tablets or mobile devices and after doing some research they seem to be using the same brand of sensor.

Comment 8

[Michael Ventnor]

Er, hang on, that interface may be specific to Maemo I think, others might use a different sysfs location. Ignore previous whinge.

Comment 9

[Doug Turner (:dougt)]

michael, don't worry about pref'ing the update interval, or filtering the update notifications.  We can do this in the super class. i have a patch that I need to attach somewhere that does this.

Comment 10

[Doug Turner (:dougt)]

michael, we g2g on the review then?  should I ignore comments 7 and 8?

Comment 11

[Michael Ventnor]

The latest patch implements filtering of update notifications, should I remove that first?
Also, you said you were pref'ing the update interval, what should I make the backend timer interval (currently 50ms)?

Comment 12

[Doug Turner (:dougt)]

Michael, take a look at the work in progress in bug 512237.  I think we need to tweak it a bit due to the lack of std::abs on wince.

Comment 13

[Michael Ventnor]

(In reply to comment #12)
> Michael, take a look at the work in progress in bug 512237.  I think we need to
> tweak it a bit due to the lack of std::abs on wince.

I think there's PR_ABS, though.
I'll update the patch based on that.

Comment 14

[Michael Ventnor]

Attachment: Patch 3

Comment 15

[Michael Ventnor]

After looking further into things, I think 255.0 is the wrong value for Thinkpads. Not that I have one. :(

Comment 16

[Michael Ventnor]

Attachment: Patch 4

hdaps-gl tells us that HDAPS needs its own normalisation factor.

Comment 17

[Michael Ventnor]

Attachment: Patch 4.1

Whoops, had a typo in there.

Comment 18

[Doug Turner (:dougt)]

Comment on attachment 396970 [details] [diff] [review]
Patch 4.1



>+// The proposal for accelerometer events defines values between -1 and 1.
>+#define APPLE_NORMALIZE_FACTOR 255.0
>+#define IBM_NORMALIZE_FACTOR 180.0
>+#define MAEMO_NORMALIZE_FACTOR -1000.0


Lets drop these in favor of adding a X_NORMALIZE_FACTOR, Y_NORMALIZE_FACTOR, Z_NORMALIZE_FACTOR to the Accelerometer.  

>+  // MacBook
>+  {"/sys/devices/platform/applesmc.768/position",
>+   "/sys/devices/platform/applesmc.768/calibrate",
>+   eAppleSensor
     255.0
     255.0
    -255.0
},


I wonder if you can change this switch to be completely data driven (so that you don't have a switch at all?)  Maybe create another structure that you can copy Accelerometer into, and have the 3 (x,y,z) calibration fields in it?


>+  switch (self->mType) {
>+    case eAppleSensor:
>+    {




>+    if (access(gAccelerometers[i].mPosition, R_OK) != 0)
>+      continue;

Can we just call fopen instead of access?

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 396970 [details] [diff] [review]
Patch 4.1

btw, they don't have to be actually name X_NORM*_FACTOR :-)

Comment 20

[Michael Ventnor]

(In reply to comment #18)
> (From update of attachment 396970 [details] [diff] [review])
> 
> 
> >+// The proposal for accelerometer events defines values between -1 and 1.
> >+#define APPLE_NORMALIZE_FACTOR 255.0
> >+#define IBM_NORMALIZE_FACTOR 180.0
> >+#define MAEMO_NORMALIZE_FACTOR -1000.0
> 
> 
> Lets drop these in favor of adding a X_NORMALIZE_FACTOR, Y_NORMALIZE_FACTOR,
> Z_NORMALIZE_FACTOR to the Accelerometer.  
> 
> >+  // MacBook
> >+  {"/sys/devices/platform/applesmc.768/position",
> >+   "/sys/devices/platform/applesmc.768/calibrate",
> >+   eAppleSensor
>      255.0
>      255.0
>     -255.0
> },

I thought about doing that but I didn't like the idea of storing a float and constantly having to retrieve it from the struct. Should I just add it as a magic number rather than a macro?

> I wonder if you can change this switch to be completely data driven (so that
> you don't have a switch at all?)  Maybe create another structure that you can
> copy Accelerometer into, and have the 3 (x,y,z) calibration fields in it?
> 
> 
> >+  switch (self->mType) {
> >+    case eAppleSensor:
> >+    {

I don't really understand what you mean here. The problem is that each accelerometer driver provides the coordinates in different ways that requires their own parsing.

> >+    if (access(gAccelerometers[i].mPosition, R_OK) != 0)
> >+      continue;
> 
> Can we just call fopen instead of access?

IIRC doesn't access detect more errors more cleanly?

Comment 21

[Doug Turner (:dougt)]

> I thought about doing that but I didn't like the idea of storing a float and
constantly having to retrieve it from the struct. Should I just add it as a
magic number rather than a macro?

I just didn't like having this sort of data floating around outside of the structure which already has device specific data (like the file path and sensor type).

>I don't really understand what you mean here.

Just ignore.  I was hoping that it could be data driven, but it does look like each does a slightly different thing :(

> IIRC doesn't access detect more errors more cleanly?

Such as?  Either you get a FILE *  or not.  that is all we need right?

Comment 22

[Michael Ventnor]

Attachment: Patch 5

Call me old fashioned, but I'd prefer not to have to read from memory any more than we have to when we're doing polling as frequent as this.

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 397243 [details] [diff] [review]
Patch 5

olli, would you mind looking at this patch as well.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 397243 [details] [diff] [review]
Patch 5

>diff --git a/widget/src/gtk2/nsAccelerometerGtk.cpp b/widget/src/gtk2/nsAccelerometerGtk.cpp
Actually, is there anything gtk specific in the implementation?
If not, perhaps the files could be called just nsAccelerometer.h/cpp, or 
nsAccelerometerUnix.h/cpp.
(I'm not sure about the status of QT port, but at least on Linux that could probably use the
same Accelerometer implementation.)

>+void nsAccelerometerGtk::Startup()
...
>+      if (!mCalibrateFile) {
>+        fclose(mPositionFile);
          mPositionFile = NULL;

>+void nsAccelerometerGtk::Shutdown()
...
>+  if (mPositionFile)
>+    fclose(mPositionFile);
if (mPositionFile) {
  fclose(mPositionFile);
  mPositionFile = NULL;
}

...
>+  if (mCalibrateFile)
>+    fclose(mCalibrateFile);
  if (mCalibrateFile) {
    fclose(mCalibrateFile);
    mCalibrateFile = NULL;
  }
  mType = eNoSensor;

Comment 25

[Doug Turner (:dougt)]

good suggestions olli.


Try builds:
https://build.mozilla.org/tryserver-builds/[e-mail]-try-80f4cdc242b9/

Comment 26

[Michael Ventnor]

Attachment: Patch 6

dougt, I'll need you to check this in for me.

If you could make a blog post asking Linux Thinkpad users to test things out, that would also be appreciated :)

Comment 27

[Doug Turner (:dougt)]

okay.  pushed.  Will post something soon -- as soon as there are Minefield nightlies available.

http://hg.mozilla.org/mozilla-central/rev/860134e65933

Great work!

Comment 28

[Doug Turner (:dougt)]

patch 6 is olli's comments incorporated.

Comment 29

[Michael Ventnor]

Oops, I forgot to add the mType = eNoSensor; line into Shutdown() like Olli wanted...

dougt, could you check that line in as a followup?

Comment 30

[Doug Turner (:dougt)]

I think I did:

   2.198 +    fclose(mCalibrateFile);
   2.199 +    mCalibrateFile = nsnull;
   2.200 +  }
   2.201 +
   2.202 +  mType = eNoSensor;
   2.203 +
   2.204 +  if (mUpdateTimer) {
   2.205 +    mUpdateTimer->Cancel();
   2.206 +    mUpdateTimer = nsnull;
   2.207 +  }
   2.208 +}

Comment 31

[Michael Ventnor]

Yeah, that's good.

Comment 32

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ee9dc00f592e

Comment 33

[Oleg Romashin (:romaxa)]

One question about power management :)
Are you parsing acc data all the time? is it going to be disconnected when device goes to idle state or when browser page is not visible on the screen (browser goes to background)?

Comment 34

[Doug Turner (:dougt)]

romaxa, power management was not considered during the implementation of this feature.  We will not disconnect creating events during system idle.  Please file a new bug.