From e1b4e9b7c7cdd7a022a5ab113b68c8fa6be2c74d Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Mon, 30 Jul 2018 20:49:20 -0500 Subject: [PATCH 1/2] UART: Always allocate UART objects in the long-lived pool Particularly when they have buffers that are written via IRQ or DMA, UART objects do not relocate gracefully. If such an object is relocated to the long-lived pool after its original creation, the IRQ or DMA will write to an unexpected location within the Python heap, leading to a variety of symptoms. The most frequent symptom is inability to read from the UART. Consider the particular case of atmel-samd: usart_uart_obj_t contains a usart_async_descriptor contains a _usart_async_device. In _sercom_init_irq_param the address of this contained _usart_async_device is assigned to a global array sercom_to_sercom_dev which is later used from the interrupt context _sercom_usart_interrupt_handler to store the received data in the right ring buffer. When the UART object is relocated to the long-lived heap, there's no mechanism to re-point these internal pointers, so instead take the cowardly way and allocate the UART object as long-lived. Happily, almost all UART objects are likely to be long-lived, so this is unlikely to have a negative effect on memory usage or heap fragmentation. Closes: #1056 --- shared-bindings/busio/UART.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/shared-bindings/busio/UART.c b/shared-bindings/busio/UART.c index 1b63d8d0e0..9bad2468e5 100644 --- a/shared-bindings/busio/UART.c +++ b/shared-bindings/busio/UART.c @@ -66,7 +66,11 @@ extern const busio_uart_parity_obj_t busio_uart_parity_odd_obj; STATIC mp_obj_t busio_uart_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *pos_args) { mp_arg_check_num(n_args, n_kw, 0, MP_OBJ_FUN_ARGS_MAX, true); - busio_uart_obj_t *self = m_new_obj(busio_uart_obj_t); + // Always initially allocate the UART object within the long-lived heap. + // This is needed to avoid crashes with certain UART implementations which + // cannot accomodate being moved after creation. (See + // https://github.com/adafruit/circuitpython/issues/1056) + busio_uart_obj_t *self = m_new_ll_obj(busio_uart_obj_t); self->base.type = &busio_uart_type; mp_map_t kw_args; mp_map_init_fixed_table(&kw_args, n_kw, pos_args + n_args); From b0e33f6a116ac75c6a26216224182529db78e0a3 Mon Sep 17 00:00:00 2001 From: Jeff Epler Date: Wed, 1 Aug 2018 20:21:20 -0500 Subject: [PATCH 2/2] atmel-samd: UART: allocate rx buffer in long-lived region This is not strictly needed in order for #1056 to be resolved, because the "make long-lived" machinery is unaware of this pointer. However, as UARTs are assumed to be long-lived, this change is beneficial because it moves the long-lived buffer into the upper memory area with other long-lived objects, instead of remaining in the low heap. --- ports/atmel-samd/common-hal/busio/UART.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ports/atmel-samd/common-hal/busio/UART.c b/ports/atmel-samd/common-hal/busio/UART.c index bcdeae7b53..20e662ec62 100644 --- a/ports/atmel-samd/common-hal/busio/UART.c +++ b/ports/atmel-samd/common-hal/busio/UART.c @@ -132,7 +132,13 @@ void common_hal_busio_uart_construct(busio_uart_obj_t *self, if (rx && receiver_buffer_size > 0) { self->buffer_length = receiver_buffer_size; - self->buffer = (uint8_t *) gc_alloc(self->buffer_length * sizeof(uint8_t), false, false); + // Initially allocate the UART's buffer in the long-lived part of the + // heap. UARTs are generally long-lived objects, but the "make long- + // lived" machinery is incapable of moving internal pointers like + // self->buffer, so do it manually. (However, as long as internal + // pointers like this are NOT moved, allocating the buffer + // in the long-lived pool is not strictly necessary) + self->buffer = (uint8_t *) gc_alloc(self->buffer_length * sizeof(uint8_t), false, true); if (self->buffer == NULL) { common_hal_busio_uart_deinit(self); mp_raise_msg(&mp_type_MemoryError, "Failed to allocate RX buffer");