Description on UART API (uart.h) is vague, semantics unclear, possibly questionable

[zephyrbot]

Reported by Paul Sokolovsky:

Reading thru uart.h, there're few questionable docstrings for functions, let's go thru the few:

{code:java}

• @brief Enable RX interrupt in IER.
  static inline void uart_irq_rx_enable(struct device *dev)
  {code}

Using acronyms like "IER" leaves an impression that the API is adhoc, designed/tailored for a particular hardware, nor a generic UART interface.

{code:java}

• @brief Check if nothing remains to be transmitted
  static inline int uart_irq_tx_empty(struct device *dev)
  {code}

The semantics from the description doesn't seem to be useful. The expected meaning of "tx empty" (from how corresponding condition/IRQ/status bits are usually called in various SoCs) is "there's a room in tx FIFO". That's useful, because in that case we can write more data into FIFO. Note that this works even for degenerate case of FIFO of depth 1 (i.e. single TX data register).

{code:java}

• @brief Check if Rx IRQ has been raised.
  static inline int uart_irq_rx_ready(struct device *dev)
  {code}

That's too generic. An apparent description should be "check if one or more characters are ready in receive FIFO" (also works for a case of single RX data reg).

{code:java}

• @brief Check if Tx IRQ has been raised.
  static inline int uart_irq_tx_ready(struct device *dev)
  {code}

Similar to rx_ready, and with semantics of tx_empty clearly defined, the meaning of this should be "entire tx FIFO has been transmitted". This event is rarely useful in practice. A case when it's useful is e.g. when one wants to disable UART block (e.g. to save power). Then, one needs to wait for transmission to complete before doing this. It would make sense to rename this to tc_complete.

Note that I may get semantics of tx_empty vs tx_ready reversed. But again, this is based on how vendors often (well, maybe sometimes) call the things, at least for a case when there's single TX register. It being empty means that more data can be written into it. So, if the original semantics is reversed to what I propose, then there's still possibility that some implementer may get it wrong. Then we probably should rename the calls to disambiguate. As an example, STM32 implements both tx_empty and tx_ready using the same underlying flag, TXE (tx register empty), which can't be right. Note also that tx_enable and tx_disable work with another bit, TC (tx complete), which is also likely not right (before commit 8c079e9, it worked with TXE[IE]).

{code:java}

• @brief Update cached contents of IIR.
  static inline int uart_irq_update(struct device *dev)
  {code}

I would say what this call should do is clear, unfortunately, the description throws it all off. It should update cached contents of IIR, but what is there's no "IIR"? Undefined behavior, anything is possible. Indeed, STM32 for some reason acknowledges (resets) TC bit. Even the datasheet says: "The TC bit can also be cleared by writing a '0' to it. This clearing sequence is recommended only for multibuffer communication." (multibuffer == apparently double-buffered DMA one, which we don't do).

(Imported from Jira ZEP-2016)

[zephyrbot]

by Paul Sokolovsky:

I would say what this call should do is clear

To pronounce it explicitly: some hardware has auto-acknowledge for interrupts, i.e. just reading an interrupt status register resets pending interrupts bits in it. uart_irq_update() is thus supposed to read UART int status register once, and store it in a variable. Afterwards, uart_irq_rx_ready, uart_irq_tx_ready, uart_irq_tx_empty (err checks if any) are supposed to work off this variable.

This can't work for STM32 due to artifacts it does in uart_irq_update(), that's why https://gerrit.zephyrproject.org/r/#/c/12794/1/subsys/console/getchar.c , line 38 has it commented. Before it was commented, I experienced following behavior: pressing a key into a uart echo application didn't lead to echo of this key. When a(nother) key was pressed, both key were echoed. However, pasting 3, 4, 5, etc. char led to them being echoed right away.

[zephyrbot]

by Daniel Thompson:

The expected meaning of "tx empty" (from how corresponding condition/IRQ/status bits are usually called in various SoCs) is "there's a room in tx FIFO".

How did you figure that? I can only see one driver (that has FIFOs enabled) where tx empty behaves like this (and it is on a niche platform with a comment saying the proper behaviour can't be supported). For sure the frequently used drivers, such as 16550, don't seem to work like this.

[zephyrbot]

by Paul Sokolovsky:

How did you figure that?

From the reverse logic ("what APIs we need to implement something like https://gerrit.zephyrproject.org/r/#/c/12794"), backed by previous experience with this stuff. Terminology however varies wildely - empty, not full, underflow, low watermark, whatever. I understand this logic is rather partial, and in Zephyr case, I can back it only by stm32 driver (sorry I'm backlogged on looking at the other drivers, that's in my TODO, but to even reason well about stm32 one, I'm backing it by reading stm32 datasheet, plus I'm overwhelmed by health matters).

In other case, what we discuss is how UART API definition is confusing, and how various people may be confused with it. So, even if my reasoning is confused itself, well, the more usecases we have, to come up with the ultimate deconfusion plan.

[zephyrbot]

by Daniel Thompson:

In terms of describing the expected behaviour of the current API I think the 16550 is a much more sensible reference implementation. Its been there since the first public commit and the description of the current UART API was copied from it. The STM32 UART driver is relatively new and based on Paul's investigation has a number of fairly serious bugs (long busy wait during FIFO fill, calling uart_irq_update() interferes with data delivery and printk() stops working when we preempt other serial activity). IMHO the STM32 deviations from the prior art are "merely" bugs and should be fixed regardless of anything else.

Having said that it would be good to improve the documentation. There are certain (non-API) behaviours of the 16550 that cannot be copied on other platforms. For example, a UART with a level triggered FIFO half-empty interrupt can never behave like the latched interrupts of a 16550. This it would be especially useful to make clear to callers that not all UART implementations will lower their IRQ at the same point in the IRQ service sequence and to recommend a sequence that is guaranteed to service the interrupt.

[zephyrbot]

by Paul Sokolovsky:

Daniel Thompson : Thanks, the ideas here and in the mail are good. As my mind is still overwhelmed with other things, I created a helper spreadsheet to survey how different serial drivers do the things: https://docs.google.com/spreadsheets/d/1e4hx9iVmAp5pBSTd2namF_VdsiMBS-OmlJUI4UPBMN4/edit#gid=0

From it, it's clear that stm32 driver is the only one which does "too much" in irq_update() call, and so this issue is independent on any other possible changes, so let me prepare and test patch just for that for starters.

[zephyrbot]

by Paul Sokolovsky:

Daniel Thompson : Please have a look at tests/drivers/uart/uart_basic_api/src/test_uart_fifo.c:

{code:java}
if (uart_irq_tx_ready(dev)) {
data_transmitted = true;
char_sent++;
}
{code}

So, this tests thinks that "tx_ready" has semantics of "tx complete" (vs "tx is ready to transmit another character"). Just a singled-out fact, let me catch up with review of other sources.

[zephyrbot]

by Paul Sokolovsky:

Also note that test_fifo_fill() there calls uart_fifo_fill() with len=1. Let me assess that as "the author of the test knew that it's unsafe to call it with len > 1 with the current driver base" ;-).

[zephyrbot]

by Daniel Thompson:

Let me assess that as "the author of the test knew that it's unsafe to call it with len > 1 with the current driver base" .

I'm not sure about this assessment.

I cannot think of any test author I have ever met who writes test and hopes the pass! All test authors are motivated by wanting to prove there are bugs so deliberately setting the length to 1 to avoid finding a bug is both insane and not motivating for a test author.

[zephyrbot]

by Paul Sokolovsky:

{quote}From it, it's clear that stm32 driver is the only one which does "too much" in irq_update() call, and so this issue is independent on any other possible changes, so let me prepare and test patch just for that for starters.{quote}

That was posted as https://gerrit.zephyrproject.org/r/#/c/12917 , and got immediately caught into another layer of issues. So, tests/drivers/uart/uart_basic_api (quite arbitrarily) assumes that it's possible to call any UART API function whenever one likes. Well, in the presence of interrupts, it's not safe to call uart_fifo_fill() outside of ISR. Actually not, it's unsafe not to call uart_fifo_fill() inside ISR! Note that tests/drivers/uart/uart_basic_api smartly calls uart_fifo_read() inside ISR, but not uart_fifo_fill().

Daniel Thompson : Thoughts?

[zephyrbot]

by Paul Sokolovsky:

Another specimen of serial interface handling: https://github.com/01org/zephyr.js/blob/master/src/ashell/comms-uart.c#L320 .

[zephyrbot]

by Paul Sokolovsky:

API documentation fixes for this issue went into 1.8.0: #49 , #159, #281 .

What's left here is to actually go thru the existing drivers and make them compliant with the elaborated definition (specifically, make sure that "tx_ready" and "tx_complete" are implemented differently (many drivers implement one in terms of another) and that superfluous event masking is removed). This activity may requires consulting of the individual SoCs, so may take some time.

[zephyrbot]

by Anas Nashif:

can we close this one then and open other jira issue for the drivers?

[zephyrbot]

by Paul Sokolovsky:

Sounds good, closing per above.