Skip to content
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.

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

pybricks/experimental: Add LWP3Device. #68

Merged
merged 2 commits into from
Aug 30, 2021
Merged

Conversation

NStrijbosch
Copy link
Contributor

Given that bluetooth communication with other devices than the remote was pushed forward on your timeline, I gave this a shot.

These commits could provide a starting point to the LWP3Device class as mentioned here: pybricks/support#262 (comment)

This is tested on: Technic Hub/City Hub/Prime Hub, each connects/reads/writes successfully in combination with a City hub and Remote running the stock LEGO firmware. Mario was previously working but can currently not connect see: pybricks/support#442.

I am curious what your view is on this new feature, any feedback is welcome!

I will provide some code examples in the comments below.

@NStrijbosch
Copy link
Contributor Author

NStrijbosch commented Aug 20, 2021

Example to blink the LEDs on two city hubs in sync:

from pybricks.experimental import LWP3Device
from pybricks.parameters import Color
from pybricks.tools import wait
from pybricks.hubs import CityHub

CITY_HUB = const(0x41)

hub_pybricks = CityHub()

# Connect to the City Hub.
hub_lwp3 = LWP3Device(CITY_HUB)

print("connected")

while True:
    
    # Set LEDs to BLUE
    hub_lwp3.write(b'\x08\x00\x81\x32\x11\x51\x00\x03')
    hub_pybricks.light.on(Color.BLUE)
    
    wait(1000)
    
    # Set LEDs to GREEN
    hub_lwp3.write(b'\x08\x00\x81\x32\x11\x51\x00\x06')
    hub_pybricks.light.on(Color.GREEN)
    
    wait(1000)

@NStrijbosch
Copy link
Contributor Author

NStrijbosch commented Aug 20, 2021

Example to use Mario's barcode/color sensor to detect if Mario is on red:

from pybricks.experimental import LWP3Device
from pybricks.parameters import Color
from pybricks.tools import wait
from pybricks.hubs import TechnicHub

MARIO = const(0x43)

hub = TechnicHub()

# Connect to the Mario
mario = LWP3Device(MARIO)
print("connected")

#subscribe to Mario barcode/color sensor
mario.write(b'\x0a\x00\x41\x01\x00\x01\x00\x00\x00\x01')

wait(500)

while True:
    msg = mario.read()
    
    print("msg: ", msg[6])

    if msg[6] == 26: #Mario is on red
        hub.light.on(Color.RED)
    else:
        hub.light.on(Color.BLUE)

    wait(100)

@dlech
Copy link
Member

dlech commented Aug 20, 2021

I think this looks like a great start. From a high level, I would eventually like to be able to use this in a non-blocking way, i.e. the object should be pollable. This applies to both reading and writing. For writing we would need to use write without response to make it non-blocking. For reading, I wonder if we can process notifications fast enough or if we need to buffer more than one message.

We might also want to think about eventually adding specialized methods for commonly used functions like getting/setting hub properties or subscribing to a port. But for now, I think this barebones approach is just fine.

@NStrijbosch
Copy link
Contributor Author

For writing we would need to use write without response to make it non-blocking.

This indeed would be nice. Although what I noticed when you changed from read without response to read with response on the Technic/City hubs the reliability of the communication greatly improved (not sure if this was due to another fact). So if this change is made, it would be nice to at least have an option to make it write with response to obtain this good reliability.

or if we need to buffer more than one message.

For now this single message was the easiest to implement. To buffer more messages it might be best to think about a smart way how to buffer more messages, since some messages, e.g., sensor data, are high frequent and others, e.g., mode information, are low frequent. In other words if you buffer all messages in a buffer a small buffer, this could get full of sensor data quickly , thereby missing low frequent messages. Moreover, a buffer full of old sensor data is also useless, most use cases only the latest is of interest.

A possible way could be the following: buffer the last message for each port (idea from a discussion with @laurensvalk)

adding specialized methods for commonly used functions like getting/setting hub properties or subscribing to a port.

For now I aimed for the complete barebones indeed, it is definitely not beginner level but a lot can be done. In the future definitely more general functions can of course be added.

@laurensvalk
Copy link
Member

laurensvalk commented Aug 21, 2021

Nice work @NStrijbosch. I'm looking forward to trying this out.

This class looks like a good fit for the pybricks.iodevices module. I wonder if we could make the name a bit easier than LWP3Device. (Maybe PUPDevice should have been PoweredUpDevice as well.)

from pybricks.experimental import LWP3Device
from pybricks.parameters import Color, LWP3
mario = LWP3Device(LWP3.MARIO)

I think we could do without the separate LWP3 class in parameters. One option would be to use LWP3Device.MARIO, but it might be even better to simply use MARIO = const(0x43). This is a pattern that MicroPython tends to follow with general purpose modules/classes, and this is one of those.

It saves quite a bit of space, and it will allow people to use this class right away to explore new devices when they come out, without having to wait for us to update the parameters in a new firmware release.

@NStrijbosch
Copy link
Contributor Author

I wonder if we could make the name a bit easier than LWP3Device. (Maybe PUPDevice should have been PoweredUpDevice as well.)

Following the same reasoning as above for PUPDevice, LegoWirelessProtocolDevice is an option, which I don't think is easier...
BLEHub or BluetoothHub don't seem good candidates either since this will get confusing when NUS is implemented to connect two hubs running Pybricks.

I think the name should indicate that you are dealing with a hub running the official LEGO Firmware. Some candidates

  • LegoBtHub, LegoBtDevice, LegoBluetoothHub, LegoBluetoothDevice (in other words replace LWP3 with LegoBluetooth)
  • PUPBluetoothDevice, PUPBluetoothHub (similar to above, but more specific to PoweredUp devices, not sure if everyone would count Mario and Duplo train in this category though)
  • StockHub, StandardHub (in contrast to the ones above these aim to emphasize that you are dealing with a hub with the Standard/Stock firmware, i.e., not another Pybricks hub)

To me there is no clear winner, any other suggestions are welcome too (I am not convinced that the perfect name is in this list yet)

it might be even better to simply use MARIO = const(0x43). it will allow people to use this class right away to explore new devices when they come out, without having to wait for us to update the parameters in a new firmware release.

This is a valid point, MARIO = const(0x43) seems alright to me.

@JorgePe
Copy link
Contributor

JorgePe commented Aug 21, 2021

"I` think the name should indicate that you are dealing with a hub running the official LEGO Firmware."

I would say "running the official LEGO protocol" since it should be possible to create a 3rd party device that uses the protocol (I believe Gianlucca achieved that)

LWP3Device is fine. A better name might be PUPDevice but that would force Pybricks to change it's own name to PBDevice or something for consistance sake (or their long names: PoweredUpDevice, PybricksDevice...).

@NStrijbosch
Copy link
Contributor Author

I would say "running the official LEGO protocol" since it should be possible to create a 3rd party device that uses the protocol

True, and another reason not to fix the possible LWP3 Devices in the firmware but use something like MARIO = const(0x43) instead.

Another view point on the name discussion: given that to be able to use this class one should have some basic understanding on the LWP, i.e., for those LWP3Device() is an intuitive name. For those who don't have a clue about LWP it is clear from the name that they need to learn something new to be able to use this class. Hence, in case there is sufficient links/other references in the docs the name LWP3Device() could be fine.

@NStrijbosch
Copy link
Contributor Author

I think we could do without the separate LWP3 class in parameters. One option would be to use LWP3Device.MARIO, but it might be even better to simply use MARIO = const(0x43).

I removed the LWP3 class in the most recent commit. Code examples above are updated.

@NStrijbosch
Copy link
Contributor Author

Of course a code example with the Duplo train was still missing.

Blinking the LED on a cityhub depending on the TAG measurement of the Duplo train base:

from pybricks.experimental import LWP3Device
from pybricks.parameters import Color
from pybricks.tools import wait
from pybricks.hubs import CityHub

DUPLO_TRAIN = const(0x20)

hub = CityHub()

# Connect to the Duplo train.
duplo_train = LWP3Device(DUPLO_TRAIN)
print("connected")

# Subscribe to color sensor in mode  'C TAG' (mode 1)
duplo_train.write(b'\x0a\x00\x41\x12\x01\x01\x00\x00\x00\x01')

# Start duplo train
duplo_train.write(b'\x08\x00\x81\x00\x11\x51\x00\x28')

