Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Value from mem address is not passed to read callback #3813

Open
VelpaChallenger opened this issue Oct 28, 2023 · 5 comments · May be fixed by #3821
Open

Value from mem address is not passed to read callback #3813

VelpaChallenger opened this issue Oct 28, 2023 · 5 comments · May be fixed by #3821
Labels
Core: BSNES Super Nintendo Entertainment System (SNES) / Super Famicom (SFC) / Super Game Boy (SGB) core Core: Genplus-gx Sega Genesis / Mega Drive core Enhancement For feature requests or possible improvements re: APIHawk Relating to EmuHawk's public .NET API or to the creation of external tools re: Lua API/scripting Relating to EmuHawk's Lua API (not the Lua Console) Repro: Fixed/added in branch/fork
Milestone

Comments

@VelpaChallenger
Copy link

VelpaChallenger commented Oct 28, 2023

Summary

When running a lua script which registers a memory callback function to the on_bus_read hook, and the core calls the callback function, the value from the memory address is not passed.

So far I found it happens with BSNES and Genplus-gx, but it could potentially happen with other cores as well. Also with every game, for example Virtual Bart (Sega Genesis version, that is core Genplus-gx) and Contra 3: The Alien Wars (for BSNES). Both memory addresses are for the RNG. Contra 3: The Alien Wars' RNG starts advancing as soon as the Konami presentation starts (that is, before the title screen) and Virtual Bart's after the roulette starts spinning (start the game, skip the cutscene, then wait a few moments until the roulette starts spinning, you can also go to the practice area which makes the RNG advance as well).

Repro

  1. Run Lua script which registers a memory callback function to on_bus_read hook (see below Lua script)
  2. Go to section of the game which reads to the memory address passed to hook

Lua script:

function my_callback(addr, val, flags)
    print("addr being read is" .. string.format("%x", addr))
    print("value is" .. string.format("%x", val))
    print("flags is" .. flags)
    client.pause()
end

event.on_bus_read(my_callback, 0xFFFFC676) --FFFFC676 for Virtual Bart, E6 for Contra 3: The Alien Wars

Output

The value printed to the console is always 0.

Host env.

  • BizHawk 2.9.1 Win10 AMD
@YoshiRulz YoshiRulz added Enhancement For feature requests or possible improvements Core: Genplus-gx Sega Genesis / Mega Drive core Core: BSNES Super Nintendo Entertainment System (SNES) / Super Famicom (SFC) / Super Game Boy (SGB) core re: Lua API/scripting Relating to EmuHawk's Lua API (not the Lua Console) re: APIHawk Relating to EmuHawk's public .NET API or to the creation of external tools labels Oct 28, 2023
@Morilli
Copy link
Collaborator

Morilli commented Oct 28, 2023

I believe that is actually somewhat intentional, as reading data may have side-effects.

Also, the lua method documentation for on_bus_read states Fires immediately before the given address is read by the core... which makes me wonder, if the callback is to be called BEFORE reading the address (as is being done at least in the case of bsnes), how is the read address supposed to be passed to the callback?

@YoshiRulz
Copy link
Member

