Colby-CPU-Sim / CPUSim

GNU General Public License v3.0
52 stars 18 forks source link

Excise model #112

Closed Nava2 closed 7 years ago

Nava2 commented 7 years ago

Firstly, I am horribly sorry that this took so long to get opened to begin with. Turns out, teaching a course is a lot of work the first time.

Changes

Before Merging

I have been trying to test this as I go, there will be bugs. I really need help in testing this to make sure I didn't break things (I have inevitably done so, I'm sure).

Next Steps

We need more tests, I am so terrified about these changes because I can't just run gradle test and check the changes, but I've added tests when I had time. Going forward, the project needs more unit tests so that things can be verified as working to what the expected cases are.

djskrien commented 7 years ago

I like the changes you made to the form and layout of the dialogs. They look great.

Here are some errors I found when I first tried to use the code in the PR. Am I doing something wrong that is causing all these errors or do you get them too?

  1. [x] I can't load old .cpu files. In particular, the Wombat1.cpu that is included as a sample machine with the CPU Sim distribution cannot be loaded.

  2. [x] I created a new empty machine by starting CPU Sim and immediately tried saving the current new machine, but it threw an exception.

  3. [x] I tried a lot of the menu items with a new empty machine, but encountered the following problems:

    1. [x] I tried to create some registers and rams, but the Edit Modules dialog has the New buttons disabled for all four tabs.
    2. [x] In the Edit Modules dialog, the Register Arrays tab has no names in the buttons--only the symbols, whereas the buttons in the other tabs have names as well as symbols.
    3. [x] In some of the dialogs, such as the Edit Modules dialog, the help ("?") button is disabled.
    4. [x] The Help button is enabled for some dialogs, but when I clicked it, nothing happened.
    5. [x] I opened the Edit Fetch Sequence dialog and dragged the End micro to the implementation pane and it threw a NullPointerException. Same thing with a Comment micro.
    6. [x] In the EQUs dialog, I created a new EQU, then selected it, and then clicked the Duplicate button and it threw an exception.
    7. [x] From the Edit Machine instructions dialog, I opened the Edit Fields dialog and tried to close it by clicking the Cancel button but nothing happened when I did so.
    8. [x] In the Edit Machine Instructions dialog, I was able to create a new instruction and then went to the Implementation tab and added a Comment micro. But I was unable to edit the text in the Comment micro.
    9. [x] I created an empty text file, opened it and chose Assemble from the Execute menu, and an exception was thrown.
    10. [x] When I selected Options... from the Execute menu, an exception was thrown.
    11. [x] No fields are shown in the FieldsList in EditMachineInstructions
    12. [x] Editing the name of a MachineInstruction throws an exception for some reason...
    13. [x] In the Fields dialog, using the arrow keys changes the name of the instruction to the previous one
    14. [x] Drag and drop does not work for fields, could also use more height
  4. [x] Need to add proper converters for the TableViewColumn to show the name, not the toString() for almost all components.. probably just need a ColumnForNamedObjectConvertor or something

That's enough for starters.

Nava2 commented 7 years ago

I made everything checkboxes.

I apologize, I thought I tested it better than that! I think I was micro-testing over macro, I'll be working on these tomorrow.

Nava2 commented 7 years ago

Most of the loading of machines was blind refactoring that I forgot to test and update, working on that now. The others will require some more debugging, I'm adding the two CPUs to the test resources and writing some unit tests to check this behaviour. No need to have to retest by hand later! 😨

Nava2 commented 7 years ago

I've been working on fixing the reader and am adding some "legacy switches" so we can change it later without breaking previous reads (it'd write in the newer format hopefully). Few things:

  1. The files aren't consistent in their format between the emailed files and the ones in the repository
  2. I found test1.cpu and Wombat2.cpu in the repository, is one newer than the other?
  3. Arithmetic operations have two names for source1 as lhs and source2 as rhs, the two you sent me via email use sourceX and the two in the repo use lhs/rhs. I think sourceX makes more sense, but which one would you consider "correct" for legacy?
  4. The JVM2.cpu file has no fields for the contents, it just says <FieldLength length="8" /> for example, I feel like this file may be older than the most recent version.

If I had to wager, I think the Wombat1.cpu you sent me is the "correct" of the group as it is the one shipped with the v4.0.9 download and is identical (minus some line endings 😄) to the one you sent me.

djskrien commented 7 years ago

