pybricks-micropython Add support for programming over USB

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.

Oct 25 '23 07:10 nkarstens

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

Oct 26 '23 02:10 coveralls

Download the artifacts for this pull request:

Oct 26 '23 02:10 github-actions[bot]

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?

Nov 23 '23 15:11 nkarstens

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.

Nov 23 '23 16:11 dlech

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?

Nov 23 '23 21:11 nkarstens

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.

Nov 24 '23 03:11 dlech

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.

Also tested on Linux.

Nov 27 '23 00:11 dlech

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

Nov 27 '23 01:11 dlech

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?

Nov 27 '23 09:11 nkarstens

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.

Nov 27 '23 15:11 dlech

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.

Nov 27 '23 19:11 nkarstens

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

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

Nov 27 '23 19:11 dlech

I squashed your fixup into the latest push and added a co-authored-by tag for you.

I tried making it an interrupt transfer instead of bulk transfer and the transfer speed was significantly lower (42.5Kb/s with interrupt and 237kB/s with bulk transfer). The device is currently operating in full speed mode. I tried making it high speed but I think that is more involved. For now I think it makes sense to leave it using bulk transfer.

Nov 28 '23 06:11 nkarstens

With the latest push I'm able to receive stdout data from the hub. pybricks/pybricksdev#69 has been updated as well, though the monitor_responses method current used could probably be improved.

Nov 28 '23 08:11 nkarstens

This transmits status flags. Between this and the changes to pybricks/pybricksdev#69 something is preventing programs from being loaded. Will investigate more soon. In the meantime we can see status flags in Wireshark.

Dec 01 '23 12:12 nkarstens

Latest patch set improves the architecture of sending status quite a bit (no longer utilizes a separate process).

The only known issue remaining is that if you unplug the USB cable while the device is powered down then the battery LED remains on. There appears to be something preventing the device from powering down.

Dec 21 '23 08:12 nkarstens

Fixed issue with power down. Calling USBD_DeInit() results in HAL_PCD_MspDeInit() being called, which disables the IRQ needed to determine if the VBUS status changes.

Dec 22 '23 13:12 nkarstens

I went ahead an picked up the first few commits (with some changes to the commit messages) so we don't have to keep rebasing/rebuilding those.

Dec 22 '23 22:12 dlech

I went ahead an picked up the first few commits (with some changes to the commit messages) so we don't have to keep rebasing/rebuilding those.

Awesome, thanks!

Dec 23 '23 12:12 nkarstens

I got a few more commits polished up and merged.

Dec 23 '23 23:12 dlech

The latest changes do not seem to be working for me. Two things I've noticed:

Windows now lists the device as "USB Serial Device (COM8)"
The changes in pybricksdev no longer work to send & execute a program

Dec 25 '23 07:12 nkarstens

Windows now lists the device as "USB Serial Device (COM8)"

I'm not seeing this. Maybe this is due to Windows caching device information? We can probably fix this by changing the bcdDevice value which will trigger Windows to re-enumerate the device.

I switched back and forth between the official LEGO firmware and this PR and could not reproduce the problem.

Dec 27 '23 17:12 dlech

2. The changes in pybricksdev no longer work to send & execute a program

Hmm... I'm seeing this too. The only intentional change I made was dropping the zero-terminator from the version strings. After fixing that in pybricksdev, the download and run is sort of working. The program starts but hangs (on printing, I am assuming) and there are no status messages either. So it would seem that something is broken with the IN endpoint. Also, pressing the stop button to stop the program doesn't work and pressing it again cause the hub to reboot, so there is a crash or infinite loop somewhere.

Dec 27 '23 19:12 dlech

After some more experimenting, this feels like a data corruption bug, e.g. there is a buffer overflow out out of bounds array access writing over husbd or possibly something else.

I disabled the status reporting by commenting it out and made the following change:

