Implement Random RAM #158

naclander · 2020-05-03T22:23:42Z

If the random flag is set, iterate through the existing RAM arrays and
randomize the values.

While there are no specific RAM unit tests, I played through some of the
ROMs I have locally and the games seemed to be running fine.

krs013 · 2020-05-03T22:44:57Z

Thanks! The randomization looks good, but there are two things that I get caught on:

First, I'm not sure I with the change in the initialization argument from random to is_random. What's your reasoning on that?

Also, what's all the numpy stuff for? Is that just to avoid using 0x100 as the max? We're not that afraid of magic numbers. Personally, I'm a lot more afraid of excessive dependencies... ;)

It'll be good to have this, though! That particular to-do has been there for quite a while, haha.

naclander · 2020-05-03T22:53:55Z

Hey @krs013, thanks for the comments.

I changed the flag to is_random because random is the name of the Python module, which caused conflicts with the flag name.

Yup, I used numpy for figuring out the max value of the "B" typecode. I'm not sure if this was system dependent. I couldn't find a way to automatically get this upper bound without numpy, but am happy to switch it if there is a way. I figured importing numpy was fine, as it's already a dependency of the project, but I do agree with your point of limiting dependencies. I don't have a strong preference in any particular direction, so I'm happy to change it to whatever you prefer.

krs013 · 2020-05-03T23:18:45Z

Ah, I see, of course. I guess I didn't think about the name aliasing before the import was needed. Could I suggest randomize instead of is_random? The latter sounds more like a check rather than setting a parameter.

Regardless of the platform that PyBoy is running on, the maximum value for a gameboy register will always be the same, so you can just hardcode that to 0-255 (or 0x00-0xFF if you want it to look nicer)!

Also, Python array.array objects do allow you see their .typecode and .itemsize, though I'd suggest using the type code for bounds setting, since only the minimum byte size is guaranteed, but even if the array gets promoted to a larger itemsize, I believe it will still throw an exception if a value is set that is out of bounds.

And yeah, I love numpy and it's great, but leaving it as an install dependency is a hassle sometimes since numpy is sometimes problematic to install. I've been trying to squeeze it out to only be in the optional parts of PyBoy for quite a while now! Maybe one day I'll finish.

naclander · 2020-05-04T02:14:57Z

Sounds good. I've gone ahead and hardcoded the max value, and removed numpy as a dependency. I've changed the flag name as well.

pyboy/core/ram.py

Baekalfen · 2020-05-04T14:30:32Z

That particular to-do has been there for quite a while

It was written before the first PPU was implemented 😉

Baekalfen · 2020-05-04T14:54:04Z

Thanks for your work @naclander! I think it needs a little more before we're ready to merge. Do you want to do it?

I can see we need the following:

Add optional argument to __main__.py
Add optional argument to PyBoy class (default=False)
Randomize other registers like the ones in lcd.py, sound.py, timer.py, interaction.py ― maybe more?
Write some quick, simple tests to verify the values are set.
It cannot break any current tests (as randomize should default to False for now)
(6.) Should some tests be run with randomize=True?

Baekalfen · 2020-05-04T14:54:58Z

@krs013 Does it make sense to implement this as a plugin?

krs013 · 2020-05-04T16:40:40Z

I think this belongs in the core, since it's part of what you can expect any emulator to do, not just ours.

nicoeps · 2020-05-04T16:53:28Z

Thanks for your work @naclander! I think it needs a little more before we're ready to merge. Do you want to do it?

I can see we need the following:

Add optional argument to __main__.py

Add optional argument to PyBoy class (default=False)

Randomize other registers like the ones in lcd.py, sound.py, timer.py, interaction.py ― maybe more?

Write some quick, simple tests to verify the values are set.

It cannot break any current tests (as randomize should default to False for now)
(6.) Should some tests be run with randomize=True?

I only think WRAM/HRAM should be randomized (Well VRAM and OAM too technically). Some registers have bits that are unreadable/writeable. Also, IO registers are initialized to specific values, where most are just 0xFF.

nicoeps · 2020-05-04T17:06:47Z

pyboy/core/ram.py

        self.internal_ram0 = array.array("B", [0] * (INTERNAL_RAM0))
        self.non_io_internal_ram0 = array.array("B", [0] * (NON_IO_INTERNAL_RAM0))
        self.io_ports = array.array("B", [0] * (IO_PORTS))
        self.internal_ram1 = array.array("B", [0] * (INTERNAL_RAM1))
        self.non_io_internal_ram1 = array.array("B", [0] * (NON_IO_INTERNAL_RAM1))
        self.interrupt_register = array.array("B", [0] * (INTERRUPT_ENABLE_REGISTER))

+        if randomize: # NOTE: In real life, the RAM is scrambled with random data on boot.
+            for mem in (self.internal_ram0, self.non_io_internal_ram0, self.io_ports, self.internal_ram1,


FEA0-FEFF range seems not to be randomized on DMG
Also, not really important, but maybe write hardware instead of real life?

Suggested change

for mem in (self.internal_ram0, self.non_io_internal_ram0, self.io_ports, self.internal_ram1,

for mem in (self.internal_ram0, self.internal_ram1):

nicoeps · 2020-05-04T17:07:11Z

pyboy/core/ram.py

        self.internal_ram0 = array.array("B", [0] * (INTERNAL_RAM0))
        self.non_io_internal_ram0 = array.array("B", [0] * (NON_IO_INTERNAL_RAM0))
        self.io_ports = array.array("B", [0] * (IO_PORTS))
        self.internal_ram1 = array.array("B", [0] * (INTERNAL_RAM1))
        self.non_io_internal_ram1 = array.array("B", [0] * (NON_IO_INTERNAL_RAM1))
        self.interrupt_register = array.array("B", [0] * (INTERRUPT_ENABLE_REGISTER))

+        if randomize: # NOTE: In real life, the RAM is scrambled with random data on boot.
+            for mem in (self.internal_ram0, self.non_io_internal_ram0, self.io_ports, self.internal_ram1,
+                      self.non_io_internal_ram1, self.interrupt_register):


Only randomize RAM

Suggested change

self.non_io_internal_ram1, self.interrupt_register):

What isn't RAM in this case?

Everything is inside the RAM class, should some things be moved elsewhere?

Well, the IO registers and the interrupt register shouldn't be randomized.
I've attached an example from BGB that shows the values of some of the IO Registers.
WRAM, HRAM, VRAM, and OAM do contain random values on boot. Initializing VRAM with random values is redundant though, as the bootrom clears it.

We still want to randomize VRAM, as we have our own boot ROM, which currently doesn't clear VRAM. If somebody makes a mistake and the VRAM isn't cleared, it will show up.

Baekalfen · 2020-05-04T18:02:53Z

Some registers have bits that are unreadable/writeable. Also, IO registers are initialized to specific values, where most are just 0xFF.

You're totally right. But I'm not sure if this can easily be fixed in this PR, or if we should do that separately?

naclander · 2020-05-05T02:43:19Z

@Baekalfen

Add optional argument to __main__.py
Add optional argument to PyBoy class (default=False)

Is this something we want to plumb all the way from the user?

As a user starting PyBoy, I would imagine the emulator starting with random RAM ( as close to the hardware as possible ) by default is good enough.

I do agree that this should be a flag on the Motherboard class, and so probably on the PyBoy class, so that it can programmatically be changed.

Randomize other registers like the ones in lcd.py, sound.py, timer.py, interaction.py ― maybe more?

I would have to look at these classes more closely. I wasn't thinking of changing anything outside the RAM module initially.

Write some quick, simple tests to verify the values are set.

Cool, I'll do that.

It cannot break any current tests (as randomize should default to False for now)
(6.) Should some tests be run with randomize=True?

I would say any end to end tests should be run with how the emulator is presented to users by default, that is, with randomize=True. Surely we can have specific end-to-end tests that test randomize=False, although I'm not sure what value those would bring.

In either case, I don't have access to all the ROMs that the unit tests use, so I can't verify all tests pass, but from the small amount of testing I did manage to do everything seems to be working fine.

BTW, I like the mooneye and blargg tests, but some of them seem to be broken when ran ( at least on my machine ).

Baekalfen · 2020-06-30T19:40:06Z

I'm not sure why this was closed

If the random flag is set, iterate through the existing RAM arrays and randomize the values. While there are no specific RAM unit tests, I played through some of the ROMs I have locally and the games seemed to be running fine. (cherry picked from commit 76604d5)

(cherry picked from commit e921fe9)

Co-authored-by: Nico <55274642+nicoeps@users.noreply.github.com> (cherry picked from commit 13139ec)

(cherry picked from commit 130a782)

(cherry picked from commit 57b5c6a)

krs013 · 2020-09-30T19:21:51Z

I've reset the base branch and cherry picked the commits that belong in this PR. Unfortunately, since that technically creates new commits, the time stamps have changed, so the comments are a little out of order. Still, we can at least see the real history and file changes now.
EDIT: Also, sorry about the force pushes. I know that's a hassle. Hopefully we don't have to do this too much.
EDIT2: If anyone wants to keep working on this branch, and you don't have any local changes, you can hard reset your local copy: https://stackoverflow.com/a/14787801/2102965. If you have uncommitted changes, you can stash them before the reset and pop them after to reapply. If you have committed changes you can rebase or cherry pick them. Merge will not work anymore, though.

Cython does not return array length with len() so I had to change it for explicit for loops. All tests pass.

krs013 · 2020-10-01T05:53:19Z

I started adding the arguments and things got away from me and I ended up finishing the tests and the feature. I ended up having to change more than I expected; it turns out that you can't use len on arrays with Cython. Thanks to the new tests for helping me discover that! The old tests all work again, and the default behavior is to leave the memory zeroed, as before. If Travis agrees with my test results I'd say this is ready for a merge, pending a quick review. Thanks for your work on this, naclander and everyone!

Baekalfen · 2020-10-01T14:59:31Z

Great work on fixing this PR up @krs013! I think we'll need to run at least one test-ROM to determine, that the emulator is still functional after this modification. Does Blargg's tests for example work after randomization?

krs013 · 2020-10-01T18:29:42Z

Just tried it and they all pass with randomization on. I'll configure some to rerun with randomization, or I guess we could run all of them twice if we want maximum coverage but that sets a tedious precedent.

Baekalfen · 2020-10-01T18:39:42Z

Just tried it and they all pass with randomization on. I'll configure some to rerun with randomization, or I guess we could run all of them twice if we want maximum coverage but that sets a tedious precedent.

Just some quick ones. Or even just add it to one of the existing tests. For example the ones generating GIFs?

krs013 · 2020-10-02T07:55:16Z

Done. Sorry it took so long for so little, haha, I took a while before deciding that randomizing the Blargg tests wasn't really worth it, and then had to use a different machine to do the GIF tests because I don't trust my Linux machine due to a GIF bug that I need to confirm. If it shows up on Debian or Ubuntu or something then expect an issue... sometimes I get 1.GIF rendering with lingering pixels in the health bar. Also a problem is that somehow this doesn't fail the tests?

Baekalfen · 2020-10-02T08:07:41Z

Ok, that should be good enough. Maybe one day, I'll move more and more of the tests over to use randomized RAM as default.

I think I've disabled the GIF testing on Linux, as that doesn't work because of the artifacts you describe. I haven't been able to make it go away. But it's outside PyBoy it happens.

nicoeps suggested changes May 4, 2020

View reviewed changes

pyboy/core/ram.py Outdated Show resolved Hide resolved

nicoeps suggested changes May 4, 2020

View reviewed changes

Baekalfen force-pushed the prerelease branch from 58e33b6 to 63c078e Compare May 14, 2020 21:14

Baekalfen changed the base branch from prerelease to v1.2.0 May 15, 2020 21:43

Baekalfen closed this Jun 24, 2020

Baekalfen reopened this Jun 30, 2020

naclander and others added 5 commits September 30, 2020 14:11

Implement Random RAM

2c59b36

If the random flag is set, iterate through the existing RAM arrays and randomize the values. While there are no specific RAM unit tests, I played through some of the ROMs I have locally and the games seemed to be running fine. (cherry picked from commit 76604d5)

Hardcode max register value

36f029e

(cherry picked from commit e921fe9)

Update pyboy/core/ram.py

238cb89

Co-authored-by: Nico <55274642+nicoeps@users.noreply.github.com> (cherry picked from commit 13139ec)

Update ram.py

44e918a

(cherry picked from commit 130a782)

Update ram.py

9b80a59

(cherry picked from commit 57b5c6a)

krs013 changed the base branch from v1.2.0 to master September 30, 2020 19:14

krs013 force-pushed the naclander/ram branch from 0e66de5 to 9b80a59 Compare September 30, 2020 19:16

Added args and tests for ram randomization.

1a202cd

Cython does not return array length with len() so I had to change it for explicit for loops. All tests pass.

Added ram randomization to super mario land gif test

1910894

Baekalfen approved these changes Oct 2, 2020

View reviewed changes

Baekalfen merged commit 155c94f into Baekalfen:master Oct 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Random RAM #158

Implement Random RAM #158

naclander commented May 3, 2020

krs013 commented May 3, 2020

naclander commented May 3, 2020

krs013 commented May 3, 2020

naclander commented May 4, 2020

Baekalfen commented May 4, 2020

Baekalfen commented May 4, 2020

Baekalfen commented May 4, 2020

krs013 commented May 4, 2020

nicoeps commented May 4, 2020

nicoeps May 4, 2020 •

edited

nicoeps May 4, 2020

naclander May 5, 2020

nicoeps May 5, 2020

Baekalfen May 5, 2020

Baekalfen commented May 4, 2020

naclander commented May 5, 2020

Baekalfen commented Jun 30, 2020

krs013 commented Sep 30, 2020 •

edited

krs013 commented Oct 1, 2020

Baekalfen commented Oct 1, 2020

krs013 commented Oct 1, 2020

Baekalfen commented Oct 1, 2020

krs013 commented Oct 2, 2020

Baekalfen commented Oct 2, 2020

	for mem in (self.internal_ram0, self.non_io_internal_ram0, self.io_ports, self.internal_ram1,
	for mem in (self.internal_ram0, self.internal_ram1):

Implement Random RAM #158

Implement Random RAM #158

Conversation

naclander commented May 3, 2020

krs013 commented May 3, 2020

naclander commented May 3, 2020

krs013 commented May 3, 2020

naclander commented May 4, 2020

Baekalfen commented May 4, 2020

Baekalfen commented May 4, 2020

Baekalfen commented May 4, 2020

krs013 commented May 4, 2020

nicoeps commented May 4, 2020

nicoeps May 4, 2020 • edited

Choose a reason for hiding this comment

nicoeps May 4, 2020

Choose a reason for hiding this comment

naclander May 5, 2020

Choose a reason for hiding this comment

nicoeps May 5, 2020

Choose a reason for hiding this comment

Baekalfen May 5, 2020

Choose a reason for hiding this comment

Baekalfen commented May 4, 2020

naclander commented May 5, 2020

Baekalfen commented Jun 30, 2020

krs013 commented Sep 30, 2020 • edited

krs013 commented Oct 1, 2020

Baekalfen commented Oct 1, 2020

krs013 commented Oct 1, 2020

Baekalfen commented Oct 1, 2020

krs013 commented Oct 2, 2020

Baekalfen commented Oct 2, 2020

nicoeps May 4, 2020 •

edited

krs013 commented Sep 30, 2020 •

edited