sverx
Add support for large songs spawning multiple banks
Long songs can easily grow past 16kB, and this happened to me yesterday (my song is 24kB), so I had to do something.
My solution was to add support for multi-bank playback to PSGlib. The requirement is simply that the song data be consecutive in ROM. PSGlib takes care of increasing the bank and restraining the song pointer to slot 2.
In theory the limit in size is now 64kB if the song is compressed (substring offset is 16 bit), but I suspect this could be worked around by not compressing or only using substrings within the first 64kB, etc.
I added some documentation to the .h:
/* About multi bank support
*
* If PSGlib is compiled with MULTIBANK, the current bank in slot 2
* will be remembered by PSGlib and the bank will be switched
* automatically when calling PSGFrame(), but you must still take care
* of switching to the correct bank before calling PSGPlay, PSGPlayLoops
* or PSGPlayNoRepeat.
*
* PSGlib will automatically switch banks if the song data occupies more
* than 16kB, but it is assumed that the data is not segmented. (i.e.
* consecutive in ROM)
*
* Make sure to save/restore the bank in slot 2 before calling
* PSGFrame(), as bank 2 will change.
*
* (SMS_saveROMBank/restoreROMBank can help here!)
*/
Oh, and I also added a makefile which generates the standard PSGlib and PSGlib_multibank
I'm checking this and I'm trying to figure if it would still support looping points and compressed data correctly.
I seem to have found where the new code saves the bank with the address for the loop point, I still can't find where it handles compressed sections that are found in a different bank. :thinking:
also, as a minor improvement, instead of conditionally jumping to a label where a RET instruction is located, you can just conditionally return... here:
jp z, _nobankchange
res 6,h ; Reset the bit (back to slot 2)
inc a ; And advance to next bank
ld (_PSGMusicPointerBank), a ; Save new bank
_nobankchange:
ret
this could just be:
ret z ; no bank change
res 6,h ; Reset the bit (back to slot 2)
inc a ; And advance to next bank
ld (_PSGMusicPointerBank), a ; Save new bank
ret
oh, wait, I think I have found it.
I also think you meant bit 15 and 14 here:
; Restrict to slot 2 (bit 16 set, bit 15 cleared)
set 7, h
res 6, h
In theory the limit in size is now 64kB if the song is compressed (substring offset is 16 bit), but I suspect this could be worked around by not compressing or only using substrings within the first 64kB, etc.
Yeah, we have to make sure compression doesn't reference substrings outside the first 64 kB. I can try updating my compressor, I'll also ask Calindro to update his as that's the tool people are using (me too!)
Thank you for reviewing this! Thanks for the suggestion of using a conditional return, that's a neat Z80 feature, I was not aware of it yet. I made the change and also corrected the comment (I suppose must have been thinking "16th" and "15th" bits..)
No worries! Thanks for fixing that. Still one possible RET z here:
jp z, _nobankchangeC
res 6,h ; Reset the bit (back to slot 2)
inc a ; And advance to next bank
ld (_PSGMusicPointerBank), a ; Save new bank
_nobankchangeC:
ret
One more question...
When using PSGlib with ROM mapping, the programmer has to map the correct ROM bank where the song is stored before calling PSGFrame, like this:
SMS_mapROMBank(current_song_bank);
PSGFrame();
with multi bank support, would the programmer have to keep track of the bank changes and update the value of current_song_bank for the next call? :thinking:
With multibank support, the initial song bank is saved when PSG_Play is called. But after that, the programmer can just call PSGFrame without setting a particular bank beforehand, since _PSG_ReadByte_B/C takes care of switching the bank before reading memory.
The programmer should know however that a side effect of calling PSGFrame is that slot2 will/may be pointing to a different bank after the call.
Perhaps I should rephrase the documentation to state:
- The pointer passed to PSGPlay must be valid at the time the call is made, therefore make sure to switch the bank before calling if necessary.
- PSGFrame will take care of switching banks. So when it returns, keep in mind that a different bank may be in slot 2.
Oh, I now understand how it works.
Since in the original library the user wasn't required to map the song bank when calling PSGPlay(), I believe a better option here is not to require that and have those Play* functions accept a second parameter, that is the (initial) bank number.
This way, no bank mapping from the user would be necessary neither when triggering the song and neither when processing the frame.
Would you be OK with letting PSGPlay() family functions accept a bank argument for all builds of the lib? (i.e. not only for the multibank build of the lib?)
That would be my preference, given that everyone is probably doing this already, except for very small projects:
SMS_mapROMBank(current_song_bank);
PSGFrame();
Of course storing the song bank in PSGlib would use one byte of memory, but so does the current_song_bank var which would no longer be needed. Also no more need for the user to call SMS_mapROMBank.
So the two versions of the lib would store the initial song bank, and both would switch bank inside PSGFrame/PSGSFXFrame. However only the lib built with MULTIBANK would use additional memory for multi-bank support and use the slower bank-boundary-aware memory access code.
No, I still prefer to keep the 'standard' build of PSGlib to have nothing to do with bankswitching directly. One might not need it, or want it, or use some other bankswitching system other than the SEGA mapper.
When functions with the same name have different arguments depending on the build, it's easy to make mistakes, so here is what I propose:
Use ifdefs in the source to make change the PSGPlay(ptr) function to something like PSGPlay_mb(ptr, bank) when compiling the library with multibank support. The .h file however simply holds prototypes for both PSGPlay(ptr) and PSGPlay_mb(ptr,bank), with comments about which one is available in which build. No ifdefs there.
This way, the user does not need to also #define MULTIBANK before including PSGplay.h, and if the user ever tries to link the wrong version of the library, the linker will issue a missing symbol error. (the .lib either has only PSGPlay or PSGPlay_mb)
Well, in this case the function will have a different number of arguments so it will be the compiler stopping you from calling the 'wrong' function anyway. What I mean is: if PSGLIB_MULTIBANK is defined then you won't be able to call PSGPlay(song) and conversely when it's not defined you won't be able to call PSGPlay(song, bank).
Apart from that, having to possibly add some specific define before including a header file it's already present in SMSlib, so I'd keep the same approach... (see TARGET_GG, MD_PAD_SUPPORT, NO_SPRITE_CHECKS etc...)
Ok, I just updated the pull request. I also reworded the documentation and updated the README.md. Let me know what you think...