827707 - [GonkSensor] sensor field in sensors_event_t doesn't mean the index of sensors list from HAL.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

Currently we assumed the "sensor" field in sensors_event_t is used as an index for indicating which one in sensors list reported this event. But actually this "sensor" field is mapped to "handle" field in sensor_t. And this "handle" field may not the same with the order of position in sensors list.

In one of our partner's device, it has 3 sensors but their handles are set as 0, 4, 5. so we will dismiss events from handle 4 & 5.

Comment 1

[Marco Chen [:mchen]]

Attachment: patch v1

Comment 2

[Michael Wu [:mwu]]

Comment on attachment 699067 [details] [diff] [review]
patch v1

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

Is the sensor type set correctly in the partner's sensor hal? This code is mostly meant as a fallback for the emulator which is implemented incorrectly..

::: hal/gonk/GonkSensor.cpp
@@ +161,5 @@
>  static sensors_poll_device_t* sSensorDevice;
>  static sensors_module_t* sSensorModule;
>  
> +static int
> +FindSensorIndex(const sensor_t* aSensors, int aSize, int aHandle)

This function should just be inlined unless we're planning to reuse it somewhere.

@@ +165,5 @@
> +FindSensorIndex(const sensor_t* aSensors, int aSize, int aHandle)
> +{
> +  MOZ_ASSERT(aSensors != nullptr);
> +  int index;
> +  for (int index = 0; index < aSize; index++) {

index is defined twice

Comment 3

[Marco Chen [:mchen]]

Attachment: patch v2

The info of sensors list from a specific partner's device is
  index 0: type = accelerometer, handle = 0
  index 1: type = proximity, handle = 3
  index 2: type = light, handle = 4
The example of sensor event is
  type = light, sensor = 4

The code in the link used sensor field as an index of sensors list.
http://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp#185
But actually it doesn't indicate to index but the handle field of sensors list.

So in this case, the checking will be failed due to event's sensor field for light will be 4 which is greater then maximum index.

The patch here 
  1. removed that wrong checking.
     http://mxr.mozilla.org/mozilla-central/source/hal/gonk/GonkSensor.cpp#185
  2. Add a macro for changing sensor field (handle) to real index in sensors list.
  3. modify the rest places using sensor field with new macro.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 702142 [details] [diff] [review]
patch v2

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

::: hal/gonk/GonkSensor.cpp
@@ +160,5 @@
>  static base::Thread* sPollingThread;
>  static sensors_poll_device_t* sSensorDevice;
>  static sensors_module_t* sSensorModule;
>  
> +#define FIND_SENSOR_INDEX_BY_HANDLE(aSensors, aSize, aHandle, aIndex)     \

I meant inline this code in the source rather than force the compiler to inline. The compiler should have inlined the function anyway.

Comment 5

[Marco Chen [:mchen]]

Attachment: patch v3

Follow the comment.

Comment 6

[Marco Chen [:mchen]]

Comment on attachment 702637 [details] [diff] [review]
patch v3

Thanks for the review.

Comment 7

[Marco Chen [:mchen]]

Hi Michael,

Could you help to review this patch?
Thanks.

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 702637 [details] [diff] [review]
patch v3

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

Looks good. I just have one suggestion to clarify the wording of the warning.

::: hal/gonk/GonkSensor.cpp
@@ +196,3 @@
>          } else {
> +          LOGW("buffer's type field is SENSOR_UNKNOWN of hal sensor type," \
> +               "or buffer's sensor field didn't match any sensor handle");

"Could not determine sensor type of event"

Comment 9

[Marco Chen [:mchen]]

Result of Try Server.

https://tbpl.mozilla.org/?tree=Try&rev=1167341b4699

Comment 10

[Marco Chen [:mchen]]

Attachment: Patch Checkin-Version

Follow the comment and add reviewer name.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f487741e08

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4f487741e08

Comment 13

[Wayne Chang [:wchang]]

Marking hd+ as needed specifically for helix device.

Comment 15

[Wayne Chang [:wchang]]

Marco, can you merge this to hd?

Comment 16

[Marco Chen [:mchen]]

Hi sheriff,

I tried that this patch can be applied to 1.1.0hd branch directly. 
Please help to uplift and thanks for you help.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/0f06fdd1ab4f

Sorry for not getting this earlier. This bug was resolved literally 1 day earlier than my queries were looking for.

Comment 18

[William Hsu [:whsu]]

I still can reproduce this bug on following build.
I will test it again after the patch uplift to V1.1.0 HD
* Test build:(Mozilla-b2g18_v1_1_0_hd-helix/2013-08-12-04-22-03)
  + Mercurial-Information
    - Gecko revision="afd50da34b9f"
  + Git-information
    - Gaia revision="134356dc21d0627455f8bc536b140a18f39e20e9"

Comment 19

[William Hsu [:whsu]]

I cannot reproduce this bug on following build.
I will test it again after the patch uplift to V1.1.0 HD
* Test build:(Mozilla-b2g18_v1_1_0_hd-helix/2013-08-22-04-22-00)
  + Mercurial-Information
    - Gecko revision="84e2aa041c3c"
  + Git-information
    - Gaia revision="e3a6815b10080f9e257640cd068bf8efd86e04c7"

Marked as "Verified".