Various bugfixes for the Sonic 1 sound driver

With Clownacy posting lots of fixes for the Sonic 2 driver recently, I remembered that I wanted to post some fixes for the Sonic 1 sound driver.
Actually I planned to post these fixes a lot earlier (the text file with the original notes dates back to 30 March 2013), but kept forgetting about it.
I ran into most of these bugs when working on the S2 Clone Driver for Sonic 2 Recreation.
For some of the bugs there are example files that demonstrate them. There are MP3s and VGMs, the latter ones contain slightly longer segments of the used songs.

Note: Most of the bugs were fixed in the Sonic 2 driver, sometimes in a slightly different way. Those that aren't should be easily portable to the S2 driver though.

A few general notes about the code style:
The code excerpts are based on the sound driver I used in S2R, so it's a mix between the 2005 disassembly (actual numbers for offsets) and the SVN/Git disassembly (comments/labels). Labels from the 2005 disassembly are listed in the comments, either directly after the label or in [square brackets] for branches.
The locret_xxx labels are called @locret in the SVN/Git disassembly.

Fix modulation during rests(fixed in Sonic 2)
You won't notice this bug unless you have an FM instrument with a long release rate and enable modulation in areas with rests in your song.
But in these cases, you'll likely be confronted with "rumbling" in your songs.

What causes this? Sonic 1 applies the modulation effect on rests where the frequency is set to 0, so it sort of corrupts the frequency.
(It will apply the modulation effect to frequency 0, so it will send frequencies like 0, 2, 0, 7FE, 0 to the FM chip.
Also, the "rest frequency" is 0 for FM only. It is -1 for PSG channels, but the bug has no audible effect on them.)

The fix is quite easy and actually a backport from the Sonic 2 sound driver. Just paste these 2 lines right below the addq.w:

btst #1,(a5) ; Is note playing?
bne.s locret_71E16 ; no - return

Fix PSG fading bug (1-up bug, part 1)(fixed in Sonic 2)
After the 1-up ends, when the music fades in, you may randomly hear some short and loud PSG notes. Or even worse, a long PSG tone with rising pitch. (Example MP3/VGM)
The tone ends as soon as another notes plays on the respective PSG channels. It can still be pretty annoying though.

