[microTVM] Use default Project Options in template projects and add Makefile for Arduino template project #12818

mehrdadh · 2022-09-16T17:14:51Z

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
Adds CMSIS support to Arduino template project
Refactors zephyr_board and arduino_board to board

guberti

I like the idea of adding a Makefile to the Arduino project, but I would really love more documentation about how to use it (full disclosure - I've never been great with make 😅). I'd love some usage examples both in the Makefile.template and in a README.md file.

I've left some other comments as well - thanks for the change!

guberti · 2022-09-20T08:04:16Z

apps/microtvm/arduino/template_project/Makefile.template

+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>


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?

I removed MAKE_DIR, we don't need that
Also added more info on each variable

apps/microtvm/arduino/template_project/Makefile.template

apps/microtvm/arduino/template_project/microtvm_api_server.py

python/tvm/micro/project_api/server.py

guberti · 2022-09-20T08:52:26Z

python/tvm/micro/project_api/server.py

+    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))


Could you add a comment explaining what these lines are doing?

these are updating the default options and I think the function string explains it

mehrdadh

@guberti thanks for the review! PTAL

apps/microtvm/arduino/template_project/microtvm_api_server.py

python/tvm/micro/project_api/server.py

mehrdadh · 2022-09-21T17:07:39Z

python/tvm/micro/project_api/server.py

+    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))


these are updating the default options and I think the function string explains it

apps/microtvm/arduino/template_project/Makefile.template

guberti

Two small nits, but looks good. Thanks for the change!

guberti · 2022-09-21T18:27:51Z

python/tvm/micro/project_api/server.py

+    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))


possible nit:

Suggested change

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))

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)

that doesn't work because the kw is a list of suggested changes but option has ProjectOption type

Sorry, this should be if option.name in kw. Fixed!

I'll fix this in a follow up PR if there was no other comments

guberti · 2022-09-21T18:28:28Z

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):


what are your thoughts on the typing stuff here?

areusch

thanks @mehrdadh ! cc @gromero

areusch · 2022-09-22T19:57:11Z

apps/microtvm/arduino/template_project/microtvm_api_server.py

+
+    # These tokens are used in the Makefile.template file.
+    # They are replaced with proper value in generate_project step.
+    FQBN_TOKEN = "<FQBN>"


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)

areusch · 2022-09-22T19:58:26Z

apps/microtvm/arduino/template_project/microtvm_api_server.py

+        self,
+        makefile_template_path: pathlib.Path,
+        makefile_path: pathlib.Path,
+        board: str,


why not accept options dict here? it might make it a little easier to move code around in this file

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?

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?

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:
aa13413#diff-f461d1a4c216e770f22b27fc7926b7fd31cde6273b1e15f9d5d08ec2892ec251R1

mehrdadh · 2022-09-23T16:43:19Z

@tvm-bot rerun

mehrdadh · 2022-09-23T16:44:23Z

@guberti @areusch PTAL, thanks!

areusch

last couple things here--apologies for the delay

areusch · 2022-09-26T22:33:15Z

tests/micro/project_api/test_project_api.py

+        assert option.name in option_names
+
+
+def test_default_options_requirements(platform):


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.

thanks! I updated the test.

areusch · 2022-09-26T22:33:45Z

tests/micro/project_api/test_project_api.py

+            assert option.optional == [API_GENERATE_PROJECT]
+
+
+def test_extra_options_requirements(platform):


same with this one

mehrdadh · 2022-10-03T17:38:09Z

@areusch I think this is ready, PTAL. thanks!

…akefile for Arduino template project (apache#12818) * add default options * add updat * add more options * add makefile for arduino * fixed flash command * add arduino-cli flag option * cleanup * lint * lint and test added * lint * fix test * fix flash command * fix test, add board to makefile * fix test * fix flash and build command * add cmsis support * address comments * lint * address comments, refactor two more options * change build.extra_flags to be empty when no flag added * fix test * address comments * add test for project options * update test

github-actions bot requested review from areusch and gromero September 16, 2022 17:15

mehrdadh force-pushed the micro/update_project_options branch 3 times, most recently from f8848c0 to 3c476d1 Compare September 19, 2022 23:45

guberti requested changes Sep 20, 2022

View reviewed changes

mehrdadh commented Sep 21, 2022

View reviewed changes

guberti approved these changes Sep 21, 2022

View reviewed changes

mehrdadh force-pushed the micro/update_project_options branch from 2022a91 to 014fa3b Compare September 21, 2022 22:38

areusch reviewed Sep 22, 2022

View reviewed changes

areusch reviewed Sep 26, 2022

View reviewed changes

mehrdadh added 17 commits September 29, 2022 12:05

add default options

99e7e66

add updat

ab0905f

add more options

6d8a143

add makefile for arduino

c685e45

fixed flash command

855e307

add arduino-cli flag option

048255c

cleanup

bd233e1

lint

73b2006

lint and test added

e867595

lint

d7886dc

fix test

3ae8dca

fix flash command

3a206ab

fix test, add board to makefile

a7f7f0f

fix test

90d522d

fix flash and build command

601369f

add cmsis support

b507c9d

address comments

a8f6d8e

mehrdadh added 7 commits September 29, 2022 12:05

lint

cc514f8

address comments, refactor two more options

1ed547a

change build.extra_flags to be empty when no flag added

da4bff7

fix test

31a348e

address comments

88adaec

add test for project options

71c32de

update test

3ffd38b

mehrdadh force-pushed the micro/update_project_options branch from aa13413 to 3ffd38b Compare September 29, 2022 22:06

areusch approved these changes Oct 4, 2022

View reviewed changes

areusch merged commit 61a7632 into apache:main Oct 4, 2022

mehrdadh deleted the micro/update_project_options branch January 20, 2023 22:25

leandron mentioned this pull request Feb 1, 2023

TVM v0.11.0 Release Candidate Notes #13899

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[microTVM] Use default Project Options in template projects and add Makefile for Arduino template project #12818

[microTVM] Use default Project Options in template projects and add Makefile for Arduino template project #12818

mehrdadh commented Sep 16, 2022 •

edited

Loading

guberti left a comment

guberti Sep 20, 2022

mehrdadh Sep 21, 2022

guberti Sep 20, 2022

mehrdadh Sep 21, 2022

mehrdadh left a comment

mehrdadh Sep 21, 2022

guberti left a comment

guberti Sep 21, 2022 •

edited

Loading

mehrdadh Sep 21, 2022

guberti Sep 23, 2022

mehrdadh Sep 23, 2022

guberti Sep 21, 2022 •

edited

Loading

areusch left a comment

areusch Sep 22, 2022

mehrdadh Sep 22, 2022

mehrdadh Sep 22, 2022

areusch Sep 22, 2022

mehrdadh Sep 22, 2022

areusch Sep 23, 2022

mehrdadh Sep 23, 2022

mehrdadh commented Sep 23, 2022

mehrdadh commented Sep 23, 2022

areusch left a comment

areusch Sep 26, 2022

mehrdadh Sep 29, 2022

areusch Sep 26, 2022

mehrdadh commented Oct 3, 2022

		assert option.name in option_names


		def test_default_options_requirements(platform):

		assert option.optional == [API_GENERATE_PROJECT]


		def test_extra_options_requirements(platform):

[microTVM] Use default Project Options in template projects and add Makefile for Arduino template project #12818

[microTVM] Use default Project Options in template projects and add Makefile for Arduino template project #12818

Conversation

mehrdadh commented Sep 16, 2022 • edited Loading

guberti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guberti left a comment

Choose a reason for hiding this comment

guberti Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guberti Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadh commented Sep 23, 2022

mehrdadh commented Sep 23, 2022

areusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehrdadh commented Oct 3, 2022

mehrdadh commented Sep 16, 2022 •

edited

Loading

guberti Sep 21, 2022 •

edited

Loading

guberti Sep 21, 2022 •

edited

Loading