Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[microTVM][TVMC] Add TVMC test for Arduino and Zephyr #9584

Merged
merged 3 commits into from
Dec 1, 2021
Merged

[microTVM][TVMC] Add TVMC test for Arduino and Zephyr #9584

merged 3 commits into from
Dec 1, 2021

Conversation

mehrdadh
Copy link
Member

This PR adds TVMC test for Arduino and Zephyr platforms and fixes few issues with project API and API servers.

cc @areusch @guberti @gromero

@leandron leandron self-assigned this Nov 25, 2021
gromero
gromero requested changes Nov 26, 2021
View reviewed changes
Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehrdadh Thanks a lot for adding these tests!

I've got some questions inline mainly about prepare_options but overall looks good.

apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
python/tvm/micro/project.py Show resolved Hide resolved
tests/micro/common/test_tvmc.py Show resolved Hide resolved
tests/micro/arduino/test_utils.py Outdated Show resolved Hide resolved
tests/micro/common/test_tvmc.py Outdated Show resolved Hide resolved
@gromero
Copy link
Contributor

gromero commented Nov 26, 2021

@mehrdadh think the following two tags to the PR (and to the commit if you're going to push force - if multiple commits exist the commit tile that lands is the PR's title, otherwise if it's a single commit the commit's title is used for landing): [microTVM] and [TVMC], case sensitive.

@mehrdadh mehrdadh changed the title [microtvm] Add TVMC test for Arduino and Zephyr [microTVM][TVMC] Add TVMC test for Arduino and Zephyr Nov 29, 2021
mehrdadh
mehrdadh commented Nov 29, 2021
View reviewed changes
Copy link
Member Author

@mehrdadh mehrdadh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gromero for your comprehensive review. I have address your comments, PTAL.

apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
python/tvm/micro/project.py Show resolved Hide resolved
tests/micro/arduino/test_utils.py Outdated Show resolved Hide resolved
tests/micro/common/test_tvmc.py Outdated Show resolved Hide resolved
tests/micro/common/test_tvmc.py Show resolved Hide resolved
@mehrdadh mehrdadh force-pushed the add_zephyr_tvmc_test branch 2 times, most recently from 083cbf8 to 830f1c1 Compare November 30, 2021 00:20
@mehrdadh mehrdadh mentioned this pull request Nov 30, 2021
Closed
@mehrdadh mehrdadh force-pushed the add_zephyr_tvmc_test branch from 830f1c1 to 53524e5 Compare November 30, 2021 00:33
gromero
gromero requested changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mehrdadh Thanks! LGTM except for one single fix that's necessary in arduino_cli_cmd option for the default metadatum, please see my comment inline. For Zephyr zephyr_base option the default is right.

apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/arduino/template_project/microtvm_api_server.py Show resolved Hide resolved
apps/microtvm/zephyr/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
areusch
areusch reviewed Nov 30, 2021
View reviewed changes
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mehrdadh just a couple comments here

apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
apps/microtvm/arduino/template_project/microtvm_api_server.py Outdated Show resolved Hide resolved
python/tvm/micro/project.py Show resolved Hide resolved
areusch
areusch approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@areusch areusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @mehrdadh , let's wait for @gromero to approve before merging

gromero
gromero approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@gromero gromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mehrdadh , LGTM.

leandron
leandron approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@leandron leandron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@areusch areusch merged commit 32e8012 into apache:main Dec 1, 2021
masahi pushed a commit to masahi/tvm that referenced this pull request Dec 1, 2021
@mehrdadh @masahi
[microTVM][TVMC] Add TVMC test for Arduino and Zephyr (apache#9584)
fbbc525 
* Add TVMC test for Arduino and Zephyr

* Address @gromero comments

* address comments
@mehrdadh mehrdadh deleted the add_zephyr_tvmc_test branch December 1, 2021 14:47
@gromero gromero mentioned this pull request Dec 7, 2021
Closed
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@mehrdadh @ylc
[microTVM][TVMC] Add TVMC test for Arduino and Zephyr (apache#9584)
0ce3f0f 
* Add TVMC test for Arduino and Zephyr

* Address @gromero comments

* address comments
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
@mehrdadh @yangulei
[microTVM][TVMC] Add TVMC test for Arduino and Zephyr (apache#9584)
64fb0b9 
* Add TVMC test for Arduino and Zephyr

* Address @gromero comments

* address comments
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
@mehrdadh @yangulei
[microTVM][TVMC] Add TVMC test for Arduino and Zephyr (apache#9584)
845a627 
* Add TVMC test for Arduino and Zephyr

* Address @gromero comments

* address comments
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@mehrdadh @ylc
[microTVM][TVMC] Add TVMC test for Arduino and Zephyr (apache#9584)
0a2c876 
* Add TVMC test for Arduino and Zephyr

* Address @gromero comments

* address comments
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gromero gromero gromero approved these changes

@areusch areusch areusch approved these changes

@leandron leandron leandron approved these changes

@comaniac comaniac Awaiting requested review from comaniac

@icemelon icemelon Awaiting requested review from icemelon

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@liangfu liangfu Awaiting requested review from liangfu

@manupak manupak Awaiting requested review from manupak

@merrymercy merrymercy Awaiting requested review from merrymercy

@tmoreau89 tmoreau89 Awaiting requested review from tmoreau89

@tqchen tqchen Awaiting requested review from tqchen

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

Assignees

@leandron leandron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mehrdadh @gromero @leandron @areusch