Hooking up MIDI output from soundboard #226
Conversation
Used by Magical Truck Adventure to check region code Patch to enable region change no longer required; new patch to force Japan region disabled by default
Used by Magical Truck Adventure to check region code Patch to enable region change no longer required; new patch to force Japan region disabled by default
| MidiStack[MidiOutW++]=val; | ||
| MidiOutW&=31; | ||
| MidiOutStack[MidiOutW++]=val; | ||
| MidiOutW &= 3; |
There was a problem hiding this comment.
Yes, according to the official SCSP documentation.
There was a problem hiding this comment.
I see. In that case, shouldn't SoundBoard.cpp use 4 rather than 256? Definitely makes sense to define that constant in SCSP.cpp or SCSP.h. I think your original suggestion of functions that indicate full vs. empty makes the most sense because it doesn't make sense for external components like CSoundBoard to be worrying about FIFO size explicitly.
| UINT8 CSoundBoard::CheckMIDIStatus(void) | ||
| { | ||
| UINT8 status = 0x80; | ||
| if (SCSP_MidiInFill() < 256) // MIDI input buffer not full |
There was a problem hiding this comment.
How come 256 is used here but it appears that inside of SCSP.cpp, the FIFOs are only of size 4? It may indeed make sense to have either a function or a static constexpr var that specifies the buffer length.
There was a problem hiding this comment.
The real MIDI input FIFO buffer is only 4 bytes, but it is increased to 256 bytes in Supermodel simply because the soundboard doesn't synchronize with the mainboard in the emulator as often as it does on real hardware. Some games (Sega Rally 2 in particular) write several sound commands in a single frame so the increased FIFO size is a way of making sure they are all received by the soundboard.
As a side note, the MIDI input FIFO buffer was actually originally 128 bytes but I increased it to 256 because Sega Rally 2 was sending so many commands it was overflowing the FIFO buffer (this commit adds a check to prevent overflows).
In SCSP.cpp there is MIDI_STACK_SIZE which is defined as 0x100 (256) but SoundBoard.cpp doesn't have access to this. If I were to create a function within SCSP.cpp such as SCSP_MidiInFull() I could use the existing MIDI_STACK_SIZE; probably a good idea to turn it into a constexpr as well.
There was a problem hiding this comment.
Ok, I understand, that makes sense. Ignore what I said in the comment I just posted, then. I think the functions you propose make sense and would be good to include this note about the buffer being larger due to less frequent synchronization.
There was a problem hiding this comment.
What's the status on this? Was there a reason it didn't get merged?
There was a problem hiding this comment.
Still haven't finished refining the code, in particular I wanted to remove the hardcoded FIFO sizes. I will get around to it at some point
Used by Magical Truck Adventure to check region code; ROM in Games.xml is Export version, not Japan.
The patch to enable region change is no longer required and has been removed. A new patch to set the region to Japan has been added but commented out to disable it by default.
I don't really like how I have hardcoded the MIDI FIFO sizes in CheckMIDIStatus(); I think adding functions to SCSP.cpp such as SCSP_MidiInFull() and SCSP_MidiOutEmpty() would be a cleaner way to do it. Thoughts?