You said "Arithmetic operations have two names for source1 as lhs and source2 as rhs, the two you sent me via email use sourceX and the two in the repo use lhs/rhs. I think sourceX makes more sense, but which one would you consider 'correct' for legacy?" I couldn't find "lhs" or "rhs" in the test1.cpu and Wombat2.cpu in the repo. They contain sourceX like the ones I sent you. "source1" and "source2" are the only correct names for the attributes.

There are a few backwards compatibility issues that the MachineReader needs to deal with but the main one concerns the fields. See the MachineReader's startMachineInstruction() and endMachineInstruction() methods for details.

Nava2 commented 7 years ago

I couldn't find "lhs" or "rhs" in the test1.cpu and Wombat2.cpu in the repo. They contain sourceX like the ones I sent you. "source1" and "source2" are the only correct names for the attributes.

Maybe it was lost in translation, I'll go searching some more!

There are a few backwards compatibility issues that the MachineReader needs to deal with but the main one concerns the fields. See the MachineReader's startMachineInstruction() and endMachineInstruction() methods for details.

The Field tag works okay, it seems that the JVM2.cpu doesn't define any fields.

From Wombat1.cpu:

<!--......... machine instruction fields ............-->
    <Field name="addr" type="required" numBits="12" relativity="absolute" signed="false" defaultValue="0" id="model.Field14ee11fc">
    </Field>
    <Field name="unused" type="ignored" numBits="12" relativity="absolute" signed="true" defaultValue="0" id="model.Field7e3f8810">
    </Field>
    <Field name="op" type="required" numBits="4" relativity="absolute" signed="false" defaultValue="0" id="model.Field246a65ca">
    </Field>

In JVM2.cpu there are no <Field> tags, just <FieldLength> and none are referenced. Could the file be older than you thought? I found some commented material in the MachineReader regarding an old and now non-existent FieldLength element.

In the repository I found: these. I have the Wombat1.cpu working fully (I think!) and the test1.cpu almost works, I need to fix a bug I introduced in Comparable usage in List properties (I had no idea it was used...).

djskrien commented 7 years ago

On Jan 25, 2017, at 6:06 PM, Kevin Brightwell notifications@github.com wrote:

The Field tag works okay, it seems that the JVM2.cpu doesn't define any fields. ... In JVM2.cpu there are no tags, just and none are referenced. Could the file be older than you thought? I found some commented material in the MachineReader regarding an old and now non-existent FieldLength element.

The JVM2.cpu file is properly loaded, as I said, by CPU Sim 4.0.9. The MachineReader accesses the FieldLengths using the startFieldLength() method. As it reads more and more field lengths, it builds the format for the instruction by appending the lengths in a string. Then, in the endMachineInstruction() method, the format string is used to construct the desired fields and add them to the machine instruction.

In the repository I found: these. I have the Wombat1.cpu working fully (I think!) and the test1.cpu almost works, I need to fix a bug I introduced in Comparable usage in List properties (I had no idea it was used…).

Yes, those are the only two .cpu files in the test resources right now. I have several more .cpu files to use to test reading and writing. I’ll add them to the repository.

Dale

Nava2 commented 7 years ago

The JVM2.cpu file is properly loaded, as I said, by CPU Sim 4.0.9. The MachineReader accesses the FieldLengths using the startFieldLength() method. As it reads more and more field lengths, it builds the format for the instruction by appending the lengths in a string. Then, in the endMachineInstruction() method, the format string is used to construct the desired fields and add them to the machine instruction.

I'll check that, I didn't think to even look how it was being loaded. I'll work on getting that working. Progress has been made though.

I do want to replace the DTD with an XSD to allow for varying order of elements, the current DTD doesn't let the order change which is an unnecessary change.

Thanks for clarifying this stuff! 👍

Nava2 commented 7 years ago

@djskrien I now have them all loading and working, from what I can tell. Everything seems to be loading. Some aesthetics are needed to be fixed, but yeah. I'll start working on the rest of the list.

Nava2 commented 7 years ago

Almost done with these bugs I think. Sorry I've been slow with this, some of them were harder than others, and teaching has eaten a lot of my time.

Nava2 commented 7 years ago

I fixed all of the errors that have been found. I'm terribly sorry it took so long!

This is what is shown on the empty assembly now: image

Nava2 commented 7 years ago

I've closed this as we are doing large changes over the summer. There are many things we are doing to get peripherals added to the model and expose them.