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

Changes UART ISR to only trigger on RX FIFO Full and timeout #6930

Merged
merged 12 commits into from
Sep 15, 2022

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Jun 30, 2022

Description of Change

This PR tries to make Arduino HardwareSerial be configurable the way the application needs it to work by fine tunning its RX Interrupt parameters.
It also forces UART RX ISR to only trigger on RX Timout and FIFO Full, ignoring errors such as BREAK, Parity or Overflow.

If the user wants each single arriving byte to be immediately inserted in the RxBuffer and made available right away, the sketch can use void setRxFIFOFull(uint8_t fifoBytes) setting it to 1 byte. It will consume more CPU time, but it wil trigger the transfer from RX FIFO to Arduino Buffer on a single byte level.

Receiving a single byte will depend on the Serial port baud rate. It is easier to see that using slower baud rates, such as 9600.

Another importante change is that by default onReceive() callback will be called when FIFO is full (by default on receiving 120 bytes or when Rx Timeout on about 2 symbols in the current baudrate).
In order to only get called when all data is received at the end of a transmission, it is necessary to explicity use true in onReceive(cbFunc, bool) and set an adequate buffer size in order to get all data at once, using setRxBufferSize() before begin().

Tests scenarios

It was tested using ESP32 with this sketch:

This sketch forces Arduino Serial to make data available every 5 bytes received

#include "Arduino.h"

// Example of 64 bytes string for using with Serial Monitor:
//0123456789abcdefghijklmnopqrstwuxyzABCDEFGHIJKLMNOPQRSTUWXYZ!@#$

void serialReport() {
  char serialFIFO[128];
  int bytesAvailable = Serial.available();
  Serial.printf("Got %d bytes:", bytesAvailable);
  int bytesToRead = bytesAvailable > sizeof(serialFIFO) - 1 ? sizeof(serialFIFO) - 1 : bytesAvailable;
  int bytesRead = 0;
  while (bytesAvailable > 0) {
    bytesRead = Serial.read(serialFIFO, bytesToRead);
    bytesAvailable -= bytesRead;
    serialFIFO[bytesRead] = '\0';
    Serial.println(serialFIFO);
  }
}

void setup() {
  Serial.begin(115200);
  while (!Serial) delay(50);
  // By default, Serial will read blocks of 120 bytes each time, but setRxFIFOFull(numBytes) can change it
  // comment out the next line and check different Serial behaviour
  Serial.setRxFIFOFull(5); // Serial will trigger callback and fill up Serial Read every 5 bytes received in the Serial port
  Serial.onReceive(serialReport, false);  // report will be called on timeout and FIFO threshold reached
  Serial.printf("\n\nSend a long string to Serial Port and see the number of bytes received onReceive() callback.\n\n");
}

void loop() {
}

This sketch demonstrates how to set a call back only whenever all data is received by Serial

#include "Arduino.h"

// Example of 64 bytes string for using with Serial Monitor:
//0123456789abcdefghijklmnopqrstwuxyzABCDEFGHIJKLMNOPQRSTUWXYZ!@#$

void serialReport() {
  char serialFIFO[128];
  int bytesAvailable = Serial.available();
  Serial.printf("Got %d bytes:", bytesAvailable);
  int bytesToRead = bytesAvailable > sizeof(serialFIFO) - 1 ? sizeof(serialFIFO) - 1 : bytesAvailable;
  int bytesRead = 0;
  while (bytesAvailable > 0) {
    bytesRead = Serial.read(serialFIFO, bytesToRead);
    bytesAvailable -= bytesRead;
    serialFIFO[bytesRead] = '\0';
    Serial.println(serialFIFO);
  }
}

void setup() {
  Serial.setRxBufferSize(2048);
  Serial.begin(115200);
  while (!Serial) delay(50);

  // serialReport will be called when all data is received in the internal ringeBuffer, set to 2048 bytes
  Serial.onReceive(serialReport, true);
  Serial.printf("\n\nSend a long string to Serial Port and see the number of bytes received onReceive() callback.\n\n");
}

void loop() {
}

Related links

