Remove printf completely and fix the optimization check #9296

SenRamakri · 2019-01-08T21:08:40Z

Description

printf call from mbed_error.c has been causing elevated rom usage for GCC builds for develop profile. This change refactor the code to use mbed_error_print before rebooting to avoid using printf. Also contains a fix for optimization check.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@bulislaw

0xc0170 · 2019-01-09T14:46:05Z

CI started

mbed-ci · 2019-01-10T04:04:50Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

0xc0170 · 2019-01-10T12:06:35Z

There are two devices failures, we will restart the test later today to make sure they are related to this changes . Please review

cmonr · 2019-01-10T16:16:01Z

Restarted jenkins-ci/greentea-test job.

cmonr · 2019-01-10T20:50:54Z

@SenRamakri Please take a look at the following logs: http://mbed-os-ci.s3-website-eu-west-1.amazonaws.com/?prefix=jenkins-ci/artifacts/9296/1/greentea-test/766/

The rerun failed a subset of test that originally failed.

SenRamakri · 2019-01-11T22:07:23Z

@cmonr - Thanks for the info. The failures in K64F doesn't seem to be related to my changes. It appears to be some random failures. The test failures on 746ZG and 429ZI is related to crash-reporting but unfortunately I'm not able to reproduce the failure. Both IAR/GCC_ARM works just fine at my desk including the test which is timing-out in CI. I'm continuing to look into this though.

SenRamakri · 2019-01-11T23:15:59Z

The test which is timing out is "crash_reporting" test. This test includes a host side scriptl which sends a message to the device to start the test. If the message goes out from host before the device starts listening for the message we may see timeouts like this. That's the only reason I can think as I see from the log that the host did send out that message but device acted like it never received it. I'm going to increase the time host side waits for the device to come up and then send the message. I'll try pushing this change and see if that helps.

SenRamakri · 2019-01-13T01:34:36Z

@ARMmbed/mbed-os-maintainers - Can you please start CI on this?

0xc0170 · 2019-01-14T10:37:13Z

CI started

mbed-ci · 2019-01-14T14:24:05Z

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

SenRamakri · 2019-01-14T18:52:14Z

@ARMmbed/mbed-os-maintainers - FYI: It appears greentea tests failed across all targets for some reason. Looks like its unrelated to the commit.

0xc0170 · 2019-01-15T08:46:52Z

Test restarted

0xc0170 · 2019-01-15T12:15:29Z

Restarted and failures are similar - isn't this related ? mbed_platform-crash_reporting is failing for few targets

SenRamakri · 2019-01-15T16:21:10Z

@0xc0170 - mbed_platform-crash_reporting failure is related. But the challenge is I'm not able to reproduce this at my desk. I'm going to seek help from CI/Test team to see if we can formulate something to reproduce this at desk.

OPpuolitaival · 2019-01-17T15:03:00Z

greentea test restarted

0xc0170 · 2019-01-23T09:47:45Z

CI was restarted

mbed-ci · 2019-01-23T13:16:41Z

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 4
Build artifacts

0xc0170 · 2019-01-23T13:19:28Z

@SenRamakri Tests passed with debug msgs, please review

SenRamakri · 2019-01-23T17:52:50Z

@0xc0170 , @cmonr - I have reviewed the test logs and see that all tests pass, and my debug messages confirm that tests did not timeout and worked as expected. Interestingly the changes did not fix anything in particular and I can see there may be some timing change associated with this which is helping to overcome some test issues in CI. At this point, I would like to keep these extra debug messages to debug issues in future in case the tests starts timing-out again and also due to the fact that the tests were randomly failing previously across different targets. So, after monitoring it for next few weeks/months we can remove these extra messages. If this is ok, we can merge this commit as it addresses some important memory issues. I'm also confused about the dynamic-memory-change reporting +192 bytes. When I compiled a tiny app(without any printf) with latest mbed-os/master I see
Total Flash memory (text + data): 59702(+59702) bytes
and with my branch its
Total Flash memory (text + data): 54012(+54012) bytes. (less by few Kbs)
So, I don't know reason for that +192 bytes increase reported here.

SenRamakri · 2019-01-23T23:37:50Z

Ok, I did more analysis and I found the reason for +192 bytes reported by dynamic-memory-test. First of all, the dynamic-memory-test always build release profile and thus it never sees the 5K difference we are trying to address here for develop builds. In addition the new changes of moving the mbed_error_printf() added some extra stuff for release builds which probably caused +192b. As of now, I have fixed that with latest commit which should cut-down on that +192bytes increase for release builds.

@ARMmbed/mbed-os-maintainers - Please restart CI on this.

cmonr · 2019-01-24T06:15:23Z

CI started

mbed-ci · 2019-01-24T09:40:13Z

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 5
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

SenRamakri · 2019-01-24T16:54:55Z

