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::setTxBufferSize() #6383

Merged
merged 8 commits into from
Mar 28, 2022

Conversation

gonzabrusco
Copy link
Contributor

Summary

It adds HardwareSerial::setTxBufferSize() with default value of 0 bytes (like it is now by default).
No issue related. Just adds functionality to HardwareSerial. Allows non blocking writes when they exceed the TX FIFO.

Impact

None.

Related links

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

@CLAassistant
Copy link

CLAassistant commented Mar 7, 2022

CLA assistant check
All committers have signed the CLA.

@me-no-dev me-no-dev requested a review from SuGlider March 10, 2022 13:48
@me-no-dev me-no-dev added Area: Peripherals API Relates to peripheral's APIs. Status: Test needed Issue needs testing Status: To be implemented Selected for Development labels Mar 10, 2022
@SuGlider
Copy link
Collaborator

Thanks @gonzabrusco !
I'll review it soon.

SuGlider
SuGlider previously approved these changes Mar 19, 2022
@SuGlider
Copy link
Collaborator

@gonzabrusco - This is a nice improvement! Thanks!

Allows non blocking writes when they exceed the TX FIFO.

https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/peripherals/uart.html#_CPPv416uart_write_bytes11uart_port_tPKv6size_t

Based on your PR, this is exactly the intended behaviour.

@SuGlider SuGlider dismissed their stale review March 19, 2022 13:07

Found an issue when running the PR with an example.

@SuGlider SuGlider self-requested a review March 19, 2022 13:07
@SuGlider
Copy link
Collaborator

SuGlider commented Mar 19, 2022

@gonzabrusco

I just tested it with the example below and I got ASSERT error.
This is related to:
https:/espressif/esp-idf/blob/master/components/driver/uart.c#L1529-L1529

void setup() {
  Serial.setTxBufferSize(64);
  Serial.begin(115200);
}

void loop() {
}

Error message with the ESP32-S2:

Rebooting...
ESP-ROM:esp32s2-rc4-20191025
Build:Oct 25 2019
rst:0x3 (RTC_SW_SYS_RST),boot:0x8 (SPI_FAST_FLASH_BOOT)
Saved PC:0x400262e1
SPIWP:0xee
mode:DIO, clock div:1
load:0x3ffe6100,len:0x510
load:0x4004c000,len:0xa50
load:0x40050000,len:0x28bc
entry 0x4004c18c
E (91) uart: uart_driver_install(1529): uart tx buffer length error
ESP_ERROR_CHECK failed: esp_err_t 0xffffffff (ESP_FAIL) at 0x4002927c
file: "C:\Users\rocor\Documents\Arduino\hardware\espressif\esp32\cores\esp32\esp32-hal-uart.c" line 166
func: uartBegin
expression: uart_driver_install(uart_nr, 2*rx_buffer_size, 2*tx_buffer_size, 20, &(uart->uart_event_queue), 0)

abort() was called at PC 0x4002927f on core 0


Backtrace:0x400262b6:0x3ffc65b00x40029289:0x3ffc65d0 0x4002dcd5:0x3ffc65f0 0x4002927f:0x3ffc6670 0x40082042:0x3ffc6690 0x40081362:0x3ffc66f0 0x40080feb:0x3ffc6730 0x40081b72:0x3ffc6760 




Makes sure that the buffer size will not cause a reset to the board.
Keeps Rx/Tx buffer size as set, not doubling it. It makes the process more clear.
@SuGlider
Copy link
Collaborator

@gonzabrusco - already commited necessary changes to the code.

@gonzabrusco
Copy link
Contributor Author

gonzabrusco commented Mar 19, 2022

@gonzabrusco - already commited necessary changes to the code.

Thanks @SuGlider !! I was about to check the error and noticed you were working on it (because of the commit after the comment). Thanks for helping me push this pull request (my first by the way!).

I appreciate you removed the double buffer thing. I did not undestand that decision but I kept it like that because I thought that was an "Espressif thing". But now for me it makes perfect sense.

Copy link
Collaborator

@SuGlider SuGlider left a comment

Choose a reason for hiding this comment

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

Added 2 commits to comply with IDF requirements and avoid the issue commented in this PR.

@SuGlider
Copy link
Collaborator

@gonzabrusco - already commited necessary changes to the code.

Thanks @SuGlider !! I was about to check the error and noticed you were working on it (because of the commit after the comment). Thanks for helping me push this pull request (my first by the way!).

I appreciate you removed the double buffer thing. I did not undestand that decision but I kept it like that because I thought that was an "Espressif thing". But now for me it makes perfect sense.

Thanks! Congratulations for your first PR!
I hope it will be the first of many!

@SuGlider
Copy link
Collaborator

@me-no-dev - I have fininshed my review.
Ready for merging, after your final review.

@SuGlider SuGlider removed the Status: Test needed Issue needs testing label Mar 19, 2022
@SuGlider SuGlider added this to the 2.0.3 milestone Mar 20, 2022
@me-no-dev me-no-dev merged commit 77e9531 into espressif:master Mar 28, 2022
@gonzabrusco gonzabrusco deleted the uart-txbuffer branch May 4, 2022 12:43
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. Status: To be implemented Selected for Development
Projects
Development

Successfully merging this pull request may close these issues.

4 participants