pokecrystal using hardware.inc

[rawr51919]

This draft PR shows what pokecrystal would look like if it was using hardware.inc instead of it's present constants file.
This PR now aims to convert pokecrystal to using hardware.inc above what it uses now for constants.
Resolves #914

[rawr51919]

Using the latest RGBDS master, this builds a ROM perfectly fine and matches up

[rawr51919]

@ISSOtm Comments added for the changed constants, you're welcome to give it another look
Only problem is getting GitHub Actions to be happy about it

[Rangi42]

Using the latest RGBDS master, this builds a ROM perfectly fine and matches up

But does it build with rgbds 0.5.2?

Also, I don't think hardware.inc should be a submodule. We used to have one for the extras tools, and it was a frequent source of issues (people not cloning it with the repo, either being stuck on the submodule HEAD or having to manually update which commit it points to, etc). If we do end up using hardware.inc, let's copy it into the repo, like with lzcomp.

[rawr51919]

Using the latest RGBDS master, this builds a ROM perfectly fine and matches up

But does it build with rgbds 0.5.2?

Also, I don't think hardware.inc should be a submodule. We used to have one for the extras tools, and it was a frequent source of issues (people not cloning it with the repo, either being stuck on the submodule HEAD or having to manually update which commit it points to, etc). If we do end up using hardware.inc, let's copy it into the repo, like with lzcomp.

Couldn't updating hardware.inc be automated using GitHub Actions to prevent those issues? Because it is a submodule it can be updated per-commit to remove those issues @Rangi42

[rawr51919]

@Rangi42 hardware.inc has been directly inserted into the repo, as a sideeffect fixing GitHub Actions' build which uses v0.5.2 so yes, it does build with 0.5.2

[rawr51919]

@ISSOtm Anything else I should do before this is considered complete?

[vulcandth]

@coltongit How would you feel about re-adding the constants that were removed and putting them in their own constants file. Possibly constants/extra_hardware_constants.asm?

[rawr51919]

@coltongit How would you feel about re-adding the constants that were removed and putting them in their own constants file. Possibly constants/extra_hardware_constants.asm?

Using hardware.inc allowed some of these unknown constants to be identified which should make that task a bit easier

[ISSOtm]

Maybe in a legacy file, then? What constants are those?

[rawr51919]

Maybe in a legacy file, then? What constants are those?

I'm working on the next iteration getting some of those constants addressed and the new extra constants file completed, two of the unknown registers get names thanks to hardware.inc.

[ISSOtm]

I agree with Rangi that a lot of the symbols shouldn't have turned into constants; I have pointed out some of the corresponding symbols hardware.inc provides, but didn't outline them all in a web interface. (A couple of regexes should take care of them.)

The MBC3-specific constants are not provided by hardware.inc, and this is an important omission that should be corrected upstream; I will open a PR there regarding those.

Finally, hardware.inc provides not only *B_* constants containing bit indices, but also *F_* ones containing the corresponding 1 << *B_* for convenience. This should significantly reduce the size of many lines (find * -name '*.asm' -exec sed -Ei 's/1 << ([A-Z]+)B_/\1F_/g' {} +, then maybe a find * -name '*.asm' -exec sed -Ei 's/\(([A-Za-z_][A-Za-z0-9_@#]*)\)//g' to clean up the leftover (LCDCF_BGON) | (LCDCF_OBJON) etc. parens).

[rawr51919]

So the constants have been added in, yet I'm not sure what's causing the build to not match here
@ISSOtm Otherwise does this look any better?

[aaaaaa123456789]

@coltongit Every change you make through the interface is a new email and a new commit, and a new CI run which counts against our credits. Please make them locally and commit all at once. This is way out of hand.

[ISSOtm]

Please use the "add this suggestion to batch" button, at least.

[rawr51919]

Much better, I'm gonna have to get used to the batch option real quick

[Rangi42]

Closing this since hardware.inc itself is being actively revised, and it's still unclear whether we'll want to use it.

[ISSOtm]

What's there is already published, I'm not sure how this warrants closing this PR.

[rawr51919]

What's there is already published, I'm not sure how this warrants closing this PR.

Depends on if hardware.inc is wanted at all. We can always return to this PR if the consensus shows it actually IS wanted by pret for these disassemblies.

[ISSOtm]

It won't be "wanted" because the status quo "works fine", but that PR has never been about whether it's wanted, but about harmonization.

[rawr51919]

It won't be "wanted" because the status quo "works fine", but that PR has never been about whether it's wanted, but about harmonization.

Also simultaneously the reason why it can be reopened if and when it's decided it's actually wanted

[ISSOtm]

Closing this since hardware.inc itself is being actively revised, and it's still unclear whether we'll want to use it.

And there we go! gbdev/hardware.inc@23eaa16 This can be re-opened now.