Skip to content

Commit

Permalink
Inefficient Print::write(data,len) shows message if used (only in deb…
Browse files Browse the repository at this point in the history 
…ug mode) (#4537)

* inefficient Print::write(data,len) shows message if used (only in debug mode)
* make HardwareSerial's write(data,len) efficient
* HardwareSerial: remove duplicate tests, move trivial code from .cpp to .h
  • Loading branch information
@d-a-v
d-a-v authored Mar 22, 2018
1 parent 2013af1 commit f8f205d
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 107 deletions.
85 changes: 2 additions & 83 deletions cores/esp8266/HardwareSerial.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,8 @@ void HardwareSerial::end()
uart_set_debug(UART_NO);
}

if (_uart) {
uart_uninit(_uart);
_uart = NULL;
}
uart_uninit(_uart);
_uart = NULL;
}

size_t HardwareSerial::setRxBufferSize(size_t size){
Expand All @@ -70,30 +68,6 @@ size_t HardwareSerial::setRxBufferSize(size_t size){
return _rx_size;
}

void HardwareSerial::swap(uint8_t tx_pin)
{
if(!_uart) {
return;
}
uart_swap(_uart, tx_pin);
}

void HardwareSerial::set_tx(uint8_t tx_pin)
{
if(!_uart) {
return;
}
uart_set_tx(_uart, tx_pin);
}

void HardwareSerial::pins(uint8_t tx, uint8_t rx)
{
if(!_uart) {
return;
}
uart_set_pins(_uart, tx, rx);
}

void HardwareSerial::setDebugOutput(bool en)
{
if(!_uart) {
Expand All @@ -113,16 +87,6 @@ void HardwareSerial::setDebugOutput(bool en)
}
}

bool HardwareSerial::isTxEnabled(void)
{
return _uart && uart_tx_enabled(_uart);
}

bool HardwareSerial::isRxEnabled(void)
{
return _uart && uart_rx_enabled(_uart);
}

int HardwareSerial::available(void)
{
int result = static_cast<int>(uart_rx_available(_uart));
Expand All @@ -132,27 +96,6 @@ int HardwareSerial::available(void)
return result;
}

int HardwareSerial::peek(void)
{
// this may return -1, but that's okay
return uart_peek_char(_uart);
}

int HardwareSerial::read(void)
{
// this may return -1, but that's okay
return uart_read_char(_uart);
}

int HardwareSerial::availableForWrite(void)
{
if(!_uart || !uart_tx_enabled(_uart)) {
return 0;
}

return static_cast<int>(uart_tx_free(_uart));
}

void HardwareSerial::flush()
{
if(!_uart || !uart_tx_enabled(_uart)) {
Expand All @@ -165,33 +108,9 @@ void HardwareSerial::flush()
delayMicroseconds(11000000 / uart_get_baudrate(_uart) + 1);
}

size_t HardwareSerial::write(uint8_t c)
{
if(!_uart || !uart_tx_enabled(_uart)) {
return 0;
}

uart_write_char(_uart, c);
return 1;
}

int HardwareSerial::baudRate(void)
{
// Null pointer on _uart is checked by SDK
return uart_get_baudrate(_uart);
}


HardwareSerial::operator bool() const
{
return _uart != 0;
}


#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL)
HardwareSerial Serial(UART0);
#endif
#if !defined(NO_GLOBAL_INSTANCES) && !defined(NO_GLOBAL_SERIAL1)
HardwareSerial Serial1(UART1);
#endif

64 changes: 51 additions & 13 deletions cores/esp8266/HardwareSerial.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,26 +93,50 @@ class HardwareSerial: public Stream
{
swap(1);
}
void swap(uint8_t tx_pin); //toggle between use of GPIO13/GPIO15 or GPIO3/GPIO(1/2) as RX and TX
void swap(uint8_t tx_pin) //toggle between use of GPIO13/GPIO15 or GPIO3/GPIO(1/2) as RX and TX
{
uart_swap(_uart, tx_pin);
}

/*
* Toggle between use of GPIO1 and GPIO2 as TX on UART 0.
* Note: UART 1 can't be used if GPIO2 is used with UART 0!
*/
void set_tx(uint8_t tx_pin);
void set_tx(uint8_t tx_pin)
{
uart_set_tx(_uart, tx_pin);
}

/*
* UART 0 possible options are (1, 3), (2, 3) or (15, 13)
* UART 1 allows only TX on 2 if UART 0 is not (2, 3)
*/
void pins(uint8_t tx, uint8_t rx);
void pins(uint8_t tx, uint8_t rx)
{
uart_set_pins(_uart, tx, rx);
}

int available(void) override;
int peek(void) override;
int read(void) override;
int availableForWrite(void);

int peek(void) override
{
// this may return -1, but that's okay
return uart_peek_char(_uart);
}
int read(void) override
{
// this may return -1, but that's okay
return uart_read_char(_uart);
}
int availableForWrite(void)
{
return static_cast<int>(uart_tx_free(_uart));
}
void flush(void) override;
size_t write(uint8_t) override;
size_t write(uint8_t c) override
{
return uart_write_char(_uart, c);
}
inline size_t write(unsigned long n)
{
return write((uint8_t) n);
Expand All @@ -129,13 +153,27 @@ class HardwareSerial: public Stream
{
return write((uint8_t) n);
}
using Print::write; // pull in write(str) and write(buf, size) from Print

This comment has been minimized.

Sign in to view

Copy link
@devyte

devyte Mar 24, 2018

Collaborator

@d-a-v he's right, the using statement pulls in all overloads.

This comment has been minimized.

Sign in to view

Copy link
@devyte

devyte Mar 24, 2018

Collaborator

@jasoroony you know, you could just make a pr with your fix...
operator bool() const;

size_t write(const uint8_t *buffer, size_t size)
{
return uart_write(_uart, (const char*)buffer, size);
}
operator bool() const
{
return _uart != 0;
}
void setDebugOutput(bool);
bool isTxEnabled(void);
bool isRxEnabled(void);
int baudRate(void);
bool isTxEnabled(void)
{
return uart_tx_enabled(_uart);
}
bool isRxEnabled(void)
{
return _uart && uart_rx_enabled(_uart);
}
int baudRate(void)
{
return uart_get_baudrate(_uart);
}

bool hasOverrun(void)
{
Expand Down
10 changes: 10 additions & 0 deletions cores/esp8266/Print.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@

/* default implementation: may be overridden */
size_t Print::write(const uint8_t *buffer, size_t size) {

#ifdef DEBUG_ESP_CORE
static char not_the_best_way [] ICACHE_RODATA_ATTR STORE_ATTR = "Print::write(data,len) should be overridden for better efficiency\r\n";
static bool once = false;
if (!once) {
once = true;
os_printf_plus(not_the_best_way);
}
#endif

size_t n = 0;
while (size--) {
size_t ret = write(*buffer++);
Expand Down
25 changes: 16 additions & 9 deletions cores/esp8266/uart.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ inline size_t uart_rx_fifo_available(uart_t* uart) {
return (USS(uart->uart_nr) >> USRXC) & 0x7F;

This comment has been minimized.

Sign in to view

Copy link
@devyte

devyte Mar 24, 2018

Collaborator

@jasoroony Given the testing done by @d-a-v , I would not blindly believe the docs.
What you describe is a boundary condition. What would be required is to send 126,/127/128/129 bytes to the serial port, and check how this line of code behaves in each case, then change the line to fit the docs and redo the tests, then compare behaviour and decide which is technically correct.
Care to pick it up yourself?

This comment has been minimized.

Sign in to view

Copy link
@devyte

devyte Mar 24, 2018

Collaborator

You've come up with interesting comments in that other line of investigation, I strongly encourage you to test your ideas (e.g.: guard access of rxbuffer members with turning off interrupts). If you find the cause, you'll have fixed a very difficult bug.

This comment has been minimized.

Sign in to view

Copy link
@devyte

devyte Mar 25, 2018

Collaborator

@jasoroony try undoing your extra line, and changing peek to this:

int uart_peek_char(uart_t* uart)
 {
     if(uart == NULL || !uart->rx_enabled) {
         return -1;
     }
     ETS_UART_INTR_DISABLE();
     if (!uart_rx_available(uart)) {
         ETS_UART_INTR_ENABLE();
         return -1;
     }
     if (uart_rx_buffer_available(uart) == 0) {
         uart_rx_copy_fifo_to_buffer(uart);
     }
     int ret = uart->rx_buffer->buffer[uart->rx_buffer->rpos]
     ETS_UART_INTR_ENABLE();
     return ret;
 }

This comment has been minimized.

Sign in to view

Copy link
@devyte

devyte Mar 25, 2018

Collaborator

The code in question is like a critical section: it has to be guarded against what happens in the isr to keep it consistent. The line you added makes it less likely that the issue you had will be hit, but it is still possible to happen. This change addresses the underlying cause.
If you make a pr, I'd appreciate it.
About the buffer size change, let's discuss that separately.

This comment has been minimized.

Sign in to view

Copy link
@d-a-v

d-a-v Mar 26, 2018

Author Collaborator

@devyte seems that this PR is your duty :)

@jasoroony In your proposal, you check uart->rx_buffer->rpos == uart->rx_buffer->wpos. When code is not protected (ISR disabled, muteces...) then there is a chance that an interrupt and potential changes occur anytime like in the middle of the ==. You can always narrow the problem, the chance will always exist and eventually happen at some point. This why @devyte's proposal is more robust. Anyway thanks for having found and fixed this issue.
Was your application's issue introduced by this commit ?
}

char overrun_str [] ICACHE_RODATA_ATTR STORE_ATTR = "uart input full!\r\n";
const char overrun_str [] ICACHE_RODATA_ATTR STORE_ATTR = "uart input full!\r\n";

// Copy all the rx fifo bytes that fit into the rx buffer
inline void uart_rx_copy_fifo_to_buffer(uart_t* uart) {
Expand Down Expand Up @@ -214,24 +214,31 @@ void uart_stop_isr(uart_t* uart)
ETS_UART_INTR_ATTACH(NULL, NULL);
}

static void uart_do_write_char(uart_t* uart, char c)
{
while((USS(uart->uart_nr) >> USTXC) >= 0x7f);
USF(uart->uart_nr) = c;
}

void uart_write_char(uart_t* uart, char c)
size_t uart_write_char(uart_t* uart, char c)
{
if(uart == NULL || !uart->tx_enabled) {
return;
return 0;
}
while((USS(uart->uart_nr) >> USTXC) >= 0x7f);
USF(uart->uart_nr) = c;
uart_do_write_char(uart, c);
return 1;
}

void uart_write(uart_t* uart, const char* buf, size_t size)
size_t uart_write(uart_t* uart, const char* buf, size_t size)
{
if(uart == NULL || !uart->tx_enabled) {
return;
return 0;
}
while(size--) {
uart_write_char(uart, *buf++);
size_t ret = size;
while (size--) {
uart_do_write_char(uart, *buf++);
}
return ret;
}

size_t uart_tx_free(uart_t* uart)
Expand Down
4 changes: 2 additions & 2 deletions cores/esp8266/uart.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,8 @@ int uart_get_baudrate(uart_t* uart);

size_t uart_resize_rx_buffer(uart_t* uart, size_t new_size);

void uart_write_char(uart_t* uart, char c);
void uart_write(uart_t* uart, const char* buf, size_t size);
size_t uart_write_char(uart_t* uart, char c);
size_t uart_write(uart_t* uart, const char* buf, size_t size);
int uart_read_char(uart_t* uart);
int uart_peek_char(uart_t* uart);
size_t uart_rx_available(uart_t* uart);
Expand Down

0 comments on commit f8f205d

Please sign in to comment.