Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(414)

Issue 2929603003: Add RELATIVE_ORIENTATION sensor implementation on macOS to //device/generic_sensor (Closed)

Created:
3 years, 6 months ago by juncai
Modified:
3 years, 5 months ago
CC:
chromium-reviews, wanming.lin, mac-reviews_chromium.org, shalamov, Mikhail, Reilly Grant (use Gerrit)
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add RELATIVE_ORIENTATION sensor implementation on macOS to //device/generic_sensor At //device/sensors, device relative orientation event is implemented on macOS using sudden motion sensor, and this method can be generalized to implement RELATIVE_ORIENTATION using ACCELEROMETER and be reused anywhere where we need to provide a RELATIVE_ORIENTATION sensor but only has an ACCELEROMETER. This CL implements RELATIVE_ORIENTATION PlatformSensor type by using ACCELEROMETER and add it to macOS to //device/generic_sensor. BUG=733707

Patch Set 1 : add RELATIVE_ORIENTATION sensor implementation on macOS to //device/generic_sensor #

Patch Set 2 : updated readme file #

Patch Set 3 : updated readme file #

Patch Set 4 : address comments #

Patch Set 5 : address comments #

Patch Set 6 : updated readme file #

Total comments: 10

Patch Set 7 : address comments #

Patch Set 8 : updated OnSensorError() #

Total comments: 17

Patch Set 9 : address more comments #

Patch Set 10 : updated IsSignificantlyDifferent() #

Total comments: 14
Unified diffs Side-by-side diffs Delta from patch set Stats (+442 lines, -8 lines) Patch
M device/generic_sensor/BUILD.gn View 1 2 3 4 5 6 2 chunks +6 lines, -0 lines 0 comments Download
M device/generic_sensor/README.md View 1 2 3 4 5 2 chunks +3 lines, -2 lines 0 comments Download
M device/generic_sensor/platform_sensor.h View 1 2 3 4 5 6 7 8 4 chunks +8 lines, -6 lines 2 comments Download
M device/generic_sensor/platform_sensor.cc View 1 2 3 4 5 6 7 8 1 chunk +7 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_fusion_relative_orientation.h View 1 2 3 4 5 6 7 8 1 chunk +64 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_fusion_relative_orientation.cc View 1 2 3 4 5 6 7 8 9 1 chunk +141 lines, -0 lines 0 comments Download
M device/generic_sensor/platform_sensor_provider_mac.cc View 1 2 3 4 5 6 2 chunks +6 lines, -0 lines 0 comments Download
M device/generic_sensor/public/cpp/sensor_reading.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M device/generic_sensor/public/cpp/sensor_reading.cc View 1 2 3 4 5 6 7 8 2 chunks +30 lines, -0 lines 2 comments Download
A device/generic_sensor/relative_orientation_fusion_algorithm.h View 1 2 3 4 5 6 1 chunk +55 lines, -0 lines 4 comments Download
A device/generic_sensor/relative_orientation_fusion_algorithm.cc View 1 2 3 4 5 6 1 chunk +13 lines, -0 lines 2 comments Download
A device/generic_sensor/relative_orientation_fusion_algorithm_using_accelerometer.h View 1 2 3 4 5 6 1 chunk +31 lines, -0 lines 0 comments Download
A device/generic_sensor/relative_orientation_fusion_algorithm_using_accelerometer.cc View 1 2 3 4 5 6 1 chunk +74 lines, -0 lines 4 comments Download

Messages

Total messages: 56 (42 generated)
juncai
Please take a look.
3 years, 6 months ago (2017-06-08 00:50:24 UTC) #8
Reilly Grant (use Gerrit)
As discussed offline, instead of basing this directly on the sudden motion sensor and thus ...
3 years, 6 months ago (2017-06-09 22:36:44 UTC) #14
juncai
On 2017/06/09 22:36:44, Reilly Grant (use Gerrit) wrote: > As discussed offline, instead of basing ...
3 years, 6 months ago (2017-06-13 01:09:20 UTC) #27
juncai
ping timvolodine@, :).
3 years, 6 months ago (2017-06-13 23:46:53 UTC) #30
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc File device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc (right): https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc#newcode74 device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:74: return mojom::ReportingMode::ON_CHANGE; This should return accelerometer_->GetReportingMode() which means that ...
3 years, 6 months ago (2017-06-14 00:59:56 UTC) #31
shalamov
https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc File device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc (right): https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc#newcode59 device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:59: : PlatformSensor(mojom::SensorType::RELATIVE_ORIENTATION, Would it be better to create 'fusion' ...
3 years, 6 months ago (2017-06-14 07:22:46 UTC) #33
timvolodine
Also, is there a design doc for this? think would be good to add a ...
3 years, 6 months ago (2017-06-14 18:26:26 UTC) #34
juncai
On 2017/06/14 18:26:26, timvolodine wrote: > Also, is there a design doc for this? think ...
3 years, 6 months ago (2017-06-15 01:41:17 UTC) #35
juncai
https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc File device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc (right): https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc#newcode59 device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:59: : PlatformSensor(mojom::SensorType::RELATIVE_ORIENTATION, On 2017/06/14 07:22:45, shalamov wrote: > > ...
3 years, 6 months ago (2017-06-19 23:23:25 UTC) #39
Mikhail
Thanks for implementing this! https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/platform_sensor_fusion_relative_orientation.cc File device/generic_sensor/platform_sensor_fusion_relative_orientation.cc (right): https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/platform_sensor_fusion_relative_orientation.cc#newcode21 device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:21: constexpr double kQuaternionThreshold = 0.01; ...
3 years, 6 months ago (2017-06-20 13:50:00 UTC) #45
juncai
https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/platform_sensor_fusion_relative_orientation.cc File device/generic_sensor/platform_sensor_fusion_relative_orientation.cc (right): https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/platform_sensor_fusion_relative_orientation.cc#newcode21 device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:21: constexpr double kQuaternionThreshold = 0.01; On 2017/06/20 13:50:00, Mikhail ...
3 years, 6 months ago (2017-06-20 19:50:56 UTC) #48
Mikhail
Couple of comments, looks good otherwise. https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/platform_sensor.h File device/generic_sensor/platform_sensor.h (right): https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/platform_sensor.h#newcode72 device/generic_sensor/platform_sensor.h:72: bool GetLatestReading(SensorReading* result); ...
3 years, 6 months ago (2017-06-21 11:10:00 UTC) #54
timvolodine
https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/platform_sensor_fusion_relative_orientation.cc File device/generic_sensor/platform_sensor_fusion_relative_orientation.cc (right): https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/platform_sensor_fusion_relative_orientation.cc#newcode21 device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:21: constexpr double kQuaternionThreshold = 0.01; On 2017/06/20 19:50:55, juncai ...
3 years, 6 months ago (2017-06-21 18:22:12 UTC) #55
juncai
3 years, 5 months ago (2017-07-20 00:22:16 UTC) #56
This CL is closed in favor of a new CL:
https://chromium-review.googlesource.com/c/578253/
The new CL adds a general fusion sensor and fusion sensor algorithm which can
provide more flexibility to implement different fusion sensor types.

