cpu/efm32/uart: uart_init(): begin with TX pin at idle level #12380

benemorius · 2019-10-07T03:15:27Z

Contribution description

This corrects an unintentional momentary assertion of the UART TX pin during uart_init() which fixes some edge cases that are sensitive to that.

Currently when uart_init() calls gpio_init() the TX pin is set to output low. It remains low until some (configuration-dependent) time later when the UART hardware initializes it to the idle (high) level. In a lot of cases this hasn't caused a problem but of course it's wrong behavior.

Testing procedure

Reproducing the bug may be deceptively nontrivial without simply checking for it on a scope.

The specific case that triggered an issue for me was using LPUART at 9600 baud connected to a peer that has a pullup on its RX with an application that calls uart_write() very soon after uart_init() (hello-world does this).

The LPUART takes a while to initialize, so TX is asserted for about 200 usec which is significant and would normally be interpreted as a single garbage character. However since uart_write() is called so soon after this, the legitimate characters are also misinterpreted until the bus goes idle long enough for the receiver to resynchronize on the start bit of a clean character. For hello-world this means that ALL characters are received as garbage since there is no such idle period until the end of the output.

The green trace (2) is with this PR and looks normal. The orange trace (R1) is without this PR and is a perfect match to the green trace except for the extra pulse just before the legitimate characters begin. Despite the otherwise perfect match, all characters appeared in my terminal as garbage because of the extra pulse.

This took quite some effort to find since it's very easy to accidentally mask the buggy behavior and that's why I've ended up with such a detailed explanation for a small and obvious change.

else TX will be asserted low for a few usec until the peripheral is initialized and deasserts it

benpicco · 2019-10-07T08:34:09Z

I wonder: what happens when you don't call gpio_init() at all in uart_init()? On other platforms this is unnecessary as the alternative function will overwrite the GPIO configuration anyway. But maybe efm32 is different here.

In that case the change looks good and the oscilloscope trace shows it fixes a bug.

benemorius · 2019-10-07T08:42:33Z

I do think efm32 is different here but I share your curiosity and I've already forgotten what I found when I looked in to that so I'll look in to it again.

basilfx · 2019-10-07T08:46:42Z

I have checked the EMDRV library as a reference, and it does something similar (this method is invoked from both UARTDRV_InitUart and UARTDRV_InitLeuart).

The only thing it that we now have something that calls a RIOT-OS API with a magic (unsupported) option.

basilfx · 2019-10-07T08:51:46Z

cpu/efm32/periph/uart.c

@@ -68,7 +68,7 @@ int uart_init(uart_t dev, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)

    /* initialize the pins */
    gpio_init(uart_config[dev].rx_pin, GPIO_IN_PU);
-    gpio_init(uart_config[dev].tx_pin, GPIO_OUT);
+    gpio_init(uart_config[dev].tx_pin, GPIO_OUT | 1); /* 1 for high */


Would it be an option to invoke gpio_set(uart_config[dev].tx_pin)? That will also set the pin, and we're not using an undocumented option.

In the I2c driver, I do something similar (but for a different reason).

If we call gpio_set() first, it gets overridden by gpio_init(). And if we call it after, we still get an unwanted TX assertion even though the duration is much shorter.

benemorius · 2019-10-07T08:53:42Z

The only thing it that we now have something that calls a RIOT-OS API with a magic (unsupported) option.

I agree 100%. Do you think we can allow it for now and use it to exemplify the problem? Basically efm32 sets/clears the output during gpio_init() whereas other platforms don't touch it. This means on other platforms we can do gpio_set(); gpio_init() but on efm32 we can't. But I don't even like the gpio_set(); gpio_init() paradigm because it seems wrong to use a gpio before initializing it and maybe there's even a platform where that breaks if the gpio isn't clocked yet, though I haven't seen such a platform yet.

benpicco · 2019-10-07T09:02:10Z

Since you already HAVE_GPIO_MODE_T on efm32 you could extend it by GPIO_OUT_HIGH and GPIO_OUT_LOW to make it less magic. Using efm32 API extension in an efm32 periph driver should be fine.

Possibly gpio_init(); gpio_set(); is fast enough that you don't notice it though.

benemorius · 2019-10-07T10:18:50Z

Possibly gpio_init(); gpio_set(); is fast enough that you don't notice it though.

