MelonDS system bus 16 and 32 bit reads do not work

[SuuperW]

Summary

MelonDS has functions to read and write 8, 16, and 32 bit values from the ARM7 and ARM9 system buses. However, BizHawk only uses the 8 bit versions of these functions. This is a problem because some memory addresses cannot be read from or written to with the 8 bit versions, but can with 16 or 32 bit versions. This is a problem that I had previously fixed with commit 99141e8, but has regressed since MelonDS has been Waterboxed.

Repro

1. Start BizHawk debug build, so that you can see the console for MelonDS output
2. Open any ROM in the MelonDS core
3. Run a Lua script that contains this line: memory.read_u16_le(0x01000000, "ARM7 System Bus")

Output

The console window will display two lines beginning with "unknown arm7 read8", indicating that BizHawk called NDS::ARM7Read8 twice instead of NDS::ARM7Read16 once.

[CasualPokePlayer]

This is more or less just a restriction with the built in waterbox GetMemoryAreas function, which doesn't give that sort of information in the parameters of a function hook, so there isn't a way to know what width to use (besides guessing with the amount of bytes to read, which is given, but that isn't guaranteed to be 1/2/4 due to Hex Editor). Also, most (all?) of what would be blocked would be I/O, which is likely to have side effects on read and might have to be blocked regardless. I'm not sure if changing widths should be calling different read functions regardless, optimally it would be not blocked in any width.

Not sure of a solution here, adding in the parameter for width would require modification of any wbx core using function hooks, and a custom IMemoryDomains implemention would just be hacky. I don't know if changing widths should be changing read results, optimally it would be the same for any width for peeks.

Also going to note your repro isn't exactly great when that address would also be unknwon as a single 16 bit read.

[SuuperW]

It is true that IO might have side effects, but I don't see that as a reason to block access to these reads and writes for Lua scripts. Some of what is being blocked here is also GPU addresses. I also don't see how this is an inherent issue with Waterbox, since the reads and writes in this case are being handled by MelonDS functions (as shown by the console window displaying the unknown read message, which is from MelonDS and not Waterbox). Maybe this is due to me not understanding what Waterbox does, as I have no clue how or even what it's doing, but it seems to me like BizHawk's MelonDS core could include calls to MelonDS's 16 and 32 bit read/write functions. I just don't know how to do that myself, because Waterbox is a mystery to me.

My repro steps were only to demonstrate that MelonDS's 8 bit read function is being called when Lua attempts to write a 16 bit value. I was attempting to write a Lua script to read/write the MAC address, in order to test some RNG stuff. I opted not to share that because I can't be certain that my script would work without this issue, as that obviously hasn't been tested. If you would like an example of something that this issue breaks, here is a Lua script that I have that should display inputs for AVI recording and has worked on previous versions of BizHawk: NSMB_Input.txt. I have modified it to use current domain names. This script will not show unknown read/write messages in the debug console, however, because MelonDS recognizes those addresses and simply ignores the write requests. Why it ignores them instead of writing in this particular case, I don't know.

[CasualPokePlayer]

I have handling in BizInterface to block reading addresses with side effects, if 16/32 bit reads are added any addresses with side effects will be blocked accordingly, as to ensure determinism.

It's not technically a limitation with Waterbox but rather WaterboxCore, which is a base implementation on the c# side for waterbox cores to use, and that has the GetMemoryAreas function imported and used to handle IMemoryDomains. System Buses have a function hook given to the c# side to call, and width is not a parameter given to that function hook. A solution here is to add that as a parameter, but even so it's questionable itself to blocking reads depending on width, optimally width should not be considered for peeks.

[nattthebear]

I'm not against an addition to WaterboxCore if needed

[YoshiRulz]

I think you should ask upstream for a separate peek function rather than make a best effort to prevent side-effects.