513233 - Implement accelerometer support for Windows ThinkPad

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement)

Description

[Jesper Kristensen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; da; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: 

I would like to add support for the ThinkPad accelerometer under Windows to the new acceleration API from bug 485943.

Reproducible: Always

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Jesper, just to be clear, do you plan on adding this yourself or is this a request for someone else to add the functionality?

Comment 2

[Jesper Kristensen]

I would like to add this myself.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Excellent.  Updating the bug appropriately.

Comment 4

[Jesper Kristensen]

Attachment: Patch 1

Based on Patch 5 from bug 512345.

Comment 5

[Doug Turner (:dougt)]

this is great.  I am doing something very similar on WindowsCE -- load a DLL, call startup, then call getvalues on the timer, and then shutdown.  Maybe we can just put all of this in a common file.  Let me get that patch together (bug 513183).

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 397288 [details] [diff] [review]
Patch 1

I do not think we need a nsAccelerometerThinkPad file.  Take a look at what I did with the CE sensors in the bug I mentioned above.

in the UpdateHandler, could you comment as what these values are -- it isn't obvious to me what they are.

otherwise, looks great.

Comment 7

[Jesper Kristensen]

(In reply to comment #6)
> (From update of attachment 397288 [details] [diff] [review])
> I do not think we need a nsAccelerometerThinkPad file.  Take a look at what I
> did with the CE sensors in the bug I mentioned above.

I think I will wait until that bug is resolved then. Why is the same thing implemented in such different ways in unix (bug 512345) and windows (bug 513183)? And why does each platform have its own update timer?

> in the UpdateHandler, could you comment as what these values are -- it isn't
> obvious to me what they are.

OK

Comment 8

[Doug Turner (:dougt)]

> Why is the same thing implemented in such different ways in unix (bug 512345) and windows (bug 513183)?

what do you mean?

> And why does each platform have its own update timer?

I would imagine that not all platforms need a timer to poll the sensor.  Other platforms may have a callback that gets called (like windows 7)

Comment 9

[Jesper Kristensen]

Attachment: Patch 2

Now based on Attachment #397881 [details] [diff].

I am not sure if we need another h file or if I should reuse nsAccelerometerCE.h instead, as it has exactly the same content.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 398099 [details] [diff] [review]
Patch 2

wow, this is pretty similar to the others.  I think you are right, we should just roll this into the existing "CE" version, and rename it to "Win".

Also, Lets just use LoadLibrary like the other implementations on WinCE.

Comment 11

[Jesper Kristensen]

Should I only merge the CE and Win h files or also the cpp files (moving the #ifdefs in nsWinWidgetFactory.cpp to nsAccelerometerWin.cpp) ?

Comment 12

[Doug Turner (:dougt)]

merge everything such that there is only one c++ and one h for windows.

Comment 13

[Jesper Kristensen]

Attachment: Patch 3

Comment 14

[Doug Turner (:dougt)]

Comment on attachment 398148 [details] [diff] [review]
Patch 3

Looks great!.  

Please fix up these nits, and r+

wrap the long line:

+  gShockproofGetAccelerometerData = (ShockproofGetAccelerometerData) GetProcAddress(mLibrary, "ShockproofGetAccelerometerData");

typo:
accekerometer


No spaces before the #

+  #ifdef WINCE_WINDOWS_MOBILE

+  #else

+  #endif

Comment 15

[Jesper Kristensen]

Attachment: Patch 3.1

I think I don't need to ask for review again, but I am not 100% sure, so I will ask just to be safe.

Comment 16

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/d07a46ba2eba

Thanks Jesper!

Comment 17

[Doug Turner (:dougt)]

backed up.  busted win32.  mLibrary not defined.

Comment 18

[Jesper Kristensen]

(In reply to comment #17)
> backed up.  busted win32.  mLibrary not defined.

This seems to be from bug 513183, but was not discovered because tinderbox does not make test builds for Windows Mobile? Will you fix this, Doug, or should I try?

Comment 19

[Doug Turner (:dougt)]

i have a fix, and will push when it passes try.

Comment 20

[Doug Turner (:dougt)]

relanded:
http://hg.mozilla.org/mozilla-central/rev/101ba406c527

Comment 21

[Doug Turner (:dougt)]

Bustage, wtf.  pushed to try didn't catch this.  why don't we have wince build machines yet.


e:/builds/moz2_slave/mozilla-central-wince/build/widget/src/windows/nsAccelerometerWin.cpp(311) : error C2664: 'LoadLibraryW' : cannot convert parameter 1 from 'const char [11]' to 'LPCWSTR'
        Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast

Comment 22

[Jesper Kristensen]

Attachment: Patch 3.2

Seems like the setup of Tinderbox is a little different than my MozillaBuild installation. I guess this will fix it, but I am not sure as I cannot reproduce the compile error.

Comment 23

[Doug Turner (:dougt)]

I think the problem is that the thinkpad code probably shouldn't run on WINCE.  Basically, we should play with the ifdefs.

Comment 24

[Doug Turner (:dougt)]

Attachment: patch (with correct ifdefs)

if this passes try, i will push to mc later today.

Comment 26

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/82c8c0993190

Comment 27

[Jo Hermans]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090918 Minefield/3.7a1pre

Seems to work for me (Lenovo T400 on Windows XP) using the testcase at <http://people.mozilla.org/~dougt/ball.html>.

Comment 28

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/414bd85cc0d0

Comment 29

[Helder "Lthere" Magalhães]

Should this be marked as dependent on bug 485943, in order to use that issue as a tracker for implementations? It seems that bug 512345 is already doing it with this is mind...? ;-)

Comment 30

[Vít Ondruch]

It work on my Lenovo Z61m on Windows XP, however some calibration of sensor is necessary. My laptop acts as titled every time I start some of the demos.

Comment 31

[Jesper Kristensen]

Attachment: Accelerometer test app

(In reply to comment #30)
> It work on my Lenovo Z61m on Windows XP, however some calibration of sensor is
> necessary. My laptop acts as titled every time I start some of the demos.

I determined the balance point experimentally to be x: between 529 and 528, y: between 525 and 527 on my T500 using the attached test program. Getting data from other laptops would be great, I think.

Comment 32

[Jo Hermans]

The test application didn't work for me on a Lenovo T400 with windows XP SP 3, I got the following error :

"The application has failed to start because the application configuration is incorrect. Reinstalling the application may fix this problem."

See <http://social.msdn.microsoft.com/forums/en-US/vcgeneral/thread/36971526-95f3-4a9f-a601-1843c86332c1/> for an explanation. The easiest fix is to link statically.

Comment 33

[Jesper Kristensen]

(In reply to comment #32)
I guess you need to install http://www.microsoft.com/downloads/details.aspx?FamilyID=200B2FD9-AE1A-4A14-984D-389C36F85647&displaylang=en

Comment 34

[Vít Ondruch]

(In reply to comment #33)
> (In reply to comment #32)
> I guess you need to install
> http://www.microsoft.com/downloads/details.aspx?FamilyID=200B2FD9-AE1A-4A14-984D-389C36F85647&displaylang=en

It doesn't work either for me and I am sure that I have VC2005 redist installed ...

Comment 35

[Jesper Kristensen]

Attachment: Accelerometer test app

Try this test app instead. I have checked that it works on another computer.

Comment 36

[Jo Hermans]

results on on Lenovo T400, on Windows XP SP 3 (on a level surface) :

dll 3604480
func 3610566
Status: 0
x:  522, y:  517
xx: 522, yy: 517
x0: 522, y0: 517
temp: -1

Comment 37

[Vít Ondruch]

Results from Lenovo Z61m

dll 268435456
func 268441803
Status: 0
x:  502, y:  496
xx: 502, yy: 496
x0: 502, y0: 495
temp: 48


However I expect that results from my docking station will be different. I will post them tomorrow.

Comment 38

[Vít Ondruch]

Results from Lenovo Z61m sitting in my docking station

dll 268435456
func 268441803
Status: 0
x:  518, y:  497
xx: 517, yy: 496
x0: 516, y0: 496
temp: 44

Comment 39

[Daniel B.]

ThinkPad R61 on desk:

dll 1845690368
func 1845696715
Status: 2
x:  522, y:  526
xx: 521, yy: 525
x0: 521, y0: 525
temp: -1

Haven't tried a 3.6 build though.

Comment 40

[Jesper Kristensen]

Attachment: Accelerometer test app (web page)

Should there be more willing to provide data on the level point, this should be a little easier to do.

Comment 41

[Helder "Lthere" Magalhães]

(In reply to comment #0)
> I would like to add support for the ThinkPad accelerometer under Windows
> to the new acceleration API from bug 485943.

I'd suggest adding a dependency from bug 485943 (not only is what's happening but also in order to help crawling for related issue and also possibly use it as a meta-bug...?). ;-)

Comment 42

[Helder "Lthere" Magalhães]

(In reply to comment #41)
> I'd suggest adding a dependency from bug 485943

Naturally, it's "to" and not "from": this issue should be depend on the other one. As a side note/example, I've done this in bug 521902. Sorry for the bug spam!

Comment 43

[[e-mail]]

So, today's Google home page uses the accelerator API. I don't have an accelerator, so I get two assertions, one when leaving the page to complain that the sensor didn't start up, and one when returning to the page to complain that the sensor has already been created! I guess you want new bugs on this.