Skip to content
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.

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

Forces UART Flush() to wait until all bits are sent #6026

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

SuGlider
Copy link
Collaborator

Summary

HardwareSerial flush() was returning before all data was sent out through serial port.
This is a problem to some RS485 libraries that depend on it to signaling.

This PR solves the issue by forcing it to block flush() until all data is sent.

Impact

None, just fixes an issue.

Related links

fix #5877

@SuGlider SuGlider requested a review from me-no-dev December 15, 2021 05:28
@SuGlider SuGlider self-assigned this Dec 15, 2021
@me-no-dev me-no-dev merged commit c2c8d18 into espressif:master Dec 15, 2021
@TD-er
Copy link
Contributor

TD-er commented Dec 15, 2021

Just curious, why must the uart mode be set to half duplex for all use cases?
This doesn't seem to make sense.
It looks like this will break use cases where a device may be sending data while the ESP is still sending (or starts sending while the other device was still sending data)
Another use case that will very likely be broken by this, is my own project, where I do accept commands to be entered via the terminal and will send logs by the ESP.

It would make more sense to add such a set mode function to the class, or add the mode as a parameter to te uartBegin function, so it can be used when needed.

Now we're stuck with a change in mode settings we cannot adjust when you need full duplex.

So can you please explain why this can only be fixed by this, instead of fixing it where it (IMHO) should be fixed: the flush function.

@VojtechBartoska
Copy link
Contributor

@SuGlider Just sending a reminder, PTAL on last comment.

@SuGlider SuGlider deleted the RS485-UART branch December 15, 2021 11:18
@SuGlider
Copy link
Collaborator Author

SuGlider commented Dec 18, 2021

@TD-er / @VojtechBartoska (FYI)

IDF 4.4 supports some features RS-485 natively.
Arduino SerialHardware is based on IDF but it doesn't implement RS485 and it doesn't use IDF RS485 features.
Checking the source code of IDF for uart, I found out that this Arduino Issue was related to a feature of IDF that returns from uart_wait_tx_done() whenever the data was pushed to the UART FIFO. The problem with it is that Arduino flush() should wait until the data is sent out completely and hold up to the TX FIFO is empty.

The way how IDF forces it is by setting the flag with uart_set_mode(uart_nr, UART_MODE_RS485_HALF_DUPLEX).
But it doesn't mean that Arduino UART will only work with half or even full duplex. It just makes the IDF driver work in the way Arduino needs it.

It is not stuck to any mode, because Arduino Libraries for MODBUS or RS-485 will actually control it in whatever it wants by software. This change doesn't force UART to act as full or half duplex.

As said, it just forces IDF UART driver to wait until all data is flushed out when using arduino flush() function.
The sketches will be able to write and read UART and the same time in a non-blocking way, allowing it to work in full-duplex mode.

@mrengineer7777
Copy link
Collaborator

This patch feels hacky and I suspect is what's causing issue #6126.

Digging into the IDF libraries:
uart.c::uart_set_mode() calls uart_hal_set_mode()
uart_hal.c::uart_hal_set_mode() calls uart_ll_set_mode()
uart_ll.h::uart_ll_set_mode() calls uart_ll_set_mode_rs485_half_duplex() for 485 half duplex.
uart_ll.h::uart_ll_set_mode_rs485_half_duplex() vs uart_ll_set_mode_normal():
For 485_half_duplex
hw->rs485_conf.rx_busy_tx_en = 1;
hw->rs485_conf.en = 1;
hw->conf0.sw_rts = 1;
For normal
hw->rs485_conf.rx_busy_tx_en = 0;
hw->rs485_conf.en = 1;
I was not able to find source files (compiled binary only?) where rx_busy_tx_en is used.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Jan 12, 2022

@mrengineer7777
Yes, it's probable. That's exactly what is described in the issue #6126

@SuGlider
Copy link
Collaborator Author

@mrengineer7777
The issue has been reviewed and fixed by #6133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MODBUS communication randomly broken due to misbehaving Serial.flush() function
5 participants