[microTVM][Zephyr] Add 'serial_number' option #13377

mehrdadh · 2022-11-14T17:59:12Z

This PR does these changes:

add serial number to specify device serial number in Zephyr testing
change Zephyr server API to run tests with specific serial number
remove west_cmd in test arguments as it is not used
refactor Zephyr server API to specify required/optional project options at the beginning of each API call.

tvm-bot · 2022-11-14T17:59:14Z

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.

cc @alanmacd, @gromero _{See #10317 for details}
Built docs for commit ec2e55e can be found here.

_{Generated by tvm-bot}

tvm-bot · 2022-11-14T17:59:15Z

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.

cc @alanmacd, @gromero _{See #10317 for details}

_{Generated by tvm-bot}

mehrdadh · 2022-11-16T17:26:58Z

cc @areusch @guberti for review

gromero

@mehrdadh Hi.

I'm still reviewing it, but when testing this PR I've realized that since commit 2252f958f75c6e33b946d23f1ebb803d41f0b63d (#11362) the CMSIS_PATH, although marked as optional, is actually required: either as env. var or passed as a server option. Due to portability, I'd rather like to not force users to have CMSIS-NN installed to use microTVM. Could it be fixed ?

So far iirc only new options were added to the server, but this PR will change an existing option, zephyr_board, to board. This will break previous command line uses passing zephyr_board, do you know if we have any policy about it? I roughly seems an API breakage.

gromero · 2022-11-16T21:11:12Z

apps/microtvm/zephyr/template_project/microtvm_api_server.py

+
+        extra_files_tar = options.get("extra_files_tar")
+
+        cmsis_path = get_cmsis_path(options)


Here, although cmsis_path is declared as optional, it's actually required, so on an env, without CMSIS installed I get:

$ tvmc micro create-project --force /tmp/micro_speech ~/scripts/micro_speech.tar zephyr --project-option board=nrf5340dk_nrf5340_cpuapp project_type=host_driven WARNING:__main__:Board memory information is not available. The following error occurred on the Project API server side: calling method generate_project: JSON-RPC error # -32000: calling method generate_project Traceback (most recent call last): File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 486, in serve_one_request # <--- Outermost server-side stack frame self._dispatch_request(request) File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 598, in _dispatch_request return_value = dispatch_method(**params) File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 630, in _dispatch_generate_project options, File "/home/gromero/git/tvm/build/microtvm_template_projects/zephyr/microtvm_api_server.py", line 564, in generate_project cmsis_path = get_cmsis_path(options) File "/home/gromero/git/tvm/build/microtvm_template_projects/zephyr/microtvm_api_server.py", line 382, in get_cmsis_path cmsis_path = options.get("cmsis_path", os.environ["CMSIS_PATH"]) File "/usr/lib/python3.6/os.py", line 669, in __getitem__ raise KeyError(key) from None KeyError: 'CMSIS_PATH

This is a bug, I fixed it.

Now getting this exception since pathlib.Path() is not expecting a None type:

$ tvmc micro create-project --force /tmp/micro_speech ~/scripts/micro_speech.tar zephyr --project-option board=nrf5340dk_nrf5340_cpuapp project_type=host_driven WARNING:__main__:Board memory information is not available. The following error occurred on the Project API server side: calling method generate_project: JSON-RPC error # -32000: calling method generate_project Traceback (most recent call last): File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 486, in serve_one_request # <--- Outermost server-side stack frame self._dispatch_request(request) File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 598, in _dispatch_request return_value = dispatch_method(**params) File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 630, in _dispatch_generate_project options, File "/home/gromero/git/tvm/build/microtvm_template_projects/zephyr/microtvm_api_server.py", line 564, in generate_project cmsis_path = get_cmsis_path(options) File "/home/gromero/git/tvm/build/microtvm_template_projects/zephyr/microtvm_api_server.py", line 383, in get_cmsis_path return pathlib.Path(cmsis_path) File "/usr/lib/python3.6/pathlib.py", line 1001, in __new__ self = cls._from_parts(args, init=False) File "/usr/lib/python3.6/pathlib.py", line 656, in _from_parts drv, root, parts = self._parse_args(args) File "/usr/lib/python3.6/pathlib.py", line 640, in _parse_args a = os.fspath(a) TypeError: expected str, bytes or os.PathLike object, not NoneType

Fixed this, I added a test to build without this options/environment variable to protect this.

mehrdadh · 2022-11-17T22:27:24Z

@gromero the change from zephyr_board to board happened in previous PRs to make it standard across the API servers. It is not part of this PR.

guberti

I like this PR, and I've left a few small nits. However, I can't seem to get your branch working locally. I'm running:

python -m pytest -s tests/micro/zephyr/test_zephyr.py::test_add_uint --board nucleo_l4r5zi

and I get:

E           tvm.micro.project_api.server.ServerError: calling method flash: JSON-RPC error # -32000: calling method flash
E           Traceback (most recent call last):
E             File "/home/guberti/tvm/python/tvm/micro/project_api/server.py", line 486, in serve_one_request  # <--- Outermost server-side stack frame
E               self._dispatch_request(request)
E             File "/home/guberti/tvm/python/tvm/micro/project_api/server.py", line 598, in _dispatch_request
E               return_value = dispatch_method(**params)
E             File "/home/guberti/tvm/tests/micro/zephyr/workspace_nucleo_l4r5zi/2022-11-18T02-28-22/project/microtvm_api_server.py", line 765, in flash
E               check_call(
E             File "/home/guberti/tvm/tests/micro/zephyr/workspace_nucleo_l4r5zi/2022-11-18T02-28-22/project/microtvm_api_server.py", line 90, in check_call
E               return subprocess.check_call(cmd_args, *args, **kwargs)
E             File "/usr/lib/python3.8/subprocess.py", line 359, in check_call
E               retcode = call(*popenargs, **kwargs)
E             File "/usr/lib/python3.8/subprocess.py", line 340, in call
E               with Popen(*popenargs, **kwargs) as p:
E             File "/usr/lib/python3.8/subprocess.py", line 858, in __init__
E               self._execute_child(args, executable, preexec_fn, close_fds,
E             File "/usr/lib/python3.8/subprocess.py", line 1704, in _execute_child
E               raise child_exception_type(errno_num, err_msg, err_filename)
E           FileNotFoundError: [Errno 2] No such file or directory: '/usr/bin/python -m west'

python/tvm/micro/project_api/client.py:135: ServerError

It seems like check_call is being passed the arguments ['/usr/bin/python -m west', 'flash', '-r', 'openocd'], and subprocess assumes that '/usr/bin/python -m west' is one str argument and surrounds it with quotes. Maybe this is a bug, or maybe I'm using it wrong?

guberti · 2022-11-18T09:25:07Z

apps/microtvm/zephyr/template_project/microtvm_api_server.py

+CMSIS_PATH_ERROR = (
+    "cmsis_path is not defined! Please pass it as"
+    "an option or provide it with setting `CMSIS_PATH` env variable."
+)


Suggested change

CMSIS_PATH_ERROR = (

"cmsis_path is not defined! Please pass it as"

"an option or provide it with setting `CMSIS_PATH` env variable."

)

CMSIS_PATH_ERROR = (

"cmsis_path is not defined! Please pass it as"

"an option or set the `CMSIS_PATH` env variable."

)

guberti · 2022-11-18T09:39:07Z

apps/microtvm/zephyr/template_project/microtvm_api_server.py

-    def _generate_cmake_args(self, mlf_extracted_path, options) -> str:
+    def _generate_cmake_args(
+        self,
+        mlf_extracted_path,


What's the type here?

guberti · 2022-11-18T09:43:22Z

apps/microtvm/zephyr/template_project/microtvm_api_server.py

+                heap_size = recommended_heap_size
+                if heap_size_bytes:
+                    heap_size = heap_size_bytes


Having two different variables heap_size_bytes and heap_size might be confusing. Could we instead do:

Suggested change

heap_size = recommended_heap_size

if heap_size_bytes:

heap_size = heap_size_bytes

heap_size = options.get("heap_size_bytes") or recommended_heap_size

changed that.

gromero · 2022-11-18T14:13:24Z

apps/microtvm/zephyr/template_project/microtvm_api_server.py

+
+        extra_files_tar = options.get("extra_files_tar")
+
+        cmsis_path = get_cmsis_path(options)


Now getting this exception since pathlib.Path() is not expecting a None type:

$ tvmc micro create-project --force /tmp/micro_speech ~/scripts/micro_speech.tar zephyr --project-option board=nrf5340dk_nrf5340_cpuapp project_type=host_driven WARNING:__main__:Board memory information is not available. The following error occurred on the Project API server side: calling method generate_project: JSON-RPC error # -32000: calling method generate_project Traceback (most recent call last): File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 486, in serve_one_request # <--- Outermost server-side stack frame self._dispatch_request(request) File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 598, in _dispatch_request return_value = dispatch_method(**params) File "/home/gromero/git/tvm/python/tvm/micro/project_api/server.py", line 630, in _dispatch_generate_project options, File "/home/gromero/git/tvm/build/microtvm_template_projects/zephyr/microtvm_api_server.py", line 564, in generate_project cmsis_path = get_cmsis_path(options) File "/home/gromero/git/tvm/build/microtvm_template_projects/zephyr/microtvm_api_server.py", line 383, in get_cmsis_path return pathlib.Path(cmsis_path) File "/usr/lib/python3.6/pathlib.py", line 1001, in __new__ self = cls._from_parts(args, init=False) File "/usr/lib/python3.6/pathlib.py", line 656, in _from_parts drv, root, parts = self._parse_args(args) File "/usr/lib/python3.6/pathlib.py", line 640, in _parse_args a = os.fspath(a) TypeError: expected str, bytes or os.PathLike object, not NoneType

gromero · 2022-11-18T14:27:19Z

@mehrdadh I changed the title to add a space after the tags and tweak it a bit the wording to emphasize the new option 'serial_number' since the tweak on the tests would be a merely a consequence of it, although I do guess you're adding it for the internal test fleet.

guberti · 2022-11-18T14:52:36Z

@mehrdadh I changed the title to add a space after the tags and tweak it a bit the wording to emphasize the new option 'serial_number' since the tweak on the tests would be a merely a consequence of it, although I do guess you're adding it for the internal test fleet.

We should also update the README.md file to explain correct usage:

https://github.com/apache/tvm/blob/main/tests/micro/zephyr/README.md

mehrdadh · 2022-11-21T23:31:00Z

@guberti in cortex_m docker image, the west_cmd is alos '/venv/apache-tvm-py3.7/bin/python3 -m west'
and it works without issue. Not sure what happened in that case

Update: I take back what I said, I need to fix this.
Update2: I fixed it by parsing the command for subprocess case

mehrdadh · 2022-11-21T23:42:31Z

@guberti I added details about using serial_number in testing.

guberti

LGTM! I've tested this locally with the NUCLEO-L4R5ZI and it all works as intended.

It would be awesome to make this part of the microTVM pytest plugin and enable this functionality for the Arduino and common tests in a future PR.

gromero

@mehrdadh Thanks a lot for addressing the comments. Just three remaining tweaks and I think it's good to land.

tests/micro/zephyr/README.md

apps/microtvm/zephyr/template_project/microtvm_api_server.py

mehrdadh · 2022-11-23T19:15:16Z

@gromero thanks for catching those! PTAL.

gromero

@mehrdadh LGTM!

gromero · 2022-11-23T20:02:09Z

To whom / Committer / Maintainer that will squash & merge this PR, please consider copying the text from this PR body/description and pasting it as the commit message, avoiding the commit message prepared by GH automatically that brings the review comments, it just takes 1 second and produces a clearer git log. Also in consonance with the Commit Message Guideline. Thanks!

mehrdadh · 2022-11-23T22:04:49Z

I rebased because of this commit: 8cccc25

alanmacd · 2022-11-23T22:56:36Z

tests/micro/zephyr/README.md

@@ -40,3 +40,9 @@ To see the list of supported values for `--board`, run:
 ```
 $ pytest test_zephyr.py --help
 ```
+
+If you like to test with a real hardware, you have the option to pass the serial number


nit:

Suggested change

If you like to test with a real hardware, you have the option to pass the serial number

If you like to test with real hardware, you have the option to pass the serial number

I'll apply it to the next PR if the CI passed.

gromero · 2022-11-24T00:53:53Z

Thanks @mehrdadh @guberti @alanmacd! It's merged.

This commit adds a new option 'serial_number' to allow specifying the serial device number in Zephyr, used to flash and to communicate with target boards attached to specific serial ports, and adjusts Zephyr tests to run with this new option. This is particularly useful in an environment where multiple boards are connected to the host. It also removes 'west_cmd' in test arguments, as it is not used, and refactors Zephyr server API to set the required/optional project options at the beginning of each API call.

Added config_main_stack_size to autotune since running graph debugger requires more workspace. Also added to micro_aot because of KWS model. Also removed west_cmd option in tutorial because of #13377

mehrdadh requested a review from gromero November 14, 2022 17:59

mehrdadh force-pushed the micro/zephyr_use_serial_number branch 2 times, most recently from f47a3f1 to f6b907e Compare November 15, 2022 21:47

gromero reviewed Nov 16, 2022

View reviewed changes

guberti reviewed Nov 18, 2022

View reviewed changes

gromero requested changes Nov 18, 2022

View reviewed changes

gromero changed the title ~~[microTVM][Zephyr]Add serial_number test args~~ [microTVM][Zephyr] Add 'serial_number' option Nov 18, 2022

mehrdadh requested review from guberti and gromero and removed request for guberti November 21, 2022 23:42

mehrdadh force-pushed the micro/zephyr_use_serial_number branch 3 times, most recently from d5f9ee9 to ef992a8 Compare November 22, 2022 18:02

guberti approved these changes Nov 23, 2022

View reviewed changes

gromero requested changes Nov 23, 2022

View reviewed changes

tests/micro/zephyr/README.md Outdated Show resolved Hide resolved

apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved

apps/microtvm/zephyr/template_project/microtvm_api_server.py Show resolved Hide resolved

gromero approved these changes Nov 23, 2022

View reviewed changes

mehrdadh added 3 commits November 23, 2022 14:03

Add serial number in Flash

57c96d0

testing

54c2898

add serial_number in args

a8a02de

mehrdadh added 9 commits November 23, 2022 14:03

cleanup options

881658d

fix error

62195bf

fix CMSIS_PATH ENV being required

e2f18b9

fix cmsis_path and test added

2e0162a

address comments

3bad053

fix lint

3ed71ec

fix west_cmd

6bec462

add serial number to test_zephyr

5809cc0

address comments

ec2e55e

mehrdadh force-pushed the micro/zephyr_use_serial_number branch from 06fce1f to ec2e55e Compare November 23, 2022 22:04

alanmacd reviewed Nov 23, 2022

View reviewed changes

alanmacd approved these changes Nov 23, 2022

View reviewed changes

gromero merged commit 3680b3c into apache:main Nov 24, 2022

mehrdadh deleted the micro/zephyr_use_serial_number branch November 24, 2022 00:56

mehrdadh mentioned this pull request Nov 29, 2022

[microTVM][Tutorial] Fix micro_aot and micro_autotune tutorials #13513

Merged

leandron mentioned this pull request Feb 1, 2023

TVM v0.11.0 Release Candidate Notes #13899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[microTVM][Zephyr] Add 'serial_number' option #13377

[microTVM][Zephyr] Add 'serial_number' option #13377

mehrdadh commented Nov 14, 2022

tvm-bot commented Nov 14, 2022 •

edited

Loading

tvm-bot commented Nov 14, 2022

mehrdadh commented Nov 16, 2022

gromero left a comment •

edited

Loading

gromero Nov 16, 2022 •

edited

Loading

mehrdadh Nov 17, 2022

gromero Nov 18, 2022

mehrdadh Nov 21, 2022

mehrdadh commented Nov 17, 2022

guberti left a comment •

edited

Loading

guberti Nov 18, 2022

mehrdadh Nov 21, 2022

guberti Nov 18, 2022

mehrdadh Nov 21, 2022

guberti Nov 18, 2022

mehrdadh Nov 21, 2022

gromero Nov 18, 2022

gromero commented Nov 18, 2022

guberti commented Nov 18, 2022

mehrdadh commented Nov 21, 2022 •

edited

Loading

mehrdadh commented Nov 21, 2022

guberti left a comment

gromero left a comment •

edited

Loading

mehrdadh commented Nov 23, 2022

gromero left a comment

gromero commented Nov 23, 2022

mehrdadh commented Nov 23, 2022

alanmacd Nov 23, 2022

mehrdadh Nov 23, 2022

gromero commented Nov 24, 2022


		extra_files_tar = options.get("extra_files_tar")

		cmsis_path = get_cmsis_path(options)

	If you like to test with a real hardware, you have the option to pass the serial number
	If you like to test with real hardware, you have the option to pass the serial number

[microTVM][Zephyr] Add 'serial_number' option #13377

[microTVM][Zephyr] Add 'serial_number' option #13377

Conversation

mehrdadh commented Nov 14, 2022

tvm-bot commented Nov 14, 2022 • edited Loading

tvm-bot commented Nov 14, 2022

mehrdadh commented Nov 16, 2022

gromero left a comment • edited Loading

Choose a reason for hiding this comment

gromero Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadh commented Nov 17, 2022

guberti left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gromero commented Nov 18, 2022

guberti commented Nov 18, 2022

mehrdadh commented Nov 21, 2022 • edited Loading

mehrdadh commented Nov 21, 2022

guberti left a comment

Choose a reason for hiding this comment

gromero left a comment • edited Loading

Choose a reason for hiding this comment

mehrdadh commented Nov 23, 2022

gromero left a comment

Choose a reason for hiding this comment

gromero commented Nov 23, 2022

mehrdadh commented Nov 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gromero commented Nov 24, 2022

tvm-bot commented Nov 14, 2022 •

edited

Loading

gromero left a comment •

edited

Loading

gromero Nov 16, 2022 •

edited

Loading

guberti left a comment •

edited

Loading

mehrdadh commented Nov 21, 2022 •

edited

Loading

gromero left a comment •

edited

Loading