-
Notifications
You must be signed in to change notification settings - Fork 22
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
Conversation
Reword the section about the release policy and the reviewer who made these suggestions Co-authored-by: Eievui <[email protected]>
Separate the line stating the baseline and the fact that this is the new official reference Co-authored-by: Eldred Habert <[email protected]>
Most of the rewording suggestions from avivace's review implemented Co-authored-by: Antonio Vivace <[email protected]>
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree here
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 |
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 |
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