@ARMmbed/mbed-os-maintainers - The tests passed and with the latest commit the memory increase has also been optimized. The failure with NUCLEO_F429(in the previous run) seems to be some reporting issue as I see from the logs that tests actually passed. I think this PR can be merged now.

janjongboom · 2019-01-24T19:50:18Z

This PR did not solve my issue. Without MBED_FAULT_HANDLER_DISABLED declared and no printf calls in my code I see 5.2K flash increase and 712 bytes static RAM increase.

JanneKiiskila · 2019-01-28T12:17:24Z

@SenRamakri - did you notice Jan's comment?

SenRamakri · 2019-01-28T16:12:03Z

@JanneKiiskila - Yes, we did and it was already responded, please see - #9464 (comment).

pan- · 2019-01-29T11:05:33Z

TESTS/host_tests/crash_reporting.py

@@ -60,7 +60,7 @@ def test_steps(self):
        wait_after_reset = wait_after_reset if wait_after_reset is not None else DEFAULT_CYCLE_PERIOD

        #Wait 2 seconds for system to init
-        time.sleep(2.0)
+        time.sleep(7.0)


The comment above should have been fixed before merge.

@SenRamakri Please review

SenRamakri requested a review from bulislaw January 8, 2019 21:08

kjbracey approved these changes Jan 9, 2019

View reviewed changes

0xc0170 approved these changes Jan 9, 2019

View reviewed changes

0xc0170 added release-version: 5.11.2 needs: CI labels Jan 9, 2019

bulislaw approved these changes Jan 10, 2019

View reviewed changes

cmonr added needs: work and removed needs: CI release-version: 5.11.2 labels Jan 10, 2019

0xc0170 added needs: CI and removed needs: work labels Jan 14, 2019

0xc0170 added the release-version: 5.11.3 label Jan 14, 2019

cmonr added needs: work and removed needs: CI release-version: 5.11.3 labels Jan 17, 2019

0xc0170 removed the needs: work label Jan 23, 2019

0xc0170 mentioned this pull request Jan 23, 2019

Huge increase (5K flash, 700B static RAM) between 5.10 and 5.11 #9464

Closed

0xc0170 added the release-version: 5.11.3 label Jan 23, 2019

0xc0170 added ready for merge needs: work and removed needs: CI ready for merge labels Jan 23, 2019

Print crash report reboot messages only for non-release builds

d0b9503

SenRamakri force-pushed the sen_RemovePrintfCrashReport branch from fc6a1f9 to d0b9503 Compare January 23, 2019 23:40

cmonr added needs: review needs: CI and removed needs: work needs: review labels Jan 24, 2019

0xc0170 added ready for merge and removed needs: CI labels Jan 24, 2019

cmonr merged commit 2941c8d into ARMmbed:master Jan 24, 2019

cmonr removed the ready for merge label Jan 24, 2019

pan- reviewed Jan 29, 2019

View reviewed changes

kjbracey mentioned this pull request Jan 30, 2019

Disable reboot limit #9545

Closed

Remove printf completely and fix the optimization check #9296

Remove printf completely and fix the optimization check #9296

Uh oh!

Conversation

SenRamakri commented Jan 8, 2019

Description

Pull request type

Reviewers

Uh oh!

0xc0170 commented Jan 9, 2019

Uh oh!

mbed-ci commented Jan 10, 2019

Test run: FAILED

Uh oh!

0xc0170 commented Jan 10, 2019

Uh oh!

cmonr commented Jan 10, 2019

Uh oh!

cmonr commented Jan 10, 2019

Uh oh!

SenRamakri commented Jan 11, 2019

Uh oh!

SenRamakri commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SenRamakri commented Jan 13, 2019

Uh oh!

0xc0170 commented Jan 14, 2019

Uh oh!

mbed-ci commented Jan 14, 2019

Test run: FAILED

Uh oh!

SenRamakri commented Jan 14, 2019

Uh oh!

0xc0170 commented Jan 15, 2019

Uh oh!

0xc0170 commented Jan 15, 2019

Uh oh!

SenRamakri commented Jan 15, 2019

Uh oh!

OPpuolitaival commented Jan 17, 2019

Uh oh!

0xc0170 commented Jan 23, 2019

Uh oh!

mbed-ci commented Jan 23, 2019

Test run: SUCCESS

Uh oh!

0xc0170 commented Jan 23, 2019

Uh oh!

SenRamakri commented Jan 23, 2019

Uh oh!

SenRamakri commented Jan 23, 2019

Uh oh!

cmonr commented Jan 24, 2019

Uh oh!

mbed-ci commented Jan 24, 2019

Test run: FAILED

Uh oh!

SenRamakri commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janjongboom commented Jan 24, 2019

Uh oh!

JanneKiiskila commented Jan 28, 2019

Uh oh!

SenRamakri commented Jan 28, 2019

Uh oh!

pan- Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jan 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SenRamakri commented Jan 11, 2019 •

edited

Loading

SenRamakri commented Jan 24, 2019 •

edited

Loading