Skip to content

Conversation

evie-calico
Copy link
Contributor

Also updated the Nintendo logo macro.

@evie-calico
Copy link
Contributor Author

Let me know if I should update the revision number, I wasn't sure what constituted a new version because it seems like not every PR bumps it.

@ISSOtm
Copy link
Member

ISSOtm commented May 3, 2021

Changes that affect how it's used bump the revision number; given that this breaks backwards compat, this should be a major bump.

Also updated the Nintendo logo macro.

Bumped major version to 4.0

This breaks compatibility with RGBDS versions prior to 0.5.0.

Bumped major revision to 4.0

3 -> 03, updated rev check macro
@evie-calico
Copy link
Contributor Author

This should be complete now. All constants use DEF, both macros use MACRO name, and the revision was bumped to 4.0.

@daid
Copy link
Contributor

daid commented May 4, 2021

If we are breaking compatibility, we might also want to talke a small look at the STAT interrupt. The pandocs consistenty call this the STAT interrupt, but hardware inc calls is LCDC interrupt at some places.

@evie-calico
Copy link
Contributor Author

It's only IEF_LCDC, but I agree that it should change to IEF_STAT. I can change this tomorrow if this is okay with other people.

Are there any other changes that should be made?

@evie-calico
Copy link
Contributor Author

Should these two bit constants have the "1"s removed? It looks like they were accidentally copied from the flags.

https://github.com/GreenAndEievui/hardware.inc/blob/a907ebdeaf1a174de1870466bc3ec1fbd4919d4d/hardware.inc#L900

@ISSOtm
Copy link
Member

ISSOtm commented May 4, 2021

Maybe we could keep the IEF_LCDC definition, but move it to a "compatibility"/"legacy" section at the end of the file?

@DuoDreamer
Copy link

Redefining flags names should not be considered so lightly. Changing that IE flag to STAT would have a wide-ranging impact. My vote is to keep it to what the big N labels it "LCDC", as this interrupt signal comes from the LCD Controller, to avoid any confusion.

@ISSOtm
Copy link
Member

ISSOtm commented May 4, 2021

On one hand, this is why I advocated for keeping the old constant around for compatibility's sake; on the other hand, the name is confusing, even it can make sense in hindsight by squinting a little, so renaming would be an improvement.

Either "LCD interrupt" or "STAT interrupt" is fine, but "LCDC interrupt" has confused plenty of people as to how to control the interrupt via LCDC, which you can't. Nintendo's official names can and have been problematic, so I personally don't think we should always follow them. Consistency with other documents, primarily the Pan Docs, is imo primordial.

@evie-calico
Copy link
Contributor Author

evie-calico commented May 4, 2021

I've left IEF_LCDC at the bottom of the file for compatibility.

While I'm making changes, could I add OAMF_GBCPAL constants to the OAM flags? I know they're equivalent to the corresponding integer value, but I think it's worth explicitly defining them. Better for a different PR. This should be fine for now.

@ISSOtm
Copy link
Member

ISSOtm commented May 4, 2021

It should be possible to FAIL if __RGBDS_MAJOR__ == 0 && __RGBDS_MINOR__ < 5.

@evie-calico
Copy link
Contributor Author

I added the version check to the top of the file, right above the check revision macro.

DEF HARDWARE_INC SET 1

rev_Check_hardware_inc : MACRO
IF __RGBDS_MAJOR__ == 0 && __RGBDS_MINOR__ < 5
Copy link
Member

Choose a reason for hiding this comment

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

This check needs to be above the DEF line if we want to avoid a fatal syntax error.

@evie-calico evie-calico requested a review from ISSOtm May 5, 2021 13:37
Moved IEF_LCDC within the include guard
Moved version check above the include guard.

Move include guard comment back to the include guard
@ISSOtm
Copy link
Member

ISSOtm commented May 9, 2021

Given that no further opposition has emerged, and that @DuoDreamer's compatibiltiy concern has been taken care of (in addition to an explicit version requirement), unless any objection arises, I'll merge this tomorrow morning.

@DuoDreamer
Copy link

All good. 👍

@ISSOtm ISSOtm merged commit 72ec03f into gbdev:master May 10, 2021
avivace pushed a commit that referenced this pull request Jan 11, 2022
Also updated the Nintendo logo macro.

Bumped major version to 4.0, since this breaks compatibility with RGBDS versions prior to 0.5.0.

3 -> 03, updated rev check macro

* Deprecate `IEF_LCDC` in favor of `IEF_STAT`

* Adde check for outdated RGBDS versions

* Update include guard to use `EQU`
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.

4 participants