Skip to content

cpu/esp_common/periph_uart: fix call to _uart_set_mode#18720

Merged
gschorcht merged 1 commit intoRIOT-OS:masterfrom
maribu:cpu/esp32/uart
Oct 12, 2022
Merged

cpu/esp_common/periph_uart: fix call to _uart_set_mode#18720
gschorcht merged 1 commit intoRIOT-OS:masterfrom
maribu:cpu/esp32/uart

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Oct 10, 2022

Contribution description

The parameters for parity and stop bits was confused, resulting in the following compilation error with GCC 12.2.0:

/home/maribu/Repos/software/RIOT/cpu/esp_common/periph/uart.c: In function '_uart_config':
/home/maribu/Repos/software/RIOT/cpu/esp_common/periph/uart.c:394:61: error: implicit conversion from 'uart_stop_bits_t' to 'uart_parity_t' -Werror=enum-conversion]
  394 |     if (_uart_set_mode(uart, _uarts[uart].data, _uarts[uart].stop,
      |                                                 ~~~~~~~~~~~~^~~~~
/home/maribu/Repos/software/RIOT/cpu/esp_common/periph/uart.c:395:42: error: implicit conversion from 'uart_parity_t' to 'uart_stop_bits_t' -Werror=enum-conversion]
  395 |                              _uarts[uart].parity) != UART_OK) {
      |                              ~~~~~~~~~~~~^~~~~~~
cc1: all warnings being treated as errors

This swaps the parameters.

Testing procedure

The output of e.g. examples/hello_world on ESP boards connected via UART should still be readable after this change.

I am not sure why UART works correctly. Maybe there is a second parameter order confusion somewhere that negates the bug?

BEWARE: This is untested

Issues/PRs references

None

@maribu
cpu/esp_common/periph_uart: fix call to _uart_set_mode
6d874c2 
The parameters for parity and stop bits was confused, resulting in
the following compilation error with GCC 12.2.0:

    /home/maribu/Repos/software/RIOT/cpu/esp_common/periph/uart.c: In function '_uart_config':
    /home/maribu/Repos/software/RIOT/cpu/esp_common/periph/uart.c:394:61: error: implicit conversion from 'uart_stop_bits_t' to 'uart_parity_t' -Werror=enum-conversion]
      394 |     if (_uart_set_mode(uart, _uarts[uart].data, _uarts[uart].stop,
          |                                                 ~~~~~~~~~~~~^~~~~
    /home/maribu/Repos/software/RIOT/cpu/esp_common/periph/uart.c:395:42: error: implicit conversion from 'uart_parity_t' to 'uart_stop_bits_t' -Werror=enum-conversion]
      395 |                              _uarts[uart].parity) != UART_OK) {
          |                              ~~~~~~~~~~~~^~~~~~~
    cc1: all warnings being treated as errors

This swaps the parameters.
@maribu maribu requested a review from gschorcht as a code owner October 10, 2022 12:30
@github-actions github-actions bot added Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms labels Oct 10, 2022
@benpicco benpicco added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 10, 2022
@riot-ci
Copy link
Copy Markdown

riot-ci commented Oct 10, 2022

Murdock results

✔️ PASSED

6d874c2 cpu/esp_common/periph_uart: fix call to _uart_set_mode

Success Failures Total Runtime
1980 0 1980 06m:39s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 10, 2022

OK, stdio still works. The line that is changed is only triggered in uart_poweron(), which nobody uses anyway. That is why nobody hit the bug so far.

The test application in tests/periph_uart shows an improvement: In master executing the RIOT shell command test 1 results in a warning from the UART driver (invalid number of stop bits) followed by a crash. With this PR, the warning is gone (the crash however isn't).

It may be worth looking into what caused the crash there.

@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Oct 10, 2022

👍 good catch.

@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Oct 10, 2022 •
edited
Loading

The test application in tests/periph_uart shows an improvement: In master executing the RIOT shell command test 1 results in a warning from the UART driver (invalid number of stop bits) followed by a crash. With this PR, the warning is gone (the crash however isn't).

I can't reproduce this problem, I just get

> test 1
[START]
mismatch at index 0: ffffffff != 48
[FAILURE]

@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Oct 10, 2022 •
edited
Loading

It may be worth looking into what caused the crash there.

What board and what configuration did you use for UART_DEV(1)?

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 10, 2022

Note that you need to "short" the RX to the TX pin or UART_DEV(1). I used the MH ET Live Minikit (or rather an even cheaper clone of it).

@gschorcht
Copy link
Copy Markdown
Contributor

gschorcht commented Oct 11, 2022

Note that you need to "short" the RX to the TX pin or UART_DEV(1).

I did it, but I tried it with following configuration:

Board configuration:
	UART_DEV(0)	txd=1 rxd=3
	UART_DEV(1)	txd=12 rxd=13
	LED		pins=[ ]
	BUTTONS		pins=[ 0 ]

I still get

> test 1
[START]
mismatch at index 0: ffffffff != 48
[FAILURE]

Using GPIO9 and GPIO10 for UART_DEV(1) causes crashes. This could be because GPIO9 and GPIO10 are connected to the SPI flash. But actually they are only used in QIO or QOUT mode. The corresponding pins of the SPI Flash should be floating when using DIO and DOUT mode. However, it looks like the connection of GPIO9 and GPIO10 affects the SPI Flash access for some reason.

@maribu maribu mentioned this pull request Oct 11, 2022
Closed
1 task
@gschorcht gschorcht merged commit 96fad3f into RIOT-OS:master Oct 12, 2022
@maribu maribu deleted the cpu/esp32/uart branch October 12, 2022 14:41
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Oct 12, 2022

Thx :)

@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gschorcht gschorcht gschorcht approved these changes

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

4 participants

@maribu @riot-ci @gschorcht @benpicco