-
Notifications
You must be signed in to change notification settings - Fork 3k
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
static pin-map: patch for SerialBase class #12033
Conversation
This sounds like 5.15.0rc2 fix? cc @adbridge |
I think so. |
@mprse, thank you for your changes. |
Yes sounds like we should take this in |
860864b
to
54d3126
Compare
CI started |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
Ups...
|
@ARMmbed/team-nuvoton Can you review above ^^ ? The target seems to be on its limit. We will soon provide numbers in this PR (they should be low, so even prior this PR the size was close to the target limit?). |
Memory usage statistics for this patch can be found below:
|
54d3126
to
20f4ec3
Compare
Related PR: ARMmbed#10924 The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized. I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split. If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
20f4ec3
to
b6c25b1
Compare
Commit (b6c25b1) ensures that Serial Flow Control related functions (class members, HAL functions, pin-maps) are pulled into the image only if Flow Control is used.
Checked that this fix works also for |
For #12033 (comment), I cannot re-produce on my environment (IAR version has been the same). But according to the log, I create #12047 to try to address relevant error. Please inform me of the compile result regarding the above error. |
Have you enabled logging and stats? |
CI started while reviews are completed |
@ccli8 Thanks for checking this. Please run the following command to reproduce the failure: I got the following results on my side.
It looks like its maybe not related to the ROM usage. b6c25b1 should give extra 600 bytes of ROM savings and it does not help. It's even worse: |
Test run: FAILEDSummary: 1 of 4 test jobs failed Failed test jobs:
|
@mprse I can re-produce the error now. It is caused by static SRAM OOM on IAR, on which heap configuration cannot be adjusted according to static SRAM usage. I will tune heap configuration on IAR to fix the issue, though it is resource limit issue. |
@0xc0170 |
@ccli8 @hugueskamba I confirm that reducing HEAP or BOOT STACK SIZE in |
I update #12047 to fix #12033 (comment) on NUMAKER_PFM_NANO130/IAR:
|
@0xc0170 is this ready for CI again now ? |
@ccli8 |
@hugueskamba I suggested to start using dynamic heap (minimum size, so should be possible to get it lower). The PR fixes the issue but another PR should make it future proof. PR integrated, restarting CI |
Test run: FAILEDSummary: 1 of 11 test jobs failed Failed test jobs:
|
@mprse @0xc0170 @hugueskamba I create #12061 to add IAR dynamic heap configuration support to NANO130. |
No failed test, but |
Restarted tests, the boards had some issues yesterday |
Summary of changes
Related PR: #10924
The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.
I missed this change while working on the static pin-map and unfortunately it has an impact on it. The reinitialization is a problem for static pin-map. Now the HAL
init()/init_direct()
function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pin-map constructor was used to create an object (andinit_direct()
HAL API), when reinitialization is done it usesinit()
HAL API. This must be split.If static pinmap constructor is used, then the peripheral must be always initialized using HAL
init_direct()
function. If the regular constructor is used, then the peripheral must be initialized using HALinit()
function. The same split also must be done while setting flow control during reinitialization.Documentation
Pull request type
Test results
Reviewers
@jamesbeyond
@kjbracey-arm
@0xc0170