New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 718459 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Email to this user bounced
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Feature



Sign in to add a comment

Consider using union for SensorReading at //device/generic_sensor/public/cpp/sensor_reading.h

Project Member Reported by juncai@chromium.org, May 4 2017

Issue description

Based on the comment:
https://codereview.chromium.org/2847253002/diff/100001/device/generic_sensor/public/cpp/sensor_reading.h
The |values| is used by different sensors. So it would be more clear and self documenting about what kind of sensor data the |values| represents if using union.
 
There is some code sharing allowed by using a generic array in SensorReading. For example, Sensor.h provides a ReadingValue() function that can be called by IDL accessors:

double Sensor::ReadingValueUnchecked(int index) const {
  DCHECK(sensor_proxy_);
  DCHECK(index >= 0 && index < device::SensorReading::kValuesCount);
  return sensor_proxy_->Reading().values[index];
}

double Sensor::ReadingValue(int index, bool& is_null) const {
  is_null = !CanReturnReadings();
  return is_null ? 0.0 : ReadingValueUnchecked(index);
}

double Gyroscope::x(bool& is_null) const {
  return ReadingValue(0, is_null);
}

If we use a union this will become:

const device::Reading& Sensor::Reading() const {
  return sensor_proxy_->Reading();
}

double Gyroscope::x(bool& is_null) const {
  is_null = !CanReturnReadings();
  return sensor_proxy_->Reading().gyroscope.x;
}

I think this is reasonable.
Blocking: -612322
Components: Blink>Sensor
Labels: -DeviceService
This doesn't block content modularization.

Comment 3 by juncai@chromium.org, May 18 2017

Status: Started (was: Assigned)

Comment 4 by scheib@chromium.org, May 30 2017

Status: Available (was: Started)

Comment 5 by juncai@chromium.org, Jun 15 2017

Labels: Type-Feature
Owner: mikhail....@intel.com
Status: Started (was: Available)
Cc: juncai@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b0954dbdf504fba9d1e51ba449a716979422f91

commit 5b0954dbdf504fba9d1e51ba449a716979422f91
Author: Mikhail Pozdnyakov <mikhail.pozdnyakov@intel.com>
Date: Wed Aug 09 10:13:36 2017

[sensors] Use union type for sensor reading representation

1) It improves code readability and helps to avoid possible
mistakes with taking data at wrong indexes.
2) The newly added SensorReading.. classes will include rounding
strategies to provide the required precision level for a given
sensor type

Bug:  718459 
Change-Id: If30cb1840ff9dcce55212028206c70b67c27153a
Reviewed-on: https://chromium-review.googlesource.com/584768
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Jun Cai <juncai@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Mikhail Pozdnyakov <mikhail.pozdnyakov@intel.com>
Cr-Commit-Position: refs/heads/master@{#492934}
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/content/browser/generic_sensor_browsertest.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/absolute_orientation_euler_angles_fusion_algorithm_using_accelerometer_and_magnetometer.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/generic_sensor_service_unittest.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/linear_acceleration_fusion_algorithm_using_accelerometer.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/linux/sensor_data_linux.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/linux/sensor_data_linux.h
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/linux/sensor_device_manager.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/orientation_euler_angles_fusion_algorithm_using_quaternion.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/orientation_quaternion_fusion_algorithm_using_euler_angles.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_accelerometer_mac.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_ambient_light_mac.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_and_provider_unittest_win.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_android.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_fusion.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_fusion_algorithm.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_linux.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_reader_linux.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_reader_win.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/platform_sensor_reader_win.h
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/generic_sensor/relative_orientation_euler_angles_fusion_algorithm_using_accelerometer.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/public/cpp/generic_sensor/sensor_reading.cc
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/services/device/public/cpp/generic_sensor/sensor_reading.h
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/Accelerometer.cpp
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/AmbientLightSensor.cpp
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/Gyroscope.cpp
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/Magnetometer.cpp
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/OrientationSensor.cpp
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/Sensor.cpp
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/Sensor.h
[modify] https://crrev.com/5b0954dbdf504fba9d1e51ba449a716979422f91/third_party/WebKit/Source/modules/sensor/SensorProxy.cpp

Status: Verified (was: Started)

Sign in to add a comment