UART: Always allocate UART objects in the long-lived pool #1080
Conversation
shared-bindings/busio/UART.c
Outdated
| @@ -66,7 +66,7 @@ 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); | |||
| busio_uart_obj_t *self = m_new_ll_obj(busio_uart_obj_t); | |||
There was a problem hiding this comment.
You shouldn't need this. If it long lives the object later the buffer will stay in the same spot.
https://github.com/adafruit/circuitpython/blob/master/py/gc.c#L727
There was a problem hiding this comment.
It's the busio_uart_obj_t itself which MUST initially be allocated in the long-lived pool:
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.
There was a problem hiding this comment.
Ah! My bad. Please add the comment above then it should be good to go. Thanks!
| @@ -132,7 +132,7 @@ 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); | |||
| self->buffer = (uint8_t *) gc_alloc(self->buffer_length * sizeof(uint8_t), false, true); | |||
There was a problem hiding this comment.
Please add a comment explaining why its long lived.
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: adafruit#1056
This is not strictly needed in order for adafruit#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.
|
@tannewt I've added source code comments at both changed sites, hopefully they provide good signposts for future contributors. |
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
Please read the comments in #1078.
Testing performed: 10 iterations of my "badecho" test with no failures.