Open vulcandth opened 2 years ago
mid-kid
commented
2 years ago IIRC one of the issues was that it doesn't have all the defines that we need, but if we need them, then they're likely very useful to other projects as well, so we should be contributing them upstream as well. Until then it's fine to have a modified hardware.inc in this repository.
Other than that it's just porting busywork.
vulcandth
commented
2 years ago Would it be hard to use hardware.inc and then define new pret constants in a separate file? Although I agree, it could just be busy work.
aaaaaa123456789
commented
2 years ago I've always been a strong advocate of just taking whatever you need from hardware.inc instead of polluting your namespace with lots of constants you have no intentions of using. After all, hardware.inc describes hardware that has been out of manufacturing for over 20 years; there's no chance for that hardware to change, so keeping "up to date" with changes upstream is irrelevant here. While it would be a very bad idea to use hardware.inc's constant names for other things, I don't see the harm in leaving most of them out.
And yes, the focus on MBC5 is problematic. I have no idea of why hardware.inc makes this choice, as not even all homebrew uses MBC5. But it does, and it would collide with the MBC3+RTC cart that Pokémon Crystal uses.
ISSOtm
commented
2 years ago The advantages are not much, but still exist:
I'm not sure what focus hardware.inc might be having? Grepping -i for MBC5 yields the cartridge types (which are obviously legitimate), and a single comment, which is technically not wrong but can be changed if you really really want.
aaaaaa123456789
commented
2 years ago Keeping up to date with RGBDS is part of what I'm worried about — pokecrystal occasionally lags behind, so it's better to update those definitions along with everything else. The effort is minimal and we ensure that we can always build the repo with a single version of the toolchain. (This is not much of a concern as long as pokecrystal is part of RGBDS's CI, but we're talking about minor differences here.)
The MBC5 constants (the ones under that comment) are simply incorrect for other cart types. $3000 maps to the ROM bank, not to the "ROM bank upper byte", for instance. (And it makes no sense to speak of a "byte 0" when there's only one byte.)
What differences in constants can you find?
mid-kid
commented
2 years ago Keeping up to date with RGBDS is part of what I'm worried about — pokecrystal occasionally lags behind, so it's better to update those definitions along with everything else.
This is a non-issue. The file would be copied and checked into this repository. Even if the repository were to be a submodule (it won't), the version would be pinned.
Most of the benefit is in following community standards and contributing to how the hardware definitions are specified, as well as making it easier for users to use additional hardware that we haven't even considered adding to hardware_constants.inc (for example, MBCs), or future clarifications of or additions to the constants.
aaaaaa123456789
commented
2 years ago Most of the benefit is in following community standards and contributing to how the hardware definitions are specified...
Don't we already follow standards in everything but MBC registers (where we can't, because hardware.inc doesn't implement MBC3)?
...as well as making it easier for users to use additional hardware that we haven't even considered adding to hardware_constants.inc (for example, MBCs)...
hardware.inc doesn't support anything but MBC5, and what other additional hardware are you referring to?
...or future clarifications of or additions to the constants.
I don't see much changing when we're talking about a device that has been out of manufacturing for 20 years.
If we're going to take a snapshot of the file and check it in, what's there to gain with respect to what we already have, which is essentially a snapshot with the constants we use?
mid-kid
commented
2 years ago Don't we already follow standards in everything but MBC registers
We don't
what other additional hardware are you referring to?
GB hardware and bit definitions that the game doesn't use.
what's there to gain with respect to what we already have
Keeping things in sync. If we were already using all the same constants it'd be a drop-in. It isn't.
aaaaaa123456789
commented
2 years ago I think a step we can agree on is to rename our constants to match hardware.inc's names, then?
mid-kid
commented
2 years ago Yes. Although there's no reason not to use and contribute to hardware.inc at that point. Polluting the namespace with definitions relating to common gameboy hardware simply isn't a concern, and the MBC5-specific definitions can be ignored.
ISSOtm
commented
2 years ago And, as I've asked in my previous post, how does hardware.inc "only" support MBC5? It does not have anything MBC-specific.
rawr51919
commented
2 years ago Draft PR exists that has a proof-of-concept working compile and matching builds using latest RGBDS master as well as RGBDS v0.5.2 (#943)
rawr51919
commented
2 years ago Said hardware.inc PR will also need companions across the whole of pret, updating their builds to use the latest hardware.inc if they use it already and refactoring them to use hardware.inc if they don't
@ISSOtm mentioned this on discord today, that we should be using
hardware.incfrom https://github.com/gbdev/hardware.inc/blob/master/hardware.inc. Looking at the discord history, It sems this was discussed in the past, but I didn't see a clear definitive reason why or why not use it. A couple points on why not seemed to do with extra "noise", that it might be based on MBC5, and how pokecrystal uses more constants related to individual bits.Anyways, I'm going to let you all who have more experience weigh in on this. I'm just opening this to start a discussion. If we decide to take action with this, i'll help out.