guberti commented on a change in pull request #8940:
URL: https://github.com/apache/tvm/pull/8940#discussion_r704050902
##########
File path: tests/micro/arduino/conftest.py
##########
@@ -48,13 +35,23 @@
).resolve()
+def arduino_micro_devices() -> dict:
Review comment:
Can we cache this value? `arduino_micro_devices()` will probably be
called several times by our test file.
##########
File path: apps/microtvm/reference-vm/README.md
##########
@@ -72,20 +72,20 @@ For example:
$ ./base-box-tool.py --provider virtualbox build zephyr
```
-2. **Run** release tests for each platform:
+2. **Run** release tests for each microTVM device:
Review comment:
```suggestion
2. **Run** release tests for the specified `MICROTVM_DEVICE` (e.g.
`nano33ble` for Arduino)
```
I don't actually think `base-box-tool.py` supports testing more than one
microTVM device at once.
##########
File path: apps/microtvm/reference-vm/README.md
##########
@@ -72,20 +72,20 @@ For example:
$ ./base-box-tool.py --provider virtualbox build zephyr
```
-2. **Run** release tests for each platform:
+2. **Run** release tests for each microTVM device:
A. Connect any needed hardware to the VM host machine;
B. Run tests:
```bash
- $ ./base-box-tool.py [--provider=PROVIDER] test
--microtvm-platform=MICROTVM_PLATFORM [--test-device-serial=SERIAL] PLATFORM
+ $ ./base-box-tool.py [--provider=PROVIDER] test
--microtvm-device=MICROTVM_DEVICE [--test-device-serial=SERIAL] PLATFORM
```
- where MICROTVM_PLATFORM is one of the options listed in the
+ where MICROTVM_DEVICE is one of the options listed in the
PLATFORM/base-box/test-config.json file.
For example:
```base
- $ ./base-box-tool.py --provider virtualbox test
--microtvm-platform=stm32f746xx_disco zephyr
+ $ ./base-box-tool.py --provider virtualbox test
--microtvm-device=stm32f746xx_disco zephyr
Review comment:
```suggestion
$ ./base-box-tool.py --provider virtualbox test zephyr
--microtvm-device=stm32f746xx_disco
```
I'd love to specify `microtvm-device` after setting `platform` to
Arduino/Zephyr in the example commands, to help reinforce that the options for
what `microtvm-device` can be depend on the choice of platform. I'd love to do
this for all other example `test` commands in this file as well.
##########
File path: apps/microtvm/zephyr/template_project/microtvm_api_server.py
##########
@@ -57,6 +57,19 @@
IS_TEMPLATE = not (API_SERVER_DIR / MODEL_LIBRARY_FORMAT_RELPATH).exists()
+# Maps a short, identifying microtvm device string to (target, zephyr_board).
+MICRO_DEVICES = {
+ "qemu_x86": ("host", "qemu_x86"),
Review comment:
^
##########
File path: tests/micro/arduino/conftest.py
##########
@@ -48,13 +35,23 @@
).resolve()
+def arduino_micro_devices() -> dict:
+ sys.path.insert(0, str(TEMPLATE_PROJECT_DIR))
+ try:
+ import microtvm_api_server
+ finally:
+ sys.path.pop(0)
+
+ return microtvm_api_server.MICRO_DEVICES
Review comment:
I'm with Andrew - is there any way this can be done by reading the
choices from `ProjectOption`?
##########
File path: apps/microtvm/arduino/template_project/microtvm_api_server.py
##########
@@ -43,6 +43,19 @@
IS_TEMPLATE = not (API_SERVER_DIR / MODEL_LIBRARY_FORMAT_RELPATH).exists()
+# Maps a short, identifying microtvm device string to (target, arduino_board).
+MICRO_DEVICES = {
Review comment:
I agree with @gromero that `BOARDS` is a better name for these than
`MICRO_DEVICES`. However, I don't like the idea of having the identifying
device string map hard-coded into `microtvm_api_server.py`.
Could we merge the `MICRO_DEVICES/BOARDS` and `BOARD_PROPERTIES` into a
single `boards.json` file? That way, we wouldn't have to edit
`microtvm_api_server.py` whenever a new Arduino board is tested and shown to
work with TVM (and will also make `microtvm_api_server.py`like 80 lines
shorter).
--
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]
