cpu/stm32: Fix garbage on UART init #14426

maribu · 2020-07-02T13:08:26Z

Contribution description

cpu/stm32: Fix gpio_init() / gpio_int_af()

Do not set an intermediate mode, prepare correct mode settings in a temporary variable
Consistently enabled the GPIO periph in gpio_init_af()
- Previously, STM32 F1 did not require a separate call to gpio_init() prior to a call of gpio_init_af(), but other STM32 families did
- Now, gpio_init_af() can be used without gpio_init() consistently
STM32 F1: Do not touch ODR for non input pins
- For input pins, this enables / disabled pull up resistors. For outputs, this register should remain untouched (according to API doc)

cpu/stm32: Fix uart_init()

Make use of the fact that gpio_init_af() does not need prior call to gpio_init() for all STM32 families anymore and drop call to gpio_init()
Initialize the UART periph first, before initializing the pins
- While uninitialized, the UART periph will send signal LOW to TXD. This results in a start bit being picked up by the other side.
- Instead, we do not connect the UART periph to the pins until it is initialized, so that the TXD level will already be HIGH when the pins are attached.
- This results in no more garbage being send during initialization

Testing procedure

UART should still work for STM32F1 and STM32Fx (x != 1)
gpio_init() should still work for ST32F1 and STM32Fx (x != 1)

Note: It should be sufficient to test with STM32F1 and one of the other STM32 not belonging to the STM32F1 family

Issues/PRs references

Fixes #8045

benpicco · 2020-07-02T13:56:58Z

cpu/stm32/periph/gpio_all.c

+    return _gpio_init(pin, mode, 0);
+}
+
+int gpio_init_set(gpio_t pin, gpio_mode_t mode)


How about gpio_init_high() 😉

Then, should #12612 get merged we just have to define GPIO_HAVE_INIT_LEVEL

fjmolinas · 2020-07-03T15:08:38Z

Is � in:

2020-07-03 17:07:25,205 # �main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-pr-14426)
2020-07-03 17:07:25,206 # Hello World!
2020-07-03 17:07:25,211 # You are running RIOT on a(n) nucleo-f103rb board.
2020-07-03 17:07:25,214 # This board features a(n) stm32 MCU.

the garbage? do I need to interface directly over uart or though the debugger works? Or do I need a logic analyzer?

maribu · 2020-07-03T17:55:05Z

Is � in:

Yep, exactly. With a logic analyzer you should be able to confirm that the initialization is only "almost flicker free", as the pin sadly still goes briefly down during initialization. However, with this PR the duration of the pin being LOW during initialization is much shorter. On my Nucleo F446RE, I tested booting about 50 times and the UART interface of the debugger never picked up this as a start bit, while it previously got a 0xff character on every boot.

For testing this PR just using make term should be sufficient. The garbage character before main() in the boot message should no longer appear with this PR.

fjmolinas · 2020-07-03T19:35:34Z

For testing this PR just using make term should be sufficient. The garbage character before main() in the boot message should no longer appear with this PR.

In that case just with make term I still get the garbage � in stm32f1, specifically nucleo-d103rb when using pyterm when just having make term in a terminal and flashing from another. I think make term is not reliable for this.

maribu · 2020-07-03T21:14:19Z

In that case just with make term I still get the garbage � in stm32f1

It seems I god lucky with my UART adapter:

Console output (multiple reboots triggered via reset button)

2020-07-03 22:38:15,881 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:38:15,882 # Hello World!
2020-07-03 22:38:15,887 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:38:15,890 # This board features a(n) stm32 MCU.
2020-07-03 22:38:16,285 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:38:16,286 # Hello World!
2020-07-03 22:38:16,291 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:38:16,294 # This board features a(n) stm32 MCU.
2020-07-03 22:38:25,596 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:38:25,597 # Hello World!
2020-07-03 22:38:25,602 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:38:25,605 # This board features a(n) stm32 MCU.
2020-07-03 22:38:26,195 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:38:26,196 # Hello World!
2020-07-03 22:38:26,201 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:38:26,204 # This board features a(n) stm32 MCU.
2020-07-03 22:38:26,721 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:38:26,722 # Hello World!
2020-07-03 22:38:26,727 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:38:26,730 # This board features a(n) stm32 MCU.
2020-07-03 22:38:27,149 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:38:27,151 # Hello World!
2020-07-03 22:38:27,155 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:38:27,159 # This board features a(n) stm32 MCU.
2020-07-03 22:39:41,864 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:39:41,865 # Hello World!
2020-07-03 22:39:41,870 # You are running RIOT on a(n) bluepill-128kib board.
2020-07-03 22:39:41,873 # This board features a(n) stm32 MCU.
2020-07-03 22:40:04,588 # main(): This is RIOT! (Version: 2020.07-devel-1726-gadffa6-stm32f4_uart_init)
2020-07-03 22:40:04,590 # Welcome to RIOT!

With the logic analyzer I can however see a start bit long enough that it should actually be detected by my UART adapter.

