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

Add option to disable default UART console #10328

Merged
merged 1 commit into from Apr 9, 2019

Conversation

Projects
None yet
8 participants
@kjbracey-arm
Copy link
Contributor

commented Apr 5, 2019

Description

New target.console-uart option added to indicate whether a target has a console UART on STDIO_UART_TX/RX/RTS/CTS pins. (The existing option target.console-uart-flow-control indicates whether RTS and or CTS is available in addition to TX and RX).

The option defaults to true, and is currently true on all platforms. It only applies if DEVICE_SERIAL is true, so no need to go through and mark it false for non-SERIAL platforms.

An application can turn off target.console-uart to save ROM/power/etc if they don't want to use the serial console. If this is turned off, the console won't be activated for stdin/stdout, but the application is
still free to open UARTSerial(STDIO_UART_TX, STDIO_UART_RX) themselves.

Fixes #10327.

Pull request type

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

Reviewers

Release Notes

  • New config option target.console-uart to allow default serial console functionality to be deactivated, either because the target has serial but no console, or to save power/memory by not using the console.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:stdio_serial_option branch from 2bf2e8d to 4ac36eb Apr 5, 2019

@@ -261,7 +261,7 @@ MBED_WEAK FileHandle *mbed::mbed_override_console(int fd)

static FileHandle *default_console()
{
#if DEVICE_SERIAL
#if MBED_CONF_STDIO_SERIAL && MBED_CONF_TARGET_CONSOLE_UART && DEVICE_SERIAL

This comment has been minimized.

Copy link
@LMESTM

LMESTM Apr 5, 2019

Contributor

Shouldn't this be MBED_CONF_PLATFORM_STDIO_SERIAL ?
Also why adding both checks here ? What would happen if only one of the configurations is set to false ?
I think maybe ...
#if MBED_CONF_STDIO_SERIAL && `DEVICE_SERIAL
would be enough in this file here

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Apr 8, 2019

Author Contributor

I'm trying to make a subtle distinction between "target has a UART console interface" and "application wants to use UART console interface if available".

I think you're right, this probably isn't worth splitting into two options. If it is a target option, then target can have it true or false, and an application can either leave it unmodified if it wants to use it if available, or set it to false if not.

This would make it consistent with the existing target.console-uart-flow-control.

@@ -274,7 +274,7 @@ static FileHandle *default_console()
# else
static DirectSerial console(STDIO_UART_TX, STDIO_UART_RX, MBED_CONF_PLATFORM_STDIO_BAUD_RATE);
# endif
#else // DEVICE_SERIAL
#else // MBED_CONF_STDIO_SERIAL && MBED_CONF_TARGET_CONSOLE_UART && DEVICE_SERIAL

This comment has been minimized.

Copy link
@LMESTM

LMESTM Apr 5, 2019

Contributor

same here

@@ -15,6 +15,10 @@
"bootloader_supported": false,
"static_memory_defines": true,
"config": {
"console-uart": {

This comment has been minimized.

Copy link
@LMESTM

LMESTM Apr 5, 2019

Contributor

I am still not clear what this is used for ...

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 5, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

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

@LMESTM

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@kjbracey-arm I've made comments - but this anyway the idea looks good to me - thanks for this !

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:stdio_serial_option branch from 4ac36eb to e704bee Apr 8, 2019

@kjbracey-arm kjbracey-arm changed the title Add options to disable default UART console Add option to disable default UART console Apr 8, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

Reduced down to one option.

@LMESTM

LMESTM approved these changes Apr 8, 2019

@0xc0170

0xc0170 approved these changes Apr 8, 2019

Show resolved Hide resolved targets/targets.json Outdated
Add option to disable default UART console
New `target.console-uart` option added to indicate whether a target has
a console UART on STDIO_UART_TX/RX/RTS/CTS pins. (The existing option
`target.console-uart-flow-control` indicates whether RTS and or CTS is
available in addition to TX and RX).

The option defaults to true, and is currently true on all platforms. It
only applies if DEVICE_SERIAL is true, so no need to go through and mark
it false for non-SERIAL platforms.

An application can turn off target.console-uart to save ROM/power/etc if
they don't want to use the serial console.  If this is turned off, the
console won't be activated for stdin/stdout, but the application is
still free to open `UARTSerial(STDIO_UART_TX, STDIO_UART_RX)`
themselves.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:stdio_serial_option branch from e704bee to f6456d8 Apr 8, 2019

@cmonr cmonr added needs: CI and removed needs: review labels Apr 8, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 9, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Apr 9, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Final approval from @ARMmbed/mbed-os-core ?

@cmonr cmonr merged commit b419cfc into ARMmbed:master Apr 9, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9140 cycles (-871 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.