The problem is, that some parts of the sound driver call the SetPSGVolume subroutine without checking, if the PSG volume is between $0 and $F. And if it isn't, the calculation will overflow and it will write to the next channel's frequency register instead.
(The problematic code is not in DoFadeIn, btw. It's in PSGDoVolFX. It is possible to easily reproduce the bug by setting the PSG channel volume to $10+ and its instrument to $00.)

To fix this, we could either add an additional check at every point where SetPSGVolume is called. Or we could add it one time, in PSGSendVolume, and catch all cases - this is what we'll do.
This is the code that sends the PSG volume:

Like above, you can remove the 3 lines from cmpi.b to moveq. You can also remove the sendpsgvol-label, because it isn't used anymore.

Fix PSG noise bug (1-up bug, part 2)
You probably never used the PSG's periodic noise, but it makes a nice sawtooth-like bass sound.
Sonic 1 has a very annoying bug that occurs when fading back to a song that uses it: It doesn't set the noise mode after fading from the 1-up, so it plays white noise instead of a bass sound. (Example MP3/VGM)

The solution is to restore the noise type in the cfFadeInToPrevious routine. (This is already done when an SFX ends, btw.)

Fix for 0 FM/DAC channels(fixed in Sonic 2)
Most of you will probably never make SMPS files that use only the 3 PSG channels, but if you like to experiment with SMPS music, you might encounter this bug.
In general, the song might or might not load. The behaviour can be random, because it uses uninitialized registers.

You see the 3 lines marked with [!] - these load a few default values that are important for the driver setup.
If there are 0 FM/DAC tracks, the beq.w L_bgm_fmdone skips them and the PSG tracks will be set up with garbage values.
(Since you're lucky, an unmodded Sonic 1 driver just won't play anything at all. But it can also play the first note and hang then.)

The fix is quite simple, just move these 3 lines a bit up somewhere above of the beq.w so that they get processed even if there are no FM/DAC channels.
I recommend to move them between addq.w #6,a4 and moveq #0,d7.

Fix the DAC 1-up bug(fixed in Sonic 2)
You know how getting a 1-up in the Special Stage in Sonic 1 mutes the 6th FM channel when fading in?
The reason is, that the 1-up enables the YM2612's DAC mode (replaces the 6th FM channel with 8-bit PCM), but when fading in it doesn't disable the DAC mode.
The solution is really simple - just write value $00 to register $2B of YM2612's port 0 to disable the DAC and enable the 6th FM channel again. It doesn't do any harm to songs that use the DAC, because the Z80 DAC driver enables the DAC mode before playing any sound.* (Sonic 1 only disables the DAC when starting to play a song with exactly 7 FM/DAC channels, btw.)* Custom DAC drivers might behave differently.

Note: There is also an alternative solution that isn't as simple, but would allow dynamic switching between DAC mode and 6 FM channels during a song:
Keep track of the current DAC mode (driver init should set it to $00) by using a byte in sound RAM that is always the same as the last value written to YM2612 register $02B.
Set the DAC mode to $80 when playing a DAC sound (at the end of @gotsampleduration/loc_71C88) and to $00 when playing a note on FM6. The latter requires an additional check in FMNoteOn to prevent it from disabling DAC mode when a note is played on FM 1-5.

Fix the SEGA sound panning bug
Does your Game Over/Continue song use panned DAC drums? If it does, you may've run into a bug where the SEGA chant is panned to the left or right speaker.

The cause is, that the song's panning doesn't get reset when the SEGA chant is played, so the effect remains.
(It should be noted that starting any song with 1-6 FM/DAC channels resets the panning.)

Anyway, the solution is simple: We reset the panning bits before playing the SEGA chant. This is done using these 3 lines (the DAC channel uses the FM6 stereo mask):

[23:36] (+vladiklaptop) So, I found a fatal programming flow in SMPS 68k.
[23:36] (+vladiklaptop) The reason why they couldn't get it properly working with Horizontal Interrupts in Sonic 1, the reason they had to go with that dirty and unreliable trick of cancelling SMPS call if interrupt is about to happen above line #96 and moving the call to HBlank routine itself.
[23:36] (+vladiklaptop) Due to a program error, SMPS 68k enters an infinite loop if an interrupt happens during DoModulation routine.
[23:36] (+vladiklaptop) They couldn't figure the issue in Sonic 1, thus, they had to ensure nothing will interrupt SMPS. On the other side, if interrupts were disabled during SMPS processing, the game won't receive HBlank at time and LZ water will glitch out. So they came up with the dirty solution described above.
[23:38] (+vladiklaptop) The bug is really intersting and hideous here
[23:38] (+vladiklaptop) What may cause the processor to hang after an interrupt?
[23:38] (+vladiklaptop) Almost there
[23:38] (+vladiklaptop) Hold on
[23:39] (+vladiklaptop) [02:14:30] <vladiklaptop> There's a programming error in DoMudilation routine that only shows itself when interrupts occur
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> It does this:
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> DoMudulation:
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> ___addq.w #4,sp ; don't want to return to caller =(
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> ___<Some code>
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> ___<Some condition>
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> ___beq.s @locret
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> ___subq.w #4,sp ; nah, changed my mind. return me to the caller, dear M68K!
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> @locret:
[23:39] (+vladiklaptop) [02:16:21] <vladiklaptop> ___rts
[23:39] (+vladiklaptop) [02:17:08] <vladiklaptop> So, when interrupt happens between the shown lines, the stack contents are being rewritten, so when it attempts to change the mind and return to the caller...
[23:39] (+vladiklaptop) [02:17:23] <vladiklaptop> ... it will return to itself
[23:39] (+vladiklaptop) [02:17:35] <vladiklaptop> ...causing an infinite loop
[23:39] (+vladiklaptop) [02:18:40] <vladiklaptop> because that stack was overwritten once interrupt happen -- the processor has pushed the address of the opcode where interrupt happenned, overwritting the "caller" it wanted to return to

This issue was seemingly resolved in SMPS 68k Type 2, where no such stack meddling takes place. Rather, it seems the subroutine that the addq.w was to avoid performs a check to decide if it should run or not. This fix seems to rely on Type 2's FM modulation, so, for Sonic 1's Type 1-based driver, I'll provide a more basic fix.

All 'rts's here skip the subroutine's caller, safe for the one under @calcfreq, which is sometimes used without skipping. The most obvious fix would be to remove the original 'addq.w #4,sp' and 'subq.w #4,sp', and insert 'addq.w #4,sp' before all 'rts's but the one under @calcfreq. There is a slight complication with this, however: as said before, DoModulation uses @locret to skip the caller, but the 'rts' under @locret is used by @calcfreq without skipping. The ideal thing to do would be to move the @locret label to before the 'rts' (and 'addq.w #4,sp') above @waitdone.

By doing this, you can remove UpdateMusic from H-Int: remove the VBla_Exit label and the sole branch to it, along with its 'addq.w #4,sp', under VBla_08. Then remove the jsr to UpdateMusic in loc_119E.

By doing this, you can remove UpdateMusic from H-Int: remove the VBla_Exit label and the sole branch to it, along with its 'addq.w #4,sp', under VBla_08. Then remove the jsr to UpdateMusic in loc_119E.

There's one important thing you forget.

Upon calling UpdateMusic, any interrupts are still masked, because the status registers (SR) is set to 26xx when processor accepts level 6 interrupt, that is vertical blanking interrupt (VBlank). Since interrupt priority mask is set to 6, all interrupts of this level or less will be inhibited. Since horizontal blanking interrupts (HBlank) are level 4 interrupts, the processor won't any interrupts unless the main VBlank routine finishes, which doesn't happen unless UpdateMusic finishes. That brings us the point of why they actually moved UpdateMusic to VBlank, besides the bug described above.

The SMPS 68K driver itself can be a real monster sometimes. When it comes to initializing the BGM playback, or loading many instruments at once, -- it can take up to 25% of CPU time. It happens rarely, it happens one frame worth, but still, it happens. And when it does, the SMPS takes so long to execute, that the M68K still cannot leave the VBlank when the VDP starts drawing a new TV-frame already. Here comes the big problem: when VDP reaches a scanline where the horizontal interrupt should happen, but the processor still hasn't finished the vertical interrupt, the first one be received in time. This will lead to a lot of graphical glitches with water in LZ, which may only pop up for a couple of frames, but still rather annoying.

So, let me describe the fix step by step in detail. It's really nothing bug and it's almost identical to yours, except for a few important additions.

Fixing the interrupt crash bug in SMPS and optimizing the horizontal interrupts

It's really more of a design choice -- you may completely ignore this fix. Everything won't get more buggy or less stable as Sonic Team did implement a solution for this major bug. It's a dirty, unwise and straight-forward solution, but it works -- and it works rather well.

However, if you're looking into more flexibility, less code hacks and slightly more polished and organized interrupt routines, you may consider the steps I suggest below. Doing these little steps will allow for proper interrupt handling in the game, the way it should be done. This in turn, will allow you to perform fairly more interrupt-intensive tricks, like having per-line horizontal interrupts, something that wasn't possible with the original setup.

First of all, open s1.sounddriver.asm, go to the DoModulation routine and replace all its code with this:

That was the mentioned dirty workaround. If the water was to appear above scanline #96, they knew SMPS can slow down VBlank to longer than that, so they didn't call SMPS then, they set a special flag ($FFFFF64F), so SMPS will be called later in HBlank routine itself. However, this solution isn't exactly perfect: if SMPS happens to lag for more than entire frame, VBlank may be missed or delayed, moreover, future horizontal interrupts may be delayed or have their timming messed up. But that's an exotic case.

Now, it's time to clean up HBlank routine from the coding garbage. Find it, find this lines near the end:

tst.b ($FFFFF64F).w
bne.s loc_119E

Delete them. You may also delete this code below as it becomes a dead code:

Since we've enabled interrupts within VBlank, there appears a chance, although almost impossible, that SMPS will lag so hard so it takes about a TV-frame to process, when the next VBlank will be received. In this case, we don't want another instance of SMPS to overlay the existing one, otherwise, in this almost impossible case, the sound system may crash completely. The SMPS/Treasure implements the said fix.

Now, there appears the following race condition: when SMPS has lagged badly and the next VBlank occurred, there will be a conflict on taking Z80 bus. The original SMPS stops the Z80 for the whole time and starts it by the end, however, if VBlank will overlay its execution, various routines will attempt to stop Z80 again, then start it. So, by the end of VBlank, the Z80 will be running and when processor returns to the SMPS, it'll suffer from not being able to write to the YM2612 chip properly.

In order to fix the last issue, go to VBla_08, then VBla_00 and remove every single stopZ80, waitZ80 and startZ80 macro from there. This way, the Z80 will remain stopped if VBlank interrupts SMPS. You may remove Z80 stops from the other VBlank routines as well.

So, it turns out SMPS is terrible! I know, its super shocking... Anyway, this is a fairly obscure bug probably nobody would ever notice, except me. But I did, and I am gonna help you fix it, its fairly easy too. Simply, all the bug is, is that SMPS forgets to include modulation frequency when updating frequency just after reading the tracker. Have a listen. So then, lets do this! In Hivebrain disassembly, Sonic1.asm, go to label sub_71E18. You should see following:

Are you sure this is a thorough fix? Looking at the code, the issue is that the modulation isn't being updated on the frame a 'no attack' note starts. The modulation not being applied to the frequency seems like a side-effect of it. S2 fixed both of those at the same time, as overkill as its method was.