-
Notifications
You must be signed in to change notification settings - Fork 3k
Add new target NUMAKER_PFM_NANO130 #4631
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
1. Remove RTX_CM_lib.h modification for NANO130 platform 2. Remove semaphore test code's speific modification for NANO130 platform 3. Fix duplicate definitions of __NVIC_SetVector/__NVIC_GetVector 4. Add spi_master_block_write()
1. Support CMSIS_VECTAB_VIRTUAL feature 2. Reduce the register sync waiting time in LP ticker 3. Adjust the stack and heap size in GCC and IAR toolchains
@cyliangtw Could you remove the |
@theotherjimmy, yes, I removed |
tools/toolchains/arm.py
Outdated
@@ -162,6 +162,10 @@ def compile(self, cc, source, object, includes): | |||
|
|||
cmd.extend(self.get_dep_option(object)) | |||
|
|||
# Remove '.\' from relative path names of source files. For example, '.\mbed-os\platform\retarget.cpp' will truncate to 'mbed-os\platform\retarget.cpp'. |
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 believe this shall be sent as separate patch
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.
Fixed in commit 127a09c
targets/targets.json
Outdated
"is_disk_virtual": true, | ||
"supported_toolchains": ["ARM", "uARM", "GCC_ARM", "IAR"], | ||
"inherits": ["Target"], | ||
"progen": {"target": "numaker-pfm-nano130"}, |
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.
this line (2922) can be removed, not used anymore
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.
Fixed in commit 348fa75
@@ -19,7 +19,7 @@ | |||
|
|||
#include <stdint.h> | |||
|
|||
#if defined(TARGET_NUMAKER_PFM_NUC472) | |||
#if defined(TARGET_NUVOTON) |
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 have seen this change in another PR to add another nuvoton target - I dont see any dependency, so this one might state it and will be rebased ?
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.
In both of these 2 PR NANO130 & M487, they have the same mbed_rtx.h .
#define TMR_CMP_MIN 2 | ||
#define TMR_CMP_MAX 0xFFFFFFu | ||
|
||
void us_ticker_init(void) |
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 can see lot of copy paste in nuvoton targets. I was reviewing tickers the last week, are they using the same peripheral with only slight changes? Is there a chance all these would be unified?
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.
Yes, we have the plan to review the unchanged part of chip series of our existing platforms and future platforms, then the unified code will be possible.
PDMA_SAR_INC, // Source address incremental | ||
(uint32_t) NU_MODBASE(obj->serial.uart), // Destination address | ||
PDMA_DAR_FIX); // Destination address fixed | ||
#if 0 // NOTE: |
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.
dead code ,pleas remove
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.
Fixed in commit 2f8e3fb
|
||
#include "cmsis.h" | ||
|
||
// NOTE: Ensurce mbed_sdk_init() will get called before C++ global object constructor. |
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.
is this needed? I believe not
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.
Yes, it's needed. The cmsis.h
will include target specific header files for mbed_overrides.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.
I was referring to the line below, 19 lines and below, regarding C++ global object constructors. I would say these lines can be removed?
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.
Remove the unused code of mbed_sdk_init_forced() in commit 3bbacad.
|
||
uint32_t pin_index = NU_PINNAME_TO_PIN(pin); | ||
|
||
#if 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.
dead code pls remove from any file 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.
Fixed in commit 2f8e3fb
void (*vec)(void); | ||
}; | ||
|
||
// NOTE: NANO130 doesn't support relocating vector table. ISR vector passed into NVIC_SetVector() can only be weak symbol defined in startup_Nano100Series.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.
how does this work? no VTOR, thus how is it working ?
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.
Though not support VTOR
at M0, for keep the same architecture, we implemented vec
to point to the same original embedded flash.
@ccchang12
return (elapsed >= timeout); | ||
} | ||
|
||
#if 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.
to be removed
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.
Fixed in commit 2f8e3fb
/morph export-build |
/morph-test |
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 112 Exporter Build failed! |
@0xc0170 , It shows the result of "/morph export-build" is failure yesterday, but the link of job/exporter_build_test/112/ cannot be found. How can I check the output message to fix the issue? |
/morph test |
Result: FAILUREYour command has finished executing! Here's what you wrote!
OutputBuild failed! |
This one from the last run : build_matrix » NUMAKER_PFM_NANO130,IAR completed with result FAILURE The previous export build was due to master issue that was fixed yesterday, we will rerun once the above failure is fixed, please review |
@0xc0170 , modify the .icf file of IAR toolchain to fix the build error. In original code, the total uncommitted space is 0x3800. But in FAIL result log, the TESTS-MBED_DRIVERS-C_STRINGS allocates space for sections/blocks with size of 0x3810 bytes. Now enlarge the total uncommitted space to be 0x3a00 to fix the issue. But how we can duplicate this issue? After merge with master, the allocates space of TESTS-MBED_DRIVERS-C_STRINGS is only 0x3640 bytes, not 0x3810 bytes. So we cannot duplicate this issue in our environment. From the log file, I saw the "morph test" includes "-DMBED_TRAP_ERRORS_ENABLED=1", "-DMBED_STACK_STATS_ENABLED=1" and "-DMBED_HEAP_STATS_ENABLED=1" options. Is it the root cause? If yes, how we can enable these options? |
Compile with this defines, and run greentea to point to the binary file (2 step process). You should be able to reproduce it. These 2 commands you can edit to get the output you need:
|
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
OutputAll builds and test passed! |
/morph export-build |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 125 All exports and builds passed! |
@cyliangtw This commit: bc0fc2e |
@adbridge , thanks of your advice, we will follow your rule to avid too many individual commits & do rebase before send one PR. |
@cyliangtw We actually encourage having many commits, so long as they are a logical section of work. @adbridge was trying to tell you not to do merge commits in PRs (created by |
@theotherjimmy Thanks of your explanation, our original purpose of |
Description
This PR is to add support for Nuvoton's new target NUMAKER_PFM_NANO130.
Greentea Test
All pass: attached the test log files of mbed Greentea test at ARMCC, GCC, IAR & uARM toolchain,
os55_greentea_GCC.txt
os55_greentea_IAR.txt
os55_greentea_uARM.txt
os55_greentea_ARM.txt
The snippet of GCC test log as below: