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

Add support for programming over USB #208

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nkarstens
Copy link
Contributor

@nkarstens nkarstens commented Oct 25, 2023

Gets Windows to use the WinUSB driver for device. No other functionality is provided at this time.

See pybricks/pybricksdev#69 for changes to pybricksdev.

@coveralls
Copy link

coveralls commented Oct 26, 2023

Coverage Status

coverage: 55.645% (-0.09%) from 55.738%
when pulling de2b325 on nkarstens:usb
into 32e3cb0 on pybricks:master.

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.

Nice work getting this working! I've made a few suggestions.

lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/STM32_USB_Device_Library/Class/CDC/Src/usbd_cdc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/usb_stm32.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/usb_stm32.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/usb_stm32.c Show resolved Hide resolved
@nkarstens nkarstens force-pushed the usb branch 2 times, most recently from 8486b47 to 72d3412 Compare November 9, 2023 07:02
@nkarstens nkarstens force-pushed the usb branch 2 times, most recently from 909644d to 24871fa Compare November 23, 2023 15:35
@nkarstens
Copy link
Contributor Author

This is able to receive a program. Still cleaning up changes to pybricks/pybricksdev#69, but that should be available in a day or two.

This is currently not sending the hub kind or variant. I'm not sure this is needed, but I could encode that in the BOS descriptor using the PnP ID UUID, like in BLE. What do you think, @dlech?

@dlech
Copy link
Member

dlech commented Nov 23, 2023

This is able to receive a program.

Amazing! I've been wanting to take this for a test drive but haven't had time yet.

This is currently not sending the hub kind or variant.

This is not needed since it can be inferred from the USB VID/PID.

lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/stm32_usbd/usbd_pybricks.h Outdated Show resolved Hide resolved
lib/pbio/drv/usb/usb_stm32.c Show resolved Hide resolved
@nkarstens
Copy link
Contributor Author

Amazing! I've been wanting to take this for a test drive but haven't had time yet.

I'm pretty happy with how it came together. My main concern now is that it will only work on a SPIKE Prime because I don't have any of the other bricks to test it with. How do you think we should handle that?

This is not needed since it can be inferred from the USB VID/PID.

In pybricksdev, should I set the hub_kind and hub_variant manually then? What should those be?

@dlech
Copy link
Member

dlech commented Nov 24, 2023

In pybricksdev, should I set the hub_kind and hub_variant manually then? What should those be?

You can find all of the numbers here (both USB PID/VID and hub kind/variant): https://github.com/pybricks/technical-info/blob/master/assigned-numbers.md

How do you think we should handle that?

I can test the other hubs. We will need to add some code that reads the variant in the case of SPIKE Prime and Robot Inventor that will select the correct PID at runtime. The SPIKE Essential hub can just have the PID as a compile option.

@nkarstens nkarstens force-pushed the usb branch 3 times, most recently from ebcb76d to a519633 Compare November 26, 2023 19:12
@dlech
Copy link
Member

dlech commented Nov 27, 2023

I finally had a chance to try this out. I've made a few changes I would like to incorporate and pushed them to https://github.com/pybricks/pybricks-micropython/tree/dlech-usb. I didn't want to push to your branch since you are still working on this.

Very cool that it "just works" on Windows.
image

Also tested on Linux.

lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/stm32_usbd/usbd_desc.c Outdated Show resolved Hide resolved
lib/pbio/drv/usb/usb_stm32.c Outdated Show resolved Hide resolved
Comment on lines 381 to 378
((USBD_Pybricks_ItfTypeDef *)pdev->pUserData[pdev->classId])->Receive(hPybricks->RxBuffer, &hPybricks->RxLength);

return (uint8_t)USBD_OK;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
((USBD_Pybricks_ItfTypeDef *)pdev->pUserData[pdev->classId])->Receive(hPybricks->RxBuffer, &hPybricks->RxLength);
return (uint8_t)USBD_OK;
return ((USBD_Pybricks_ItfTypeDef *)pdev->pUserData[pdev->classId])->Receive(hPybricks->RxBuffer, &hPybricks->RxLength);

Copy link
Member

Choose a reason for hiding this comment

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

We should also check other function calls like this to make sure we aren't ignoring return values. (If there is a good reason for ignoring a return value, we should add a comment explaining why.)

@dlech
Copy link
Member

dlech commented Nov 27, 2023

I was also wondering if you gave much thought to using bulk transfers vs interrupt transfers?

@nkarstens
Copy link
Contributor Author

nkarstens commented Nov 27, 2023

I finally had a chance to try this out. I've made a few changes I would like to incorporate and pushed them to https://github.com/pybricks/pybricks-micropython/tree/dlech-usb. I didn't want to push to your branch since you are still working on this.

Great! What is your strategy for this branch? Do you want me to work those changes into my patches or keep those separate?

Very cool that it "just works" on Windows.

What are you using for your backend? I can't remember exactly but I think I downloaded https://github.com/libusb/libusb/releases/download/v1.0.26/libusb-1.0.26-binaries.7z and copied libusb-1.0.26-binaries\VS2015-x64\dll\libusb-1.0.dll into C:\Windows\System32. Is that what you did too?

I was also wondering if you gave much thought to using bulk transfers vs interrupt transfers?

I intended to go with an interrupt transfer because it seemed like the amount of data needing to be transmitted was so small that any performance increase from using a bulk transfer would be negligible. Looking at the capture in Wireshark though it looks like it is using a bulk transfer? PyUSB supposedly infers the correct method, which I thought would have been an interrupt transfer because the endpoints are created using USBD_EP_TYPE_INTR (see USBD_Pybricks_Init()). What are your thoughts?

@dlech
Copy link
Member

dlech commented Nov 27, 2023

Great! What is your strategy for this branch? Do you want me to work those changes into my patches or keep those separate?

The !fixup commit should be squashed but for the others, whatever it easiest for you (I would appreciate credit if you end up using a significant chunk of code that I wrote in your commits).

What are you using for your backend?

I didn't get that far yet on Windows. I just plugged it in to make sure that the driver matching works.

If I can find the time, I would eventually like to make a Python Windows USB library using PyWinRT so that we don't have to worry about libusb there and everything is async.

What are your thoughts?

Using interrupt transfers seems like the right choice for this application to me too. It looks like "Bulk" is specified currently in usbd_pybricks.c.

@nkarstens
Copy link
Contributor Author

I would appreciate credit

Yeah, no problem!

I would eventually like to make a Python Windows USB library using PyWinRT

Are you planning on that for a future improvement or as part of this?

It looks like "Bulk" is specified currently in usbd_pybricks.c

Yes, you're right! I'll change that and see how it works.

@dlech
Copy link
Member

dlech commented Nov 27, 2023

Are you planning on that for a future improvement or as part of this?

Depends on if I have time and motivation 😄 So, I guess consider it a future improvement for now.

@dlech
Copy link
Member

dlech commented Feb 5, 2024

I would expect that it could. It's just a matter of figuring out how to setup all of the config/descriptors/etc. to be the same.

@nkarstens
Copy link
Contributor Author

Can I just chime in and say thanks for your awesome work so far 😄

This got me wondering. How feasible would it be to add an equivalent USB driver to the Pybricks build for EV3RT? It currently has enough high level code to provide a CDC and/or mass storage device driver, which could be disabled and perhaps adapted to work just like the drivers in this PR...

If the intention is to get USB for SPIKE eventually supported in Pybricks Code, getting an equivalent thing going in EV3 may just become feasible at some point. (Which would be amazing).

This is probably a long shot, but if it is remotely feasible, I might start spending some time to try and get other parts of the EV3RT Pybricks port in better shape, like getting motors or sensors going.

You're welcome :)

My understanding is that LEGO has discontinued the EV3. What's the purpose in continuing to support that platform? I imagine there are plenty of EV3 users out there still, but didn't know if there was more to it than that.

@laurensvalk
Copy link
Member

That's precisely it. There are numerous EV3s out there, and it is still the platform of choice for many competition teams.

By making it a bit more like SPIKE (possibly but not necessarily without Linux) it could work with the web IDE just like the others. Once the initial chunk of development is done, it could mostly just be taken along for the ride as we add features to all platforms at once.

LEGO Education recently discontinued the Android and iOS apps (but reneged on this for a few more months after very strong public outcry). Most schools can't just switch to the desktop version.

So it's also from a sustainability point of view, avoiding hundreds of thousands of EV3 bricks becoming e-waste overnight.

Or put differently - you can still build with regular bricks from 1960, so why give up on elements sold as recently as 2018? 😄

@nkarstens nkarstens force-pushed the usb branch 2 times, most recently from 5cca9dc to 5a78c75 Compare February 10, 2024 20:21
@nkarstens
Copy link
Contributor Author

nkarstens commented Feb 10, 2024

Uses a single-byte message ID instead of 16-byte UUID. Still on the TODO list are:

  1. Sending command response messages
  2. Round-robin transmit
  3. Fix WebUSB

@nkarstens
Copy link
Contributor Author

That's precisely it. There are numerous EV3s out there, and it is still the platform of choice for many competition teams.

By making it a bit more like SPIKE (possibly but not necessarily without Linux) it could work with the web IDE just like the others. Once the initial chunk of development is done, it could mostly just be taken along for the ride as we add features to all platforms at once.

LEGO Education recently discontinued the Android and iOS apps (but reneged on this for a few more months after very strong public outcry). Most schools can't just switch to the desktop version.

So it's also from a sustainability point of view, avoiding hundreds of thousands of EV3 bricks becoming e-waste overnight.

Or put differently - you can still build with regular bricks from 1960, so why give up on elements sold as recently as 2018? 😄

That makes sense, and it's great that existing teams wouldn't have to purchase new kits when their existing stuff works just fine. I don't have an EV3 and so wouldn't be able to help with that.

@nkarstens
Copy link
Contributor Author

nkarstens commented Feb 19, 2024

Updated TODO list:

  • Sending command response messages
  • Round-robin transmit
  • Fix WebUSB
  • Resolve outstanding comments (mostly minor fixups)

@dlech
Copy link
Member

dlech commented Feb 20, 2024

The commit "Add function to read VBUS status" should no longer be needed, so I think we can drop that one.

Then to break this down in to smaller parts for testing and final review, it would be nice if we could get all of the BOS stuff working and tested and merge that first, then look at the Pybricks profile stuff.

nkarstens and others added 8 commits February 22, 2024 22:27
Adds a new Pybricks device class and provides the correct
descriptors so that Windows will use the WinUSB driver.

Signed-off-by: Nate Karstens <[email protected]>
Co-authored-by: David Lechner <[email protected]>
Adds function to copy UUIDs in little endian format.
USB is a little endian protocol, so this will be used
to encode UUIDs properly on this medium.

Signed-off-by: Nate Karstens <[email protected]>
Adds additional information to the BOS Descriptor by
appending dynamically-generated platform descriptors
that use the same UUIDs that are used with BLE and
contain the following values:

 * Device Name
 * Firmware version
 * Software (protocol) version
 * Hub capabilities

Signed-off-by: Nate Karstens <[email protected]>
Configures the USB device and processes pybricks commands.

Signed-off-by: Nate Karstens <[email protected]>
Transmit the response to a command. This is used to
communicate any errors in handling the command back
to the user.

Signed-off-by: Nate Karstens <[email protected]>
Periodically sends status flags over the USB connection.

Signed-off-by: Nate Karstens <[email protected]>
Adds support for transmitting stdout messages over USB.

Signed-off-by: Nate Karstens <[email protected]>
Adds a WebUSB platform descriptor to the BOS descriptor
and a landing page pointing to https://code.pybricks.com.

Signed-off-by: Nate Karstens <[email protected]>
@nkarstens
Copy link
Contributor Author

The commit "Add function to read VBUS status" should no longer be needed, so I think we can drop that one.

Then to break this down in to smaller parts for testing and final review, it would be nice if we could get all of the BOS stuff working and tested and merge that first, then look at the Pybricks profile stuff.

Sounds good, I dropped the VBUS status change. WebUSB is still a work-in-progress but support for that is optional. I think we could go ahead and start working on merging changes in.

@laurensvalk
Copy link
Member

Thanks again for your contribution. Could you perhaps add a status summary to the first post in this thread?

It would be great to have an overview of:

  • Implemented functionality.
  • Remaining "to do" functionality.

I'd also love to know what the currently intended user experience is, perhaps in tandem with #195. Is USB currently disabled when the user is connected via Bluetooth or not, for example? What if users try both at the same time?

Is Bluetooth still the default even when USB is plugged in (because it might just be charging)?

I'd be happy to start helping review this and work towards getting it merged.


Separately, I've previously mentioned EV3 compatibility. This is probably a bit too far off.

However, the NXT is starting to look a bit more realistic at this point. Other than a lacking implementation of connectivity for download and run, it is now running "standard" Pybricks (i.e. "like SPIKE Prime") as of this writing. Now that we've gotten this far, I think I'll go ahead and start implementing motors and sensors, which shouldn't be the biggest hurdle.

Its USB driver (obtained from another project) is here. Do you think this contain enough handles to implement something similar as in this PR? Even if we don't go ahead and do this for now, it would be great to have the big picture. Thanks!

@laurensvalk
Copy link
Member

image

Connecting via Bluetooth and starting the REPL via Pybricks Code freezes the output. Cancelling by pressing the stop button then freezes the hub. This seems to happen regardless of whether USB is plugged in.

