Skip to content

ports SAMD and RP2: Fix allocation of UART buffers. #16884

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 27, 2025
Merged

ports SAMD and RP2: Fix allocation of UART buffers. #16884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b2a431
rp2/machine_uart: Fix unintended UART buffer allocation on init().
robert-hh Mar 9, 2025
6db7b47
samd/machine_uart: Fix unintended UART buffer allocation on init().
robert-hh Mar 9, 2025
4dfee50
samd/machine_uart: Fix lock-up in loopback mode if read buffer is full.
robert-hh Mar 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 29 additions & 12 deletions ports/rp2/machine_uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,10 @@ typedef struct _machine_uart_obj_t {
uint16_t timeout_char; // timeout waiting between chars (in ms)
uint8_t invert;
uint8_t flow;
uint16_t rxbuf_len;
ringbuf_t read_buffer;
mutex_t *read_mutex;
uint16_t txbuf_len;
ringbuf_t write_buffer;
mutex_t *write_mutex;
uint16_t mp_irq_trigger; // user IRQ trigger mask
Expand All @@ -128,10 +130,10 @@ typedef struct _machine_uart_obj_t {
static machine_uart_obj_t machine_uart_obj[] = {
{{&machine_uart_type}, uart0, 0, 0, DEFAULT_UART_BITS, UART_PARITY_NONE, DEFAULT_UART_STOP,
MICROPY_HW_UART0_TX, MICROPY_HW_UART0_RX, MICROPY_HW_UART0_CTS, MICROPY_HW_UART0_RTS,
0, 0, 0, 0, {NULL, 1, 0, 0}, &read_mutex_0, {NULL, 1, 0, 0}, &write_mutex_0, 0, 0, NULL},
0, 0, 0, 0, 0, {NULL, 1, 0, 0}, &read_mutex_0, 0, {NULL, 1, 0, 0}, &write_mutex_0, 0, 0, NULL},
{{&machine_uart_type}, uart1, 1, 0, DEFAULT_UART_BITS, UART_PARITY_NONE, DEFAULT_UART_STOP,
MICROPY_HW_UART1_TX, MICROPY_HW_UART1_RX, MICROPY_HW_UART1_CTS, MICROPY_HW_UART1_RTS,
0, 0, 0, 0, {NULL, 1, 0, 0}, &read_mutex_1, {NULL, 1, 0, 0}, &write_mutex_1},
0, 0, 0, 0, 0, {NULL, 1, 0, 0}, &read_mutex_1, 0, {NULL, 1, 0, 0}, &write_mutex_1, 0, 0, NULL},
};

static const char *_parity_name[] = {"None", "0", "1"};
Expand Down Expand Up @@ -252,7 +254,7 @@ static void mp_machine_uart_print(const mp_print_t *print, mp_obj_t self_in, mp_
mp_printf(print, "UART(%u, baudrate=%u, bits=%u, parity=%s, stop=%u, tx=%d, rx=%d, "
"txbuf=%d, rxbuf=%d, timeout=%u, timeout_char=%u, invert=%s, irq=%d)",
self->uart_id, self->baudrate, self->bits, _parity_name[self->parity],
self->stop, self->tx, self->rx, self->write_buffer.size - 1, self->read_buffer.size - 1,
self->stop, self->tx, self->rx, self->txbuf_len, self->rxbuf_len,
self->timeout, self->timeout_char, _invert_name[self->invert], self->mp_irq_trigger);
}

Expand Down Expand Up @@ -365,25 +367,33 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args,
}

// Set the RX buffer size if configured.
size_t rxbuf_len = DEFAULT_BUFFER_SIZE;
if (args[ARG_rxbuf].u_int > 0) {
rxbuf_len = args[ARG_rxbuf].u_int;
size_t rxbuf_len = args[ARG_rxbuf].u_int;
if (rxbuf_len < MIN_BUFFER_SIZE) {
rxbuf_len = MIN_BUFFER_SIZE;
} else if (rxbuf_len > MAX_BUFFER_SIZE) {
mp_raise_ValueError(MP_ERROR_TEXT("rxbuf too large"));
}
// Force re-allocting of the buffer if the size changed
if (rxbuf_len != self->rxbuf_len) {
self->read_buffer.buf = NULL;
self->rxbuf_len = rxbuf_len;
}
}

// Set the TX buffer size if configured.
size_t txbuf_len = DEFAULT_BUFFER_SIZE;
if (args[ARG_txbuf].u_int > 0) {
txbuf_len = args[ARG_txbuf].u_int;
size_t txbuf_len = args[ARG_txbuf].u_int;
if (txbuf_len < MIN_BUFFER_SIZE) {
txbuf_len = MIN_BUFFER_SIZE;
} else if (txbuf_len > MAX_BUFFER_SIZE) {
mp_raise_ValueError(MP_ERROR_TEXT("txbuf too large"));
}
// Force re-allocting of the buffer if the size changed
if (txbuf_len != self->txbuf_len) {
self->write_buffer.buf = NULL;
self->txbuf_len = txbuf_len;
}
}

// Initialise the UART peripheral if any arguments given, or it was not initialised previously.
Expand Down Expand Up @@ -423,11 +433,14 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args,
uart_set_hw_flow(self->uart, self->flow & UART_HWCONTROL_CTS, self->flow & UART_HWCONTROL_RTS);

// Allocate the RX/TX buffers.
ringbuf_alloc(&(self->read_buffer), rxbuf_len + 1);
MP_STATE_PORT(rp2_uart_rx_buffer[self->uart_id]) = self->read_buffer.buf;

ringbuf_alloc(&(self->write_buffer), txbuf_len + 1);
MP_STATE_PORT(rp2_uart_tx_buffer[self->uart_id]) = self->write_buffer.buf;
if (self->read_buffer.buf == NULL) {
ringbuf_alloc(&(self->read_buffer), self->rxbuf_len + 1);
MP_STATE_PORT(rp2_uart_rx_buffer[self->uart_id]) = self->read_buffer.buf;
}
if (self->write_buffer.buf == NULL) {
ringbuf_alloc(&(self->write_buffer), self->txbuf_len + 1);
MP_STATE_PORT(rp2_uart_tx_buffer[self->uart_id]) = self->write_buffer.buf;
}

// Set the irq handler.
if (self->uart_id == 0) {
Expand All @@ -454,6 +467,10 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg

// Get static peripheral object.
machine_uart_obj_t *self = (machine_uart_obj_t *)&machine_uart_obj[uart_id];
self->rxbuf_len = DEFAULT_BUFFER_SIZE;
self->read_buffer.buf = NULL;
self->txbuf_len = DEFAULT_BUFFER_SIZE;
self->write_buffer.buf = NULL;

// Initialise the UART peripheral.
mp_map_t kw_args;
Expand Down
52 changes: 35 additions & 17 deletions ports/samd/machine_uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,10 @@ typedef struct _machine_uart_obj_t {
uint16_t timeout; // timeout waiting for first char (in ms)
uint16_t timeout_char; // timeout waiting between chars (in ms)
bool new;
uint16_t rxbuf_len;
ringbuf_t read_buffer;
#if MICROPY_HW_UART_TXBUF
uint16_t txbuf_len;
ringbuf_t write_buffer;
#endif
#if MICROPY_PY_MACHINE_UART_IRQ
Expand Down Expand Up @@ -154,7 +156,8 @@ void common_uart_irq_handler(int uart_id) {
}
}
#endif
} else if (uart->USART.INTFLAG.bit.DRE != 0) {
}
if (uart->USART.INTFLAG.bit.DRE != 0) {
#if MICROPY_HW_UART_TXBUF
// handle the outgoing data
if (ringbuf_avail(&self->write_buffer) > 0) {
Expand Down Expand Up @@ -287,16 +290,6 @@ void machine_uart_set_baudrate(mp_obj_t self_in, uint32_t baudrate) {

static void mp_machine_uart_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
machine_uart_obj_t *self = MP_OBJ_TO_PTR(self_in);
size_t rxbuf_len = self->read_buffer.size - 1;
#if MICROPY_HW_UART_TXBUF
size_t txbuf_len = self->write_buffer.size - 1;
#endif
if (self->bits > 8) {
rxbuf_len /= 2;
#if MICROPY_HW_UART_TXBUF
txbuf_len /= 2;
#endif
}

mp_printf(print, "UART(%u, baudrate=%u, bits=%u, parity=%s, stop=%u, "
"tx=\"%q\", rx=\"%q\", timeout=%u, timeout_char=%u, rxbuf=%d"
Expand All @@ -312,9 +305,9 @@ static void mp_machine_uart_print(const mp_print_t *print, mp_obj_t self_in, mp_
")",
self->id, self->baudrate, self->bits, _parity_name[self->parity],
self->stop + 1, pin_find_by_id(self->tx)->name, pin_find_by_id(self->rx)->name,
self->timeout, self->timeout_char, rxbuf_len
self->timeout, self->timeout_char, self->rxbuf_len
#if MICROPY_HW_UART_TXBUF
, txbuf_len
, self->txbuf_len
#endif
#if MICROPY_HW_UART_RTSCTS
, self->rts != 0xff ? pin_find_by_id(self->rts)->name : MP_QSTR_None
Expand Down Expand Up @@ -358,6 +351,11 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args,
// Set bits if configured.
if (args[ARG_bits].u_int > 0) {
self->bits = args[ARG_bits].u_int;
// Invalidate the buffers since the size may have to be changed
self->read_buffer.buf = NULL;
#if MICROPY_HW_UART_TXBUF
self->write_buffer.buf = NULL;
#endif
}

// Set parity if configured.
Expand Down Expand Up @@ -414,26 +412,36 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args,
}

// Set the RX buffer size if configured.
size_t rxbuf_len = DEFAULT_BUFFER_SIZE;
size_t rxbuf_len = self->rxbuf_len;
if (args[ARG_rxbuf].u_int > 0) {
rxbuf_len = args[ARG_rxbuf].u_int;
if (rxbuf_len < MIN_BUFFER_SIZE) {
rxbuf_len = MIN_BUFFER_SIZE;
} else if (rxbuf_len > MAX_BUFFER_SIZE) {
mp_raise_ValueError(MP_ERROR_TEXT("rxbuf too large"));
}
// Force re-allocting of the buffer if the size changed
if (rxbuf_len != self->rxbuf_len) {
self->read_buffer.buf = NULL;
self->rxbuf_len = rxbuf_len;
}
}

#if MICROPY_HW_UART_TXBUF
// Set the TX buffer size if configured.
size_t txbuf_len = DEFAULT_BUFFER_SIZE;
size_t txbuf_len = self->txbuf_len;
if (args[ARG_txbuf].u_int > 0) {
txbuf_len = args[ARG_txbuf].u_int;
if (txbuf_len < MIN_BUFFER_SIZE) {
txbuf_len = MIN_BUFFER_SIZE;
} else if (txbuf_len > MAX_BUFFER_SIZE) {
mp_raise_ValueError(MP_ERROR_TEXT("txbuf too large"));
}
// Force re-allocting of the buffer if the size changed
if (txbuf_len != self->txbuf_len) {
self->write_buffer.buf = NULL;
self->txbuf_len = txbuf_len;
}
}
#endif

Expand Down Expand Up @@ -462,10 +470,14 @@ static void mp_machine_uart_init_helper(machine_uart_obj_t *self, size_t n_args,
}

// Allocate the RX/TX buffers.
ringbuf_alloc(&(self->read_buffer), rxbuf_len + 1);
if (self->read_buffer.buf == NULL) {
ringbuf_alloc(&(self->read_buffer), rxbuf_len + 1);
}

#if MICROPY_HW_UART_TXBUF
ringbuf_alloc(&(self->write_buffer), txbuf_len + 1);
if (self->write_buffer.buf == NULL) {
ringbuf_alloc(&(self->write_buffer), txbuf_len + 1);
}
#endif

// Step 1: Configure the Pin mux.
Expand Down Expand Up @@ -503,6 +515,12 @@ static mp_obj_t mp_machine_uart_make_new(const mp_obj_type_t *type, size_t n_arg
self->stop = 0;
self->timeout = 1;
self->timeout_char = 1;
self->rxbuf_len = DEFAULT_BUFFER_SIZE;
self->read_buffer.buf = NULL;
#if MICROPY_HW_UART_TXBUF
self->txbuf_len = DEFAULT_BUFFER_SIZE;
self->write_buffer.buf = NULL;
#endif
#if defined(pin_TX) && defined(pin_RX)
// Initialize with the default pins
self->tx = mp_hal_get_pin_obj((mp_obj_t)pin_TX);
Expand Down
Loading