api: uart: Add new asynchronous UART API (Transfer sub-API)

[Mierunski]

Added new UART API, that allows for longer transmissions, leaves IRQ handling on driver side and allows for DMA usage.

Added configuration option for setting like baudrate, stop bits, parity and data bits. Added as #11185

PR contains API proposal, driver implementationf for nRF UART and tests.

This PR is created as conclusion to #8243 #1682 #8306 #8307 #3457

Example call diagram.

[codecov-io]

Codecov Report

Merging #10820 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #10820   +/-   ##
=======================================
  Coverage   53.78%   53.78%           
=======================================
  Files         242      242           
  Lines       27697    27697           
  Branches     6729     6729           
=======================================
  Hits        14896    14896           
  Misses       9997     9997           
  Partials     2804     2804

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f956a9...53e37e0. Read the comment docs.

[qianfan-Zhao]

I think the PR is good but seems hard to implement the xfer API in the lowlevel, because the defines of xfer is not clear.

leaves IRQ handling on driver side and allows for DMA usage

There should has a buffer for buffered the received RX data for each serial port, how much bytes the buffer has, that should defined by the user. We need talk more about this.
Since uart high level try leave IRQ handing on driver side, should we disable CONFIG_UART_INTERRUPT_DRIVEN if CONFIG_UART_HIGH_LEVEL_API selected?

Seems the disign of this PR try to add sync and async mode for uart transfer, and could you please write some examples?

[Mierunski]

I think the PR is good but seems hard to implement the xfer API in the lowlevel, because the defines of xfer is not clear.

As written in code comments, uart_xfer is not for direct use. uart_rx and uart_tx functions describe how uart_xfer function should behave with different parameters.

There should has a buffer for buffered the received RX data for each serial port, how much bytes the buffer has, that should defined by the user. We need talk more about this.

User provides rx buffer and its length in uart_rx function, in contrary to receiving one byte at a time. This allows driver implementation to use DMA for handling longer transfers.

Since uart high level try leave IRQ handing on driver side, should we disable CONFIG_UART_INTERRUPT_DRIVEN if CONFIG_UART_HIGH_LEVEL_API selected?

Eventually this should become preferred API, so this option will be removed. For now we can disable it in this case.

Seems the disign of this PR try to add sync and async mode for uart transfer, and could you please write some examples?

I will add driver example after initial review is done.

[tbursztyka]

Nice step forward

We definitely needed a better API for UART for a long time. Having to deal with "interrupt" by user code was quite heavy, and was a legacy thing for early zephyr days (when it was not even zephyr actually).

[tbursztyka]

How is this going to handle full-duplex transfers? I find it hard to see it will be able to.

[Mierunski]

Full-duplex transfers can be handled in non-blocking mode, by independently setting buffers using uart_rx and uart_tx functions.

I haven't seen UART drivers that handle full-duplex transmission in blocking mode, so I don't think that function like spi_transceive for setting both buffers at the same time is needed. If there is a use case then I can change that.

[tbursztyka]

Full-duplex transfers can be handled in non-blocking mode, by independently setting buffers using uart_rx and uart_tx functions.

The problem here is the driver will not know when the actual transfer should start.

[Mierunski]

TX transfer should start immediately after uart_tx function is called.
RX transfer should start when data is available on line.

[tbursztyka]

So you might have timing issue then? Let's say you start tx, but the time you start rx, something already came in and your driver just dropped it. Does that sound plausible or am I being on a far fetched assumption?

[edit] oh then you would start rx, then tx, and that should do the trick right?

[qianfan-Zhao]

Have you read the serial port driver on arduino? I think that's is a good way, every UART has a rx ringbuffer for buffered all the received data in the background. The high level APIs don't need deal with the interrupts, they can read data from rx ringbuffer directly.

Also there can has a tx ringbuffer if needed, buffered all datas the high level to send, sent them when the bus is not busy.

In my opinion we can implement the high level at top of uart interrupt, like whats modem receiver did.

[pfalcon]

@qianfan-Zhao : That's what subsys/console has been doing for 2nd year or so, with API being gradually refactored towards POSIX-likeness, the latest piece is at #10765 . It goes slowly because every Zephyr developer wants their own original API, that's how we got original UART line input, uart_pipe, more recently modem_receiver, and now this API.

[qianfan-Zhao]

Not every developer wants to implement their own application interface, but there isn't enough good design yet, because UART is so special relative to SPI or I2C.

[pfalcon]

but there isn't enough good design yet

Yes, that's why we all are here, to make sure that we don't repeat previous mistakes. There're well-known best practices how to deal with this stuff, both algorithmically and API-wise, we just need to accept them and implement.

[nordic-krch]

let me put my 2 cents to this. i've been using following uart rx approach for some time and that worked well (contrary to many other in the past):

• uart is initialized with rx timeout
• 2 rx event types: RX_BUF_REQUEST, RX_RDY(data, len)
• rx_buf_rsp(buf, len) function for providing rx buf
• rx_enable(buf, len) - function for starting RX with initial buffer.

RX_RDY is generated when timeout occurs or buffer is fully filled. In that case RX_BUF_REQUEST is also triggered.
RX is started with first buffer provided. RX_BUF_REQUEST will be immediately triggered to get next buffer. Ideally, driver shall operate in a way that it has always one buffer ahead. If HW supports DMA linked transfers it will be possible to ensure that there is no gap in reception (even reliable reception without flow control).

Upper layer (hw independent thus common, can be console) has pool of buffers (min 3) or ring buffer. On RX_RDY can copy that to ring_buffer (or it can even use data directly from ring_buffer using recently added api) and takes care of managing memory. In simplest case (no dma) buffers are 1 byte long and timeout is set to 0. In order to make the timeout work with dma, uart must support hw timeouting (nrf uarte supports that, i've seen also hw support for rx timeout in silabs efr32).

@Mierunski proposed something similar:

So at start we should set up two buffers using uart_rx function

But i think that explicit buf_req and buf_rsp is cleaner.

[tbursztyka]

I don't think we care much about the error type. User won't be able to do anything about it anyway. Let's stick to negative errno: -EIO would just summarize it all for communication issue.

[Mierunski]

I think user would like to know why their transmission failed, not just that it failed. For example framing error could indicate wrong baudrate

[pfalcon]

I think user would like to know why their transmission failed, not just that it failed.

+1

And "break" support definitely should be accounted for. (But I'd call it "interesting" to stuff it as a kind of error).

[tbursztyka]

Well, user ... in an embedded os where nobody will be looking at what's happening once in production mode? (ok I am simplifying).

Just add debug or error logging on driver side if you want, but on user level there is really nothing to be done. It's a IO error, end of transaction... It would add up more logic on user side for nothing.

[pfalcon]

Well, user ... in an embedded os where nobody will be looking at what's happening once in production mode? (ok I am simplifying).

Replying to this older comment: our task is to provide users means to get complete status, including error status. And I assume any reputable vendor would collect that info for looking at, or we'll have a crap of buggy IoT around us.

[tbursztyka]

so int status would be just fine. 0 means all is fine, < 0 an error. You could get rid of the UART_ERROR event type.

[pfalcon]

This PR is created as conclusion to #8243 #1682 #8306 #8307 #3457

Hopefully not as conclusion (yet), but as a way to address those issues ;-).

[pfalcon]

Various comments.

[pfalcon]

I think user would like to know why their transmission failed, not just that it failed.

+1

And "break" support definitely should be accounted for. (But I'd call it "interesting" to stuff it as a kind of error).

[pfalcon]

What about received stuff?

[nordic-krch]

this should rather be union of struct uart_error_event and uart_rxtx_event (since it's the same).
why buf is unsigned char and not u8_t?

[pfalcon]

Posted this into inline comment thread, but let me repost here:

This API doesn't seem to be high-level enough and still leaves too many underspecified and "can go wrong" moments.

I guess the root problem is that this tries to do it "like SPI or I2C", but there's one big difference between those and UART. SPI and I2C are synchronous, master-slave protocols. But UART is async "peer to peer" protocol. Even, with optional peer to peer flow control. This concept of "transfers" is poorly suited for it. "Always available buffers" is better notion, but then there's already #10765 .

So, @Mierunski, can you please look at #10765 and tell which usecases the API you propose here enables which aren't handled by #10765?

[pfalcon]

So, @Mierunski, can you please look at #10765 and tell which usecases the API you propose here enables which aren't handled by #10765?

To give additional input to your analysis, consider that Zephyr supports userspace/kernel split. You very well know that because you have __syscall in your patch. Something people keep missing is that callbacks from kernel to userspace is no-go for this mode.

So, even at the very start of your analysis you would have following conjecture:

1. This PR proposes API which is suitable only for kernel mode.
2. Whereas the claim is made that subsys: console/tty: Refactor to introduce POSIX-like read/write API #10765 API is suitable for both kernel and userspace.

(Keep in mind that #10765 is just gradual WIP and misses a lot of things as the implementation. But as an API, it stays on the shoulders of the POSIX giant serving IT for ~50years.)

[qianfan-Zhao]

there has another talk on #10726. CC: @gon1332

[Mierunski]

I would like to continue with this API, in direction which @nordic-krch proposed, which is more refined version of what I proposed:

• uart is initialized with rx timeout
• 2 rx event types: RX_BUF_REQUEST, RX_RDY(data, len)
• rx_buf_rsp(buf, len) function for providing rx buf
• rx_enable(buf, len) - function for starting RX with initial buffer.

RX_RDY is generated when timeout occurs or buffer is fully filled. In that case RX_BUF_REQUEST is also triggered.
RX is started with first buffer provided. RX_BUF_REQUEST will be immediately triggered to get next buffer. Ideally, driver shall operate in a way that it has always one buffer ahead. If HW supports DMA linked transfers it will be possible to ensure that there is no gap in reception (even reliable reception without flow control).

Upper layer (hw independent thus common, can be console) has pool of buffers (min 3) or ring buffer. On RX_RDY can copy that to ring_buffer (or it can even use data directly from ring_buffer using recently added api) and takes care of managing memory. In simplest case (no dma) buffers are 1 byte long and timeout is set to 0. In order to make the timeout work with dma, uart must support hw timeouting (nrf uarte supports that, i've seen also hw support for rx timeout in silabs efr32).

About callbacks

To give additional input to your analysis, consider that Zephyr supports userspace/kernel split. You very well know that because you have __syscall in your patch. Something people keep missing is that callbacks from kernel to userspace is no-go for this mode.

I don't see a good alternative on how some features can be implemented without callbacks. Also GPIO API is showing that they are simply needed. Some kind of signal mechanism would be a solution, but still I don't see alternatives now.

Regarding console API

So, @Mierunski, can you please look at #10765 and tell which usecases the API you propose here enables which aren't handled by #10765?

#10765 is great as highest level UART API, users don't have to think about memory management, they just use already prepared method to achieve stable communication. API I propose is supposed to be solution in middle - users don't have to worry about sending every single byte, but they still have to think about buffer sizes.
For most cases #10765 will be sufficient, but I want to provide API that is lightweight and works under heavy loads, by using DMA and available hardware solutions, sometimes byte by byte transmission is just not fast enough.

As a summary, console API should be built on top of this API and should be first choice for basic communication. For long transmissions and resource constrained devices UART API will provide required functionality.

[galak]

How about replace u32_t flags with:

u8_t parity;
u8_t stop_bits;
u8_t data_bits;
u8_t flow_ctrl;

And than have enum values instead of defines.

[Mierunski]

I used enums before, after conversation with @tbursztyka I changed it to bitmasks.
I prefer enums, which one should I finally use?

[tbursztyka]

enums are little biteasier, but it takes more space. It's a tradeoff, either we focus on making device config as tiny as possible, or we focus on a certain ease of use. That said, bitmasks aren't exactly hard to use either (see i2c, spi, etc...)

If we were to use bitmasks, it looks to me it would be possible to get the baudrate being part of the flags (22 bits are available), saving 4 bytes.

Also, this timeout, as I said earlier it's a broader issue on every bus API, and we could probably find a generic way to solve this (an in-between solution for uart could be found through a timeout set via Kconfig for the time being if you want)

[nordic-krch]

@tbursztyka regarding timeout. I don't think that kconfig is a good place since it will vary between instances.

What do you think about using character length as timeout unit. Main advantage is that if you change baudrate you don't need to change timeout because it will be aligned.

[galak]

Is the savings really worth it? We're probably adding code size to do shift and masks vs just being able to directly access the variable. Also, I feels like the shift/mask is error prone to the driver developer from a security point of view. (I feel like I've seen over and over again coverity reporting simliar issues in GPIO drivers related to some bit masks there).

[pfalcon]

What do you think about using character length as timeout unit.

Please use standard units (milliseconds) for timeout.

[nordic-krch]

Please use standard units (milliseconds) for timeout.

Why? Because POSIX is doing that? I also got used to using standard units but then eventually i was setting timeout in characters so it is aligned when i change baudrate.

If we use standard units then it should be microseconds since on 1mbaud 1 character is ~10us and I would like to see macro for calculating character length in uart.h (e.g.UART_CHAR_USEC(baudrate))

[tbursztyka]

@galak The space being saved could be relevant on systems with numerous ports (such case exists in industry). Also, most of the built time pre-computed masks are directly usable and as such will take exact same amount of code as enum comparison. Anyway, to me, it's mostly a question of consistency against other APIs we have.

@nordic-krch What would be the use-case for such unit and macros? Sending 1 character takes around 10µs, but you need to take into account all the cycles necessary to program the controller, sets the buffers, get the int to actually tx or rx etc... Then 1ms as the smallest timeout sounds just right. Could you detail, because to me as well timeouts should be fine in ms. Also, we miss the timeout error here, -EGAIN.

[nordic-krch]

@tbursztyka If we have communication like HCI with ACK's and you might have short packets (<10 bytes) which last <100us @1mbaud. There will be turnaround time of 1ms because application will not receive packet until timeout (1ms later) so such long timeout will reduce uart channel capacity. Sending ten 10byte packets will take at least 10ms were actual transfers will take 10% of that.

We have seen such issues with communication over uart with PC. Timeout on PC side was limiting throughput so much that there was no significant difference between 115200 and 1M.

[tbursztyka]

There will be turnaround time of 1ms because application will not receive packet until timeout

in case of error only though yes.
And, isn't this use case valid only on blocking separate tx/rx calls?

This proposal is also trying to solve full duplex, so you could still tx, while rx would be pending, and you would get tx/rx termination events via cb. Wouldn't that fix your use case without the need for µs timeout?

Just asking, I am not against µs. If that one proves to be more relevant than ms, then fine. However, I am also concerned about driver side: exposed kernel primitives only takes ms (semaphore or else). Maybe the sys_clock.h _timeout then.

[Mierunski]

I guess this gets as good as it can. Removing -1 to make sure this doesn't have issues to go into 1.14.

+1 are also welcome

@nordic-krch @tbursztyka @nashif Please check as I think this is close to being merged

[pfalcon]

+1 for merging this for 1.14.

lgtm

[Mierunski]

Documentation issues fixed, @dbkinder Could you check uart.h file?

[Mierunski]

@nashif Could you merge this?

[Mierunski]

@nashif @galak @carlescufi ping

[carlescufi]

I suggest we merge this as part of the API meeting today.

[tautologyclub]

I don't see a good alternative on how some features can be implemented without callbacks. Also GPIO API is showing that they are simply needed. Some kind of signal mechanism would be a solution, but still I don't see alternatives now.

Someone mind explaining this to me? I.e. why can't the kernel run callbacks at "lower permission level"? Is it because we need a thread context for "user mode" to make sense as a concept? If so, is pushing work to the work queue also disallowed in user mode?

Another maybe obvious question: What's the problem with using the k_poll API? I.e. instead of registering an RX callback, we register a k_poll_event which the kernel signals with the appropriate event type using k_poll_raise.

[carlescufi]

I don't see a good alternative on how some features can be implemented without callbacks. Also GPIO API is showing that they are simply needed. Some kind of signal mechanism would be a solution, but still I don't see alternatives now.

Someone mind explaining this to me? I.e. why can't the kernel run callbacks at "lower permission level"? Is it because we need a thread context for "user mode" to make sense as a concept? If so, is pushing work to the work queue also disallowed in user mode?

Another maybe obvious question: What's the problem with using the k_poll API? I.e. instead of registering an RX callback, we register a k_poll_event which the kernel signals with the appropriate event type using k_poll_raise.

@tautologyclub that's a very interesting discussion. Callbacks have been around in Zephyr for much longer than k_poll has, so there's historical reasons.
That said, I think you raise a valid point, maybe deserving of its own GitHub issue so that it's generalized for all drivers/async subsystems.

[pfalcon]

I.e. why can't the kernel run callbacks at "lower permission level"?

Because when kernel calls into something, that something runs with the same privilege as kernel, aka that's security risk. Of course, that can be surmounted, by burning enough resources on that. But why?

Another maybe obvious question: What's the problem with using the k_poll API?

The same as above - it's orders of magnitudes less efficient than just executing a callback.

The designers of the API in this PR wanted a kernel-only, as close to zero-overhead as possible solution. It's very adhoc, very niche. I estimate a few parties would be interested in it.

Sooner or later we'll indeed have poll() (or more efficient epoll()) support based on the POSIX model.

[pabigot]

Another maybe obvious question: What's the problem with using the k_poll API?

The same as above - it's orders of magnitudes less efficient than just executing a callback.

Yes. Configurable reaction to an event should generally be provided by a callback, which should be documented as necessary to indicate its invocation context (from within an interrupt handler, with or without interrupts enabled, etc). Any constraints on what can be done within the callback should also be documented (e.g. see #12515). The mechanism actually used to convey the information from the callback to the application should then be under the application's control.

This is the most flexible way to allow the application to react in an appropriate way:

• immediately deal with the event (e.g. required for some gpio and timer interrupts where a reaction such as reconfiguring the device must be "atomic" with the event);
• put a task onto a work queue
• kick a semaphore/trigger a poll event

[tautologyclub]

I.e. why can't the kernel run callbacks at "lower permission level"?

Because when kernel calls into something, that something runs with the same privilege as kernel, aka that's security risk. Of course, that can be surmounted, by burning enough resources on that. But why?

Why? To enable a secure, usable userspace UART API of course. I don't think you answered my question though - it's obvious how running callbacks at escalated privilege level is a security concern. I was wondering why dynamically de-escalating the privilege level during a callback is not straight-forward.

[tautologyclub]

Btw, it wasn't clear to me that this API doesn't have userspace ambitions - if not it certainly makes the k_poll discussion moot. To be honest, the arguments presented against it presented thus far seems rather weak but I'm sure I'm missing something so I'll follow the new issue with interest, if one is created.

[pfalcon]

@tautologyclub

Why? To enable a secure, usable userspace UART API of course.

Patches welcome.

[pfalcon]

This is the most flexible way to allow the application to react

Except not "application", but "kernel-level handler". (And yes, there's a usecase to write applications as kernel-level handlers, no contradiction here, just as no security.)