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.

Sign up for GitHub

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

Jump to bottom

pokecrystal using hardware.inc #943

Closed
wants to merge 55 commits into from
Closed

pokecrystal using hardware.inc #943

wants to merge 55 commits into from

Conversation

rawr51919
Copy link
Contributor

@rawr51919 rawr51919 commented Jun 14, 2022 •
edited
Loading

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 rawr51919 marked this pull request as draft June 14, 2022 23:18
@rawr51919
Copy link
Contributor Author

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

@rawr51919 rawr51919 mentioned this pull request Jun 15, 2022
Open
Rangi42
Rangi42 reviewed Jun 15, 2022
View reviewed changes
audio/engine.asm Outdated Show resolved Hide resolved
@rawr51919 rawr51919 marked this pull request as ready for review June 15, 2022 13:45
@rawr51919 rawr51919 marked this pull request as draft June 15, 2022 13:45
@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 15, 2022 •
edited
Loading

@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

@rawr51919 rawr51919 marked this pull request as ready for review June 15, 2022 15:00
@Rangi42
Copy link
Member

Rangi42 commented Jun 15, 2022

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.

@Rangi42 Rangi42 marked this pull request as draft June 15, 2022 15:26
@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 15, 2022 •
edited
Loading

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
Copy link
Contributor Author

rawr51919 commented Jun 15, 2022 •
edited
Loading

@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
Copy link
Contributor Author

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

@vulcandth
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Contributor

ISSOtm commented Jun 15, 2022

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

@rawr51919
Copy link
Contributor Author

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
ISSOtm suggested changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@ISSOtm ISSOtm left a comment

Choose a reason for hiding this comment

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

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).

audio/engine.asm Outdated Show resolved Hide resolved
constants/serial_constants.asm Outdated Show resolved Hide resolved
constants/serial_constants.asm Outdated Show resolved Hide resolved
data/sprites/facings.asm Outdated Show resolved Hide resolved
engine/battle_anims/anim_commands.asm Outdated Show resolved Hide resolved
engine/phone/phonering_copytilemapatonce.asm Outdated Show resolved Hide resolved
home/audio.asm Outdated Show resolved Hide resolved
home/init.asm Outdated Show resolved Hide resolved
home/init.asm Outdated Show resolved Hide resolved
home/init.asm Outdated Show resolved Hide resolved
@rawr51919
Copy link
Contributor Author

rawr51919 commented Jun 15, 2022 •
edited
Loading

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
Copy link
Contributor

image

@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
Copy link
Contributor

ISSOtm commented Jun 26, 2022

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

@rawr51919
Copy link
Contributor Author

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

@rawr51919 rawr51919 mentioned this pull request Jun 26, 2022
Closed
This was referenced Jun 26, 2022
Closed
Merged
Merged
Merged
@Rangi42
Copy link
Member

Rangi42 commented Jun 27, 2022

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

@Rangi42 Rangi42 closed this Jun 27, 2022
@ISSOtm
Copy link
Contributor

ISSOtm commented Jun 27, 2022

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

@rawr51919 rawr51919 mentioned this pull request Jul 1, 2022
Merged
@rawr51919
Copy link
Contributor Author

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
Copy link
Contributor

ISSOtm commented Jul 1, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

ISSOtm commented Jul 5, 2022

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.

@rawr51919 rawr51919 mentioned this pull request Jul 5, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rangi42 Rangi42 Rangi42 left review comments

@ISSOtm ISSOtm ISSOtm requested changes

@mid-kid mid-kid mid-kid left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use hardware.inc from gbdev
6 participants
@rawr51919 @Rangi42 @vulcandth @ISSOtm @aaaaaa123456789 @mid-kid