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

Issue 2533793002: [sensors](CrOS/Linux) Implement Sensor device manager for sensors (Closed)

Created:
4 years ago by maksims (do not use this acc)
Modified:
3 years, 10 months ago
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[sensors](CrOS/Linux) Implement Sensor device manager for sensors This cl introduces a SensorDeviceManager for Linux and ChromeOS platforms. Manager: 1) PlatformSensorProviderLinux owns a SensorDeviceManager that uses DeviceMonitorLinux to enumerate iio devices and get removed/add udev events. Once device is enumerated, added or removed, it is identified and all the necessary information is put to SensorInfoLinux structure and passed to the provider. If there is no such a device, it is added to cache. In case of removal, manager checks if there was such a device and updates it's cache accordingly + notifies the provider about a removed device, which update own cache. 2) Requests: when a request for a sensor comes, it can be processed immediately or asynchronously once service is up and running. Reader: 1) There will be two strategies for a reader. At the moment only polling read is supported. 2) The reader doesn't search for sensor files any more, those are passed to it in SensorInfoLinux structure. Unittests: 1) In order to make it possible to test the whole path, I've made SensorDeviceManager's methods virtual, which allowed me to mock and override them. 2) Unittests implement own udev methods that just read values from files. If read is successful, the manager adds this type of sensor associated with the files that have been read to it's cache and notifies the provider. BUG=606766 Committed: https://crrev.com/061d113db7069c23fe84a0a9365965647ed97b51 Cr-Commit-Position: refs/heads/master@{#437550}

Patch Set 1 #

Patch Set 2 : change style errors #

Patch Set 3 : fix compilation erros #

Patch Set 4 : fix build.gn #

Total comments: 21

Patch Set 5 : Merge SensorDeviceManager with Platform....Provider. Rename SensorDeviceService to SensorDeviceMana… #

Total comments: 33

Patch Set 6 : comments #

Total comments: 18

Patch Set 7 : nits #

Total comments: 6

Patch Set 8 : nits #

Patch Set 9 : construct manager #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+1568 lines, -767 lines) Patch
M device/BUILD.gn View 1 2 3 4 5 1 chunk +3 lines, -2 lines 0 comments Download
M device/generic_sensor/BUILD.gn View 1 2 3 4 5 6 3 chunks +16 lines, -7 lines 0 comments Download
A device/generic_sensor/linux/platform_sensor_manager.h View 1 2 3 4 5 6 1 chunk +83 lines, -0 lines 1 comment Download
A device/generic_sensor/linux/platform_sensor_manager.cc View 1 2 3 4 5 6 1 chunk +173 lines, -0 lines 0 comments Download
D device/generic_sensor/linux/platform_sensor_utils_linux.h View 1 chunk +0 lines, -52 lines 0 comments Download
D device/generic_sensor/linux/platform_sensor_utils_linux.cc View 1 chunk +0 lines, -131 lines 0 comments Download
M device/generic_sensor/linux/sensor_data_linux.h View 1 2 3 4 5 2 chunks +52 lines, -15 lines 0 comments Download
M device/generic_sensor/linux/sensor_data_linux.cc View 1 2 3 4 7 chunks +62 lines, -36 lines 0 comments Download
D device/generic_sensor/linux/sensor_reader_unittest.cc View 1 chunk +0 lines, -372 lines 0 comments Download
A device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc View 1 2 3 4 5 6 1 chunk +661 lines, -0 lines 0 comments Download
M device/generic_sensor/platform_sensor_linux.h View 1 2 3 4 5 4 chunks +8 lines, -19 lines 0 comments Download
M device/generic_sensor/platform_sensor_linux.cc View 1 2 3 4 5 6 3 chunks +36 lines, -62 lines 0 comments Download
M device/generic_sensor/platform_sensor_provider.cc View 1 2 3 4 5 2 chunks +4 lines, -2 lines 0 comments Download
M device/generic_sensor/platform_sensor_provider_base.h View 1 2 3 4 5 6 1 chunk +8 lines, -2 lines 0 comments Download
M device/generic_sensor/platform_sensor_provider_base.cc View 1 2 3 4 5 6 3 chunks +17 lines, -4 lines 0 comments Download
M device/generic_sensor/platform_sensor_provider_linux.h View 1 2 3 4 5 6 7 2 chunks +72 lines, -27 lines 0 comments Download
M device/generic_sensor/platform_sensor_provider_linux.cc View 1 2 3 4 5 6 7 8 3 chunks +144 lines, -36 lines 1 comment Download
A device/generic_sensor/platform_sensor_reader_linux.h View 1 2 3 4 5 6 1 chunk +66 lines, -0 lines 0 comments Download
A device/generic_sensor/platform_sensor_reader_linux.cc View 1 2 3 4 5 6 1 chunk +163 lines, -0 lines 0 comments Download

Messages

Total messages: 64 (45 generated)
maksims_
Please review
4 years ago (2016-11-28 21:02:14 UTC) #17
Reilly Grant (use Gerrit)
I'm confused about why SensorDeviceManager, SensorDeviceService and PlatformSensorProviderLinux are all separate objects. If you need ...
4 years ago (2016-11-28 21:21:49 UTC) #18
maksims (do not use this acc)
On 2016/11/28 21:21:49, Reilly Grant wrote: > I'm confused about why SensorDeviceManager, SensorDeviceService and > ...
4 years ago (2016-11-29 09:54:50 UTC) #21
maksims (do not use this acc)
On 2016/11/29 09:54:50, maksims wrote: > On 2016/11/28 21:21:49, Reilly Grant wrote: > > I'm ...
4 years ago (2016-11-29 09:55:26 UTC) #22
Reilly Grant (use Gerrit)
Thanks for the explanation. I've had some more time to look through this patch and ...
4 years ago (2016-11-29 20:19:56 UTC) #23
Mikhail
https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/platform_sensor_manager_linux.cc File device/generic_sensor/platform_sensor_manager_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/platform_sensor_manager_linux.cc#newcode44 device/generic_sensor/platform_sensor_manager_linux.cc:44: : observer_(this), manager_(nullptr), task_runner_(task_runner) { nit: move(task_runner) https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/platform_sensor_manager_linux.cc#newcode68 device/generic_sensor/platform_sensor_manager_linux.cc:68: ...
4 years ago (2016-11-30 12:29:36 UTC) #24
maksims (do not use this acc)
Please review https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/linux/platform_sensor_reader_linux.cc File device/generic_sensor/linux/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/linux/platform_sensor_reader_linux.cc#newcode1 device/generic_sensor/linux/platform_sensor_reader_linux.cc:1: // Copyright 2016 The Chromium Authors. All ...
4 years ago (2016-12-05 13:07:00 UTC) #29
Reilly Grant (use Gerrit)
https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/BUILD.gn File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/BUILD.gn#newcode80 device/generic_sensor/BUILD.gn:80: "//device/udev_linux", You might need to include the *_linux.* sources ...
4 years ago (2016-12-08 02:31:15 UTC) #33
Mikhail
https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/platform_sensor_provider_base.cc File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/platform_sensor_provider_base.cc#newcode120 device/generic_sensor/platform_sensor_provider_base.cc:120: std::vector<mojom::SensorType> PlatformSensorProviderBase::GetRequestTypes() { GetPendingRequestedTypes https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/platform_sensor_provider_base.cc#newcode130 device/generic_sensor/platform_sensor_provider_base.cc:130: mojo::ScopedSharedBufferMapping mapping = ...
4 years ago (2016-12-08 11:15:22 UTC) #34
maksims (do not use this acc)
https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/BUILD.gn File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/BUILD.gn#newcode80 device/generic_sensor/BUILD.gn:80: "//device/udev_linux", On 2016/12/08 02:31:14, Reilly Grant wrote: > You ...
4 years ago (2016-12-08 18:39:18 UTC) #37
Reilly Grant (use Gerrit)
lgtm https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/platform_sensor_reader_linux.cc File device/generic_sensor/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/platform_sensor_reader_linux.cc#newcode62 device/generic_sensor/platform_sensor_reader_linux.cc:62: sensor_file_paths_(std::move(sensor_device->device_reading_files)), nit: Since sensor_device is const this move ...
4 years ago (2016-12-08 23:19:52 UTC) #40
Mikhail
looks good overall https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/platform_sensor_manager_linux.h File device/generic_sensor/platform_sensor_manager_linux.h (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/platform_sensor_manager_linux.h#newcode6 device/generic_sensor/platform_sensor_manager_linux.h:6: #define DEVICE_GENERIC_SENSOR_PLATFORM_SENSOR_MANAGER_LINUX_H_ could we put it ...
4 years ago (2016-12-09 08:15:57 UTC) #42
maksims (do not use this acc)
https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/platform_sensor_manager_linux.h File device/generic_sensor/platform_sensor_manager_linux.h (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/platform_sensor_manager_linux.h#newcode6 device/generic_sensor/platform_sensor_manager_linux.h:6: #define DEVICE_GENERIC_SENSOR_PLATFORM_SENSOR_MANAGER_LINUX_H_ On 2016/12/09 08:15:56, Mikhail wrote: > could ...
4 years ago (2016-12-09 10:22:42 UTC) #44
Mikhail
lgtm https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/platform_sensor_provider_linux.cc File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/platform_sensor_provider_linux.cc#newcode41 device/generic_sensor/platform_sensor_provider_linux.cc:41: if (!sensor_nodes_enumerated_) { if (!sensor_nodes_enumeration_started_) { sensor_nodes_enumeration_started_ =... ...
4 years ago (2016-12-09 11:17:29 UTC) #46
maksims (do not use this acc)
https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/platform_sensor_provider_linux.cc File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/platform_sensor_provider_linux.cc#newcode41 device/generic_sensor/platform_sensor_provider_linux.cc:41: On 2016/12/09 11:17:29, Mikhail wrote: > if (!sensor_nodes_enumerated_) { ...
4 years ago (2016-12-09 13:03:07 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2533793002/180001
4 years ago (2016-12-09 15:35:57 UTC) #58
commit-bot: I haz the power
Committed patchset #9 (id:180001)
4 years ago (2016-12-09 16:05:30 UTC) #61
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/061d113db7069c23fe84a0a9365965647ed97b51 Cr-Commit-Position: refs/heads/master@{#437550}
4 years ago (2016-12-09 16:08:38 UTC) #63
Reilly Grant (use Gerrit)
4 years ago (2016-12-09 18:54:19 UTC) #64
Message was sent while issue was closed.
Two changes for a followup patch.

https://codereview.chromium.org/2533793002/diff/180001/device/generic_sensor/...
File device/generic_sensor/linux/platform_sensor_manager.h (right):

https://codereview.chromium.org/2533793002/diff/180001/device/generic_sensor/...
device/generic_sensor/linux/platform_sensor_manager.h:6: #define
DEVICE_GENERIC_SENSOR_LINUX_PLATFORM_SENSOR_MANAGER_H_
Please rename this file to sensor_device_manager.h as it does not contain a
class called PlatformSensorManager.

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

https://codereview.chromium.org/2533793002/diff/180001/device/generic_sensor/...
device/generic_sensor/platform_sensor_provider_linux.cc:205: if (it ==
sensor_devices_by_type_.end() &&
it != sensor_devices_by_type_.end()

Powered by Google App Engine
This is Rietveld 408576698