Optional hardware flow control for STDOUT #6603

marcuschangarm · 2018-04-11T17:01:05Z

Description

Some platforms have interface chips with hardware flow control
enabled by default. This commit adds configurable flow control to
STDOUT.

Usage:

Define pin names STDIO_UART_RTS for Rx-flow-control and
STDIO_UART_CTS for Tx-flow-control.
Set target.console-uart-flow-control-type to any SerialBase::Flow.
Enable flow control by setting target.console-uart-flow-control to true.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

marcuschangarm · 2018-04-11T17:01:33Z

@0xc0170 as discussed.

0xc0170 · 2018-04-12T09:35:46Z

platform/mbed_retarget.cpp

@@ -147,6 +147,9 @@ DirectSerial::DirectSerial(PinName tx, PinName rx, int baud) {
    if (stdio_uart_inited) return;
    serial_init(&stdio_uart, tx, rx);
    serial_baud(&stdio_uart, baud);
+#if STDIO_UART_FC


Looking at the other config there, should this become

MBED_CONF_PLATFORM_STDIO_FC or FLOW_CONTROL ?

That would imply the configuration is defined in platform/mbed_lib.json, which would make it impossible to configure from targets.json or custom_targets.json.

So why not put it in the base target, Target, in targets.json?

kjbracey · 2018-04-12T09:45:06Z

Certainly a good thing to have.

For the pin names, I think I'd be happier with using the standard RTS/CTS names.

As to activation, I think this has to be app-configurable - the target is indicating by defines that it has a default serial port for console, and what its pins are, and certainly it must indicate that it has RTS/CTS pins. The current concept is that "STDIO_UART" defines are normally set by the target, not the app, I believe.

But that doesn't mean an app necessarily definitely wants to use them - eg it may be that you've not configured your terminal properly, or you're talking to a test rig that doesn't have it wired. I would have thought you needed a JSON config enable for this, alongside the baudrate. Particularly if we start adding the defines to existing targets - having it on by default could cause backwards compatibility issues.

So counter suggestion:

Target specifies pins as STDIO_UART_RTS/CTS alongside the TX and RX (in emergency app can set them through JSON if target didn't)
New option to set flow control type in platform/mbed_lib.json alongside baud rate, defaulting to None (specific target can override to have a different default)
Take care to only reference target's STDIO_UART_RTS/CTS if that flow type is set to hardware.

marcuschangarm · 2018-04-12T10:39:14Z

@kjbracey-arm

As to activation, I think this has to be app-configurable - the target is indicating by defines that it has a default serial port for console, and what its pins are, and certainly it must indicate that it has RTS/CTS pins.

Wouldn't that be a deviation from our current behavior? Specifically, NRF52 derived boards come in variants that require flow control to be either on or off explicitly. So, without a default setting (not on the app-level) debug output wont work.

Target specifies pins as STDIO_UART_RTS/CTS alongside the TX and RX (in emergency app can set them through JSON if target didn't)

Sounds good.

New option to set flow control type in platform/mbed_lib.json alongside baud rate, defaulting to false (specific target can override to have a different default)

I originally did something similar, but we got a request to not put this is an mbed_lib.json file because our users wanted to be able to override/configure the setting from custom_target.json which the current system doesn't allow.

I would like to keep it out of targets.json, where it currently lives, so this was the compromise.

kjbracey · 2018-04-12T11:01:44Z

I guess in many cases the debug UART is via an on-board device which may have its own fixed requirement, rather than pass it through as user-visible.

So if I understand correctly the problem is that the target itself can only override a library setting via the target_overrides either directly in a mbed_lib.json or mbed_app.json? Not from targets.json or custom_targets.json?

We can add target overrides to platform/mbed_lib.json - I see a couple for EFM32/EFR32 setting baud rate to 115200 for example. That would seem to be the existing pattern.

Feels like this is a general tooling issue it would be nice to address - have a way for the "overrides" inside the target JSON to affect libraries. I can't see why that couldn't operate at the same basic ordering level as the target_overrides in the lib JSON.

If the request is "avoid use the mbed config system because it isn't flexible enough", I think the correct response is to improve the flexibility, not avoid it.

Easier said than done though, I suspect. Who do we need to discuss this with?

kjbracey · 2018-04-12T11:07:01Z

It's interesting that the docs for the config use this specific example of target.serial_console_speed and how a derived target might override it. Only problem is the setting we actually ended up with was a library setting, not a target one.

Should baud and flow control actually be target options in the base Target, rather than library options?

Are target config options totally unused at present? Don't see any in targets.json. Do they work as documented?

0xc0170 · 2018-04-12T11:38:36Z

@theotherjimmy Can you please review this ?

marcuschangarm · 2018-04-12T14:14:22Z

Should baud and flow control actually be target options in the base Target, rather than library options?

Are target config options totally unused at present? Don't see any in targets.json. Do they work as documented?

They are used by the NRF52: https://github.com/ARMmbed/mbed-os/blob/master/targets/targets.json#L3506-L3516

The annoying part is that it is tied to DAPLINK on the NRF52. If you use JLINK, flow control has to be disabled. So in that sense it is not really a hardware setting.

Usually the NRF52_DK board is shipped with JLINK as default. But in our CI it is using DAPLINK.

marcuschangarm · 2018-04-12T14:19:15Z

So if I understand correctly the problem is that the target itself can only override a library setting via the target_overrides either directly in a mbed_lib.json or mbed_app.json? Not from targets.json or custom_targets.json?

Yes, I believe so.

We can add target overrides to platform/mbed_lib.json - I see a couple for EFM32/EFR32 setting baud rate to 115200 for example. That would seem to be the existing pattern.

I agree. However, our users would like to keep everything in custom_target.json.

theotherjimmy · 2018-04-12T14:41:32Z

The annoying part is that it is tied to DAPLINK on the NRF52. If you use JLINK, flow control has to be disabled. So in that sense it is not really a hardware setting.

I don't really follow that, and this feels like splitting hairs in a way that prevents the targets.json solution from looking attractive (so for no benefit, and possibly a disadvantage). From an application's perspective, the debug interface is a fixed function device whether it's programmable or not because no code in Mbed OS can change it. I think this sort of thing is appropriate for targets.json.

theotherjimmy · 2018-04-12T14:42:49Z

I would like to keep it out of targets.json, where it currently lives, so this was the compromise.

I have yet to see a justification for this statement. I still consider targets.json the appropriate place for this flag.

theotherjimmy · 2018-04-12T14:48:55Z

have a way for the "overrides" inside the target JSON to affect libraries. I can't see why that couldn't operate at the same basic ordering level as the target_overrides in the lib JSON.

It's a limitation of the config system as it's implemented ATM. You may think that it could be as easy as just "flipping a switch" but the current config system is structured around the assumptions that:

target's can't override libs.
app configs can be applied twice without consequence.
configuration parameters and overrides may be applied at the same time.

This proposed flexibility breaks all 2/3 of these assumptions, and would require a pretty big rewrite of the config system (which might be a good thing, it's currently extremely difficult read/understand)

0Grit · 2018-04-12T18:55:46Z

I'm with @theotherjimmy on this one. Flow control should be defined at the target level. Even more specifically at the BOARD level.

We have custom NRF boards that do not break out flow control lines and therefore do not support flow control.

marcuschangarm · 2018-04-12T21:26:55Z

It sounds like if I rename the flow control pins to STDIO_UART_RTS and STDIO_UART_CTS as suggested by @kjbracey-arm then everyone gets something they want.

Flow control will be disabled by default so @loverdeg-ep doesn't have to worry about disabling it anymore and @theotherjimmy can enable flow control in targets.json if he wants.

kjbracey · 2018-04-13T07:12:52Z

It sounds like @theotherjimmy and I are in agreement - this would structually fit as a proper target JSON setting, in the base Target. Then it can be neatly overridden by targets.json, custom_targets.json and mbed_app.json, and it is a real setting, not just a define. That seems to be using the system as intended.

I agree with the config docs example that the term console is a better term than stdio and it's what we're already using for the redirection hooks. This would be target.console-uart-flow-control, or similar. "stdio" is C-specific, and even there it does not mean "stdin+stdout+stderr".

This leaves the platform.stdio-baud-rate somewhat inconsistent and not able to be overridden by custom_targets.json, but I think that could be revised to align in a subsequent PR as follows:

add corresponding target.console-uart-baud-rate setting in base Target
set default for platform.stdio-baud-rate to null - which means use the target setting
add a note that platform.stdio-baud-rate is deprecated and target.console-baud-rate should be used instead (but it will still work if set by apps)
move the EFR32 target_overrides for baud rate into targets.json

I'm the person who put it in as platform.stdio-baud-rate - it was on the basis that it was platform code that would be reading the setting, and there were no existing target JSON settings to follow the model of. I can see that the target.console-uart-baud-rate will work better, and restores consistency, as all the other parameters to the Serial drivers are coming from the target anyway. I was assuming the console baud rate was an externally visible, non-board-dependent thing, but that's not necessarily true.

Arguably all those STDIO_UART pin selection defines could also be Target JSON settings. Something for a later PR, perhaps.

marcuschangarm · 2018-04-13T09:27:14Z

@kjbracey-arm I've added the console settings to the base target. Please take a look!

kjbracey · 2018-04-13T09:58:32Z

targets/targets.json

+            },
+            "console-uart-flow-control-type": {
+                "help": "Console hardware flow control type. Value must be of type SerialBase::Flow.",
+                "value": "SerialBase::RTSCTS"


Yay, looks like the correct place.

I'd prefer to have a single setting here, rather than setting + enable. Not sure what @theotherjimmy thinks.

We should avoid the "SerialBase::" prefix - just enumerate the valid token-type strings, and the C++ code can add prefixes or otherwise detect as required.

If you ever need special compile-time behaviour for certain settings rather than simple passthrough, using simple identifiers makes that possible. (Compare config changes done in #6566 for channel band names)

So one option with valid values null, "RTS", "CTS" and "RTSCTS"? Then for the current simple case you just #ifdef on that, rather than #if on the enable, and it leaves open the door for future concat-type trickery. If you did that trickery now, you could specifically detect "Disabled", but seems pointless.

Some platforms have interface chips with hardware flow control enabled by default. This commit adds configurable flow control to STDOUT. Usage: * Define pin names STDIO_UART_RTS for Rx-flow-control and STDIO_UART_CTS for Tx-flow-control. * Set target.console-uart-flow-control. Valid options are: null, RTS, CTS, and RTSCTS.

marcuschangarm · 2018-04-16T03:15:25Z

@kjbracey-arm That should be it.

kjbracey

Looks good to me. Would like a second opinion from @theotherjimmy

kjbracey · 2018-04-16T07:27:10Z

platform/mbed_retarget.cpp

+#   elif CONSOLE_FLOWCONTROL == CONSOLE_FLOWCONTROL_CTS
+        console.set_flow_control(SerialBase::CTS, NC, STDIO_UART_CTS);
+#   elif CONSOLE_FLOWCONTROL == CONSOLE_FLOWCONTROL_RTSCTS
+        console.set_flow_control(SerialBase::RTSCTS, STDIO_UART_RTS, STDIO_UART_CTS);


Was originally thinking this would use concat(FlowControl, MBED_CONF_TARGET_CONSOLE_UART_FLOW_CONTROL) and SerialBase::MBED_CONF_TARGET_CONSOLE_UART_FLOW_CONTROL, but yes, guess we should be spelling it out like this to not reference the CTS/RTS we're not using, so that's not necessary.

kjbracey · 2018-04-16T07:28:52Z

targets/targets.json

+        "bootloader_supported": false,
+        "config": {
+            "console-uart-flow-control": {
+                "help": "Console hardware flow control. Options: null, RTS, CTS, RTSCTS.",


Should it be including quotes in the help? Last 3 options are JSON strings.

0xc0170 · 2018-04-16T14:14:26Z

/morph build

mbed-ci · 2018-04-16T14:36:47Z

Build : SUCCESS

Build number : 1758
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6603/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-04-16T17:20:26Z

Exporter Build : FAILURE

Build number : 1395
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6603/

marcuschangarm · 2018-04-16T17:35:47Z

/morph export-build

mbed-ci · 2018-04-16T22:25:37Z

Test : FAILURE

Build number : 1565
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6603/1565

mbed-ci · 2018-04-16T22:31:34Z

Exporter Build : SUCCESS

Build number : 1398
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6603/

marcuschangarm · 2018-04-16T22:38:12Z

/morph test

cmonr · 2018-04-16T23:56:01Z

/morph test

mbed-ci · 2018-04-17T04:58:44Z

Test : SUCCESS

Build number : 1569
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6603/1569

mbed-ci · 2018-04-17T06:25:06Z

Test : FAILURE

Build number : 1570
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6603/1570

0xc0170 · 2018-04-17T07:43:22Z

/morph test

0xc0170 · 2018-04-17T07:54:45Z

@studavekar Can you check the errors, if we shall track this issue with echo for kw24d and report it? I checked the logs, in the last 20 runs, few reported similar failure and dont see any issue reported.

mbed-ci · 2018-04-17T18:12:25Z

Test : SUCCESS

Build number : 1578
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6603/1578

0xc0170 added the needs: review label Apr 12, 2018

0xc0170 requested a review from kjbracey April 12, 2018 09:34

0xc0170 reviewed Apr 12, 2018

View reviewed changes

kjbracey reviewed Apr 13, 2018

View reviewed changes

kjbracey approved these changes Apr 16, 2018

View reviewed changes

0xc0170 approved these changes Apr 16, 2018

View reviewed changes

theotherjimmy approved these changes Apr 16, 2018

View reviewed changes

cmonr added needs: CI and removed needs: review labels Apr 16, 2018

cmonr added ready for merge release-version: 5.8.3 and removed needs: CI labels Apr 17, 2018

0xc0170 added needs: CI and removed ready for merge labels Apr 17, 2018

0xc0170 mentioned this pull request Apr 17, 2018

Update Nordic NRF52 based targets to SDK 14.2 #6547

Merged

cmonr merged commit d680cee into ARMmbed:master Apr 17, 2018

cmonr removed the needs: CI label Apr 17, 2018

marcuschangarm mentioned this pull request Apr 18, 2018

Enable flow control in Greentea #6660

Merged

kjbracey mentioned this pull request Apr 30, 2018

Mbed config for STDIO_UART_TX not consistently implemented across targets #6689

Closed

3 tasks

marcuschangarm deleted the fix-flowcontrol branch May 10, 2018 21:52

kjbracey mentioned this pull request Sep 11, 2018

NRF52 serial: Fix UART console RX #8046

Merged

Optional hardware flow control for STDOUT #6603

Optional hardware flow control for STDOUT #6603

Uh oh!

Conversation

marcuschangarm commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

marcuschangarm commented Apr 11, 2018

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 commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcuschangarm commented Apr 12, 2018

Uh oh!

kjbracey commented Apr 12, 2018

Uh oh!

kjbracey commented Apr 12, 2018

Uh oh!

0xc0170 commented Apr 12, 2018

Uh oh!

marcuschangarm commented Apr 12, 2018

Uh oh!

marcuschangarm commented Apr 12, 2018

Uh oh!

theotherjimmy commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theotherjimmy commented Apr 12, 2018

Uh oh!

theotherjimmy commented Apr 12, 2018

Uh oh!

0Grit commented Apr 12, 2018

Uh oh!

marcuschangarm commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjbracey commented Apr 13, 2018

Uh oh!

marcuschangarm commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcuschangarm commented Apr 16, 2018

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey Apr 16, 2018 • 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!

0xc0170 commented Apr 16, 2018

Uh oh!

mbed-ci commented Apr 16, 2018

Triggering tests

Uh oh!

mbed-ci commented Apr 16, 2018

Uh oh!

marcuschangarm commented Apr 16, 2018

Uh oh!

mbed-ci commented Apr 16, 2018

Uh oh!

mbed-ci commented Apr 16, 2018

Uh oh!

marcuschangarm commented Apr 16, 2018

Uh oh!

cmonr commented Apr 16, 2018

Uh oh!

mbed-ci commented Apr 17, 2018

Uh oh!

marcuschangarm commented Apr 11, 2018 •

edited

Loading

kjbracey commented Apr 12, 2018 •

edited

Loading

theotherjimmy commented Apr 12, 2018 •

edited

Loading

marcuschangarm commented Apr 12, 2018 •

edited

Loading

kjbracey Apr 16, 2018 •

edited

Loading