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!

[avivace]

@dmitry-shechtman could you check if you have Allow edits from other maintainers in the PR? We can't seem to be able to apply suggestions etc

[dmitry-shechtman]

@dmitry-shechtman could you check if you have Allow edits from other maintainers in the PR? We can't seem to be able to apply suggestions etc

I don't see the option.

[avivace]

@dmitry-shechtman it should be on this page, in the top right options:

[dmitry-shechtman]

Sadly, my sidebar has no such option.

[avivace]

@dmitry-shechtman alright! thanks for checking :/ I suspect this is happening because you are PRing from an organisation, not your personal namespace, and maybe that weirds out GitHub permission system on PR branches..

Can you try to give me (@avivace) and @Rangi42 the 'Maintain' permission on the repo you PRed from (https://github.com/TheLeanArt/hardware.inc) ?

[Rangi42]

Thank you, I'm able to commit to the PR branch now! I'm just getting some formatting nitpicks out of the way, and may have a few more significant questions/suggestions after that.

[avivace]

Thanks @dmitry-shechtman !

[Rangi42]

Thank you for your patience with this PR! hardware.inc doesn't get frequent updates, but we are looking forward to 6.0 soon, and I'd like this to be a part of it.

[Rangi42]

If I understand https://gbdev.io/pandocs/SGB_Command_Multiplayer.html#sgb-command-11--mlt_req correctly, should these be binary %00, %01, and %11?

[Rangi42]

This does not add constants for the original hardware.inc request in #76, covering the MLT_REQ data which can be read from the P1/JOYP register.

We already define some SGB-related constants in hardware.inc itself -- including the B_JOYP_SGB_ONE/ZERO and JOYP_SGB_START/FINISH bits essential to all SGB commands -- so that may be the correct place for them after all?

On the other hand, if multiple SGB command types all reuse the P1 register for reading certain command-specific values (I don't know, haven't read about every single one) then sgb.inc would IMO be the correct place, to avoid bloating hardware.inc's JOYP section with mostly SGB-related content.

[Narishma-gb]

I don't think any command expects to read a return via rJOYP. In case of MLT_REQ, joypad reads are now "signed" with the controller number being polled, but as it is permanent, it's a side effect of the command.
I'd suggest relocating B_JOYP_SGB_ONE, JOYP_SGB_START, ... here, because in order to transfer a packet to the SGB, this file will likely be included since it conveniently has the command definition. Besides, it would be very surprising to check for SGB hardware, and do nothing else with it.

[Rangi42]

ATTR_BLK ($04), ATTR_LIN ($05), ATTR_DIV ($06), and ATTR_CHR ($07) all go in-between here, and have A_* byte offsets (and significant bit/mask values for some of those bytes' meanings) which could be defined here. Same goes for the other command types which aren't yet addressed.

[Rangi42]

I don't have much SGB-to-SNES programming experience at all -- I've only dealt with the commands necessary for 4bpp SGB borders -- so may not be the best person to thoroughly review this. However, I can't really tell how the A_* constants are meant to actually be used. They're neat as documentation, but would assembly code ever have the hard-coded numbers which these would replace?

[Narishma-gb]

SNES_TILEMAP_WIDTH/HEIGHT could be a nice addition to sgb.inc
For PCT_TRN VRAM data, it's mandatory that the border palette data is offset by 32 * 32 * 16 bits, so that color data is correctly read.

[Narishma-gb]

For the PCT_TRN command data, these constants would be useful:

DEF SGB_XFLIP EQU 1 << 6
DEF SGB_YFLIP EQU 1 << 7

DEF SGB_PAL4 EQU 4 << 2
DEF SGB_PAL5 EQU 5 << 2
DEF SGB_PAL6 EQU 6 << 2

A typical tilemap entry can then be written as db $xx, SGB_PAL4 | SGB_XFLIP