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

Proper SWO support for the Black Magic Probe #1052

Merged
merged 3 commits into from
Jan 12, 2025

Conversation

ssimek
Copy link
Contributor

@ssimek ssimek commented Oct 12, 2024

Contains the following changes

  • slight modification of the deployment model so the binary dependencies are included directly in the package, the same way they are in e.g. arm.device-manager extension
  • something must have changed in the meantime with Electron, because the original prebuilt binaries seem to work on all platforms without recompilation
    • tested on macOS, Win and Linux (ARM64) - everything works except SerialPort on Windows ARM64, but that wouldn't work with the old approach just the same
  • a new UsbSWOSource has been added that reads a raw bulk endpoint
    • it autodetects the probe based on productName and interfaceName, both can be overridden

@haneefdm
Copy link
Collaborator

haneefdm commented Jan 12, 2025

Question: Did you just package the serial port binary extension with this extension?

If so, I cannot accept that. We used to do that but last year we removed it because of numerous compatibility issues with Node/Electron and VSCode updates. We had to keep updating our extension constantly when VSCode updated and we also had to keep multiple versions of the binary serial port package.

This is a lot of constant work to keep releasing new versions even when nothing changed in our code or in the serial port code.

Instead, we relied on Microsoft's serial port facility. We should probably make that a requirement. I don't have the resources Microsoft/ARM does.

https://marketplace.visualstudio.com/items?itemName=ms-vscode.vscode-serial-monitor

@ssimek
Copy link
Contributor Author

ssimek commented Jan 12, 2025

I did, and I have also extensively tested it and it seems to work reliably. I took inspiration from other extensions that do exactly the same (e.g. ARM Device Manager, or the Microsoft one you used to "borrow" the serial package from). I presume something must have changed in the way Electron handles native dependencies because I also found a lot of horror stories on this topic, they just don't seem to be (entirely) true at this point.

@haneefdm
Copy link
Collaborator

Okay, but not sure you know what happens with every release of VSCode. Testing with one version does not suffice. We have to rebuild and re-release for all platforms every time VSCode updates Electron. Happens several times a year.

@ssimek
Copy link
Contributor Author

ssimek commented Jan 12, 2025

True and I don't insist on putting this in. I'm using my version since October with no ill effects even after three VS minor updates. I'm quite happy waiting for a few more months.
Also really glad to see you guys back! Funny thing actually - I started working on publishing my fork literally an hour ago or so.

@haneefdm
Copy link
Collaborator

The trouble happens when VSCode switches verion of Electron. All binary extensions break and that the nature of Electron. Not every update of VSCode updates their Electron/Node dependency. That is why it is random and unpredictable. I hate to break other peoples workflow because of my extension.

We should focus on supporting SWO first. Bundling the serial port should be a separate (and independent) topic from SWO support.

@ssimek
Copy link
Contributor Author

ssimek commented Jan 12, 2025

I respectfully disagree. While you technically can keep the hack in for serial port, it would be untenable for libusb (I spent an unhealthy amount of time trying to make it work only to find out it really seems to be possible to include unmodified native extensions). Also, I'm not an expert in native node dependencies, but I believe something has to have changed in the meantime - both libusb and serialport use NAPI which claims to be stable and version independent.

If we just want SWO on BMP, I can carve out the simplified serialport loading, but it will be weird having one native dependency loaded natively and the other borrowed from another extension.

@ssimek ssimek force-pushed the feat/bmp-usb-swo branch 2 times, most recently from 1ba7db3 to 29a004a Compare January 12, 2025 19:53
- halt after reset
- configure Manchester when using SWO from probe
- includes slight refactoring of the GDB SWO support helpers
@haneefdm
Copy link
Collaborator

haneefdm commented Jan 12, 2025

The N-API is good news. I will look into it. I was also getting warnings from the marketplace that my extension is too large and that I should split it up. This was when we were releasing 2-3 versions of binary serial port packages for three OSes.

How big is the packaged extension with serial-port bundled?

What is your objection to splitting up the SWO support from binary package bundling?

I would like to clone your repo and see what you did. Did you use/modify the existing script to build the packages?

@haneefdm
Copy link
Collaborator

Thanks for refactoring the gdb init scripts

@haneefdm
Copy link
Collaborator

haneefdm commented Jan 12, 2025

I am looking at your changes. Oh, I didn't understand what you did. Earlier, we had to compile and package the serial port module ourselves manually. I used to spend 2-3 days doing this several times a year. Now, you are using it as a normal dependency via package.json?

@ssimek
Copy link
Contributor Author

ssimek commented Jan 12, 2025

All I did is in this PR :) See the change in the vscode:prepublish npm task, it just runs npm ci in the binary_modules and then copies the installed node_modules to dist

The final VSIX is about 4.5 MiB, which is reasonable given it now contains both Serial and USB libs

I guess you can use one from this PR for testing :) https://github.com/Marus/cortex-debug/actions/runs/12737126756
Or one from my fork, which also includes ST-Link SWO support (that would be a follow-up PR): https://github.com/minuteos/cortex-debug/actions/runs/12737157776

@haneefdm
Copy link
Collaborator

Okay, ELI5 to me. If all the dist has is just javascript file, why do we need to build and put it in the dist directory?

@ssimek
Copy link
Contributor Author

ssimek commented Jan 12, 2025

Not just javascript, it gets the entire node_modules for the dependencies containing native components. They also have to be excluded from webpacking (externals in webpack.config.js).

The entire output tree looks like this (the sizes are misreported as bytes instead of KB for some reason).

[4.5K]  .
└── [4.4K]  extension
    ├── [2.5K]  dist
    │   └── [2.2K]  node_modules
    │       ├── [ 512]  @serialport
    │       ├── [  96]  @types
    │       ├── [ 192]  debug
    │       ├── [ 192]  ms
    │       ├── [ 288]  node-gyp-build
    │       ├── [ 192]  serialport
    │       └── [ 480]  usb
    ├── [ 416]  images
    ├── [ 448]  node_modules
    │   └── [ 352]  @vscode
    │       └── [ 256]  webview-ui-toolkit
    ├── [ 160]  resources
    ├── [ 192]  support
    └── [ 128]  syntaxes

As you can see, what's new is the dist/node_modules folder, containing untouched versions downloaded via npm. This is very similar to what the original patch was doing - pointing to a very similar node_modules folder inside another extension.

@haneefdm
Copy link
Collaborator

Also really glad to see you guys back! Funny thing actually - I started working on publishing my fork literally an hour ago or so.

Coincidental. Btw, there are no 'guys'. Just me. I am monitoring PRs and Issues and addressing them when I can. There are several changes sitting on my laptop that I haven't merged yet. Some of it had to do with changes in VSCode that we can take advantage of.

@haneefdm haneefdm merged commit 767476a into Marus:master Jan 12, 2025
3 checks passed
@ssimek
Copy link
Contributor Author

ssimek commented Jan 13, 2025

Coincidental. Btw, there are no 'guys'. Just me. I am monitoring PRs and Issues and addressing them when I can. There are several changes sitting on my laptop that I haven't merged yet. Some of it had to do with changes in VSCode that we can take advantage of.

Oh. I thought Marcel is still around, based on #1026 (I tried to reach out to you both there some time ago to see if I can help). That said, is there any way I can help?

@ssimek ssimek deleted the feat/bmp-usb-swo branch January 13, 2025 12:55
@haneefdm
Copy link
Collaborator

That said, is there any way I can help?

My biggest time suck is reviewing issues and responding to users. Most of them are not bugs but a bad gdb or a very old openocd. Mostly from Linux users. Many don't read our documentation (Wiki) or understand what all you can do with launch configs. Any help here is very much appreciated

The other place is to support modules that you added. BMP is a good example. I don't use it and I never touched that module. I don't have a HW setup for it either.

If you are up to it, I can assign issues for you to fix/enhance. You can chime into an issue and offer help. We have to be very careful with our changes as there are a million downloads.

@PhilippHaefele is the only person who helps me.

@ssimek
Copy link
Contributor Author

ssimek commented Jan 15, 2025

Sounds great, feels like I can actually fill a complementary space - I'm mostly on macOS, using all kinds of probes except OpenOCD (had terrible experience with it about 10-15 years ago performance-wise, leading to me writing my own tool for driving SWD/SWO over FTDI, not sure what the situation is now, maybe I'll look into again).

I've been using and loved cortex-debug since its beginnings, but I've been away from microcontroller development for several years now. I'm planning to get back to it and invest quite a bit of time this year. A lot of what I'd like to work on is motivated by "selfish" reasons (adding missing functionality I personally need/want), but I can certainly help with existing issues and communicate with other users.

@daantimmer
Copy link

I've got nothing of value to contribute besides a big thank you for spending your own time on this project.

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.

3 participants