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

Optional hardware flow control for STDOUT #6603

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
7 participants
@marcuschangarm
Contributor

marcuschangarm commented Apr 11, 2018

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

This comment has been minimized.

Contributor

marcuschangarm commented Apr 11, 2018

@0xc0170 as discussed.

@0xc0170 0xc0170 requested a review from kjbracey-arm Apr 12, 2018

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

This comment has been minimized.

@0xc0170

0xc0170 Apr 12, 2018

Member

Looking at the other config there, should this become

MBED_CONF_PLATFORM_STDIO_FC or FLOW_CONTROL ?

This comment has been minimized.

@marcuschangarm

marcuschangarm Apr 12, 2018

Contributor

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.

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 12, 2018

Contributor

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

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 12, 2018

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

This comment has been minimized.

Contributor

marcuschangarm commented Apr 12, 2018

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

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 12, 2018

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

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 12, 2018

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

This comment has been minimized.

Member

0xc0170 commented Apr 12, 2018

@theotherjimmy Can you please review this ?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 12, 2018

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

This comment has been minimized.

Contributor

marcuschangarm commented Apr 12, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Apr 12, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Apr 12, 2018

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

This comment has been minimized.

Contributor

theotherjimmy commented Apr 12, 2018

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)

@loverdeg-ep

This comment has been minimized.

loverdeg-ep commented Apr 12, 2018

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

This comment has been minimized.

Contributor

marcuschangarm commented Apr 12, 2018

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.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:fix-flowcontrol branch from b6f62bf to f67fd3c Apr 12, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 13, 2018

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 marcuschangarm force-pushed the marcuschangarm:fix-flowcontrol branch from f67fd3c to a378e75 Apr 13, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 13, 2018

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

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 13, 2018

Contributor

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.

Optional hardware flow control for STDOUT
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 marcuschangarm force-pushed the marcuschangarm:fix-flowcontrol branch from a378e75 to 7e6538f Apr 16, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 16, 2018

@kjbracey-arm That should be it.

@kjbracey-arm

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

# 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);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 16, 2018

Contributor

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.

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

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Apr 16, 2018

Contributor

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 16, 2018

/morph build

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

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 16, 2018

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

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 16, 2018

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 16, 2018

/morph test

1 similar comment
@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 16, 2018

/morph test

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

/morph test

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 17, 2018

@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

This comment has been minimized.

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

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10218 cycles (+1368 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the needs: CI label Apr 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment