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

Fix: Machine is a God class #393

Merged
merged 25 commits into from
Aug 2, 2023
Merged

Fix: Machine is a God class #393

merged 25 commits into from
Aug 2, 2023

Conversation

maximilien-noal
Copy link
Member

@maximilien-noal maximilien-noal commented Aug 1, 2023

Description of Changes

Removes all the references to the Machine in the Emulator core, reverse engineeering classes, GDB namespace, exceptions, and the UI.
Moves the DMA loop and DMA thread to the DMA Controller class.
Moves the emulation loop and the emulation thread to a new EmulationLoop class.

All in all, removes all TrainWrecks (aka Law of Demeter violations) related to the Machine.

The Machine is now close to being removed entirely. It's only a dumb service container.

It is only created and used by the Program Executor and unit tests.

But removing it entirely would need the usage of Pure.DI (already used on the UI side), which requires interfaces.

There are also no cyclic dependencies between:

  • The CPU and the DualPic. It is instantiated by the CPU.
  • The CPU and the ExecutionFlowRecorder. It is instantiated by the CPU.
  • The CPU and the IOPortDispatcher
  • The CPU and the Callback Handler
  • The CPU and the MachineBreakpoints

Rationale behind Changes

Makes the code more SOLID, more testable, and way easier to work with.
All classes or nearly all of them only access their toys and their friends, they do not overreach.
Fixes #338

Suggested Testing Steps

Already manually tested:

  • emulation
  • UI

To be tested against master very soon (by me) - but should still be working:

  • Reverse engineering classes (ExecutionFlowRecorder, RecorderDataWriter, and friends)
  • GDB

@maximilien-noal maximilien-noal added tests About unit or integration testing refactoring Involves refactoring existing code labels Aug 1, 2023
@maximilien-noal maximilien-noal self-assigned this Aug 1, 2023
@maximilien-noal maximilien-noal changed the title Fix/god Fix: Machine is a God class Aug 1, 2023

public VideoState VideoState => Machine.VgaRegisters;

public ArgbPalette ArgbPalette => Machine.VgaRegisters.DacRegisters.ArgbPalette;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of these public properties? They do not seem to be related to the concept of a programexecutor.
Is this the ProgramExecuter taking over some of the "servicecontainer" functionality of the Machine?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a temporary fix so I could remove all references that where left in the MainWindowViewModel to the Machine.

Copy link
Contributor

@JorisVanEijden JorisVanEijden left a comment

Choose a reason for hiding this comment

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

Looks good to me. Nice cleanup.

Signed-off-by: Maximilien Noal <[email protected]>
Comment on lines +68 to +71
} catch (Exception e) {
e.Demystify();
throw new InvalidVMOperationException(_cpu.State, e);
}

Check notice

Code scanning / CodeQL

Generic catch clause

Generic catch clause.
@maximilien-noal maximilien-noal merged commit d7cc99e into master Aug 2, 2023
@maximilien-noal maximilien-noal deleted the fix/god branch August 3, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Involves refactoring existing code tests About unit or integration testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: make the emulator code more testable. Refactor the Machine especially.
3 participants