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

UI: Add MemoryInspector #111

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

AlessioC31
Copy link
Collaborator

Added a new UiTool to read values from memory.

I don't like very much how it is implemented but it was the only way (or at least the only one I found) to make it work since we have Gba<T: Cpu> and Cpu trait didn't have any method to access memory and memory is stored into an Rc<RefCell<>>.

Please tell me if you think it could be implemented in a better way (maybe without returning Ref? But I think it's needed since InternalMemory is managed by a RefCell).

It's very simple because I want to use it to debug an issue we may have in the instruction implementations. In the future, we could show a matrix with all the values and so on but for now I think this does the job.

@gallottino
Copy link
Collaborator

In my refactor in #110 I removed T from GBA and I moved all the hardware outside the cpu so every UI tools can access it easly

@AlessioC31
Copy link
Collaborator Author

In my refactor in #110 I removed T from GBA and I moved all the hardware outside the cpu so every UI tools can access it easly

Yep I saw it! Now we need to decide which one should we merge first :D

I'm okay with keeping this in pause and merge yours first and then I rebase. I can use this branch in any case to debug

@@ -110,6 +111,10 @@ impl Cpu for Arm7tdmi {
fn registers(&self) -> Vec<u32> {
self.registers.to_vec()
}

fn get_memory(&self) -> Ref<'_, InternalMemory> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can return Rc<RefCell<InternalMemory>> instead with self.memory.clone() that returns another usable instance of the Rc. But I don't want to keep it for later, because cpu doesn't need to return the memory, but gba. No problem for now, just a reminder for later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once you merge #111 and I rebase this get_memory fn will probably go away

@AlessioC31 AlessioC31 marked this pull request as draft November 6, 2022 23:41
@AlessioC31
Copy link
Collaborator Author

I converted it to draft while we wait for #111 to be merged

@guerinoni
Copy link
Collaborator

Agreed! We will come back to this after other PR's will be merged!

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.

3 participants