guberti commented on code in PR #13518:
URL: https://github.com/apache/tvm/pull/13518#discussion_r1037092340


##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -524,26 +531,39 @@ def _parse_connected_boards(self, tabular_str):
                 device[col_name] = str_row[column.start() : 
column.end()].strip()
             yield device
 
-    def _auto_detect_port(self, arduino_cli_cmd: str, board: str) -> str:
-        list_cmd = [self._get_arduino_cli_cmd(arduino_cli_cmd), "board", 
"list"]
-        list_cmd_output = subprocess.run(
-            list_cmd, check=True, stdout=subprocess.PIPE
-        ).stdout.decode("utf-8")
-
-        desired_fqbn = self._get_fqbn(board)
-        for device in self._parse_connected_boards(list_cmd_output):
-            if device["fqbn"] == desired_fqbn:
-                return device["port"]
+    def _auto_detect_port(self, arduino_cli_cmd: str, board: str, 
serial_number) -> str:

Review Comment:
   Type annotation for serial_number?



##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -524,26 +531,39 @@ def _parse_connected_boards(self, tabular_str):
                 device[col_name] = str_row[column.start() : 
column.end()].strip()
             yield device
 
-    def _auto_detect_port(self, arduino_cli_cmd: str, board: str) -> str:
-        list_cmd = [self._get_arduino_cli_cmd(arduino_cli_cmd), "board", 
"list"]
-        list_cmd_output = subprocess.run(
-            list_cmd, check=True, stdout=subprocess.PIPE
-        ).stdout.decode("utf-8")
-
-        desired_fqbn = self._get_fqbn(board)
-        for device in self._parse_connected_boards(list_cmd_output):
-            if device["fqbn"] == desired_fqbn:
-                return device["port"]
+    def _auto_detect_port(self, arduino_cli_cmd: str, board: str, 
serial_number) -> str:
+        if not serial_number:
+            # If serial_number is not set, it is assumed only one board
+            # with this type is connected to this host machine.
+            list_cmd = [self._get_arduino_cli_cmd(arduino_cli_cmd), "board", 
"list"]
+            list_cmd_output = subprocess.run(
+                list_cmd, check=True, stdout=subprocess.PIPE
+            ).stdout.decode("utf-8")
+
+            desired_fqbn = self._get_fqbn(board)
+            for device in self._parse_connected_boards(list_cmd_output):
+                if device["fqbn"] == desired_fqbn:
+                    device_port = device["port"]
+                    break
+        else:
+            com_ports = serial.tools.list_ports.comports()
+            for port in com_ports:
+                if port.serial_number == serial_number:
+                    device_port = port.device
 
+        if device_port:
+            return device_port
         # If no compatible boards, raise an error
         raise BoardAutodetectFailed()
 
-    def _get_arduino_port(self, arduino_cli_cmd: str, board: str, port: int):
+    def _get_arduino_port(self, arduino_cli_cmd: str, board: str, port: int, 
serial_number: str):

Review Comment:
   Update type annotations to reflect that port and serial_number are optional.



##########
python/tvm/micro/testing/pytest_plugin.py:
##########
@@ -60,6 +60,11 @@ def pytest_addoption(parser):
             "Also, it will enable debug level logging in project generation."
         ),
     )
