minimal-printf should not bypass the retargetting code #11235

evedon · 2019-08-15T11:52:01Z

Description

minimal-printf should not be trying to work around the retargetting - it should just use fputc.
Fixes #11234

Pull request type

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

Reviewers

@kjbracey-arm

Release Notes

kjbracey

If making this change, it can be pared back more.

If print character is just fputc, then the "enable file" option is no longer needed - you can just always do fputc, and get rid of that option. (But you need to pass stdout instead of NULL whereever you're passing NULL as stream?)

The "enable file" option can later be a thing that actually cuts that out at the retargetting layer, so fputc ignores its FILE parameter and just does a console write. Done there, it should be a bigger saving.

kjbracey · 2019-08-15T12:39:42Z

platform/source/minimal-printf/mbed_printf_implementation.c

-#define MBED_PRINT_CHARACTER(x)
-
-#endif // if DEVICE_SERIAL
+#define MBED_PRINT_CHARACTER(x) { fputc(x, stdout); }


Having done this, the CONSOLE_OUTPUT_UART option no longer means that, and really the whole option should be removed (for now).

We already have the retargetting mechanisms to make fputc go to SingleWireOutput, so it isn't needed here.

In future there could be some system option to hardcode fputc globally to actually be a particular console type, cutting back the retargetting generally, but that option wouldn't be processed here.

The mechanism in the retargetting code is via mbed_override_console which needs to be implemented by the application.
Althought it is simpler to have the option to output to SWO using a config option minimal-printf-console-output as was done by the original minimal-printf library, I can remove this code for the sake of simplicity.

If you want "easy-to-select SWO", it shouldn't be bound into the "minimal-printf" - the same easy-selection mechanism should be available with full retargetting.

You could have an option which does the override-console for you. In network interfaces we have such options done non-centrally as esp8266.provide-default etc, which activates that particular WiFiInterface::get_default_instance. SWO could have an option that makes it do that with mbed_override_console.

(Often I'd expect a target to be selecting SWO, if that's the way the target's wired, not the application itself).

The hypothetical minimal-retargetting would have to have a parallel hardcoded-mechanism to pick up the known hardcoded retargets. Not sure what the implementation would look like, but it could certainly follow the easy-selection option, so that option just uses the generic mechanism with full retargetting, or selects a hardcoded thing in minimal retargetting.

hugueskamba · 2019-08-15T12:45:50Z

tools/profiles/extensions/minimal-printf.json

@@ -1,12 +1,12 @@
 {
    "GCC_ARM": {
-       "common": ["-DMBED_MINIMAL_PRINTF", "-fno-builtin-printf"],
+       "common": ["-DMBED_MINIMAL_PRINTF"],


Why was this removed?

Because the change is making printf work "normally", so the compiler no longer needs to treat it as "abnormal".

Without this option, the compiler can transform printf("Hello\n") into puts("Hello"), or printf(".") into putchar('.'). That would have triggered the problem we're now fixing.

Putting this option in meant output was consistent as long as the user always used printf and never did puts or putchar themselves.

Ok. I need to understand how that retarget work.

It's not so much about how the retargetting works in detail as the fact you're short-circuiting it

printf, putchar and puts are all supposed to go to the same place as fputc(stdout).

If those things don't all go to the same place, or skip the buffering among only some paths, then things get weird when you mix them.

(The retargetting controls where fputc(stdout) goes, and then all other I/O calls follow).

evedon · 2019-08-15T13:39:00Z

If making this change, it can be pared back more.

If print character is just fputc, then the "enable file" option is no longer needed - you can just always do fputc, and get rid of that option. (But you need to pass stdout instead of NULL whereever you're passing NULL as stream?)

The "enable file" option can later be a thing that actually cuts that out at the retargetting layer, so fputc ignores its FILE parameter and just does a console write. Done there, it should be a bigger saving.

The enable file option wraps fprintf & vfprintf so it is still useful.

kjbracey · 2019-08-15T13:54:13Z

The enable file option wraps fprintf & vfprintf so it is still useful.

Okay, I'll need to double-check that a bit more - I was looking at the apparently-unneeded if it guards in the core C code.

kjbracey · 2019-08-16T07:38:08Z

Okay, the enable file thing also controls "do we also send fprintf via minimal printf".

Turning that on then activated the fputc code in the core formatter. But if that's now unconditionally included, then there's no reason not to always send fprintf through the minimal printf.

Again, simplifying fputc would be done separately from the formater.

One different potential saving I could see is eliminating the fputc dependency in mbed_minimal_putchar if the formatter is only ever used for sprintf, not printf or fprintf forms. (It would be nice if that could be automatic, but not sure how to do that).

evedon · 2019-08-16T10:34:50Z

My last commit simpifies the logic around fputc.

One different potential saving I could see is eliminating the fputc dependency in mbed_minimal_putchar if the formatter is only ever used for sprintf, not printf or fprintf forms. (It would be nice if that could be automatic, but not sure how to do that).

I agree that we can remove the fputc dependency in mbed_minimal_putchar if only sprintf is called by the application but I would rather leave this for a different PR.

platform/source/minimal-printf/mbed_printf_implementation.c

hugueskamba · 2019-08-22T09:28:33Z

platform/mbed_lib.json

@@ -128,10 +128,6 @@
            "help": "Use the MPU if available to fault execution from RAM and writes to ROM. Can be disabled to reduce image size.",
            "value": true
        },
-        "minimal-printf-console-output": {


Should minimal-printf-enable-file-stream also be removed given mbed_minimal_putchar no longer considers it?
If it should be removed, references in mbed_printf.c/h and mbed_printf_wrapper.c also should be removed.

…etargetting code Removed minimal-printf-console-output

hugueskamba · 2019-08-29T10:37:41Z

This force push rebases from master and squashes all commits to fix issue #11234.

0xc0170 · 2019-08-29T10:47:24Z

CI started

0xc0170 · 2019-08-29T13:07:20Z

Job aborted, internal error, restarting

mbed-ci · 2019-08-29T13:07:32Z

Test run: FAILED

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

Failed test jobs:

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

mbed-ci · 2019-08-29T15:24:11Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

evedon requested a review from hugueskamba August 15, 2019 12:36

kjbracey suggested changes Aug 15, 2019

View reviewed changes

hugueskamba reviewed Aug 15, 2019

View reviewed changes

kjbracey reviewed Aug 16, 2019

View reviewed changes

platform/source/minimal-printf/mbed_printf_implementation.c Outdated Show resolved Hide resolved

evedon force-pushed the mprintf-rework branch from 62b14c8 to fa37b20 Compare August 16, 2019 13:36

0xc0170 added the needs: work label Aug 20, 2019

hugueskamba reviewed Aug 22, 2019

View reviewed changes

hugueskamba force-pushed the mprintf-rework branch 2 times, most recently from ec7e232 to be6413c Compare August 29, 2019 10:26

Changed minimal-printf to call fputc so that it does not bypass the r…

d9f3263

…etargetting code Removed minimal-printf-console-output

hugueskamba force-pushed the mprintf-rework branch from be6413c to d9f3263 Compare August 29, 2019 10:34

0xc0170 requested a review from kjbracey August 29, 2019 10:41

0xc0170 added needs: review and removed needs: work labels Aug 29, 2019

kjbracey approved these changes Aug 29, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Aug 29, 2019

adbridge added the CI finished label Aug 29, 2019

ciarmcom added ready for merge and removed needs: CI labels Aug 29, 2019

0xc0170 added the release-version: 5.14.0-rc1 label Aug 29, 2019

0xc0170 merged commit ea582f3 into ARMmbed:master Aug 29, 2019

0xc0170 removed CI finished ready for merge labels Aug 29, 2019

evedon deleted the mprintf-rework branch September 20, 2019 10:37

minimal-printf should not bypass the retargetting code #11235

minimal-printf should not bypass the retargetting code #11235

Uh oh!

Conversation

evedon commented Aug 15, 2019

Description

Pull request type

Reviewers

Release Notes

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Aug 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evedon commented Aug 15, 2019

Uh oh!

kjbracey commented Aug 15, 2019

Uh oh!

kjbracey commented Aug 16, 2019

Uh oh!

evedon commented Aug 16, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugueskamba Aug 29, 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!

hugueskamba commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

0xc0170 commented Aug 29, 2019

Uh oh!

mbed-ci commented Aug 29, 2019

Uh oh!

mbed-ci commented Aug 29, 2019

Uh oh!

Uh oh!

kjbracey Aug 15, 2019 •

edited

Loading

hugueskamba Aug 29, 2019 •

edited

Loading