maribu · 2020-07-04T21:04:24Z

OK. For the STM32F1xx I found the culprits:

The UART needs to be initialized first, and only afterwards the UART needs to be connected to the pins.
The ODR register is both used for output in case of pins configured as output, and for pull up in case of input pins. STM32F1 gpio_init() will clear the ODR register for all modes but GPIO_IN_PU, it should however not touch the ODR register for non-input modes.

I think I don't need gpio_init_high() to fix the UART issue anymore, as the pin should remain high when the UART is initialized prior to routing it to the output pins. Still, other use cases require that PR.

I will update this PR when I have time to clean up the WIP code.

maribu · 2020-07-05T21:12:48Z

I updated the fix and force pushed, as this is basically a rewrite compared to the original approach. Now the result is correct even when using a logic analyzer. There should be no garbage being detected anymore.

I think this fix might need a bit more testing, as a broken GPIO implementation might result in very unhappy users.

Btw: I realized that gpio_init() is not thread safe. If one thread interrupts another during gpio_init() and performs itself gpio_init() on a pin of the same PORT, some of the configuration changes done by the higher prio thread might be reverted by the lower prio thread. (Lost update in a non-atomic read modify write sequence.)

I think the odds of this resulting in an issue are pretty low, as most GPIO initialization is done during auto_init() in sequential fashion. Two threads using bit-banging with half-duplex protocols are the only obvious example coming to my mind thread safety in gpio_init() would be needed. (E.g. two DHT sensors being read out concurrently.)

maribu · 2020-07-06T08:43:27Z

I'll rebase to fix a typo in a commit message. I wont change the commits or squash. (I'll also disable ready for build now: Now need to rebuild for a changed commit message.)

maribu · 2020-07-15T06:33:02Z

Out of the bugs still open for the release, this is likely the least important one. The UART is working reliable after the first broken character without this. So we might focus on the other open PRs first.

(IMO, we might even just leave this out of the release. UART is known to not be super reliable anyway, so one if one byte of garbage more breaks some application, that application is desperately in need of some error detection mechanism anyway.)

fjmolinas · 2020-07-15T09:45:39Z

This now realiably fixes the issue for me on stm32*:

PR

stm32f1

2020-07-15 11:38:17,008 # main(): This is RIOT! (Version: 2020.07-devel-1728-gb7f7-pr-14426)
2020-07-15 11:38:17,008 # Hello World!
2020-07-15 11:38:17,008 # You are running RIOT on a(n) iotlab-m3 board.
2020-07-15 11:38:17,023 # This board features a(n) stm32 MCU.
2020-07-15 11:38:21,184 # main(): This is RIOT! (Version: 2020.07-devel-1728-gb7f7-pr-14426)
2020-07-15 11:38:21,184 # Hello World!
2020-07-15 11:38:21,184 # You are running RIOT on a(n) iotlab-m3 board.
2020-07-15 11:38:21,185 # This board features a(n) stm32 MCU.

stm32*

2020-07-15 11:45:19,381 # main(): This is RIOT! (Version: 2020.07-devel-1728-gb7f7-pr-14426)
2020-07-15 11:45:19,382 # Hello World!
2020-07-15 11:45:19,387 # You are running RIOT on a(n) nucleo-l152re board.
2020-07-15 11:45:19,388 # This board features a(n) stm32 MCU.
2020-07-15 11:45:25,118 # main(): This is RIOT! (Version: 2020.07-devel-1728-gb7f7-pr-14426)
2020-07-15 11:45:25,119 # Hello World!
2020-07-15 11:45:25,121 # You are running RIOT on a(n) nucleo-l152re board.
2020-07-15 11:45:25,123 # This board features a(n) stm32 MCU.

master

stm32f1

2020-07-15 11:38:04,993 # �main(): This is RIOT! (Version: 2020.07-devel-1690-g772b6-HEAD)
2020-07-15 11:38:04,993 # Hello World!
2020-07-15 11:38:04,993 # You are running RIOT on a(n) iotlab-m3 board.
2020-07-15 11:38:04,994 # This board features a(n) stm32 MCU.
2020-07-15 11:38:08,208 # �main(): This is RIOT! (Version: 2020.07-devel-1690-g772b6-HEAD)
2020-07-15 11:38:08,209 # Hello World!
2020-07-15 11:38:08,224 # You are running RIOT on a(n) iotlab-m3 board.
2020-07-15 11:38:08,224 # This board features a(n) stm32 MCU.

stm32*

2020-07-15 11:44:42,723 # �main(): This is RIOT! (Version: 2020.07-devel-1690-g772b6-HEAD)
2020-07-15 11:44:42,723 # Hello World!
2020-07-15 11:44:42,728 # You are running RIOT on a(n) nucleo-l152re board.
2020-07-15 11:44:42,730 # This board features a(n) stm32 MCU.
2020-07-15 11:44:47,468 # �main(): This is RIOT! (Version: 2020.07-devel-1690-g772b6-HEAD)
2020-07-15 11:44:47,469 # Hello World!
2020-07-15 11:44:47,475 # You are running RIOT on a(n) nucleo-l152re board.
2020-07-15 11:44:47,477 # This board features a(n) stm32 MCU.

