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

Add constants from pokecrystal #32

Closed
wants to merge 15 commits into from
Closed

Add constants from pokecrystal #32

wants to merge 15 commits into from

Conversation

rawr51919
Copy link
Contributor

This PR is a proof-of-concept showing what a hardware.inc using constants from pokecrystal would look like.
Some of these names can be used for new names for constants down the line, so this one might be a little while before any action comes of this.
Fixes #26

@rawr51919 rawr51919 marked this pull request as draft July 5, 2022 22:15
hardware.inc Outdated Show resolved Hide resolved
hardware.inc Outdated Show resolved Hide resolved
hardware.inc Outdated Show resolved Hide resolved
hardware.inc Outdated Show resolved Hide resolved
hardware.inc Outdated Show resolved Hide resolved
hardware.inc Outdated Show resolved Hide resolved
DEF IEB_TIMER EQU 2
DEF IEB_STAT EQU 1
DEF IEB_VBLANK EQU 0
DEF IEB_DEFAULT EQU (1 << IEB_SERIAL) | (1 << IEB_TIMER) | (1 << IEB_STAT) | (1 << IEB_VBLANK)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should have a "default" because this doesn't have anything to do with the hardware; this is the type of thing that would be put in a defines.inc file

DEF SERIAL EQU IEB_SERIAL
DEF TIMER EQU IEB_TIMER
DEF LCD_STAT EQU IEB_STAT
DEF VBLANK EQU IEB_VBLANK
Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't think these aliases should be provided, especially since it's likely people are already using them for other things.

Copy link
Member

Choose a reason for hiding this comment

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

Agree here

@evie-calico
Copy link
Contributor

I don't know if there was prior discussion about this, but I think this would really clutter the file simply to become compatible with pokecrystal. Why not update pokecrystal to conform to hardware.inc instead?

@rawr51919
Copy link
Contributor Author

rawr51919 commented Jul 5, 2022

I don't know if there was prior discussion about this, but I think this would really clutter the file simply to become compatible with pokecrystal. Why not update pokecrystal to conform to hardware.inc instead?

There wasn't much in the way of discussion about this, yet pokecrystal's PR #943 does exactly that

@evie-calico
Copy link
Contributor

I think rather than updating this file to support every different project's naming scheme, we should get the projects to settle on using hardware.inc

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

Backported a constant addition from this PR to #33, RPB_LED_ON. I had forgot to add it during #31

@rawr51919
Copy link
Contributor Author

I think rather than updating this file to support every different project's naming scheme, we should get the projects to settle on using hardware.inc

There has also been an issue opened on the subject on pokecrystal issue #914

@rawr51919 rawr51919 closed this by deleting the head repository Aug 12, 2023
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.

More human-friendly names for some constants
4 participants