Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1823193002:
add GetBattorPathFromPhoneSerial to find_usb_devices (Closed)
|
Created:
4 years, 9 months ago by alexandermont Modified:
4 years, 8 months ago CC:
catapult-reviews_chromium.org Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionAdd function GetBattorPathFromPhoneSerial() to find_usb_devices.
Adds a function GetBattorPathFromPhoneSerial() which takes a serial number of a phone and returns that TTY path (e.g. /dev/ttyUSB0) that is used to communicate with the BattOr corresponding to that port. This is necessary for tracing agents to get battery traces from the BattOr. See, e.g. go/battor-systrace, go/clock-sync-design.
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/58dc3f59eca9a1f2a3e38d51aee8f3d3bb3bec5a
Patch Set 1 #
Total comments: 9
Patch Set 2 : changes from code review #Patch Set 3 : add ability to specify explicit serial map #
Total comments: 2
Patch Set 4 : update docs #
Total comments: 20
Patch Set 5 : add new feature to add a fixed mapping #
Total comments: 17
Patch Set 6 : simplify api #Patch Set 7 : add standalone script for update #
Total comments: 16
Patch Set 8 : changes from code review #
Total comments: 2
Patch Set 9 : add named class #
Total comments: 14
Patch Set 10 : updates from code review #
Total comments: 2
Patch Set 11 : fix nits #
Total comments: 1
Patch Set 12 : fix test #
Total comments: 4
Patch Set 13 : fix test #
Messages
Total messages: 60 (15 generated)
alexandermont@chromium.org changed reviewers: + jbudorick@chromium.org, rnephew@google.com
rnephew@chromium.org changed reviewers: + rnephew@chromium.org
https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/battor_ma... File devil/devil/utils/battor_mapping_test.py (right): https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/battor_ma... devil/devil/utils/battor_mapping_test.py:1: #!/usr/bin/env python Is it possible for these tests to just live in find_usb_devices_test.py? https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:10: from devil.utils import cmd_helper, usb_hubs, lsusb I do not think we do a whole lot of multiple imports per line. I think these should be separated to keep consistency with the rest of the codebase. https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:531: hub_types = ['plugable_7port'] #default: use plugable 7-port hub nit: Space between # and comment. https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:539: if serial: I think it would be more readable if it was written as if not serial and len(all_battors) > 1: raise BattorError('Two or more BattOrs connected, no serial provided') then just get rid of the else clause all together and move everything else up one indentation level. Just my opinion though, so if you and/or other feel otherwise feel free to ignore this. https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:596: print GetBattorPathFromPhoneSerial('test') Guessing this is leftover testing.
alexandermont@chromium.org changed reviewers: - rnephew@google.com
https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:10: from devil.utils import cmd_helper, usb_hubs, lsusb On 2016/03/22 at 18:43:14, rnephew1 wrote: > I do not think we do a whole lot of multiple imports per line. I think these should be separated to keep consistency with the rest of the codebase. Done https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:531: hub_types = ['plugable_7port'] #default: use plugable 7-port hub On 2016/03/22 at 18:43:14, rnephew1 wrote: > nit: Space between # and comment. Done https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:539: if serial: On 2016/03/22 at 18:43:14, rnephew1 wrote: > I think it would be more readable if it was written as > > if not serial and len(all_battors) > 1: > raise BattorError('Two or more BattOrs connected, no serial provided') > > then just get rid of the else clause all together and move everything else up one indentation level. > > Just my opinion though, so if you and/or other feel otherwise feel free to ignore this. Done https://codereview.chromium.org/1823193002/diff/1/devil/devil/utils/find_usb_... devil/devil/utils/find_usb_devices.py:596: print GetBattorPathFromPhoneSerial('test') On 2016/03/22 at 18:43:14, rnephew1 wrote: > Guessing this is leftover testing. Removed
alexandermont@chromium.org changed reviewers: + nednguyen@google.com
alexandermont@chromium.org changed reviewers: + zhenw@chromium.org
https://codereview.chromium.org/1823193002/diff/40001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/40001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:535: def GetBattorPathFromPhoneSerial(serial, hub_types=None, serial_map=None): So this is the interface we are going to use in battor_tracing_agent, right? I am ok with this. For the details, other reviewers may be better than me. By the way, serial_map should be before hub_types, because hub_types depend on whether serial_map is available. i.e. GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None) And in the comments, it should be: 1. if serial_map is available ... 2. if serial_map is None and hub_types is available ... 3. if serial_map is None and hub_types is None ... This sequence is more consistent with the logic in the code and it is easier to understand.
https://codereview.chromium.org/1823193002/diff/40001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/40001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:535: def GetBattorPathFromPhoneSerial(serial, hub_types=None, serial_map=None): On 2016/03/22 at 23:51:37, Zhen Wang wrote: > So this is the interface we are going to use in battor_tracing_agent, right? I am ok with this. For the details, other reviewers may be better than me. > > By the way, serial_map should be before hub_types, because hub_types depend on whether serial_map is available. i.e. > GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None) > > And in the comments, it should be: > 1. if serial_map is available ... > 2. if serial_map is None and hub_types is available ... > 3. if serial_map is None and hub_types is None ... > > This sequence is more consistent with the logic in the code and it is easier to understand. Done
I defer to John & Randy for reviewing this https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:638: Clients are intended to call the functions in this script directly. Then why even bother supporting this as a command line script?
Description was changed from ========== add GetBattorPathFromPhoneSerial to find_usb_devices ========== to ========== Add function GetBattorPathFromPhoneSerial() to find_usb_devices. Adds a function GetBattorPathFromPhoneSerial() which takes a serial number of a phone and returns that TTY path (e.g. /dev/ttyUSB0) that is used to communicate with the BattOr corresponding to that port. This is necessary for tracing agents to get battery traces from the BattOr. See, e.g. go/battor-systrace, go/clock-sync-design. ==========
On 2016/03/24 at 02:04:21, nednguyen wrote: > I defer to John & Randy for reviewing this > > https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... > File devil/devil/utils/find_usb_devices.py (right): > > https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... > devil/devil/utils/find_usb_devices.py:638: Clients are intended to call the functions in this script directly. > Then why even bother supporting this as a command line script? That is mainly there for testing purposes.
https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:57: class BattorError(Exception): This should at least inherit from BaseError: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:517: """Parses a file consisting of a list of lines: What determines the format of this map file? Do we have control over it? If so, it should be a more structured data format, e.g. json. https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:575: if serial_map is not None: Can you split these two different cases into nonpublic helper functions that get called by GetBattorPathFromPhoneSerial? https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:594: hub_types = [usb_hubs.GetHubType(x) for x in hub_types] What happens if hub_types is []? Depending on the answer to that, you might be able to combine this line and the two above into: hub_types = [usb_hubs.GetHubType(x) for x in (hub_types or ['plugable_7port']) https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:595: p2serial = GetAllPhysicalPortToSerialMaps(hub_types, You're essentially validating both of these maps below. Should that happen within these functions instead? https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:605: if port_num != -1: TBH, I don't think this is a case you need to worry about. _a lot_ of things will break if this happens, starting with adb. https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:615: if port_num in hub: Restructure this using hub.get(port_num) https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/test_... File devil/devil/utils/test_serial_map.txt (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/test_... devil/devil/utils/test_serial_map.txt:1: Phone1 Battor1 this either: - should be in its own directory somewhere, or - should be written as a temporary file by the test https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/usb_h... File devil/devil/utils/usb_hubs.py (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/usb_h... devil/devil/utils/usb_hubs.py:92: plugable_7port = HubType(_is_plugable_7port_hub, PLUGABLE_7PORT_LAYOUT) PLUGABLE_7PORT
https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:517: """Parses a file consisting of a list of lines: On 2016/03/24 17:34:09, jbudorick wrote: > What determines the format of this map file? Do we have control over it? If so, > it should be a more structured data format, e.g. json. +1 for this. It also might be useful for this to be able to generate a serial map file for lab architecture as well. That would allow us to store this much like we do the known_devices and device_blacklist and speed up execution time since it wouldn't have to run every time.
https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:57: class BattorError(Exception): On 2016/03/24 at 17:34:09, jbudorick wrote: > This should at least inherit from BaseError: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:517: """Parses a file consisting of a list of lines: On 2016/03/24 at 19:10:21, rnephew1 wrote: > On 2016/03/24 17:34:09, jbudorick wrote: > > What determines the format of this map file? Do we have control over it? If so, > > it should be a more structured data format, e.g. json. > > +1 for this. > > It also might be useful for this to be able to generate a serial map file for lab architecture as well. That would allow us to store this much like we do the known_devices and device_blacklist and speed up execution time since it wouldn't have to run every time. Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:575: if serial_map is not None: On 2016/03/24 at 17:34:09, jbudorick wrote: > Can you split these two different cases into nonpublic helper functions that get called by GetBattorPathFromPhoneSerial? Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:594: hub_types = [usb_hubs.GetHubType(x) for x in hub_types] On 2016/03/24 at 17:34:10, jbudorick wrote: > What happens if hub_types is []? > > Depending on the answer to that, you might be able to combine this line and the two above into: > > hub_types = [usb_hubs.GetHubType(x) for x in (hub_types or ['plugable_7port']) Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:595: p2serial = GetAllPhysicalPortToSerialMaps(hub_types, On 2016/03/24 at 17:34:10, jbudorick wrote: > You're essentially validating both of these maps below. Should that happen within these functions instead? The possible error checks don't really represent things wrong with the maps themselves. "Two devices with same serial number" - removed, don't need to worry about this case, see comment above. "Device with given serial number not found" - this isn't a problem with the map, it just means that the serial number passed into GetBattorPathFromPhoneSerial was not found. "Two TTY devices with matching port number" - this is still a valid map, since a user could plug in two BattOrs, one on port 1 of one hub and one on port 1 of another hub. It's just that if the user then plugged in a phone on port 1 of a third hub, it wouldn't know which of the two BattOrs the phone goes with. https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:605: if port_num != -1: On 2016/03/24 at 17:34:09, jbudorick wrote: > TBH, I don't think this is a case you need to worry about. _a lot_ of things will break if this happens, starting with adb. Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:615: if port_num in hub: On 2016/03/24 at 17:34:10, jbudorick wrote: > Restructure this using hub.get(port_num) Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/test_... File devil/devil/utils/test_serial_map.txt (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/test_... devil/devil/utils/test_serial_map.txt:1: Phone1 Battor1 On 2016/03/24 at 17:34:10, jbudorick wrote: > this either: > - should be in its own directory somewhere, or > - should be written as a temporary file by the test Done https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/usb_h... File devil/devil/utils/usb_hubs.py (right): https://codereview.chromium.org/1823193002/diff/60001/devil/devil/utils/usb_h... devil/devil/utils/usb_hubs.py:92: plugable_7port = HubType(_is_plugable_7port_hub, PLUGABLE_7PORT_LAYOUT) On 2016/03/24 at 17:34:10, jbudorick wrote: > PLUGABLE_7PORT Done
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/usb_h... File devil/devil/utils/usb_hubs.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/usb_h... devil/devil/utils/usb_hubs.py:80: def _is_plugable_7port_hub(node): Note that I had to change this from what it was before, because in my testing, the description string that lsusb gives for that hub was different than what it was during my testing earlier. This might be due to some update to lsusb or to the hub's firmware. I have uploaded this CL now so you can look at the rest of it, but I will look into this to see if there is a more reliable way of identifying the USB hubs.
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:267: def GetBusNumberToDeviceTreeMap(fast=False): Since everywhere seems to use fast=True, should that be the default? https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:535: [{'phone':<phone serial 1>, 'battor':<battor serial 1>}, Nit: Spaces here too. See below. https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:562: result.append({'phone':phone, 'battor':battor}) nit: 'phone': phone, 'battor': battor note the spaces. https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:570: [{'phone':<phone serial 1>, 'battor':<battor serial 1>}, Spaces here too.
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:651: def GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None): In which use case a client will not want to use serial_map but use "hub_types" instead? I usually advocate for simpler API, so unless there is a use case which people can't use serial_map and have to use hub_types, I would say deprecating hub_types to simplify your design.
On 2016/03/25 03:05:29, nednguyen wrote: > https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... > File devil/devil/utils/find_usb_devices.py (right): > > https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... > devil/devil/utils/find_usb_devices.py:651: def > GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None): > In which use case a client will not want to use serial_map but use "hub_types" > instead? I usually advocate for simpler API, so unless there is a use case which > people can't use serial_map and have to use hub_types, I would say deprecating > hub_types to simplify your design. I think the plan is to have the bots use hub_types if the serial_map is not found on the bots. This would happen if the people in the labs switch devices and delete the mapping file.
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:694: nit: no space here. https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:698: if not tty_string: space here instead.
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:651: def GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None): > I think the plan is to have the bots use hub_types if the serial_map is not > found on the bots. This would happen if the people in the labs switch devices > and delete the mapping file. Thinking more about it, that would most likely happen outside of this function.
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:651: def GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None): On 2016/03/25 05:03:19, rnephew1 wrote: > > I think the plan is to have the bots use hub_types if the serial_map is not > > found on the bots. This would happen if the people in the labs switch devices > > and delete the mapping file. > > Thinking more about it, that would most likely happen outside of this function. I see. Can we leave the hub_types param out and instead have the callsites translate hubtypes to serial_map themselves? That's way, it's less ambiguous about what this function will do when both serial_map & hub_types are present.
On 2016/03/24 at 23:04:22, alexandermont wrote: > https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/usb_h... > File devil/devil/utils/usb_hubs.py (right): > > https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/usb_h... > devil/devil/utils/usb_hubs.py:80: def _is_plugable_7port_hub(node): > Note that I had to change this from what it was before, because in my testing, the description string that lsusb gives for that hub was different than what it was during my testing earlier. This might be due to some update to lsusb or to the hub's firmware. I have uploaded this CL now so you can look at the rest of it, but I will look into this to see if there is a more reliable way of identifying the USB hubs. It appears that the ID number (1a40:0101) is stable. I am updating the function to check for the ID number.
https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:535: [{'phone':<phone serial 1>, 'battor':<battor serial 1>}, On 2016/03/25 at 00:17:53, rnephew1 wrote: > Nit: Spaces here too. See below. Done https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:562: result.append({'phone':phone, 'battor':battor}) On 2016/03/25 at 00:17:53, rnephew1 wrote: > nit: 'phone': phone, 'battor': battor > > note the spaces. Done https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:570: [{'phone':<phone serial 1>, 'battor':<battor serial 1>}, On 2016/03/25 at 00:17:52, rnephew1 wrote: > Spaces here too. Done https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:651: def GetBattorPathFromPhoneSerial(serial, serial_map=None, hub_types=None): On 2016/03/27 at 16:44:32, nednguyen wrote: > On 2016/03/25 05:03:19, rnephew1 wrote: > > > I think the plan is to have the bots use hub_types if the serial_map is not > > > found on the bots. This would happen if the people in the labs switch devices > > > and delete the mapping file. > > > > Thinking more about it, that would most likely happen outside of this function. > > I see. Can we leave the hub_types param out and instead have the callsites translate hubtypes to serial_map themselves? That's way, it's less ambiguous about what this function will do when both serial_map & hub_types are present. Done https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:694: On 2016/03/25 at 05:02:41, rnephew1 wrote: > nit: no space here. Done https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/find_... devil/devil/utils/find_usb_devices.py:698: if not tty_string: On 2016/03/25 at 05:02:41, rnephew1 wrote: > space here instead. Done https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/usb_h... File devil/devil/utils/usb_hubs.py (right): https://codereview.chromium.org/1823193002/diff/80001/devil/devil/utils/usb_h... devil/devil/utils/usb_hubs.py:80: def _is_plugable_7port_hub(node): On 2016/03/24 at 23:04:22, alexandermont wrote: > Note that I had to change this from what it was before, because in my testing, the description string that lsusb gives for that hub was different than what it was during my testing earlier. This might be due to some update to lsusb or to the hub's firmware. I have uploaded this CL now so you can look at the rest of it, but I will look into this to see if there is a more reliable way of identifying the USB hubs. Done. Now using ID number.
lgtm from my side. devil folks, can you review this?
lgtm with one question. See if John wants to go over it one last time before commiting. https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/upda... File devil/devil/utils/update_mapping.py (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/upda... devil/devil/utils/update_mapping.py:19: desc = 'Example: ./update_mapping.py -o mapping.json' Should the description also include a description of what the script does?
https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:531: def ReadSerialMapFile(filename): I should've brought this up on whatever review introduced the BattOr utility functions above, but better late than never: this module contains a mix of generic USB logic and highly specific BattOr logic. Can we move the BattOr logic into its own module? https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:585: hub_types: List of hub types to check for nit: add that this defaults to ['plugable_7port'] https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:591: p2serial = GetAllPhysicalPortToSerialMaps(hub_types, nit: rename p2serial https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:593: port_to_devices = collections.defaultdict(list) Rather than a list of serials, wdyt about having the values in this dict be a collections.namedtuple with two string fields, phone_serial and battor_serial? https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:597: port_to_devices[port].append(serial) Following from the above, you'd then check whether the serial is a battor serial here. If so, you'd set the serial as the battor_serial if one hasn't already be sent (and raise a BattorError for duplicate battor serial numbers if it has). If not, you'd set the serial as the phone_serial if one hasn't been set (and raise a BattorError for multiple phones if it has). You'd then go through port_to_devices and, for each entry with a valid battor_serial, you'd: - ensure that phone_serial is not None and raise a BattorError if not - set result[phone_serial] = battor_serial I think that'd be more clear than what you have here. https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:633: (1) If serial_map is a dict, serial_map is treated as a dictionary mapping Why is this handling two arguments in one...? Handle them separately, i.e., have serial_map and serial_map_file (or revise the call sites s.t. this only receives one or the other). https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/test... File devil/devil/utils/testdata/test_serial_map.json (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/test... devil/devil/utils/testdata/test_serial_map.json:1: [{"phone": "Phone1", "battor": "Battor1"}, {"phone": "Phone2", "battor": "Battor2"}, {"phone": "Phone3", "battor": "Battor3"}] nit: devil/utils/test/data/test_serial_map.json https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/usb_... File devil/devil/utils/usb_hubs.py (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/usb_... devil/devil/utils/usb_hubs.py:1: # Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c), 2016
https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices.py (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:585: hub_types: List of hub types to check for On 2016/03/30 at 19:28:58, jbudorick wrote: > nit: add that this defaults to ['plugable_7port'] Done https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:591: p2serial = GetAllPhysicalPortToSerialMaps(hub_types, On 2016/03/30 at 19:28:58, jbudorick wrote: > nit: rename p2serial Done https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:593: port_to_devices = collections.defaultdict(list) On 2016/03/30 at 19:28:57, jbudorick wrote: > Rather than a list of serials, wdyt about having the values in this dict be a collections.namedtuple with two string fields, phone_serial and battor_serial? See below. https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:597: port_to_devices[port].append(serial) On 2016/03/30 at 19:28:58, jbudorick wrote: > Following from the above, you'd then check whether the serial is a battor serial here. If so, you'd set the serial as the battor_serial if one hasn't already be sent (and raise a BattorError for duplicate battor serial numbers if it has). If not, you'd set the serial as the phone_serial if one hasn't been set (and raise a BattorError for multiple phones if it has). > > You'd then go through port_to_devices and, for each entry with a valid battor_serial, you'd: > - ensure that phone_serial is not None and raise a BattorError if not > - set result[phone_serial] = battor_serial > > I think that'd be more clear than what you have here. This won't work if the value in the dictionary is a namedtuple, since a namedtuple is immutable (so you couldn't set the phone_serial once, and the battor_serial a different time). But you could do this with a list of length 2. Note that it is possible to have a mutable tuple if you use a specialized class (e.g. recordclass) but it is probably not worth introducing an additional dependency just for this. I have also put more comments to try to explain this function better. https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/find... devil/devil/utils/find_usb_devices.py:633: (1) If serial_map is a dict, serial_map is treated as a dictionary mapping On 2016/03/30 at 19:28:57, jbudorick wrote: > Why is this handling two arguments in one...? Handle them separately, i.e., have serial_map and serial_map_file (or revise the call sites s.t. this only receives one or the other). Done https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/test... File devil/devil/utils/testdata/test_serial_map.json (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/test... devil/devil/utils/testdata/test_serial_map.json:1: [{"phone": "Phone1", "battor": "Battor1"}, {"phone": "Phone2", "battor": "Battor2"}, {"phone": "Phone3", "battor": "Battor3"}] On 2016/03/30 at 19:28:58, jbudorick wrote: > nit: devil/utils/test/data/test_serial_map.json Done https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/upda... File devil/devil/utils/update_mapping.py (right): https://codereview.chromium.org/1823193002/diff/120001/devil/devil/utils/upda... devil/devil/utils/update_mapping.py:19: desc = 'Example: ./update_mapping.py -o mapping.json' On 2016/03/30 at 14:39:54, rnephew1 wrote: > Should the description also include a description of what the script does? Done
https://codereview.chromium.org/1823193002/diff/140001/devil/devil/utils/batt... File devil/devil/utils/battor_device_mapping.py (right): https://codereview.chromium.org/1823193002/diff/140001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:109: port_to_devices = collections.defaultdict(lambda: [None, None]) A list really isn't clear here. [0] and [1] convey nothing. I suggested a namedtuple for clarity (and because I'd forgotten that the values were immutable), but since that won't work, the value here should be either a dict or a custom object with named fields.
https://codereview.chromium.org/1823193002/diff/140001/devil/devil/utils/batt... File devil/devil/utils/battor_device_mapping.py (right): https://codereview.chromium.org/1823193002/diff/140001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:109: port_to_devices = collections.defaultdict(lambda: [None, None]) On 2016/03/30 at 23:40:15, jbudorick wrote: > A list really isn't clear here. [0] and [1] convey nothing. I suggested a namedtuple for clarity (and because I'd forgotten that the values were immutable), but since that won't work, the value here should be either a dict or a custom object with named fields. Done
https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... File devil/devil/utils/battor_device_mapping.py (right): https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:21: return '0403:6001' in node.desc This holds for all BattOrs? This logic changed from earlier this week... https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:22: nit: two blank lines between functions at module scope https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:132: result[pair.phone] = pair.battor Wasn't this going to do some error checking for None values of either pair.phone or pair.battor? https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:135: def _PhoneToPathMap(serial, serial_map, devtree): Are callers ok with this returning None if it doesn't find a node matching the battor serial? https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:196: if not serial: nit: move this check before the serial_map and serial_map_file handling. https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices_test.py (right): https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:36: from devil.utils import battor_device_mapping nit: alphabetize https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/upda... File devil/devil/utils/update_mapping.py (right): https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/upda... devil/devil/utils/update_mapping.py:33: 'Supported types:' Would choices be helpful here? https://docs.python.org/3/library/argparse.html#choices
https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... File devil/devil/utils/battor_device_mapping.py (right): https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:21: return '0403:6001' in node.desc On 2016/03/31 at 20:57:31, jbudorick wrote: > This holds for all BattOrs? This logic changed from earlier this week... Yes. I did test this with several different BattOrs. The "ID" here is a vendor ID (the 4 hexdigits before the : ) and a prodoct ID (the 4 hexdigits after the : ). The reason I changed it to look at the ID rather than the rest of the string is that I found out that the ID number is stable but the rest of the string is not necessarily stable. https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:22: On 2016/03/31 at 20:57:31, jbudorick wrote: > nit: two blank lines between functions at module scope Done https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:132: result[pair.phone] = pair.battor On 2016/03/31 at 20:57:31, jbudorick wrote: > Wasn't this going to do some error checking for None values of either pair.phone or pair.battor? Done. https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:135: def _PhoneToPathMap(serial, serial_map, devtree): On 2016/03/31 at 20:57:31, jbudorick wrote: > Are callers ok with this returning None if it doesn't find a node matching the battor serial? Yes. The only caller is GetBattorPathFromPhoneSerial, which checks the output of this function and raises a BattorError if it is None. https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/batt... devil/devil/utils/battor_device_mapping.py:196: if not serial: On 2016/03/31 at 20:57:31, jbudorick wrote: > nit: move this check before the serial_map and serial_map_file handling. Done https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices_test.py (right): https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:36: from devil.utils import battor_device_mapping On 2016/03/31 at 20:57:31, jbudorick wrote: > nit: alphabetize Done https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/upda... File devil/devil/utils/update_mapping.py (right): https://codereview.chromium.org/1823193002/diff/160001/devil/devil/utils/upda... devil/devil/utils/update_mapping.py:33: 'Supported types:' On 2016/03/31 at 20:57:31, jbudorick wrote: > Would choices be helpful here? https://docs.python.org/3/library/argparse.html#choices Done
lgtm w/ nit https://codereview.chromium.org/1823193002/diff/180001/devil/devil/utils/upda... File devil/devil/utils/update_mapping.py (right): https://codereview.chromium.org/1823193002/diff/180001/devil/devil/utils/upda... devil/devil/utils/update_mapping.py:32: action='store', choices=['plugable_7port'], nit: this should be '--hub', action='append', dest='hub_types'. You then won't need to split the hub_types on commas below.
https://codereview.chromium.org/1823193002/diff/180001/devil/devil/utils/upda... File devil/devil/utils/update_mapping.py (right): https://codereview.chromium.org/1823193002/diff/180001/devil/devil/utils/upda... devil/devil/utils/update_mapping.py:32: action='store', choices=['plugable_7port'], On 2016/04/01 at 17:43:36, jbudorick wrote: > nit: this should be '--hub', action='append', dest='hub_types'. You then won't need to split the hub_types on commas below. Done
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, rnephew@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1823193002/#ps200001 (title: "fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823193002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823193002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
https://codereview.chromium.org/1823193002/diff/200001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices_test.py (right): https://codereview.chromium.org/1823193002/diff/200001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:307: 'test/data/test_serial_map.json') Might need to do something like: os.path.abspath(os.path.join(os.path.dirname(__file__), 'test', 'data', 'test_serial_map.json))
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, jbudorick@chromium.org, rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/1823193002/#ps220001 (title: "fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823193002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823193002/220001
The CQ bit was unchecked by rnephew@chromium.org
https://codereview.chromium.org/1823193002/diff/220001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices_test.py (right): https://codereview.chromium.org/1823193002/diff/220001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:307: curr_dir = os.path.dirname(os.path.realpath(__file__)) One of two things should happen, either you make this variable the entire path: test_data_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'test', 'data', 'test_serial_map.json') https://codereview.chromium.org/1823193002/diff/220001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:309: curr_dir+'/test/data/test_serial_map.json') or make this: os.path.join(curr_dir, 'test', 'data', 'test_serial_map.json')
The CQ bit was checked by alexandermont@chromium.org
https://codereview.chromium.org/1823193002/diff/220001/devil/devil/utils/find... File devil/devil/utils/find_usb_devices_test.py (right): https://codereview.chromium.org/1823193002/diff/220001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:307: curr_dir = os.path.dirname(os.path.realpath(__file__)) On 2016/04/01 at 20:51:18, rnephew1 wrote: > One of two things should happen, either you make this variable the entire path: > test_data_path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'test', 'data', 'test_serial_map.json') Done (the way below) https://codereview.chromium.org/1823193002/diff/220001/devil/devil/utils/find... devil/devil/utils/find_usb_devices_test.py:309: curr_dir+'/test/data/test_serial_map.json') On 2016/04/01 at 20:51:18, rnephew1 wrote: > or make this: > os.path.join(curr_dir, 'test', 'data', 'test_serial_map.json') Done
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, jbudorick@chromium.org, rnephew@chromium.org Link to the patchset: https://codereview.chromium.org/1823193002/#ps240001 (title: "fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1823193002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1823193002/240001
Message was sent while issue was closed.
Description was changed from ========== Add function GetBattorPathFromPhoneSerial() to find_usb_devices. Adds a function GetBattorPathFromPhoneSerial() which takes a serial number of a phone and returns that TTY path (e.g. /dev/ttyUSB0) that is used to communicate with the BattOr corresponding to that port. This is necessary for tracing agents to get battery traces from the BattOr. See, e.g. go/battor-systrace, go/clock-sync-design. ========== to ========== Add function GetBattorPathFromPhoneSerial() to find_usb_devices. Adds a function GetBattorPathFromPhoneSerial() which takes a serial number of a phone and returns that TTY path (e.g. /dev/ttyUSB0) that is used to communicate with the BattOr corresponding to that port. This is necessary for tracing agents to get battery traces from the BattOr. See, e.g. go/battor-systrace, go/clock-sync-design. Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
