probe-rs / vscode

VSCode debug extension for probe-rs. It uses the MS DAP protocol to communicate directly with the probe (via probe-rs), and supports basic command line debugging in addition to VSCode UI.
https://probe.rs/
Other
65 stars 6 forks source link

Use of `launch` and `attach` requests is confusing #7

Closed Tiwalun closed 2 years ago

Tiwalun commented 3 years ago

I think the current usage of launch and attach in the debugger is confusing, because it's different from the normal usage of these request types.

From what I understand, the probe-rs debugger currently uses launch and attach in relation to the debugger itself, i.e. launch starts a new debugger process and attach attaches to an existing debugger process.

However, usually launch and attach are used in relation to the program being debugged. Launch is used when the debugger starts a new instance of the program being debugged, attach is used to attach to an already running program. See also the VS Code Documentation.

Could we change the usage in the probe-rs debugger to be the same? So attach would not reset or flash anything, but just attach. And launch would be used to reset the target, optionally flash the binary, and then debug it out of reset.

What do you think?

noppej commented 3 years ago

Valid point. I was aware of the 'attach' shortcoming when I built it, but at the time was more focussed on implementing the ability to resolve variable values during debug stepping. Now is probably a good time to discuss this, especially since the multicore PR is going to impact how sessions start and end in the debugger.

So, before I open a PR, I'd like to clarify the understanding of current behaviour first.

So here is what I propose for addressing your concern. Let me know what you think ... 1) Leave launch as is for now. 2) For multicore, we need to discuss if we want a multi-threaded launch/attach or multiple probe-rs-debugger processes. (I recommend multi-threaded in a single process, because it will allow for better managing of how cores run/halt during debugging ... IMHO) 3) Fix the attach, so that it doesn't disconnect when the VSCode debug session ends. 4) Leave the optional flashing/reset functionality for attach. I would like to leave this in place, because it allows full functionality when the probe-rs-debugger process is running on a different server than the VSCode debug session. I know this is a deviation from the VSCode docs you quote above, but their example doesn't really include embedded workflows, and other embedded debugging tools I've used in the past have allowed the use of a remote attach with flashing and reset if the use case desired.

Thoughts?

Tiwalun commented 3 years ago
4\. Leave the optional flashing/reset functionality for `attach`. I would like to leave this in place, because it allows full functionality when the `probe-rs-debugger` process is running on a different server than the VSCode debug session. I know this is a deviation from the VSCode docs you quote above, but their example doesn't really include embedded workflows, and other embedded debugging tools I've used in the past have allowed the use of a remote `attach` with flashing and reset if the use case desired.

I agree that the ability to attach to an already running debugger process is very useful, and I don't think it should be removed. However, I think this should not depend on the request type. My suggestion would be to use an additional configuration flag for this, so the user can decide if they want this ability or not. From my experience with other debuggers, it's quite common to have this kind of config option.

I don't have a strong opinion in regards to multicore, I would prefer whatever is easier to implement ;)

noppej commented 3 years ago

I'm aligned with this. I will PR this as soon as @Yatekii and I release the first iteration of RTT integration.