Skip to content
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

minimal-printf should not bypass the retargetting code #11235

Merged
merged 1 commit into from Aug 29, 2019

Conversation

@evedon
Copy link
Contributor

commented Aug 15, 2019

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

@evedon evedon requested a review from hugueskamba Aug 15, 2019
Copy link
Contributor

left a comment

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.

#define MBED_PRINT_CHARACTER(x)

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 15, 2019

Contributor

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.

This comment has been minimized.

Copy link
@evedon

evedon Aug 15, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 15, 2019

Contributor

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.

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

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Aug 15, 2019

Contributor

Why was this removed?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 15, 2019

Contributor

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.

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Aug 15, 2019

Contributor

Ok. I need to understand how that retarget work.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Aug 15, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

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-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

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-arm

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

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.

@evedon evedon force-pushed the evedon:mprintf-rework branch from 62b14c8 to fa37b20 Aug 16, 2019
@0xc0170 0xc0170 added the needs: work label Aug 20, 2019
@@ -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": {

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Aug 22, 2019

Contributor

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.

This comment has been minimized.

Copy link
@hugueskamba

hugueskamba Aug 29, 2019

Contributor
@hugueskamba hugueskamba force-pushed the evedon:mprintf-rework branch 2 times, most recently from ec7e232 to be6413c Aug 29, 2019
…etargetting code

Removed minimal-printf-console-output
@hugueskamba hugueskamba force-pushed the evedon:mprintf-rework branch from be6413c to d9f3263 Aug 29, 2019
@hugueskamba

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

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

@0xc0170 0xc0170 requested a review from kjbracey-arm Aug 29, 2019
@0xc0170 0xc0170 added needs: review and removed needs: work labels Aug 29, 2019
@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 29, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Job aborted, internal error, restarting

@mbed-ci

This comment has been minimized.

Copy link

commented Aug 29, 2019

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

This comment has been minimized.

Copy link

commented Aug 29, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit ea582f3 into ARMmbed:master Aug 29, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(-72 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8689 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
@evedon evedon deleted the evedon:mprintf-rework branch Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.