-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[microTVM] Refactor required external functions in CRT to platform-template.c #13885
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.
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
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
93fba20 to
7e7df14
Compare
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.h
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.cc
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.cc
Outdated
Show resolved
Hide resolved
mehrdadh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanmacd thanks for the review! I addressed them but haven't pushed since I'm waiting to see the result of ci_cortexm tests in case anything is broken
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.cc
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.cc
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.h
Outdated
Show resolved
Hide resolved
mkatanbaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mehrdadh, I left a few comments.
apps/microtvm/zephyr/template_project/src/host_driven/platform.h
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/aot_standalone_demo/platform.c
Outdated
Show resolved
Hide resolved
| #include <tvm/runtime/crt/stack_allocator.h> | ||
|
|
||
| // Used to read data from the UART. | ||
| extern tvm_workspace_t app_workspace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to place the prototypes of the functions defined in platform.c here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdym?
| // Called by TVM when a message needs to be formatted. | ||
| size_t TVMPlatformFormatMessage(char* out_buf, size_t out_buf_size_bytes, const char* fmt, | ||
| va_list args) { | ||
| return vsnprintk(out_buf, out_buf_size_bytes, fmt, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this function to platform.c?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.cc
Outdated
Show resolved
Hide resolved
apps/microtvm/arduino/template_project/src/example_project/platform.c
Outdated
Show resolved
Hide resolved
apps/microtvm/arduino/template_project/src/host_driven/platform.c
Outdated
Show resolved
Hide resolved
apps/microtvm/arduino/template_project/src/host_driven/platform.c
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/aot_standalone_demo/platform.c
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/aot_standalone_demo/platform.c
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/host_driven/platform.h
Outdated
Show resolved
Hide resolved
apps/microtvm/zephyr/template_project/src/mlperftiny/platform.h
Outdated
Show resolved
Hide resolved
mkatanbaf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left a few minor comments.
areusch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @mehrdadh ! the main question i have here is around making TVMPlatform functions weak--i don't think we should do that, unless i'm missing something. would love to hear others' opinions
|
|
||
| tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) { | ||
| // Fill a buffer with random data. | ||
| __attribute__((weak)) tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how come these are weak? if you miss implementing one, could you accidentally link the default and then wind up with two implementations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea here is that platform.c has one implementation of these functions which is added by default to the project. But in case the user wants to change the implementation, they could reimplement it in their main.c
That's why it is defined as weak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of these being weak - I think allowing the user to change the implementation is worth the increased complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| g_microtvm_timer_running = 1; | ||
| return kTvmErrorNoError; | ||
| // Initialize UART | ||
| void UARTInit() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use consistent caps (UART vs Uart; i prefer Uart because the string parses easier)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| void main(void) { | ||
| UARTInit(); | ||
| TVMPlatformInitialize(); | ||
| g_cmd_buf_ind = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initialize the Uart after you are ready to receive stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UartInit initialize both transmit and receive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what @areusch means here (and correct me if I'm wrong) is to init Uart once the platform (including memory, rpc for example) is initialized. Otherwise, the received data might drop or even cause errors (by trying to write into memory spaces that are not allocated yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I changed the order.
apps/microtvm/zephyr/template_project/src/aot_standalone_demo/platform.c
Show resolved
Hide resolved
guberti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits, but looks good to me!
|
|
||
| tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) { | ||
| // Fill a buffer with random data. | ||
| __attribute__((weak)) tvm_crt_error_t TVMPlatformGenerateRandom(uint8_t* buffer, size_t num_bytes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of these being weak - I think allowing the user to change the implementation is worth the increased complexity.
apps/microtvm/zephyr/template_project/src/mlperftiny/submitter_implemented.cc
Outdated
Show resolved
Hide resolved
| // ############################################################### | ||
| // Model | ||
| // ############################################################### | ||
| #define MODEL_KWS 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defining these constants is a really nice improvement!
dae1733 to
c132e9b
Compare
c132e9b to
ab944ab
Compare
This PR refactors all the necessary functions for a CRT project into
platform-template.cfile. Using this PR the content of CRT template project is as follow:The content of platform-template.c is as follows. This file shows the minimum implementation that is required for using a microTVM generated MLF file in a different environment.
Using the same idea, various project types for Zephyr and Arduino was refactored.
In addition, these templates files are also added along with
crt_config-template.h(which was accidentally removed in #13812) to generated model library format (MLF). Here is the updated structure of a MLF file: