Fix a symbol/data mismatch in wLinkPlayer*, and mailmsg

[Narishma-gb]

Fixes #140

MAIL_MSG_LENGTH is redefined as 2 * MAIL_LINE_LENGTH, so that it's still linked to the mailmsg macro.

[Narishma-gb]

Converted to draft. I'd like to work on what's happening to the received party data during the link session, which shouldn't use wOTPartyData, as it's not the same data format.

[Narishma-gb]

I went a bit further and arranged some symbols for party mail, on top of enemy party data.

Here's how it goes, for SECTION UNION "Overworld Map" that cover link data

• One UNION for the whole buffer (unchanged)
• One UNION for the actual exchange, when the game has Serial_ExchangeBytes running. The sent data is formatted with preamble bytes (and differ if it's Gen 2 or TC session), the received data has only raw data and don't contain symbols, because we don't know yet what the data stream contains.
• One UNION for the temporary data formatting: that's where we have wLinkPlayerName, and 2 party structures, whether it's Gen 2 or TC. Gen 2 also has temporary mail structure, if it's using Trade Center.
• One UNION for the final mail structure, the only one that remains inside "Overworld Map". It's using mailmsg macro structure. It uses wLinkOTMail prefix. This way, OT has the implication of actual game data.
• One UNION for wLinkOTPartyMonTypes, that's used later when doing the actual trade (unchanged).

For the opponent player's party, the received raw data is stored at the far end of WRAM, at wLinkReceivedPartyData. I had to change the size of SECTION "Stack", due to some byte being written to at wStackBottom - 1 (this is confirmed with an emulator and memory viewer).
Final Enemy party goes to wOTPartyData, so that a battle can start if it's Colosseum.

Symbols have been updated in link.asm, with more comments.

• For raw data: wLinkSend, wLinkReceived
• For data being processed: wLinkPlayerParty, wLinkReceivedMailMessages (perhaps could use wLinkPlayerMailMessages for consistency?)
• For final data: wLinkOTMail, wOTParty

There are some necessary but not very satisfactory changes:

• wPokemonDataEnd and wGameDataEnd have to go inside a union, because the maximum data stream received at wLinkReceivedPartyData is longer.
• I didn't know how to make it clear in comments where data refers to the sender, and where it's received data. I used "other Game Boy" a lot, because "player" usually refers to this Game Boy, and "opponent" feels weird in the Trade Center.
• I created LINK_PARTY_DATA_LENGTH and LINK_TC_PARTY_DATA_LENGTH to make some lines of code shorter, but it's still the big math formula. However, it may be interesting after all, helps understand what's really exchanged over the link cable.

[Narishma-gb]

The latest addition is a reformat of wLinkSendMailPatchSet and its neighbours.
There is no preamble data inside, as this is not the start of a new link stream, but continuation of the mail data exchange. The first byte at this address is the first offset of mail metadata to be patched.
One byte is reserved for the $ff terminator, so that leaves 17 bytes for each mail. It's strangely bigger than the patched metadata, which is only 14 bytes (NAME_LENGTH - 1 + player ID + mon + mail type).

[Rangi42]

@Narishma-gb If this is entirely done, please unmark as draft and I'll get around to reviewing it. Thanks!

[Narishma-gb]

One last addition, where wLinkData is now only used when clearing the whole buffer.

[Rangi42]

Great work with this one! IMO it's ready to approve + squash + merge as-is, but still have some ideas.

1. I'm not totally sure which of the code in engine/link/link.asm would need modifying if ds SERIAL_PADDING_LENGTH ; unused but written to were removed. The wTimeCapsulePatchedData label is referenced in a few places, and the step-by-step nature of how data gets read/written makes it hard to see where it goes a little too far. A comment at the code which writes to those bytes would help.

2. It's very helpful to not be using the generic wLinkData buffer any more... except for in ClearLinkData, since the point is to clear "the whole multi-use link data buffer". But since we're using a whole SECTION UNION just for that buffer, do we really need a generic label at all? How about this:

   	ld bc, STARTOF("Overworld Map") + SIZEOF("Overworld Map") - wLinkReceivedMail ; should be wLinkReceivedMailEnd - wLinkReceivedMail

   ClearLinkData:
   	ld hl, STARTOF("Overworld Map")
   	ld bc, SIZEOF("Overworld Map")

   That would match how we clear the "Miscellaneous" buffer in engine/games/unown_puzzle.asm. More importantly, it would guarantee that ClearLinkData clears the whole buffer -- as-is, someone could conceivably add more than 1300 bytes to one of the SECTION UNIONs, and leave wLinkData not covering the whole thing.

   The main reason I'd hesitate is that the section name, "Overworld Map", is not obviously link related. But IMO people deal with map size limits a lot more often than link data buffer size limits (e.g. you make a 30x31-block map and now you're overflowing the buffer), so that's the "primary" purpose of the whole SECTION UNION. What do you think?

