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

Allow to write memories within the memory-table #108

Merged

Conversation

haydar-metin
Copy link
Contributor

What it does

Introduce write memory capability within the memory table.

Top: Little Endian
Bottom: Big Endian

Peek 2024-03-14 11-16

Closes #10

How to test

  1. Click on a data group within the table
  2. Provide a different value
  3. submit with enter or blur the element
  4. Changes should be applied

Review checklist

Reminder for reviewers

@haydar-metin haydar-metin self-assigned this Mar 14, 2024
Comment on lines 46 to 50
/**
* Debug adapters can use the 'memory' event to indicate that the contents of a memory range has changed due to some request but do not specify which requests.
* We therefore track explicitly whether the debug adapter actually sends 'memory' events to know whether we should mimic the event ourselves.
*/
protected adapterSupportsMemoryEvent = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

In place of the accounting involved in either this field or the hasClientCapability check (since that doesn't actually stop DA's from emitting the event but only suggests that they shouldn't), something like the pattern in this code might be appropriate. Basically, if we perform a write, we know that an event needs to be emitted: if the adapter emits one in a timely fashion, then that should be the controlling event; if it doesn't (for whatever reason), then we'll emit our own.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented something similar, could you look at it? Thanks!

src/webview/columns/data-column.tsx Outdated Show resolved Hide resolved
src/webview/columns/data-column.tsx Outdated Show resolved Hide resolved
src/webview/columns/data-column.tsx Outdated Show resolved Hide resolved
src/webview/columns/data-column.tsx Outdated Show resolved Hide resolved
protected processData(data: string): string {
// Revert Endianness
if (this.props.options.endianness === Endianness.Little) {
const chunks = data.padStart(this.renderableCharacters, '0').match(/.{1,2}/g) || [];
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the significance of the . and why would 2 appear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just splitting here the whole string into chunks of length 2, so that I can reverse each chunk. (The 1 is not necessary, will be removed)

@jreineckearm
Copy link
Contributor

@colin-grant-work , @planger , thanks for joining today's call where we discussed the memory edit strategy! A summary can be found here: #100 (comment)

We agreed to continue with the approach as implemented in this PR, i.e. to edit values on group level which considers endianess. But I see that @colin-grant-work has already resolved the discussion about this and indicated he is looking into a potential future approach to enhance this. Thanks for bearing with us on this one, @haydar-metin ! :-)

@colin-grant-work
Copy link
Contributor

The styling of the input can push the content onto two lines:

image

Not a fatal flaw, and could be addressed in a follow-up.

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Mar 19, 2024

I have posted a branch that allows both group-level editing (by double click) and sub- or super-group editing by context menu (though it is limited to a single row):

image

image

image

If that's of interest, feel free to use it. The when clause for the item could probably be tightened up.

@colin-grant-work colin-grant-work mentioned this pull request Mar 20, 2024
1 task
@haydar-metin
Copy link
Contributor Author

haydar-metin commented Mar 20, 2024

@colin-grant-work that looks great! I will take a look at it and try it out and depending on which way we will take, I will introduce the fixes you have requested.

@colin-grant-work
Copy link
Contributor

@colin-grant-work that looks great! I will take a look at it and try it out and depending on which way we will take, I will introduce the fixes you have requested.

Based on @jreineckearm's feedback, I'd suggest deleting the pieces that relate to editing by selection - they're fairly discrete - and keeping the parts for editing at group level. Your current code also has an additional check to see whether the user actually changed anything and skips sending the write request if they didn't - that would need to be brought back.

@colin-grant-work
Copy link
Contributor

@jreineckearm, I think I'm struggling a bit to understand the role of endianness in the display.

At the moment, for example, we reverse the content of a group if the display is set to little endian. As you know, I'm a little leery about treating the group as a significant unit, but I guess the logic runs something like this:

  • If the group is a meaningful unit
  • Then on a little-endian system, the bytes are arranged from least to most significant (I assume)
  • But that's the reverse of how we usually read things, so we'll reverse it.

But then, if we take the approach here, I think we un-reverse while editing so that we're inputting things in the order they actually appear in memory (so typing f in the input becomes 0000 000f when we go back to the normal view). I'm not sure that that will make sense to users: we're nice enough to show them the more legible format, but then we ask them to enter things in the harder-to-read format, and then we take what they gave us and reverse it.

I think at a minimum, we should be consistent and let people enter the memory in the same form that they see it before they start editing it. More generally, I'm probably still not very happy with treating the group as a meaningful unit when we haven't established what meaning it should have and reversing some chunks of memory so that addresses occur in a strange order (3, 2, 1, 0, 7, 6, 5, 4, 11, 10, 9, 8, etc., for example) seems a little bit suspect to me. But I'm going to defer that unease to the bigger discussion about trying to represent the significance of the data rather than the form at some point down the road.

@jreineckearm
Copy link
Contributor

The styling of the input can push the content onto two lines:

Agreed. Needs attention but can be done after a first release. Users will be able to figure out quickly that they can widen the column a little to make that artifact go away.

I think we un-reverse while editing so that we're inputting things in the order they actually appear in memory (so typing f in the input becomes 0000 000f when we go back to the normal view).

Phew, you made me nervous for a minute. But things are as I would expect them. :-)

Background for this behavior is the expectation of how to treat a group value while editing. Fetching and ordering the value adheres to the selected endianess. The interpretation is then however based on how you'd treat a human readable variable value, i.e. MSByte/Base is left, LSByte/Base is right. If you want F0000000 then you need to type is as such. If you just type F it is padded to the left with zeroes.

I appreciate that this falls again into the area of the "meaningful unit" discussion which we need to have soon. But testing this with real users could give us answers soon whether it's confusing. TBF, we are keen to get a new release out so that we can start promoting it as part of our tooling at a big trade show soon. I am sure we'll get plenty of feedback then. :-)

I'll set up another call soon to talk the endianess story through in more details if that's OK. But would advocate to solve the remaining code review issues now, and refine the use after a release.

@haydar-metin haydar-metin marked this pull request as draft March 21, 2024 15:22
@haydar-metin haydar-metin marked this pull request as ready for review March 21, 2024 16:51
@haydar-metin
Copy link
Contributor Author

@colin-grant-work @jreineckearm The changes are now the same as #110 without the edit-by-select feature. I didn't touch the The styling of the input can push the content onto two lines issue for now. It happens only for autofit. I would need to check the CSS out again.

@jreineckearm
Copy link
Contributor

jreineckearm commented Mar 21, 2024

@haydar-metin , I've created issue #112 to address the line break in a later PR. It shouldn't hinder us to merge this bigger change here.

@jreineckearm
Copy link
Contributor

I've also created the following new issues to follow-up on open discussions triggered/associated with this change after a first release to gather user feedback:

Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

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

Sorry, @haydar-metin , one of the latest changes broke the little endian handling. When I double-click a value, the bytes/MAUs in the in-place edit swap order (basically turn big endian). Then that gets written back to the target as such. Is endianess probably applied twice now?

@haydar-metin
Copy link
Contributor Author

@jreineckearm I will return to you tomorrow by verifiying the examples done in the GIF. Maybe the correct handling was lost between the branches.

Copy link
Contributor

@jreineckearm jreineckearm left a comment

Choose a reason for hiding this comment

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

Thanks, @haydar-metin ! It is working again as I would have expected it.

@colin-grant-work colin-grant-work merged commit d1c825c into eclipse-cdt-cloud:main Mar 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ability to write memory contents
3 participants