cpu/esp32: add the new API function uart_mode to periph/uart #11231

gschorcht · 2019-03-21T17:35:43Z

Contribution description

This PR adds API function uart_mode according to PR #10743 to periph/uart low level driver of ESP32 implementation. The PR depends on some in PR #11276.

Testing procedure

Compile and flash tests/periph_uart with enabled module periph_uart_modecfg

USEMODULE=periph_uart_modecfg make BOARD=esp32-wroom-32 -C tests/periph_uart flash

Connect GPIO9 [UART_DEV(1):RX] and GPIO10 [UART_DEV(1):TX].
Use the following commands and observe the UART communication with a logic analyzer

init 1 9600
mode 1 7 o 2
send 1 hello
mode 1 6 e 2
send 1 hello
...

Issues/PRs references

Depends on PR #11276
Implementation according to #10743

yegorich · 2019-03-21T21:02:30Z

So far we have agreed upon making uart_mode a feature till all platforms implement it. Could you wrap uart_mode into #ifdef MODULE_PERIPH_UART_MODECFG and add the feature to cpu/esp32/Makefile.features?

Other than that looks good to me.

gschorcht · 2019-03-22T07:10:57Z

@yegorich

Could you wrap uart_mode into #ifdef MODULE_PERIPH_UART_MODECFG and add the feature to cpu/esp32/Makefile.features?

I didn't embed the function in #ifdef MODULE_PERIPH_UART_MODECFG by intention since the driver itself uses the function for its initial configuration. So it can be considered as an internal function as long as it isn't declared as an official feature. Alternatively, I see following options:

Alternatively, I could define two functions, a static function _uart_mode used by the driver and a API wrapper function uart_mode. It would cost one additional function call.
Another possibility would be to add USEMODULE += periph_uart_modecfg by default for ESP MCUs.

Furthermore, since uart_mode is added to periph/uart.h without #ifdef MODULE_PERIPH_UART_MODECFG, it is already documented and someone could expect to be able to use it.

yegorich · 2019-03-22T08:36:30Z

I think FEATURES_PROVIDED += periph_uart_modecfg in cpu/esp32/Makefile.features should be sufficient. This way tests/periph_uart/main.c enables uart_mode support.

@haukepetersen, @MrKevinWeiss what do you think?

gschorcht · 2019-03-22T13:51:56Z

@yegorich I'm not sure whether cpu/esp32/Makefile.features is the right location for

FEATURES_PROVIDED += periph_uart_modecfg

since it belongs to peripheral interfaces like periph_uart itself for which the features are usually defined in boards/*/Makefile.features.

gschorcht · 2019-03-22T13:57:14Z

I think FEATURES_PROVIDED += periph_uart_modecfg in cpu/esp32/Makefile.features should be sufficient. This way tests/periph_uart/main.c enables uart_mode support.

That's true for test/periph_uart but not for applications that do not explicitly ask for this feature. But module periph_uart low level driver is always used since UART_DEV(0) is used as stdio.

gschorcht · 2019-03-22T14:04:55Z

I have realized it now in that way that there is an internal function _uart_set_mode which is used by the driver itself and a wrapper function uart_mode which is embedded in #ifdef MODULE_PERIPH_UART_MODECFG and only enabled when module periph_uart_modecfg is used.

yegorich

Green light from me.

MrKevinWeiss

Would you be willing to split the PRs into a cleanup __uart -> _uart and the mode functionality?

I can do some testing at the end of the week.

MrKevinWeiss · 2019-03-25T12:35:54Z

cpu/esp32/periph/uart.c

+    /* wait until TX FIFI is empty */
+    while (_uarts[uart].regs->status.txfifo_cnt != 0) { }
+
+    critical_enter();


Can you put some information on why you added the critical*, does it apply to the *_mode or is it something separate

It has to be ensured that the reconfiguration cannot be interrupted. Even though it is something I realized when I was testing _uart_set_mode, I think it has to be ensured for reconfiguring the baudrate too. That's why I added it with this PR.

gschorcht · 2019-03-25T16:29:05Z

@MrKevinWeiss Thank you for reviewing the PR.

Would you be willing to split the PRs into a cleanup __uart -> _uart and the mode functionality?

Hm, ... It is only a very small naming fix to get the code compatible with the reimplementation of ESP8266 in PR #11108 which will allow code deduplication with future PRs.

Sure, I could provide a separate PR, but I didn't want to open too many different development branches which later cause a lot of merging conflicts or too many dependencies of different PRs. But, if you think that it might be better to have different PRs, I would provide a separate PR for renaming and would make the this PR dependent on the separate PR. BTW, PR #10644 requires changed names too.

gschorcht · 2019-03-26T09:29:55Z

@MrKevinWeiss
Finally, I decided to split the PR into cleanup #11276 and uart_mode (this PR).

At the moment, this PR includes all commits of PR #11276 since I couldn't separate it completely due to unresolvable merge conflicts. Once, PR #11276 is merged, I will rebase this PR to remove them.

MrKevinWeiss · 2019-03-26T10:14:05Z

No problem, the cleanup PR should get in soon.

MrKevinWeiss · 2019-03-26T17:52:32Z

I guess ready for rebase now 😃

gschorcht · 2019-03-26T17:53:22Z

@MrKevinWeiss Rebased to current master 😄

MrKevinWeiss

I think it is pretty good. I tested with loopback but I would like to confirm with the scope (so not until Thursday or Friday). Outside of that I am happy.

MrKevinWeiss

Tested against scope. Only the stop bits seem to be an issue, everything else appeared correct.

MrKevinWeiss · 2019-03-28T12:24:39Z

cpu/esp32/periph/uart.c

+        case UART_STOP_BITS_1: _uarts[uart].regs->conf0.stop_bit_num = 1;
+                               _uarts[uart].regs->rs485_conf.dl1_en = 1;
+                               break;
+        case UART_STOP_BITS_2: _uarts[uart].regs->conf0.stop_bit_num = 1;


Should this be 2? Either that or say not implemented.

No, it is a known hardware bug. _uarts[uart].regs->conf0.stop_bit_num has to be set to 1 if there is any stop bit. With _uarts[uart].regs->rs485_conf.dl1_en it is decided whether there are 1 or 2 stop bits.

MrKevinWeiss · 2019-03-28T12:25:59Z

cpu/esp32/periph/uart.c

+                 critical_exit();
+                 return UART_NOMODE;
+    }
+    #else


Is the ifdef needed here since they are all MCU_ESP32 that use this file?

Yes, at the moment. But, the UART source code is already compatible with that of the complete ESP8266 reimplemenation in PR #11108 (https://github.com/RIOT-OS/RIOT/blob/2f008018200c180a96f6457e7d0ec655b2209510/cpu/esp8266/periph/uart.c). To make planned deduplication easier, I'm already adapting the ESP32 code if I make changes.

gschorcht · 2019-03-28T13:22:57Z

@MrKevinWeiss

Tested against scope.

Thanks.

Only the stop bits seem to be an issue, everything else appeared correct.

What exactly is the issue? It worked for me.

MrKevinWeiss · 2019-03-28T13:29:27Z

What exactly is the issue? It worked for me.

No difference between 1 stop bit and 2 stop bits, it appears that the second stop bit is not being sent (always just one stop bit). Also in the code it appears you are assigning the same thing if you set stop bits to 1 or 2.

case UART_STOP_BITS_1:
    _uarts[uart].regs->conf0.stop_bit_num = 1;
    _uarts[uart].regs->rs485_conf.dl1_en = 1;
    break;
case UART_STOP_BITS_2:
    _uarts[uart].regs->conf0.stop_bit_num = 1;
    _uarts[uart].regs->rs485_conf.dl1_en = 1;
    break;

yegorich · 2019-03-28T13:35:37Z

At least this is how ESP-IDF sets stop bits: https://github.com/espressif/esp-idf/blob/106dc05903a1691c024bb61ac7b29ca728829671/components/driver/uart.c#L139

