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/16 17:14:52 UTC
[GitHub] [tvm] mehrdadh opened a new pull request, #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
mehrdadh opened a new pull request, #12818:
URL: https://github.com/apache/tvm/pull/12818
This PR:
- Refactors common project options to a single place for template projects
- Adds a feature to update project options for customization
- Adds a Makefile for Arduino project to make the build/flash process more standalone
- Refactors `zephyr_board` and `arduino_board` to `board`
cc @gromero @guberti @areusch
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r976836451
##########
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 removed MAKE_DIR, we don't need that
Also added more info on each variable
--
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
[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
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r976833788
##########
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:
Ah, forgot about that. Makes sense!
--
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
[GitHub] [tvm] mehrdadh commented on pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #12818:
URL: https://github.com/apache/tvm/pull/12818#issuecomment-1256433928
@guberti @areusch PTAL, thanks!
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r978098147
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
Review Comment:
done
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
Review Comment:
done
--
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
[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
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r976859377
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +764,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:
possible nit:
```suggestion
assert kw.keys() <= {o.name for o in options}, "Unknown option!"
for index, option in enumerate(options):
if option in kw:
options[index] = option.replace(config)
```
##########
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:
what are your thoughts here?
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r976769358
##########
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:
arduino_cli_cmd is set as a variable in the Makefile in generate_project step. Later in the build we don't need that, you just call `make build` to build the project.
##########
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:
added comment
##########
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:
because compile_cmd is `make build` and that needs to be in the same directory as Makefile
##########
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:
it shows more debugging messages from build and flash steps
##########
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:
done, thanks!
##########
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:
board is required for generate_project, which is the default. But you can optionally pass it for flash or open_transport because by default it would get it from the generated Makefile
##########
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 added an `info` command in the makefile which explains how to use this file
##########
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:
done.
##########
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:
done
##########
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:
done
##########
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:
done
##########
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:
this is not specific to arduino, so I would like to keep it generic
##########
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:
done.
##########
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:
done
##########
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:
these are updating the default options and I think the function string explains it
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r979082601
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
+ VERBOSE_FLAG_TOKEN = "<VERBOSE_FLAG>"
+ ARUINO_CLI_CMD_TOKEN = "<ARUINO_CLI_CMD>"
+ BOARD_TOKEN = "<BOARD>"
+ BUILD_EXTRA_FLAGS_TOKEN = "<BUILD_EXTRA_FLAGS>"
+
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ board: str,
Review Comment:
As we discussed this offline, I think either of the solutions has tradeoff. To make sure we use the correct optional/required for each option, I added few tests here:
https://github.com/apache/tvm/pull/12818/commits/aa134138745b8a51d440f527b6b5851c8f526fdb#diff-f461d1a4c216e770f22b27fc7926b7fd31cde6273b1e15f9d5d08ec2892ec251R1
--
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
[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
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r976859925
##########
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:
what are your thoughts on the typing stuff here?
--
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
[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
Posted by GitBox <gi...@apache.org>.
guberti commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r978883618
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +764,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:
Sorry, this should be `if option.name in kw`. Fixed!
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +764,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:
possible nit:
```suggestion
assert kw.keys() <= {o.name for o in options}, "Unknown option!"
for index, option in enumerate(options):
if option.name in kw:
options[index] = option.replace(config)
```
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r984058260
##########
tests/micro/project_api/test_project_api.py:
##########
@@ -0,0 +1,104 @@
+# 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.
+
+import sys
+
+import tvm
+from tvm.micro.project_api import server
+
+API_GENERATE_PROJECT = "generate_project"
+API_BUILD = "build"
+API_FLASH = "flash"
+API_OPEN_TRANSPORT = "open_transport"
+
+PLATFORM_ARDUINO = "arduino"
+PLATFORM_ZEPHYR = "zephyr"
+
+
+platform = tvm.testing.parameter(PLATFORM_ARDUINO, PLATFORM_ZEPHYR)
+
+
+def test_default_options_exist(platform):
+ sys.path.insert(0, tvm.micro.get_microtvm_template_projects(platform))
+ import microtvm_api_server
+
+ platform_options = microtvm_api_server.PROJECT_OPTIONS
+ default_options = server.default_project_options()
+
+ option_names = []
+ for option in platform_options:
+ option_names.append(option.name)
+
+ for option in default_options:
+ assert option.name in option_names
+
+
+def test_default_options_requirements(platform):
Review Comment:
thanks! I updated the test.
--
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
[GitHub] [tvm] mehrdadh commented on pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #12818:
URL: https://github.com/apache/tvm/pull/12818#issuecomment-1256433022
@tvm-bot rerun
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r976941031
##########
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:
that makes sense, I moved `_get_arduino_cli_cmd` call in the high-level function
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +764,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:
that doesn't work because the `kw` is a list of suggested changes but option has ProjectOption type
--
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
[GitHub] [tvm] areusch commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r978035114
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
+ VERBOSE_FLAG_TOKEN = "<VERBOSE_FLAG>"
+ ARUINO_CLI_CMD_TOKEN = "<ARUINO_CLI_CMD>"
+ BOARD_TOKEN = "<BOARD>"
+ BUILD_EXTRA_FLAGS_TOKEN = "<BUILD_EXTRA_FLAGS>"
+
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ board: str,
Review Comment:
why not accept options dict here? it might make it a little easier to move code around in this file
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
Review Comment:
can you make this a regexp and use re.findall to substitute? e.g.
```
VARS = {"VERBOSE_FLAG": "--verbose" if verbose else "", ...}
SUBST_TOKEN_RE = re.compile(r"<(?P<var>[A-Z]_+)>")
outs = []
for i, m in enumerate(re.split(SUBST_TOKEN_RE, line)):
if i % 2 == 1:
# m is a var
m = VARS[m]
outs.append(m)
```
--
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r978098988
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
+ VERBOSE_FLAG_TOKEN = "<VERBOSE_FLAG>"
+ ARUINO_CLI_CMD_TOKEN = "<ARUINO_CLI_CMD>"
+ BOARD_TOKEN = "<BOARD>"
+ BUILD_EXTRA_FLAGS_TOKEN = "<BUILD_EXTRA_FLAGS>"
+
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ board: str,
Review Comment:
I changed all the high level API functions to explicitly capture the required/optional ProjectOption and pass it to the internal function. I think this way is more clear to set the requirement for each project option. Previously options were passed multiple levels and made it hard to track. thoughts?
--
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
[GitHub] [tvm] mehrdadh commented on pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on PR #12818:
URL: https://github.com/apache/tvm/pull/12818#issuecomment-1265804656
@areusch I think this is ready, PTAL. thanks!
--
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
[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
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [tvm] mehrdadh commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
mehrdadh commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r978981345
##########
python/tvm/micro/project_api/server.py:
##########
@@ -759,6 +764,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:
I'll fix this in a follow up PR if there was no other comments
--
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
[GitHub] [tvm] areusch commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r979013935
##########
apps/microtvm/arduino/template_project/microtvm_api_server.py:
##########
@@ -313,7 +297,90 @@ def _find_modified_include_path(self, project_dir, file_path, include_path):
# It's probably a standard C/C++ header
return include_path
+ CMSIS_INCLUDE_HEADERS = [
+ "arm_nn_math_types.h",
+ "arm_nn_tables.h",
+ "arm_nn_types.h",
+ "arm_nnfunctions.h",
+ "arm_nnsupportfunctions.h",
+ ]
+
+ def _cmsis_required(self, project_path: 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():
+ # Encoding is for reading C generated code which also includes hex numbers
+ 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 self.CMSIS_INCLUDE_HEADERS):
+ return True
+ return False
+
+ def _copy_cmsis(self, project_path: 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 self.CMSIS_INCLUDE_HEADERS:
+ shutil.copy2(
+ cmsis_path / "CMSIS" / "NN" / "Include" / item,
+ project_path / "include" / "cmsis" / item,
+ )
+
+ # These tokens are used in the Makefile.template file.
+ # They are replaced with proper value in generate_project step.
+ FQBN_TOKEN = "<FQBN>"
+ VERBOSE_FLAG_TOKEN = "<VERBOSE_FLAG>"
+ ARUINO_CLI_CMD_TOKEN = "<ARUINO_CLI_CMD>"
+ BOARD_TOKEN = "<BOARD>"
+ BUILD_EXTRA_FLAGS_TOKEN = "<BUILD_EXTRA_FLAGS>"
+
+ def _populate_makefile(
+ self,
+ makefile_template_path: pathlib.Path,
+ makefile_path: pathlib.Path,
+ board: str,
Review Comment:
yeah i agree there is a bit of a problem that everything depends on all options. typically this problem manifests itself in more complex codebases where refactoring becomes difficult because the member functions organically grow to the point they need interfaces, but interfaces are hard to define because the dependency net is too wide.
the drawback to factoring stuff out here is that it's possible to modify it, and then the meaning of a var could change depending on if it used the modified value or the original one present in `options`. for example, you access some vars as mandatory here and use `.get()` which means their value could be the var or None.
overall it's a tradeoff between these two. do you feel the code is sufficiently complex to need interfaces?
--
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
[GitHub] [tvm] areusch commented on a diff in pull request #12818: [microTVM] Use default Project Options in template projects and add Makefile for Arduino template project
Posted by GitBox <gi...@apache.org>.
areusch commented on code in PR #12818:
URL: https://github.com/apache/tvm/pull/12818#discussion_r980560302
##########
tests/micro/project_api/test_project_api.py:
##########
@@ -0,0 +1,104 @@
+# 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.
+
+import sys
+
+import tvm
+from tvm.micro.project_api import server
+
+API_GENERATE_PROJECT = "generate_project"
+API_BUILD = "build"
+API_FLASH = "flash"
+API_OPEN_TRANSPORT = "open_transport"
+
+PLATFORM_ARDUINO = "arduino"
+PLATFORM_ZEPHYR = "zephyr"
+
+
+platform = tvm.testing.parameter(PLATFORM_ARDUINO, PLATFORM_ZEPHYR)
+
+
+def test_default_options_exist(platform):
+ sys.path.insert(0, tvm.micro.get_microtvm_template_projects(platform))
+ import microtvm_api_server
+
+ platform_options = microtvm_api_server.PROJECT_OPTIONS
+ default_options = server.default_project_options()
+
+ option_names = []
+ for option in platform_options:
+ option_names.append(option.name)
+
+ for option in default_options:
+ assert option.name in option_names
+
+
+def test_default_options_requirements(platform):
+ sys.path.insert(0, tvm.micro.get_microtvm_template_projects(platform))
+ import microtvm_api_server
+
+ platform_options = microtvm_api_server.PROJECT_OPTIONS
+ for option in platform_options:
+ if option.name == "verbose":
+ assert option.optional == [API_GENERATE_PROJECT]
+ if option.name == "project_type":
+ option.required == [API_GENERATE_PROJECT]
+ if option.name == "board":
+ option.required == [API_GENERATE_PROJECT]
+ if platform == PLATFORM_ARDUINO:
+ option.optional == [API_FLASH, API_OPEN_TRANSPORT]
+ if option.name == "cmsis_path":
+ assert option.optional == [API_GENERATE_PROJECT]
+ if option.name == "compile_definitions":
+ assert option.optional == [API_GENERATE_PROJECT]
+ if option.name == "extra_files_tar":
+ assert option.optional == [API_GENERATE_PROJECT]
+
+
+def test_extra_options_requirements(platform):
Review Comment:
same with this one
##########
tests/micro/project_api/test_project_api.py:
##########
@@ -0,0 +1,104 @@
+# 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.
+
+import sys
+
+import tvm
+from tvm.micro.project_api import server
+
+API_GENERATE_PROJECT = "generate_project"
+API_BUILD = "build"
+API_FLASH = "flash"
+API_OPEN_TRANSPORT = "open_transport"
+
+PLATFORM_ARDUINO = "arduino"
+PLATFORM_ZEPHYR = "zephyr"
+
+
+platform = tvm.testing.parameter(PLATFORM_ARDUINO, PLATFORM_ZEPHYR)
+
+
+def test_default_options_exist(platform):
+ sys.path.insert(0, tvm.micro.get_microtvm_template_projects(platform))
+ import microtvm_api_server
+
+ platform_options = microtvm_api_server.PROJECT_OPTIONS
+ default_options = server.default_project_options()
+
+ option_names = []
+ for option in platform_options:
+ option_names.append(option.name)
+
+ for option in default_options:
+ assert option.name in option_names
+
+
+def test_default_options_requirements(platform):
Review Comment:
hm, i appreciate the intent behind this test, but i don't know we should submit it. this one is essentially a "change-detector test"--a test that fails when the codebase changes at all. i think we can have test coverage of these options via test_zephyr.py (essentially, just see if you can build/run with excluding all the optional options). i'm ok without exhaustively testing those in this PR, though.
--
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