+    parser.addoption(
+        "--serial",

Review Comment:
   I'd prefer calling this `--serial_number` - we call it `serial_number` 
everywhere else in the code, and "serial" has other meanings that could be 
confusing. 



##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -524,26 +531,39 @@ def _parse_connected_boards(self, tabular_str):
                 device[col_name] = str_row[column.start() : 
column.end()].strip()
             yield device
 
-    def _auto_detect_port(self, arduino_cli_cmd: str, board: str) -> str:
-        list_cmd = [self._get_arduino_cli_cmd(arduino_cli_cmd), "board", 
"list"]
-        list_cmd_output = subprocess.run(
-            list_cmd, check=True, stdout=subprocess.PIPE
-        ).stdout.decode("utf-8")
-
-        desired_fqbn = self._get_fqbn(board)
-        for device in self._parse_connected_boards(list_cmd_output):
-            if device["fqbn"] == desired_fqbn:
-                return device["port"]
+    def _auto_detect_port(self, arduino_cli_cmd: str, board: str, 
serial_number) -> str:
+        if not serial_number:
+            # If serial_number is not set, it is assumed only one board
+            # with this type is connected to this host machine.
+            list_cmd = [self._get_arduino_cli_cmd(arduino_cli_cmd), "board", 
"list"]
+            list_cmd_output = subprocess.run(
+                list_cmd, check=True, stdout=subprocess.PIPE
+            ).stdout.decode("utf-8")
+
+            desired_fqbn = self._get_fqbn(board)
+            for device in self._parse_connected_boards(list_cmd_output):
+                if device["fqbn"] == desired_fqbn:
+                    device_port = device["port"]
+                    break
+        else:
+            com_ports = serial.tools.list_ports.comports()
+            for port in com_ports:
+                if port.serial_number == serial_number:
+                    device_port = port.device
 
+        if device_port:
+            return device_port
         # If no compatible boards, raise an error
         raise BoardAutodetectFailed()
 
-    def _get_arduino_port(self, arduino_cli_cmd: str, board: str, port: int):
+    def _get_arduino_port(self, arduino_cli_cmd: str, board: str, port: int, 
serial_number: str):
         if not self._port:
             if port:
                 self._port = port
             else:
-                self._port = self._auto_detect_port(arduino_cli_cmd, board)
+                self._port = self._auto_detect_port(
+                    arduino_cli_cmd, board, serial_number=serial_number

Review Comment:
   ```suggestion
                       arduino_cli_cmd, board, serial_number
   ```



##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -93,6 +93,13 @@ class BoardAutodetectFailed(Exception):
         default=None,
         help="Port to use for connecting to hardware.",
     ),
+    server.ProjectOption(
+        "serial_number",
+        optional=["open_transport", "flash"],
+        type="str",
+        default=None,
+        help=("Board serial number."),
+    ),

Review Comment:
   Can we clarify in the `help` strings how the `port` and `serial_number` 
options interact? What happens if we pass both options or neither?



##########
tests/micro/arduino/test_arduino_rpc_server.py:
##########
@@ -56,30 +59,50 @@ def _make_session(model, arduino_board, arduino_cli_cmd, 
workspace_dir, mod, bui
 
 
 def _make_sess_from_op(
-    model, arduino_board, arduino_cli_cmd, workspace_dir, op_name, sched, 
arg_bufs, build_config
+    model,
+    arduino_board,
+    arduino_cli_cmd,
+    workspace_dir,
+    op_name,
+    sched,
+    arg_bufs,
+    build_config,
+    serial_number: str,

Review Comment:
   Fix type annotation to reflect that `serial_number` is optional. This 
happens in a few other tests as well.



##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -560,17 +580,21 @@ def _get_board_from_makefile(self, makefile_path: 
pathlib.Path) -> str:
     FLASH_MAX_RETRIES = 5
 
     def flash(self, options):
+        import serial.tools.list_ports

Review Comment:
   Add a comment explaining why we can't have this at the top with the rest of 
the imports?



##########
python/tvm/micro/testing/pytest_plugin.py:
##########
@@ -60,6 +60,11 @@ def pytest_addoption(parser):
             "Also, it will enable debug level logging in project generation."
         ),
     )
+    parser.addoption(
+        "--serial",
+        default=None,
+        help="If set true, use the FVP emulator to run the test",

Review Comment:
   Fix `help` string?



##########
python/tvm/micro/testing/pytest_plugin.py:
##########
@@ -130,3 +135,8 @@ def pytest_configure(config):
         "markers",
         "skip_boards(board): skip test for the given board",
     )
+
+

Review Comment:
   We should also update `tests/micro/arduino/README.md` to mention that you 
can pass in a serial number.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to