diff --git a/lib/pbio/drv/usb/usb_stm32.c b/lib/pbio/drv/usb/usb_stm32.c
index 8ed47e5d..300872d5 100644
--- a/lib/pbio/drv/usb/usb_stm32.c
+++ b/lib/pbio/drv/usb/usb_stm32.c
@@ -152,7 +152,9 @@ pbio_error_t pbdrv_usb_stm32_stdout_tx(const uint8_t *data, uint32_t *size) {
     uint8_t *ptr = usb_stdout_buf;
     uint32_t ptr_len = sizeof(usb_stdout_buf);
 
-    // TODO: return PBIO_ERROR_INVALID_OP if not connected
+    if (husbd.dev_state != USBD_STATE_CONFIGURED) {
+        return PBIO_ERROR_INVALID_OP;
+    }
 
     if (usb_stdout_sz) {
         return PBIO_ERROR_AGAIN;

I don't think this is technically correct since it should probably also include USBD_STATE_ADDRESSED, but the main point is that after making this change, my test program ran once correctly (it didn't actually print anything because the device was not in the configured state), but when I run the program again, I get the hang I mentioned before when trying to print from MicroPython. And if I make the functinon always return PBIO_ERROR_INVALID_OP there is no hang even if I run the program multiple times. So clearly the value of husbd.dev_state has changed at this point after the first program run, otherwise it would behave the same as the case when PBIO_ERROR_INVALID_OP is always returned.

They only way dev_state gets set to USBD_STATE_CONFIGURED is if the host computer requests it, but I'm not seeing this in a Wireshark capture of USB traffic, so this leads me to believe the struct is getting written over somehow.

I've been going over all of the changes looking at the memcpys and array accesses and haven't found anything yet though (I found a couple of places where we should probably have hpcd->IN_ep[epnum | 0xF] to be safe, but making these changes didn't fix the issue.)

Test program:

echo 'from pybricks.hubs import PrimeHub; from pybricks.parameters import Color; from pybricks.tools import wait; print("hi"); hub = PrimeHub(); hub.light.on(Color.GREEN); wait(1000)' | pybricksdev run usb -

Dec 27 '23 22:12 dlech

Windows now lists the device as "USB Serial Device (COM8)"

I'm not seeing this. Maybe this is due to Windows caching device information? We can probably fix this by changing the bcdDevice value which will trigger Windows to re-enumerate the device.

I switched back and forth between the official LEGO firmware and this PR and could not reproduce the problem.

I ended up having to delete some registry entries under Computer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Enum\USB for VID/PID values 0694/0008 and 0694/0009, but it appears to show the right device now.

For future reference, this was only possible by using psexec (from SysInternals) to launch regedit.

Dec 28 '23 06:12 nkarstens

2. The changes in pybricksdev no longer work to send & execute a program

@dlech I updated pybricks/pybricksdev#69 to account for the new USB vendor and product IDs and now I'm getting the following message:

`packaging.version.InvalidVersion: Invalid version: '3.3.'

I confirmed with Wireshark that "3.3.0" is being sent, I just think that the current code is not able to read it without the null-terminator.

Dec 28 '23 10:12 nkarstens

the current code is not able to read it without the null-terminator.

@dlech Actually that was pretty easy to fix. I'm able to send a test program using the latest from pybricks/pybricksdev#69.

Dec 28 '23 10:12 nkarstens

@dlech Last change for now. The latest changes includes functionality to fully stop USB when in SHUTDOWN (so that the device is removed from the device list) and periodically poll the VBUS line to determine if the charging cable has been disconnected.

Dec 28 '23 10:12 nkarstens

The latest changes includes functionality to fully stop USB when in SHUTDOWN (so that the device is removed from the device list) and periodically poll the VBUS line to determine if the charging cable has been disconnected.

Would it be simpler to just not disable the interrupt in HAL_PCD_MspDeInit() in platform.c?

Dec 28 '23 16:12 dlech

I ended up having to delete some registry entries under Computer\HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Enum\USB for VID/PID values 0694/0008 and 0694/0009, but it appears to show the right device now.

Interesting. Maybe you had the official LEGO driver for SPIKE Prime installed from the legacy SPIKE application? I wonder if this could have been worked around by uninstalling the driver using Device Manager instead.

Dec 28 '23 16:12 dlech