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

Adds HardwareSerial::setRxBufferSize() #5583

Merged
merged 3 commits into from
Aug 24, 2021
Merged

Adds HardwareSerial::setRxBufferSize() #5583

merged 3 commits into from
Aug 24, 2021

Conversation

SuGlider
Copy link
Collaborator

@SuGlider SuGlider commented Aug 23, 2021

Summary

It adds HardwareSerial::setRxBufferSize() with default value of 256 bytes.
Fixes #5580

Impact

It allows to define a Rx Buffer Size for UART, before begin().

@bertmelis
Copy link
Contributor

uint8_t rxfifo_full_thrhd = 112, size_t rxBufferSize = 256

Isn't this inconsistent?

@SuGlider SuGlider requested a review from me-no-dev August 23, 2021 18:35
@szerwi
Copy link

szerwi commented Aug 23, 2021

I think it will be better to keep similar parameter order that is in the uartBegin() function. So instead of:
unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert, unsigned long timeout_ms, uint8_t rxfifo_full_thrhd, size_t rxBufferSize
I propose:
unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, size_t rxBufferSize, bool invert, unsigned long timeout_ms, uint8_t rxfifo_full_thrhd

@SuGlider
Copy link
Collaborator Author

SuGlider commented Aug 23, 2021

uint8_t rxfifo_full_thrhd = 112, size_t rxBufferSize = 256

Isn't this inconsistent?

Why would it be?
rxfifo_full_thrhld is related to UART RX FIFO (fixed size =128 bytes)
rxBufferSize is related to RX Queue size fed by RX ISR.

@SuGlider
Copy link
Collaborator Author

I think it will be better to keep similar parameter order that is in the uartBegin() function. So instead of:
unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, bool invert, unsigned long timeout_ms, uint8_t rxfifo_full_thrhd, size_t rxBufferSize
I propose:
unsigned long baud, uint32_t config, int8_t rxPin, int8_t txPin, size_t rxBufferSize, bool invert, unsigned long timeout_ms, uint8_t rxfifo_full_thrhd

Adding to the end of parameter list will preserve retro compatibility.

@me-no-dev
Copy link
Member

I would suggest adding back setRxBufferSize(size_t) which would set a class variable that can then be used in begin, so code like the one below would work. Similar to setPins()

Serial.setRxBufferSize(1024);
Serial.begin(115200);

@SuGlider
Copy link
Collaborator Author

SuGlider commented Aug 23, 2021

I would suggest adding back setRxBufferSize(size_t) which would set a class variable that can then be used in begin, so code like the one below would work. Similar to setPins()

Serial.setRxBufferSize(1024);
Serial.begin(115200);

Added and commited.
PR name, summary and impact were updated.

@SuGlider SuGlider changed the title Adds rxBufferSize parameter to begin() Adds HardwareSerial::setRXBufferSize() Aug 23, 2021
@SuGlider SuGlider changed the title Adds HardwareSerial::setRXBufferSize() Adds HardwareSerial::setRxBufferSize() Aug 24, 2021
@me-no-dev me-no-dev merged commit 1f59c5a into espressif:master Aug 24, 2021
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.

2.0.0-rc2 - Serial.setRxBufferSize() is missing
4 participants