[Narishma-gb]

1. I added a comment to the relevant location:
   ; this should be "ld bc, LINK_TIME_CAPSULE_PARTY_DATA_LENGTH"

2. I moved wLinkData within the longest link union. This removes the need for an assertion, while avoiding the use of section name "Overworld Map", which I agree feels a bit out of place there. It also removes the hardcoded 1300.
   I think it's fine if the Overworld Map grows beyond the vanilla-size in a rom hack. ClearLinkData only needs to erase enough bytes for its own data, but can leave the end of the buffer untouched.
   Would this be a suitable compromise?

3. Also found a couple of hardcoded values, mainly inside Link_ConvertPartyStruct1to2, which have been turned into constants.

[Rangi42]

Thanks, that does look suitable!

The data that gets "temporarily stored here before applying patch data" can be for regular link or for Time Capsule link. In both cases it starts with the player's name and party species, but those interior labels don't actually get used. So I don't think we need them shared with a UNION+NEXTU afterwards. Instead, I refactored so the regular data and the Time Capsule data each get their own SECTION UNION. I think this also makes it clearer just where the ds SERIAL_PADDING_LENGTH ; unused is written to (where it calls Link_CopyOTData with wLinkTimeCapsulePartyDataEnd - wLinkTimeCapsulePartyData, since the unused ds is inside those enclosing labels).

[Rangi42]

Turns out that the wLinkReceivedMailMessages/wLinkReceivedMailMetadata/wLinkReceivedMailPatchSet labels go right between the wLinkReceivedMail/wLinkReceivedMailEnd labels. :P

[Rangi42]

@Narishma-gb I don't want to merge my own changes without them being looked over first, and also, I think the labels can still be improved/shortened. In particular, the "wLinkSendPartyPartyCount/wLinkSendPartyPartySpecies/wLinkSendPartyPartyEnd" because "wLinkSendPartyEnd" already ends the greater chunk of data (and likewise for wLinkReceived...).

(Also I don't mean to unilaterally refactor/replace parts of your PR, it's just easier to commit changes than describe them -- plus the process of making the changes helps discover what is/isn't an improvement, e.g. the location of wLinkReceivedMailMessages. We can edit/revert anything I did wrong.)

[Rangi42]

(Game Freak reused gein_cap_data aka our wOTPartyMons for their "demo" battle aka Dude battle, so it's unavoidable that the wDude* labels are UNIONed with that. ☹️)

[Rangi42]

I'm tempted to add these two macros (or at least the first party_species_list one):

MACRO party_species_list
\1PartyCount::   db
\1PartySpecies:: ds PARTY_LENGTH
\1PartyEnd::     db ; older code doesn't check PartyCount
ENDM

MACRO party_mons_with_names
\1PartyMons::
; \1PartyMon1 - \1PartyMon6
for n, 1, PARTY_LENGTH + 1
\1PartyMon{d:n}:: \2party_struct \1PartyMon{d:n}
endr
\1PartyMonOTs::
; \1PartyMon1OT - \1PartyMon6OT
for n, 1, PARTY_LENGTH + 1
\1PartyMon{d:n}OT:: ds NAME_LENGTH
endr
\1PartyMonNicknames::
; \1PartyMon1Nickname - \1PartyMon6Nickname
for n, 1, PARTY_LENGTH + 1
\1PartyMon{d:n}Nickname:: ds MON_NAME_LENGTH
endr
ENDM

There are seven places they would be useful, not just in link code, e.g.:

; player's party data, formatted for link transfer (Gen 2 link session)
wLinkSendParty::
wLinkSendPartyPreamble:: ds SERIAL_PREAMBLE_LENGTH
wLinkSendPartyPlayerName:: ds NAME_LENGTH
wLinkSendPartyList:: party_species_list wLinkSendParty
wLinkSendPartyPlayerID:: dw
wLinkSendPartyMons:: party_mons_with_names wLinkSendPartyPlayer, ,
wLinkSendPartyPadding:: ds SERIAL_PADDING_LENGTH
wLinkSendPartyEnd::

; after the initial link session, the other player's party data
; is temporarily stored here before applying patch data
wLinkTimeCapsulePartyData::
; link player's name and party species are not patched,
; as they normally don't contain SERIAL_NO_DATA_BYTE
wLinkTimeCapsulePlayerName:: ds NAME_LENGTH
wLinkTimeCapsulePartyList:: party_species_list wLinkTimeCapsuleParty
wTimeCapsulePatchedData:: party_mons_with_names wTimeCapsule, red_
wLinkTimeCapsulePartyPadding:: ds SERIAL_PADDING_LENGTH
wLinkTimeCapsulePartyDataEnd::

; enemy party
wOTPartyData::
wOTPlayerName:: ds NAME_LENGTH
wOTPlayerID:: dw
	ds 8
wOTPartyList:: party_species_list wOT

UNION
; ot party mons
wOTPartyData:: party_mons_with_names wOT, ,
wOTPartyDataEnd::

[Narishma-gb]

I have no issue with anyone adding changes to the PR, I left it open so it could happen. And it was a modest goal initially, just aligning some data in WRAM.
Here are my thoughts on the recent additions:

• I really like the new placement of the end labels wGameDataEnd, wPokemonDataEnd, that's where I should have put them initially, I didn't think about the larger union.
• Interior labels make it easier to understand the link data format, compared to the long math string that was LINK_PARTY_DATA_LENGTH or LINK_TIME_CAPSULE_PARTY_DATA_LENGTH; same with end labels that come with it.
• The only change I would advise against, is merging the union wLinkReceivedMailMessages... wLinkReceivedMailPatchSet within wLinkReceivedMail. I think it was initially located there, and I split them on purpose to make a difference between raw data received over the link cable, and processed data that's ready for use.
• This is fine with wLinkSendParty on the other hand, because prep data is exactly as we have written it.

As I understand, it's possible for a link data stream to be slightly off by a byte or so, compared to what is expected. This is due to the fact that one gameboy decides when data should be sent/received, and assumes the other is also ready when it is. The first sent byte could be lost, and the first received byte could be NO_DATA, if the HW register wasn't set in time. That's why there are a few padding bytes at the beginning and the end, to allow data to be received slightly early or beyond the expected end boundary, without losing any meaningful content.
For that reason, I wouldn't use inside labels for any raw data that's just been received, until it's been processed and aligned in its next location.
For the mail data specifically, wLinkReceivedMail holds SERIAL_MAIL_PREAMBLE_LENGTH, until later, when this same address is reused to write wLinkReceivedMailMessages. That's the main reason I split these. The data is not aligned with the symbols during the initial link exchange. I consider these 2 buffers that overlap for size optimization.

wLinkReceivedMail::
  ds SERIAL_MAIL_PREAMBLE_LENGTH
  ds (MAIL_MSG_LENGTH + 1) * PARTY_LENGTH
  ds (MAIL_STRUCT_LENGTH - (MAIL_MSG_LENGTH + 1)) * PARTY_LENGTH
  ds (MAIL_STRUCT_LENGTH - (MAIL_MSG_LENGTH + 1) + 3) * PARTY_LENGTH + 1
wLinkReceivedMailEnd::
  ds 10

(No padding bytes at the end, but the mail patch list is already longer than its maximum used size).

The same reasoning applies to wLinkReceivedPartyData:

wLinkReceivedPartyData::
  ds SERIAL_PREAMBLE_LENGTH
  ds NAME_LENGTH
  ds 1 + PARTY_LENGTH + 1
  dw
  ds (PARTYMON_STRUCT_LENGTH + NAME_LENGTH * 2) * PARTY_LENGTH
  ds SERIAL_PADDING_LENGTH
wLinkReceivedPartyEnd:: db

There is no mention of Time Capsule here, I don't know if it's worth mentioning (TC data being shorter, it would be included within wLinkReceivedPartyEnd - wLinkReceivedPartyData).

Perhaps labels aren't well chosen, and should reflect the difference between raw and processed data? I used Received in this case, and not in other UNIONs. Although I kept wLinkReceivedMailMessages as it was named initially. It's not the final state of mail messages, as there is yet another change to format them again to mail_msg structure.

[Rangi42]

@Narishma-gb Sorry to have left this so long. If you want to fix/correct/undo anything first (e.g. merging the union wLinkReceivedMailMessages... wLinkReceivedMailPatchSet within wLinkReceivedMail, or labels to clearly reflect the difference between raw and processed data) go ahead, I'll try not to be nitpick on subsequent review and just get it merged, since overall this is clearly an improvement.

[Narishma-gb]

No worries, I was planning to get back to this one, thank you for the ping! I should be able to push a final update this month, and apply it to pokecrystal. Finding meaningful symbols is definitely not easy, especially in a foreign langage.
As always, thanks for your reviews! 😄

[Narishma-gb]

Apologies for adding yet more UNIONs. I went for nameless blocks of data, to mark the areas in WRAM where link data is initially received. As soon as the actual data start is identified, I use symbols instead.

I relocated Gen 1 & Gen 2 comments on the processed data at the top of the UNION for each.

wLinkReceivedPartyEnd's write is identified in engine/link/link.asm. Only Gen 2 trade writes it there, because Time Capsule data being shorter, this final byte is still within the UNION.

Also changed wLinkPartyData to wLinkPlayerPartyData (Gen 2 trades), it seems more similar to the symbols immediately below.

On my end, it looks like final. There's room for improvement on the symbol names, but maybe it doesn't need to be right now.

[Rangi42]

Thanks a lot! The wLinkReceivedPartyData UNION makes sense. Once pret/pokecrystal#1224 is updated the same way we can merge both.