@laurensvalk
Copy link
Member

I'm still just looking at this with a bird's-eye view as I am catching up with the changes. USB and Bluetooth are not my key expertise, so please forgive me if I'm getting a few things wrong 🙂


Following up on the above, I suppose that USB is intended an alternative to Bluetooth. Supposing that is the case, do we need to handle both USB, BLE (and soon BT) separately at the highest levels?

For example, currently mp_hal_stdout_tx_strn has implementations for USB and BLE, handling both pbdrv_usb_stdout_tx and pbsys_bluetooth_tx. Would it make sense to just have pbsys_stdout_tx for this?

Then pbsys would be responsible for sending it to the currently active mode, either BLE, USB, or BT-RFCOMM.

So basically what is currently pbsys/bluetooth becomes communication type agnostic. To a large extent, it already is.

Such a BLE/USB/BT mode might also be one way to address the notes above:

I'd also love to know what the currently intended user experience is, perhaps in tandem with #195. Is USB currently disabled when the user is connected via Bluetooth or not, for example? What if users try both at the same time? Is Bluetooth still the default even when USB is plugged in (because it might just be charging)?

@dlech
Copy link
Member

dlech commented Mar 24, 2024

, do we need to handle both USB, BLE (and soon BT) separately at the highest levels?

If you mean in the firmware, no. If you mean in Pybricks Code, yes.

Would it make sense to just have pbsys_stdout_tx for this?

Yes. I would expect this to be like upstream MicroPython's dupterm feature.

Then pbsys would be responsible for sending it to the currently active mode, either BLE, USB, or BT-RFCOMM.

There is no "active" mode from the firmware point of view. I.e. all events would be sent to all active connections.

What if users try both at the same time?

If users try to use both at the exact same time, like downloading a program, it could be problematic. But we can protect against this in Pybricks Code.

@laurensvalk
Copy link
Member

Would it make sense to just have pbsys_stdout_tx for this?

Yes. I would expect this to be like upstream MicroPython's dupterm feature.

Thanks for getting back!

There is no "active" mode from the firmware point of view. I.e. all events would be sent to all active connections.

So am I understanding correctly that what's currently in pbsys/bluetooth will be the agnostic "pbsys/protocol" (there is probably a better name), and its output (messages, including labeled stdout) will be duplicated across the active connections?

And continuing on that, getting BT-RFCOMM-NXT working will be a matter of implementing a driver backend for "pbsys/protocol" to call into, inspired by the nxos/bt driver? This is what I'm currently doing - just curious how this fits in the bigger picture 🙂

@cmarinloopers
Copy link

Hello, thank you for all the work done! 😄

I am trying to send data via USB to my SPIKE Prime.
I have the firmware from the dlech-usb branch, along with the pybricksdev repository also in the dlech-usb branch.

But when I want to send some demo via USB, through the command pipx run pybricksdev run usb shortdemo.py i get the following error:

Traceback (most recent call last):
  File "/Users/cristian/.local/pipx/.cache/40ddf222e5246f9/bin/pybricksdev", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/cristian/.local/pipx/.cache/40ddf222e5246f9/lib/python3.12/site-packages/pybricksdev/cli/__init__.py", line 389, in main
    asyncio.run(subparsers.choices[args.tool].tool.run(args))
  File "/opt/homebrew/Cellar/[email protected]/3.12.4/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/[email protected]/3.12.4/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/homebrew/Cellar/[email protected]/3.12.4/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py", line 687, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/Users/cristian/.local/pipx/.cache/40ddf222e5246f9/lib/python3.12/site-packages/pybricksdev/cli/__init__.py", line 201, in run
    await hub.connect(device_or_address)
  File "/Users/cristian/.local/pipx/.cache/40ddf222e5246f9/lib/python3.12/site-packages/pybricksdev/connections/lego.py", line 86, in connect
    raise OSError("Could not find hub.")
OSError: Could not find hub.

Have I skipped any step? Since I would like to integrate this in the pybricks code builder, to be able to send data to the SPIKE Prime via USB.

Thanks in advance!

@zeus
Copy link

zeus commented Jul 19, 2024

Any updates? Do you plan to push usb programming feature to master branch?

@laurensvalk
Copy link
Member

I'm afraid it probably won't be in the near future. There would be a long way to go to finish this both in the firmware and the editor.

We have limited time and resources, and other aspects currently have priority. Thanks for your understanding.

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

Successfully merging this pull request may close these issues.

6 participants