drivers: peripheral UART driver interface overhaul

[haukepetersen]

As already merged for GPIO and PR's for SPI (#3216), here is a slight overhaul to the peripheral UART driver. The most notable changes:

• made UART types configurable on a CPU basis
• merged uart_init_blocking() and uart_init() into one function, dropping the blocking mode
• dropped the uart_receive_blocking() function
• changed usage functions to void xxx(...)

The main reasons for removing the blocking mode are: (i) the blocking mode is in-efficient and I believe we should not push it's usage by providing this interface and (ii) it was not used anywhere...

For discussion I would like to focus on the changes to the UART interface first. Once we agree on them, I will adjust all the boards/CPUs if neccessary.

[jnohlgard]

I suggest sizeof(uart_config) / sizeof(uart_config[0]) instead

[haukepetersen]

any particular reasons for this? I don't mind, just try to understand the benefits...

[jnohlgard]

Not a strong reason, but it doesn't depend on the type of uart_config any more.

[jnohlgard]

I like the changes in general. However, I don't understand the detailed usage for TX, how is uart_begin_tx and uart_write supposed to be used together?

Is it possible to change the rx/tx callbacks after initializing the module?
Are there any use cases where it would be desirable to change callbacks without reinitializing?

Will a default TX callback functionality be added to the write syscall handler for stdout?
The stdout update may be related to the file descriptor discussion we had earlier (I forgot the PR/Issue number)

[jnohlgard]

How should we handle printf() from within ISR context?

Newlib may call a flush from inside printf which is expected to be blocking, if we don't block then buffered characters may be lost.

