Skip to content
This repository was archived by the owner on May 15, 2024. It is now read-only.

GH-1148 Check array length on sensors on Android #1150

Merged
merged 8 commits into from
Mar 11, 2020
Merged

GH-1148 Check array length on sensors on Android #1150

merged 8 commits into from
Mar 11, 2020

Conversation

jamesmontemagno
Copy link
Collaborator

Description of Change

Describe your changes here.

Bugs Fixed

Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.

API Changes

None

Behavioral Changes

Describe any non-bug related behavioral changes that may change how users app behaves when upgrading to this version of the codebase.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Updated documentation (see walkthrough)

@jamesmontemagno jamesmontemagno requested a review from Redth March 5, 2020 21:11
@jamesmontemagno jamesmontemagno added this to the 1.5.1 milestone Mar 5, 2020
@jamesmontemagno jamesmontemagno added the awaiting-review This PR needs to have a set of eyes on it label Mar 5, 2020
@Mrnikbobjeff
Copy link
Contributor

I don't think this is the optimal way to handle this (I specifically looked at the orientationsensor implementation, but I assume it holds for other sensors as well). The event includes the sensor firing it, which in turn has a property defining the type. There are some resources pointing out the structure of the data generated by the sensor. I would rather have a map detailing how many values to expect for a given sensor. As far as I can tell from the online resources we can expect that some phones will always have a different sensor value length, as they will always use a different sensor than the one we use which provides four values, see https://developer.android.com/guide/topics/sensors/sensors_position for more detail

@jamesmontemagno jamesmontemagno modified the milestones: 1.5.1, 1.5.2 Mar 9, 2020
@jamesmontemagno
Copy link
Collaborator Author

Each sensor type has a specific number of items in the array. This is documented in the Android docs.

We mostly use motion sensors: https://developer.android.com/guide/topics/sensors/sensors_motion

For Orientation we specifically use: https://developer.android.com/reference/android/hardware/Sensor#TYPE_ROTATION_VECTOR

I could put a const int in there to specify it. Basically no sensor should ever return invalid data, i have never seen it, however apparently some devices do so we shouldn't throw an exception and crash the app, instead we should check the lengths.

@Mrnikbobjeff
Copy link
Contributor

I can tell from personal experience when developing a game which was primarily motion sensor focused that some devices disregard the requested sensor type in low battery mode. This is not documented anywhere and probably a mistake in the vendor os. I guess that this is also the error occuring in the ticket which prompted this pr. In the case I recall working on we had a sensor returning a type_orientation sensor which has been deprecated. On another fringe device we had a ROTATION_VECTOR sensor on an API level > 18 and it was still missing the fourth value which is supposed to be present. This was also the reason I commented, while checking might work it would also expose us to errors, e.g. I believe you have to at least update the ROTATION_VECTOR code as it is supposed to have 5 values

(values[3], originally optional, will always be present from SDK Level 18 onwards. values[4] is a new value that has been added in SDK Level 18.)

This is why I asked for a seperate check with a map or similar :)

Copy link
Contributor

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the possibility of null needs to be considered.

@jamesmontemagno
Copy link
Collaborator Author

I did checks against the null checks and yes you are right :( I can modify this though to return 0 let me do that.

I think these tests are fine on all sensors. On orientation like you mentioned @Mrnikbobjeff it could return 3 values so we could check for those and return -1 for w if it doesn't exist.

@Redth Redth merged commit 9e74d1c into develop Mar 11, 2020
@Redth Redth deleted the gh-1148 branch March 11, 2020 20:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting-review This PR needs to have a set of eyes on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants