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 vs code launch.json auto generation in waf configure #28839

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Huibean
Copy link
Member

@Huibean Huibean commented Dec 11, 2024

auto generate vscode launch.json for setup stm32 gdb or SITL lldb debug session in VSCODE
waf configure arg "--vs-launch", only active --debug
example command
./waf configure --board Pixhawk4 --debug --vs-launch
STM32 debug need https://github.com/Marus/cortex-debug and STLINK
image

./waf configure --board sitl --debug --vs-launch
image
image

@Huibean Huibean force-pushed the pr-add-vs-code-launch-auto-generation branch 2 times, most recently from 6d4d3b6 to 1cf121c Compare December 11, 2024 14:51
wscript Outdated
openocd_target = 'stm32g4x.cfg'

with open(openocd_cfg_path, 'w+') as f:
f.write("source [find interface/stlink-v2.cfg]\n")
Copy link
Contributor

@joshanne joshanne Dec 12, 2024

Choose a reason for hiding this comment

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

I don't think this is right - pointing to the stlink-v2 config.

The most of the stlink*.cfg files point to the more common stlink.cfg - eg: https://github.com/openocd-org/openocd/blob/master/tcl/interface/stlink-v2.cfg

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, thanks for the hint, shoule be stlink.cfg, updated

@Huibean Huibean force-pushed the pr-add-vs-code-launch-auto-generation branch from 1cf121c to 709ece0 Compare December 12, 2024 06:11
wscript Outdated
"args": [
"-S",
"--model",
"+",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that this is a valid model for all vehicle types/targets

Copy link
Member Author

Choose a reason for hiding this comment

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

Should accept a extra params to overwirite this, now it's more like a template, the default params I get from the output of sim_vehicle.py

wscript Outdated
mcu_type = cfg.env.get_flat('APJ_BOARD_TYPE')
openocd_target = ''
if mcu_type.startswith("STM32H7"):
openocd_target = 'stm32h7x.cfg'
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/ArduPilot/ardupilot/blob/master/Tools/debug/openocd-h7.cfg

The ardupilot supplied openocd H7 config file points to dual bank.

Not sure if we should also point this to stm32h7x_dual_bank.cfg.

When I've debugged on hardware, I have used the ardupilot config file to talk to the H7.

Copy link
Member Author

Choose a reason for hiding this comment

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

should include the dualbank, I am trying to find the dual bank condition in waf env

@MichelleRos
Copy link
Contributor

MichelleRos commented Dec 12, 2024

Good idea to have launch.json in the code itself rather than making people copy it from the documentation, but I'm wondering whether it wouldn't be simpler to commit a launch.default.json file to the repo? (Like how we have a settings.default.json)

Also, I tested the PR and it generated the launch.json file but when I tried to then launch it, I got this error message:
image

I'm not sure why it is giving me this error, since I've been using vscode with gdb with my previous launch.json file without issues.

@Huibean
Copy link
Member Author

Huibean commented Dec 12, 2024

Good idea to have launch.json in the code itself rather than making people copy it from the documentation, but I'm wondering whether it wouldn't be simpler to commit a launch.default.json file to the repo? (Like how we have a settings.default.json)

Also, I tested the PR and it generated the launch.json file (or, overwrote the one I had which has been working correctly) but when I tried to then launch it, I got this error message: image

for SITL using a launch.default.json seems fine, when switching different board tagerts launch.json would be hard to maintain unless adding board var to settings.json and using the var in launch.json,this could avoid the option list being so long to cover all case, would you mind upload your generated launch.json?

@Huibean Huibean force-pushed the pr-add-vs-code-launch-auto-generation branch from 709ece0 to b5c4f2a Compare December 13, 2024 06:18
@Huibean
Copy link
Member Author

Huibean commented Dec 13, 2024

the above error should coming from using lldb in WSL(linux), code updated, use gdb instead outside macos

@Huibean Huibean closed this Dec 16, 2024
@Huibean Huibean force-pushed the pr-add-vs-code-launch-auto-generation branch from b5c4f2a to a52c375 Compare December 16, 2024 14:18
@Huibean Huibean reopened this Dec 16, 2024
@Huibean Huibean force-pushed the pr-add-vs-code-launch-auto-generation branch from 4f3f9a7 to d217a06 Compare December 16, 2024 14:22
@Huibean
Copy link
Member Author

Huibean commented Dec 16, 2024

2024.12.16 update
thanks @robertlong13 and @rmackay9 sharing launch.json snippets

  1. allow H7 use dual_back cfg in openocd
  2. add allow using launch_profile.json to store perference configs(items in launch_profile.json will overwrite the launch.json)
    cp .vscode/launch_profile.default.json .vscode/launch_profile.json

what's the benefit of using --vs-launch instead of copy template launch.json from document?

  1. fast generating a working launch.json for new users
  2. quick handling build path with board target changes
  3. flexible perference by using launch_profile.json

@Huibean Huibean requested a review from timtuxworth December 16, 2024 14:36
Copy link
Collaborator

@robertlong13 robertlong13 left a comment

Choose a reason for hiding this comment

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

Overall, I like the idea, but I think we need to find a way to make it generate on build instead of on configure. That way, we don't need a launch configuration for every vehicle binary, just one for the last one configured/built. The --vs-launch option should just create/overwrite one launch configuration named "vs-launch" with whatever the last build was. I poked around trying to see if I could get that to work, but I've run out of time, and I don't know waf well enough yet.

I also don't think we should automatically delete the launch.json on a parse error. Throw an error and quit. Make the user fix it or delete it themselves. Since launch.json is gitignored, if we delete it, you'll likely be losing something that isn't backed up anywhere.

@@ -0,0 +1,209 @@
import os
import json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to use something that supports comments. Something like this: https://github.com/Kijewski/pyjson5

Otherwise, you will pretty much always get a parse failure and will delete the user's existing launch.json.

}

if launch_profile.get('sitl'):
merge_dicts(launch_configuration, launch_profile.get('sitl'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
merge_dicts(launch_configuration, launch_profile.get('sitl'))
launch_configuration |= launch_profile.get('sitl')

Don't need to implement a dictionary merger. This is supported in 3.9+ https://peps.python.org/pep-0584/

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks

@Huibean
Copy link
Member Author

Huibean commented Dec 24, 2024

Overall, I like the idea, but I think we need to find a way to make it generate on build instead of on configure. That way, we don't need a launch configuration for every vehicle binary, just one for the last one configured/built. The --vs-launch option should just create/overwrite one launch configuration named "vs-launch" with whatever the last build was. I poked around trying to see if I could get that to work, but I've run out of time, and I don't know waf well enough yet.

I also don't think we should automatically delete the launch.json on a parse error. Throw an error and quit. Make the user fix it or delete it themselves. Since launch.json is gitignored, if we delete it, you'll likely be losing something that isn't backed up anywhere.

my first try is putting it in the waf build, then I thought about not to slow down the build process, I will update it raise a error instead of delete a launch.json with syntax error

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

Successfully merging this pull request may close these issues.

5 participants