In particular, it must be possible to print crash information in core_panic, even if called from ISR context, before rebooting. (I don't think core_panic is actually ever used outside of ISRs)

The printf from inside ISR has caused problems for me in the past.

[haukepetersen]

TX is indeed the slightly more advanced part of the UART driver. But I see no problem for implementing printf or flush in a sensible way with the existing interface:

In the syscalls/uart_syscalls implementation, we define a ringbuffer for TX data. When ever printf is called, the following happens:

• add data to ringubffer
• call uart_tx_begin()

The TX callback should look more or less like this:

int tx_cb(arg *arg)
{
    if (tx_buf.avail > 0) {
        char data = ringbuffer_get_one(&tx_buf);
        uart_write(DEV, data);
        return 1;
    }
    return 0;

flush could be easily implemented by looping of while (tx_buf.avail > 0) with some delay like something ~10ms or so in between.

Is it possible to change the rx/tx callbacks after initializing the module?
Are there any use cases where it would be desirable to change callbacks without reinitializing?

There is no way at the moment, but neither do I really see a use case for this. But you can of course always call uart_init again.

[jnohlgard]

@haukepetersen What about the case of printf from inside ISR, how do we handle that?

[haukepetersen]

same, it does not make any difference if you call it from inside an ISR: all printf does is basically filling a ringbuffer and setting the uart TX isr interrupt. All we need to make sure, that we disable interrupts whenever tinkering the the TX buffer, so that there is no concurrent write access to the buffer...

[jnohlgard]

@haukepetersen What happens if the buffer gets full? Does the printf block? if so, we will get a deadlock if the blocking printf was called from inside an ISR.

[haukepetersen]

nope, I would not block, Id say we just throw away anything that does not fit into the buffer. Also we could implement both: block, but only if not in interrupt context. This is just a question of how we implement the syscalls, this is completely independent from the UART driver...

[jnohlgard]

I did a check for PRIMASK in #2605 in blocking mode, maybe some similar solution may be appropriate here?

https://github.com/RIOT-OS/RIOT/pull/2605/files#diff-0f87a847fbd98c6b77134bcd0c723ee5R363

[haukepetersen]

PRIMASK would not work here, as it is not portable. We already have the inISR() function (which calls internally a PRIMASK check on Cortex-Mx platforms), so I would go with that.

[jnohlgard]

Right, I was confused by the name, since IRQs may be disabled outside of
ISR.
Should the function inISR be renamed "irq_enabled" perhaps? That's a
separate issue though.
On Jun 29, 2015 1:11 PM, "Hauke Petersen" notifications@github.com wrote:

PRIMASK would not work here, as it is not portable. We already have the
inISR() function (which calls internally a PRIMASK check on Cortex-Mx
platforms), so I would go with that.

—
Reply to this email directly or view it on GitHub
#3265 (comment).

[haukepetersen]

Sure, we could rename it (and do also s/enableIRQ()/irq_enable()/ while we are at it), but that's some other issue...

[jnohlgard]

PR needs rebase

[haukepetersen]

rebased.

[kaspar030]

The main reasons for removing the blocking mode are: (i) the blocking mode is in-efficient and I believe we should not push it's usage by providing this interface and (ii) it was not used anywhere...

This (ii) is unfortunately not correct. Our newlib _write() implementation only used blocking mode, and many platforms only implemented that. So essentially, only blocking mode was ever used.

I'm with @gebart here, we need to find a rock-solid concept for ISR and crash cases here, otherwise we'll cause more trouble than the performance gain can justify.

[haukepetersen]

This (ii) is unfortunately not correct. Our newlib _write() implementation only used blocking mode, and many platforms only implemented that. So essentially, only blocking mode was ever used.

Almost, the uart_write_blocking() is used in the syscalls (and that function is still part of the interface). but the uart_read_blocking() was not used, and that one I threw out of the interface...

[haukepetersen]

I'm with @gebart here, we need to find a rock-solid concept for ISR and crash cases here, otherwise we'll cause more trouble than the performance gain can justify.

I agree a 100%!

[kaspar030]

but the `uart_read_blocking()` was not used, and that one I threw out of the interface...

Ah ok, perfect!

[haukepetersen]

After some more test and discussions offline, I updated the UART interface once more. I decided for a different concept when sending data. The main reasons for this are:

• sending data using DMA was not possible, as the interface forced the use of interrupt driven sending or active waiting
• the old concept was hard to understand by people who just wanted to use the interface
• I run into trouble on many platforms due to erratas on the handling of the TX interrupt bit

Now there is only once send function uart_write. This function get's a buffer and the length as arguments and only returns, once the buffer is send. It is then up to the low-level driver implementation, how this sending is handled. This may be with active waiting, interrupt driven or using DMA - all depends how much effort is spend on the driver...

A test implementation is done for the stm32f4 -> stm32f4discovery

@daniel-k, @PeterKietzmann, @gebart, @kaspar030: What to you think about this change? If you approve, I will finally go through all the CPUs again and adapt them.

[daniel-k]

Now there is only once send function uart_write. This function get's a buffer and the length as arguments and only returns, once the buffer is send.

I haven't looked at any implementation yet, just at periph/uart.h that basically has the same wording: once the buffer is send If take this literally it would mean, that any uart_write, no matter which mechanism (DMA, IRQ, blocking) is used, would block the calling thread for long times, just imagine low baud rates. Did you maybe mean to say once the buffer is consumed, i.e. the buffer is copied somewhere, e.g. DMA takes care of sending, and control is transfered back to the caller? This would make much more sense to me.

[haukepetersen]

No, I actually menat 'once the buffer is send'. The idea is, that the function really does block for the time it takes to send the data and only returns after the bytes have been send out. This does however not implay, that the thread is actively blocking, as it is doing when using the current uart_send_blocking. When switching to interrupt driven or better DMA, it would mean that the function waits on a mutex or similar, allowing the CPU to go to sleep, until the data is send.

Making the send function synchronous as this reduces the implementation complexity by some factors and removes on many occasions the need for double buffering, hence making the use of UART more memory efficient.

[daniel-k]

I see your point. The use case I had recently was that I had a lot debug output in some relatively timing sensitive code that could only be debugged reasonably with non-blocking UART. I'm not sure if there are others.
Of course you can implement a debugging thread then that buffers the output, but this increases complexity and overhead by more than you save here I guess.

[kaspar030]

What to you think about this change?

I like it a lot. The interface is probably as simple as possible now.

[OlegHahm]

Also I like the proposal in general, I think @daniel-k has a very valid point here.

(And to be a smartass, the documentation should be once the buffer is sent (or has been sent)).

[kaspar030]

I think @daniel-k has a very valid point here.

We should introduce caching, but not at this level.

[haukepetersen]

@daniel-k I see your use-case. But in general, I don't see printf as the vehicle of choice when it comes to debugging time-sensitive code! There are ways far more suited for debugging this kind of things, e.g. Logic analyzers + gpio pins or similar...

We should introduce caching, but not at this level.

Yes! This was always the idea with this interface.

[daniel-k]

@haukepetersen
Sure debugging with GPIOs is nice for getting the timing right, but not to get internal states of your code. There are sophisticated debug tools for ARM, but unfortunately there's no free toolchain and cheap debugger that supports them.

In my case the code breaks when the debugging introduces too big delays, but I can't just debug with 2 GPIOs. A trace tool would be the right choice.

[OlegHahm]

And not everyone has everywhere access to logic analyzers. Think for, e.g. testbeds.

[kaspar030]

The issue is not whether we want fast uart output, but if it should become asynchronous at this level (within the uart interface).

[OlegHahm]

Yes, I agree with this.

[OlegHahm]

So, basically we need an accompanying PR that introduces this caching somewhere above, right?

[kaspar030]

@OlegHahm Wasn't all uart output synchronous before?

[OlegHahm]

Was it? I dunno. If this is true, yes, we can work on this later on.

[daniel-k]

I think @kaspar030 is right. But @haukepetersen was introducing interrupt-driven UART with this PR that got me excited and now you take that toy away again :-D

[haukepetersen]

Was it?

YES.

But @haukepetersen was introducing interrupt-driven UART with this PR

Not quite. Interrupt driven UART did exist before... And to clarify - I am not taking it away again, the decision of how to use the UART is simply moved from the user of the interface into the implementation of the interface.

[sgso]

This function is blocking, as it will only return after @p len bytes from the given buffer have been sent

For UARTs with integrated FIFO buffers, I assume it's ok to just pass the data to it and return? (and check on the next call, if the FIFO buffer can be written again?)

[haukepetersen]

For UARTs with integrated FIFO buffers, I assume it's ok to just pass the data to it and return? (and check on the next call, if the FIFO buffer can be written again?)

Yes.

[haukepetersen]

closed in favor of #4114