513183 - Implement accelerometer support for Windows Mobile (HTC and Samsung)

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Doug Turner (:dougt)]

See bug 485943.

This is the widget support for the windows mobile devices that I have (an HTC device and a Samsung device)

Comment 1

[Doug Turner (:dougt)]

patching coming up.  probably tomorrow or later tonight....

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

i haven't been able to try the samsung implementation.  The libraries do not exist on my device.  Not sure why.

Comment 3

[kantha kanchiraju]

Attachment: Library for Samsung Devices

This package must be installed on the Samsung devices to get the Library.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 5

[Doug Turner (:dougt)]

kkanchir - can you try out this patch and/or look at the code.  It isn't working.  I am unsure why we are failing to load the symbols.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 397344 [details] [diff] [review]
patch v.2

olli, similar to what you review for Unix.

I am somewhat worried about the SMISensor implementation as it isn't working for me.  Olli, would you mind reviewing the code with the assumption that SMISensor is going to be #ifdef off if I can't get data from the sensor.  Fwiw, if the user doesn't install the software above, they will not have either of the two dll's that we are looking for.

Comment 7

[kantha kanchiraju]

Attachment: Orientation Patch with fix for SMISensor

Doug: SmiSensorServer_1.dll is availble on the device but doesnt have the exports. SamsungMobilesdk_1.dll has the right exports and it is working. I tested the SMISensor class in a Seperate App. 
Patch has the necessary changes.

Comment 8

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

Comment on attachment 397643 [details] [diff] [review]
Orientation Patch with fix for SMISensor


>+PFN_HTCSensorOpen           pfnHTCSensorOpen;
>+PFN_HTCSensorClose          pfnHTCSensorClose;
>+PFN_HTCSensorGetDataOutput  pfnHTCSensorGetDataOutput;
Could you initialize these to some reasonable value.
Same also elsewhere, and make sure the values are cleared when
the variables don't point to a valid value anymore.

