Further improvements on the SGB border guide (#71)

PR #70 ended with different suggested changes stomping on one another.
Finish the last few tasks mentioned in that PR's discussion.

- fix a comma splice
- explain why wait 4 frames
- emphasize why the screen must be on
- use line comments (;) instead of C block comments (/*...*/)
- transform detection code into a subroutine to eliminate `.init`
- rewrite detection code to use a busy wait loop instead of vblank interrupt
- let the lines settle after each read in case A+Right held on DMG
- store the detection result to a variable
- Remove mention of use of palettes 1-3 and 7

---------

Co-authored-by: Antonio Vivace <[email protected]>

Author: pinobatch

website/guides/sgb_border.md

@@ -4,13 +4,13 @@ This document aims to help developers of DMG-compatible homebrew with adding Sup
 
 We will see how to:
 
-- Detect whether we are running on a SGB
+- Detect whether the program is running on a SGB
 - Transfer the border's tiles
 - Transfer and display the border's tilemap and palettes
 
 <small>
 
-Written by [sylvie (zlago)](https://zlago.github.io/me/), idea (and minor help) by [valentina (coffee bat)](https://coffeebat.neocities.org/), reviews and improvements by [ISSOtm](https://eldred.fr) and [avivace](https://github.com/avivace).
+Written by [sylvie (zlago)](https://zlago.github.io/me/), idea (and minor help) by [valentina (coffee bat)](https://coffeebat.neocities.org/), reviews and improvements by [ISSOtm](https://eldred.fr), [avivace](https://github.com/avivace), and [PinoBatch](https://github.com/pinobatch).
 
 </small>
 
@@ -41,9 +41,9 @@ An SGB packet consists of:
 
 You must set `P1` to `%xx11xxxx` between each pulse.
 
-This adds up to 16 Bytes of data (LSB first), If a packet doesn't read all 16 bytes, the unused bytes are ignored.
+This adds up to 16 bytes of data (LSB first). If a packet doesn't read all 16 bytes, the unused bytes are ignored.
 
-**You should wait 4 frames between each packet.**
+**You should wait 4 frames between each packet and the next.** This gives the SGB BIOS a chance to receive a packet even if it is doing something else time-consuming.
 
 For an example of such routine, see this [code](https://github.com/zlago/violence-gbc/blob/11cfdb6ee8a35e042fa9712484d814e0961cea7c/src/sub.sm83#L413-L463) and the related Pan Docs entry: [SGB Command Packet on Pandocs](https://gbdev.io/pandocs/SGB_Command_Packet.html).
 
@@ -53,7 +53,7 @@ This guide glosses over a minor detail, as certain packets can be (albeit unccom
 
 Bulk transfer (TRN) packets tell the SGB to copy the contents of the screen to buffers in Super NES work RAM. The `CHR_TRN` and `PCT_TRN` packets are used to send data for SGB borders.
 
-For that to function properly, you must:
+For a transfer to function properly, you must prepare VRAM and the LCD registers:
 
 1. set `BGP` to `$e4` and `LCDC` to `$91` (screen enabled, BG uses tiles `$8000`-`$8fff` and tilemap `$9800`, WIN and OBJ disabled, BG enabled)
 2. set `SCX` and `SCY` to `$00`
@@ -62,77 +62,91 @@ For that to function properly, you must:
 
 You can do 1, 2 and 3 via [this snippet](https://github.com/zlago/snek-gbc/blob/baef0369f57d2b0d58316cb1c28c6cc22475a6c9/code/init.sm83#L208-L230)
 
-- **You must load the data into VRAM and enable the screen before sending the TRN packet**
-- **You must wait ~8 frames after each TRN** instead of just 4
+- **You must load the data into VRAM and enable the screen before sending the TRN packet.** The SGB reads TRN payloads from the screen. If rendering is off, there is nothing on the screen to read.
+- **You must wait ~8 frames after each TRN** instead of just 4. The SGB BIOS has to finish what it's doing, receive the packet, and then read the screen.
 
 ## Detecting SGB
 
 Here's how a SGB detection routine should look like:
 
-- wait 12 or more frames after the console boots
-- send a `MLT_REQ`, selecting 2 or 4 players (`$89, $01` or `$89, $03`), and wait at least 1 frame
-- attempt to advance the read player (set `P1.5` to 0 then 1)
-- set `P1.4`-`P1.5` to `%11` (`%xx11xxxx`)
-- read `P1.0`-`P1.3`, and check if it either
-	* has changed
-	* isn't `%1111`
-- advance and reread a few times and branch somewhere if the test keeps failing
+1. Wait 12 or more frames for the SGB BIOS to start listening for packets.
+2. Send a `MLT_REQ` packet selecting 2 or 4 players (`$89, $01` or `$89, $03`), and wait a couple frames for the SGB to receive the packet.
+3. Read the controller. Set `P1` bit 5 to 0, then set `P1` bits 5 and 4 to `%11` (`%xx11xxxx`) to release the key matrix.
+4. Read the low nibble (bits 3-0) of `P1`, and look for a value other than `%1111` (which indicates player 1).
+5. If player 1 was still found, repeat steps 3 and 4 once more, in case the next read indicates player 2.
+6. Optionally turn off multiplayer mode.
 
-Let's go over an example snippet ([source](https://github.com/zlago/snek-gbc/blob/baef0369f57d2b0d58316cb1c28c6cc22475a6c9/code/init.sm83#L167-L196)):
+If a non-`%1111` value was found in step 4 either time, the program is running on SGB.
+
+A routine like this may be used to detect SGB (modified from [source](https://github.com/zlago/snek-gbc/blob/baef0369f57d2b0d58316cb1c28c6cc22475a6c9/code/init.sm83#L167-L196)):
 
 ```sm83asm
-; test for SGB
-xor a /* first it enables STAT interrupt, since snek-gbc happens to just have a `reti` as the handler */
-ldh [rLYC], a
-ld a, STATF_LYC
-ldh [rSTAT], a
-ld a, IEF_STAT
-ldh [rIE], a
-ei
-	; wait for SGB
-	ld b, 12 /* You must wait 12 or more frames before trying to send a packet */
-	:halt
-	dec b
-	jr nz, :-
-	; enable multi
-	ld hl, Packets.mlt /* send a `MLT_REQ` */
-	call Packet
-	rept 4 /* then wait 4 more frames, since the SNES won't "read" the packet instantly */
-		halt
-	endr
-	; check if SGB responds
-		/* and now We actually try to detect the SGB
-		setting `P1.5` to low then high advances the "read player"
-		setting `P1.4` and `P1.5` high will make the SGB return which player is currently selected in `P1.0` and `P1.1` */
-		lb bc, 5, LOW(rP1) /* b is loaded with 5 which is how many times We try to check if the player changes to anything but P1 */
-		:ld a, P1F_4 /* try to advance player */
-		ldh [c], a
-		ld a, P1F_4|P1F_5 /* then set P1 to return the current player */
-		ldh [c], a
-		ldh a, [c]
-		dec b
-		jp z, .init ; give up /* if 5 (4 actually) attempts fail, assume this isn't an SGB */
-		cp $ff /* `P1.6` and `P1.7` always return %1, We set `P1.4` and `P1.5` to %1, and DMG, or SGB player 1, return %1111 in `P1.0`-`P1.3` */
-		jr z, :- ; /* try again if detection fails */
+SGB_Detect:
+  ; test for SGB
+  di
+  call SGB_Wait4Frames
+  call SGB_Wait4Frames
+  call SGB_Wait4Frames
+
+  ; Send MLT_REQ packet to enable multiplayer
+  ld hl, Packets.mltOn  ; send MLT_REQ for 2 players
+  call SGB_SendPacket
+  call SGB_Wait4Frames
+
+  ; Detect the SGB by checking if SGB responded to MLT_REQ.
+  ; Setting P1.5 to low then high advances the selected player.
+  ; Setting P1.4 and P1.5 high causes the ICD2 to reflect the
+  ; selected player in low bits of P1.
+  ld b, 4              ; Number of attempts
+  ld c, LOW(rP1)       ; Address to write
+  .loop
+    ld a, P1F_4        ; Try to advance player
+    ldh [c], a
+    ld a, P1F_4|P1F_5  ; Set P1 to return the selected player
+    ldh [c], a
+    ; In case this is DMG and not SGB, let the input lines settle
+    ; for a few cycles before reading P1 again
+    call SGB_Wait4Frames.knownRet
+
+    ldh a, [c]         ; Player 1 has A.0 = 1; players 2 and 4 have A.0 = 0
+    cpl                ; Invert this
+    and %00000001      ; Keep only bit 0, which is 0 for player 1 or nonzero for players 2 and 4
+    jr nz, .done
+    dec b              ; Keep trying until we've cycled through all players
+    jr nz, .loop
+  .done
+  ; After this loop, A is $01 for SGB or $00 for not SGB.  Remember this
+  ld [wIsSGB], a
+
+  ; (Optional) Disable multiplayer
+  ld hl, Packets.mltOff  ; send MLT_REQ for 2 players
+  call nz, SGB_SendPacket
+  ret
+
+SGB_Wait4Frames:
+  ld bc, -(456 * 154 / 16 * 4)
+  .loop
+    inc c  ; inner loop takes 16 T-states per iteration
+    jr nz, .loop
+    inc b
+    jr nz, .loop
+  .knownRet
+  ret
 ```
 
-If you adopt this, remember to change `.init` to wherever code should jump to if it's _not_ on SGB, and put code meant to run on SGB after the snippet.
-
 ### SGB detection notes
 
-- It would be a good idea to save somewhere in RAM whether the game is running on an SGB capable device or not, such that if you wish to change the border mid-gameplay, you won't have to perform SGB detection again
-
+- It would be a good idea to save somewhere in RAM whether the game is running on an SGB capable device or not, such that if you wish to change the border mid-gameplay, you won't have to perform SGB detection again.  The sample code stores it in `wIsSGB`.
 - **If you wish to only use 1 controller for the game, you will have to send another `MLT_REQ` to disable multiplayer** (`$89, $00`)
-
-- You can change the waitloop to not use interrupts, or to use di+halt instead
+- `SGB_Wait4Frames` above uses busy waiting. Depending on the structure of your initialization code, you can change it to use vblank interrupts or `di`+`halt` instead.
 
 ## Border limitations
 
 An SGB border has:
 
 - 255 tiles + 1 transparent tile (preferably tile #0)
 - 3 palettes of 15 (+ 1 transparent) colors, (up to 45 solid colors total)
-- a 256x224px tilemap (there's a bit more to this, see [notes](#notes))
+- a 256×224-pixel tilemap (there's a bit more to this, see [notes](#notes))
 
 ## Converting borders
 
@@ -174,7 +188,7 @@ See also the related Pan Docs entry: [SGB Command Border](https://gbdev.io/pando
 
 1. You can set the first row of tiles to your transparent color to force superfamiconv to put the transparent tile as the 1st tile, however you must then exclude 64 bytes of the tilemap (`incbin "border.pct"` -> `incbin "border.pct", 64`)
 
-2. You can use palettes 0-3 and 7, if you really know what you're doing (animated borders yay). You will probably have to edit the border in YY-CHR, as there aren't really any other tools for that.
+2. SGB BIOS reserves palettes 4 through 6 for borders. If you really know what you're doing, you may be able to use palette 0 (the gameplay palette) for animated borders. You will probably have to edit the border in a tile editor such as YY-CHR, as there aren't yet any other tools for that.
 
 3. When the SNES lags, scanline 225 of the SGB border will be visible! You can set the topmost row of the 29th row of tiles to black to hide this.