"make long lived" adversely affects UART on Metro M4 Express

[jepler]

This is a weird one, so I hope someone else can reproduce it.

I have a Metro M4 Express marked "beta", purchased from the Adafruit store on May 8. It's loaded with the 3.0 release: Adafruit CircuitPython 3.0.0 on 2018-07-09; Adafruit Metro M4 Express with samd51j19. Attached to it is a GPS shield which at power on starts sending NMEA sentences every second.

Normally, an 'echo' program will work properly, showing GPS packets to the computer via USB. While preparing a minimum test case to file this issue, I pared it down to this, which just reads up to 96 bytes:

import board
import busio

uart = busio.UART(board.TX, board.RX, baudrate=9600)

#class F:
#    def f(): pass

data = uart.read(96)
print(repr(data))

Note the commented-out class F definition. When saved as echo.py, it can be imported and it shows the first few bytes of communication with the GPS:

Adafruit CircuitPython 3.0.0 on 2018-07-09; Adafruit Metro M4 Express with samd51j19
>>> import echo
b'$GPRMC,013202.00,A,4047.12000,N,09641.84538,W,0.154,,250718,,,A*65\r\n$GPVTG,,T,,M,0.154,N,0.285,K'

If I uncomment the definition class F, the code breaks, read always returns None.

Adafruit CircuitPython 3.0.0 on 2018-07-09; Adafruit Metro M4 Express with samd51j19
>>> import badecho
None

It is nothing to do with the amount of time spent between uart = busio.UART... and the first uart.read, because inserting time.sleep(2) there works just fine.

[jerryneedell]

I have reproduced this on a metro m4 express with an Ultimate GPS breakout: good_uart.py has the lines commented -- bad_uart.py has them uncommented

Press any key to enter the REPL. Use CTRL-D to reload.

Adafruit CircuitPython 4.0.0-alpha on 2018-07-20; Adafruit Metro M4 Express with samd51j19
>>> 
>>> 
>>> import good_uart
b'$GPGGA,023220.000,4308.5911,N,07055.3644,W,1,04,1.63,179.9,M,-32.9,M,,*5D\r\n$GPGSA,A,3,21,24,15,1'
>>> 
soft reboot

Auto-reload is on. Simply save files over USB to run them or enter REPL to disable.



Press any key to enter the REPL. Use CTRL-D to reload.

Adafruit CircuitPython 4.0.0-alpha on 2018-07-20; Adafruit Metro M4 Express with samd51j19
>>> import bad_uart
None
>>> 
>>> 
soft reboot

Auto-reload is on. Simply save files over USB to run them or enter REPL to disable.



Press any key to enter the REPL. Use CTRL-D to reload.

Adafruit CircuitPython 4.0.0-alpha on 2018-07-20; Adafruit Metro M4 Express with samd51j19
>>> import good_uart
b'\xaa\xaar\x9a\x82\xba\x9ab\xba\xb10.47,163.97,250718,,,A*73\r\n$GPVTG,163.97,T,,M,0.47,N,0.88,K,A*34\r\n$GPGGA,023247.000,43'
>>> 

[jerryneedell]

Warning: running this test has makes the system unstable. In one instance, it just hung. In another , it eventually disconnected the USB port. Something is not playing nice....

[jerryneedell]

FWIW - my normal GPS capture program runs just fine for extended time periods using the UART.

[jepler]

Thanks @jerryneedell I'm glad to see this problem confirmed by somebody else. I don't even know where to start debugging this, but I'll stick the SWD debugger on it next chance I get and work backwards from common_hal_busio_uart_read in the hopes something turns up. It may not be before the weekend.

I see that during the execution of a class statement, we do end up reaching into the "make long-lived" machinery: o->locals_dict = make_dict_long_lived(locals_dict, 10); I also plan to pursue the angle that the problem could stem from this, e.g., because the uart REALLY doesn't like its ringbuffer being relocated unannounced. Perhaps under this theory gc_make_long_lived would need to disable interrupts... This is totally speculative at this point, but it should be possible to disprove pretty easily.

[jepler]

Yes, neutering the long-lived machinery makes the badecho program work reliably.

diff --git a/py/gc.c b/py/gc.c
index 2c2354e4c..4d18c57dc 100755
--- a/py/gc.c
+++ b/py/gc.c
@@ -679,6 +679,7 @@ bool gc_has_finaliser(const void *ptr) {
 }
 
 void *gc_make_long_lived(void *old_ptr) {
+    if (1) return old_ptr;
     // If its already in the long lived section then don't bother moving it.
     if (old_ptr >= MP_STATE_MEM(gc_lowest_long_lived_ptr)) {
         return old_ptr;

It's not 100% conclusive because I built from a slightly older than 3.0 ref for this test, but I would take a bet about it at this point.

[tannewt]

@dhalbert just made a fix to this. It may help this problem too. 08def6b

[dhalbert]

The fix I made in 08def6b should not affect what gets long-lived or not: it's meant just to prevent redundant walking through dicts. So I don't think it will fix this problem.

The code to create the buffer is here and it's not long-lived. The second false says that. https://github.com/adafruit/circuitpython/blob/master/ports/atmel-samd/common-hal/busio/UART.c#L135.

Perhaps it should be long-lived, along with any other such dynamically allocated storage in other common-hal classes, because that would mean it would not be relocated. "Not relocatable" is the semantics we want. Previously, since the GC is not compacting, all such code assumed dynamically allocated storage would not move.

[jepler]

This change, which makes the buffer object as long-lived from the get go, does not fix the problem:

diff --git a/ports/atmel-samd/common-hal/busio/UART.c b/ports/atmel-samd/common-hal/busio/UART.c
index bcdeae7b5..e7aed11a6 100644
--- a/ports/atmel-samd/common-hal/busio/UART.c
+++ b/ports/atmel-samd/common-hal/busio/UART.c
@@ -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);
         if (self->buffer == NULL) {
             common_hal_busio_uart_deinit(self);
             mp_raise_msg(&mp_type_MemoryError, "Failed to allocate RX buffer");

However, when I additionally make the UART object itself long-lived from creation, it appears to be fixed:

diff --git a/shared-bindings/busio/UART.c b/shared-bindings/busio/UART.c
index 1b63d8d0e..3c6ddb123 100644
--- a/shared-bindings/busio/UART.c
+++ b/shared-bindings/busio/UART.c
@@ -33,11 +33,11 @@
 #include "lib/utils/context_manager_helpers.h"
 
 #include "py/ioctl.h"
+#include "py/gc.h"
 #include "py/objproperty.h"
 #include "py/runtime.h"
 #include "py/stream.h"
 
-
 //| .. currentmodule:: busio
 //|
 //| :class:`UART` -- a bidirectional serial protocol
@@ -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 = gc_make_long_lived(m_new_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);

(which of course affects all UARTs in all ports, not just M4 UARTs)