Unlike writes, there's not a before and after value—unless you count registers but those are even easier to peek from the callback than memory is. So the value would be populated with the read function's return value as NesHawk does here, or equivalently from a peek (side-effects shouldn't come into this) at addr with the correct width.

if (MemoryCallbacks.HasReads)
{
uint flags = (uint)(MemoryCallbackFlags.CPUZero | MemoryCallbackFlags.AccessRead);
MemoryCallbacks.CallMemoryCallbacks(addr, ret, flags, "System Bus");
}
DB = ret;
return ret;

The other option is to fix the docs to reflect reality.

@VelpaChallenger
Copy link
Author

Yes I didn't add it in the description, but it might be worth noting that the reason this behavior happens is because when the read memory callback function for each core (BSNES and Genplus-gx) is called, the value 0 is being passed as the value. My point being that as Yoshi says if the value is populated with the read function's return value, I think it should work, since only one read is happening anyway.

Here's what I mean:

ReadCallback = new LibGPGX.mem_cb(a =>
{
if (MemoryCallbacks.HasReads)
{
uint flags = (uint)MemoryCallbackFlags.AccessRead;
MemoryCallbacks.CallMemoryCallbacks(a, 0, flags, "M68K BUS");
}
});

private void ReadHook(uint addr)
{
if (MemoryCallbacks.HasReads)
{
MemoryCallbacks.CallMemoryCallbacks(addr, 0, (uint) MemoryCallbackFlags.AccessRead, "System Bus");
}
}

As for the docs, I imagine it means before the address is read by the game's code, but not necessarily the core itself. So the way I understand it, whenever the game needs to read from an address, the read memory function is called (ReadMemory for NesHawk), so the game waits for the return value, meaning that until that value is not returned, the address was not read, even though of course it has to be read before by the core to be able to pass it to the game (and to the callback for that matter). I understood it that way the first time I read it, and if that's what it means, then yes it can be confusing, although I am not sure if it would require an update.

I am also giving it a try and seeing if I can do the fix myself. You can of course do a memory.read_u16_be or something of the sort inside your callback to read the value from the address, but I still think it would be nice to have the value returned and ready to be used inside the callback. I think it might also make it easier to understand why val is being passed, as otherwise it can cause confusion why val is being passed to the callback if it's always 0.

@Morilli
Copy link
Collaborator

Morilli commented Oct 28, 2023

Unlike writes, there's not a before and after value

Well yes, but there's still a before and after state when considering side-effects, so I believe the distinction of before and after still matters. When the docs explicitly specify before, I implement before.

In that regard I'd definitely appreciate an update to the docs to clarify for both bizhawk devs and lua devs how exactly those functions should work. As it reads currently, calling the callback function before a value was read, but including the read value does not make sense.

@YoshiRulz
Copy link
Member

YoshiRulz commented Oct 28, 2023

It makes sense from the perspective of the script author: At the time the callback gets execution, the 'current'/'before' state is visible through the APIs, and since the 'future'/'after' state can't easily be reconstructed, the callback is provided those values via the parameters. It just happens that the val parameter is semantically the value of the memory at addr (and not a register), so for reads there is no difference between 'before' and 'after'.

'Before' in the internal API contract means before. If the implementation necessitates that the side-effects of a read must happen before or during a read, then the memory callback part will have to include an extra peek. (Or return default, which is fine but currently undocumented.)

VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
- related to issue TASEmulators#3813
- update signatures, send returned value from bus read as data to read callback
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
- related to issue TASEmulators#3813
- update signatures, create new value variable in each of the memory read core functions, pass it to the callback and return it instead of the inline calculations. Also, pass val to write and exec callbacks in IDebuggable since they all use the same mem_cb signature and it would break otherwise. I want to update write and exec callbacks in next commit though to ensure nothing unexpected happens.
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
- related to issue TASEmulators#3813
- update signatures, create new value variable in each of the memory read core functions, pass it to the callback and return it instead of the inline calculations. Also, pass val to write and exec callbacks in IDebuggable since they all use the same mem_cb signature and it would break otherwise. I want to update write and exec callbacks in next commit though to ensure nothing unexpected happens.
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
- related to issue TASEmulators#3813
- update signatures, send returned value from bus read as data to read callback
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
- related to issue TASEmulators#3813
- update signatures, create new value variable in each of the memory read core functions, pass it to the callback and return it instead of the inline calculations. Also, pass val to write and exec callbacks in IDebuggable since they all use the same mem_cb signature and it would break otherwise. I want to update write and exec callbacks in next commit though to ensure nothing unexpected happens.
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
VelpaChallenger added a commit to VelpaChallenger/BizHawk that referenced this issue Nov 5, 2023
@Morilli Morilli linked a pull request Nov 12, 2023 that will close this issue
2 tasks
@YoshiRulz YoshiRulz added this to the 2.9.2 milestone Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core: BSNES Super Nintendo Entertainment System (SNES) / Super Famicom (SFC) / Super Game Boy (SGB) core Core: Genplus-gx Sega Genesis / Mega Drive core Enhancement For feature requests or possible improvements re: APIHawk Relating to EmuHawk's public .NET API or to the creation of external tools re: Lua API/scripting Relating to EmuHawk's Lua API (not the Lua Console) Repro: Fixed/added in branch/fork
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants