Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2370343002:
[sensors] Ambient light sensor implementation for ChromeOS and Linux. (Closed)
|
Created:
4 years, 2 months ago by maksims (do not use this acc) Modified:
4 years, 1 month ago Reviewers:
Ken Rockot(use gerrit already), Reilly Grant (use Gerrit), shalamov, timvolodine, alexmos, darktears, Mikhail CC:
darktears, chromium-reviews, riju_ Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] Ambient light sensor implementation for ChromeOS and Linux.
The patch introduces sensors implementation for
ChromeOS/Linux platforms using ambient light sensor. Both
platforms share the same code and require polling threads to
be used.
SensorDataIio structure is used to initialize a generic
SensorReader, which is created only when sensor read files
are found, and to create a concrete sensor, which takes
an ownership of the SensorReader.
A SensorReader must always be created on a polling thread,
which is further used by a sensor to poll data. Each new
sensor will have its own thread in order to avoid blocking of
each other.
As a temp solution to manage polling threads, which are not
passed to new sensors, a manager thread is used. It kills a
polling thread if a sensor cannot be created. In the future,
the manager thread will evolve to a manager class that will
manage finding new sensors attached to a system and notify a
provider about that. The provider will have its own cache in
order to avoid trying to find sensors each time after it
fails to find a requested sensor. Once the manager notifies
the provider about a new sensor, the provider updates its
cache and starts to process requests for that type of sensor.
Intent to Implement:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xLGN2b1-AAAJ
BUG=606766
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2
Cr-Commit-Position: refs/heads/master@{#429584}
Patch Set 1 #Patch Set 2 : ctor and dtor for deleter #
Total comments: 1
Patch Set 3 : rebased on top of Mihail's cl #
Total comments: 1
Patch Set 4 : changed implementation #
Total comments: 28
Patch Set 5 : Newest version. and some comments #
Total comments: 15
Patch Set 6 : Rebased. Comments from Mikhail #
Total comments: 18
Patch Set 7 : comments and added unittests for sensor reader #Patch Set 8 : fixed comment #
Total comments: 15
Patch Set 9 : comments #
Total comments: 2
Patch Set 10 : use CreateUniqueTempDir in tests. Notify sensor error on read error #Patch Set 11 : ... #
Total comments: 4
Patch Set 12 : small fixes #Patch Set 13 : add comments to unittest #
Total comments: 20
Patch Set 14 : Comments from Alex #
Total comments: 18
Patch Set 15 : Comments from Reilly Grant #Patch Set 16 : fix compilation #Patch Set 17 : comments #Patch Set 18 : rebased #
Total comments: 2
Patch Set 19 : pass file thread task runner that is used to destroy sensor threads #Patch Set 20 : rebased. use one polling thread #Patch Set 21 : move set task runner inside if statements #
Total comments: 1
Patch Set 22 : change SetTaskRunner to SetFileTaskRunner as per offline discussion with Mikhail #
Total comments: 30
Patch Set 23 : comments #Patch Set 24 : use std::move #Patch Set 25 : rebased #Patch Set 26 : rebased after win implementation landed #
Total comments: 2
Messages
Total messages: 301 (222 generated)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #1 (id:1) has been deleted
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_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
maksim.sisov@intel.com changed reviewers: + alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com, timvolodine@chromium.org
Please review.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:60002) has been deleted
alexis.menard@intel.com changed reviewers: + alexis.menard@intel.com
https://codereview.chromium.org/2370343002/diff/60001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_ambient_light_iio.cc (right): https://codereview.chromium.org/2370343002/diff/60001/device/generic_sensor/p... device/generic_sensor/platform_sensor_ambient_light_iio.cc:86: StopSensor(); Is this the behavior that we should follow?
Reminder
On 2016/10/06 11:59:01, maksims wrote: > Reminder Let's first introduce SeqLock to generic classes (https://codereview.chromium.org/2395853003/)
Patchset #3 (id:110001) has been deleted
maksim.sisov@intel.com changed reviewers: - alexis.menard@intel.com
please review
alexis.menard@intel.com changed reviewers: + alexis.menard@intel.com
https://codereview.chromium.org/2370343002/diff/130001/device/generic_sensor/... File device/generic_sensor/polling_platform_sensor.cc (right): https://codereview.chromium.org/2370343002/diff/130001/device/generic_sensor/... device/generic_sensor/polling_platform_sensor.cc:30: polling_thread_task_runner_(base::MessageLoop::current()->task_runner()), This is deprecated. Use base::ThreadTaskRunnerHandle::Get().
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...
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. Authors: Maksim Sisov, Rijubrata Bhaumik The patch introduces sensors implemention for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. Checking whether sensors exist or not is done by enumerating specific ambient light files. If a file is not found, the ALS sensor doesn't exist and nullptr is returned. Otherwise, a concrete sensor is returned. This operation is done asynchronously. The changes that introduced asynchronous creation of sensors are in another CL and this patch has dependency on that. What is more, there is another dependency to another CL through "asynchronous CL" that moves around seq lock buffer in order to make it possible to use that in device/generic_sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. Authors: Maksim Sisov, Rijubrata Bhaumik The patch introduces sensors implemention for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. Checking whether sensors exist or not is done by enumerating specific ambient light files. If a file is not found, the ALS sensor doesn't exist and nullptr is returned. Otherwise, a concrete sensor is returned. This operation is done asynchronously. The changes that introduced asynchronous creation of sensors are in another CL and this patch has dependency on that. What is more, there is another dependency to another CL through "asynchronous CL" that moves around seq lock buffer in order to make it possible to use that in device/generic_sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... device_sensors_consts.h taken from https://codereview.chromium.org/2332903002/ BUG=606766 ==========
please review
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/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/BUILD.gn:44: if (is_chromeos || is_linux) { isn't 'is_linux' == true on CrOS? https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/ambient_light_reader_iio.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:17: const base::FilePath::CharType kAmbientLightIioBasePath[] = better make it a constructor argument than hide here. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:26: *lux = std::numeric_limits<double>::infinity(); you should not modify the output parameter on call failure https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:28: if (!base::ReadFileToString(sensor_path_, &value)) how do you know if 'sensor_path_' has been initialized? https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:37: bool AmbientLightSensorReaderIio::DetectLightSensor() { should be renamed to smth like 'InitLightSensorPath' https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:49: for (unsigned int i = 0; i < arraysize(input_names); i++) { s/unsigned int/uint32_t https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:49: for (unsigned int i = 0; i < arraysize(input_names); i++) { nit: ++i https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/ambient_light_reader_iio.h (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.h:19: bool ReadSensorLuxValue(double* lux); methods description is missing https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:10: PlatformSensorIio::PlatformSensorIio( pls add thread check to methods https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:10: PlatformSensorIio::PlatformSensorIio( pls add thread checks to methods https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:50: StopSensor(); is it called on polling thread? if so, than just call StopPoll() https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.h (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.h:26: virtual void UpdateReading() = 0; rename to 'PollForReadingData()'. Can it be protected? https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_base.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_base.cc:79: if (ContainsKey(sensor_map_, type)) why this change is needed? https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:66: polling_thread_->Stop(); won't it be stopped automatically when deleted? We'd rather need a call when there are no sensors left at the provider (can add a virtual method in the parent class to be invoked from 'PlatformSensorProviderBase::RemoveSensor'), and stop the thread inside such call. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:87: base::Bind(&PlatformSensorProviderIio::DoCreateSensor, s/DoCreateSensor/SensorReaderFound https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:120: mojom::SensorType type) { thread check is missing https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_utils_iio.h (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_utils_iio.h:13: class SensorReader { how is that different to ambient_light_reader_iio.h ? https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_utils_iio.h:23: std::vector<base::FilePath> sensor_paths); pass vector as const ref
https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/ambient_light_reader_iio.h (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.h:19: bool ReadSensorLuxValue(double* lux); On 2016/10/11 14:46:46, Mikhail wrote: > methods description is missing This class is old. I've forgotten to remove this. See platform_sensor_utils_iio instead
On 2016/10/11 15:00:57, maksims wrote: > https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... > File device/generic_sensor/ambient_light_reader_iio.h (right): > > https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... > device/generic_sensor/ambient_light_reader_iio.h:19: bool > ReadSensorLuxValue(double* lux); > On 2016/10/11 14:46:46, Mikhail wrote: > > methods description is missing > > > This class is old. I've forgotten to remove this. See platform_sensor_utils_iio > instead same comments would apply there as well
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...
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. Authors: Maksim Sisov, Rijubrata Bhaumik The patch introduces sensors implemention for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. Checking whether sensors exist or not is done by enumerating specific ambient light files. If a file is not found, the ALS sensor doesn't exist and nullptr is returned. Otherwise, a concrete sensor is returned. This operation is done asynchronously. The changes that introduced asynchronous creation of sensors are in another CL and this patch has dependency on that. What is more, there is another dependency to another CL through "asynchronous CL" that moves around seq lock buffer in order to make it possible to use that in device/generic_sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... device_sensors_consts.h taken from https://codereview.chromium.org/2332903002/ BUG=606766 ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. Authors: Maksim Sisov, Rijubrata Bhaumik The patch introduces sensors implemention for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. Checking whether sensors exist or not is done by enumerating specific ambient light files. If a file is not found, the ALS sensor doesn't exist and nullptr is returned. Otherwise, a concrete sensor is returned. This operation is done asynchronously. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 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:170001) has been deleted
please review https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/BUILD.gn:44: if (is_chromeos || is_linux) { On 2016/10/11 14:46:46, Mikhail wrote: > isn't 'is_linux' == true on CrOS? Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/ambient_light_reader_iio.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:17: const base::FilePath::CharType kAmbientLightIioBasePath[] = On 2016/10/11 14:46:46, Mikhail wrote: > better make it a constructor argument than hide here. Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:26: *lux = std::numeric_limits<double>::infinity(); On 2016/10/11 14:46:46, Mikhail wrote: > you should not modify the output parameter on call failure Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:28: if (!base::ReadFileToString(sensor_path_, &value)) On 2016/10/11 14:46:46, Mikhail wrote: > how do you know if 'sensor_path_' has been initialized? Right now, sensor_paths_ is a vector of path. If it is empty, it will not go into the loop. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.cc:37: bool AmbientLightSensorReaderIio::DetectLightSensor() { On 2016/10/11 14:46:46, Mikhail wrote: > should be renamed to smth like 'InitLightSensorPath' Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/ambient_light_reader_iio.h (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/ambient_light_reader_iio.h:19: bool ReadSensorLuxValue(double* lux); On 2016/10/11 14:46:46, Mikhail wrote: > methods description is missing Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:10: PlatformSensorIio::PlatformSensorIio( On 2016/10/11 14:46:47, Mikhail wrote: > pls add thread checks to methods Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.h (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.h:26: virtual void UpdateReading() = 0; On 2016/10/11 14:46:47, Mikhail wrote: > rename to 'PollForReadingData()'. Can it be protected? Done. https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:87: base::Bind(&PlatformSensorProviderIio::DoCreateSensor, On 2016/10/11 14:46:47, Mikhail wrote: > s/DoCreateSensor/SensorReaderFound Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2370343002/diff/190001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2370343002/diff/190001/chrome/browser/about_f... chrome/browser/about_flags.cc:2024: kOsAndroid | kOsMac | kOsLinux | kOsCrOS, nice :) https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:14: const size_t kAlsCols = 5; can omit it https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:30: bool CreateSensorData(mojom::SensorType type, SensorDataIio* data) { DCHECK(data) or pass it as SensorDataIio& https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:6: #define DEVICE_GENERIC_SENSOR_SENSOR_DATA_IIO_H_ looks like it's better to create 'iio' folder and put it and other aux stuff there https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:14: const char kBasePathSensorIio[] = FILE_PATH_LITERAL("/sys/bus/iio/devices"); Declaring them like this would cause copying in every .cc that will include this header. make them static struct members. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:33: polling_thread_task_runner_ = polling_thread_->task_runner(); could be inited in ctor init list. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:103: if ((GetReportingMode() == mojom::ReportingMode::ON_CHANGE)) { GetReportingMode is supposed to be invoked from other thread: comment pls that it's thread safe and declare reporting_mode_ as const https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:139: std::unique_ptr<SensorReader> PlatformSensorProviderIio::GetSensorReader( can we do without this method? (call SensorReader::Create(data); right away)
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/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:14: const size_t kAlsCols = 5; On 2016/10/14 13:59:04, Mikhail wrote: > can omit it Done. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:30: bool CreateSensorData(mojom::SensorType type, SensorDataIio* data) { On 2016/10/14 13:59:04, Mikhail wrote: > DCHECK(data) or pass it as SensorDataIio& Done. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:6: #define DEVICE_GENERIC_SENSOR_SENSOR_DATA_IIO_H_ On 2016/10/14 13:59:04, Mikhail wrote: > looks like it's better to create 'iio' folder and put it and other aux stuff > there It's already there. Haven't changed the string here. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:14: const char kBasePathSensorIio[] = FILE_PATH_LITERAL("/sys/bus/iio/devices"); On 2016/10/14 13:59:04, Mikhail wrote: > Declaring them like this would cause copying in every .cc that will include this > header. > make them static struct members. Done. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:33: polling_thread_task_runner_ = polling_thread_->task_runner(); On 2016/10/14 13:59:05, Mikhail wrote: > could be inited in ctor init list. Done. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:103: if ((GetReportingMode() == mojom::ReportingMode::ON_CHANGE)) { On 2016/10/14 13:59:05, Mikhail wrote: > GetReportingMode is supposed to be invoked from other thread: comment pls that > it's thread safe and declare reporting_mode_ as const reporting_mode_ is already const. https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/190001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:139: std::unique_ptr<SensorReader> PlatformSensorProviderIio::GetSensorReader( On 2016/10/14 13:59:05, Mikhail wrote: > can we do without this method? (call SensorReader::Create(data); right away) Done.
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...
Patchset #7 (id:230001) 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...
Patchset #6 (id:210001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:29: for (size_t i = 0; i < size; i++) { nit: ++i https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:30: base::FilePath als_path = check_path.Append(*(input_names + i)); why als_ ? https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:88: reading->values[i++] = new_value; given parameter should not be modified unless the call is completely successful https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.h (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:16: class SensorReader { needs unit tests https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:28: const char* SensorDataIio::base_path_sensor_iio = FilePath::CharType != char* https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:12: struct SensorDataIio { pls add description comments for the structure and its fields https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:13: static const char* base_path_sensor_iio; does not have to be static https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:21: bool CreateSensorData(mojom::SensorType type, SensorDataIio* data); InitSensorData
https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.h (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:16: class SensorReader { On 2016/10/17 10:34:11, Mikhail wrote: > needs unit tests Potentially tricky. Not all ChromeOS have the aforementioned files so depending what runs on the infra it may or may not actually test/work. Some ChromeOS do not have ambient light sensors so /sys/bus/iio is empty or nonexistent.
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...
https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:29: for (size_t i = 0; i < size; i++) { On 2016/10/17 10:34:11, Mikhail wrote: > nit: ++i Done. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:30: base::FilePath als_path = check_path.Append(*(input_names + i)); On 2016/10/17 10:34:11, Mikhail wrote: > why als_ ? left from prev versions. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:88: reading->values[i++] = new_value; On 2016/10/17 10:34:11, Mikhail wrote: > given parameter should not be modified unless the call is completely successful Done. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.h (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:16: class SensorReader { On 2016/10/17 10:34:11, Mikhail wrote: > needs unit tests Got it! https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:16: class SensorReader { On 2016/10/17 20:35:29, darktears wrote: > On 2016/10/17 10:34:11, Mikhail wrote: > > needs unit tests > > Potentially tricky. Not all ChromeOS have the aforementioned files so depending > what runs on the infra it may or may not actually test/work. Some ChromeOS do > not have ambient light sensors so /sys/bus/iio is empty or nonexistent. There is no problem at all. It's possible to create temp folder with temp files, fill some data and read from them. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:28: const char* SensorDataIio::base_path_sensor_iio = On 2016/10/17 10:34:11, Mikhail wrote: > FilePath::CharType != char* Done. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:12: struct SensorDataIio { On 2016/10/17 10:34:12, Mikhail wrote: > pls add description comments for the structure and its fields Done. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:13: static const char* base_path_sensor_iio; On 2016/10/17 10:34:11, Mikhail wrote: > does not have to be static Done. https://codereview.chromium.org/2370343002/diff/250001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:21: bool CreateSensorData(mojom::SensorType type, SensorDataIio* data); On 2016/10/17 10:34:11, Mikhail wrote: > InitSensorData Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
looks good overall, some comments to the newly added code. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:12: struct SensorDataIio { description, what does this structure represents? https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:17: // Number of words in one set. could you give more concrete description? i.e. "number of sensor files" or smth like that https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:19: // Number of arrays of words. ditto. "corresponds to the sensor reading fields number" https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:23: const char* kSensorFileNameTest1 = "sensor_data1"; nit: const char kSensorFileNameTest1[] https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:71: ASSERT_TRUE(temp_dir_.Set(path)); CreateUniqueTempDir() API looks more appropriate here https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:92: std::string value1, let's make it accept doubles https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:185: const std::string value1 = "20"; double (and in other places) https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:212: WriteStringToFile(temp_sensor_file1, value1); WriteReadingFieldToFile(file, double)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. Authors: Maksim Sisov, Rijubrata Bhaumik The patch introduces sensors implemention for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. Checking whether sensors exist or not is done by enumerating specific ambient light files. If a file is not found, the ALS sensor doesn't exist and nullptr is returned. Otherwise, a concrete sensor is returned. This operation is done asynchronously. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:12: struct SensorDataIio { On 2016/10/18 11:37:20, Mikhail wrote: > description, what does this structure represents? Done. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:17: // Number of words in one set. On 2016/10/18 11:37:20, Mikhail wrote: > could you give more concrete description? i.e. "number of sensor files" or smth > like that Done. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:19: // Number of arrays of words. On 2016/10/18 11:37:21, Mikhail wrote: > ditto. "corresponds to the sensor reading fields number" Done. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:23: const char* kSensorFileNameTest1 = "sensor_data1"; On 2016/10/18 11:37:21, Mikhail wrote: > nit: > const char kSensorFileNameTest1[] Done. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:71: ASSERT_TRUE(temp_dir_.Set(path)); On 2016/10/18 11:37:21, Mikhail wrote: > CreateUniqueTempDir() API looks more appropriate here Why??? I have a dir which lookis like /tmp/.org.chromium.generic_sensor_test/device0. It is created in SetUp. And in order to delete both, I need to first remove device0, take ownership of .org.chromium.generic_sensor_test and only then delete .org.chromium.generic_sensor_test. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:185: const std::string value1 = "20"; On 2016/10/18 11:37:21, Mikhail wrote: > double (and in other places) Done. https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:212: WriteStringToFile(temp_sensor_file1, value1); On 2016/10/18 11:37:21, Mikhail wrote: > WriteReadingFieldToFile(file, double) Done.
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/2370343002/diff/310001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/310001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:96: if (!sensor_reader_->ReadSensorReading(&reading)) { Should notify PlatformSensor about error. check PlatformSensor::NotifySensorError.
On 2016/10/19 06:57:09, maksims wrote: > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > File device/generic_sensor/iio/sensor_data_iio.h (right): > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_data_iio.h:12: struct SensorDataIio { > On 2016/10/18 11:37:20, Mikhail wrote: > > description, what does this structure represents? > > Done. > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_data_iio.h:17: // Number of words in one set. > On 2016/10/18 11:37:20, Mikhail wrote: > > could you give more concrete description? i.e. "number of sensor files" or > smth > > like that > > Done. > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_data_iio.h:19: // Number of arrays of words. > On 2016/10/18 11:37:21, Mikhail wrote: > > ditto. "corresponds to the sensor reading fields number" > > Done. > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > File device/generic_sensor/iio/sensor_reader_unittest.cc (right): > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_reader_unittest.cc:23: const char* > kSensorFileNameTest1 = "sensor_data1"; > On 2016/10/18 11:37:21, Mikhail wrote: > > nit: > > const char kSensorFileNameTest1[] > > Done. > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_reader_unittest.cc:71: > ASSERT_TRUE(temp_dir_.Set(path)); > On 2016/10/18 11:37:21, Mikhail wrote: > > CreateUniqueTempDir() API looks more appropriate here > > Why??? I have a dir which lookis like > /tmp/.org.chromium.generic_sensor_test/device0. > It is created in SetUp. And in order to delete both, I need to first remove > device0, take ownership of .org.chromium.generic_sensor_test and only then > delete .org.chromium.generic_sensor_test. > because it makes sure that there is no directory with the same name is present. what if some of your tests run in parallel?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/19 10:49:07, Mikhail wrote: > On 2016/10/19 06:57:09, maksims wrote: > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > File device/generic_sensor/iio/sensor_data_iio.h (right): > > > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > device/generic_sensor/iio/sensor_data_iio.h:12: struct SensorDataIio { > > On 2016/10/18 11:37:20, Mikhail wrote: > > > description, what does this structure represents? > > > > Done. > > > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > device/generic_sensor/iio/sensor_data_iio.h:17: // Number of words in one set. > > On 2016/10/18 11:37:20, Mikhail wrote: > > > could you give more concrete description? i.e. "number of sensor files" or > > smth > > > like that > > > > Done. > > > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > device/generic_sensor/iio/sensor_data_iio.h:19: // Number of arrays of words. > > On 2016/10/18 11:37:21, Mikhail wrote: > > > ditto. "corresponds to the sensor reading fields number" > > > > Done. > > > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > File device/generic_sensor/iio/sensor_reader_unittest.cc (right): > > > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > device/generic_sensor/iio/sensor_reader_unittest.cc:23: const char* > > kSensorFileNameTest1 = "sensor_data1"; > > On 2016/10/18 11:37:21, Mikhail wrote: > > > nit: > > > const char kSensorFileNameTest1[] > > > > Done. > > > > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > > device/generic_sensor/iio/sensor_reader_unittest.cc:71: > > ASSERT_TRUE(temp_dir_.Set(path)); > > On 2016/10/18 11:37:21, Mikhail wrote: > > > CreateUniqueTempDir() API looks more appropriate here > > > > Why??? I have a dir which lookis like > > /tmp/.org.chromium.generic_sensor_test/device0. > > It is created in SetUp. And in order to delete both, I need to first remove > > device0, take ownership of .org.chromium.generic_sensor_test and only then > > delete .org.chromium.generic_sensor_test. > > > because it makes sure that there is no directory with the same name is present. > what if some of your tests run in parallel? Ok. 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...
On 2016/10/18 11:37:21, Mikhail wrote: > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_reader_unittest.cc:92: std::string value1, > let's make it accept doubles > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_reader_unittest.cc:185: const std::string > value1 = "20"; > double (and in other places) > > https://codereview.chromium.org/2370343002/diff/290001/device/generic_sensor/... > device/generic_sensor/iio/sensor_reader_unittest.cc:212: > WriteStringToFile(temp_sensor_file1, value1); > WriteReadingFieldToFile(file, double) these comments are still valid
Patchset #10 (id:330001) 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...
https://codereview.chromium.org/2370343002/diff/370001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/370001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:59: base::ScopedTempDir sensors_dir; pls use just 'base::CreateDirectory(path)' instead of having another ScopedTempDir instance https://codereview.chromium.org/2370343002/diff/370001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:76: data->base_path_sensor_iio = base_dir.value().c_str(); data->base_path_sensor_iio will contain garbage after base_dir dies.
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/2370343002/diff/310001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/310001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:96: if (!sensor_reader_->ReadSensorReading(&reading)) { On 2016/10/19 10:03:22, shalamov wrote: > Should notify PlatformSensor about error. check > PlatformSensor::NotifySensorError. Done. https://codereview.chromium.org/2370343002/diff/370001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/370001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:59: base::ScopedTempDir sensors_dir; On 2016/10/19 13:10:13, Mikhail wrote: > pls use just 'base::CreateDirectory(path)' instead of having another > ScopedTempDir instance Done. https://codereview.chromium.org/2370343002/diff/370001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:76: data->base_path_sensor_iio = base_dir.value().c_str(); On 2016/10/19 13:10:13, Mikhail wrote: > data->base_path_sensor_iio will contain garbage after base_dir dies. Done.
lgtm https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:88: readings.values[i] = new_value; nit: readings.values[i++] = new_value;
https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:75: bool SensorReader::ReadSensorReading(SensorReading* reading) { why not to pass SensorReading& reading and then populate it here without making temporary? https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.h (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:9: #include "device/generic_sensor/iio/sensor_data_iio.h" forward declare https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:10: #include "device/generic_sensor/public/cpp/sensor_reading.h" ditto https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:68: return configuration.frequency() > 0 && I think we have restricted frequency to be > 0 and <= 60 Hz in PlatformSensorConfiguration. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.h (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.h:11: #include "base/timer/timer.h" forward declare https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.h:13: #include "device/generic_sensor/iio/sensor_data_iio.h" ditto https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:22: PlatformSensorProviderIio(); make private https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:23: ~PlatformSensorProviderIio() override; ditto https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:91: if (!manager_thread_) { who is stopping this thread? Looks like it will be always running even there are no sensors.
https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:75: bool SensorReader::ReadSensorReading(SensorReading* reading) { On 2016/10/19 16:37:11, shalamov wrote: > why not to pass SensorReading& reading and then populate it here without making > temporary? so that the given argument is not modified on function failure. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:91: if (!manager_thread_) { On 2016/10/19 16:37:11, shalamov wrote: > who is stopping this thread? Looks like it will be always running even there are > no sensors. that requires an extra API in generic classes, I'll add it shortly.
On 2016/10/19 17:25:41, Mikhail wrote: > On 2016/10/19 16:37:11, shalamov wrote: > > who is stopping this thread? Looks like it will be always running even there > are > > no sensors. > > that requires an extra API in generic classes, I'll add it shortly. here it is https://codereview.chromium.org/2434073002/
https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:88: readings.values[i] = new_value; On 2016/10/19 13:56:13, Mikhail wrote: > nit: readings.values[i++] = new_value; Done. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.h (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:9: #include "device/generic_sensor/iio/sensor_data_iio.h" On 2016/10/19 16:37:11, shalamov wrote: > forward declare Done. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.h:10: #include "device/generic_sensor/public/cpp/sensor_reading.h" On 2016/10/19 16:37:11, shalamov wrote: > ditto Done. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:68: return configuration.frequency() > 0 && On 2016/10/19 16:37:11, shalamov wrote: > I think we have restricted frequency to be > 0 and <= 60 Hz in > PlatformSensorConfiguration. It will be sensor dependent, when I start to work on accelerometer https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.h (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.h:11: #include "base/timer/timer.h" On 2016/10/19 16:37:11, shalamov wrote: > forward declare Done. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.h:13: #include "device/generic_sensor/iio/sensor_data_iio.h" On 2016/10/19 16:37:11, shalamov wrote: > ditto Done.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
The CQ bit was unchecked by maksim.sisov@intel.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
maksim.sisov@intel.com changed reviewers: + reillyg@chromium.org, rockot@chromium.org
tim, rockot and reilly, please review
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ==========
Patchset #14 (id:430001) 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...
https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:22: PlatformSensorProviderIio(); On 2016/10/19 16:37:11, shalamov wrote: > make private Done. https://codereview.chromium.org/2370343002/diff/410001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:23: ~PlatformSensorProviderIio() override; On 2016/10/19 16:37:11, shalamov wrote: > ditto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_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 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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #16 (id:490001) has been deleted
Patchset #15 (id:470001) has been deleted
Patchset #14 (id:450001) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I defer to reillyg
Why must we use a timer to poll sysfs instead of non-blocking reads on the IIO character device?
On 2016/10/20 20:35:43, Reilly Grant wrote: > Why must we use a timer to poll sysfs instead of non-blocking reads on the IIO > character device? I don't quite understand what you mean. Can you explain?
On 2016/10/20 at 20:59:38, maksim.sisov wrote: > On 2016/10/20 20:35:43, Reilly Grant wrote: > > Why must we use a timer to poll sysfs instead of non-blocking reads on the IIO > > character device? > > I don't quite understand what you mean. Can you explain? In Chrome we try to avoid using timers to poll for data unless there is a well documented reason to prefer it. The Linux IIO subsystem provides a character device that delivers sensor update events and can be polled on Chrome's IO thread along with everything else we wait for events from. Can you investigate that interface?
On 2016/10/20 21:14:31, Reilly Grant wrote: > On 2016/10/20 at 20:59:38, maksim.sisov wrote: > > On 2016/10/20 20:35:43, Reilly Grant wrote: > > > Why must we use a timer to poll sysfs instead of non-blocking reads on the > IIO > > > character device? > > > > I don't quite understand what you mean. Can you explain? > > In Chrome we try to avoid using timers to poll for data unless there is a well > documented reason to prefer it. The Linux IIO subsystem provides a character > device that delivers sensor update events and can be polled on Chrome's IO > thread along with everything else we wait for events from. Can you investigate > that interface? Hi, I've walked through the interface and tried a sample application to get events. I tried both chromeos with kernel 3.14 (I have a Chrome laptop with samus board) and xubuntu with kernel 4.4 (HP revolve 810 G1). I cannot get event fd's from both of those. ioctl just returns "This device does not support events".
On 2016/10/24 08:25:06, maksims wrote: > On 2016/10/20 21:14:31, Reilly Grant wrote: > > On 2016/10/20 at 20:59:38, maksim.sisov wrote: > > > On 2016/10/20 20:35:43, Reilly Grant wrote: > > > > Why must we use a timer to poll sysfs instead of non-blocking reads on the > > IIO > > > > character device? > > > > > > I don't quite understand what you mean. Can you explain? > > > > In Chrome we try to avoid using timers to poll for data unless there is a well > > documented reason to prefer it. The Linux IIO subsystem provides a character > > device that delivers sensor update events and can be polled on Chrome's IO > > thread along with everything else we wait for events from. Can you investigate > > that interface? > > Hi, > > I've walked through the interface and tried a sample application to get events. > I tried both chromeos with kernel 3.14 (I have a Chrome laptop with samus board) > and xubuntu with kernel 4.4 (HP revolve 810 G1). > I cannot get event fd's from both of those. ioctl just returns "This device does > not support events". It seems like its driver dependent and will add more complexity to this. At least, I don't have any sensors with drivers that support events. Thus, I think polling is a good solution at this moment.
It's unfortunate that so many drivers don't support events. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:66: } nit: No braces around a single-line if. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:67: return base::WrapUnique(new SensorReader(std::move(sensor_paths))); base::MakeUnique<SensorReader>(std::move(sensor_paths)) https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:80: int i = 0; DCHECK_LE(sensor_paths_.size(), arraysize(readings.values)); https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:20: const char** sensor_file_names[3]; Since we have a function that builds these structures I'd rather we use std::vector<std::string> here and not have to use the length fields below. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:80: } nit: no braces around single-line if https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:17: bool HasValuesChanged(const SensorReading& lhs, const SensorReading& rhs) { HaveValuesChanged https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:112: if ((GetReportingMode() == mojom::ReportingMode::ON_CHANGE)) { nit: Extra set of parens. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:115: } nit: No braces around single-line if.
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...
Yes, unfortunately, drivers are not that good yet. Hope it will improve in the future. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:66: } On 2016/10/24 21:49:27, Reilly Grant wrote: > nit: No braces around a single-line if. Done. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:67: return base::WrapUnique(new SensorReader(std::move(sensor_paths))); On 2016/10/24 21:49:26, Reilly Grant wrote: > base::MakeUnique<SensorReader>(std::move(sensor_paths)) Done. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:80: int i = 0; On 2016/10/24 21:49:26, Reilly Grant wrote: > DCHECK_LE(sensor_paths_.size(), arraysize(readings.values)); Done. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:20: const char** sensor_file_names[3]; On 2016/10/24 21:49:27, Reilly Grant wrote: > Since we have a function that builds these structures I'd rather we use > std::vector<std::string> here and not have to use the length fields below. Basically, the data is static and is defined only once. What is more, it is a convenient to know how many sensor files must be read. According to this data, we set a certain number of paths, which correspon how many field of SensorReading will be filled in with data. In the future, we will add more sensors, which will have, for example, 3 files to be read from. Thus, I would prefer to use this implementation, otherwise vector of vectors will have to be used. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:80: } On 2016/10/24 21:49:27, Reilly Grant wrote: > nit: no braces around single-line if Done. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/platform_sensor_iio.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:17: bool HasValuesChanged(const SensorReading& lhs, const SensorReading& rhs) { On 2016/10/24 21:49:27, Reilly Grant wrote: > HaveValuesChanged Done. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:112: if ((GetReportingMode() == mojom::ReportingMode::ON_CHANGE)) { On 2016/10/24 21:49:27, Reilly Grant wrote: > nit: Extra set of parens. Done. https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/platform_sensor_iio.cc:115: } On 2016/10/24 21:49:27, Reilly Grant wrote: > nit: No braces around single-line if. 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_...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #15 (id:530001) 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #15 (id:550001) 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...
https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:67: return base::WrapUnique(new SensorReader(std::move(sensor_paths))); On 2016/10/25 06:23:54, maksims wrote: > On 2016/10/24 21:49:26, Reilly Grant wrote: > > base::MakeUnique<SensorReader>(std::move(sensor_paths)) > The constructor is private.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_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 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.
https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.h (right): https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.h:20: const char** sensor_file_names[3]; On 2016/10/25 at 06:23:54, maksims wrote: > On 2016/10/24 21:49:27, Reilly Grant wrote: > > Since we have a function that builds these structures I'd rather we use > > std::vector<std::string> here and not have to use the length fields below. > > Basically, the data is static and is defined only once. What is more, it is a convenient to know how many sensor files must be read. According to this data, we set a certain number of paths, which correspon how many field of SensorReading will be filled in with data. > > In the future, we will add more sensors, which will have, for example, 3 files to be read from. Thus, I would prefer to use this implementation, otherwise vector of vectors will have to be used. A vector of vectors is the right data structure for this.
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...
On 2016/10/25 17:36:25, Reilly Grant wrote: > https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... > File device/generic_sensor/iio/sensor_data_iio.h (right): > > https://codereview.chromium.org/2370343002/diff/510001/device/generic_sensor/... > device/generic_sensor/iio/sensor_data_iio.h:20: const char** > sensor_file_names[3]; > On 2016/10/25 at 06:23:54, maksims wrote: > > On 2016/10/24 21:49:27, Reilly Grant wrote: > > > Since we have a function that builds these structures I'd rather we use > > > std::vector<std::string> here and not have to use the length fields below. > > > > Basically, the data is static and is defined only once. What is more, it is a > convenient to know how many sensor files must be read. According to this data, > we set a certain number of paths, which correspon how many field of > SensorReading will be filled in with data. > > > > In the future, we will add more sensors, which will have, for example, 3 files > to be read from. Thus, I would prefer to use this implementation, otherwise > vector of vectors will have to be used. > > A vector of vectors is the right data structure for this. done!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_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 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_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #18 (id:630001) has been deleted
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_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 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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #19 (id:670001) has been deleted
Patchset #18 (id:650001) has been deleted
Patchset #17 (id:610001) 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...
https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:62: std::unique_ptr<base::Thread> manager_thread_; Can you explain why a separate thread for this is necessary and the browser's usual FILE or IO thread isn't good?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:62: std::unique_ptr<base::Thread> manager_thread_; On 2016/10/26 21:53:42, Reilly Grant wrote: > Can you explain why a separate thread for this is necessary and the browser's > usual FILE or IO thread isn't good? Yes, sure. First of all, BrowserThread is located in content/ and it's not possible to access it directly from device/ due gn layering. Of course, this entire code runs on Browser's IO thread, but it disallows IO and we cannot block browser anyway. Secondly, this manager thread is being used to delete io threads, which are created to 1) find out if sensor's files exist 2) create a sensor reader on that thread 3) pass an ownership of the thread to a concrete sensor that will poll data using that thread. If a sensor doesn't exist, the thread must be removed, but it cannot be removed on browser's IO thread (it disallows IO and basically this an IPC thread as you know) and cannot be removed/stopped on a self running thread (join will crash, of course).
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 #19 (id:730001) 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...
On 2016/10/27 05:29:19, maksims wrote: > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > File device/generic_sensor/platform_sensor_provider_iio.cc (right): > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > device/generic_sensor/platform_sensor_provider_iio.cc:62: > std::unique_ptr<base::Thread> manager_thread_; > On 2016/10/26 21:53:42, Reilly Grant wrote: > > Can you explain why a separate thread for this is necessary and the browser's > > usual FILE or IO thread isn't good? > > Yes, sure. > > First of all, BrowserThread is located in content/ and it's not possible to > access it directly from device/ due gn layering. Of course, this entire code > runs on Browser's IO thread, but it disallows IO and we cannot block browser > anyway. > > Secondly, this manager thread is being used to delete io threads, which are > created to 1) find out if sensor's files exist 2) create a sensor reader on that > thread 3) pass an ownership of the thread to a concrete sensor that will poll > data using that thread. If a sensor doesn't exist, the thread must be removed, > but it cannot be removed on browser's IO thread (it disallows IO and basically > this an IPC thread as you know) and cannot be removed/stopped on a self running > thread (join will crash, of course). Ok, after the last changes the sensor manager is used to create a sensor reader (SensorReader::Create checks if sensors files exist as well. it is a blocking operation as I said. Polling thread is created by the sensor itself only after StartSensor() is called.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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 #20 (id:770001) has been deleted
Patchset #19 (id:750001) 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 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...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #19 (id:790001) has been deleted
On 2016/10/27 at 10:41:57, maksim.sisov wrote: > On 2016/10/27 05:29:19, maksims wrote: > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > > File device/generic_sensor/platform_sensor_provider_iio.cc (right): > > > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > > device/generic_sensor/platform_sensor_provider_iio.cc:62: > > std::unique_ptr<base::Thread> manager_thread_; > > On 2016/10/26 21:53:42, Reilly Grant wrote: > > > Can you explain why a separate thread for this is necessary and the browser's > > > usual FILE or IO thread isn't good? > > > > Yes, sure. > > > > First of all, BrowserThread is located in content/ and it's not possible to > > access it directly from device/ due gn layering. Of course, this entire code > > runs on Browser's IO thread, but it disallows IO and we cannot block browser > > anyway. > > > > Secondly, this manager thread is being used to delete io threads, which are > > created to 1) find out if sensor's files exist 2) create a sensor reader on that > > thread 3) pass an ownership of the thread to a concrete sensor that will poll > > data using that thread. If a sensor doesn't exist, the thread must be removed, > > but it cannot be removed on browser's IO thread (it disallows IO and basically > > this an IPC thread as you know) and cannot be removed/stopped on a self running > > thread (join will crash, of course). > > Ok, after the last changes the sensor manager is used to create a sensor reader (SensorReader::Create checks if sensors files exist as well. it is a blocking operation as I said. > Polling thread is created by the sensor itself only after StartSensor() is called. See ChromeDeviceClient for how we get a handle to BrowserThread::FILE from //device so we can use it for HID and USB. In general we try not to create threads for single tasks. Given that the FILE thread supports running periodic tasks that touch the filesystem I think we can use that thread for the IIO-based sensor implementation.
On 2016/10/27 at 22:49:15, Reilly Grant wrote: > On 2016/10/27 at 10:41:57, maksim.sisov wrote: > > On 2016/10/27 05:29:19, maksims wrote: > > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > > > File device/generic_sensor/platform_sensor_provider_iio.cc (right): > > > > > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > > > device/generic_sensor/platform_sensor_provider_iio.cc:62: > > > std::unique_ptr<base::Thread> manager_thread_; > > > On 2016/10/26 21:53:42, Reilly Grant wrote: > > > > Can you explain why a separate thread for this is necessary and the browser's > > > > usual FILE or IO thread isn't good? > > > > > > Yes, sure. > > > > > > First of all, BrowserThread is located in content/ and it's not possible to > > > access it directly from device/ due gn layering. Of course, this entire code > > > runs on Browser's IO thread, but it disallows IO and we cannot block browser > > > anyway. > > > > > > Secondly, this manager thread is being used to delete io threads, which are > > > created to 1) find out if sensor's files exist 2) create a sensor reader on that > > > thread 3) pass an ownership of the thread to a concrete sensor that will poll > > > data using that thread. If a sensor doesn't exist, the thread must be removed, > > > but it cannot be removed on browser's IO thread (it disallows IO and basically > > > this an IPC thread as you know) and cannot be removed/stopped on a self running > > > thread (join will crash, of course). > > > > Ok, after the last changes the sensor manager is used to create a sensor reader (SensorReader::Create checks if sensors files exist as well. it is a blocking operation as I said. > > Polling thread is created by the sensor itself only after StartSensor() is called. > > See ChromeDeviceClient for how we get a handle to BrowserThread::FILE from //device so we can use it for HID and USB. In general we try not to create threads for single tasks. Given that the FILE thread supports running periodic tasks that touch the filesystem I think we can use that thread for the IIO-based sensor implementation. Since sensors would benefit from low-latency and the FILE thread can be blocked by disk I/O then I would be comfortable with us adding a single thread for polling all sensors, but not one for each.
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 #20 (id:830001) 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...
On 2016/10/27 22:50:33, Reilly Grant wrote: > On 2016/10/27 at 22:49:15, Reilly Grant wrote: > > On 2016/10/27 at 10:41:57, maksim.sisov wrote: > > > On 2016/10/27 05:29:19, maksims wrote: > > > > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > > > > File device/generic_sensor/platform_sensor_provider_iio.cc (right): > > > > > > > > > https://codereview.chromium.org/2370343002/diff/710001/device/generic_sensor/... > > > > device/generic_sensor/platform_sensor_provider_iio.cc:62: > > > > std::unique_ptr<base::Thread> manager_thread_; > > > > On 2016/10/26 21:53:42, Reilly Grant wrote: > > > > > Can you explain why a separate thread for this is necessary and the > browser's > > > > > usual FILE or IO thread isn't good? > > > > > > > > Yes, sure. > > > > > > > > First of all, BrowserThread is located in content/ and it's not possible > to > > > > access it directly from device/ due gn layering. Of course, this entire > code > > > > runs on Browser's IO thread, but it disallows IO and we cannot block > browser > > > > anyway. > > > > > > > > Secondly, this manager thread is being used to delete io threads, which > are > > > > created to 1) find out if sensor's files exist 2) create a sensor reader > on that > > > > thread 3) pass an ownership of the thread to a concrete sensor that will > poll > > > > data using that thread. If a sensor doesn't exist, the thread must be > removed, > > > > but it cannot be removed on browser's IO thread (it disallows IO and > basically > > > > this an IPC thread as you know) and cannot be removed/stopped on a self > running > > > > thread (join will crash, of course). > > > > > > Ok, after the last changes the sensor manager is used to create a sensor > reader (SensorReader::Create checks if sensors files exist as well. it is a > blocking operation as I said. > > > Polling thread is created by the sensor itself only after StartSensor() is > called. > > > > See ChromeDeviceClient for how we get a handle to BrowserThread::FILE from > //device so we can use it for HID and USB. In general we try not to create > threads for single tasks. Given that the FILE thread supports running periodic > tasks that touch the filesystem I think we can use that thread for the IIO-based > sensor implementation. > > Since sensors would benefit from low-latency and the FILE thread can be blocked > by disk I/O then I would be comfortable with us adding a single thread for > polling all sensors, but not one for each. I'm passing a file thread task runner from renderer_frame_host_impl. Now only one thread is used. Please take a look
Patchset #21 (id:870001) 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...
Patchset #20 (id:850001) has been deleted
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...
FYI, I'm going to implement a manager as a next step using udev from //devices. What is more, I found another way allowing to use events from udev nodes. But not all sensors support events. Thus, there will be a strategy - whether to poll or use events.
https://codereview.chromium.org/2370343002/diff/910001/device/generic_sensor/... File device/generic_sensor/sensor_provider_impl.cc (right): https://codereview.chromium.org/2370343002/diff/910001/device/generic_sensor/... device/generic_sensor/sensor_provider_impl.cc:40: provider->SetTaskRunner(task_runner); 1) this is better to be a ctor partameter 2) it should be seen from the arg names that this is a "file" task runner
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...
On 2016/10/28 11:29:03, Mikhail wrote: > https://codereview.chromium.org/2370343002/diff/910001/device/generic_sensor/... > File device/generic_sensor/sensor_provider_impl.cc (right): > > https://codereview.chromium.org/2370343002/diff/910001/device/generic_sensor/... > device/generic_sensor/sensor_provider_impl.cc:40: > provider->SetTaskRunner(task_runner); > 1) this is better to be a ctor partameter > 2) it should be seen from the arg names that this is a "file" task runner Fixed as per offline discussion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/28 11:20:09, maksims wrote: > FYI, I'm going to implement a manager as a next step using udev from //devices. > What is more, I found another way allowing to use events from udev nodes. But > not all sensors support events. Thus, there will be a strategy - whether to poll > or use events. I meant in the next cl. The above mentioned is out of scope of this cl.
https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:81: DCHECK(!file_task_runner->BelongsToCurrentThread()); think this DCHECK is unnecessary. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:82: file_task_runner_ = file_task_runner; Should be only set once. And on the valid thread. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:87: file_task_runner_->PostTask( DCHECK(file_task_runner_); https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:87: file_task_runner_->PostTask( pls add a short comment explaining why this delegated to file_task_runner_ https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:93: DCHECK(file_task_runner_->BelongsToCurrentThread()); DCHECK(file_task_runner_);
a couple drive-by comments below https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:38: polling_thread_.reset(new base::Thread("Linux/CrOS manager thread")); If the file is chrome/linux specific it seems to be clear this is a Linux/CrOS thread. Maybe better name something like "Sensor polling thread" or so.. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.h (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:19: class PlatformSensorProviderIio : public PlatformSensorProvider { shouldn't the platform-specific implementations live in *_linux or *_chromeos files? https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:60: std::unique_ptr<base::Thread> polling_thread_; is this a single thread per renderer or browser? maybe having one 'singleton' polling thread is sufficient? https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:74: #endif /* DEVICE_GENERIC_SENSOR_PUBLIC_PLATFORM_SENSOR_PROVIDER_IIO_H_ */ usually #endif // DEVICE_GENERIC...
https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:38: polling_thread_.reset(new base::Thread("Linux/CrOS manager thread")); On 2016/10/31 18:28:17, timvolodine wrote: > If the file is chrome/linux specific it seems to be clear this is a Linux/CrOS > thread. Maybe better name something like "Sensor polling thread" or so.. thanks. forgot to change that. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.h (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:19: class PlatformSensorProviderIio : public PlatformSensorProvider { On 2016/10/31 18:28:17, timvolodine wrote: > shouldn't the platform-specific implementations live in *_linux or *_chromeos > files? chromeos and linux share the same code. that is why it was decided to name them *_iio https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:60: std::unique_ptr<base::Thread> polling_thread_; On 2016/10/31 18:28:17, timvolodine wrote: > is this a single thread per renderer or browser? maybe having one 'singleton' > polling thread is sufficient? It's a single thread per browser.
https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.h (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:19: class PlatformSensorProviderIio : public PlatformSensorProvider { On 2016/10/31 at 19:21:48, maksims wrote: > On 2016/10/31 18:28:17, timvolodine wrote: > > shouldn't the platform-specific implementations live in *_linux or *_chromeos > > files? > > chromeos and linux share the same code. that is why it was decided to name them *_iio When Chrome OS and Linux share code we generally just call it Linux. *_linux.* sources get built by default on Chrome OS.
lgtm once the existing comments are resolved https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:63: return base::WrapUnique(new SensorReader(std::move(sensor_paths))); Since this is a static function on SensorReader it can use base::MakeUnique even if the constructor is private. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:37: data->sensor_file_names.push_back(file_names); nit: use swap unless preserving any values already in sensor_file_names is important https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:41: EXPECT_EQ(base::WriteFile(file, NULL, 0), 0); nit: nullptr https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:82: data->sensor_file_names.push_back(file_names); nit: Same here, swap is better.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #24 (id:970001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #23 (id:950001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
maksim.sisov@intel.com changed reviewers: + alexmos@chromium.org
alexmos@chromium.org: Please review changes in about_flags https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:63: return base::WrapUnique(new SensorReader(std::move(sensor_paths))); On 2016/10/31 21:18:09, Reilly Grant wrote: > Since this is a static function on SensorReader it can use base::MakeUnique even > if the constructor is private. Nope, it will not event compile! Otherwise I have to make it friend, but it's disallowed. https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/d5FFq... https://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:81: DCHECK(!file_task_runner->BelongsToCurrentThread()); On 2016/10/31 12:50:32, Mikhail wrote: > think this DCHECK is unnecessary. Done. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:82: file_task_runner_ = file_task_runner; On 2016/10/31 12:50:32, Mikhail wrote: > Should be only set once. And on the valid thread. Done. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:87: file_task_runner_->PostTask( On 2016/10/31 12:50:32, Mikhail wrote: > DCHECK(file_task_runner_); Done. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:87: file_task_runner_->PostTask( On 2016/10/31 12:50:32, Mikhail wrote: > pls add a short comment explaining why this delegated to file_task_runner_ Done. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.cc:93: DCHECK(file_task_runner_->BelongsToCurrentThread()); On 2016/10/31 12:50:32, Mikhail wrote: > DCHECK(file_task_runner_); Done. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.h (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:19: class PlatformSensorProviderIio : public PlatformSensorProvider { On 2016/10/31 19:33:21, Reilly Grant wrote: > On 2016/10/31 at 19:21:48, maksims wrote: > > On 2016/10/31 18:28:17, timvolodine wrote: > > > shouldn't the platform-specific implementations live in *_linux or > *_chromeos > > > files? > > > > chromeos and linux share the same code. that is why it was decided to name > them *_iio > > When Chrome OS and Linux share code we generally just call it Linux. *_linux.* > sources get built by default on Chrome OS. Done.
https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:37: data->sensor_file_names.push_back(file_names); On 2016/10/31 21:18:09, Reilly Grant wrote: > nit: use swap unless preserving any values already in sensor_file_names is > important As I said. std::vector<std::string> != std::vector<std::vector<std::string>>. It will require a temp vector. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/sensor_reader_unittest.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:41: EXPECT_EQ(base::WriteFile(file, NULL, 0), 0); On 2016/10/31 21:18:09, Reilly Grant wrote: > nit: nullptr Done. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_reader_unittest.cc:82: data->sensor_file_names.push_back(file_names); On 2016/10/31 21:18:09, Reilly Grant wrote: > nit: Same here, swap is better. It doesn't make any sense, because I'll have to create a temp std::vector<std::vector<std::string>> push back vectors and then do swap. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_iio.h (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_iio.h:74: #endif /* DEVICE_GENERIC_SENSOR_PUBLIC_PLATFORM_SENSOR_PROVIDER_IIO_H_ */ On 2016/10/31 18:28:17, timvolodine wrote: > usually > #endif // DEVICE_GENERIC... Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
maksim.sisov@intel.com changed reviewers: - alexmos@chromium.org
maksim.sisov@intel.com changed reviewers: + alexmos@chromium.org
alexmos@chromium.org: sorry, I meant frame_host/
https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/platform_sensor_utils_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/platform_sensor_utils_iio.cc:63: return base::WrapUnique(new SensorReader(std::move(sensor_paths))); On 2016/11/01 at 08:10:31, maksims wrote: > On 2016/10/31 21:18:09, Reilly Grant wrote: > > Since this is a static function on SensorReader it can use base::MakeUnique even > > if the constructor is private. > > Nope, it will not event compile! Otherwise I have to make it friend, but it's disallowed. > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/iQgMedVA8-k/d5FFq... > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts Right, because the ability to access the constructor is not transitively granted to base::MakeUnique just because it's called from inside the class. Sorry. https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... File device/generic_sensor/iio/sensor_data_iio.cc (right): https://codereview.chromium.org/2370343002/diff/930001/device/generic_sensor/... device/generic_sensor/iio/sensor_data_iio.cc:37: data->sensor_file_names.push_back(file_names); On 2016/11/01 at 08:14:08, maksims wrote: > On 2016/10/31 21:18:09, Reilly Grant wrote: > > nit: use swap unless preserving any values already in sensor_file_names is > > important > > As I said. std::vector<std::string> != std::vector<std::vector<std::string>>. It will require a temp vector. Ah, right. You can use std::move(file_names) to avoid copying the temp vector though which is really the effect I was looking for.
frame_host LGTM
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 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 mikhail.pozdnyakov@intel.com, reillyg@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2370343002/#ps1010001 (title: "use std::move")
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
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
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, alexmos@chromium.org, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/2370343002/#ps1030001 (title: "rebased")
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
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
On 2016/11/02 14:06:47, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...) Note that the linux_site_isolation purpleness is https://bugs.chromium.org/p/chromium/issues/detail?id=661447 and should be fixed shortly.
The CQ bit was checked by maksim.sisov@intel.com
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
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was unchecked by maksim.sisov@intel.com
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_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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, alexmos@chromium.org, mikhail.pozdnyakov@intel.com Link to the patchset: https://codereview.chromium.org/2370343002/#ps1050001 (title: "rebased after win implementation landed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #26 (id:1050001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ==========
Message was sent while issue was closed.
Patchset 26 (id:??) landed as https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584}
Message was sent while issue was closed.
A revert of this CL (patchset #26 id:1050001) has been created in https://codereview.chromium.org/2468283003/ by mathp@chromium.org. The reason for reverting is: Causing a failure on Linux bot: https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/28030.
Message was sent while issue was closed.
https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... File device/generic_sensor/linux/sensor_data_linux.cc (right): https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... device/generic_sensor/linux/sensor_data_linux.cc:17: const std::string kAmbientLightFileNames[] = { This runs initializers, which are forbidden. Use C string constants instead and you should be able to reland.
Message was sent while issue was closed.
On 2016/11/03 at 16:14:35, Ken Rockot wrote: > https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... > File device/generic_sensor/linux/sensor_data_linux.cc (right): > > https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... > device/generic_sensor/linux/sensor_data_linux.cc:17: const std::string kAmbientLightFileNames[] = { > This runs initializers, which are forbidden. Use C string constants instead and you should be able to reland. static* initializers, that is
Message was sent while issue was closed.
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ==========
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...
Description was changed from ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ========== to ========== Reland of [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ==========
Please confirm if I can commit again. https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... File device/generic_sensor/linux/sensor_data_linux.cc (right): https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... device/generic_sensor/linux/sensor_data_linux.cc:17: const std::string kAmbientLightFileNames[] = { On 2016/11/03 16:14:35, Ken Rockot wrote: > This runs initializers, which are forbidden. Use C string constants instead and > you should be able to reland. Oh, thanks! Fixed
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/04 08:37:33, maksims wrote: > Please confirm if I can commit again. > > https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... > File device/generic_sensor/linux/sensor_data_linux.cc (right): > > https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... > device/generic_sensor/linux/sensor_data_linux.cc:17: const std::string > kAmbientLightFileNames[] = { > On 2016/11/03 16:14:35, Ken Rockot wrote: > > This runs initializers, which are forbidden. Use C string constants instead > and > > you should be able to reland. > > Oh, thanks! Fixed I meant - can I reland this cl again or should I create a new one?
On 2016/11/04 11:13:44, maksims wrote: > On 2016/11/04 08:37:33, maksims wrote: > > Please confirm if I can commit again. > > > > > https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... > > File device/generic_sensor/linux/sensor_data_linux.cc (right): > > > > > https://codereview.chromium.org/2370343002/diff/1050001/device/generic_sensor... > > device/generic_sensor/linux/sensor_data_linux.cc:17: const std::string > > kAmbientLightFileNames[] = { > > On 2016/11/03 16:14:35, Ken Rockot wrote: > > > This runs initializers, which are forbidden. Use C string constants instead > > and > > > you should be able to reland. > > > > Oh, thanks! Fixed > > I meant - can I reland this cl again or should I create a new one? Create a new CL, upload patchset 26 and 27 (to show what changed from the 1st one landed) and send it out for review.
Patchset #27 (id:1070001) has been deleted
Description was changed from ========== Reland of [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ========== to ========== [sensors] Ambient light sensor implementation for ChromeOS and Linux. The patch introduces sensors implementation for ChromeOS/Linux platforms using ambient light sensor. Both platforms share the same code and require polling threads to be used. SensorDataIio structure is used to initialize a generic SensorReader, which is created only when sensor read files are found, and to create a concrete sensor, which takes an ownership of the SensorReader. A SensorReader must always be created on a polling thread, which is further used by a sensor to poll data. Each new sensor will have its own thread in order to avoid blocking of each other. As a temp solution to manage polling threads, which are not passed to new sensors, a manager thread is used. It kills a polling thread if a sensor cannot be created. In the future, the manager thread will evolve to a manager class that will manage finding new sensors attached to a system and notify a provider about that. The provider will have its own cache in order to avoid trying to find sensors each time after it fails to find a requested sensor. Once the manager notifies the provider about a new sensor, the provider updates its cache and starts to process requests for that type of sensor. Intent to Implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/TkfdVqYAYiE/xL... BUG=606766 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/de2d07084492be68633a47bae161cf9014ac88e2 Cr-Commit-Position: refs/heads/master@{#429584} ========== |
