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

VERIFIED FIXED

Status

Firefox OS
General
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: mchen, Assigned: mchen)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:hd+, b2g18 wontfix, b2g18-v1.0.0 unaffected, b2g18-v1.0.1 unaffected, b2g-v1.1hd fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 699067 [details] [diff] [review]
patch v1
(Assignee)

Updated

5 years ago
Attachment #699067 - Flags: review?(mwu)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
Created attachment 702142 [details] [diff] [review]
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.
Attachment #699067 - Attachment is obsolete: true
Attachment #699067 - Flags: review?(mwu)
(Assignee)

Updated

5 years ago
Attachment #702142 - Flags: review?(mwu)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Created attachment 702637 [details] [diff] [review]
patch v3

Follow the comment.
Attachment #702142 - Attachment is obsolete: true
Attachment #702142 - Flags: review?(mwu)
(Assignee)

Comment 6

5 years ago
Comment on attachment 702637 [details] [diff] [review]
patch v3

Thanks for the review.
Attachment #702637 - Flags: review?(mwu)
(Assignee)

Comment 7

5 years ago
Hi Michael,

Could you help to review this patch?
Thanks.
Flags: needinfo?(mwu)

Comment 8

5 years ago
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"
Attachment #702637 - Flags: review?(mwu) → review+
Flags: needinfo?(mwu)
(Assignee)

Comment 9

5 years ago
Result of Try Server.

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

Comment 10

5 years ago
Created attachment 705704 [details] [diff] [review]
Patch Checkin-Version

Follow the comment and add reviewer name.
Attachment #702637 - Attachment is obsolete: true
Attachment #705704 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f487741e08
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f4f487741e08
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Marking hd+ as needed specifically for helix device.
blocking-b2g: --- → hd+

Updated

4 years ago
Duplicate of this bug: 898950
(Assignee)

Updated

4 years ago
status-b2g-v1.1hd: --- → affected
Marco, can you merge this to hd?
Flags: needinfo?(mchen)
(Assignee)

Comment 16

4 years ago
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.
Flags: needinfo?(mchen)
Keywords: checkin-needed
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.
status-b2g18: --- → wontfix
status-b2g18-v1.0.0: --- → unaffected
status-b2g18-v1.0.1: --- → unaffected
status-b2g-v1.1hd: affected → fixed
Keywords: checkin-needed

Comment 18

4 years ago
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

4 years ago
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".
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.