arduino / arduino/ArduinoCore-renesas

Serial[x].print/write - Could use some major performance enhancement.

Open
#44 13 comments 0 reactions 0 assignees View on GitHub
topic: code type: enhancement
Dominant language
C
Stars
193
Forks
112
PR merge metrics
No merged PRs in 30d

Description

It appears like any calls to Serial.print(), including Serial.println and likewise Serial.write(buffer, cnt), will remain in these calls, until the last byte is queued to the underlying UART.

Here is a simple sketch:
```
void setup() {
Serial1.begin(115200);
pinMode(2, OUTPUT);
pinMode(3, OUTPUT);
pinMode(4, OUTPUT);
pinMode(5, OUTPUT);
}

uint32_t loop_count = 0;
void loop() {
digitalWrite(5, HIGH);
Serial1.print("Hello World...");
digitalWrite(5, LOW);
digitalWrite(5, HIGH);
Serial1.println(loop_count++, DEC);
digitalWrite(5, LOW);
delay(10);
digitalWrite(5, HIGH);
Serial1.write("Buffer\n", 7);
digitalWrite(5, LOW);
delay(1000);
}
```
Note, I instrumented the UART::write methods in Serial.cpp:
```
/* -------------------------------------------------------------------------- */
size_t UART::write(uint8_t c) {
/* -------------------------------------------------------------------------- */
if(init_ok) {
tx_done = false;
digitalWrite(2, HIGH);
R_SCI_UART_Write(&uart_ctrl, &c, 1);
digitalWrite(3, HIGH);
while (!tx_done) {}
digitalWrite(3, LOW);
digitalWrite(2, LOW);
return 1;
}
else {
return 0;
}
}

size_t UART::write(uint8_t* c, size_t len) {
if(init_ok) {
tx_done = false;
digitalWrite(2, HIGH);
R_SCI_UART_Write(&uart_ctrl, c, len);
digitalWrite(3, HIGH);
while (!tx_done) {}
digitalWrite(3, LOW);
digitalWrite(2, LOW);
return len;
}
else {
return 0;
}
}
```

Likewise the callback method:
```
/* -------------------------------------------------------------------------- */
void UART::WrapperCallback(uart_callback_args_t *p_args) {
/* -------------------------------------------------------------------------- */

uint32_t channel = p_args->channel;

UART *uart_ptr = UART::g_uarts[channel];

if(uart_ptr == nullptr) {
return;
}

digitalWrite(4, HIGH);

switch (p_args->event){
case UART_EVENT_ERR_PARITY:
case UART_EVENT_ERR_FRAMING:
case UART_EVENT_ERR_OVERFLOW:
case UART_EVENT_RX_COMPLETE: // This is called when all the "expected" data are received
{
break;
}
case UART_EVENT_TX_COMPLETE:
case UART_EVENT_TX_DATA_EMPTY:
{
//uint8_t to_enqueue = uart_ptr->txBuffer.available() < uart_ptr->uart_ctrl.fifo_depth ? uart_ptr->txBuffer.available() : uart_ptr->uart_ctrl.fifo_depth;
//while (to_enqueue) {
uart_ptr->tx_done = true;
break;
}
case UART_EVENT_RX_CHAR:
{
if (uart_ptr->rxBuffer.availableForStore()) {
uart_ptr->rxBuffer.store_char(p_args->data);
}
break;
}
case UART_EVENT_BREAK_DETECT:
{
break;
}
}

digitalWrite(4, LOW);
}
```
Here are some images of the Logic analyzer output.
![image](https://github.com/arduino/ArduinoCore-renesas/assets/1558080/d910bb93-dfdf-4133-8218-71ee9b45a04c)

![image](https://github.com/arduino/ArduinoCore-renesas/assets/1558080/34beefca-3217-4cfd-8b05-834305c779c0)

Observations:
1) it appears like in all cases, the write method: size_t UART::write(uint8_t c)
is being called, I would have expected the version: size_t UART::write(uint8_t* c, size_t len)
to be called for the print of a string as well as the simple write of a buffer.

2) I would have expected that there would be some form of software FIFO queue, like is on most of the other platforms, and only have to wait if you are writing more data than will fit. Would also expect that availableForWrite)_ would be implemented to give you a clue of how much I can write.

3) Hardware FIFO? I believe the first two UARTS have 16 byte queues? If so why does it look like only one bytes at a type is sent...
Maybe part of 1).

4) Why are the write methods waiting at the end at all? Especially when they are waiting for an ISR to be called to set the tx_done?
The only wait that should happen is if the hardware and/or software queues are full at the start of the call, should they wait for space to become available. And this waiting code should understand and deal with if the code calling write has a higher priority than the Serial UART ISR handler, but that is a different issue #38

Maybe more as I try to better understand all of the layers of this code.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the UART::write(uint8_t) and UART::write(uint8_t*, size_t) implementations in Serial.cpp, then trace WrapperCallback and the TX events; also review issue #38 for the priority interaction. Done means the requested transmit buffering, non-blocking behavior, and availableForWrite semantics are specified and verified against the observed sketch.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
embedded-iot, performance
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.