gbdev / hardware.inc

RGBDS include file for Game Boy hardware definitions
Creative Commons Zero v1.0 Universal
115 stars 21 forks source link

`PADF_*` and `PADB_*` constants don't represent hardware values #37

Open aaaaaa123456789 opened 1 year ago

aaaaaa123456789 commented 1 year ago

The PADF_* and PADB_* constants have been around for a long time, but they don't describe any hardware at all. Their values represent the bitfields of the synthetic joypad value computed by some games after reading both lines of inputs and packing them into a single value; they represent convention, not hardware, and as such, they don't belong here. (This is not a statement about their utility: regardless of their value, it's a fact that there's nothing at all in the GB/GBC hardware representing the "down" D-pad button by the value $80.)

The correct values for the F series of constants for the right, left, up and down buttons are respectively 1, 2, 4 and 8, not their current values of $10, $20, $40 and $80. (The B series should be likewise adjusted.) These values overlap with the respective values for A, B, select and start: those bits also overlap in hardware, and thus the correct hardware constants should have the same values. However, changing the values of the defined constants would represent a silent incompatible change, which would be undoubtedly undesirable. Therefore, my proposal is:

  1. Create new constants for the inputs with the correct values and different names, so they don't collide with current constants. (For instance, INPUTF_SELECT and INPUTF_UP, both equal to 4.)
  2. Announce the current constants as deprecated.
  3. Remove the current constants in the next major version.

Any thoughts?

zlago commented 1 year ago

i dont see the point of constants that.. arent exactly usable unless you store read input states in two separate bytes ive seen isso suggesting companion files for hardware.inc a few times, which seems like a reasonable solution (hardware.inc would only contain constants everyone actively uses, then there would be opt-in companion files for things like mapper Memory Bank Controller constants, or pad definitions)

aaaaaa123456789 commented 1 year ago

The constants are useful for defining your own without pulling up Pan Docs (DEF DOWN EQU INPUTF_DOWN << 4), and they can also be used directly that way. The whole point of hardware.inc is to describe hardware, as the name implies, and in hardware, "down" and "start" are both bit 3.

tobiasvl commented 1 year ago

I raised this issue on the Discord because I was trying to migrate the LADX disassembly from gbhw.inc to hardware.inc. I have no idea which order is more common for games to use in their synthetic bitfields, but it's interesting that an official Nintendo game used the opposite one from hardware.inc.

Although these values don't represent hardware, I do think there's value in utility as well. My hope when originally standardizing this ancient file was that people would use it, for both disassemblies of official games and for homebrew. If this is desirable we could create a second set of constants that have the inverted nybble order. (If we do, I would propose we call them something more agnostic than "SGB" and "GBA" order.) It wouldn't reflect hardware per se, but it would reflect extremely common boilerplate that makes developing for the GB a little less cumbersome.

Failing that, I do think we should at least have constants for the input states, for the reasons @aaaaaa123456789 stated.

ISSOtm commented 1 year ago

Considering the register is called rP1, they should be called P1F_*.

As was mentioned by zlago, I am in favour of a companion file that provides what otherwise becomes boilerplate. Seeing as it seems to make consensus, I'll open an issue for it.

pinobatch commented 1 year ago

What's wrong with the names "SGB nibble order" and "GBA nibble order"? They correspond to the behavior of other GB-adjacent Nintendo products, namely the Super Game Boy accessory's ICD2 bridge chip and the Game Boy Advance platform.

I imagine that this companion file would include commonly used definitions that are pedantically not directly hardware-related. Instead of having a separate file, would it be useful to condition these definitions on not having defined HARDWARE_LEAN_AND_MEAN beforehand, analogous to WIN32_LEAN_AND_MEAN in windows.h? Depending on what is defined beforehand, hardware.inc would behave as follows with respect to P1 bits:

aaaaaa123456789 commented 1 year ago

First of all, if a constant for "extra" definitions was to be added, it would have to be opt in, not opt out. That being said, it's still a bad idea. This project has been offered to users as a way to reference hardware, not to impose opinionated non-hardware non-conventions onto unaware codebases. The registers have official names, and the flags have obvious behaviors; anything else shouldn't be here, because the advertised goal isn't to impose on random codebases.

If you want to make a "standard library", the proper way to do that is to make an actual standard library repository.

(As for the SGB/GBA nibble orders: those represent non-GB hardware (and specifically, hardware that a GB/GBC game has no way to access), and therefore they don't belong here any more than Dreamcast constants do.)

nitro2k01 commented 1 year ago

Opposition to removal

I oppose removing these constants, mostly for the sake of backward compatibility. If hardware.inc was designed from scratch now I would agree that those constants should not have been added. Now they are there, and a bunch of source code likely relies on them. Teaching users of hardware.inc that stuff will potentially break in every new version of the library because constants are deprecated left and right is an excellent way of making people give up and not update, which benefits no one in the long run. In my opinion the cost of removing them and causing trouble for users is higher than the cost of keeping things that technically don't belong in the file.

What might sway my opinion is if it's either shown that few projects actually rely on these constants, or that there's some other natural place for them to go, so that they don't have to be ad hoc defined by every person whose project is broken by the change.

Opposition to "SGB/GBA order"

I oppose calling calling the nibble order by SGB or GBA, as this is "neither here nor there". The internal representation in the SGB and GBA registers have no bearing on what code running on the Gameboy's CPU is doing.

Statistics of de facto usage

Regarding the question of how opinionated the nibble order is, I created a small script that looped through my copy of GoodGBX. The test methodology is mainly as follows: look for $3E followed by $10 or $20 (ie ld A,$x0). Check if this is followed by what looks like a joypad read routine, with a few variations. If $20 was written first, record that the ROM is using the "D-pad in upper nibble" order. If $10 was written first, record that the ROM is using the "D-pad in lower nibble" order. If neither was found anywhere in the ROM image, record that the ROM has an unknown joypad routine.

The results are as follows: Unknown: 1718 D-pad in upper nibble: 7161 D-pad in lower nibble: 454

There may be some false results, but the trend is clear. LADX is in the minority. Joypad in upper nibble (same as hardware.inc's constants) is by far the most common. This likely also what Nintendo promoted in their example code. So, as far as the constants are opinionated they are in good company.

Suggested action

In my opinion, the way forward is this:

  1. Keep the existing constants with their current names and values, but document their meaning and intended usage.
  2. Maybe create a new constants for the hardware order, but name them after both buttons. This is essentially what the P1F_n are, minus the naming. So for example, P1F_0 could be aliased to P1F_A_OR_RIGHT etc.

Natural nibble order in GBC test mode (side note)

A fun thing to note is that there is a natural nibble order on GBC, stemming from the undocumented register FF63. However, this register is only available in test mode (requiring hardware modifications or similar). So this really has no bearing on what hardware.inc should do, as it doesn't document undocumented registers that are not available in the regular "user mode" context. Just a side note.

pinobatch commented 1 year ago

Is Order Crossing next?

Super Game Boy order can be accessed through Game Boy software that sends 65816 code in DATA_SND packets followed by a JUMP. Some time ago, I made a tech demo that JUMPs to 65816 code and sends data back to the GB program through the ICD2. At present, it amounts to a "hello world" proving that JUMP had been called. Would it sway the argument if I made it into a more polished proof of concept?

aaaaaa123456789 commented 1 year ago

I sincerely don't think that would be at all relevant here.

ISSOtm commented 1 year ago

As you mentioned, such code would be 65816, running on the SNES. Thus, it would have no use of hardware.inc, and so I believe it wouldn't sway this discussion.