Skip to content

Fixes UART MODBUS and Loopback issue #6133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 17, 2022
Merged

Fixes UART MODBUS and Loopback issue #6133

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions cores/esp32/esp32-hal-uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,6 @@ uart_t* uartBegin(uint8_t uart_nr, uint32_t baudrate, uint32_t config, int8_t rx
ESP_ERROR_CHECK(uart_set_line_inverse(uart_nr, UART_SIGNAL_TXD_INV | UART_SIGNAL_RXD_INV));
}

// Set RS485 half duplex mode on UART. This shall force flush to wait up to sending all bits out
ESP_ERROR_CHECK(uart_set_mode(uart_nr, UART_MODE_RS485_HALF_DUPLEX));

UART_MUTEX_UNLOCK();

uartFlush(uart);
Expand Down Expand Up @@ -306,7 +303,7 @@ void uartFlushTxOnly(uart_t* uart, bool txOnly)
}

UART_MUTEX_LOCK();
ESP_ERROR_CHECK(uart_wait_tx_done(uart->num, portMAX_DELAY));
while(!uart_ll_is_tx_idle(UART_LL_GET_HW(uart->num)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could somebody explain why exactly was the uart_wait_tx_done() call replaced here with the uart_ll_is_tx_idle() loop?

The ModbusMaster library is now broken in ESP32S2 v2.0.3, and it was working correctly in v2.0.2 (checked, I switch between the two versions at will). Our project, like many others, uses RS485 and flips the data-enable and read-enable pins, and uses flush() to wait until TX done before switching to RX mode. I still have to do more tests, but this change in particular seems the most likely culprit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avillacis

Have you read the issues related to this PR? They are related to MODBUS and flush() issues.

In summary:
uart_wait_tx_done() returns as soon as data is stored in TXRingBuffer, before the TX FIFO is really empty.

uart_ll_is_tx_idle() returns true just when TX FIFO is completely empty.
The code for it is here - but it is the same for all other SoC, thus, the issue you see should happen for any ESP32-xx
https://github.com/espressif/esp-idf/blob/master/components/hal/esp32s2/include/hal/uart_ll.h#L714-L717

I tested flush() and I'm sure it works fine.
Maybe the issue you see isn't related to this PR.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Objection withdrawn. With access to the actual hardware, I found out that our RS485 card was echoing back request commands, and this echo was previously hidden by the uart_set_mode() call. Now I have manually added the call in our own code, and the issue was fixed.

However, is there any upstream bug report documenting this issue with uart_wait_tx_done()? Is it a true software bug, or a documentation error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently uart_wait_tx_done() works correctly when used with uart_set_mode(uart_num, UART_MODE_RS485_HALF_DUPLEX), considering an IDF environment.

But for Arduino, it isn't useful because of the its generic UART usage and APIs.
Thus we had to change it for the LL API.

Copy link
Collaborator Author

@SuGlider SuGlider May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, IDF starts UART with uart_set_mode(uart_num, UART_MODE_UART) and in this case, it returns as soon as data is stored in TXRingBuffer, before the TX FIFO is really empty.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could anyone recommend any solution to be using with Arduino as ModbusMaster is not working any more?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could anyone recommend any solution to be using with Arduino as ModbusMaster is not working any more?

The solution I adopted in my project is as follows:

#include "driver/uart.h"
/* ... */
Serial1.begin(9600, SERIAL_8N1, GPIO_RX, GPIO_TX);
while (Serial1) {  /* Wait for serial init */ }
uart_set_mode(UART_NUM_1, UART_MODE_RS485_HALF_DUPLEX);

Where UART_NUM_{N} must be chosen to match Serial{N} as used in the project. The #include "driver/uart.h" is to have access to the function uart_set_mode().

After this, initialize ModbusMaster as before with the Serial{N}.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avillacis Thank you. It works fine. I really appreciate your support


if ( !txOnly ) {
ESP_ERROR_CHECK(uart_flush_input(uart->num));
Expand Down