while True:
    # read notifications from color sensor
    msg = duplo_train.read()
    	
    if msg[4] is 3:
    	hub.light.on(Color.BLUE)
    elif msg[4] is 5:
    	hub.light.on(Color.GREEN)
    elif msg[4] is 7:
    	hub.light.on(Color.YELLOW)
    elif msg[4] is 9:
    	hub.light.on(Color.RED)

    wait(10)

Copy link
Member

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a close look and there are some details to take care of (see inline comments), but overall it looks great.

One high-level thing that we need to think about is blocking vs. non-blocking reads and writes. Currently, this is implemented with a blocking write but a non-blocking read. Ideally, both read and write should be the same when it comes to blocking.

So I would like to suggest that we change the read() method to be blocking for now. This means that if no notifications have been received since the last read, then the function blocks until a new notification has been received.

Then, in the future, we can add a blocking=True parameter to the object constructor. When blocking is set to false, then the read() method will return None instead of blocking if there are no pending messages and the write() method will use Write Without Response instead of Write With Response and will return None if the Bluetooth chip is busy and can't send immediately.

A blocking read might look like this:

STATIC mp_obj_t lwp3device_read(mp_obj_t self_in) {
    pb_lwp3device_t *lwp3device = &pb_lwp3device_singleton;

    // wait until a notification is received
    for (;;) {
        lwp3device_assert_connected();

        if (lwp3device->notification_received) {
            lwp3device->notification_received = false;
            break;
        }

        MICROPY_EVENT_POLL_HOOK
    }

    size_t len = lwp3device->buffer[0];

    if (len < 3 /* header size */ || len > MAX_MESSAGE_SIZE) {
        mp_raise_msg(&mp_type_RuntimeError, "bad data");
    }

    return mp_obj_new_bytes(lwp3device->buffer, len);
}
STATIC MP_DEFINE_CONST_FUN_1(lwp3device_read_obj, lwp3device_read);

lwp3device->notification_received = true would be called in handle_notification().

lib/lego/lego_lwp3.h Outdated Show resolved Hide resolved
lib/pbio/include/pbdrv/bluetooth.h Outdated Show resolved Hide resolved
pybricks/experimental.h Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
pybricks/experimental/pb_type_experimental_lwp3device.c Outdated Show resolved Hide resolved
@dlech
Copy link
Member

dlech commented Aug 23, 2021

And a couple of administrative things:

  • Let's add a CHANGELOG.md entry for this.
  • Feel free to add yourself to AUTHORS.md if you like (start a new **Contributors:** section at the end).

@dlech
Copy link
Member

dlech commented Aug 23, 2021

if msg[4] is 3:

It is generally considered "bad" to use is with integers in Python. It may work 99.9% of the time, but not 100% of the time, so it is not a good habit to form. == should be used instead.

Copy link
Member

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Nard, thanks for making the changes. I've tested this out and it seems to be working. I'm using the City hub and the Bluetooth disconnects from both the computer and Mario after a while when the program is running. I'm not sure why, but I don't think it is related to any of the code here, so I will open a new issue for it.

Here is the program I used for testing. I modified your example a bit to take advantage of the changes you made.

from pybricks.hubs import ThisHub
from pybricks.iodevices import LWP3Device
from pybricks.parameters import Color

MARIO = const(0x43)
PORT_VALUE_MSG = const(0x45)

hub = ThisHub()

# Connect to the Mario
print("waiting for Mario...")
mario = LWP3Device(MARIO)
print("connected")

#subscribe to Mario barcode/color sensor
mario.write(b'\x0a\x00\x41\x01\x00\x01\x00\x00\x00\x01')

while True:
    msg = mario.read()

    kind = msg[2]
    
    if kind != PORT_VALUE_MSG:
        continue

    color = msg[6]
    
    print("color:", color)

    if color == 21:  # Mario is on red
        hub.light.on(Color.RED)
    else:
        hub.light.on(Color.BLUE)

I've requested a few more changes. Just mostly nitpicks to help keep things neat and consistent plus a few corrections. After that, we should be ready to merge.

bricks/stm32/stm32.mk Outdated Show resolved Hide resolved
pybricks/iodevices.h Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
pybricks/iodevices/pb_type_iodevices_lwp3device.c Outdated Show resolved Hide resolved
Class to communicate with a device that supports the LEGO Wireless Protocol 3.0.00.
Copy link
Member

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants