[microTVM][Arduino]Add serial_number to project options and tests
#13518
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
|
cc @guberti for review |
There was a problem hiding this comment.
Added a few general comments, but I can't get this PR working. I see this error:
guberti@guberti-Precision-7540:~/tvm$ python -m pytest -s tests/micro/arduino/test_arduino_rpc_server.py::test_compile_runtime[due] --board due --serial 123
...lines omitted for brevity...
E File "/home/guberti/tvm/tests/micro/arduino/workspace_due/2022-12-01T05-46-57/project/microtvm_api_server.py", line 597, in flash
E port = self._get_arduino_port(cli_command, board, port, serial_number)
E File "/home/guberti/tvm/tests/micro/arduino/workspace_due/2022-12-01T05-46-57/project/microtvm_api_server.py", line 564, in _get_arduino_port
E self._port = self._auto_detect_port(
E File "/home/guberti/tvm/tests/micro/arduino/workspace_due/2022-12-01T05-46-57/project/microtvm_api_server.py", line 549, in _auto_detect_port
E com_ports = serial.tools.list_ports.comports()
E NameError: name 'serial' is not defined
python/tvm/micro/project_api/client.py:135: ServerError
=========================== short test summary info ============================
FAILED tests/micro/arduino/test_arduino_rpc_server.py::test_compile_runtime[due]
========================= 1 failed, 1 warning in 2.63s =========================
I think moving import serial.tools.list_ports from flash to _auto_detect_port will fix the issue.
| # 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): |
There was a problem hiding this comment.
Update type annotations to reflect that port and serial_number are optional.
| # TODO: This is to avoid breaking GPU docker on running the tutorials. | ||
| import serial | ||
| import serial.tools.list_ports |
There was a problem hiding this comment.
Can you clarify this comment a bit? I don't understand how this fixes the GPU docker on tutorials.
There was a problem hiding this comment.
When I moved these imports to the top of the file, there was an error with doc stage in GPU. I think the reason is that we don't have dependencies for this in GPU docker. cc @driazati
There was a problem hiding this comment.
if this file is imported but this function isn't run during the tutorial docs build then it would fail, this fixes it with a lazy import. it would be nice to link to an issue about this though for more context
There was a problem hiding this comment.
@driazati I remember there was some discussion about running tutorials in the related docker and only render the results in GPU/Doc stage. Did that happen?
There was a problem hiding this comment.
No we never implemented anything like that, though here it might be easiest to just install pyserial in the GPU docker image and call it a day
| if device_port: | ||
| return device_port | ||
| # If no compatible boards, raise an error | ||
| raise BoardAutodetectFailed() |
There was a problem hiding this comment.
I think this logic is bugged. If I pass an invalid serial_number, I get:
E File "/home/guberti/tvm/tests/micro/arduino/workspace_due/2022-12-01T16-09-00/project/microtvm_api_server.py", line 560, in _auto_detect_port
E if device_port:
E UnboundLocalError: local variable 'device_port' referenced before assignment
This makes sense, as device_port is never defined if device["fqbn"] == desired_fqbn.
There was a problem hiding this comment.
this was an issue. I changed _get_arduino_port function to address this issue. Now in this function we have this priority:
- port
- detect port using serial_number
- auto detect port with arduino-cli
In 2 and 3 we show proper exception if it was not able to find the port. Also I changed project option help messages to include this change.
Also if both port and serial_number are set it will show exception
There was a problem hiding this comment.
hm but isn't device_port only set if we find a valid port?
There was a problem hiding this comment.
yeah. Are you suggesting to just return it in the for loop?
| if device_port: | ||
| return device_port | ||
| # If no compatible boards, raise an error | ||
| raise BoardAutodetectFailed() |
There was a problem hiding this comment.
hm but isn't device_port only set if we find a valid port?
tests/micro/arduino/README.md
Outdated
| @@ -33,3 +33,9 @@ To see the list of supported values for `--board`, run: | |||
| ``` | |||
| $ pytest --help | |||
| ``` | |||
|
|
|||
| If you like to test with a real hardware, you have the option to pass the serial number | |||
There was a problem hiding this comment.
| If you like to test with a real hardware, you have the option to pass the serial number | |
| If you would like to test with a real hardware and need to target one of many identical devices, you have the option to pass the serial number |
This PR adds serial_number as a project option to Arduino project API. In addition, it is added to micro pytest_plugin to enable testing with assigned serial number.