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

UARTSerial: add flow control and format APIs #5088

Merged
merged 1 commit into from Nov 2, 2017

Conversation

Projects
None yet
10 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Sep 13, 2017

Add passthrough APIs to enable the flow control and format methods from
SerialBase to be accessed.

Modify the RX data pump so it stops reading data and disables the IRQ
when the buffer is full, to allow UART automatic flow control to work.

In principle it would also be possible as a future enhancement to
provide XON/XOFF flow control, or manual RTS/CTS control using GPIO, but
this commit at least restores the functionality present in Serial,
SerialBase and RawSerial that was missing in UARTSerial.

Implements #4428

UARTSerial: add flow control and format APIs
Add passthrough APIs to enable the flow control and format methods from
SerialBase to be accessed.

Modify the RX data pump so it stops reading data and disables the IRQ
when the buffer is full, to allow UART automatic flow control to work.

In principle it would also be possible as a future enhancement to
provide XON/XOFF flow control, or manual RTS/CTS control using GPIO, but
this commit at least restores the functionality present in Serial,
SerialBase and RawSerial that was missing in UARTSerial.
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 19, 2017

@hasnainvirk

This comment has been minimized.

Contributor

hasnainvirk commented Sep 19, 2017

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Sep 21, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 21, 2017

@sg- Could you take a look at this API change?

@sg-

This comment has been minimized.

Member

sg- commented Sep 22, 2017

I dont like the mixture of DEVICE_XXX in our APIs and suggest we should be using inheritance for this.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 22, 2017

Don't really follow - the HAL API is conditioned that way, and this is just passing through to the HAL, same as the other serial classes.

Might be better to make it so the call is always available but returns an error code if it can't do it because the HAL doesn't support it?

Might make sense as it could in principle support non-hardware-assisted flow control without help from the HAL - wiggle the RTS and CTS as GPIO based on software buffer level.

@0xc0170 0xc0170 added needs: review and removed needs: CI labels Sep 27, 2017

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Sep 27, 2017

@kjbracey-arm, @sg-: is there any way that we can move this change along? I have a thing that really doesn't work without HW flow control.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 28, 2017

Don't really follow - the HAL API is conditioned that way, and this is just passing through to the HAL, same as the other serial classes.

That is correct, this is how it is now. We could change this to not wrap entire function definiton but only the implementation. The question is then as many of our API do not have return value, how to report an error.
We need to define error handling path (compiler error, runtime error), and use it in our codebase (I would prefer compiler error - catch those as early as possible, not via macros but use C++ for this - something like in python NotImplemented error? Or similar. Something that reports This feature xxx is not implemented by your target in this case). As currently these device capabilities are set via macros, I would not block this PR because of it. As result, to create a new issue where we can discuss the issue with device capabilities and API further.
Or is there a path we could do it for this PR withour much refactoring?

@kjbracey-arm suggestions?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Sep 28, 2017

For this one I'd be happy to have an integer error return value on the function being added. Whether that fits with wider style views, I don't know.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 29, 2017

For this one I'd be happy to have an integer error return value on the function being added. Whether that fits with wider style views, I don't know.

@sg- What do you think?

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 19, 2017

bump

@sg-

This comment has been minimized.

Member

sg- commented Oct 23, 2017

I dont understand where the error code fits but isn't really my problem. The mixture of serial capabilities in developer APIs is a bad path to go down IMO. While this pattern exists I think we'd all agree its not the best way to present capabilities. That said, this patch is starting to define failure modes but we dont have a way to signal that to the application. As with other failure modes, I'd suggest for at least a debug build we were asserting the failure case given the problems are very hard to diagnose. I think @c1728p9 had a patch at one point to add RX OVERRUN in the HAL so I wonder how much of this just needs to be expedited as a review for completeness versus just poking at it more.

@SenRamakri @bulislaw

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 24, 2017

There's nothing here about returning error conditions like overrun - this is just letting apps specify flow control mode, which is dependent on target support.

If the app requests flow control but the target lacks it - do we want a compile time error, a run-time error(), or a negative error code return?

(I'm kind of inclined towards the last, but not that strongly, as in principle different ports may have different capabilities. Not that the serial HAL currently allows error returns to indicate that)

At the minute, Serial, RawSerial and SerialBase all provide set_flow_control #if DEVICE_SERIAL_FC. UARTSerial doesn't provide it at all, and that's an omission that has to be fixed fairly urgently.

If you don't want UARTSerial to just work the same as the existing ones, then we need a concrete proposal for what you actually do want.

You said "inheritance" - are you envisaging a UARTSerialWithFlowControl that only exists #if DEVICE_SERIAL_FC?

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Oct 24, 2017

Can I ask that we separate making architectural changes from simply exposing an interface that has always been there and is fundamental to serial port operation? This change is required if we are to use the Cellular API at a rate faster than 115200 which, if we consider only 3G on-air rates, is boringly normal. I'm not really following what approach might be adopted to present "set baud rate" in a more architecturally sympathetic way but I need to get on and re-architecting is taking too long.

@sg-

This comment has been minimized.

Member

sg- commented Oct 24, 2017

In that case I'd suggest we drop the part that disables the RX handler and only add support for the missing flow control and format.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Oct 25, 2017

The RX handler change is a necessary part of the flow control change:

Modify the RX data pump so it stops reading data and disables the IRQ when the buffer is full, to allow UART automatic flow control to work.

The existing version's RX data pump always reads bytes from the data register, and throws it away if the software buffer is full. That defeats any hardware flow control if active, as the hardware buffer never fills.

The replacement code here stops reading the data register if the software buffer is full, allowing the hardware buffer to fill to cause the UART to signal "full".

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Oct 27, 2017

Thanks @kjbracey-arm . Looks fine to me as it is now.

@adbridge adbridge added needs: CI and removed needs: review labels Oct 31, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Oct 31, 2017

/morph build

@studavekar

This comment has been minimized.

Collaborator

studavekar commented Oct 31, 2017

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Oct 31, 2017

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Nov 2, 2017

@theotherjimmy theotherjimmy merged commit 5768693 into ARMmbed:master Nov 2, 2017

7 checks passed

AWS-CI uVisor Build & Test Success
Details
AWS-CI uvisor Test DIDNT RUN UVISOR TESTS
Details
Cam-CI uvisor Build & Test DIDNT RUN UVISOR TESTS
Details
ci-morph-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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment