Closed Bug 513183 Opened 10 years ago Closed 10 years ago

Implement accelerometer support for Windows Mobile (HTC and Samsung)

Categories

(Core :: Widget: Win32, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dougt, Assigned: dougt)

Details

Attachments

(3 files, 4 obsolete files)

See bug 485943.

This is the widget support for the windows mobile devices that I have (an HTC device and a Samsung device)
patching coming up.  probably tomorrow or later tonight....
Attached patch patch v.1 (obsolete) — Splinter Review
i haven't been able to try the samsung implementation.  The libraries do not exist on my device.  Not sure why.
Attachment #397318 - Flags: review?(bugmail)
This package must be installed on the Samsung devices to get the Library.
Attachment #397318 - Attachment is obsolete: true
Attachment #397318 - Flags: review?(bugmail)
Attached patch patch v.2 (obsolete) — Splinter Review
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 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.
Attachment #397344 - Flags: review?(Olli.Pettay)
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.
Attachment #397643 - Flags: review?(Olli.Pettay)
Attachment #397344 - Flags: review?(Olli.Pettay)
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.
(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.
Well, why don't we then let values smaller than -1 or greater than 1?
Dougt, you designed the API, any comments?
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.
Attached patch patch v.3 (obsolete) — Splinter Review
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.
Attachment #397344 - Attachment is obsolete: true
Attachment #397643 - Attachment is obsolete: true
Attachment #397643 - Flags: review?(Olli.Pettay)
kantha, thank you.  works great.
(olli, yes, i will remove the printf!)
Attachment #397681 - Flags: review?(Olli.Pettay)
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.
Attachment #397681 - Flags: review?(Olli.Pettay) → review+
Attached patch patch to pushSplinter Review
Attachment #397681 - Attachment is obsolete: true
Attachment #397881 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/55def08d14ab
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: blocking1.9.2?
Attachment #397881 - Flags: approval1.9.2?
Attachment #397881 - Flags: approval1.9.2? → approval1.9.2+
Flags: blocking1.9.2?
You need to log in before you can comment on or make changes to this bug.