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

Fixes HardwareSerial::availableForWrite + setTxBufferSize #6998

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

SuGlider
Copy link
Collaborator

Description of Change

Fixes an issue with HardwareSerial::availableForWrite() that only returns TX FIFO available space even when there is a TX RingBuffer in use through HardwareSerial::setTxBufferSize().
This isse was fixed by IDF 4.4 espressif/esp-idf@dfb75aa

new IDF UART API esp_err_t uart_get_tx_buffer_free_size(uart_port_t uart_num, size_t *size) has been incorporated to the code in uint32_t uartAvailableForWrite(uart_t* uart)

NOTE:

This will only pass CI whenever latest IDF 4.4 gets merged once it depends on uart.h and repective uart precompiled library to include new UART API esp_err_t uart_get_tx_buffer_free_size(uart_port_t uart_num, size_t *size)

DEPENDENCY on PR #6994

Tests scenarios

Tested with ESP32, ESP32-S2, ESP32-S3 and ESP32-C3 using a modified local version of IDF uart.c and uart.h

Related links

Fixes #6697

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.

Test code:

#include <Arduino.h>
#define BUFFER_SIZE 256
HardwareSerial HWS{1};

void setup() {
  Serial.begin(115200);
    while(!Serial){delay(10);}
  Serial.printf("Serial ready.\n");
#if CONFIG_IDF_TARGET_ESP32
  HWS.begin(9600, 12, 13); // Default pins for ESP32 conflict with SPI FLASH resulting in WDT
#else
  HWS.begin(9600);
#endif
  Serial.printf("Available for write before setting up buffer: %d\n", HWS.availableForWrite());
  HWS.end();

  HWS.setTxBufferSize(BUFFER_SIZE);
#if CONFIG_IDF_TARGET_ESP32
  HWS.begin(9600, 12, 13); // Default pins for ESP32 conflict with SPI FLASH resulting in WDT
#else
  HWS.begin(9600);
#endif
  Serial.printf("Available for write after setting up buffer of size %d: %d\n", BUFFER_SIZE, HWS.availableForWrite());
}

void loop() {
  delay(1000);
}

Output:

Available for write before setting up buffer: 128
Available for write after setting up buffer of size 256: 384

Tested on:

  • ESP32
  • ESP32S2
  • ESP32C3

@VojtechBartoska VojtechBartoska added the Status: Pending Merge Pull Request is ready to be merged label Jul 26, 2022
@SuGlider SuGlider changed the base branch from master to idf-release/v4.4 August 7, 2022 20:25
@SuGlider SuGlider changed the base branch from idf-release/v4.4 to master August 7, 2022 20:42
@SuGlider SuGlider merged commit 054e6b3 into espressif:master Aug 8, 2022
@VojtechBartoska VojtechBartoska removed the Status: Pending Merge Pull Request is ready to be merged label Aug 8, 2022
@SuGlider SuGlider mentioned this pull request Mar 30, 2023
1 task
@dagnall53
Copy link

@SuGlider Rodrigo, Sorry to open this again. but I have also run into this issue.. and was wondering if it was supposed to be corrected in 4.4.6-dirty .. or if it is waiting for the "full breaking" new V3 compiler.?
Is there any other (reliable!) method to use that can tell if the Serial is currently active sending out data?
I am very reluctant to check .availableForWrite() and to have to subtract 128 (the apparent bug) as a "hack" to get the free size of the tx buffer when it is set greater than the default (128).
Thanks.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

@SuGlider Rodrigo, Sorry to open this again. but I have also run into this issue.. and was wondering if it was supposed to be corrected in 4.4.6-dirty .. or if it is waiting for the "full breaking" new V3 compiler.?

Hi, the IDF version is OK. It may have been released a bit before the final 4.4.6 was out. But both (HardwareSerial::availableForWrite() and setTxBufferSize()) shall work correctly. Both depended on a new feature added to IDF 4.4.6.

Is there any other (reliable!) method to use that can tell if the Serial is currently active sending out data?

Yes, but not using Arduino API. It is possible to read the UART TX FIFO Count Register and see that it is empty, with data, etc.
This is exactly what HardwareSerial::flush() does. It waits for the FIFO to be fully empty and only then return its execution (it stays in busy wait state).

I am very reluctant to check .availableForWrite() and to have to subtract 128 (the apparent bug) as a "hack" to get the free size of the tx buffer when it is set greater than the default (128). Thanks.

128 is the size of the HW TX FIFO, therefore is it not a bug. The total available space for writing is the Buffer Size + 128 bytes.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

@dagnall53 - Please feel free to open a new issue report if you consider/find that there is a bug with it.
I'll personally take care of it.

@dagnall53
Copy link

@SuGlider Thanks for the fast reply.
I think we were expecting setTxBufferSize and availableForWrite() to be reporting on a single "TX buffer size", and not that one was "ADDING" to the hardware buffer in reports.

I think there is an argument that says that the setTxBufferSize is buggy or at the least, confusing.

Certainly setTxBufferSize(N) does nothing if N<128. Which seems reasonable. and makes one expect that (say)
setTxBufferSize(200) would set a total size of 200.. not 328 as it does.

It also means its impossible to set a (total size) buffer of (say) 200! as setTxBufferSize(72) will be rejected.

Its very complex and I have been getting lost in this.. but does that make sense?

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

I see your point. Yes, I'll think about it and place a PR to make it a single size.

Your arguments are good. Thanks!

@dagnall53
Copy link

@SuGlider
Wow.. thanks..

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

@dagnall53 - analysing the question, this is the possible solution for it:

Considering that IDF driver (mandatory for our Arduino implementation) only allows to set an additional Tx (or Rx) Buffer when its size is over 128 bytes, which is the HW TX FIFO size:

  • setTxBufferSize(tx_size) would behave in this way:
  1. if tx_size is lower than 128, it will do nothing and return 128, as the size set.
  2. if tx_size is higher than 128, it will set, internally, the IDF Driver buffer with size = tx_size - 128. It will return tx_size as the size set.
  • availableForWrite() will return the total available space, including the HW TX FIFO space of 128 bytes.

I think that it would address your points. Let me know if you agree.
I'll run some tests on it and then submit the PR.

@dagnall53
Copy link

@SuGlider
I think that would be a clearer approach.
Its certainly not vital unless others run into the confusion that I did!!

I hate to cross subjects.. but I came across this while trying to sort some very strange results with a Serial2.print.
I came to the conclusion that perhaps the Serial2 can suffer some sort of overflow or interrupt "issue". (once in that state , it stays there.
So I tried a drastic solution and changed all my Serial. to Serial2 and vice versa. and the problem appears to have been resolved. BUt having just written this I'm sure I will be punished and it will reoccur.. :-( But do you know if the Serial2 UART code is completely identical to the Serial code.. ??

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

But do you know if the Serial2 UART code is completely identical to the Serial code.. ??

Yes, both use exactly the same driver and code. What is the SoC (ESP32, ESP32-S2 or ESP32-S3)?
If you could, please, write a sketch that I can use to reproduce the issue and describe the problem you see, it would be better to debug it.

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

Just for tracking the Buffer Size problem better, I've open a new issue #9317

@SuGlider
Copy link
Collaborator Author

SuGlider commented Mar 1, 2024

@dagnall53 - PR submitted #9319
Now setTxBufferSize() return value and availableForWrite() will match (when empty).

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

Successfully merging this pull request may close these issues.

HardwareSerial::availableForWrite still return 128 after HardwareSerial::setTxBufferSize
5 participants