cpu/sam0_common/periph/uart: implement non-blocking write #12064

benpicco · 2019-08-22T11:45:10Z

Contribution description

This replaces the polling based UART TX implementation on the sam0 platform with an interrupt based one.
Thread-safe Ringbuffer is used for the TX buffer with a compile-time configurable buffer size. (May be tweaked.)

This violates the specification of uart_write() as this makes uart_write() non-blocking.

The non-blocking mode is now only used if the module periph_uart_nonblocking is used.

I wanted to have a non-blocking debug output when debugging a race condition, so I implemented this. Maybe a non-blocking uart_write could be generally useful though.

Testing procedure

add USEMODULE += periph_uart_nonblocking to your application.

Everything should behave as before, although the console output feels more snappy.
Only the shell / debug output have been tested.

Issues/PRs references

none

jcarrano · 2019-08-22T15:43:34Z

I experimented with this a while ago, but never PR'd it. Here is my branch:

jcarrano@6e7e64a

I defined a new function uart_write_async which blocks only if there is data waiting to be sent, otherwise it queues the current data and returns. I used a zero-copy approach by just storing a pointer to the buffer and then I redefined uart_write in terms of uart_write_async.

benpicco · 2019-08-26T13:39:21Z

I don't think zero-copy is the right approach for a general uart_write function though, e.g. you can't do zero-copy when using printf.

dylad · 2019-08-28T18:38:35Z

Isn't it better to create a dedicated pseudomodule for this ? the ROM cost looks heavy otherwise.
I can have a deeper look and run some test after the summit.

jcarrano · 2019-08-29T10:35:08Z

the ROM cost looks heavy otherwise.

Is there any application that stops fitting because of this? The cost of making it optional (at leat in the way it is done here and in most of riot) is a considerable increase in complexity and a decrease in readability, with all the ifdefs.

Add to that the fact that now you have two code paths in the uart driver that you have to test.

keestux · 2019-09-23T20:54:30Z

Is this a normal (allowed?) way of defining a regular macro?

USEMODULE += periph_uart_nonblocking

In most cases when I see #ifdef MODULE_blabla it means "if module blabla is present". But in this case, periph_uart_nonblocking is not a module. It is a macro to select a certain behaviour of the uart module.

Shouldn't we just have a macro (defined through CFLAGS) with a name such as PERIPH_UART_NONBLOCKING?

keestux · 2019-09-26T19:09:48Z

Notice that there is something called PSEUDOMODULES. I think makes sense to add periph_uart_nonblocking in makefiles/pseudomodules.inc.mk. No?

benpicco · 2019-09-26T19:26:31Z

There is already

PSEUDOMODULES += periph_%

in pseudomodules.inc.mk - this also matches periph_uart_nonblocking.

keestux · 2019-09-26T19:37:42Z

@benpicco Yes, you're right. So it's not a problem.

benpicco · 2019-10-30T15:20:46Z

I took the liberty to squash because it's more convenient being able to just grab the patch when debugging race conditions.

dylad

any reason to keep SAM D&L 21 away from these changes ?

dylad · 2019-11-13T09:20:32Z

Makefile.dep

+ifneq (,$(filter periph_uart_nonblocking, $(USEMODULE)))
+  USEMODULE += periph_uart
+endif
+


Why is this needed ?

So everything that depends on periph_uart is also fulfilled with periph_uart_nonblocking

But should it belongs to Makefile.dep ? Why not cpu/sam0_common/Makefile.dep ?

So other platforms can implement it too.

benpicco · 2019-11-13T13:01:33Z

any reason to keep SAM D&L 21 away from these changes ?

I also tested this on samr21-xpro. Only saml1x and samd5x have a separate TXE IRQ.

aabadie

I've looked quickly at this PR. I have no comment on the implementation of the non blocking uart mode in sam0 (I'm not an expert on these platforms) but I think the integration in the build system needs a complete rework:

add FEATURES_PROVIDED += periph_uart_nonblocking to cpu/sam0_common/Makefile.features
in Makefile.dep, just add:
```
ifneq (,$(filter periph_uart_nonblocking,$(USEMODULE)))
  FEATURES_REQUIRED += periph_uart
endif
```
instead of the current version (See how this is already done for periph_gpio/periph_gpio_irq in Makefile.dep)
Remove all uses and definition of the USE_NONBLOCKING macro and just replace the uses of #if USE_NONBLOCKING by #ifdef MODULE_PERIPH_UART_NONBLOCKING
Remove all cpu/samxx/Makefile.dep introduced by this PR: they are empty and thus useless.
add a test application with:
```
FEATURES_REQUIRED += periph_uart periph_uart_nonblocking
```
in its makefile. This application will print some data and this should work. A Python test script could be added for this purpose.
Note that, thanks to the use FEATURES_REQUIRED, this application will only be built for sam0 based boards as only sam0 provides this feature (for the moment). The good thing it that it will ensure that the CI is building the nonblocking related code.

I'm blocking this PR until the mentioned problems are fixed and a dedicated test application is added.

benpicco · 2019-11-15T14:08:24Z

Remove all cpu/samxx/Makefile.dep introduced by this PR: they are empty and thus useless.

They are all but useless. This implementation depends on tsrb, this dependency is resolved in sam0_common/Makefile.dep so the sam0-siblings have to include it.

I've addressed all the other comments.
With the test application I get

== printed in 2100735/2100000 µs ==

with periph_uart_nonblocking and

== printed in 2152407/2100000 µs ==

with periph_uart.

aabadie · 2019-11-15T16:04:51Z

They are all but useless.

You are right. I don't know why I thought they were initially empty.

Thanks for updating following the other suggestions. Just one is still missing: add a Python script for automatic testing.

benpicco · 2019-11-24T21:50:50Z

I added a small Python script.
When using periph_uart instead of periph_uart_nonblocking the test does fail as expected.

My comments are addressed.

aabadie · 2019-11-25T06:48:37Z

@dylad, can you have a look at this one when you have time ? I'm fine with the test application and automatic test script. The build system integration is also ok for me.
Just the important part, e.g the non blocking uart code, needs a review now :)

dylad

SAML21 isn't happy. See comment below.
Can someone doublecheck on SAMD21 just to be sure ?

dylad · 2019-11-25T11:10:59Z

cpu/sam0_common/periph/uart.c

        NVIC_EnableIRQ(SERCOM0_2_IRQn + (sercom_id(dev(uart)) * 4));
 #else
+        /* enable UART ISR */
        NVIC_EnableIRQ(SERCOM0_IRQn + sercom_id(dev(uart)));


Got a hard time debugging this one.
In the current shape the provided test app doesn't work at all on SAML21.
No DRE interrupt is triggered. In fact, the NVIC_EnableIRQ is never call because there is no callback provided so we did not enter here.
The callback is not provided because weren't using a shell.
So we need a second way to enable the NVIC in this specific case (TX only, no RX).

Yea you are right.
I only tested the non-callback case on samd5x, for samd2x/saml2x the UART interrupt was only enabled when a callback was provided.
It is now also enabled when MODULE_PERIPH_UART_NONBLOCKING is defined.

dylad · 2019-11-25T15:22:36Z

cpu/sam0_common/periph/uart.c

+    NVIC_EnableIRQ(SERCOM0_0_IRQn + (sercom_id(dev(uart)) * 4));
+#else
+    /* enable UART ISR */
+    NVIC_EnableIRQ(SERCOM0_IRQn + sercom_id(dev(uart)));


In some case, the NVIC_EnabeIRQ is called twice, this might be confusing.
I would rather go to something like :

