eclipse-cdt-cloud / vscode-memory-inspector

vscode memory inspector
https://open-vsx.org/extension/eclipse-cdt/memory-inspector
Eclipse Public License 2.0
6 stars 10 forks source link

Support storing and applying of memory content #96

Closed martin-fleck-at closed 5 months ago

martin-fleck-at commented 6 months ago

What it does

Add commands to store and apply memory content as Intel HEX file

Use quick inputs to guide user through necessary input

Communicate with webview through messenger requests and notifications

Minor improvements

https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/assets/19170971/9c3e2772-cfd5-4b7c-8d45-e2707338d0cb

Closes #50

How to test

Review checklist

Reminder for reviewers

martin-fleck-at commented 6 months ago

@colin-grant-work Thank you for your detailed feedback! I tried to address everything in a follow up commit (after a rebase) and added another commit on top based on the feedback from Jens. Besides the type placement, I think I have addressed everything.

jreineckearm commented 6 months ago

@martin-fleck-at , re-tested functionality.

Did the ultimate test with storing memory from one of our existing debug tools to a hex file. And applying it with the Memory Inspector. And vice versa. Looks all healthy! :-)

The recent change to connect a memory window instance to a debug session ID has the unfortunate side effect of not being able to reuse a memory window anymore. This was possible, would be good to get that back. Use case: I debug something and use the Memory Inspector, I make a code change, then restart debug, and would like to inspect related changes in the previously opened window instance.

Some minor suggestions inline.

One bigger change that may be required and is possibly out of scope for this one is to honor MS-DAP memory events. Our debug adapter correctly sends them on memory contents change. But I see that the Memory Inspector debug tracker doesn't make use of them to trigger a refresh if in the loaded memory range of a window instance. At the moment you need to manually trigger a refresh after loading a hex file into memory.

martin-fleck-at commented 6 months ago

@jreineckearm Thank you very much for your feedback! I rebased the change and added the new functionality on top: The memory inspector should now properly refresh automatically and we also support the memory event as requested here: https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/105.

Furthermore, I added the missing context menu entries for https://github.com/eclipse-cdt-cloud/vscode-memory-inspector/issues/51.

Could you please have another look at it?

colin-grant-work commented 6 months ago

Quite a few changes since my last look - I'll read things over later today.

martin-fleck-at commented 6 months ago

@colin-grant-work Thank you very much! I added them as separate commits so it shouldn't be too bad, I hope! I'll do a quick rebase then.

martin-fleck-at commented 6 months ago

@colin-grant-work @jreineckearm Everything should be good to review again ;-)

jreineckearm commented 6 months ago

Thanks, @martin-fleck-at !

From a functionality point of view, things work as outlined in your last comment. This is a really nice new feature!

Minor improvement for later (not this PR)

martin-fleck-at commented 5 months ago

@colin-grant-work @jreineckearm I pushed an update that should address the latest concerns.

martin-fleck-at commented 5 months ago

@colin-grant-work Thank you very much! I'll push an update for the decimal address support in a few minutes!

martin-fleck-at commented 5 months ago

@colin-grant-work If you agree with the latest commit, please feel free to merge this PR.