Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306453003:
Update AccelerometerReader to support separate iio devices (Closed)
DescriptionUpcoming changes to Chromium OS change how accelerometers are presented.
Currently both base and lid accelerometers are presented as a single iio_device. AccelerometerReader would read this file system to get the needed information.
Now they are being separated into different iio_devices. There have been other changes to the underlying file system that needs to be addressed for correct initialization.
This change update AccelerometerReader to handle this new format. While preserving the legacy path for devices on older kernels.
TEST=Manual testing on devices. This was loaded on a device representing both kernel versions.
BUG=515949, chrome-os-partner:40177, 431391
Committed: https://crrev.com/01d453e2ae32b26bb9384853696db55b082302c4
Cr-Commit-Position: refs/heads/master@{#346148}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Re-add Legacy Support #
Total comments: 5
Patch Set 3 : #
Total comments: 29
Patch Set 4 : Update AcclerometerObservers to handle axes changes #
Total comments: 1
Patch Set 5 : Rebase #Patch Set 6 : Unify Readings with a new config #
Total comments: 6
Patch Set 7 : TODO gyp separation #
Total comments: 16
Patch Set 8 : Update reading #
Total comments: 2
Patch Set 9 : Clarify constants #
Total comments: 7
Patch Set 10 : #
Total comments: 6
Patch Set 11 : #Patch Set 12 : Fix missed tests #
Depends on Patchset: Messages
Total messages: 44 (11 generated)
jonross@chromium.org changed reviewers: + flackr@chromium.org
Hey Rob,
This is an early review of my changes to AccelerometerReader to account for
multiple iio devices.
It does not include the legacy parsing path. Which I'll be adding back in.
Changes on ChromiumOS will have the devices appear at:
/dev/cros-ec-accel/${device_num}
For legacy, where there is only one iio device the symlink has been moved to
/dev/cros-ec-accel/${device_num}
However legacy has the lid/base file name structure still.
PTAL
https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... File chromeos/accelerometer/accelerometer_reader.cc (left): https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:245: configuration_.scale[ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD][i] *= -1.0f; Don't we still need to fix the axes' directions? https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:38: const base::FilePath::CharType kScaleFileName[] = "in_accel_scale"; Can we just remove this if the scale location has changed? This was added to accommodate the name change in kernel 3.18 but if it has changed again it seems this won't ever be the filesystem format. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:94: LOG(ERROR) << "JR Failed to parse \"" << s << "\" from " << path.value(); Remove JR, also as per my comment on 110 this should probably specify that it's expecting an int now that there are multiple types. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:110: LOG(ERROR) << "Accelerometer Failed to parse \"" << s << "\" from " Add the type being parsed so that it's obvious from the log what it's trying to parse as. Also, the LOG should show the filename generating the error so it's probably redundant to say "Accelerometer". https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:237: LOG(ERROR) << "Accelerometer device file invalid.\n"; Output name and change log message to describe exactly what failed so that you can check the file in the filesystem on seeing this error. e.g. "Failed to read symbolic link " << kAccelerometerDevicePath << "/" << name https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:250: for (size_t j = 0; j < arraysize(kAccelerometerNames); ++j) { It would probably be more clear / shorter to use std::find. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:258: LOG(ERROR) << "JR invalid location.\n"; Name the location and the file it was read from so we can debug this. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:268: LOG(ERROR) << "Accelerometer the scale file cannot be parsed\n"; ReadFileToDouble logs if it can't read or parse the file, is this extra log line necessary? https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:274: "scan_elements/in_accel_%s_index", kAccelerometerAxes[j]); Use a constant. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:280: configuration_.scale[config_index][j] = scale; The scale file now contains a double and scales to G's? https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:282: configuration_.device_length[config_index] = kDataSize * 3; This looks constant. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:287: LOG(ERROR) << "JR no location file\n"; I'd suggest splitting this off into functions for reading new and old config if it's not reasonable for the two configurations to share a code path. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:343: base::FilePath(kAccelerometerTriggerPath), "1\n", 2); We don't need multiple triggers?
https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... File chromeos/accelerometer/accelerometer_reader.cc (left): https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:245: configuration_.scale[ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD][i] *= -1.0f; On 2015/08/19 21:43:25, flackr wrote: > Don't we still need to fix the axes' directions? The new values being reported are quite incorrect. Laptop mode thinkgs it's 90 degree portrait tablet. I need to talk with them to see what is actually expected. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:287: LOG(ERROR) << "JR no location file\n"; On 2015/08/19 21:43:25, flackr wrote: > I'd suggest splitting this off into functions for reading new and old config if > it's not reasonable for the two configurations to share a code path. Yeah I had at first wondered if I could keep them merged. Too far diverged. Will refactor out. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:343: base::FilePath(kAccelerometerTriggerPath), "1\n", 2); On 2015/08/19 21:43:24, flackr wrote: > We don't need multiple triggers? Discussion implied only one was needed. I can add logging to confirm.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:38: const base::FilePath::CharType kScaleFileName[] = "in_accel_scale"; On 2015/08/19 21:43:25, flackr wrote: > Can we just remove this if the scale location has changed? This was added to > accommodate the name change in kernel 3.18 but if it has changed again it seems > this won't ever be the filesystem format. Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:94: LOG(ERROR) << "JR Failed to parse \"" << s << "\" from " << path.value(); On 2015/08/19 21:43:25, flackr wrote: > Remove JR, also as per my comment on 110 this should probably specify that it's > expecting an int now that there are multiple types. Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:110: LOG(ERROR) << "Accelerometer Failed to parse \"" << s << "\" from " On 2015/08/19 21:43:25, flackr wrote: > Add the type being parsed so that it's obvious from the log what it's trying to > parse as. Also, the LOG should show the filename generating the error so it's > probably redundant to say "Accelerometer". Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:237: LOG(ERROR) << "Accelerometer device file invalid.\n"; On 2015/08/19 21:43:25, flackr wrote: > Output name and change log message to describe exactly what failed so that you > can check the file in the filesystem on seeing this error. e.g. "Failed to read > symbolic link " << kAccelerometerDevicePath << "/" << name Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:258: LOG(ERROR) << "JR invalid location.\n"; On 2015/08/19 21:43:25, flackr wrote: > Name the location and the file it was read from so we can debug this. Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:268: LOG(ERROR) << "Accelerometer the scale file cannot be parsed\n"; On 2015/08/19 21:43:24, flackr wrote: > ReadFileToDouble logs if it can't read or parse the file, is this extra log line > necessary? Nope, missed this when refactoring readtodouble out. removed. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:274: "scan_elements/in_accel_%s_index", kAccelerometerAxes[j]); On 2015/08/19 21:43:25, flackr wrote: > Use a constant. Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:280: configuration_.scale[config_index][j] = scale; On 2015/08/19 21:43:25, flackr wrote: > The scale file now contains a double and scales to G's? Correct, this was also changed in the new kernel. Also one scale per accelerometer, instead of per axis. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:282: configuration_.device_length[config_index] = kDataSize * 3; On 2015/08/19 21:43:25, flackr wrote: > This looks constant. Done. https://codereview.chromium.org/1306453003/diff/1/chromeos/accelerometer/acce... chromeos/accelerometer/accelerometer_reader.cc:343: base::FilePath(kAccelerometerTriggerPath), "1\n", 2); On 2015/08/19 22:22:24, jonross wrote: > On 2015/08/19 21:43:24, flackr wrote: > > We don't need multiple triggers? > > Discussion implied only one was needed. I can add logging to confirm. Confirmed with on device testing. One trigger.
jonross@chromium.org changed reviewers: + oshima@chromium.org
Hey Oshima, Could you please review my change to AccelerometerReader? Due to changes in the kernel, the file system for accelerometers has changed. Now each accelerometer is a separate iio_device. This change updates AccelerometerReader to handle the entire new file format. Meanwhile I'm preserving the legacy code paths, as we will have Chrome versions shipping to devices across kernel versions. Thanks, Jon
https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:180: base::FilePath device_path[ACCELEROMETER_SOURCE_COUNT]; don't have to be now, but it looks to me that the struct gets complicated enough to consider refactoring this so that you can access by record, rather than index like Record& record = data_[index]; record.scale[]... https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:188: bool InitializeDevice(base::FilePath& iio_path, passing non const reference is uncommon, and out parameters should usually be pointers. Is there any reason you want them to be references? https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:194: void InitializeBothAccelerometers(base::FilePath& iio_path); ditto
https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:180: base::FilePath device_path[ACCELEROMETER_SOURCE_COUNT]; On 2015/08/21 06:44:46, oshima (In Tokyo 17th - 21st) wrote: > don't have to be now, but it looks to me that the struct gets complicated enough > to consider refactoring this so that you can access by record, rather than index > like > > Record& record = data_[index]; > record.scale[]... > When all devices are updated to the new kernel, we could actually remove the legacy data from this struct. Then switch the struct to be a record of an individual accelerometer. https://codereview.chromium.org/1306453003/diff/20002/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:188: bool InitializeDevice(base::FilePath& iio_path, On 2015/08/21 06:44:46, oshima (In Tokyo 17th - 21st) wrote: > passing non const reference is uncommon, and out parameters should usually be > pointers. Is there any reason you want them to be references? I forgot to add const.... :(
https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:65: const char kSeparateAccelerometerAxes[][2] = {"x", "y", "z"}; It seems suspicious that the axes have changed order. Have you tested this with a glimmer / clapper on kernel 3.18? https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:194: void InitializeBothAccelerometers(const base::FilePath& iio_path); To emphasize that these two methods are for doing the same thing but for old and new kernel behavior, how about something like InitializeLegacyAccelerometer and InitializeAccelerometer? https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:337: std::string* location) { Should be able to share most of this code with the other Initialize method. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:339: base::TrimWhitespaceASCII(*location, base::TRIM_ALL, location); You should do this before passing the string in and pass in a const std::string& https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:341: for (size_t j = 0; j < arraysize(kAccelerometerNames); ++j) { Use std::find https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:349: LOG(ERROR) << "Invalid location: " << location << "\n"; Identify which device this came from. Also let's call this an unrecognized location. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:356: if (!ReadFileToDouble(iio_path.Append(kAccelerometerScaleFileName), &scale)) { nit: no curlies { } https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:365: configuration_.has[config_index] = false; Since the format has changed again, we should know now (again) whether to expect these files to be there or not, so make this failure more verbose / fail instantly if the axis index file is missing. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:405: configuration_.has[i] = false; Same rationale as 365, make this fatal again. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:423: LOG(ERROR) << "Accelerometer trigger failure: " << bytes_written; Keep the PLOG here. The purpose is to display the last system error message if the write fails: See PLOG code here https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=P... https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:430: if (configuration_.read_device_separately) { Thinking this over, it's not clear to me that the read code needs to branch. We could isolate the differences to the code which sets up the configuration data. I'm thinking we could have a list of device files to read, for each of which you have a list of locations in order. e.g. pseudocode: old config = {"/dev/cros-ec-accel": [LID, BASE]} new config = {"/dev/cros-ec-accel/device0": [LID], "/dev/cros-ec-accel/device1": [BASE]} Then once you have access to the 6 bytes of data for each accelerometer you can call into the same old code to read the data from the correct reading array. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:453: LOG(ERROR) << "Acceleromter Read " << bytes_read << " byte(s), expected " nit: s/Accelerometer/Accelerometer Since this is not reading from a single device anymore identify the device that was read from.
https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:65: const char kSeparateAccelerometerAxes[][2] = {"x", "y", "z"}; On 2015/08/21 19:12:03, flackr wrote: > It seems suspicious that the axes have changed order. Have you tested this with > a glimmer / clapper on kernel 3.18? kernel 3.18 is not available for glimmer/clapper. On kernel 3.14 the axes are still written in this order. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:194: void InitializeBothAccelerometers(const base::FilePath& iio_path); On 2015/08/21 19:12:03, flackr wrote: > To emphasize that these two methods are for doing the same thing but for old and > new kernel behavior, how about something like InitializeLegacyAccelerometer and > InitializeAccelerometer? I was tempted to do this when I thought we would be able to remove the legacy path. However with no plans to bring minnie et al. up to 3.18, I'm not sure if this is appropriate. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:337: std::string* location) { On 2015/08/21 19:12:02, flackr wrote: > Should be able to share most of this code with the other Initialize method. Will look at merging as much as possible. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:339: base::TrimWhitespaceASCII(*location, base::TRIM_ALL, location); On 2015/08/21 19:12:03, flackr wrote: > You should do this before passing the string in and pass in a const std::string& Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:341: for (size_t j = 0; j < arraysize(kAccelerometerNames); ++j) { On 2015/08/21 19:12:02, flackr wrote: > Use std::find Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:349: LOG(ERROR) << "Invalid location: " << location << "\n"; On 2015/08/21 19:12:03, flackr wrote: > Identify which device this came from. Also let's call this an unrecognized > location. Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:356: if (!ReadFileToDouble(iio_path.Append(kAccelerometerScaleFileName), &scale)) { On 2015/08/21 19:12:03, flackr wrote: > nit: no curlies { } Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:365: configuration_.has[config_index] = false; On 2015/08/21 19:12:03, flackr wrote: > Since the format has changed again, we should know now (again) whether to expect > these files to be there or not, so make this failure more verbose / fail > instantly if the axis index file is missing. Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:405: configuration_.has[i] = false; On 2015/08/21 19:12:03, flackr wrote: > Same rationale as 365, make this fatal again. Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:423: LOG(ERROR) << "Accelerometer trigger failure: " << bytes_written; On 2015/08/21 19:12:03, flackr wrote: > Keep the PLOG here. The purpose is to display the last system error message if > the write fails: See PLOG code here > https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&q=P... Done. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:430: if (configuration_.read_device_separately) { On 2015/08/21 19:12:03, flackr wrote: > Thinking this over, it's not clear to me that the read code needs to branch. We > could isolate the differences to the code which sets up the configuration data. > > I'm thinking we could have a list of device files to read, for each of which you > have a list of locations in order. > e.g. pseudocode: > old config = {"/dev/cros-ec-accel": [LID, BASE]} > new config = {"/dev/cros-ec-accel/device0": [LID], > "/dev/cros-ec-accel/device1": [BASE]} > > Then once you have access to the 6 bytes of data for each accelerometer you can > call into the same old code to read the data from the correct reading array. The updated init gives us: old config = {"/dev/cros-ec-accel/0": [LID, BASE]} new config = {"/dev/cros-ec-accel/0": [LID], "/dev/cros-ec-accel/1": [BASE]} Where the numbers are never guaranteed. I could update the reading to split based on the two paths. Then remerge the generation of the update. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:453: LOG(ERROR) << "Acceleromter Read " << bytes_read << " byte(s), expected " On 2015/08/21 19:12:02, flackr wrote: > nit: s/Accelerometer/Accelerometer > > Since this is not reading from a single device anymore identify the device that > was read from. Done. https://codereview.chromium.org/1306453003/diff/70001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/70001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:408: // type specified in chromeos/accelerometer/accelerometer_types.h. With the latest change to the kernel, the axes and signs of accelerometer values match the accelerometer_types.h, as well as the html5 spec. ScreenOrientationController was updates to work with the correct axes. SensorManagerChromeOs removes the remapping of values it had. The adjustments here bring the 3.14 kernel values inline with those of 3.18, while supporting the fixes to ScreenOrientationController and SensorManagerChromeOs.
https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:194: void InitializeBothAccelerometers(const base::FilePath& iio_path); On 2015/08/24 18:30:34, jonross wrote: > On 2015/08/21 19:12:03, flackr wrote: > > To emphasize that these two methods are for doing the same thing but for old > and > > new kernel behavior, how about something like InitializeLegacyAccelerometer > and > > InitializeAccelerometer? > > I was tempted to do this when I thought we would be able to remove the legacy > path. However with no plans to bring minnie et al. up to 3.18, I'm not sure if > this is appropriate. I think it's still appropriate. Include in the comment the kernel version(s) each is for and probably reference the bug to remove the legacy behavior that I filed: http://crbug.com/510831. The bug should probably be updated to reflect what the legacy behavior now is. It occurs to me the right solution here is probably to define a gyp flag to switch the behavior which we can add on the chromeos side. Then the initialization could be defined in two files and which one gets compiled in depends on this flag. For an example where this is switched by platform, see the InitHostLogging function: https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/logg... https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:430: if (configuration_.read_device_separately) { On 2015/08/24 18:30:34, jonross wrote: > On 2015/08/21 19:12:03, flackr wrote: > > Thinking this over, it's not clear to me that the read code needs to branch. > We > > could isolate the differences to the code which sets up the configuration > data. > > > > I'm thinking we could have a list of device files to read, for each of which > you > > have a list of locations in order. > > e.g. pseudocode: > > old config = {"/dev/cros-ec-accel": [LID, BASE]} > > new config = {"/dev/cros-ec-accel/device0": [LID], > > "/dev/cros-ec-accel/device1": [BASE]} > > > > Then once you have access to the 6 bytes of data for each accelerometer you > can > > call into the same old code to read the data from the correct reading array. > > The updated init gives us: > old config = {"/dev/cros-ec-accel/0": [LID, BASE]} > new config = {"/dev/cros-ec-accel/0": [LID], > "/dev/cros-ec-accel/1": [BASE]} > > Where the numbers are never guaranteed. I think my suggestion was misunderstood, I was suggesting generating a config structure like the above but with the correctly detected path numbers in the initialize method so that we can have a single read method. > > I could update the reading to split based on the two paths. Then remerge the > generation of the update.
https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:194: void InitializeBothAccelerometers(const base::FilePath& iio_path); On 2015/08/25 21:27:55, flackr wrote: > On 2015/08/24 18:30:34, jonross wrote: > > On 2015/08/21 19:12:03, flackr wrote: > > > To emphasize that these two methods are for doing the same thing but for old > > and > > > new kernel behavior, how about something like InitializeLegacyAccelerometer > > and > > > InitializeAccelerometer? > > > > I was tempted to do this when I thought we would be able to remove the legacy > > path. However with no plans to bring minnie et al. up to 3.18, I'm not sure if > > this is appropriate. > > I think it's still appropriate. Include in the comment the kernel version(s) > each is for and probably reference the bug to remove the legacy behavior that I > filed: http://crbug.com/510831. The bug should probably be updated to reflect > what the legacy behavior now is. > > It occurs to me the right solution here is probably to define a gyp flag to > switch the behavior which we can add on the chromeos side. Then the > initialization could be defined in two files and which one gets compiled in > depends on this flag. For an example where this is switched by platform, see the > InitHostLogging function: > https://code.google.com/p/chromium/codesearch#chromium/src/remoting/host/logg... We might want to switch to the gyp flag style. From recent discussions there isn't a plan to backport these changes to 3.14, which would leave us maintaining these two paths for quite some time. With touchview currently broken in ToT I would recommend that we do that as a followup. https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:430: if (configuration_.read_device_separately) { On 2015/08/25 21:27:55, flackr wrote: > On 2015/08/24 18:30:34, jonross wrote: > > On 2015/08/21 19:12:03, flackr wrote: > > > Thinking this over, it's not clear to me that the read code needs to branch. > > We > > > could isolate the differences to the code which sets up the configuration > > data. > > > > > > I'm thinking we could have a list of device files to read, for each of which > > you > > > have a list of locations in order. > > > e.g. pseudocode: > > > old config = {"/dev/cros-ec-accel": [LID, BASE]} > > > new config = {"/dev/cros-ec-accel/device0": [LID], > > > "/dev/cros-ec-accel/device1": [BASE]} > > > > > > Then once you have access to the 6 bytes of data for each accelerometer you > > can > > > call into the same old code to read the data from the correct reading array. > > > > The updated init gives us: > > old config = {"/dev/cros-ec-accel/0": [LID, BASE]} > > new config = {"/dev/cros-ec-accel/0": [LID], > > "/dev/cros-ec-accel/1": [BASE]} > > > > Where the numbers are never guaranteed. > > I think my suggestion was misunderstood, I was suggesting generating a config > structure like the above but with the correctly detected path numbers in the > initialize method so that we can have a single read method. > > > > > I could update the reading to split based on the two paths. Then remerge the > > generation of the update. Ah, I did misinterpret your previous comment there.
Patchset #6 (id:110001) has been deleted
https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/50001/chromeos/accelerometer/... chromeos/accelerometer/accelerometer_reader.cc:430: if (configuration_.read_device_separately) { On 2015/08/25 22:26:07, jonross wrote: > On 2015/08/25 21:27:55, flackr wrote: > > On 2015/08/24 18:30:34, jonross wrote: > > > On 2015/08/21 19:12:03, flackr wrote: > > > > Thinking this over, it's not clear to me that the read code needs to > branch. > > > We > > > > could isolate the differences to the code which sets up the configuration > > > data. > > > > > > > > I'm thinking we could have a list of device files to read, for each of > which > > > you > > > > have a list of locations in order. > > > > e.g. pseudocode: > > > > old config = {"/dev/cros-ec-accel": [LID, BASE]} > > > > new config = {"/dev/cros-ec-accel/device0": [LID], > > > > "/dev/cros-ec-accel/device1": [BASE]} > > > > > > > > Then once you have access to the 6 bytes of data for each accelerometer > you > > > can > > > > call into the same old code to read the data from the correct reading > array. > > > > > > The updated init gives us: > > > old config = {"/dev/cros-ec-accel/0": [LID, BASE]} > > > new config = {"/dev/cros-ec-accel/0": [LID], > > > "/dev/cros-ec-accel/1": [BASE]} > > > > > > Where the numbers are never guaranteed. > > > > I think my suggestion was misunderstood, I was suggesting generating a config > > structure like the above but with the correctly detected path numbers in the > > initialize method so that we can have a single read method. > > > > > > > > I could update the reading to split based on the two paths. Then remerge the > > > generation of the update. > > Ah, I did misinterpret your previous comment there. Done.
jonross@chromium.org changed reviewers: + derat@chromium.org
Hi Derat, Could you provide an owners review to the changes in: ash/content/display/* chromeos/accelerometer/* Thanks, Jon
jonross@chromium.org changed reviewers: + timvolodine@chromium.org
Hi Tim, Could you provide an owners review of content/browser/device_sensors/* Thanks, Jon
content/browser/device_sensors/ -- lgtm https://codereview.chromium.org/1306453003/diff/150001/content/browser/device... File content/browser/device_sensors/sensor_manager_chromeos.cc (left): https://codereview.chromium.org/1306453003/diff/150001/content/browser/device... content/browser/device_sensors/sensor_manager_chromeos.cc:144: // accelerometer values have been fixed. crbug.com/431391 q: has crbug.com/431391 been fixed?
https://codereview.chromium.org/1306453003/diff/150001/content/browser/device... File content/browser/device_sensors/sensor_manager_chromeos.cc (left): https://codereview.chromium.org/1306453003/diff/150001/content/browser/device... content/browser/device_sensors/sensor_manager_chromeos.cc:144: // accelerometer values have been fixed. crbug.com/431391 On 2015/08/27 15:16:59, timvolodine wrote: > q: has crbug.com/431391 been fixed? There have been changes on the firmware/kernel side to partially fix 431391. The changes in this review to accelerometer_reader have what is necessary to fix them for older kernels. So this workaround is no longer needed.
https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:59: const char kSeparateAccelerometerScanIndexPath[] = marginal nit: i think i'd prefer if this constant and kAccelerometerScanIndexPath had names ending with "FormatString" https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:335: int config_index = std::find(std::begin(kAccelerometerNames), huh, i hadn't seen this before. just to double-check, is it allowed in chromium? https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:337: std::begin(kAccelerometerNames); is it safe to subtract std::begin from std::last (if no value is found) like this? part of me feels like the simple loop-based approach (one line longer) is easier to read, but i can be swayed if flackr@ disagrees. :-) int config_index = 0; for (; config_index < ARRAYSIZE(kAccelerometerNames); ++config_index) { if (location == kAccelerometerNames[config_index]) break; } if (config_index >= ARRAYSIZE(kAccelerometerNames)) { LOG(ERROR) << ... https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:348: const int number_axes = arraysize(kSeparateAccelerometerAxes); nit: kNumberAxes https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:349: for (size_t j = 0; j < number_axes; ++j) { nit: s/j/i/ https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:443: int bytes_read; nit: scope this within the for loop
https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:59: const char kSeparateAccelerometerScanIndexPath[] = nit: Let's consistently call variables only relevant to the old paths "Legacy". SeperateAccelerometer and Unified makes it seem like the two need to be simultaneously supported. This will also make it more evident what can be removed / moved to a separate file if we split this up. https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:78: const size_t kDataSizeForSeparateDevices = 6; This is technically the reading size regardless of if they're separate or not. Use a formula here to show what it comes from: e.g. kDataSize * kNumberOfAxes https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:89: // The size of data in one reading of the accelerometers. nit: This (and above) should be the "maximum" size. We only actually use this for the buffer size to read into but there can actually be fewer accelerometers and we only actually read reading_data.length bytes. https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:164: size_t offset; Instead of length and offset, if we have std::vector<AccelerometerSource> sources then the read code can process each file immediately, e.g.: Then the read code would look like: update_ = new AccelerometerUpdate(); for (auto reading_data : configuration_.reading_data) { ReadFile(reading_data.path, buffer, reading_data.size() * kReadingSize); for (AccelerometerSource source : reading_data.sources) { int16* values = reinterpret_cast<int16*>(buffer); update_->Set(...); } } The main advantage being we don't have to change the indices we've read from the accelerometer config so it's closer to the real /sys filesystem and it's closer to what we'll need when we have some sensors reading at separate rates. https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:337: std::begin(kAccelerometerNames); On 2015/08/27 15:36:19, Daniel Erat wrote: > is it safe to subtract std::begin from std::last (if no value is found) like > this? std::find returns the second argument (the end position, last) if it doesn't find the item. This means if we fail to find the item config_index will actually be ARRAYSIZE(kAccelerometerNames) (e.g. 2, not -1). That being said, this is confusing to deduce. > > part of me feels like the simple loop-based approach (one line longer) is easier > to read, but i can be swayed if flackr@ disagrees. :-) > > int config_index = 0; > for (; config_index < ARRAYSIZE(kAccelerometerNames); ++config_index) { > if (location == kAccelerometerNames[config_index]) > break; > } > if (config_index >= ARRAYSIZE(kAccelerometerNames)) { > LOG(ERROR) << ... I hadn't realized all the iterator conversions that using std::find with arrays requires. I had imagined something like. auto accel_iter = std::find(std::begin(kAccelerometerNames), std::end(kAccelerometerNames), location); if (accel_iter == std::end(kAccelerometerNames) { LOG(ERROR) << "..." return; } int config_index = accel_iter - std::begin(kAccelerometerNames); But seeing it written out, the loop approach is probably simpler.
https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:59: const char kSeparateAccelerometerScanIndexPath[] = On 2015/08/27 15:36:19, Daniel Erat wrote: > marginal nit: i think i'd prefer if this constant and > kAccelerometerScanIndexPath had names ending with "FormatString" Done. https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:164: size_t offset; On 2015/08/27 16:24:22, flackr wrote: > Instead of length and offset, if we have std::vector<AccelerometerSource> > sources then the read code can process each file immediately, e.g.: > > Then the read code would look like: > update_ = new AccelerometerUpdate(); > for (auto reading_data : configuration_.reading_data) { > ReadFile(reading_data.path, buffer, reading_data.size() * kReadingSize); > for (AccelerometerSource source : reading_data.sources) { > int16* values = reinterpret_cast<int16*>(buffer); > update_->Set(...); > } > } > > The main advantage being we don't have to change the indices we've read from the > accelerometer config so it's closer to the real /sys filesystem and it's closer > to what we'll need when we have some sensors reading at separate rates. Done. https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:337: std::begin(kAccelerometerNames); On 2015/08/27 16:24:22, flackr wrote: > On 2015/08/27 15:36:19, Daniel Erat wrote: > > is it safe to subtract std::begin from std::last (if no value is found) like > > this? > > std::find returns the second argument (the end position, last) if it doesn't > find the item. This means if we fail to find the item config_index will actually > be ARRAYSIZE(kAccelerometerNames) (e.g. 2, not -1). That being said, this is > confusing to deduce. > > > > > part of me feels like the simple loop-based approach (one line longer) is > easier > > to read, but i can be swayed if flackr@ disagrees. :-) > > > > int config_index = 0; > > for (; config_index < ARRAYSIZE(kAccelerometerNames); ++config_index) { > > if (location == kAccelerometerNames[config_index]) > > break; > > } > > if (config_index >= ARRAYSIZE(kAccelerometerNames)) { > > LOG(ERROR) << ... > > I hadn't realized all the iterator conversions that using std::find with arrays > requires. I had imagined something like. > > auto accel_iter = std::find(std::begin(kAccelerometerNames), > std::end(kAccelerometerNames), location); > if (accel_iter == std::end(kAccelerometerNames) { > LOG(ERROR) << "..." > return; > } > int config_index = accel_iter - std::begin(kAccelerometerNames); > > But seeing it written out, the loop approach is probably simpler. Yeah I thought std:find would have been cleaner than it was. Switched to the for loop https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:348: const int number_axes = arraysize(kSeparateAccelerometerAxes); On 2015/08/27 15:36:19, Daniel Erat wrote: > nit: kNumberAxes Done. https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:349: for (size_t j = 0; j < number_axes; ++j) { On 2015/08/27 15:36:19, Daniel Erat wrote: > nit: s/j/i/ Done. https://codereview.chromium.org/1306453003/diff/150001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:443: int bytes_read; On 2015/08/27 15:36:19, Daniel Erat wrote: > nit: scope this within the for loop Done.
lgtm as an owner, but please wait for rob to take another look https://codereview.chromium.org/1306453003/diff/170001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/170001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:151: // The accelerometer sources which can be read from |path| nit: add trailing period
Updated, including the names of the constants to specify which are legacy. A request of flackr@ I missed. I'll await flackr@'s approval before landing. https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:59: const char kSeparateAccelerometerScanIndexPath[] = On 2015/08/27 16:24:22, flackr wrote: > nit: Let's consistently call variables only relevant to the old paths "Legacy". > SeperateAccelerometer and Unified makes it seem like the two need to be > simultaneously supported. This will also make it more evident what can be > removed / moved to a separate file if we split this up. Done. https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:78: const size_t kDataSizeForSeparateDevices = 6; On 2015/08/27 16:24:22, flackr wrote: > This is technically the reading size regardless of if they're separate or not. > > Use a formula here to show what it comes from: e.g. kDataSize * kNumberOfAxes Done. https://codereview.chromium.org/1306453003/diff/130001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:89: // The size of data in one reading of the accelerometers. On 2015/08/27 16:24:21, flackr wrote: > nit: This (and above) should be the "maximum" size. We only actually use this > for the buffer size to read into but there can actually be fewer accelerometers > and we only actually read reading_data.length bytes. Merged with above to represent the size of one read, and calculated vs the length of reading_data.sources. https://codereview.chromium.org/1306453003/diff/170001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/170001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:151: // The accelerometer sources which can be read from |path| On 2015/08/27 20:23:20, Daniel Erat wrote: > nit: add trailing period Done.
https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:414: reading_data.sources.push_back(ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD); This isn't necessarily true, reading_data.sources should be the same as configuration_.has, e.g. add to sources where count is incremented in the if from line 401. https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:436: int bytes_read = base::ReadFile(reading_data.path, reading, reading_size); Maybe DCHECK_GT(reading_size, 0) before this to be sure we don't have reading configurations with an empty sources vector. https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:444: if (!configuration_.has[source]) If it's in reading_data.sources, it should also "have" this accelerometer. In fact, I feel as though the has array is unnecessary at this point since I don't think we need to quickly answer that question, but I don't feel strongly that it needs to be removed.
https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:414: reading_data.sources.push_back(ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD); On 2015/08/27 21:48:23, flackr wrote: > This isn't necessarily true, reading_data.sources should be the same as > configuration_.has, e.g. add to sources where count is incremented in the if > from line 401. Done. https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:436: int bytes_read = base::ReadFile(reading_data.path, reading, reading_size); On 2015/08/27 21:48:23, flackr wrote: > Maybe DCHECK_GT(reading_size, 0) before this to be sure we don't have reading > configurations with an empty sources vector. Done. https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:444: if (!configuration_.has[source]) On 2015/08/27 21:48:23, flackr wrote: > If it's in reading_data.sources, it should also "have" this accelerometer. In > fact, I feel as though the has array is unnecessary at this point since I don't > think we need to quickly answer that question, but I don't feel strongly that it > needs to be removed. It's only other usage is int Initialize for confirming all indices bounds. We could get rid of them in the follow up when the two inits are separated out.
LGTM with nits. https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/190001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:444: if (!configuration_.has[source]) On 2015/08/27 22:08:08, jonross wrote: > On 2015/08/27 21:48:23, flackr wrote: > > If it's in reading_data.sources, it should also "have" this accelerometer. In > > fact, I feel as though the has array is unnecessary at this point since I > don't > > think we need to quickly answer that question, but I don't feel strongly that > it > > needs to be removed. > > It's only other usage is int Initialize for confirming all indices bounds. > > We could get rid of them in the follow up when the two inits are separated out. Can you make !configuration_.has[source] an error instead of silently skipped? Maybe CHECK or DCHECK? https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:65: // The axes on each accelerometer. nit: Add to comment that the order of axes was flipped on kernel 3.18+ so that it's obvious it's not a typo :-). https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:247: bool separate_devices = false; nit: s/separate_devices/legacy_cros_accel https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:372: int scale_divisor = 0; nit: Move declaration to for loop scope right before read of scale file.
Switched to the DCHECK, as it should not occur. https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... File chromeos/accelerometer/accelerometer_reader.cc (right): https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:65: // The axes on each accelerometer. On 2015/08/27 22:32:11, flackr wrote: > nit: Add to comment that the order of axes was flipped on kernel 3.18+ so that > it's obvious it's not a typo :-). Done. https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:247: bool separate_devices = false; On 2015/08/27 22:32:11, flackr wrote: > nit: s/separate_devices/legacy_cros_accel Done. https://codereview.chromium.org/1306453003/diff/210001/chromeos/accelerometer... chromeos/accelerometer/accelerometer_reader.cc:372: int scale_divisor = 0; On 2015/08/27 22:32:11, flackr wrote: > nit: Move declaration to for loop scope right before read of scale file. Done.
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org, derat@chromium.org, flackr@chromium.org Link to the patchset: https://codereview.chromium.org/1306453003/#ps230001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306453003/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306453003/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fixed the missing tests. Small tweak to SensorManageChromeOS to address the handling of X. Confirmed functionality matches spec & android using: http://www.html5rocks.com/en/tutorials/device/orientation/deviceorientationsa...
The CQ bit was checked by jonross@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, derat@chromium.org, timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/1306453003/#ps190002 (title: "Fix missed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306453003/190002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306453003/190002
Message was sent while issue was closed.
Committed patchset #12 (id:190002)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/01d453e2ae32b26bb9384853696db55b082302c4 Cr-Commit-Position: refs/heads/master@{#346148} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