if ((rx_cb) && (uart_config[uart].rx_pin != GPIO_UNDEF)) { uart_ctx[uart].rx_cb = rx_cb; uart_ctx[uart].arg = arg; #ifdef UART_HAS_TX_ISR /* enable RXNE ISR */ NVIC_EnableIRQ(SERCOM0_2_IRQn + (sercom_id(dev(uart)) * 4)); #else /* enable UART ISR */ NVIC_EnableIRQ(SERCOM0_IRQn + sercom_id(dev(uart))); #endif /* UART_HAS_TX_ISR */ dev(uart)->CTRLB.reg |= SERCOM_USART_CTRLB_RXEN; dev(uart)->INTENSET.reg |= SERCOM_USART_INTENSET_RXC; /* set wakeup receive from sleep if enabled */ if (uart_config[uart].flags & UART_FLAG_WAKEUP) { dev(uart)->CTRLB.reg |= SERCOM_USART_CTRLB_SFDE; } } #if defined(MODULE_PERIPH_UART_NONBLOCKING) else { NVIC_EnableIRQ(SERCOM0_IRQn + sercom_id(dev(uart))); } #if defined(UART_HAS_TX_ISR) /* enable TXE ISR */ NVIC_EnableIRQ(SERCOM0_0_IRQn + (sercom_id(dev(uart)) * 4)); #endif #endif /* MODULE_PERIPH_UART_NONBLOCKING */

dylad · 2019-11-26T07:57:37Z

This now works on SAML21 but I tested on SAML10 and SAML11 and here is result WITH uart_nonblocking:

2019-11-26 08:54:20,067 # m�ain(): This is RIOT! (Version: 2020.01-devel-426-ge36b5-sam0-buffe
red_uart)
2019-11-26 08:54:20,068 # 
2019-11-26 08:54:20,166 # J�oin us now and share the software;
2019-11-26 08:54:20,268 # Y�ou'll be free, hackers, you'll be free.
2019-11-26 08:54:20,368 # J�oin us now and share the software;
2019-11-26 08:54:20,470 # Y�ou'll be free, hackers, you'll be free.
2019-11-26 08:54:20,568 # 
2019-11-26 08:54:20,671 # �H�oarders can get piles of money,
2019-11-26 08:54:20,773 # T�hat is true, hackers, that is true.
2019-11-26 08:54:20,874 # B�ut they cannot help their neighbors;
2019-11-26 08:54:20,976 # T�hat's not good, hackers, that's not good.
2019-11-26 08:54:21,074 # 
2019-11-26 08:54:21,177 # �W�hen we have enough free software
2019-11-26 08:54:21,279 # A�t our call, hackers, at our call,
2019-11-26 08:54:21,380 # W�e'll kick out those dirty licenses
2019-11-26 08:54:21,481 # E�ver more, hackers, ever more.
2019-11-26 08:54:21,580 # 
2019-11-26 08:54:21,684 # �J�oin us now and share the software;
2019-11-26 08:54:21,785 # Y�ou'll be free, hackers, you'll be free.
2019-11-26 08:54:21,886 # J�oin us now and share the software;
2019-11-26 08:54:21,988 # Y�ou'll be free, hackers, you'll be free.
2019-11-26 08:54:22,085 # 
2019-11-26 08:54:22,190 # �=�= printed in 2105365/2100000 µs ==
2019-11-26 08:54:22,191 # [SUCCESS]

The output is fine without the uart_nonblocking module. I'll try to investigate further.

dylad · 2019-11-26T08:45:13Z

One really quick&dirty workaround is the following:

#ifdef MODULE_PERIPH_UART_NONBLOCKING
static inline void irq_handler_tx(unsigned uartnum)
{
    if(!tsrb_avail(&uart_tx_rb[uartnum])) {
        return;
    }
    dev(uartnum)->DATA.reg = tsrb_get_one(&uart_tx_rb[uartnum]);
    /* disable the interrupt if there are no more bytes to send */
    if (tsrb_empty(&uart_tx_rb[uartnum])) {
        dev(uartnum)->INTENCLR.reg = SERCOM_USART_INTENSET_DRE;
    }
}
#endif

But I'd like to find a better patch...

dylad

Tested work on SAMR21 (Iotlab), SAML10, SAML11, SAML21, SAME54.
Please squash !

Implement interrupt based uart_write() using a tsrb for the TX buffer. To enable it, add USEMODULE += periph_uart_nonblocking to your Makefile.

dylad · 2019-11-28T07:55:20Z

static tests failed, you can amend directly

The application is mainly to compile-test non-blocking UART functionality, but some functional testing is also possible. With non-blocking UART the total runtime of the program is 2100735 µs on same54-xpro. With blocking UART the total runtime is 2152407 µs.

dylad · 2019-11-28T09:07:06Z

Here we go ! Thanks for the PR @benpicco !

benpicco · 2019-11-28T09:10:07Z

Thank you for the review!

benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: drivers Area: Device drivers labels Aug 22, 2019

benpicco changed the title ~~[RFC] cpu/sam0_common/periph/uart: implement non-blocking write~~ cpu/sam0_common/periph/uart: implement non-blocking write Sep 23, 2019

benpicco requested review from dylad and bergzand September 26, 2019 19:27

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 26, 2019

benpicco removed the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Oct 30, 2019

benpicco force-pushed the sam0-buffered_uart branch 2 times, most recently from 8f61d99 to fb96555 Compare October 30, 2019 15:18