@SuGlider SuGlider added Area: Peripherals API Relates to peripheral's APIs. Peripheral: UART labels Jun 30, 2022
@SuGlider SuGlider added this to the 2.1.0 milestone Jun 30, 2022
@SuGlider SuGlider requested a review from me-no-dev June 30, 2022 15:21
@SuGlider SuGlider self-assigned this Jun 30, 2022
@SuGlider SuGlider marked this pull request as draft June 30, 2022 15:21
@VojtechBartoska VojtechBartoska modified the milestones: 2.1.0, 2.0.5 Jul 13, 2022
@SuGlider SuGlider marked this pull request as ready for review September 6, 2022 15:09
@P-R-O-C-H-Y
Copy link
Member

@SuGlider Do you have any sketch to easily demonstrate the PR? :)

@SuGlider
Copy link
Collaborator Author

SuGlider commented Sep 8, 2022

@SuGlider Do you have any sketch to easily demonstrate the PR? :)

Added an example in PR text. Thanks.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Sep 8, 2022

@gonzabrusco - Please note the observation above, in the PR description.
This is a change that may affect your projects.
All necessary on your end is to use onReceive(callbackFunc, bool) with true or false.
By controlling the number of bytes, to consider RX FIFO Full, that will trigger the callback may improve your project as well.

@gonzabrusco
Copy link
Contributor

Thanks for the heads up @SuGlider

After reviewing the changes I see that not much has changed in the way everything works. The main changes are:

  • Now by default the callback is called on Timeout and FIFO full.
  • RxTimeout is 2 characters by default.
  • New method that changes the FIFO Full interrupt threshold (this is by default 120 bytes right?)

If I use onReceive(callbackFunc, true) I will be called only on Timeout and with a timeout of 2 characters. As it was on 2.0.4 right?

It seems this change allows a user to change the fifo full interrupt threshold and have a better reception control, particularly if the expected byte quantity is known (eg. you except always a fixed message of 30 bytes from another device). If not (like my use case with modbus) the timeout is still needed and the fifo full interrupt is not relevant because you can't predict the sizes of the messages on the RS485 bus. But nevertheless I like the change because it offers more control which is always nice to have 😀

I will try it and let you know if everything is working

@SuGlider
Copy link
Collaborator Author

SuGlider commented Sep 8, 2022

  • New method that changes the FIFO Full interrupt threshold (this is by default 120 bytes right?)

Yes, IDF default is 120 bytes.

If I use onReceive(callbackFunc, true) I will be called only on Timeout and with a timeout of 2 characters. As it was on 2.0.4 right?

Yes, exactly. You can still change the timeout using setRxTimeout(uint8_t symbols_timeout)

But nevertheless I like the change because it offers more control which is always nice to have 😀

That's the target. To improve user control over UART.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Sep 9, 2022

@gonzabrusco - some more new changes... talking to my colleague @P-R-O-C-H-Y, he tested the second sketch I posted, sending 2049 bytes, using the Serial Monitor and he asked me why the callback was never called and he had the impression that the Serial port was frozen as well, because it was not working anymore.

In this case, this happens because the Serial port gets its Buffer FULL and then later, FIFO Full as well. User callback is never called, but instead it calls the onReceiveError(func, erroNo) cback func.

It made me think that for most users, it is important that the regular User Call back with Data is also called in order to "consume" the incoming data, even when there is an error. The Error Callback gets called first, letting the Application to signal the error to the User Callback or even treat the data there.

This was done in my last commit. Please let me know your thought about it.

@gonzabrusco
Copy link
Contributor

LGTM @SuGlider @P-R-O-C-H-Y

In my current project I treat the data on the error callback so I won't be called again. But I think it's a nice addition for most users.

@@ -61,7 +61,8 @@ typedef enum {
UART_BUFFER_FULL_ERROR,
UART_FIFO_OVF_ERROR,
UART_FRAME_ERROR,
UART_PARITY_ERROR
UART_PARITY_ERROR,
UART_NO_ERROR
Copy link
Member

Choose a reason for hiding this comment

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

I think that having UART_NO_ERROR be the first item in the enum makes most sense. It will equal to 0, which conforms to most/all error uses in C/C++ (and ESP-IDF)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@me-no-dev
Copy link
Member

LGTM apart from one small comment :)

Copy link
Contributor

@PilnyTomas PilnyTomas left a comment

Choose a reason for hiding this comment

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

LGTM, tested on ESP32

Set NO_ERROR as first error code, same as ESP_OK = 0
@me-no-dev me-no-dev merged commit dca1a1e into espressif:master Sep 15, 2022
@SuGlider SuGlider deleted the uart_interrupt branch October 28, 2022 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Peripherals API Relates to peripheral's APIs. Peripheral: UART
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants