625664 - accelerometer support using Windows 7 Sensor API

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Makoto Kato [:m_kato]]

Sensor API has accelerometer supports.  We should use this.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: v1

Comment 2

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 3

[Makoto Kato [:m_kato]]

sensorapi.lib has CLSID information for Sensor API, so we have to link it.  But, since there is no export function, linked binary works on 2000 and XP.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 519369 [details] [diff] [review]
fix v2

Thanks for jumping on this!

Could you use CComQIPtr instead of raw pointers.  They are similar to our nsCOMPtr's and will save you from worrying about releasing.

throw up another patch.  i think you are very close.

Comment 5

[Makoto Kato [:m_kato]]

(In reply to comment #4)
> Comment on attachment 519369 [details] [diff] [review]
> fix v2
> 
> Thanks for jumping on this!
> 
> Could you use CComQIPtr instead of raw pointers.  They are similar to our
> nsCOMPtr's and will save you from worrying about releasing.

Doug, could we use ATL?  CComQIPtr is ATL's class.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: fix v3

Comment 7

[Jim Mathies [:jimm]]

Kinda related - shouldn't nsAccelerometerSystem be moved to dom?

Comment 8

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

Maybe, but please don't move files before hg is fixed
http://mercurial.selenic.com/bts/issue1576

Comment 9

[Doug Turner (:dougt)]

jimm, probably /dom/system.  Do you know the answer to Makoto's question?


Also, how do I test this?  Other than getting-a-windows-laptop...

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #9)
> jimm, probably /dom/system.  Do you know the answer to Makoto's question?

I've never used ATL in mozilla code, I use our own wrappers. (I believe in at least one build configs, the headers aren't available.)

Comment 11

[Makoto Kato [:m_kato]]

(In reply to comment #9)
> 
> Also, how do I test this?  Other than getting-a-windows-laptop...

You have to get Laptop that has accelerometer with Sensor API such as VAIO P (http://www.sonystyle.com/webapp/wcs/stores/servlet/ProductDisplay?catalogId=10551&storeId=10151&langId=-1&productId=8198552921666173660).  This code is tested by my VAIO P.

Doug, Should I change reviewer to anyone? jimm?

Also, the latest fix uses nsRefPtr instead of CComQIPtr.  Although I don't think nsRefPtr is best for Windows COM, it is used by netwerk/system/win32 for same reason.

Comment 12

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

Could you perhaps provide tryserver builds so that people could 
try out the patch?
(I don't have dev tools on Windows so can't compile myself.
 I wonder if my Vaio VPC-SB1A9E has accelerometer.)

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 522335 [details] [diff] [review]
fix v3

the patch looks fine, but:

1) I am unsure about the build changes -- ted or some build peer should be consulted.

2) I am not sure nsRefPtr is the best substitue for CComQIPtr, but if it works, great.

3) I am unable to verify that the axis data is correct since I do not have a similar laptop.  Please ensure that the demo's behave properly.


Lastly, do you have access to a gyroscope on the laptop you have -- is there a Window 7 defined sensor for that?

Comment 14

[Makoto Kato [:m_kato]]

Humm, due to bug 615597, this fix doesn't work.  When I submit a fix for reviewing, it works fine.

I think that new DeviceOrientation code may not work on other platforms except to Android.

Comment 15

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

Why does using a COM API require linking to an import library?

Comment 16

[Makoto Kato [:m_kato]]