dylad reviewed Nov 13, 2019

View reviewed changes

aabadie previously requested changes Nov 14, 2019

View reviewed changes

benpicco force-pushed the sam0-buffered_uart branch from a5bfd13 to ff4c18b Compare November 24, 2019 21:48

dylad requested changes Nov 25, 2019

View reviewed changes

dylad reviewed Nov 25, 2019

View reviewed changes

dylad approved these changes Nov 27, 2019

View reviewed changes

dylad added this to the Release 2020.01 milestone Nov 27, 2019

cpu/sam0_common/periph/uart: implement buffered write

0d977b3

Implement interrupt based uart_write() using a tsrb for the TX buffer. To enable it, add USEMODULE += periph_uart_nonblocking to your Makefile.

benpicco force-pushed the sam0-buffered_uart branch from 9fdebe8 to 8e35c24 Compare November 27, 2019 18:01

benpicco force-pushed the sam0-buffered_uart branch from 8e35c24 to 0c08abd Compare November 28, 2019 08:27

dylad merged commit 6a4259e into RIOT-OS:master Nov 28, 2019

benpicco deleted the sam0-buffered_uart branch November 28, 2019 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/sam0_common/periph/uart: implement non-blocking write #12064

cpu/sam0_common/periph/uart: implement non-blocking write #12064

benpicco commented Aug 22, 2019 •

edited

Loading

jcarrano commented Aug 22, 2019

benpicco commented Aug 26, 2019

dylad commented Aug 28, 2019

jcarrano commented Aug 29, 2019

keestux commented Sep 23, 2019

keestux commented Sep 26, 2019

benpicco commented Sep 26, 2019

keestux commented Sep 26, 2019

benpicco commented Oct 30, 2019

dylad left a comment

dylad Nov 13, 2019

benpicco Nov 13, 2019

dylad Nov 13, 2019

benpicco Nov 15, 2019

benpicco commented Nov 13, 2019 •

edited

Loading

aabadie left a comment

benpicco commented Nov 15, 2019

aabadie commented Nov 15, 2019

benpicco commented Nov 24, 2019

aabadie commented Nov 25, 2019

dylad left a comment

dylad Nov 25, 2019 •

edited

Loading

benpicco Nov 25, 2019

dylad Nov 25, 2019

dylad commented Nov 26, 2019

dylad commented Nov 26, 2019

dylad left a comment

dylad commented Nov 28, 2019

dylad commented Nov 28, 2019

benpicco commented Nov 28, 2019

cpu/sam0_common/periph/uart: implement non-blocking write #12064

cpu/sam0_common/periph/uart: implement non-blocking write #12064

Conversation

benpicco commented Aug 22, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

jcarrano commented Aug 22, 2019

benpicco commented Aug 26, 2019

dylad commented Aug 28, 2019

jcarrano commented Aug 29, 2019

keestux commented Sep 23, 2019

keestux commented Sep 26, 2019

benpicco commented Sep 26, 2019

keestux commented Sep 26, 2019

benpicco commented Oct 30, 2019

dylad left a comment

Choose a reason for hiding this comment

dylad Nov 13, 2019

Choose a reason for hiding this comment

benpicco Nov 13, 2019

Choose a reason for hiding this comment

dylad Nov 13, 2019

Choose a reason for hiding this comment

benpicco Nov 15, 2019

Choose a reason for hiding this comment

benpicco commented Nov 13, 2019 • edited Loading

aabadie left a comment

Choose a reason for hiding this comment

benpicco commented Nov 15, 2019

aabadie commented Nov 15, 2019

benpicco commented Nov 24, 2019

aabadie commented Nov 25, 2019

dylad left a comment

Choose a reason for hiding this comment

dylad Nov 25, 2019 • edited Loading

Choose a reason for hiding this comment

benpicco Nov 25, 2019

Choose a reason for hiding this comment

dylad Nov 25, 2019

Choose a reason for hiding this comment

dylad commented Nov 26, 2019

dylad commented Nov 26, 2019

dylad left a comment

Choose a reason for hiding this comment

dylad commented Nov 28, 2019

dylad commented Nov 28, 2019

benpicco commented Nov 28, 2019

benpicco commented Aug 22, 2019 •

edited

Loading

benpicco commented Nov 13, 2019 •

edited

Loading

dylad Nov 25, 2019 •

edited

Loading