Collaboration?

  • Topic Archived
You're browsing the GameFAQs Message Boards as a guest. Sign Up for free (or Log In if you already have an account) to be able to post messages, change how messages are displayed, and view media in posts.

User Info: Cheezmeister

Cheezmeister
1 year ago#111
I <3 this thread. Where's the repo?
I make games!
http://luchenlabs.com

User Info: ReconditePhreak

ReconditePhreak
1 year ago#112
https://github.com/mreiland/trustiNES

come join the fun! :)
Believes the individuals who report to moderators wish they had more control than they do.

User Info: Cheezmeister

Cheezmeister
1 year ago#113
Tests all pass on OSX ;)
I make games!
http://luchenlabs.com

User Info: ReconditePhreak

ReconditePhreak
1 year ago#114
good to hear :)

I'll be a little slow for the next week, I've gotten a surprise project in my lap and it pays money, so... :)
Believes the individuals who report to moderators wish they had more control than they do.

User Info: Sinroth

Sinroth
1 year ago#115
I started implementing cpu_executor.execute, but I have a question. Since we're doing this by constructing a new cpu_state which represents the state of the cpu after executing an instruction, shouldn't cpu_executor.execute return the new cpu_state? Or will we do it by updating the reference passed in to point to a new cpu_state? I personally consider the former more elegant (though probably less efficient), as we can treat execution as a pipeline and shove extra things into the pipeline for logging/recording/etc. should those things wish to operate on the old and new cpu_states per step.

In cloning cpu_state you have to clone instruction_register and decode_register but it seems kind of silly to do that since these just hold info about what instruction is to be executed, and that's not relevant to the new cpu_state created after execution. We could change those to be Option<u8> and Option<DecodeRegister>, then implement our own clone which will copy all the old cpu_state over to the new cpu_state, except for those two members which will just be set to None.

Thoughts? These are just the questions that come to mind as I play around with it, I'm still a chump at Rust :)
I live in a big house and it's handy to have a pair of running shoes so that it doesn't take me forever to get from one area of the house to another.

User Info: SeraphLance

SeraphLance
1 year ago#116
I was going to do a thing where cpu_executor.execute returned a new state and saved the previous state into a ring buffer, but I've failed (yet again) to do what I said I was going to do.
Hello. My name is Inigo Montoya. You killed my father. Prepare to die.

User Info: Sinroth

Sinroth
1 year ago#117
I wanted to work on this at uni but they use some old version of Rust and I don't have any permissions

After finally finding some sketchy binary for Cargo it seems it won't let me compile because of the use of unstable library features (for the version of rustc on the computers)

And the sysadmins are kind of dicks so I don't want to ask them for help D:
I live in a big house and it's handy to have a pair of running shoes so that it doesn't take me forever to get from one area of the house to another.

User Info: Sinroth

Sinroth
1 year ago#118
Ok sweet I implemented a few instructions: ASL, LDA, LDX, LDY, LSR, and NOP. I'll probably let ReconditePhreak review those changes before hacking on it anymore.
I live in a big house and it's handy to have a pair of running shoes so that it doesn't take me forever to get from one area of the house to another.

User Info: ReconditePhreak

ReconditePhreak
1 year ago#119
To answer your question, we need to be able to pass the state changes to both memory and cpu_state, so for now we're just doing a mutable borrow through the parameter list.

That said, if you want to do it differently hop in :) value semantics would definitely be more understandable for those new to the code, the important thing is that changes to memory also get propogated.

And I believe you're right about the cloning with 1 caveat: I wouldn't call it clone. I believe the expected semantics for clone() are that of a deep or shallow clone and that operation would be neither. I'm not sure what a good name for it would be, but I think clone() would give people the wrong assumption about the semantics.

On that note, we'll want the clone operation to be outside of the execute function. The reason is twofold: a separation of concerns that leads to cleaner interactions between parts of the code, and clearer semantics. I'll try to explain.

If you look in src/bin.rs you'll see the general loop we have for the cpu (ignore the 10 iterations, that's temporary).


