Skip to content

More HardwareSerial UART interrupts #9922

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

Closed
1 task done
maciekish opened this issue Jun 23, 2024 · 18 comments
Closed
1 task done

More HardwareSerial UART interrupts #9922

maciekish opened this issue Jun 23, 2024 · 18 comments
Assignees
Labels
Resolution: Wontfix Arduino ESP32 team will not fix the issue Type: Feature request Feature request for Arduino ESP32

Comments

@maciekish
Copy link

Related area

HardwareSerial UART Interrupts

Hardware specification

Any ESP32 with UARTs

Is your feature request related to a problem?

Hi, the current HardwareSerial has an onReceive() function (#5678) that uses interrupts to call a user function when data is received, but there is no equivalent for the UART_TX_DONE_INT or UART_TXFIFO_EMPTY_INT. I require these interrupts to implement an RS485 solution for ESP32.

Describe the solution you'd like

I would propose two new functions similar to the existing function

void HardwareSerial::onReceive(OnReceiveCb function, bool onlyOnTimeout)

The proposed functions are:

void HardwareSerial::onTransmitComplete(OnTransmitCompleteCb function)
void HardwareSerial::onTransmitBufferEmpty(OnTransmitBufferEmptyCb function, uint8_t threshold)

To be triggered on UART_TX_DONE_INT and UART_TXFIFO_EMPTY_INT respectively. The second function should also take a threshold parameter and update the uart_intr_config_t.txfifo_empty_intr_thresh value as requested by the user.

Describe alternatives you've considered

No response

Additional context

No response

I have checked existing list of Feature requests and the Contribution Guide

  • I confirm I have checked existing list of Feature requests and Contribution Guide.
@maciekish maciekish added the Type: Feature request Feature request for Arduino ESP32 label Jun 23, 2024
@SuGlider SuGlider self-assigned this Jun 23, 2024
@SuGlider
Copy link
Collaborator

Thanks @maciekish for the suggestion. I'll work on adding it.

@maciekish
Copy link
Author

Thanks @maciekish for the suggestion. I'll work on adding it.

I had a look around for the event queue code to hack it myself and found uart.h but no uart.c. Is that proprietary?

@me-no-dev
Copy link
Member

@bertmelis
Copy link
Contributor

Also interested in this feature.
But RS485 on its own is implemented in IDF, right? You can just put the UART in RS485 mode and set the RTS/CTS pins. Or am I missing something?

I could use the interrupts/events to manage bus timing though.

@TD-er
Copy link
Contributor

TD-er commented Jun 24, 2024

Also interested in this feature. But RS485 on its own is implemented in IDF, right? You can just put the UART in RS485 mode and set the RTS/CTS pins. Or am I missing something?

I could use the interrupts/events to manage bus timing though.

See for reference the part of source code in my own project related to this:

bool Port_ESPEasySerial_HardwareSerial_t::setRS485Mode(int8_t rtsPin, bool enableCollisionDetection)
{
  #ifdef ESP32
  if (_serial != nullptr) {
    if (rtsPin >= 0) {
      return _serial->setPins(-1, -1, -1, rtsPin) &&
            _serial->setHwFlowCtrlMode(UART_HW_FLOWCTRL_RTS) &&
            _serial->setMode(enableCollisionDetection ? UART_MODE_RS485_COLLISION_DETECT : UART_MODE_RS485_HALF_DUPLEX);
    }
    _serial->setMode(UART_MODE_UART);
  }
  #endif  
  return false;
}

By setting setHwFlowCtrlMode you don't need to calculate the duration of sending some bytes and keep track of them actually being flushed before toggling the DE/RE pin.

For collision detection, you need /RE connected to GND.

@SuGlider
Copy link
Collaborator

Arduino HardwareSerial API relies on IDF UART API.
We must use IDF in order to keep one code for all chips.

Thus, I have no good news...

There is no UART Event that matches this feature.
https://github.com/espressif/esp-idf/blob/release/v5.1/components/driver/uart/include/driver/uart.h#L52-L65

/**
 * @brief UART event types used in the ring buffer
 */
typedef enum {
    UART_DATA,              /*!< UART data event*/
    UART_BREAK,             /*!< UART break event*/
    UART_BUFFER_FULL,       /*!< UART RX buffer full event*/
    UART_FIFO_OVF,          /*!< UART FIFO overflow event*/
    UART_FRAME_ERR,         /*!< UART RX frame error event*/
    UART_PARITY_ERR,        /*!< UART RX parity event*/
    UART_DATA_BREAK,        /*!< UART TX data and break event*/
    UART_PATTERN_DET,       /*!< UART pattern detected */
#if SOC_UART_SUPPORT_WAKEUP_INT
    UART_WAKEUP,            /*!< UART wakeup event */
#endif
    UART_EVENT_MAX,         /*!< UART event max index*/
} uart_event_type_t;

The only IDF API that could help is:
esp_err_t uart_wait_tx_done(uart_port_t uart_num, TickType_t ticks_to_wait);

There is no clear way to capture the IRQ UART_TX_DONE_INT or UART_TXFIFO_EMPTY_INT using the IDF API.
Therefore no callback can be triggered.

@bertmelis
Copy link
Contributor

Hacky workaround for onTransmitComplete and if you have spare input pins: set UART to RS485 mode and put a gpio interrupt on the RTS/CTS pin.

@TD-er
Copy link
Contributor

TD-er commented Jun 25, 2024

The only IDF API that could help is:
esp_err_t uart_wait_tx_done(uart_port_t uart_num, TickType_t ticks_to_wait);

Maybe another RTOS task could be started which only runs this call and when it is done, it could fire an event or execute a callback?
Afterall you need to set some UART number when starting this RTOS task, so why not also setup some callback?

Only disadvantage is that it takes a semaphore. So potentially there could be some deadlock.

@SuGlider
Copy link
Collaborator

The only IDF API that could help is:
esp_err_t uart_wait_tx_done(uart_port_t uart_num, TickType_t ticks_to_wait);

Maybe another RTOS task could be started which only runs this call and when it is done, it could fire an event or execute a callback? Afterall you need to set some UART number when starting this RTOS task, so why not also setup some callback?

Only disadvantage is that it takes a semaphore. So potentially there could be some deadlock.

The issue here is that the Task should monitor if a UART write happens first...
Otherwise, the function uart_wait_tx_done() will always return OK when TX FIFO is empty.
uart_write() could enable the Semaphore in case the task is running to start monitorng the TX FIFO empty.

But it means using the CPU cycles in a sort of busy wait loop of a high priority Task... it can take ticks from other Tasks, such as setup()/loop()

That may reduce a lot the execution performance of the whole firmware.

@SuGlider
Copy link
Collaborator

The best way to achieve it is by using the ISR...
let me see if the IDF 5.3 which will be used in the next Arduino Core has a better solution for it.

@SuGlider
Copy link
Collaborator

Nothing new in IDF 5.3... the only way to get this feature correctly done by software would be by writing a new UART driver for Arduino.

Another way is the hack suggested by @bertmelis in #9922 (comment)

Hacky workaround for onTransmitComplete and if you have spare input pins: set UART to RS485 mode and put a gpio interrupt on the RTS/CTS pin.

@SuGlider
Copy link
Collaborator

Another point here... Interrupt TX FIFO Empty / TX Done is not good enough.

There are 2 possible situations: There is a TX Buffer or not.

  1. There is a TX RingBuffer, when HardwareSerial::setTxBufferSize() is used before HardwareSerial::begin().

HardwareSerial::write() will just add the all data to the RingBuffer and return.
In case that the Ring Buffer is not big enough, the IDF UART Driver will add as much data to the Buffer as possible, waiting for the UART to send the bytes until there is enough space in the RingBuffer in order to fill it and return.

In such case, because UART TX FIFO has just 127 bytes of space, many UART_TXFIFO_EMPTY_INT events can happen along a single call to Serial.print(), depending of the TX Buffer Size. After that other many UART_TXFIFO_EMPTY_INT will happen in background, processed by the IDF ISR, until the all the data in the RingBuffer is sent out.

  1. When there is no TX Buffer (default situation), the IDF also do the same, based on the char * size sent to HardwareSerial::write().

In this case, any call to HardwareSerial::write() will be blocked until almost all data is sent out through the UART TX pin. It will keep filling TX FIFO until all data from char *buf fits in it. Again, many UART_TX_FIFO_EMPTY_INT would be generated.

In other words, if the sketch writes, for instance, 500 bytes using Serial.print(), it will keep feeding TX FIFO with about 120 bytes per time, for 4 times (480 bytes), checking UART_TX_FIFO_EMPTY_INT to refill it, and at the end it will fill TX FIFO with the last 20 bytes and return.

TX FIFO will be controlled by the IDF Driver with multiple FIFO Empty interrupts until all the data is placed in the TX FIFO.
ISR will get many UART_TXFIFO_EMPTY_INT events which are not good for the application.


Therefore, I think that the best way to achieve what this Feature Request wants would be by creating a task that is a Writter Task, which sends the bytes and waits until all data is sent out using HardwareSerial::flush() and after that, it would call back some user function.
In this case, NO TX BUFFER shall be set... don't call HardwareSerial::setTxBufferSize(), otherwise, there is no way to know if all data has been sent out, bacause data would be in the Ring Buffer, for backgorund processing.

I hope that this can help you guys to understand how it works and to create the code that better fits your needs.

With that said, I can say that there is no way to achieve the requested Feature of this issue.

@SuGlider SuGlider added the Resolution: Wontfix Arduino ESP32 team will not fix the issue label Jun 25, 2024
@jax-gin
Copy link

jax-gin commented Jun 26, 2024

Hi, I have been using the library "esp32ModbusEsp32" in PlatformIO and I think I have the problem mentioned here.

The communication RS485 fail rondomly because a CRC error, and maybe it's because the program does not wait until all the data is sent, causing a loss of data.

What could i do for fix it or verify if is it the problem?

The funcion that control the data send is this:

void esp32ModbusRTU::_send(uint8_t* data, uint8_t length) {
  while (millis() - _lastMillis < _interval) delay(1);  // respect _interval
  // Toggle rtsPin, if necessary
  if (_rtsPin >= 0) digitalWrite(_rtsPin, HIGH);
  _serial->write(data, length);
  _serial->flush();
  // Toggle rtsPin, if necessary
  if (_rtsPin >= 0) digitalWrite(_rtsPin, LOW);
  _lastMillis = millis();
}

The communication is at 115200 baud, and the variable "_interval" is set to 1ms.

If this is not the correct way to ask about it here, sorry, it is my first time here.

@TD-er
Copy link
Contributor

TD-er commented Jun 26, 2024

@jax-gin Please see my previous comment to show how to use hardware RTS handling for RS485: #9922 (comment)

@jax-gin
Copy link

jax-gin commented Jun 26, 2024

@jax-gin Please see my previous comment to show how to use hardware RTS handling for RS485: #9922 (comment)

It worked, thank you very much.

I had to change the flag used in "setHwFlowCtrlMode" to disable it, so:

setHwFlowCtrlMode(UART_HW_FLOWCTRL_DISABLE)

Otherwise, it conflicts with "setMode". Now the communication is uninterrupted.

@SuGlider
Copy link
Collaborator

Guys, I'll close it, given that this feature won't be implemented.
If you have any better ideas, please reopen it or open a new feature request.

@maciekish
Copy link
Author

Another point here... Interrupt TX FIFO Empty / TX Done is not good enough.

There are 2 possible situations: There is a TX Buffer or not.

  1. There is a TX RingBuffer, when HardwareSerial::setTxBufferSize() is used before HardwareSerial::begin().

HardwareSerial::write() will just add the all data to the RingBuffer and return. In case that the Ring Buffer is not big enough, the IDF UART Driver will add as much data to the Buffer as possible, waiting for the UART to send the bytes until there is enough space in the RingBuffer in order to fill it and return.

In such case, because UART TX FIFO has just 127 bytes of space, many UART_TXFIFO_EMPTY_INT events can happen along a single call to Serial.print(), depending of the TX Buffer Size. After that other many UART_TXFIFO_EMPTY_INT will happen in background, processed by the IDF ISR, until the all the data in the RingBuffer is sent out.

  1. When there is no TX Buffer (default situation), the IDF also do the same, based on the char * size sent to HardwareSerial::write().

In this case, any call to HardwareSerial::write() will be blocked until almost all data is sent out through the UART TX pin. It will keep filling TX FIFO until all data from char *buf fits in it. Again, many UART_TX_FIFO_EMPTY_INT would be generated.

In other words, if the sketch writes, for instance, 500 bytes using Serial.print(), it will keep feeding TX FIFO with about 120 bytes per time, for 4 times (480 bytes), checking UART_TX_FIFO_EMPTY_INT to refill it, and at the end it will fill TX FIFO with the last 20 bytes and return.

TX FIFO will be controlled by the IDF Driver with multiple FIFO Empty interrupts until all the data is placed in the TX FIFO. ISR will get many UART_TXFIFO_EMPTY_INT events which are not good for the application.

Therefore, I think that the best way to achieve what this Feature Request wants would be by creating a task that is a Writter Task, which sends the bytes and waits until all data is sent out using HardwareSerial::flush() and after that, it would call back some user function. In this case, NO TX BUFFER shall be set... don't call HardwareSerial::setTxBufferSize(), otherwise, there is no way to know if all data has been sent out, bacause data would be in the Ring Buffer, for backgorund processing.

I hope that this can help you guys to understand how it works and to create the code that better fits your needs.

With that said, I can say that there is no way to achieve the requested Feature of this issue.

Sorry I'm not very well versed with the lower level stuff but I'm trying to understand why it's impossible if the functions to register for the interrupt are available... Wouldn't something like this work in HardwareSerial.cpp? Can't test it right now as im traveling and don't have any hardware with me. But at least it compiles 🤷🏻‍♂️

void HardwareSerial::onTransmitComplete(OnTransmitCompleteCb function) {
    HSERIAL_MUTEX_LOCK();

    uart_isr_handle_t *handle;
    uart_isr_register(_uart_nr, _onTransmitCompleteCB, _uart, ESP_INTR_FLAG_SHARED, handle);

    HSERIAL_MUTEX_UNLOCK();
}

@SuGlider
Copy link
Collaborator

The suggested code would replace the IDF ISR and the HardwareSerialAPI would stop working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Wontfix Arduino ESP32 team will not fix the issue Type: Feature request Feature request for Arduino ESP32
Projects
None yet
Development

No branches or pull requests

6 participants