(In reply to comment #15)
> Why does using a COM API require linking to an import library?

CLSID_SensorManager isn't include in uuid.lib.  It is into sensorapi.lib
If we define CLSID_SensorManager into our source, we doesn't need import library.

Comment 17

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

fwiw I'd prefer that we defined the CLSID in our own source rather than needing to do this build hackery to link sensorapi.lib.

Comment 18

[Makoto Kato [:m_kato]]

(In reply to comment #17)
> fwiw I'd prefer that we defined the CLSID in our own source rather than
> needing to do this build hackery to link sensorapi.lib.

Kyle, how about "#pragma comment(lib, "sensorsapi.lib")" instead of?

If not linking sensorspai.lib, I have to add the following.

const CLSID CLSID_SensorManager = ...;
const IID IID_ISensorManager = ...;
REFSENSOR_TYPE_ID SENSOR_TYPE_ACCELEROMETER_3D = ...;
REFPROPERTYKEY SENSOR_DATA_TYPE_X_G = ...;
REFPROPERTYKEY SENSOR_DATA_TYPE_Y_G = ...;
REFPROPERTYKEY SENSOR_DATA_TYPE_Z_G = ...;

Comment 19

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

Sure, that works too.

Comment 20

[Jim Mathies [:jimm]]

(In reply to comment #19)
> Sure, that works too.

Sensorsapi.dll is only available on Windows 7.

Comment 21

[Makoto Kato [:m_kato]]

(In reply to comment #20)
> (In reply to comment #19)
> > Sure, that works too.
> 
> Sensorsapi.dll is only available on Windows 7.

See comment #16.  This is static library.  This DLL is loaded via COM and this #pragma doesn't import any global values and functions.

So, the binary that has this fix works on XP/Vista.  (Of course XP and Vista don't have Sensor API and DLL).

Comment 22

[Jim Mathies [:jimm]]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Sure, that works too.
> > 
> > Sensorsapi.dll is only available on Windows 7.
> 
> See comment #16.  This is static library.  This DLL is loaded via COM and
> this #pragma doesn't import any global values and functions.
> 
> So, the binary that has this fix works on XP/Vista.  (Of course XP and Vista
> don't have Sensor API and DLL).

Ah, sorry, should have read back farther in the history.

Comment 23

[(not currently active) Ted Mielczarek]

Comment on attachment 522335 [details] [diff] [review]
fix v3

Review of attachment 522335 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/build/Makefile.in
@@ +289,5 @@
>  OS_LIBS += -framework OpenGL
>  endif
>  
> +ifeq ($(OS_ARCH),WINNT)
> +ifneq (,$(filter-out 05020000 06000000,$(MOZ_WINSDK_TARGETVER)))

This is okay, but we should probably just drop support for anything older than the Windows 7 SDK at this point. People have had plenty of time to transition. (You don't have to do so as part of this patch.)

Comment 24

[Makoto Kato [:m_kato]]

try server build
ftp://ftp.mozilla.org/pub/firefox/try-builds/[e-mail]-e57bf881795b/try-win32/

You may need sensor API driver.  I don't know that sony releases it for all regions.

Comment 25

[JL]

Hi guys, I just want to tell you about this thing I found:

http://feishare.com/latest/remotesensors-development-detail

It's an app for Android together with server software for Windows.
You install the driver for Windows and run the server, then you connect the phone to it with the app over WLAN or Bluetooth.
Then you get access to the phone's sensors in Windows. I have tried it, and accelerators work (there's a demo on that site, and it's working).

Essentially that could turn our phones into Wiimote-like game controllers for HTML5 games. :)

Comment 26

[Makoto Kato [:m_kato]]

Attachment: fix v4

rewriting for hal.

- build env of min requirement is Windows 7 SDK.  so I don't need SDK check
- adding library is static library that has GUIDs only.  (So built binary works on Windows XP, but Windows XP has no sensor API, sensor doesn't work.)

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 612119 [details] [diff] [review]
fix v4

Review of attachment 612119 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/windows/WindowsSensor.cpp
@@ +147,5 @@
> +                                 IID_IPortableDeviceValues,
> +                                 getter_AddRefs(values)))) {
> +    if (SUCCEEDED(values->SetUnsignedIntegerValue(
> +                    SENSOR_PROPERTY_CURRENT_REPORT_INTERVAL,
> +                    DEFAULT_SENSOR_POLL))) {

Seems like this would generate a lot of event traffic on a device that supports the sensor and the UI is subscribed to these events. Maybe we should only send events if the the value has actually changed?

We'll be using some of this code for Win8 metro. For winrt Microsoft had to implement something similar to this - 

http://msdn.microsoft.com/en-us/library/windows/apps/windows.devices.sensors.accelerometer.aspx

Comment 28

[Makoto Kato [:m_kato]]

(In reply to Jim Mathies [:jimm] from comment #27)
> Comment on attachment 612119 [details] [diff] [review]
> fix v4
> 
> Review of attachment 612119 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/windows/WindowsSensor.cpp
> @@ +147,5 @@
> > +                                 IID_IPortableDeviceValues,
> > +                                 getter_AddRefs(values)))) {
> > +    if (SUCCEEDED(values->SetUnsignedIntegerValue(
> > +                    SENSOR_PROPERTY_CURRENT_REPORT_INTERVAL,
> > +                    DEFAULT_SENSOR_POLL))) {
> 
> Seems like this would generate a lot of event traffic on a device that
> supports the sensor and the UI is subscribed to these events. Maybe we
> should only send events if the the value has actually changed?

This setting is for the sensor associated by ISensor.  So this code is that event is sent by accelerometer only per setting interval.

Also ,some drivers may not be able to set 100ms because min internal of sensor driver is over 100ms.  Should we change to nsITimer implementation?

 
> We'll be using some of this code for Win8 metro. For winrt Microsoft had to
> implement something similar to this - 
> 
> http://msdn.microsoft.com/en-us/library/windows/apps/windows.devices.sensors.
> accelerometer.aspx

ISensor is on Desktop App only, so we need metro specific implementation for accelerometer that you point.

Comment 29

[Doug Turner (:dougt)]

Comment on attachment 612119 [details] [diff] [review]
fix v4

Review of attachment 612119 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/windows/WindowsSensor.cpp
@@ +72,5 @@
> +    hr = aReport->GetSensorValue(SENSOR_DATA_TYPE_ACCELERATION_X_G, &v);
> +    if (FAILED(hr)) {
> +      return hr;
> +    }
> +    values.AppendElement(float(v.dblVal * 10));

Why are you multiplying by 10 here?  Your units should be in m/s^2.  Orientation looks like http://developer.android.com/images/axis_device.png


Same for the other values.

Comment 30

[Jim Mathies [:jimm]]

(In reply to Makoto Kato from comment #28)
> 
> This setting is for the sensor associated by ISensor.  So this code is that
> event is sent by accelerometer only per setting interval.

Still seems like you are asking for a constant stream of sensor events, every 100msec, regardless if the value has changed.

> 
> Also ,some drivers may not be able to set 100ms because min internal of
> sensor driver is over 100ms.  Should we change to nsITimer implementation?
> 
>  
> > We'll be using some of this code for Win8 metro. For winrt Microsoft had to
> > implement something similar to this - 
> > 
> > http://msdn.microsoft.com/en-us/library/windows/apps/windows.devices.sensors.
> > accelerometer.aspx
> 
> ISensor is on Desktop App only, so we need metro specific implementation for
> accelerometer that you point.

Are you sure about that? I'd be willing to bet the winrt apis are calling the same com interfaces we're calling here, in which case this interface would work for both.

(Technically in metro we are still a desktop app, it's just that we are granted special privileges to access metro.)

Once this lands I can test to see.

Comment 31

[Makoto Kato [:m_kato]]

(In reply to Doug Turner (:dougt) from comment #29)

> Why are you multiplying by 10 here?  Your units should be in m/s^2. 
> Orientation looks like http://developer.android.com/images/axis_device.png

Ahh, I misunderstand this.  It should be 9.8...

Comment 32

[Makoto Kato [:m_kato]]

(In reply to Jim Mathies [:jimm] from comment #30)
> > ISensor is on Desktop App only, so we need metro specific implementation for
> > accelerometer that you point.
> 
> Are you sure about that? I'd be willing to bet the winrt apis are calling
> the same com interfaces we're calling here, in which case this interface
> would work for both.
> 
> (Technically in metro we are still a desktop app, it's just that we are
> granted special privileges to access metro.)

Really?  MSDN says that this is desktop only.  I don't test on Metro base code yet.

> Once this lands I can test to see.

When isn't working, I will write WinRT sytle code after landing.

Comment 33

[Makoto Kato [:m_kato]]

Also, on Win8/arm target, there is no some libs for sensors.  So we need WinRT C++ code for arm target.

Comment 34

[Makoto Kato [:m_kato]]

Attachment: v5

Comment 35

[Doug Turner (:dougt)]

Comment on attachment 613220 [details] [diff] [review]
v5

Review of attachment 613220 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/windows/WindowsSensor.cpp
@@ +87,5 @@
> +    hr = aReport->GetSensorValue(SENSOR_DATA_TYPE_ACCELERATION_Z_G, &v);
> +    if (FAILED(hr)) {
> +      return hr;
> +    }
> +    values.AppendElement(float(-v.dblVal * MEAN_GRAVITY));

can you confirm that you need to use the -1 here and on Y

Comment 36

[Makoto Kato [:m_kato]]

(In reply to Doug Turner (:dougt) from comment #35)
> can you confirm that you need to use the -1 here and on Y

Yes. This value of Windows 7 sensoer api is reversed.

Comment 37

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/61c2bc0d9a41

Comment 38

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/61c2bc0d9a41