for i in 0..10 {
executor.fetch_and_decode(&mut cpu,&mut mem);
executor.execute(&mut cpu,&mut mem);
}


From an understandability perspective, nothing about that code makes it obvious execute is cloning the cpu_state. And it's actually doubly dangerous because anyone reading that code is going to *think* they understand, which means they may not dive further in to check their understanding. In my opinion, with the current semantics a better name would be "clone_and_execute". If nothing else the reader would go "huh, why are we cloning?" and then start investigating. There are always going to be "assumed semantics" in code but I think in general it's best to try and avoid unexpected semantics, especially when the expected semantics are completely reasonable. And in this case it's completely reasonable to assume execute doesn't clone the cpu state.

A more clear way of implementing this would be:


for i in 0..10 {
executor.fetch_and_decode(&mut cpu,&mut mem);
do_clone(cpu); // pseudocode
executor.execute(&mut cpu,&mut mem);
}


This has the added benefit that the execute function is clearly separated from the history functionality. This could concievably be turned into this.


for i in 0..10 {
executor.fetch_and_decode(&mut cpu,&mut mem);
executor.execute(&mut cpu,&mut mem);
cpu_history.save(&cpu,&mem)
}


And the great thing about this is that there's no clone method at all. Anyone looking at this is going to understand that the cpu state is getting copied every iteration of the cpu. But it also makes the code more flexible as well. Maybe at some point we will decide we want to save the instruction/decode register for debugging purposes. The above loop and the execute function never have to change, the only piece of code worried about the semantics of the save/load of the cpu state is the "cpu_history" object.

Not only that, from a performance perspective it means we don't have to actually copy the cpu state at all for execution. Consider if we had two functions "run_with_history" and "run_without_history" in case the user didn't care about the history at some point, but did care about performance.

So the lines between the different areas of functionality are a lot cleaner, and the semantics tend to be a lot clearer. Becuase we're "sharing state" a bad citizen can still wreak havoc, but the benefits far outweight the drawbacks in my opinion.
Believes the individuals who report to moderators wish they had more control than they do.

User Info: ReconditePhreak

ReconditePhreak
1 year ago#120
I know that was long winded, but I didn't want to simply say "move the clone function" as that wouldn't properly convey the why.

I do have two other suggestions about the commenting.

The comments above the opcode implementations are a bit redundant due to the enumeration name itself. Unless the enumeration name is significantly different from the opcode name documenting it in comments is redundant and prone to getting out of date.

For example, I think the following is just fine for ASL.
// Arithmetic Shift Left

Also I think it's reasonable to assume anyone reading the code either has an understanding of bitwise operations or is willing to read/ask about them so it's not necessary to document the bit operations with so many comments.

Alright, that's all I have :) Despite the wall of text I don't think there's anything majorly wrong with the code, I just tend to get preachy sometimes (sorry about that).

The only thing really keeping us from being able to start executing is that I haven't gotten to finishing the decode implementation. It may be a few days before I'm able to get to it if you want to take a look. In particular, the indexed addressing modes needs to wrap around properly, below pseudocode


ZeroPage,X
addr = (op_val + cpu.x) % u8::max

I'm not suggesting that's the optimal implementation, just trying to describe what I mean by "wrap around properly".

The indexed addressing modes are

ZeroPage, X
ZeroPage, Y
IndexedIndirect <-- 16 bit, I would use 32-bit operations and then wrap/shrink to 16 bite
IndirectIndex <-- 16 bit, I would use 32-bit operations and then wrap/shrink to 16 bite

I think once that's done we'll need to write tests in the testing module to exercise all of the addressing modes. It'll be a lot easier to test that code by itself so when we start executing and something is wonky, we can be reasonably assured the problem is in the opcode implementation and not the addressing mode lookup.
Believes the individuals who report to moderators wish they had more control than they do.

Report Message

Terms of Use Violations:

Etiquette Issues:

Notes (optional; required for "Other"):
Add user to Ignore List after reporting

Topic Sticky

You are not allowed to request a sticky.

  • Topic Archived