[RFC] Use minimal printf by default inside the OS #12168

bulislaw · 2019-12-24T15:34:29Z

Summary of changes

Replace stdlib printf family calls with minimal printf inside Mbed OS core. System components use printf family function in many places internally. Using minimal printf allows us to to save over 15k flash in basic use case. It doesn't affect users as by default it doesn't do mapping from mbed_printf to printf.
In users or other system component will use stdlib printf family it will pull both versions of printf, but the dependencies pulled will be reduced (depending on the explicit library printf usage) resulting in overall size decrease of around 6k.

I'm raising this PR as a work in progress RFC, because I want to start discussion to move the whole system to using minimal printf internally.

Open actions

Should we make mbed_printf conditionally expand either to stdlib or minimal printf? Probably not.
Docs
Tests
Think about simplifying printing/logging infrastructure. Currently we have:

mbed_printf
printf
debug
error
mbed_warning
mbed_error
mbed_error_printf
MBED_PRINTF
tr_debug
tr_info
tr_warn
tr_err

stdlib assert pulls fiprintf. Assert is mostly used in external code like partner's SDKs
minimal-printf by default doesn't include fp, it's a configuration choice in conflict with it's README (which states the opposite). We need to either switch it on or change the README.

For simple RTOS example compiled in release
Master:

Total Static RAM memory (data + bss): 12608(+0) bytes
Total Flash memory (text + data): 47668(+0) bytes

PR:

Total Static RAM memory (data + bss): 12624(+16) bytes
Total Flash memory (text + data): 31500(-16168) bytes

@kjbracey-arm @AnttiKauppila @SeppoTakalo @evedon

Impact of changes

Migration actions required

Documentation

ToDo

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[x] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

ciarmcom · 2019-12-24T16:00:28Z

@bulislaw, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-core please review.

mbed-ci · 2019-12-24T16:11:19Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests
jenkins-ci/mbed-os-ci_build-IAR

mbed-ci · 2019-12-27T10:36:07Z

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests
jenkins-ci/mbed-os-ci_build-IAR

evedon · 2019-12-27T15:41:01Z

It is a good idea to move mbed-os and examples to use minimal printf.

Should we make mbed_printf conditionally expand either to stdlib or minimal printf?

Why expand to stdlib? This is extra complexity and does not bring anything, unless we are using format specifiers not supported by minimal printf.

0Grit · 2019-12-27T19:48:54Z

@AGlass0fMilk @trowbridgec

bulislaw · 2019-12-30T10:15:21Z

Should we make mbed_printf conditionally expand either to stdlib or minimal printf?
Why expand to stdlib? This is extra complexity and does not bring anything, unless we are using format specifiers not supported by minimal printf.

In an unlikely scenario when you are using std printf in your app you could save 1-1.5k by switching off minimal-printf, considering you are already pulling printf and its dependencies. But you're probably right, no point complicating things any more.

I'm more worried about the 10 different ways of printing in Mbed and the most obvious one (printf) being the most expensive one.

hugueskamba · 2019-12-30T10:23:49Z

drivers/I2C.h

