Add sgb.inc

[dmitry-shechtman]

Closes #76

[Rangi42]

Oh, thanks! Good idea to make this a third .inc file, given all those super-specific but official SGB values (SGB_SOUND_A_LASER_SMALL, really? lol).

It will need comments briefly mentioning what all these things are, like hardware.inc's heading/subheading/description structure, and to follow the formatting conventions (e.g. lowercase def equ).

Also, B_* bit constants are appropriate for Boolean values stored in single bits. I don't think they really make sense for things like SGB_SOUND_A_PITCH, a two-bit value. We have examples of how to handle multi-bit flag values, e.g. rSYS's modes.

Anyway, this is good reference to expand off of!

[dmitry-shechtman]

It will need comments briefly mentioning what all these things are, like hardware.inc's heading/subheading/description structure, and to follow the formatting conventions (e.g. lowercase def equ).

Sure thing, I just had to get something out there quickly.

Also, B_* bit constants are appropriate for Boolean values stored in single bits. I don't think they really make sense for things like SGB_SOUND_A_PITCH, a two-bit value. We have examples of how to handle multi-bit flag values, e.g. rSYS's modes.

I'll have a look.

Also, I just noticed that the actual MLT_REQ constants are missing. Will add them later on.

[dmitry-shechtman]

Having seen the SYS_MODE constants, I am of the opinion that the method suggested herein is more robust.

I'm OK with changing the names, but not with hardcoding the values in binary form.

[dmitry-shechtman]

I'm OK with changing the names, but not with hardcoding the values in binary form.

May I suggest S_ to indicate shift amount? This would also apply to e.g. colours in rBGP.

[Rangi42]

Sounds like a good compromise; I could accept an S_ constant.

[dmitry-shechtman]

I understand the convention of using the A suffix for addresses, e.g. OAMA_Y.

I am wondering whether SGB_SOUNDA_A won't look ridiculous.

[dmitry-shechtman]

Again, turning it into a prefix seems appropriate:

• A_SGB_SOUND_A
• A_SGB_MLT_REQ_PLAYERS
• A_SGB_MASK_EN_MASK

[nitro2k01]

As said in #76

I don't like the name JOYP_SGB_MLT_REQ for this constant. Unless there's precedent for using this name that would warrant keeping it, I'd suggest a different name. The reason is the REQ at the end. REQ is the verb for the SGB command: request multiplayer access. What you do with rJOYP at this point strictly has nothing to do with the SGB command MLT_REQ. MLT_REQ was instead used to change a setting and the command has already finished at this point. The name, as is, also doesn't doesn't explain what the purpose of the constant is, just that it's related to MLT_REQ somehow.

I also have some issues with the semantics how things are used in pokered.

	and JOYP_SGB_MLT_REQ
	cp JOYP_SGB_MLT_REQ

These constants actually have different meanings, despite being the same value. The first one is a mask to isolate the two lowest bits. The second is the ID for player 1. What the code does is to check if the player ID ever changes from the value for player 1, which also (by design) happens to be the same value that a non-SGB would return when no buttons are selected.

	ld a, JOYP_SGB_ZERO
	ldh [rJOYP], a
...
	ld a, JOYP_SGB_FINISH
	ldh [rJOYP], a
...
	ld a, JOYP_SGB_ONE
	ldh [rJOYP], a

This does not actually represent what that code is doing. It's not sending the SGB data at this point, but rather doing a "regular" read of the joypad, which also increments the player index at some stage, maybe when the selected button group changes from JOYP_GET_BUTTONS to JOYP_GET_NONE. (I forgot when exactly this event happens.)

Using the data transfer constants is obscuring what the code actually does. (The added delays also obscure what the code does. They shouldn't be needed, although that's Nintendo's fault and a disassembly can't do much about that.) I'd suggest using the regular JOYP_GET_BUTTONS/JOYP_GET_CTRL_PAD/JOYP_GET_NONE constants in that routine in pokered, as that better represents what the code does.

For hardware.inc, I'd suggest constants with names along these lines:

DEF JOYP_SGB_MLT_MASK EQU %000000_11
DEF JOYP_SGB_MLT_P1 EQU %000000_11
DEF JOYP_SGB_MLT_P2 EQU %000000_10
DEF JOYP_SGB_MLT_P3 EQU %000000_01
DEF JOYP_SGB_MLT_P4 EQU %000000_00

[ISSOtm]

TODO: once this is merged, replace gb-starter-kit's.

[dmitry-shechtman]

Now that I'm thinking about it, I believe this warrants a separate repo.
It doesn't depend on hardware.inc, and should be versioned separately (hopefully, not at all).

[ISSOtm]

I think this topically belongs with hardware.inc: even if not a dependency, it's a good companion.

[dmitry-shechtman]

It is a companion, but should it be mandatory?

The correct way of adding hardware.inc to one's repo is via a submodule. Most users will probably never use sgb.inc, so it will end up just sitting there, polluting their file system.

[ISSOtm]

Arguably, using a submodule is not great, since it introduces an additional point of failure over just vendoring the file. (I prefer it for my own use, but it's less obvious for libraries and such.)

Also, using a submodule also unnecessarily imports the README and LICENSE, so... eh?

Meanwhile, centralising the resources improves their discoverability.

[dmitry-shechtman]

I never actually used submodules so far, so I'm wondering what you mean by them being a point of failure.

As for README and LICENSE, those are necessary evils. I'd actually put the mandatory part in src and have that as the submodule's root (although I'm not sure that's technically possible).

[ISSOtm]

I never actually used submodules so far, so I'm wondering what you mean by them being a point of failure.

They require special Git flags to operate (git clone --recursive, git submodule update --init), they look like normal subtrees but operate as a single unit from the point of view of the superproject. I've seen them trip up newbies often, and tbh it's one of the gb-starter-kit's pitfalls and main friction points. (Note also that they are not included in GitHub's “Download as ZIP” functionality for technical reasons, which therefore mandates the use of Git.)

As for README and LICENSE, those are necessary evils. I'd actually put the mandatory part in src and have that as the submodule's root (although I'm not sure that's technically possible).

It's unfortunately not possible. Submodules are always entire repos (and you can't have sparse submodules, either).

Subtrees might solve the issue, but I've never experimented with them.

[Rangi42]

Anyone whose game has an SGB border would have a use for sgb.inc. As for "polluting" the file system with unused files, the same is also true of hardware_compat.inc (and for that matter, README.md, .github/workflows/testing.yml, etc) when using a submodule.

The benefit of keeping sgb.inc in the same repo as hardware.inc is making them discoverable together, maintaining the same naming/formatting conventions for both, and keeping them in sync for releases.

[dmitry-shechtman]

Anyone whose game has an SGB border would have a use for sgb.inc.

Which is probably 5% of projects, or even less until they reach advanced stages in their development?

and for that matter, README.md

Already addressed above.

.github/workflows/testing.yml, etc)

Hidden directories don't count ;)

the same is also true of hardware_compat.inc

I'd argue that that should actually be a separate repo as well.

The benefit of keeping sgb.inc in the same repo as hardware.inc is making them discoverable together

Agreed.

maintaining the same naming/formatting conventions for both

No reason not to, regardless of how they are organised.

and keeping them in sync for releases.

That's precisely my point. You don't want them in sync!