You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/09/20 08:57:24 UTC
[GitHub] [tvm] guberti commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
guberti commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r975019826
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -55,27 +57,29 @@
raise FileNotFoundError(f"Board file {{{BOARDS}}} does not exist.")
+def get_cmsis_path(cmsis_path: pathlib.Path) -> pathlib.Path:
+ """Returns CMSIS dependency path"""
+ if cmsis_path:
+ return pathlib.Path(cmsis_path)
+ if os.environ.get("CMSIS_PATH"):
+ return pathlib.Path(os.environ.get("CMSIS_PATH"))
+ assert False, "'cmsis_path' option not passed!"
+
+
class BoardAutodetectFailed(Exception):
"""Raised when no attached hardware is found matching the requested board"""
PROJECT_TYPES = ["example_project", "host_driven"]
-PROJECT_OPTIONS = [
- server.ProjectOption(
- "arduino_board",
- required=["build", "flash", "open_transport"],
- choices=list(BOARD_PROPERTIES),
- type="str",
- help="Name of the Arduino board to build for.",
- ),
+PROJECT_OPTIONS = server.default_project_options(
+ project_type={"choices": tuple(PROJECT_TYPES)},
+ board={"choices": list(BOARD_PROPERTIES), "optional": ["flash", "open_transport"]},
Review Comment:
Why has `board` become an optional parameter?
##########
apps/microtvm/arduino/template_project/Makefile.template:
##########
@@ -0,0 +1,57 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
Review Comment:
I would love a docstring-like comment explaining what this file is - e.g. "helper utility for running standalone Arduino commands" with some usage examples. Only has to be a few lines.
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -55,27 +57,29 @@
raise FileNotFoundError(f"Board file {{{BOARDS}}} does not exist.")
+def get_cmsis_path(cmsis_path: pathlib.Path) -> pathlib.Path:
+ """Returns CMSIS dependency path"""
+ if cmsis_path:
+ return pathlib.Path(cmsis_path)
+ if os.environ.get("CMSIS_PATH"):
+ return pathlib.Path(os.environ.get("CMSIS_PATH"))
+ assert False, "'cmsis_path' option not passed!"
+
+
class BoardAutodetectFailed(Exception):
"""Raised when no attached hardware is found matching the requested board"""
PROJECT_TYPES = ["example_project", "host_driven"]
-PROJECT_OPTIONS = [
- server.ProjectOption(
- "arduino_board",
- required=["build", "flash", "open_transport"],
- choices=list(BOARD_PROPERTIES),
- type="str",
- help="Name of the Arduino board to build for.",
- ),
+PROJECT_OPTIONS = server.default_project_options(
+ project_type={"choices": tuple(PROJECT_TYPES)},
+ board={"choices": list(BOARD_PROPERTIES), "optional": ["flash", "open_transport"]},
+ warning_as_error={"optional": ["build", "flash"]},
+) + [
server.ProjectOption(
"arduino_cli_cmd",
- required=(
- ["generate_project", "build", "flash", "open_transport"]
- if not ARDUINO_CLI_CMD
- else None
- ),
+ required=(["generate_project", "flash", "open_transport"] if not ARDUINO_CLI_CMD else None),
Review Comment:
Why is this no longer required for `build`? Not sure I'm understanding.
##########
apps/microtvm/arduino/template_project/Makefile.template:
##########
@@ -0,0 +1,57 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+MAKE_DIR := $(PWD)
+FQBN ?= <FQBN>
+VERBOSE_FLAG ?= <VERBOSE_FLAG>
+BUILD_DIR := $(subst :,.,build)
+PORT ?=
+ARUINO_CLI_CMD ?= <ARUINO_CLI_CMD>
+BOARD := <BOARD>
+BUILD_EXTRA_FLAGS := <BUILD_EXTRA_FLAGS>
Review Comment:
I would really love some documentation on what these flags do. I can piece together some of them, but what is `BUILD_EXTRA_FLAGS`? What is `MAKE_DIR`?
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +304,51 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ def _cmsis_required(self, project_path: Union[str, pathlib.Path]) -> bool:
Review Comment:
Could we standardize this to only accept a `pathlib.Path`? Since `_cmsis_required` is an internal function, doing so seems cleaner.
```suggestion
def _cmsis_required(self, project_path: pathlib.Path) -> bool:
```
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -341,16 +376,49 @@ def generate_project(self, model_library_format_path, standalone_crt_dir, projec
shutil.copy2(model_library_format_path, project_dir / MODEL_LIBRARY_FORMAT_RELPATH)
# For AOT, template model.h with metadata to minimize space usage
- if options["project_type"] == "example_project":
+ if project_type == "example_project":
self._template_model_header(source_dir, metadata)
self._change_cpp_file_extensions(source_dir)
# Recursively change includes
self._convert_includes(project_dir, source_dir)
- def _get_arduino_cli_cmd(self, options: dict):
- arduino_cli_cmd = options.get("arduino_cli_cmd", ARDUINO_CLI_CMD)
+ # create include directory
+ (project_dir / "include").mkdir()
+ build_extra_flags = "-I./include"
+
+ if self._cmsis_required(project_dir):
+ build_extra_flags += f" -I./include/cmsis"
+ self._copy_cmsis(project_dir, cmsis_path)
+
+ # Populate Makefile
+ with open(project_dir / MAKEFILE_FILENAME, "w") as makefile_f:
Review Comment:
Can we make the code that populates the Makefile its own function? That's what we do for `_change_cpp_file_extensions`, `_convert_includes`, `_copy_cmsis`, etc.
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,43 +436,33 @@ def _get_platform_version(self, arduino_cli_path: str) -> float:
return version.parse(str_version)
# This will only be run for build and upload
- def _check_platform_version(self, options):
+ def _check_platform_version(self, arduino_cli_cmd: str, warning_as_error: bool):
Review Comment:
I don't think this typing is right - `arduino_cli_cmd` could be `str` or `None`. IMO it would be cleaner to convert it to a string in a high-level function, and skip calling `_get_arduino_cli_cmd` here.
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -368,43 +436,33 @@ def _get_platform_version(self, arduino_cli_path: str) -> float:
return version.parse(str_version)
# This will only be run for build and upload
- def _check_platform_version(self, options):
+ def _check_platform_version(self, arduino_cli_cmd: str, warning_as_error: bool):
if not self._version:
- cli_command = self._get_arduino_cli_cmd(options)
+ cli_command = self._get_arduino_cli_cmd(arduino_cli_cmd)
self._version = self._get_platform_version(cli_command)
if self._version < MIN_ARDUINO_CLI_VERSION:
message = (
f"Arduino CLI version too old: found {self._version}, "
f"need at least {str(MIN_ARDUINO_CLI_VERSION)}."
)
- if options.get("warning_as_error") is not None and options["warning_as_error"]:
+ if warning_as_error is not None and warning_as_error:
raise server.ServerError(message=message)
_LOG.warning(message)
- def _get_fqbn(self, options):
- o = BOARD_PROPERTIES[options["arduino_board"]]
+ def _get_fqbn(self, board: str):
+ o = BOARD_PROPERTIES[board]
return f"{o['package']}:{o['architecture']}:{o['board']}"
def build(self, options):
- self._check_platform_version(options)
- BUILD_DIR.mkdir()
-
- compile_cmd = [
- self._get_arduino_cli_cmd(options),
- "compile",
- "./project/",
- "--fqbn",
- self._get_fqbn(options),
- "--build-path",
- BUILD_DIR.resolve(),
- ]
-
- if options.get("verbose"):
- compile_cmd.append("--verbose")
+ # List all used project options
+ arduino_cli_cmd = options.get("arduino_cli_cmd")
+ warning_as_error = options.get("warning_as_error")
+ self._check_platform_version(arduino_cli_cmd, warning_as_error)
+ compile_cmd = ["make", "build"]
# Specify project to compile
- subprocess.run(compile_cmd, check=True)
+ subprocess.run(compile_cmd, check=True, cwd=API_SERVER_DIR)
Review Comment:
Why this change? Would love a comment.
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -440,54 +498,59 @@ def _parse_connected_boards(self, tabular_str):
device[col_name] = str_row[column.start() : column.end()].strip()
yield device
- def _auto_detect_port(self, options):
- list_cmd = [self._get_arduino_cli_cmd(options), "board", "list"]
+ 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(options)
+ desired_fqbn = self._get_fqbn(board)
for device in self._parse_connected_boards(list_cmd_output):
if device["fqbn"] == desired_fqbn:
return device["port"]
# If no compatible boards, raise an error
raise BoardAutodetectFailed()
- def _get_arduino_port(self, options):
+ def _get_arduino_port(self, arduino_cli_cmd: str, board: str, port: int):
if not self._port:
- if "port" in options and options["port"]:
- self._port = options["port"]
+ if port:
+ self._port = port
else:
- self._port = self._auto_detect_port(options)
+ self._port = self._auto_detect_port(arduino_cli_cmd, board)
return self._port
+ def _get_board_from_makefile(self, makefile_path: pathlib.Path) -> str:
+ """Get Board from generated Makefile."""
+ with open(makefile_path) as makefile_f:
+ line = makefile_f.readline()
+ if "BOARD" in line:
+ board = line.replace(" ", "").replace("\n", "").replace("\r", "").split(":=")[1]
Review Comment:
```suggestion
board = re.sub(r"\s", "", line).split(":=")[1]
```
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +779,68 @@ def write_with_timeout(fd, data, timeout_sec): # pylint: disable=invalid-name
return num_written
+def default_project_options(**kw) -> typing.List[ProjectOption]:
+ """Get default Project Options
+
+ Attributes of any default option can be updated. Here is an example
+ when attribute `optional` from `verbose` option needs to be updates:
+
+ default_project_options(verbose={"optional": ["build"]})
+
+ This will update the `optional` attribute of `verbose` ProjectOption
+ to be `["build"]`.
+
+ Returns
+ -------
+ options: List[ProjectOption]
+ A list of default ProjectOption with modifications.
+ """
+ options = [
+ ProjectOption(
+ "verbose",
+ optional=["generate_project"],
+ type="bool",
+ help="Run build with verbose output.",
+ ),
+ ProjectOption(
+ "project_type",
+ required=["generate_project"],
+ type="str",
+ help="Type of project to generate.",
+ ),
+ ProjectOption(
+ "board",
+ required=["generate_project"],
+ type="str",
+ help="Name of the board to build for.",
Review Comment:
```suggestion
help="Name of the board to build for: e.g. spresense, nano33ble",
```
##########
python/tvm/micro/project_api/server.py:
##########
@@ -64,6 +64,26 @@ def __new__(cls, name, **kw):
return super().__new__(cls, **kw)
+ def replace(self, **kw):
+ """Update attributes associated to the project option."""
+ updated_option = self
+ for key, val in kw.items():
+ if key == "choices":
+ updated_option = updated_option._replace(choices=val)
+ elif key == "default":
+ updated_option = updated_option._replace(default=val)
+ elif key == "type":
+ updated_option = updated_option._replace(type=val)
+ elif key == "required":
+ updated_option = updated_option._replace(required=val)
+ elif key == "optional":
+ updated_option = updated_option._replace(optional=val)
+ elif key == "help":
+ updated_option = updated_option._replace(help=val)
+ else:
+ raise ValueError("Attribute {} is not supported.".format(key))
Review Comment:
```suggestion
for pair in kw.items():
updated_option = updated_option._replace(**pair)
```
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +779,68 @@ def write_with_timeout(fd, data, timeout_sec): # pylint: disable=invalid-name
return num_written
+def default_project_options(**kw) -> typing.List[ProjectOption]:
+ """Get default Project Options
+
+ Attributes of any default option can be updated. Here is an example
+ when attribute `optional` from `verbose` option needs to be updates:
+
+ default_project_options(verbose={"optional": ["build"]})
+
+ This will update the `optional` attribute of `verbose` ProjectOption
+ to be `["build"]`.
+
+ Returns
+ -------
+ options: List[ProjectOption]
+ A list of default ProjectOption with modifications.
+ """
+ options = [
+ ProjectOption(
+ "verbose",
+ optional=["generate_project"],
+ type="bool",
+ help="Run build with verbose output.",
+ ),
+ ProjectOption(
+ "project_type",
+ required=["generate_project"],
+ type="str",
+ help="Type of project to generate.",
+ ),
+ ProjectOption(
+ "board",
+ required=["generate_project"],
+ type="str",
+ help="Name of the board to build for.",
+ ),
+ ProjectOption(
+ "cmsis_path",
+ optional=["generate_project"],
+ type="str",
+ help="Path to the CMSIS directory.",
+ ),
+ ProjectOption(
+ "warning_as_error",
+ optional=["generate_project"],
+ type="bool",
+ default=False,
+ help="Treat warnings as errors and raise an Exception.",
+ ),
+ ]
+ for name, config in kw.items():
+ option_found = False
+ for ind, option in enumerate(options):
+ if option.name == name:
+ options[ind] = option.replace(**config)
+ option_found = True
+ break
+ if not option_found:
+ raise ValueError("Option {} was not found in default ProjectOptions.".format(name))
Review Comment:
Could you add a comment explaining what these lines are doing?
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -176,6 +161,12 @@ def _copy_standalone_crt(self, source_dir, standalone_crt_dir):
"src/runtime/crt/tab",
]
+ FQBN_TOKEN = "<FQBN>"
Review Comment:
What are these tokens going to be used for? Would love a comment.
I would also prefer to have them defined as constants directly above where `generate_project` is located, since they are first referenced there.
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +779,68 @@ def write_with_timeout(fd, data, timeout_sec): # pylint: disable=invalid-name
return num_written
+def default_project_options(**kw) -> typing.List[ProjectOption]:
+ """Get default Project Options
+
+ Attributes of any default option can be updated. Here is an example
+ when attribute `optional` from `verbose` option needs to be updates:
+
+ default_project_options(verbose={"optional": ["build"]})
+
+ This will update the `optional` attribute of `verbose` ProjectOption
+ to be `["build"]`.
+
+ Returns
+ -------
+ options: List[ProjectOption]
+ A list of default ProjectOption with modifications.
+ """
+ options = [
+ ProjectOption(
+ "verbose",
+ optional=["generate_project"],
+ type="bool",
+ help="Run build with verbose output.",
Review Comment:
What exactly does "verbose output" add?
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +304,51 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ def _cmsis_required(self, project_path: Union[str, pathlib.Path]) -> bool:
+ """Check if CMSIS dependency is required."""
+ project_path = pathlib.Path(project_path)
+ for path in (project_path / "src" / "model").iterdir():
+ if path.is_file():
+ with open(path, "r", encoding="ISO-8859-1") as lib_f:
+ lib_content = lib_f.read()
+ if any(
+ header in lib_content
+ for header in [
+ "<arm_nnsupportfunctions.h>",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ ]
+ ):
Review Comment:
Could we store the CMSIS_NN headers as a capitalized constant elsewhere? While we're at it, let's also include [all five](https://github.com/ARM-software/CMSIS_5/tree/develop/CMSIS/NN/Include) of the headers, not just three, e.g.
```python
CMSIS_INCLUDE_HEADERS = [
"arm_nn_math_types.h",
"arm_nn_tables.h",
"arm_nn_types.h",
"arm_nnfunctions.h",
"arm_nnsupportfunctions.h",
]
```
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +304,51 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ def _cmsis_required(self, project_path: Union[str, pathlib.Path]) -> bool:
+ """Check if CMSIS dependency is required."""
+ project_path = pathlib.Path(project_path)
+ for path in (project_path / "src" / "model").iterdir():
+ if path.is_file():
+ with open(path, "r", encoding="ISO-8859-1") as lib_f:
Review Comment:
Can we do this?
```suggestion
with open(path, "r") as lib_f:
```
If that doesn't work, let's add a comment about why.
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +304,51 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ def _cmsis_required(self, project_path: Union[str, pathlib.Path]) -> bool:
+ """Check if CMSIS dependency is required."""
+ project_path = pathlib.Path(project_path)
+ for path in (project_path / "src" / "model").iterdir():
+ if path.is_file():
+ with open(path, "r", encoding="ISO-8859-1") as lib_f:
+ lib_content = lib_f.read()
+ if any(
+ header in lib_content
+ for header in [
+ "<arm_nnsupportfunctions.h>",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ ]
+ ):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: Union[str, pathlib.Path], cmsis_path: str):
+ """Copy CMSIS header files to project.
+ Note: We use this CMSIS package:https://www.arduino.cc/reference/en/libraries/arduino_cmsis-dsp/
+ However, the latest release does not include header files that are copied in this function.
+ """
+ (project_path / "include" / "cmsis").mkdir()
+ cmsis_path = get_cmsis_path(cmsis_path)
+ for item in [
+ "arm_nn_types.h",
Review Comment:
See above comment - let's only have this list in one place.
--
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: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org