Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2492773002:
[sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms (Closed)
|
Created:
4 years, 1 month ago by maksims (do not use this acc) Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms
This cl add support for Accelerometer, Gyroscope and Magnetometer sensors.
Not all kernel version might be working correctly due to different axes
planes and axes' signs. The process in under test.
Manual testing was successful on Chromebook Pixel (samus board, kernel
n3.14 except magnetometer) and HP Revolve 810 G1 (xubuntu with kernel
v3.16 and n4.4).
BUG=
Committed: https://crrev.com/224a59760541664142c952928edfbbf15ef6c14e
Cr-Commit-Position: refs/heads/master@{#431873}
Patch Set 1 #Patch Set 2 : rebased #
Total comments: 12
Patch Set 3 : comments from Mikhail and Reilly #
Total comments: 7
Patch Set 4 : handle an error when a file with a scaling value cannot be read #
Total comments: 12
Patch Set 5 : add unittests comments #Patch Set 6 : use const char #
Total comments: 2
Patch Set 7 : rebased #
Messages
Total messages: 55 (41 generated)
The CQ bit was checked by maksim.sisov@intel.com 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== add motion sensors support BUG= ========== to ========== [sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms This cl add support for Accelerometer, Gyroscope and Magnetometer sensors. Not all kernel version might be working correctly due to different axes planes and axes' signs. The process in under test. Manual testing was successful on Chromebook Pixel (samus board, kernel n3.14 except magnetometer) and HP Revolve 810 G1 (xubuntu with kernel v3.16 and n4.4). BUG= ==========
The CQ bit was checked by maksim.sisov@intel.com 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.
maksim.sisov@intel.com changed reviewers: + alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com, reillyg@chromium.org
please take an initial look
https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(const char* scale_file_name, better to pass std::string https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:83: base::FilePath sensor_base_path = sensor_paths.back().DirName(); check that sensor_paths is not empty? https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... File device/generic_sensor/linux/sensor_data_linux.cc (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/sensor_data_linux.cc:44: data->sensor_file_names.push_back(std::move(file_names_y)); this is not appropriate, pls see: std::vector<std::string> vector; MaybeCheckKernelVersionAndAssignFileNames(vector..) // implicitly moved Now I cannot use vector any more. So, there are two options: 1) MaybeCheckKernelVersionAndAssignFileNames(const vector<>& vector) // no move inside client code: std::vector<std::string> vector; MaybeCheckKernelVersionAndAssignFileNames(vector); 2) MaybeCheckKernelVersionAndAssignFileNames(vector<> vector) // move inside client code: std::vector<std::string> vector; MaybeCheckKernelVersionAndAssignFileNames(std::move(vector));
https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/g... File device/generic_sensor/generic_sensor_consts.h (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/g... device/generic_sensor/generic_sensor_consts.h:14: constexpr double kRadiansInDegreesPerSecond = M_PI / 180.0; Include the header M_PI is defined in. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/g... device/generic_sensor/generic_sensor_consts.h:33: const char kChangedAxisKernelVersion[] = "3.18.0"; Constant string values should be declared in header files but have their value defined in a C++ file so that storage is only allocated in a single location. Since this value is only useful to the Linux sensor implementation it should probably just be defined in sensor_data_linux.cc. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:64: double scale_value = 1; There's no reason to declare a new variable here. The slightly different names are confusing.
The CQ bit was checked by maksim.sisov@intel.com 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/2492773002/diff/20001/device/generic_sensor/g... File device/generic_sensor/generic_sensor_consts.h (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/g... device/generic_sensor/generic_sensor_consts.h:14: constexpr double kRadiansInDegreesPerSecond = M_PI / 180.0; On 2016/11/10 16:53:17, Reilly Grant wrote: > Include the header M_PI is defined in. Done. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/g... device/generic_sensor/generic_sensor_consts.h:33: const char kChangedAxisKernelVersion[] = "3.18.0"; On 2016/11/10 16:53:17, Reilly Grant wrote: > Constant string values should be declared in header files but have their value > defined in a C++ file so that storage is only allocated in a single location. > Since this value is only useful to the Linux sensor implementation it should > probably just be defined in sensor_data_linux.cc. Oops. I've put this here by accident with other consts. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(const char* scale_file_name, On 2016/11/10 15:45:02, Mikhail wrote: > better to pass std::string Done. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:64: double scale_value = 1; On 2016/11/10 16:53:17, Reilly Grant wrote: > There's no reason to declare a new variable here. The slightly different names > are confusing. Done. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:83: base::FilePath sensor_base_path = sensor_paths.back().DirName(); On 2016/11/10 15:45:02, Mikhail wrote: > check that sensor_paths is not empty? Done. https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... File device/generic_sensor/linux/sensor_data_linux.cc (right): https://codereview.chromium.org/2492773002/diff/20001/device/generic_sensor/l... device/generic_sensor/linux/sensor_data_linux.cc:44: data->sensor_file_names.push_back(std::move(file_names_y)); On 2016/11/10 15:45:03, Mikhail wrote: > this is not appropriate, pls see: > > std::vector<std::string> vector; > MaybeCheckKernelVersionAndAssignFileNames(vector..) // implicitly moved > > Now I cannot use vector any more. > > So, there are two options: > 1) > MaybeCheckKernelVersionAndAssignFileNames(const vector<>& vector) // no move > inside > > client code: > std::vector<std::string> vector; > MaybeCheckKernelVersionAndAssignFileNames(vector); > 2) > > MaybeCheckKernelVersionAndAssignFileNames(vector<> vector) // move inside > > client code: > std::vector<std::string> vector; > MaybeCheckKernelVersionAndAssignFileNames(std::move(vector)); > > > Done.
https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(std::string scale_file_name, const std::string& https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:62: return scaling_value; This looks like a fatal error, we should handle it accordingly. https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:65: if (!base::StringToDouble(scale, &scaling_value)) Ditto. https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:84: GetSensorScalingValues(data.sensor_scale_name.c_str(), sensor_base_path); GetSensorScalingValues(data.sensor_scale_name, ..
The CQ bit was checked by maksim.sisov@intel.com 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/2492773002/diff/40001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(std::string scale_file_name, On 2016/11/11 09:07:41, Mikhail wrote: > const std::string& Done. https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:62: return scaling_value; On 2016/11/11 09:07:41, Mikhail wrote: > This looks like a fatal error, we should handle it accordingly. Done. https://codereview.chromium.org/2492773002/diff/40001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:65: if (!base::StringToDouble(scale, &scaling_value)) On 2016/11/11 09:07:41, Mikhail wrote: > Ditto. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(const std::string& scale_file_name, GetSensorScalingValue // it returns one value, right? https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(const std::string& scale_file_name, add a comment describing what it returns in what cases. https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:57: base::FilePath scale_path = sensor_base_path.Append(scale_file_name); maybe it would be better to pass 'scale_path' as a parameter? https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:58: if (base::PathExists(scale_path)) { would be simpler if: if (!base::PathExists(scale_path)) return 1; https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:61: scaling_value = -1; return -1; https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:67: scaling_value = -1; return -1;
The CQ bit was checked by maksim.sisov@intel.com 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...
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com 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 maksim.sisov@intel.com 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...
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_utils_linux.cc (right): https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(const std::string& scale_file_name, On 2016/11/11 10:49:11, Mikhail wrote: > GetSensorScalingValue // it returns one value, right? Done. https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:54: double GetSensorScalingValues(const std::string& scale_file_name, On 2016/11/11 10:49:11, Mikhail wrote: > add a comment describing what it returns in what cases. Done. https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:57: base::FilePath scale_path = sensor_base_path.Append(scale_file_name); On 2016/11/11 10:49:11, Mikhail wrote: > maybe it would be better to pass 'scale_path' as a parameter? Done. https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:58: if (base::PathExists(scale_path)) { On 2016/11/11 10:49:11, Mikhail wrote: > would be simpler if: > > if (!base::PathExists(scale_path)) > return 1; > Done. https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:61: scaling_value = -1; On 2016/11/11 10:49:11, Mikhail wrote: > return -1; Done. https://codereview.chromium.org/2492773002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_utils_linux.cc:67: scaling_value = -1; On 2016/11/11 10:49:11, Mikhail wrote: > return -1; Done.
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 maksim.sisov@intel.com 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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2492773002/diff/160001/device/generic_sensor/... File device/generic_sensor/generic_sensor_consts.h (right): https://codereview.chromium.org/2492773002/diff/160001/device/generic_sensor/... device/generic_sensor/generic_sensor_consts.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. I noticed that the Windows patch also adds some constants. Please make sure they all get added to this file.
The CQ bit was checked by maksim.sisov@intel.com 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 maksim.sisov@intel.com 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...
Patchset #8 (id:200001) has been deleted
The CQ bit was unchecked by maksim.sisov@intel.com
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from reillyg@chromium.org, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/2492773002/#ps180001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2492773002/diff/160001/device/generic_sensor/... File device/generic_sensor/generic_sensor_consts.h (right): https://codereview.chromium.org/2492773002/diff/160001/device/generic_sensor/... device/generic_sensor/generic_sensor_consts.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/11 16:26:53, Reilly Grant wrote: > I noticed that the Windows patch also adds some constants. Please make sure they > all get added to this file. shalamov@ will add other needed consts for windows here as well.
Message was sent while issue was closed.
Description was changed from ========== [sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms This cl add support for Accelerometer, Gyroscope and Magnetometer sensors. Not all kernel version might be working correctly due to different axes planes and axes' signs. The process in under test. Manual testing was successful on Chromebook Pixel (samus board, kernel n3.14 except magnetometer) and HP Revolve 810 G1 (xubuntu with kernel v3.16 and n4.4). BUG= ========== to ========== [sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms This cl add support for Accelerometer, Gyroscope and Magnetometer sensors. Not all kernel version might be working correctly due to different axes planes and axes' signs. The process in under test. Manual testing was successful on Chromebook Pixel (samus board, kernel n3.14 except magnetometer) and HP Revolve 810 G1 (xubuntu with kernel v3.16 and n4.4). BUG= ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms This cl add support for Accelerometer, Gyroscope and Magnetometer sensors. Not all kernel version might be working correctly due to different axes planes and axes' signs. The process in under test. Manual testing was successful on Chromebook Pixel (samus board, kernel n3.14 except magnetometer) and HP Revolve 810 G1 (xubuntu with kernel v3.16 and n4.4). BUG= ========== to ========== [sensors][CrOS/Linux] Implementation of motion sensors for CrOS/Linux platforms This cl add support for Accelerometer, Gyroscope and Magnetometer sensors. Not all kernel version might be working correctly due to different axes planes and axes' signs. The process in under test. Manual testing was successful on Chromebook Pixel (samus board, kernel n3.14 except magnetometer) and HP Revolve 810 G1 (xubuntu with kernel v3.16 and n4.4). BUG= Committed: https://crrev.com/224a59760541664142c952928edfbbf15ef6c14e Cr-Commit-Position: refs/heads/master@{#431873} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/224a59760541664142c952928edfbbf15ef6c14e Cr-Commit-Position: refs/heads/master@{#431873} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