@@ -74,7 +74,7 @@ namespace mbed {
 *         i2c.read( addr8bit, cmd, 2);
 *
 *         float tmp = (float((cmd[0]<<8)|cmd[1]) / 256.0);
- *         printf("Temp = %.2f\n", tmp);
+ *         mbed_printf("Temp = %.2f\n", tmp);


Floating point support for minimal-printf is disabled by default.

Then the minimal-printf README is wrong:

* %f: floating point (enabled by default). * %F: floating point (enabled by default, treated as %f). * %g: floating point (enabled by default, treated as %f). * %G: floating point (enabled by default, treated as %f).

I'll add it to things to consider in this PR.

Yes it is wrong: https://github.com/ARMmbed/mbed-os/blob/master/platform/mbed_lib.json#L146

There is a PR to fix it.

bulislaw · 2019-12-30T17:40:49Z

@geky I changed all the printf family calls in littlefs to mbed_printf (that is minimal-printf). But considering that we only host a copy maybe it's not the best way and adding some abstraction over it would be better. Thoughts?

mbed-ci · 2020-01-03T12:49:59Z

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_unittests

Use the same configuration as for Unix as these are broadly compatible.

It matters for 64b platforms like x86 for unit testing.

bulislaw · 2020-01-07T14:20:22Z

After some discussions I've decided to close this ticket and pursue different path. I'll enable use of minimal-printf as a default implementation of printf* family calls in Mbed OS. Looking on how fiddly it would be to replace all the occurrences of printf* family and prevent new ones creeping in and it still wouldn't guarantee users not using it without knowing the cost.

Stay tuned for the new PR.

bulislaw added do not merge needs: review labels Dec 24, 2019

ciarmcom requested review from a team December 24, 2019 16:00

hugueskamba reviewed Dec 30, 2019

View reviewed changes

bulislaw force-pushed the minimal_printf_internal branch from 43ccdf8 to 13ad49c Compare January 6, 2020 15:23

bulislaw added 7 commits January 7, 2020 13:49

Add sprintf to minimal-printf implementation

dbe9749

Use minimal printf by default inside the OS

6d55841

Squash

ba30712

Port storege to use minimal-printf

c60c71b

Enable equeue to compile on Mac

d9e39f1

Use the same configuration as for Unix as these are broadly compatible.

Add minimal-printf files to UNITTESTs

cb2caf9

Unify max size to INT_MAX

4d347b8

It matters for 64b platforms like x86 for unit testing.

bulislaw force-pushed the minimal_printf_internal branch from 13ad49c to 4d347b8 Compare January 7, 2020 14:08

bulislaw closed this Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RFC] Use minimal printf by default inside the OS #12168

[RFC] Use minimal printf by default inside the OS #12168

Uh oh!

bulislaw commented Dec 24, 2019 •

edited

Loading

Uh oh!

ciarmcom commented Dec 24, 2019

Uh oh!

mbed-ci commented Dec 24, 2019

Uh oh!

mbed-ci commented Dec 27, 2019

Uh oh!

evedon commented Dec 27, 2019

Uh oh!

0Grit commented Dec 27, 2019

Uh oh!

bulislaw commented Dec 30, 2019

Uh oh!

hugueskamba Dec 30, 2019

Uh oh!

bulislaw Dec 30, 2019

Uh oh!

bulislaw Dec 30, 2019

Uh oh!

hugueskamba Dec 30, 2019 •

edited

Loading

Uh oh!

bulislaw commented Dec 30, 2019

Uh oh!

mbed-ci commented Jan 3, 2020

Uh oh!

bulislaw commented Jan 7, 2020

Uh oh!

Uh oh!

[RFC] Use minimal printf by default inside the OS #12168

[RFC] Use minimal printf by default inside the OS #12168

Uh oh!

Conversation

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

Uh oh!

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Dec 24, 2019

Uh oh!

mbed-ci commented Dec 24, 2019

Test run: FAILED

Uh oh!

mbed-ci commented Dec 27, 2019

Test run: FAILED

Uh oh!

evedon commented Dec 27, 2019

Uh oh!

0Grit commented Dec 27, 2019

Uh oh!

bulislaw commented Dec 30, 2019

Uh oh!

hugueskamba Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

bulislaw Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

bulislaw Dec 30, 2019

Choose a reason for hiding this comment

Uh oh!

hugueskamba Dec 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bulislaw commented Dec 30, 2019

Uh oh!

mbed-ci commented Jan 3, 2020

Test run: FAILED

Uh oh!

bulislaw commented Jan 7, 2020

Uh oh!

Uh oh!

bulislaw commented Dec 24, 2019 •

edited

Loading

hugueskamba Dec 30, 2019 •

edited

Loading