fjmolinas · 2020-07-15T09:46:06Z

If uart works then gpio_init also works IMO.

fjmolinas · 2020-07-15T09:46:44Z

Please squash @maribu and trigger ci!

- Do not set an intermediate mode, prepare correct mode settings in a temporary variable - Consistently enabled the GPIO periph in gpio_init_af() - Previously, STM32 F1 did not require a separate call to gpio_init() prior to a call of gpio_init_af(), but other STM32 families did - Now, gpio_init_af() can be used without gpio_init() consistently - STM32 F1: Do not touch ODR for non input pins - For input pins, this enables / disabled pull up resistors. For outputs, this register should remain untouched (according to API doc)

- Make use of the fact that gpio_init_af() does not need prior call to gpio_init() for all STM32 families anymore and drop call to gpio_init() - Initialize the UART periph first, before initializing the pins - While uninitialized, the UART periph will send signal LOW to TXD. This results in a start bit being picked up by the other side. - Instead, we do not connect the UART periph to the pins until it is initialized, so that the TXD level will already be HIGH when the pins are attached. - This results in no more garbage being send during initialization

aabadie

Given @fjmolinas tests and the fact that the CI is green. I think we can safely merge this PR.

ACK

aabadie · 2020-07-15T19:17:33Z

@miri64 since this PR is fixing a bug, I think it needs a backport, right ?

miri64 · 2020-07-15T19:31:28Z

Yepp.

cpu/stm32: Fix garbage on UART init [backport 2020.07] #14426

maribu requested review from aabadie, DipSwitch, fjmolinas, MrKevinWeiss and vincent-d as code owners July 2, 2020 13:08

maribu mentioned this pull request Jul 2, 2020

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

Closed

fjmolinas added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jul 2, 2020

fjmolinas added this to the Release 2020.07 milestone Jul 2, 2020

fjmolinas added the Area: drivers Area: Device drivers label Jul 2, 2020

benpicco reviewed Jul 2, 2020

View reviewed changes

maribu force-pushed the stm32f4_uart_init branch from adffa67 to f0d60ef Compare July 5, 2020 21:00

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 5, 2020

maribu force-pushed the stm32f4_uart_init branch from 065ec03 to b7f7fc0 Compare July 6, 2020 08:44

maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 6, 2020

maribu mentioned this pull request Jul 6, 2020

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

Closed

maribu added 2 commits July 15, 2020 12:12

maribu force-pushed the stm32f4_uart_init branch from b7f7fc0 to aec9eb7 Compare July 15, 2020 10:13

maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 15, 2020

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 15, 2020

aabadie approved these changes Jul 15, 2020

View reviewed changes

aabadie merged commit 7fd25f2 into RIOT-OS:master Jul 15, 2020

maribu deleted the stm32f4_uart_init branch July 15, 2020 19:31

miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jul 15, 2020

maribu mentioned this pull request Jul 15, 2020

cpu/stm32: Fix garbage on UART init [backport 2020.07] #14426 #14529

Merged

miri64 added a commit that referenced this pull request Jul 15, 2020

Merge pull request #14529 from maribu/backport-uart-stm32

92184c9

cpu/stm32: Fix garbage on UART init [backport 2020.07] #14426

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/stm32: Fix garbage on UART init #14426

cpu/stm32: Fix garbage on UART init #14426

maribu commented Jul 2, 2020 •

edited

Loading

benpicco Jul 2, 2020 •

edited

Loading

fjmolinas commented Jul 3, 2020

maribu commented Jul 3, 2020

fjmolinas commented Jul 3, 2020

maribu commented Jul 3, 2020

maribu commented Jul 4, 2020

maribu commented Jul 5, 2020

maribu commented Jul 6, 2020

maribu commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

aabadie left a comment

aabadie commented Jul 15, 2020

miri64 commented Jul 15, 2020

cpu/stm32: Fix garbage on UART init #14426

cpu/stm32: Fix garbage on UART init #14426

Conversation

maribu commented Jul 2, 2020 • edited Loading

Contribution description

cpu/stm32: Fix gpio_init() / gpio_int_af()

cpu/stm32: Fix uart_init()

Testing procedure

Issues/PRs references

benpicco Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

fjmolinas commented Jul 3, 2020

maribu commented Jul 3, 2020

fjmolinas commented Jul 3, 2020

maribu commented Jul 3, 2020

maribu commented Jul 4, 2020

maribu commented Jul 5, 2020

maribu commented Jul 6, 2020

maribu commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

fjmolinas commented Jul 15, 2020

aabadie left a comment

Choose a reason for hiding this comment

aabadie commented Jul 15, 2020

miri64 commented Jul 15, 2020

maribu commented Jul 2, 2020 •

edited

Loading

benpicco Jul 2, 2020 •

edited

Loading