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

Added "InstructionMemory Dump" #68

Merged
merged 4 commits into from
Jan 23, 2020
Merged

Conversation

jowens
Copy link
Contributor

@jowens jowens commented Dec 19, 2019

Generates a trace, to be stored in a file specified by the user, with one line per datum. The four kinds of data in the trace are:

  • I: The address of an access into instruction memory
  • i: A 32-bit RISC-V instruction (the trace first dumps the address then the instruction)
  • L: The address of a memory load into data memory
  • S: The address of a memory store into data memory (the contents of the memory load/store aren’t in the trace)

@TheThirdOne
Copy link
Owner

I have two issues with that need to be fixed before this can be merged.

It currently has no explanation of what it does from the tool. A help button with the text from this PR would probably be sufficient, but a little bit more would be better.

There is no indication of success or failure to write out the log. In the event of a file error e.printStackTrace() is called, but no user facing error is given. A text log saying {number of lines} lines successfully written or Failed to write log: {reason} would fix this.

I can fix both of these issue, but it may take a while for me to get around to it.

Also minor spacing issue between the button and input field.

image

@jowens
Copy link
Contributor Author

jowens commented Dec 20, 2019

My Java knowledge is so scant as to make me suggest that even if it takes a while for you to get around to it, it's likely 20 minutes for you and a day and a half for me so I'd prefer you do it to your standards and not rely on my Java skills.

TheThirdOne added a commit that referenced this pull request Jan 23, 2020
For problem 1, the standard code for the help button was used.

For problem 2, a Jlabel was added after the filename input box. It logs
the name of the file is successful and the error message if not.
Unfortunately, it seems it will always reflow onto the next line but not
resize the window making it look like nothing is logged.

For problem 3, changing the layout from a gridbox to a flow did it.
TheThirdOne added a commit that referenced this pull request Jan 23, 2020
@TheThirdOne TheThirdOne merged commit e57be25 into TheThirdOne:master Jan 23, 2020
@TheThirdOne
Copy link
Owner

TheThirdOne commented Jan 23, 2020

You were pretty accurate in your estimation; aside from spending time not understanding why the text was not showing up (because it was reflowing onto the next line that was not visible), fixing the issues and merging it took like 20 minutes.

If you care to learn about what the changes actually entailed, 73dbacd should be pretty clear.

The one outstanding issue on this is the reflowing issue I got hung up on. I would think most users would not realize there is a way to tell if it dumped successfully. There should be some method to call to scale the window to a minimum size that shows everything, but I don't know it off the top of my head. In the interest of getting this merged in rather than keeping you waiting, that will be an issue for another day.

Thanks for the contribution.

@jowens
Copy link
Contributor Author

jowens commented Jan 23, 2020

Thank YOU! I looked at your diff. That would have taken me much longer. I look forward to using the integrated package in future courses!

TheThirdOne added a commit that referenced this pull request Feb 22, 2020
All that was needed is a call to pack. This took far too long to find.

Changing the layout is probably neccessary, but this does the job.

Discussion on #68
@TheThirdOne
Copy link
Owner

The method I was thinking of was window.pack(). However, I couldn't figure out why that wasn't working for a while until I realized I was calling it on this rather than theWindow.

This leaves a somewhat poor UI because it resizes to fit into a single line, but it is at least usable now.

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.

2 participants