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

Provide a way for DA's to control page size of readMemory #322

Closed
connor4312 opened this issue Aug 31, 2022 · 8 comments · Fixed by microsoft/vscode-hexeditor#390
Closed
Assignees
Labels
*out-of-scope Posted issue is not in scope of VS Code

Comments

@connor4312
Copy link
Member

Feedback from microsoft/vscode#155597 (comment)

Currently DAP does not specify a page size for readMemory, nor provide a way for DA's to request one. VS Code requests 128KB simply because we can quickly and easily display that amount of data in the editor, but it sounds like that may be too big for some devices.

I think the right place for this is in some initialize-type request. It's not a "capability"--should we add a new parameter to the direct return arguments of initialize, such as memoryPreferredPageSize?: uint?

@roblourens
Copy link
Member

Not knowing much about this, is this a property of the debug adapter, or more the device/runtime/something else (which the DA wouldn't yet know about in initialize)?

We could also just have a better way for DAs to return fewer bytes than requested from readMemory.

@haneefdm
Copy link
Contributor

I contend that the HexEditor should never need to read 128KB at all. This is not good client behavior, IMO. For memory views, the page size should be very small, regardless of where the program is running. Easily fixed in the Hex Editor (I found the line of code where this is done).

I think 128KB is okay for actual file systems even networked ones. Could be smaller but, l would leave it alone.

Let us say, we did a memoryPreferredPageSize that the DA can tell you. How would a DA like cppdbg/cortex-debug/MIEngine know what that would be? We have not started a debug session yet, we don't know what kind of a target we have (a tiny microcontroller or desktop/server CPU, a real OS, remote connection with unknown speed). Most micro-controllers, IOT devices top out at 1Mbit transfer speed.

When initialize is called, we haven't started a debugger or a debug-server (gdb-server) yet. All that does not happen until a launch/attach request comes in. Sure, you can do a capabilities update...

My opinion, clients should not ask for too much data at a time. There is really no tangible benefit to that as this is not a streaming interface. Things get bottled up throughout the software stack. VSCode->HexEdit->cppdbg->gdb-server .... At each stage, entire memory blob has to be collected and pushed through.

If such a memoryPreferredPageSize is needed, then cortex-debug will probably just say 4KB blindly :-)

If we make a large request to GDB, then it will stall until it reads/returns the whole thing and you cannot step/continue for a while. GDB does sometimes break up large reads but does not return until the full packet is assembled.

@weinand weinand added the feature-request Request for new features or functionality label Sep 1, 2022
@noppej
Copy link

noppej commented Nov 23, 2022

I would like to +1 this request from @haneefdm. probe-rs has implemented a RUST based DAP debugger and VSCode extension for embedded devices. Attempting to read 128K of data from an embedded device through a USB based probe, results in an unusable user experience (it can take minutes to complete).

See probe-rs/probe-rs#1275 for tracking of this issue on our side.

connor4312 added a commit to microsoft/vscode-hexeditor that referenced this issue Nov 23, 2022
Effectively resolves microsoft/debug-adapter-protocol#322

It seems the default of 128KB is too much for many embedded DA's to be able
to serve in a satisfactory way.

Also, it seems that paging debug data wasn't working before. Whoops.
Fixed that as well.
@connor4312
Copy link
Member Author

Thanks for the feedback, I put in a PR to the hex editor to change the page size used for debug memory to 4KB: microsoft/vscode-hexeditor#390

connor4312 added a commit to microsoft/vscode-hexeditor that referenced this issue Nov 23, 2022
Effectively resolves microsoft/debug-adapter-protocol#322

It seems the default of 128KB is too much for many embedded DA's to be able
to serve in a satisfactory way.

Also, it seems that paging debug data wasn't working before. Whoops.
Fixed that as well.
@connor4312 connor4312 added *out-of-scope Posted issue is not in scope of VS Code and removed feature-request Request for new features or functionality labels Nov 24, 2022
@noppej
Copy link

noppej commented Nov 24, 2022

Thanks for the feedback, I put in a PR to the hex editor to change the page size used for debug memory to 4KB: microsoft/vscode-hexeditor#390

Thank you @connor4312 !!

@noppej
Copy link

noppej commented Dec 31, 2022

@connor4312 ... Do you have a schedule / ETA for when this fix will be released?

@connor4312
Copy link
Member Author

I'm planning a hex editor release next week

@noppej
Copy link

noppej commented Jan 11, 2023

@connor4312 ... just pinging for an updated ETA if you have one please :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
*out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants