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.

Sign up for GitHub

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

Jump to bottom

Use of launch and attach requests is confusing #7

Closed
Tiwalun opened this issue Jun 22, 2021 · 3 comments · Fixed by #12
Closed

Use of launch and attach requests is confusing #7

Tiwalun opened this issue Jun 22, 2021 · 3 comments · Fixed by #12
Assignees
@noppej

Comments

@Tiwalun
Copy link
Member

Tiwalun commented Jun 22, 2021

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
Copy link
Contributor

noppej commented Jun 22, 2021

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.

  • 'launch' does exactly that. VSCode launches probe-rs-debugger, and optionally flashes and optionally resets the probe, before it opens a probe-rs connection to the core.
  • 'attach' requires there to be an existing probe-rs-debugger process running, but just like 'launch', the flashing and resetting actions are optional.
  • The flaw in the attach, is that currently the implicit detach when you end a VSCode debug session will end the probe-rs-debugger process. I can easily fix this, but that requires the user then to take responsibility for ending the probe-rs-debugger session when they are done with it. That is similar to how gdb/openocd for instance would work, so I am OK with it. Either a Control-C/kill signal to the probe-rs-debugger, or a disconnect of the probe will end the process.

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
Copy link
Member Author

Tiwalun commented Jun 22, 2021 

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
Copy link
Contributor

noppej commented Jun 23, 2021

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

@noppej noppej self-assigned this Jun 23, 2021
@noppej noppej mentioned this issue Oct 14, 2021
Merged
@noppej noppej linked a pull request Oct 14, 2021 that will close this issue
Refactor how launch and attach requests work. #12
Merged
bors bot added a commit that referenced this issue Oct 27, 2021
@bors @noppej
Merge #12
b8b797f 
12: Refactor how `launch` and `attach` requests work. r=noppej a=noppej

This PR was motivated by discussions in #7, and will require rework of the equivalent functionality in [probe-rs-debugger](https://github.com/probe-rs/probe-rs/tree/master/debugger). It will involve a big code change in both repo's.

In the first iterations of this extension, the `launch` and `attach` terminology was interpreted to apply to the debug adapter process. In other words, "launch a new instance of the debug adapter", or "attach to a to an existing debug adapter process". - Both operations allowed flashing and resetting of the target process running on the probe.
- Neither operations allowed the debug adapter session to last for more than one session of the target process on the probe.

The proposed change will reinterpret the `launch` and `attach` terminology to apply to the target process running on the probe. To help clarify the different behaviours, I will use the following terminology:
- _debug session_: This refers to host side process, and ...
  - Starts when a user initiates either a `launch` or `attach` request, and ...
  - Ends when the user either initiates `disconnect` request, or chooses to `Stop` a host side debug session. 
- _target session_:  This refers to the target side process, and ...
  - Starts when the target device boots and executes the firmware program, and ...
  - Ends when the target device processing halts as a result of a unrecoverable exception, or as part of a `Reset` request, or when the device looses power.


### `launch` request type ###

1. Start a _debug session_ :
i. Connect to the target device and executes a `Reset` request so that a new _target session_ is started
ii. Optionally flashes the target device firmware with the binary to be executed

### `attach` request type ###
2. Start a _debug session_ :
i. Connect to the target device, but do NOT issue `Reset, so that the existing _target session_ is uninterrupted

### Common behaviour to both `launch` and `attach` request types ###
3. Support all standard debug behaviours, such as "Pause", "Breakpoints", RTT, etc.
4. The `Reset` request will cause `probe-rs`, to restart the target device, and associated _target session_.
i. This will honour the value of the `halt_after_reset` flag.
ii. The _debug session_ is not affected.
5. The `Disconnect` request in the _debug session_ will ...
i. Instruct `probe-rs` to disconnect from the target device.
ii. This implicitly ends the _debug session_.
iii. The state of the _target session_ is not affected.
iv. There are some subtleties depending on `launch` vs `attach` as described in the [MS DAP Specification for the Disconnect Request](https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Disconnect)
6. The `Stop` function (`Terminate` request) in the _debug session_ will behave the same as the `disconnect`, except ...
i. The _debug session_ will send a `stop` request to `halt` the _target session_ before disconnecting. 
ii. There are some subtleties depending on `launch` vs `attach` as described in the [MS DAP Specification for the Terminate Request](https://microsoft.github.io/debug-adapter-protocol/specification#Requests_Terminate)

### Running `probe-rs-debugger` as a standalone for a VSCode _debug session_ ###
In most cases, the VSCode debug extension will take care of automatically launching (and ending) the `probe-rs-debugger` executable to act as the debug adapter for `probe-rs`.

It is possible for the user to take control of this, by running `probe-rs-debugger` from the command line, with the following command  
  - `probe-rs-debugger debug --dap --port <any availably TCP/IP port number>`

Any VSCode _debug session_ can then attach to the specified <port number> on the server where `probe-rs-debugger` runs, and use the functionality above. 

7. Such a _server debug session_ implies the following:
i. In order for VSCode to know where to find the debug adapter, the user needs to add the appropriate TCP host IP address and port number to the `launch.json` configuration, by adding the option `"server":"<host address>:<port number>"` _(e.g. `"server":"127.0.0.1:50000"`)_ 
ii. Such a _server debug session_ will put the management (start and stop) of the `probe-rs-debugger` process completely under control of the user.

### Further reading: ###
The above proposal aims to be consistent with the process described in the [VSCode debug docs](https://code.visualstudio.com/docs/editor/debugging)

Co-authored-by: JackN <noppej@hotmail.com>
@bors bors bot closed this as completed in #12 Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@noppej noppej

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor how launch and attach requests work. probe-rs/vscode
2 participants
@Tiwalun @noppej