>+void
>+HTCSensor::Shutdown()
>+{
>+  NS_ASSERTION(mHTCHandle, "mHTCHandle should not be null at shutdown!");
Can this assertion fire if Startup fails?

>+void nsAccelerometerWin::Startup()
>+{
>+  NS_ASSERTION(!mLibrary, "mLibrary should be null.  Startup called twice?");
>+
>+  PRBool started = PR_FALSE;
>+  mSensor = new SMISensor();
>+  started = mSensor->Startup();
This crashes if OOM.

>+  if (!started) {
>+    mSensor = new HTCSensor();
>+    started = mSensor->Startup();
This crashes if OOM.

>+class nsAccelerometerWin : public nsAccelerometer
Why nsAccelerometerWin? The filename is nsAccelerometerCE


>+{
>+ public:
>+  nsAccelerometerWin();
>+  ~nsAccelerometerWin();
>+
>+  void Startup();
>+  void Shutdown();
>+
>+  nsCOMPtr<nsITimer> mUpdateTimer;
>+  static void UpdateHandler(nsITimer *aTimer, void *aClosure);
>+
>+  class nsAutoPtr<Sensor> mSensor;
Why 'class ...'?

> nsAccelerometer::AccelerationChanged(double x, double y, double z)
> {
>-  if (x > 1 || y > 1 || z > 1 || x < -1 || y < -1 || z < -1)
>-    return;
>+  if (x > 1)
>+    x = 1;
>+  else if (x < -1)
>+    x < -1;
>+
>+  if (y > 1)
>+    y = 1;
>+  else if (y < -1)
>+    y = -1;
>+
>+  if (z > 1)
>+    z = 1;
>+  else if (z < -1)
>+    z = -1;
Why this change? If some nsAccelerometer implementation calls this method
with values outside (-1, 1) range, that is a bug in the implementation which
should be fixed.
Actually, there could be just an assertion to check the values.

Comment 9

[Jesper Kristensen]

(In reply to comment #8)
> >-  if (x > 1 || y > 1 || z > 1 || x < -1 || y < -1 || z < -1)
> >-    return;
> >+  if (x > 1)
...
> Why this change? If some nsAccelerometer implementation calls this method
> with values outside (-1, 1) range, that is a bug in the implementation which
> should be fixed.

Why would that be a bug? Wouldn't it just say that the device is experiencing an acceleration greater than one G in one of the measured directions? I don't see any reasons why that should not happen.

Comment 10

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

Well, why don't we then let values smaller than -1 or greater than 1?
Dougt, you designed the API, any comments?

Comment 11

[Doug Turner (:dougt)]

jesper, the api returns values between -1 and 1:

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMOrientationEvent.idl#49

It could be changed to report back the G force experienced by the device; Maybe that is the right thing to do, or not.  Either way, I think it is a separate bug.

I think olli's comment is right -- for now -- we should change the bounding to assertions.

Olli, i will make the other corrections. This change:

>+class nsAccelerometerWin : public nsAccelerometer
Why nsAccelerometerWin? The filename is nsAccelerometerCE

I left the class name to be nsAccelerometerWin so that the windows module code (nsWinWidgetFactory.cpp) could be the same between CE and desktop.  Then on Windows CE, we can just have the Makefile.in include the "CE" version of the file.

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.3

includes olli's suggestion.  Also not going to filter results. if outside of the -1 ... 1 bound.  I think Jesper is right.  I will follow up w/ a bug to fix the DOM interface.

Comment 13

[kantha kanchiraju]

Attachment: Samsung SDK with Omnia II support

Comment 14

[Doug Turner (:dougt)]

kantha, thank you.  works great.

Comment 15

[Doug Turner (:dougt)]

(olli, yes, i will remove the printf!)

Comment 16

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

Comment on attachment 397681 [details] [diff] [review]
patch v.3

>+  if (pfnHTCSensorOpen != nsnull && pfnHTCSensorClose != nsnull &&
>+      pfnHTCSensorGetDataOutput != nsnull)
>+    {
>+      mHTCHandle = pfnHTCSensorOpen(1);
>+      if (mHTCHandle)
>+	return PR_TRUE;
There are some tabs here. Use spaces.

>+    }


Nit,
if () {
 ...
}
not, 
if ()
{
...
}
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Control_Structures


>+  printf("whee... %f,%f,%f\n", *x, *y, *z);
Remove this :)

>+typedef HANDLE (WINAPI * PFN_HTCSensorOpen)(DWORD);
>+typedef void   (WINAPI * PFN_HTCSensorClose)(HANDLE);
>+typedef DWORD  (WINAPI * PFN_HTCSensorGetDataOutput)(HANDLE, PSENSORDATA);
>+
>+PFN_HTCSensorOpen           pfnHTCSensorOpen;
>+PFN_HTCSensorClose          pfnHTCSensorClose;
>+PFN_HTCSensorGetDataOutput  pfnHTCSensorGetDataOutput;
Could you initialize there to null?
Same also elsewhere with global variables.
Oh, btw, all global variables should start with 'g'.


>+  /*
>+  SmiAccelerometerCapabilities capabilities;
>+  if(pfnSmiAccelerometerGetCapabilites(&capabilities))
>+    return PR_FALSE;
>+
>+  int interval = 100;
>+  if(capabilities.callbackPeriod > interval)
>+    interval = capabilities.callbackPeriod;
>+  */
Don't leave commented-out code. Especially if there are no
comments why the code is there.


>+  printf("whee... %f,%f,%f\n", *x, *y, *z);
!

>diff --git a/widget/src/windows/nsAccelerometerCE.h~ b/widget/src/windows/nsAccelerometerCE.h~
This is file shouldn't be in the patch, right?

> nsAccelerometer::AccelerationChanged(double x, double y, double z)
> {
>-  if (x > 1 || y > 1 || z > 1 || x < -1 || y < -1 || z < -1)
>-    return;
>-
Actually this change should be done in Bug 513910.

Comment 17

[Doug Turner (:dougt)]

Attachment: patch to push

Comment 18

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/55def08d14ab

Comment 19

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7a4898a89997