pybricks.iodevices.LWP3Device: Buffer incoming notifications.

[laurensvalk]

Allows to read without blocking and allows subscribing to multiple sensor values without losing data.

As an example, here's how you can read both the color and the speed of the Duplo Train:

Fixes pybricks/support#1648

It works well, but it can sometimes crash the hub when stopping the program. Perhaps because of the buffer being freed before the connection is closed, so this might need some more work.

from pybricks.iodevices import LWP3Device
from pybricks.tools import wait

from ustruct import unpack

DUPLO_TRAIN_ID = 0x20
PORT_VALUE_MSG = const(0x45)

PORT_COLOR_SENSOR = 0x12
MODE_RGB = 0x03

PORT_SPEED = 0x13
MODE_SPEED = 0x00
MODE_POSITION = 0x01

print("Searching for the train. Make sure it is on and blinking its front light.")

# Buffer latest 16 notifications. Can reduce this if you read often.
device = LWP3Device(DUPLO_TRAIN_ID, num_notifications=16)

# Subscribe to color sensor and speed
device.write(bytes([0x0a, 0x00, 0x41, PORT_COLOR_SENSOR, MODE_RGB, 0x01, 0x00, 0x00, 0x00, 0x01]))
device.write(bytes([0x0a, 0x00, 0x41, PORT_SPEED, MODE_SPEED, 0x01, 0x00, 0x00, 0x00, 0x01]))

print("Connected!")
wait(500)

def parse_all():
    while (data := device.read()) is not None:

        if len(data) < 4:
            continue

        kind = data[2]
        port = data[3]
        
        if kind != PORT_VALUE_MSG:
            continue

        if port == PORT_COLOR_SENSOR and len(data) == 10:
            r, g, b = unpack("<hhh", data[4:10])
            print(r, g, b)

        if port == PORT_SPEED and len(data) == 6:
            speed, = unpack("<h", data[4:6])
            print(speed)

while True:

    parse_all()

    # All data parsed, go do something else here.
    wait(100)

Tagging @NStrijbosch.

[github-actions]

Download the artifacts for this pull request:

• pr_number.zip
• mpy-cross.zip
• movehub-firmware-build-3842-gitfdfcda80.zip
• ev3-firmware-build-3842-gitfdfcda80.zip
• nxt-firmware-build-3842-gitfdfcda80.zip
• cityhub-firmware-build-3842-gitfdfcda80.zip
• technichub-firmware-build-3842-gitfdfcda80.zip
• primehub-firmware-build-3842-gitfdfcda80.zip
• essentialhub-firmware-build-3842-gitfdfcda80.zip

[coveralls]

coverage: 57.141%. remained the same
when pulling e34e303 on lwp3
into 8dea17f on master.

[dlech]

It works well, but it can sometimes crash the hub when stopping the program. Perhaps because of the buffer being freed before the connection is closed, so this might need some more work.

Yes, we will need to add something like pbdrv_bluetooth_peripheral_disconnect() to balance out pbdrv_bluetooth_peripheral_scan_and_connect(). Most importantly, this should set peri->notification_handler = NULL so that it can no longer be called after the MicroPython wrapper has been freed.

[laurensvalk]

I did that initially, but couldn't see or reproduce how they would clash if I didn't.

We already have pbdrv_bluetooth_peripheral_disconnect and it runs to completion before we mp_deinit and gc_sweep.

Would it be possible for a notification event to come in after we get the disconnect event?

[dlech]

Ah, I missed that we already have pbdrv_bluetooth_peripheral_disconnect() when I looked before.

So now I think the problem is likely this:

pybricks-micropython/pybricks/iodevices/pb_type_iodevices_lwp3device.c

Lines 118 to 120 in e34e303

	static pb_lwp3device_t pb_lwp3device_singleton;
	void *pb_lwp3device_root_pointer = &pb_lwp3device_singleton;
	MP_REGISTER_ROOT_POINTER(void *pb_lwp3device_root_pointer);

Root pointers only make sense when they point to memory allocated by the garbage collector. The GC will see that this pointer didn't come from the heap and just ignore it. So noti_data will likely be garbage collected the first time the GC runs since it isn't reachable. For this to work as expected, pb_lwp3device_root_pointer would need to be dynamically allocated instead of statically. Alternately, we could just set pb_lwp3device_root_pointer = self->noti_data or just move noti_data out of pb_lwp3device_t and use the root pointer directly for that.

[laurensvalk]

Yeah, it would be better to split LWP3 from the dedicated remote class and clean it up a bit. I've been holding off on this since it probably makes the Technic Hub firmware build size bigger. Maybe we can look at it again when revising this part of the code for the simpler pbio/os model.

So noti_data will likely be garbage collected the first time the GC runs since it isn't reachable.

I wasn't able to reproduce the crash even by forcing it to collect.

or just move noti_data out of pb_lwp3device_t and use the root pointer directly for that.

I'll take this approach to keep it simple as per the above note about build size. Thanks for the help!

[dlech]

I wasn't able to reproduce the crash even by forcing it to collect.

It would likely only crash if the same memory blocks were reallocated for a new object and then that object was used later after the memory was written over.