|
|
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
Messages
Total messages: 64 (45 generated)
Description was changed from ========== [sensors](CrOS/Linux) Implement Sensor device manager for sensors BUG= ========== to ========== [sensors](CrOS/Linux) Implement Sensor device manager for sensors This cl introduces a SensorDeviceManager for Linux and ChromeOS platforms. Manager: 1) SensorDeviceManager own a SensorDeviceService 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 SensorDeviceLinux structure and passed to the manager. If there is no such a device, it is added. In case of removal, manager checks if there was such a device and updates it's cache accordingly. 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) It doesn't search for files anymore, a platform sensor owns and creates it. Unittests: 1) In order to make it possible to test the whole path, I've made sensor device service methods to be virtual that allowed me to mock those methods and override service with a test one. 2) Unittests implement own udev methods that just read values from files. If read was successful, service thinks sensor exists and provider gets all the necessary information to create a platform sensor. BUG=Lazy to search now. ==========
maksim.sisov@intel.com changed reviewers: + alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com, reillyg@chromium.org
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...
Please review
I'm confused about why SensorDeviceManager, SensorDeviceService and PlatformSensorProviderLinux are all separate objects. If you need separate objects for different threads then I would follow the pattern I used in HidServiceLinux and HidServiceLinux::FileThreadHelper.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
On 2016/11/28 21:21:49, Reilly Grant wrote: > I'm confused about why SensorDeviceManager, SensorDeviceService and > PlatformSensorProviderLinux are all separate objects. If you need separate > objects for different threads then I would follow the pattern I used in > HidServiceLinux and HidServiceLinux::FileThreadHelper. I don't want to unite PlatformSensorProviderLinux with SensorDeviceManager and SensorDeviceService, because PlatformSensorProviderLinux belongs to a generic sensor framework. The last two are missing parts of CrOS/Linux platforms and implemented more like a helper for a provider (win, android, mac - all they have some sort of managers). I agree that naming might not be appropriate in this context and I need to rename those classes. Why I have a separate instance called SensorDeviceService is because I want the code to be testable. I have virtual methods in SensorDeviceService, which I can override in my mock object. What is more, I can easily make my mock object to be used by the SensorDeviceManager when tests are running.
On 2016/11/29 09:54:50, maksims wrote: > On 2016/11/28 21:21:49, Reilly Grant wrote: > > I'm confused about why SensorDeviceManager, SensorDeviceService and > > PlatformSensorProviderLinux are all separate objects. If you need separate > > objects for different threads then I would follow the pattern I used in > > HidServiceLinux and HidServiceLinux::FileThreadHelper. > > I don't want to unite PlatformSensorProviderLinux with SensorDeviceManager and > SensorDeviceService, because PlatformSensorProviderLinux belongs to a generic > sensor framework. > The last two are missing parts of CrOS/Linux platforms and implemented more like > a helper for a provider (win, android, mac - all they have some sort of > managers). > > I agree that naming might not be appropriate in this context and I need to > rename those classes. > > Why I have a separate instance called SensorDeviceService is because I want the > code to be testable. I have virtual methods in SensorDeviceService, which I can > override in my mock object. What is more, I can easily make my mock object to be > used by the SensorDeviceManager when tests are running. PlatformSensorProviderLinux and SensorDeviceManager live on the browser i/o thread, whereas SensorDeviceService live on a file thread.
Thanks for the explanation. I've had some more time to look through this patch and I think I understand what the structure looks like now. I still recommend the following: * Merge SensorDeviceManager and PlatformSensorProviderLinux. The latter is just a thin wrapper around the former so I don't see what we would lose by merging them. * Put SensorDataLinux and SensorDeviceLinux in the same file with comments explaining how the enumeration process uses one as a template to construct the other. It just needs to be clearer that the construction path is SensorDataLinux -> SensorDeviceLinux -> PlatformSensorLinux. Maybe if they were named SensorPathsLinux and SensorInfoLinux I would be less confused. * Rename SensorDeviceService to SensorManagerLinux. I think that will help emphasize the comparison you made between the architecture on Linux and Windows (which uses an ISensorManager). In general try not to define multiple classes in the same file (structs are an exception if they are simple) and make sure the file names match the class names. That makes it a lot easier to navigate the codebase. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_reader_linux.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Put this in //device/generic_sensor alongside platform_sensor_reader_win.cc. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_reader_linux.h (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_reader_linux.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Put this in //device/generic_sensor alongside platform_sensor_reader_win.h. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/sensor_data_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/sensor_data_linux.cc:190: // NOTIMPLEMENTED(); Remove this line since now we regularly expect to return false. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc:197: InitSensorData(type, &data); EXPECT_TRUE(InitSensorData(type, &data));
https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_manager_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... 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/p... device/generic_sensor/platform_sensor_manager_linux.cc:68: task_runner_->PostTask( could be done on upper call frame https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:73: std::string SensorDeviceService::UdevDeviceGetSubsystem(udev_device* dev) { nit: Get.. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:94: if (subsystem.empty() || strcmp(subsystem.c_str(), "iio") != 0) subsystem.compare() https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:181: for (uint32_t i = first; i < last; ++i) { better to cache sensor nodes instead of running this algorithm again https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:324: CallbackToTypeMap::iterator cb_it = get_device_callbacks_.begin(); I think we can avoid caching callbacks here, if introduce API like "is_initialized()" getter and "initialized()" signal. Callbacks from mojo calls are already initialized in provider, that should be enough. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_linux.cc:66: file_task_runner_ = file_task_runner; runner should not be reset in runtime, otherwise scheduled tasks will be lost.
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
Please review https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_reader_linux.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/29 20:19:56, Reilly Grant wrote: > Put this in //device/generic_sensor alongside platform_sensor_reader_win.cc. Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/platform_sensor_reader_linux.h (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/platform_sensor_reader_linux.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/29 20:19:56, Reilly Grant wrote: > Put this in //device/generic_sensor alongside platform_sensor_reader_win.h. Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... File device/generic_sensor/linux/sensor_data_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/l... device/generic_sensor/linux/sensor_data_linux.cc:190: // NOTIMPLEMENTED(); On 2016/11/29 20:19:56, Reilly Grant wrote: > Remove this line since now we regularly expect to return false. Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_linux.cc:197: InitSensorData(type, &data); On 2016/11/29 20:19:56, Reilly Grant wrote: > EXPECT_TRUE(InitSensorData(type, &data)); Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_manager_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:44: : observer_(this), manager_(nullptr), task_runner_(task_runner) { On 2016/11/30 12:29:36, Mikhail wrote: > nit: move(task_runner) not needed any more. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:73: std::string SensorDeviceService::UdevDeviceGetSubsystem(udev_device* dev) { On 2016/11/30 12:29:36, Mikhail wrote: > nit: Get.. Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:94: if (subsystem.empty() || strcmp(subsystem.c_str(), "iio") != 0) On 2016/11/30 12:29:36, Mikhail wrote: > subsystem.compare() Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:181: for (uint32_t i = first; i < last; ++i) { On 2016/11/30 12:29:36, Mikhail wrote: > better to cache sensor nodes instead of running this algorithm again Done. https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_manager_linux.cc:324: CallbackToTypeMap::iterator cb_it = get_device_callbacks_.begin(); On 2016/11/30 12:29:36, Mikhail wrote: > I think we can avoid caching callbacks here, if introduce API like > "is_initialized()" getter and "initialized()" signal. Callbacks from mojo calls > are already initialized in provider, that should be enough. As long as I've merged this class with the provider class, would you please check another version? https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_linux.cc:66: file_task_runner_ = file_task_runner; On 2016/11/30 12:29:36, Mikhail wrote: > runner should not be reset in runtime, otherwise scheduled tasks will be lost. 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/BUILD.gn:80: "//device/udev_linux", You might need to include the *_linux.* sources in this block to avoid link errors when building without use_udev. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/linux/sensor_data_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/linux/sensor_data_linux.h:18: // SensorDeviceManager received a udev device, it uses this structure to s/received/receives/ https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/linux/sensor_data_linux.h:54: // the last one takes this structure and uses it to create a requested What does "the last one" mean? https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_linux.h:25: SensorInfoLinux* sensor_device, This should probably be a const SensorInfoLinux*. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_manager_linux.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:37: delegate_ = delegate; DCHECK(!delegate_)? https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:88: } nit: no braces around single-line if https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:91: for (auto const& names : data.sensor_file_names) { Spell out the type here so that it is clear what type |name| has below. The nesting makes it otherwise confusing. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:92: for (auto const& name : names) { const auto& https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:138: return device; auto sensor = sensor_devices_by_type_.find(type); if (sensor == sensor_devices_by_type_.end()) return nullptr; return sensor->second.get(); https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:157: file_task_runner_ = task_runner; std::move(task_runner) https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:179: scoped_refptr<PlatformSensorLinux> sensor = nullptr; No initialization necessary, scoped_refptr defaults to nullptr. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.h:71: // Shutdowns a service that tracks events from iio subsystem. s/Shutdowns/Shuts down/ https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.cc:145: sensor_device->apply_scaling_func, sensor_device->device_reading_files, Why not pass |sensor_device| to PollingSensorReader? https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.cc:161: base::Unretained(sensor_))); nit: braces around the body of a multiline if. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.h:28: SensorInfoLinux* sensor_device, This should probably be const.
https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... 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/... device/generic_sensor/platform_sensor_provider_base.cc:130: mojo::ScopedSharedBufferMapping mapping = shared_buffer_handle_->MapAtOffset( this should not be called several times, map once and then return stored mapping
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/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/BUILD.gn:80: "//device/udev_linux", On 2016/12/08 02:31:14, Reilly Grant wrote: > You might need to include the *_linux.* sources in this block to avoid link > errors when building without use_udev. How about including here linux provider and manager only? https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/linux/sensor_data_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/linux/sensor_data_linux.h:18: // SensorDeviceManager received a udev device, it uses this structure to On 2016/12/08 02:31:14, Reilly Grant wrote: > s/received/receives/ Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/linux/sensor_data_linux.h:54: // the last one takes this structure and uses it to create a requested On 2016/12/08 02:31:14, Reilly Grant wrote: > What does "the last one" mean? last one == provider. I've modified the comment https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_linux.h:25: SensorInfoLinux* sensor_device, On 2016/12/08 02:31:14, Reilly Grant wrote: > This should probably be a const SensorInfoLinux*. Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_manager_linux.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:37: delegate_ = delegate; On 2016/12/08 02:31:15, Reilly Grant wrote: > DCHECK(!delegate_)? Well, yeah. Good point. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:88: } On 2016/12/08 02:31:14, Reilly Grant wrote: > nit: no braces around single-line if Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:91: for (auto const& names : data.sensor_file_names) { On 2016/12/08 02:31:14, Reilly Grant wrote: > Spell out the type here so that it is clear what type |name| has below. The > nesting makes it otherwise confusing. Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.cc:92: for (auto const& name : names) { On 2016/12/08 02:31:14, Reilly Grant wrote: > const auto& Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:130: mojo::ScopedSharedBufferMapping mapping = shared_buffer_handle_->MapAtOffset( On 2016/12/08 11:15:22, Mikhail wrote: > this should not be called several times, map once and then return stored mapping But it does not actually overwrite existing mapping. The call chain is the following. PlatformSensorBase calls CreateSensorInternal with mapping as a parameter. When CreateSensorInternal is invoked in PlatformSensorProviderLinux, it checks if it can create a sensor (if enumeration is ready). If not, mapping is just lost, not saved or used. Then, when enumeration becomes ready, mapping is recreated according to pending requests for certain type. MapAtOffset is called only once for each type of sensor. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:138: return device; On 2016/12/08 02:31:15, Reilly Grant wrote: > auto sensor = sensor_devices_by_type_.find(type); > if (sensor == sensor_devices_by_type_.end()) > return nullptr; > return sensor->second.get(); Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:157: file_task_runner_ = task_runner; On 2016/12/08 02:31:15, Reilly Grant wrote: > std::move(task_runner) Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:179: scoped_refptr<PlatformSensorLinux> sensor = nullptr; On 2016/12/08 02:31:15, Reilly Grant wrote: > No initialization necessary, scoped_refptr defaults to nullptr. Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.h:71: // Shutdowns a service that tracks events from iio subsystem. On 2016/12/08 02:31:15, Reilly Grant wrote: > s/Shutdowns/Shuts down/ Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.cc:145: sensor_device->apply_scaling_func, sensor_device->device_reading_files, On 2016/12/08 02:31:15, Reilly Grant wrote: > Why not pass |sensor_device| to PollingSensorReader? Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.cc:161: base::Unretained(sensor_))); On 2016/12/08 02:31:15, Reilly Grant wrote: > nit: braces around the body of a multiline if. Done. https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_linux.h (right): https://codereview.chromium.org/2533793002/diff/100001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.h:28: SensorInfoLinux* sensor_device, On 2016/12/08 02:31:15, Reilly Grant wrote: > This should probably be const. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... 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 is not actually doing anything and I spent a while trying to determine why it wasn't causing your code to crash before I realized that the C++ compiler is smart enough to not generate code to corrupt const data.
Description was changed from ========== [sensors](CrOS/Linux) Implement Sensor device manager for sensors This cl introduces a SensorDeviceManager for Linux and ChromeOS platforms. Manager: 1) SensorDeviceManager own a SensorDeviceService 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 SensorDeviceLinux structure and passed to the manager. If there is no such a device, it is added. In case of removal, manager checks if there was such a device and updates it's cache accordingly. 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) It doesn't search for files anymore, a platform sensor owns and creates it. Unittests: 1) In order to make it possible to test the whole path, I've made sensor device service methods to be virtual that allowed me to mock those methods and override service with a test one. 2) Unittests implement own udev methods that just read values from files. If read was successful, service thinks sensor exists and provider gets all the necessary information to create a platform sensor. BUG=Lazy to search now. ========== to ========== [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 ==========
looks good overall https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_manager_linux.h (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_manager_linux.h:6: #define DEVICE_GENERIC_SENSOR_PLATFORM_SENSOR_MANAGER_LINUX_H_ could we put it to 'linux/platform_sensor_manager.h' https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:129: PlatformSensorProviderBase::GetScopedSharedBufferMapping( pls rename it to 'MapSharedBufferForType()' https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:132: kReadingBufferSize, SensorReadingSharedBuffer::GetOffset(type)); memset(mapping.get(), 0, kReadingBufferSize); https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:39: if (!enumeration_ready()) s/enumeration_ready()/sensor_nodes_enumerated() https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:113: sensor_device_manager_started_ = file_task_runner_->PostTask( could we move "PostTask"s inside SensorDeviceManager, it would simplify client code and make threading model consistent (i.e. public methods are thread safe) https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:188: void PlatformSensorProviderLinux::OnEnumerationReady() { s/OnEnumerationReady/OnSensorNodesEnumerated https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:201: return; DVLOG() to log that sensor is skipped https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:210: if (it == sensor_devices_by_type_.end()) nit: if (it != sensor_devices_by_type_.end() && it->second->device_node == device_node) sensor_devices_by_type_.erase(it);
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_manager_linux.h (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... 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 we put it to 'linux/platform_sensor_manager.h' Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:129: PlatformSensorProviderBase::GetScopedSharedBufferMapping( On 2016/12/09 08:15:56, Mikhail wrote: > pls rename it to 'MapSharedBufferForType()' Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:132: kReadingBufferSize, SensorReadingSharedBuffer::GetOffset(type)); On 2016/12/09 08:15:56, Mikhail wrote: > memset(mapping.get(), 0, kReadingBufferSize); Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:39: if (!enumeration_ready()) On 2016/12/09 08:15:57, Mikhail wrote: > s/enumeration_ready()/sensor_nodes_enumerated() Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:113: sensor_device_manager_started_ = file_task_runner_->PostTask( On 2016/12/09 08:15:57, Mikhail wrote: > could we move "PostTask"s inside SensorDeviceManager, it would simplify client > code and make threading model consistent (i.e. public methods are thread safe) At the moment, we cannot do so, because it would make testing harder to do. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:188: void PlatformSensorProviderLinux::OnEnumerationReady() { On 2016/12/09 08:15:57, Mikhail wrote: > s/OnEnumerationReady/OnSensorNodesEnumerated Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:201: return; On 2016/12/09 08:15:57, Mikhail wrote: > DVLOG() to log that sensor is skipped Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:210: if (it == sensor_devices_by_type_.end()) On 2016/12/09 08:15:57, Mikhail wrote: > nit: if (it != sensor_devices_by_type_.end() && it->second->device_node == > device_node) > sensor_devices_by_type_.erase(it); Done. https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_linux.cc (right): https://codereview.chromium.org/2533793002/diff/120001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_linux.cc:62: sensor_file_paths_(std::move(sensor_device->device_reading_files)), On 2016/12/08 23:19:52, Reilly Grant wrote: > nit: Since sensor_device is const this move is not actually doing anything and I > spent a while trying to determine why it wasn't causing your code to crash > before I realized that the C++ compiler is smart enough to not generate code to > corrupt const data. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:41: if (!sensor_nodes_enumerated_) { if (!sensor_nodes_enumeration_started_) { sensor_nodes_enumeration_started_ =... PostTask(); } return; } https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:108: bool PlatformSensorProviderLinux::InitializeSenorDeviceManager() { we can drop this method (pls see comment above) https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:118: return enumeration_started_; sensor_nodes_enumeration_started_
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 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/2533793002/diff/140001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_linux.cc (right): https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:41: On 2016/12/09 11:17:29, Mikhail wrote: > if (!sensor_nodes_enumerated_) { > if (!sensor_nodes_enumeration_started_) { > sensor_nodes_enumeration_started_ =... PostTask(); > } > return; > } Done. https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:108: bool PlatformSensorProviderLinux::InitializeSenorDeviceManager() { On 2016/12/09 11:17:29, Mikhail wrote: > we can drop this method (pls see comment above) Done. https://codereview.chromium.org/2533793002/diff/140001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_linux.cc:118: return enumeration_started_; On 2016/12/09 11:17:29, Mikhail wrote: > sensor_nodes_enumeration_started_ Done.
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.
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/2533793002/#ps180001 (title: "construct manager")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481297744129850, "parent_rev": "1bc71cc7f828579a1308974c2df77ac5d424f53c", "commit_rev": "b03d20405ff88f6f5354693cdb4b8d55bdb9bb30"}
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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 Review-Url: https://codereview.chromium.org/2533793002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [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 Review-Url: https://codereview.chromium.org/2533793002 ========== to ========== [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} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/061d113db7069c23fe84a0a9365965647ed97b51 Cr-Commit-Position: refs/heads/master@{#437550}
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() |