Re: [openMSX-devel] [PATCH 3/3] Implement SCC glitching

[Wouter V. <ver...@gm...>]

See below for specific comments/questions.


On Sat, Nov 16, 2013 at 11:02 AM, Sean Young <se...@me...> wrote:

> Channels 4 and 5 on the SCC (not the SCC+) has "glitches" on channel 4 and
> 5. The SCC plays the wrong samples samples. The glitch depends on when the
> frequency register is written (C) and the perod (P).
> ---
>


> +inline bool SCC::ChannelGlitches(unsigned channel)
> +{
> +       return (currentChipMode == SCC_Real && channel >= 3);
> +}
>

[detail] we prefer starting method names with lower case.



>  void SCC::writeWave(unsigned channel, unsigned address, byte value)
>  {
>         // write to channel 5 only possible in SCC+ mode
> @@ -376,11 +414,19 @@ void SCC::writeWave(unsigned channel, unsigned
> address, byte value)
>         if (!readOnly[channel]) {
>                 unsigned pos = address & 0x1F;
>                 wave[channel][pos] = value;
> -               volAdjustedWave[channel][pos] = adjust(value,
> volume[channel]);
> +               if (SCC::ChannelGlitches(channel)) {
> +                       adjustWave(channel);
> +               } else {
> +                       volAdjustedWave[channel][pos] = adjust(value,
> volume[channel]);
> +               }
>                 if ((currentChipMode != SCC_plusmode) && (channel == 3)) {
>                         // copy waveform 4 -> waveform 5
>                         wave[4][pos] = wave[3][pos];
> -                       volAdjustedWave[4][pos] = adjust(value, volume[4]);
> +                       if (SCC::ChannelGlitches(4)) {
> +                               adjustWave(4);
> +                       } else {
> +                               volAdjustedWave[4][pos] = adjust(value,
> volume[4]);
> +                       }
>                 }
>         }
>  }
>

I think this is correct, but just making sure: for a glitchy channel it is
required
to recalculate all 32 volAdjustedWave[x][32] values, right?
[detail] No need to put 'SCC::' in front of the call to channelGlitches().



> @@ -416,14 +462,16 @@ void SCC::setFreqVol(unsigned address, byte value,
> EmuTime::param time)
>                 count[channel] = (per > 11) ? (per + 1) - 11 : 0;
>                 period[channel] = per;
>                 incr[channel] = (per <= 8) ? 0 : 32;
> +               if (ChannelGlitches(channel)) {
> +                       Clock<3579545> zero(EmuTime::zero);
> +                       glitch[channel - 3] = (zero.getTicksTill(time) -
> pos[channel] * (per + 1)) & 31;
> +                       adjustWave(channel);
> +               }
>
> Very much a detail, probably won't ever matter in real software.
What happens when 'currentChipMode' changes? Currently I don't think channel
4 and 5 are recalculated to take or not take the glitch stuff into account.
Maybe a very small step in that direction: it's possible to always write
glitch[]
when the freq register of channel 4 or 5 is written. That makes it also
easier
to understand what glitch[2] means: it _always_ contains the (5 lower bits)
of the time-stamp the freq register was last written (so independent of the
scc mode).

Another detail. Maybe you don't need to calculate the number of ticks since
time zero, but since the last reset (EmuTime does not go back to zero when
the MSX is reset). Although it is certainly possible the internal SCC
counters
are NOT reset on a msx reset. Also this behavior will be very hard to test
and
it doesn't really matter either. So I don't mind at all if you don't change
this
(maybe add a small comment?).



> @@ -654,6 +694,18 @@ void SCC::serialize(Archive& ar, unsigned /*version*/)
>                 }
>         }
>
> +       if (currentChipMode == SCC_Real && ar.versionAtLeast(version, 2)) {
> +               // setFreqVol() overwrites this so do this after calling it
> +               ar.serialize("glitch", glitch);
> +       }
>

Related to the point above. Maybe you can always serialize the glitch[]
array here,
so independent of the currentChipMode status.
In case we're loading an old (version=1) savestate, the glitch[] array is
not initialized.
Might be a good idea to set it to some known (deterministic) value. Or wait
... it _is_
initialized to 0 in the constructor. I guess that's OK. Maybe add an empty
else-block
with just a comment that glitch[] is already zero-initialized.

Thanks.


Wouter