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

Debugger improvements #28

Merged
merged 29 commits into from
Aug 31, 2018
Merged

Debugger improvements #28

merged 29 commits into from
Aug 31, 2018

Conversation

LukasKalbertodt
Copy link
Owner

@LukasKalbertodt LukasKalbertodt commented Aug 27, 2018

  • Log and ASM views are now scrollable
  • ASM view will cache decoded instructions to show instructions before PC
  • Views are updated only when necessary, which should reduce CPU usage
  • Better structure

CC #18
Closes #26
Closes #27

@LukasKalbertodt LukasKalbertodt requested a review from jovobe August 27, 2018 08:01
@LukasKalbertodt
Copy link
Owner Author

I couldn't keep myself from implementing more stuff. Most importantly:

  • Fix quite a few PPU bugs (I know, this has nothing to do with the debugger, sorry for non-atomic PRs :P)
  • Add box showing (almost) all PPU register (in a second right panel)
  • Reduce CPU usage when debugger is paused
  • You can now view, set and unset breakpoints by clicking in the ASM view

But I'm gonna stop now. So this PR is ready.

(43..144, _) => {
// TODO: H-Blank
// debug!("[ppu] hblank. current_line: {}", self.current_line);
(43..144, col) if self.phase() == Phase::HBlank && col == 160 => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use SCREEN_WIDTH instead of magic number 160.

@@ -290,6 +357,13 @@ impl Ppu {
while self.fifo.len() > 8 && pixel_pushed < 4 {
self.push_pixel(display);
pixel_pushed += 1;

// We we are at the end of the line, stop everything and go to
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "we" cloned itself 😱


// We we are at the end of the line, stop everything and go to
// H-Blank.
if self.current_column == 160 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use SCREEN_WIDTH instead of magic number 160.

This reduces the time required for `NativeWindow::update()` from around
1,5ms to 200µs when the debugger is paused
Now pausing and doing nothing in the debugger needs almost no
CPU. More importantly: since the TUI is only updated every fifth
frame, the CPU usage is also lower when the emulator is running.
Now we don't use `TextView` internally anymore and do everything
ourselves. This has a significant speed advantage, but means that
long lines aren't wrapping anymore. The log message can still use
line breaks, though.
@LukasKalbertodt
Copy link
Owner Author

I, again, worked on this instead of doing other stuff 😩

image

I guess these generated comments could come handy.

Other changes include log level via CLI and performance improvements.

These comments can potentially show more information if we want to.
There might be ways to make the debugger compile on Windows, but
I don't think it's worth the trouble.
info!("[ppu] LCD was enabled");
self.set_phase(Phase::OamSearch);
self.cycle_in_line = 0;
// TODO: also reset other stuff?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to reset the current line here too? (e.g. self.current_line = Byte::new(0);)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think so. The current line is set to 0 a few lines below, when the LCD is disabled. I actually think that most things should be "reset" when the LCD is disabled instead of when it is enabled.

I think the TODO is a bit outdated. But it might still be useful until we figured this out.

@jovobe jovobe merged commit c03a049 into master Aug 31, 2018
@jovobe
Copy link
Collaborator

jovobe commented Aug 31, 2018

I ignored everything in desktop/src/debug for the review.

@LukasKalbertodt LukasKalbertodt deleted the debugger branch September 2, 2018 17:43
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