That still causes an assertion on the order of 1.5 usec which is surprisingly large. It's still enough to look like a start bit and cause a garbage character 0xff at 115200 baud. But even if it were much smaller I still wouldn't like it as a solution from a hardware-pedantic point of view.

basilfx · 2019-10-07T10:52:27Z

Another option is to replace the RIOT-OS API to initialize GPIO and use the EMLIB API directly.

benpicco · 2019-10-07T11:31:10Z

Another option is to replace the RIOT-OS API to initialize GPIO and use the EMLIB API directly.

Since you are using them anyway to initialize the UART, that would be fine too. But if that's more than one function call, better do something like GPIO_OUT_HIGH = GPIO_OUT | 1 in periph_cpu.h.

basilfx · 2019-10-07T11:44:06Z

I agree. But I'm not sure if extending the options is the best idea. Since @haukepetersen wrote most of gpio.h, he probably has an opinion about this ;-)

basilfx · 2019-10-30T21:13:16Z

@benpicco @benemorius I have patched the Gecko SDK API to expose an additional function that lets us decide if we want to change the pin state when setting the mode. That change is here. It's a compatible change, because it also provides the 'old' GPIO_PinModeSet as an inline function.

I also tested this very briefly in a separate branch.

@benemorius I think this should fix the issue. Can you give it a try? If so, then I will make it proper.

benpicco

As this is internal to efm32, using platform specific extensions to the GPIO API should be permissible.

basilfx · 2020-06-16T22:36:18Z

@benemorius were you able to check if the alternative branch works for you? If yes, than I will update Gecko SDK and provide that additional function.

benemorius · 2020-06-17T00:03:53Z

@basilfx I checked just now on tradfri after removing my patch and it works for me.

At first I got fatal: reference is not a tree but I changed 794d8214ac7069a987cb27fc13126d57d334c53a to feature/gpio_state_26 and then it compiled. Now I was able to add gpio_set() immediately before gpio_init() and this did solve the problem equivalent to this PR.

basilfx · 2020-06-17T06:46:24Z

Thanks for testing, I'll ensure that this new function will be in the next Gecko SDK update!

cpu/efm32/uart: uart_init(): begin with TX pin at idle level

8d7dc4e

else TX will be asserted low for a few usec until the peripheral is initialized and deasserts it

benpicco added Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 7, 2019

basilfx reviewed Oct 7, 2019

View reviewed changes

benpicco mentioned this pull request Oct 30, 2019

periph/gpio: add gpio_init_low() / gpio_init_high() helper functions #12612

Closed

benpicco approved these changes Feb 26, 2020

View reviewed changes

benpicco merged commit 98405fe into RIOT-OS:master Feb 26, 2020

leandrolanzieri added this to the Release 2020.04 milestone Mar 13, 2020

benemorius deleted the pr/efm32-uart-init-tx-idle branch June 17, 2020 00:03

basilfx mentioned this pull request Jun 18, 2020

pkg/gecko_sdk: update to v2.7.6 #14313

Merged

This was referenced Jul 1, 2020

stm32/periph/uart: extra byte transmitted on first transmission #8045

Closed

cpu/stm32/uart: gpio_set before gpio_init to avoid momentary assertion #14417

Closed

cpu/efm32/uart: uart_init(): begin with TX pin at idle level #12380

cpu/efm32/uart: uart_init(): begin with TX pin at idle level #12380

Uh oh!

Conversation

benemorius commented Oct 7, 2019

Contribution description

Testing procedure

Uh oh!

benpicco commented Oct 7, 2019

Uh oh!

benemorius commented Oct 7, 2019

Uh oh!

basilfx commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

basilfx Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benemorius Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

benemorius commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benemorius commented Oct 7, 2019

Uh oh!

basilfx commented Oct 7, 2019

Uh oh!

benpicco commented Oct 7, 2019

Uh oh!

basilfx commented Oct 7, 2019

Uh oh!

basilfx commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

basilfx commented Jun 16, 2020

Uh oh!

benemorius commented Jun 17, 2020

Uh oh!

basilfx commented Jun 17, 2020

Uh oh!

Uh oh!

basilfx commented Oct 7, 2019 •

edited

Loading

basilfx Oct 7, 2019 •

edited

Loading

benemorius commented Oct 7, 2019 •

edited

Loading

benpicco commented Oct 7, 2019 •

edited

Loading

basilfx commented Oct 30, 2019 •

edited

Loading