https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/...
File device/generic_sensor/platform_sensor_fusion_relative_orientation.cc
(right):

https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/...
device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:21:
constexpr double kQuaternionThreshold = 0.01;
On 2017/06/21 18:22:11, timvolodine wrote:
> On 2017/06/20 19:50:55, juncai wrote:
> > On 2017/06/20 13:50:00, Mikhail wrote:
> > > where did this value come from?
> > 
> > The quaternion is a unit quaternion so I thought 0.01 is a reasonable
> threshold
> > for detecting any changes.
> > 
> > Any suggestion of better value for this parameter? Thanks!
> 
> There should be some rational explanation for this threshold. e.g. for Euler
> angles we had a threshold of 0.1 degree (to filter out potential noise).

In the new CL:
https://chromium-review.googlesource.com/c/578253/
This threshold can be adjusted and documented for different sensor fusion
algorithms.

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
File device/generic_sensor/platform_sensor.h (right):

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/platform_sensor.h:72: bool GetLatestReading(SensorReading*
result);
On 2017/06/21 11:10:00, Mikhail wrote:
> let's make it virtual, it seems the newly added
> PlatformSensorFusionRelativeOrientation should override it.

Not sure why makes it virtual. It should be the same for all subclasses of
PlatformSensor.

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
File device/generic_sensor/public/cpp/sensor_reading.cc (right):

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/public/cpp/sensor_reading.cc:9: namespace {
On 2017/06/21 18:22:11, timvolodine wrote:
> looks like this is a merge from a different patch
> https://chromium-review.googlesource.com/c/541685/ ?
> (also below and corresp .h file)

This buffer reading functionality is moved to SensorReadingSharedBufferReader.

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
File device/generic_sensor/relative_orientation_fusion_algorithm.cc (right):

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/relative_orientation_fusion_algorithm.cc:9:
RelativeOrientationFusionAlgorithm::RelativeOrientationFusionAlgorithm() {}
On 2017/06/21 11:10:00, Mikhail wrote:
> acceleration_x_ and other members are left uninitialized,
> better define ctor as "= default"

These member variables don't exist in the new CL:
https://chromium-review.googlesource.com/c/578253/

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
File device/generic_sensor/relative_orientation_fusion_algorithm.h (right):

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/relative_orientation_fusion_algorithm.h:20: void
set_acceleration(double acceleration_x,
On 2017/06/21 18:22:11, timvolodine wrote:
> think would be good to document the units of acceleration

I will update the sensor_reading.h to add units for ACCELEROMETER,
LINEAR_ACCELERATION and GYROSCOPE in a separate CL.

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/relative_orientation_fusion_algorithm.h:30: double
angular_speed_z) {
On 2017/06/21 18:22:11, timvolodine wrote:
> this is currently not used? any plans to provide fusion based on both
> acceleration and gyroscope

A new CL:
https://chromium-review.googlesource.com/c/578253/
adds a general fusion sensor and any combination of fusion algorithm can be
used.
Follow-up CL will implement fusion based on both acceleration and gyroscope.

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
File
device/generic_sensor/relative_orientation_fusion_algorithm_using_accelerometer.cc
(right):

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/relative_orientation_fusion_algorithm_using_accelerometer.cc:69:
double gamma = std::asin(acceleration_x_ / kMeanGravity);
On 2017/06/21 18:22:12, timvolodine wrote:
> strictly speaking this is actually not real sensor fusion, just the use of
> accelerometer.. ;)

Yes, the sensor fusion concept is general here.

https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/...
device/generic_sensor/relative_orientation_fusion_algorithm_using_accelerometer.cc:71:
ComputeQuaternionFromEulerAngles(alpha, beta, gamma, x, y, z, w);
On 2017/06/21 18:22:12, timvolodine wrote:
> looking ahead e.g. when providing RELATIVE_ORIENTATION_ANGLE it would make
sense
> to avoid computing the quaternion representation unless explicitly needed..

In the new CL:
https://chromium-review.googlesource.com/c/578253/
This conversion is avoided.

Powered by Google App Engine
This is Rietveld 408576698