|
|
DescriptionAdd 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
Messages
Total messages: 56 (42 generated)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
juncai@chromium.org changed reviewers: + timvolodine@chromium.org
Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
As discussed offline, instead of basing this directly on the sudden motion sensor and thus making it specific to macOS this code can be based on any implementation of the ACCELEROMETER PlatformSensor type and thus be reused anywhere where we need to provide a RELATIVE_ORIENTATION sensor but only have an accelerometer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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 CL uses similar logic to add RELATIVE_ORIENTATION sensor implementation on macOS to //device/generic_sensor. BUG=721427 ========== to ========== 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 CL uses similar logic to add RELATIVE_ORIENTATION sensor implementation on macOS to //device/generic_sensor. BUG=721427, 727788 ==========
Description was changed from ========== 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 CL uses similar logic to add RELATIVE_ORIENTATION sensor implementation on macOS to //device/generic_sensor. BUG=721427, 727788 ========== to ========== 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 since this method can be generalized to implement RELATIVE_ORIENTATION using ACCELEROMETER. This CL implements RELATIVE_ORIENTATION sensor by using ACCELEROMETER and add it to macOS to //device/generic_sensor. BUG=721427, 727788 ==========
Description was changed from ========== 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 since this method can be generalized to implement RELATIVE_ORIENTATION using ACCELEROMETER. This CL implements RELATIVE_ORIENTATION sensor by using ACCELEROMETER and add it to macOS to //device/generic_sensor. BUG=721427, 727788 ========== to ========== 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=721427, 727788 ==========
Description was changed from ========== 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=721427, 727788 ========== to ========== 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=721427, 727788 ==========
On 2017/06/09 22:36:44, Reilly Grant (use Gerrit) wrote: > As discussed offline, instead of basing this directly on the sudden motion > sensor and thus making it specific to macOS this code can be based on any > implementation of the ACCELEROMETER PlatformSensor type and thus be reused > anywhere where we need to provide a RELATIVE_ORIENTATION sensor but only have an > accelerometer. Thanks! Done. Please take a look again.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping timvolodine@, :).
https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc (right): https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:74: return mojom::ReportingMode::ON_CHANGE; This should return accelerometer_->GetReportingMode() which means that you need to delay calling the CreateSensorCallback passed to CreateSensor until CreateAccelerometerCallback is called. https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:94: timer_.Start( |accelerometer_| already has a timer and can notify this object through its client interface when the value changes so we don't need another timer here.
alexander.shalamov@intel.com changed reviewers: + alexander.shalamov@intel.com
https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc (right): https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:59: : PlatformSensor(mojom::SensorType::RELATIVE_ORIENTATION, Would it be better to create 'fusion' (PlatformSensorFusionRelativeOrientation) sensor that will. 1. Get required sensors from platform sensor provider. 2. Based on available senors, create appropriate fusion algorithm instance. 3. Observe sensors for data changes using PlatformSensor::Client 4. Feed data to algorithm and provide fused data to fusion sensor clients. Relative orientation data can be created (fused) from Accel or Accel + Gyro combination. https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:121: void PlatformSensorRelativeOrientationUsingAccelerometer::PollForData() { Wouldn't it be better to move all algorithms outside of actual sensor? Then we will have set of different fusion / filtering algorithms that can be used whenever needed. We can use 'Magwick' algorithms for relative orientation (gyro + accel) and absolute (magnetometer + accel + gyro) orientation. http://x-io.co.uk/res/doc/madgwick_internal_report.pdf https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:149: double beta = std::atan2(-reading.values[1], reading.values[2]); Do we need to filter (low-pass) accelerometer readings for smoother roll / pitch estimation? Should this algorithm check when Y is aligned with gravity vector?
Also, is there a design doc for this? think would be good to add a pointer to such a doc for general context..
On 2017/06/14 18:26:26, timvolodine wrote: > Also, is there a design doc for this? think would be good to add a pointer to > such a doc for general context.. Yes, there is a design doc for this CL: https://docs.google.com/document/d/1J6_frvCxmhf_oZIjKngFQmerYnsZ-ZRP3HL6Shm4g... Section "(2) Fallbacks using different sensors" is related to this CL. I already added the above design doc to the issue: https://bugs.chromium.org/p/chromium/issues/detail?id=721427#c6
Description was changed from ========== 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=721427, 727788 ========== to ========== 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, 727788 ==========
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc (right): https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... 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: > > Would it be better to create 'fusion' (PlatformSensorFusionRelativeOrientation) > sensor that will. > > 1. Get required sensors from platform sensor provider. > 2. Based on available senors, create appropriate fusion algorithm instance. > 3. Observe sensors for data changes using PlatformSensor::Client > 4. Feed data to algorithm and provide fused data to fusion sensor clients. > > Relative orientation data can be created (fused) from Accel or Accel + Gyro > combination. Thanks. For this CL, the relative orientation fusion sensor is implemented by using Accel. Accel + Gyro relative orientation fusion sensor will be implemented in future follow-up CLs. Done. https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:74: return mojom::ReportingMode::ON_CHANGE; On 2017/06/14 00:59:56, Reilly Grant (use Gerrit) wrote: > This should return accelerometer_->GetReportingMode() which means that you need > to delay calling the CreateSensorCallback passed to CreateSensor until > CreateAccelerometerCallback is called. Done. https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:94: timer_.Start( On 2017/06/14 00:59:56, Reilly Grant (use Gerrit) wrote: > |accelerometer_| already has a timer and can notify this object through its > client interface when the value changes so we don't need another timer here. Done. https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:121: void PlatformSensorRelativeOrientationUsingAccelerometer::PollForData() { On 2017/06/14 07:22:45, shalamov wrote: > > Wouldn't it be better to move all algorithms outside of actual sensor? Then we > will have set of different fusion / filtering algorithms that can be used > whenever needed. > > We can use 'Magwick' algorithms for relative orientation (gyro + accel) and > absolute (magnetometer + accel + gyro) orientation. > http://x-io.co.uk/res/doc/madgwick_internal_report.pdf Done. https://codereview.chromium.org/2929603003/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_relative_orientation_using_accelerometer.cc:149: double beta = std::atan2(-reading.values[1], reading.values[2]); On 2017/06/14 07:22:44, shalamov wrote: > > Do we need to filter (low-pass) accelerometer readings for smoother roll / pitch > estimation? Should this algorithm check when Y is aligned with gravity vector? The main purpose of this CL is to add relative orientation fusion sensor (using Accel) for Mac, which makes //device/generic_sensor have the same platform-specific implementation details as //device/sensors for device orientation event. So for this CL, the implementation is the same as: https://cs.chromium.org/chromium/src/device/sensors/data_fetcher_shared_memor... In future follow-up CLs, other fusion algorithm such as Accel + Gyro relative orientation will be implemented.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mikhail.pozdnyakov@intel.com changed reviewers: + mikhail.pozdnyakov@intel.com
Thanks for implementing this! 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; where did this value come from? https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:46: provider->CreateSensor( think it's worth first look for existing sensors using 'provider->GetSensor()' https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:66: return PlatformSensorConfiguration(kDefaultAccelerometerFrequencyHz); why not use accelerometer_->GetDefaultConfiguration() ? https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:74: accelerometer_reader_ = base::MakeUnique<SensorSharedBufferReader>( instead of introducing SensorSharedBufferReader I'd add "bool PlatfromSensor::GetLatestReading(Reading*)" API, thus we do not have to expose shared_buffer_mapping() https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:113: bool PlatformSensorFusionRelativeOrientation::IsNotificationSuspended() { think here the suspend status of its own clients should be checked, otherwise the contained "accelerometer_" can never be stopped when we've gone to suspended state https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:119: return configuration.frequency() > 0 && why not forward this call to the contained "accelerometer_" ? https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... File device/generic_sensor/public/cpp/sensor_shared_buffer_reader.h (right): https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/public/cpp/sensor_shared_buffer_reader.h:16: class DEVICE_GENERIC_SENSOR_PUBLIC_EXPORT SensorSharedBufferReader { It's a good idea to export this functionality since it's used in Blink as well, but IMO it would be better just to add a free function "bool GetSensorReadingFromBuffer(const SensorReadingSharedBuffer* buffer, SensorReading* result);" and call it both from Blink and from PlatformSensor::GetLatestReading() which I proposed above. https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... File device/generic_sensor/relative_orientation_fusion_algorithm.h (right): https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/relative_orientation_fusion_algorithm.h:15: class RelativeOrientationFusionAlgorithm { cool, strategy class :)
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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/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! https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:46: provider->CreateSensor( On 2017/06/20 13:50:00, Mikhail wrote: > think it's worth first look for existing sensors using 'provider->GetSensor()' Done. https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:66: return PlatformSensorConfiguration(kDefaultAccelerometerFrequencyHz); On 2017/06/20 13:50:00, Mikhail wrote: > why not use accelerometer_->GetDefaultConfiguration() ? Done. https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:74: accelerometer_reader_ = base::MakeUnique<SensorSharedBufferReader>( On 2017/06/20 13:50:00, Mikhail wrote: > instead of introducing SensorSharedBufferReader I'd add "bool > PlatfromSensor::GetLatestReading(Reading*)" API, thus we do not have to expose > shared_buffer_mapping() Done. https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:113: bool PlatformSensorFusionRelativeOrientation::IsNotificationSuspended() { On 2017/06/20 13:50:00, Mikhail wrote: > think here the suspend status of its own clients should be checked, otherwise > the contained "accelerometer_" can never be stopped when we've gone to suspended > state Done. https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_fusion_relative_orientation.cc:119: return configuration.frequency() > 0 && On 2017/06/20 13:50:00, Mikhail wrote: > why not forward this call to the contained "accelerometer_" ? Done. https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... File device/generic_sensor/public/cpp/sensor_shared_buffer_reader.h (right): https://codereview.chromium.org/2929603003/diff/140001/device/generic_sensor/... device/generic_sensor/public/cpp/sensor_shared_buffer_reader.h:16: class DEVICE_GENERIC_SENSOR_PUBLIC_EXPORT SensorSharedBufferReader { On 2017/06/20 13:50:00, Mikhail wrote: > It's a good idea to export this functionality since it's used in Blink as well, > but IMO it would be better just to add a free function "bool > GetSensorReadingFromBuffer(const SensorReadingSharedBuffer* buffer, > SensorReading* result);" and call it both from Blink and from > PlatformSensor::GetLatestReading() which I proposed above. Done.
The CQ bit was checked by juncai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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, 727788 ========== to ========== 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 ==========
Couple of comments, looks good otherwise. 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); let's make it virtual, it seems the newly added PlatformSensorFusionRelativeOrientation should override it. 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() {} acceleration_x_ and other members are left uninitialized, better define ctor as "= default"
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/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). 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 { looks like this is a merge from a different patch https://chromium-review.googlesource.com/c/541685/ ? (also below and corresp .h file) 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, think would be good to document the units of acceleration https://codereview.chromium.org/2929603003/diff/180001/device/generic_sensor/... device/generic_sensor/relative_orientation_fusion_algorithm.h:30: double angular_speed_z) { this is currently not used? any plans to provide 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); strictly speaking this is actually not real sensor fusion, just the use of accelerometer.. ;) 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); looking ahead e.g. when providing RELATIVE_ORIENTATION_ANGLE it would make sense to avoid computing the quaternion representation unless explicitly needed..
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. |