esp_err_t uart_set_stop_bits(uart_port_t uart_num, uart_stop_bits_t stop_bit)
{
    UART_CHECK((uart_num < UART_NUM_MAX), "uart_num error", ESP_FAIL);
    UART_CHECK((stop_bit < UART_STOP_BITS_MAX), "stop bit error", ESP_FAIL);

    UART_ENTER_CRITICAL(&uart_spinlock[uart_num]);
    //workaround for hardware bug, when uart stop bit set as 2-bit mode.
    if (stop_bit == UART_STOP_BITS_2) {
        stop_bit = UART_STOP_BITS_1;
        UART[uart_num]->rs485_conf.dl1_en = 1;
    } else {
        UART[uart_num]->rs485_conf.dl1_en = 0;
    }
    UART[uart_num]->conf0.stop_bit_num = stop_bit;
    UART_EXIT_CRITICAL(&uart_spinlock[uart_num]);
    return ESP_OK;
}

gschorcht · 2019-03-28T13:41:04Z

What exactly is the issue? It worked for me.

No difference between 1 stop bit and 2 stop bits, it appears that the second stop bit is not being sent (always just one stop bit). Also in the code it appears you are assigning the same thing if you set stop bits to 1 or 2.
case UART_STOP_BITS_1:
    _uarts[uart].regs->conf0.stop_bit_num = 1;
    _uarts[uart].regs->rs485_conf.dl1_en = 1;
    break;
case UART_STOP_BITS_2:
    _uarts[uart].regs->conf0.stop_bit_num = 1;
    _uarts[uart].regs->rs485_conf.dl1_en = 1;
    break;

Ups, oh yeah. It should be

case UART_STOP_BITS_1:
    _uarts[uart].regs->conf0.stop_bit_num = 1;
    _uarts[uart].regs->rs485_conf.dl1_en = 0;
    break;

This happened propably when I changed it back from trying to remove the hack due to the hardware bug. I thought that it is not necessary to test it again since it worked before 😎

MrKevinWeiss · 2019-03-28T13:43:40Z

Once it is in the HiL retesting shouldn't be a problem

gschorcht · 2019-03-28T13:46:02Z

@MrKevinWeiss Fixed it, but I have no hardware here for testing it again.

MrKevinWeiss · 2019-03-28T14:16:41Z

Leave it to me!

MrKevinWeiss

Retested and can see stop bit 2.

Please squash.

ACK!

Add data members to the UART device configuration data structure that are needed by uart_mode API function.

Set default values for additional data members of UART device configuration data structure that are needed by uart_mode API function.

Configuration of UART mode is realized by an internal function which is also used by UART initialization function.

The internal _uart_set_mode function is exposed if module periph_uart_modecfg is enabled.

gschorcht · 2019-03-29T06:42:37Z

@MrKevinWeiss Thanks for reviewing and testing.

gschorcht added Type: new feature The issue requests / The PR implemements a new feature for RIOT Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Mar 21, 2019

yegorich approved these changes Mar 24, 2019

View reviewed changes

MrKevinWeiss reviewed Mar 25, 2019

View reviewed changes

gschorcht force-pushed the cpu/esp32/periph/uart_mode branch 2 times, most recently from a1e9e52 to fc72ca3 Compare March 26, 2019 09:23

gschorcht mentioned this pull request Mar 26, 2019

cpu/esp32: periph uart cleanups #11276

Merged

gschorcht force-pushed the cpu/esp32/periph/uart_mode branch from fc72ca3 to 43c144f Compare March 26, 2019 17:52

MrKevinWeiss reviewed Mar 26, 2019

View reviewed changes

MrKevinWeiss requested changes Mar 28, 2019

View reviewed changes

MrKevinWeiss approved these changes Mar 28, 2019

View reviewed changes

MrKevinWeiss added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 28, 2019

gschorcht added 4 commits March 28, 2019 16:36

cpu/esp32: add config members for uart_mode

6132c08

Add data members to the UART device configuration data structure that are needed by uart_mode API function.

cpu/esp32: add config member values for uart_mode

b9a8b98

Set default values for additional data members of UART device configuration data structure that are needed by uart_mode API function.

cpu/esp32: add internal _uart_set_mode function

ec013f7

Configuration of UART mode is realized by an internal function which is also used by UART initialization function.

cpu/esp32: add new uart_mode API function

217ccbe

The internal _uart_set_mode function is exposed if module periph_uart_modecfg is enabled.

gschorcht force-pushed the cpu/esp32/periph/uart_mode branch from cc6bd45 to 217ccbe Compare March 28, 2019 15:36

MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 28, 2019

MrKevinWeiss merged commit 2d7c72d into RIOT-OS:master Mar 28, 2019

gschorcht deleted the cpu/esp32/periph/uart_mode branch March 29, 2019 06:42

gschorcht mentioned this pull request Mar 29, 2019

boards/esp32: changes the approach for configurations of UART interfaces in board definitions #11294

Merged

danpetry added this to the Release 2019.04 milestone Apr 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpu/esp32: add the new API function uart_mode to periph/uart #11231

cpu/esp32: add the new API function uart_mode to periph/uart #11231

gschorcht commented Mar 21, 2019 •

edited

Loading

yegorich commented Mar 21, 2019

gschorcht commented Mar 22, 2019

yegorich commented Mar 22, 2019 •

edited

Loading

gschorcht commented Mar 22, 2019 •

edited

Loading

gschorcht commented Mar 22, 2019

gschorcht commented Mar 22, 2019

yegorich left a comment

MrKevinWeiss left a comment

MrKevinWeiss Mar 25, 2019

gschorcht Mar 25, 2019

gschorcht commented Mar 25, 2019

gschorcht commented Mar 26, 2019

MrKevinWeiss commented Mar 26, 2019

MrKevinWeiss commented Mar 26, 2019

gschorcht commented Mar 26, 2019

MrKevinWeiss left a comment

MrKevinWeiss left a comment

MrKevinWeiss Mar 28, 2019

gschorcht Mar 28, 2019

MrKevinWeiss Mar 28, 2019

gschorcht Mar 28, 2019

gschorcht commented Mar 28, 2019

MrKevinWeiss commented Mar 28, 2019

yegorich commented Mar 28, 2019

gschorcht commented Mar 28, 2019

MrKevinWeiss commented Mar 28, 2019

gschorcht commented Mar 28, 2019

MrKevinWeiss commented Mar 28, 2019

MrKevinWeiss left a comment

gschorcht commented Mar 29, 2019

cpu/esp32: add the new API function uart_mode to periph/uart #11231

cpu/esp32: add the new API function uart_mode to periph/uart #11231

Conversation

gschorcht commented Mar 21, 2019 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

yegorich commented Mar 21, 2019

gschorcht commented Mar 22, 2019

yegorich commented Mar 22, 2019 • edited Loading

gschorcht commented Mar 22, 2019 • edited Loading

gschorcht commented Mar 22, 2019

gschorcht commented Mar 22, 2019

yegorich left a comment

Choose a reason for hiding this comment

MrKevinWeiss left a comment

Choose a reason for hiding this comment

MrKevinWeiss Mar 25, 2019

Choose a reason for hiding this comment

gschorcht Mar 25, 2019

Choose a reason for hiding this comment

gschorcht commented Mar 25, 2019

gschorcht commented Mar 26, 2019

MrKevinWeiss commented Mar 26, 2019

MrKevinWeiss commented Mar 26, 2019

gschorcht commented Mar 26, 2019

MrKevinWeiss left a comment

Choose a reason for hiding this comment

MrKevinWeiss left a comment

Choose a reason for hiding this comment

MrKevinWeiss Mar 28, 2019

Choose a reason for hiding this comment

gschorcht Mar 28, 2019

Choose a reason for hiding this comment

MrKevinWeiss Mar 28, 2019

Choose a reason for hiding this comment

gschorcht Mar 28, 2019

Choose a reason for hiding this comment

gschorcht commented Mar 28, 2019

MrKevinWeiss commented Mar 28, 2019

yegorich commented Mar 28, 2019

gschorcht commented Mar 28, 2019

MrKevinWeiss commented Mar 28, 2019

gschorcht commented Mar 28, 2019

MrKevinWeiss commented Mar 28, 2019

MrKevinWeiss left a comment

Choose a reason for hiding this comment

gschorcht commented Mar 29, 2019

gschorcht commented Mar 21, 2019 •

edited

Loading

yegorich commented Mar 22, 2019 •

edited

Loading

gschorcht commented Mar 22, 2019 •

edited

Loading