I2S sound harsh artifacts

[svofski]

I2S sound with PCM5102 produces harsh and extremely loud noises. I think these 2 lines may be the culprit:

pico-zxspectrum/src/ZxSpectrumAudio.cpp

Lines 288 to 289 in 75f0396

	uint32_t l = (((vA << 1) + vB + s) << 4) - ((255 + 255 + 255 + 255) << (4 - 1));
	uint32_t r = (((vC << 1) + vB + s) << 4) - ((255 + 255 + 255 + 255) << (4 - 1));

What is the reason for the second term in the expression here? All the inputs and result are unsigned and the expression underflows for a wide range of input values. I'm not sure what should be expected here but it doesn't look right.

I tried removing the second term and everything sounds great without it. However I don't know why it was written that way and perhaps on some devices this is the right thing to do.

    uint32_t l = (((vA << 1) + vB + s) << 4);
    uint32_t r = (((vC << 1) + vB + s) << 4);

Exolon is always a good test, but I can also recommend a dedicated sound test (attached).

AY_YM_TS_test_v02.ZIP

[fruit-bat]

Honestly, can't remember what that was about... I will have a play with your suggestion. I've just corrected an issue with the pwm audio.. the smoothed beeper is signed now but I hadn't updated other parts of the code to reflect it.
Many thanks

[svofski]

I thought the idea was to scale the volume around virtual centre, so I tried to rewrite it as such:

    int32_t l = (((vA << 1) + vB + s) << 4) - ((255 + 255 + 255 + 255) << (4 - 1));
    int32_t r = (((vC << 1) + vB + s) << 4) - ((255 + 255 + 255 + 255) << (4 - 1));
    ll = (__mul_instruction(_vol, l) >> 8) + 32768;
    rr = (__mul_instruction(_vol, r) >> 8) + 32768;

This also sounds good, but I don't think it's perfectly right. In order to offset everything to zero we need to center each individual component before mixing.

    // offset 0..255 to -128..127
    int32_t ivA = vA - 128;
    int32_t ivB = vB - 128;
    int32_t ivC = vC - 128;
    int32_t is = s - 128;
    int32_t l = ((ivA << 1) + ivB + is) << 4;   // mix L = 2*A + B + s
    int32_t r = ((ivC << 1) + ivB + is) << 4;   // mix R = 2*C + B + s
    // scale by volume and offset to middle before casting back to unsigned
    ll = (__mul_instruction(_vol, l) >> 8) + 32768;
    rr = (__mul_instruction(_vol, r) >> 8) + 32768;

But in practice this doesn't seem to have any benefit compared to the simpler version above, sounds alright though.

[fruit-bat]

Just tried:
uint32_t l = (((vA << 1) + vB + s) << 4);
uint32_t r = (((vC << 1) + vB + s) << 4);
and it sounds good. It looks like previously I was trying to get a signed result... but if the simpler version sounds better that is a result!

[svofski]

The simple unsigned version worked well for me too.

I'm slightly worried that there's sine if the dynamic range lost somewhere, or if it would flip if AY and beeper play at the same time. PCM5100 expects 2's complement, so it's signed. When sending unsigned data, we must stay within 15 bits or risk some harsh noises when bit 15 becomes 1.

Say, if all inputs are -128..127 signed, 4 inputs, max range would be [-4128..4127] = [-512..508]. There's a << 4 so [-8192..8128].

I don't know what the _vol range is. It's probably 0..255 because there's >> 8 after multiplication. So [-8192 * 255..8128 * 255], well within 32-bit integer range. In fact, we're well too low on the scale after shifting >> 8 and could have >> 6, the values will stay in range [-32640..32385]. I'm going to test this later.

[svofski]

Just had a minute to test. This version is loud and clear, no artifacting.

    // offset 0..255 to -128..127
    int32_t ivA = vA - 128;
    int32_t ivB = vB - 128;
    int32_t ivC = vC - 128;
    int32_t is = s - 128;
    int32_t l = ((ivA << 1) + ivB + is) << 4;   // mix L = 2*A + B + s
    int32_t r = ((ivC << 1) + ivB + is) << 4;   // mix R = 2*C + B + s
    // scale by volume and offset to middle before casting back to unsigned
    ll = (__mul_instruction(_vol, l) >> 6) + 32768;
    rr = (__mul_instruction(_vol, r) >> 6) + 32768;

Top: old version, bottom: new version

I'm a bit puzzled now though. The range is offset to a "virtual 0" at 32768 when they become unsigned so there's again some mistreatment of integers. I'll get back to it later.

[fruit-bat]

Just had a minute to test. This version is loud and clear, no artifacting.

    // offset 0..255 to -128..127
    int32_t ivA = vA - 128;
    int32_t ivB = vB - 128;
    int32_t ivC = vC - 128;
    int32_t is = s - 128;
    int32_t l = ((ivA << 1) + ivB + is) << 4;   // mix L = 2*A + B + s
    int32_t r = ((ivC << 1) + ivB + is) << 4;   // mix R = 2*C + B + s
    // scale by volume and offset to middle before casting back to unsigned
    ll = (__mul_instruction(_vol, l) >> 6) + 32768;
    rr = (__mul_instruction(_vol, r) >> 6) + 32768;

Top: old version, bottom: new version

I'm a bit puzzled now though. The range is offset to a "virtual 0" at 32768 when they become unsigned so there's again some mistreatment of integers. I'll get back to it later.

I'm getting a lot of distortion with that version

[fruit-bat]

I'm on this branch at the moment...
https://github.com/fruit-bat/pico-zxspectrum/tree/feature/murmulator2

This is working nicely for me:

const uint32_t l = (vA << 1) + vB + s + 128;
const uint32_t r = (vC << 1) + vB + s + 128;
const uint32_t v = _vol << 4;
ll = (__mul_instruction(v, l) >> 8) & 0xffff;
rr = (__mul_instruction(v, r) >> 8) & 0xffff;

...which is just a slight optimisation of your earlier simple version.

[svofski]

That version works but it's very quiet.

Could the distortion happen because the beeper is signed in that branch?

int32_t is = s - 128; // this is not needed if s is already signed and centered around 0

[fruit-bat]

That version works but it's very quiet.

Could the distortion happen because the beeper is signed in that branch?

int32_t is = s - 128; // this is not needed if s is already signed and centered around 0

That's what I wondered. I tried taking out the -128 and it did not help... but I am really guessing here... really I should get a log of the numbers being produced!

[fruit-bat]

...how does this sound...

const uint32_t l = (vA << 1) + vB + s + 128;
const uint32_t r = (vC << 1) + vB + s + 128;
const uint32_t v = __mul_instruction(_vol, 30);
ll = (__mul_instruction(v, l) >> 8) & 0xffff;
rr = (__mul_instruction(v, r) >> 8) & 0xffff;

[svofski]

More or less the same, it's fine I guess.

Have you tried producing a ramp just to see what kind of range does the DAC take and if it matches the expectations? I'm having a hard time doing that somehow. I thought I'd just count some number and write it to ll/rr, but I'm seeing some garbage on the output.

[fruit-bat]

Does it glitch about half way?

[svofski]

this is
static volatile uint32_t divctr = 0;
...
ll = rr = divctr & 0xffff;
divctr += 1000;

[fruit-bat]

Hmm, I wonder if the values are signed...

const int32_t l = (vA << 1) + vB + s - (128*3);
const int32_t r = (vC << 1) + vB + s - (128*3);
const int32_t v = __mul_instruction(_vol, 60);
ll = (__mul_instruction(v, l) >> 8) & 0xffff;
rr = (__mul_instruction(v, r) >> 8) & 0xffff;

This works suspiciously well for me.

[fruit-bat]

The bit where it just jumps about is weird, that really should not happen.

Maybe add a delay in your ramp at 0 so we can see where it is.

[svofski]

Anyway, unsigned seems too work as expected. Signed doesn't look right. It seems that values above 55000 are not good regardless.

static constexpr int maxval = 55000;
int l = divctr - maxval/2;

ll = l & 0xffff;
rr = divctr & 0xffff;

divctr += 500;
if (divctr > maxval) {
divctr -= maxval;
}

[svofski]

I've just realised that my PCM5102 board has a jumper switch for FMT, which has been set to Left-Justified all of this time. This might explain the discrepancies between our results. Do you know how your DAC is configured?

[svofski]

Another experiment, this time with better tooling. Left-justified, same as before.

A ramp with a little delay:

   static constexpr int maxval = 0xffff;
   rr = divctr & maxval;
   if (divctr < 0) {
       ll = 0;
       rr = 0;
   }

   divctr += 500;
   if (divctr > maxval) {
       divctr = -120000;
   }

This time I measured it with a scope and it sees exactly what I expected to see, a ramp:

I'm still not sure how is this 2's complement because it looks perfectly unsigned to me, but there we go. Full range, a perfect ramp.

In support of poor Zoom H1 which produces complete nonsense that confused me so much, 3Vp-p is well beyond the spec of its microphone input and it probably gets clamped by input protection diodes.

[fruit-bat]

you

I'm using a Pimoroni Pico DV Demo Base, which has a 5100A, fmt, fit, demp to gnd, xsmt to vcc (3.3v)

[fruit-bat]

...and yes, your ramp looks unsigned.

[fruit-bat]

Sadly, my scope has recently failed on me... but to be fair it was made in 1969. I need to look into a replacement.

[fruit-bat]

https://cdn.shopify.com/s/files/1/0174/1800/files/pico_dv_schematic.pdf?v=1636985340

[svofski]

Aha, so yours is in FMT=Low, I2S mode, which seems to match the code. This makes sense, left-justified has MSB one clock earlier so the MSB was lost and I was getting only half of the range.

Here's my ramp in I2S mode with signed data:

  static constexpr int maxval = 0xffff;
  rr = (divctr - (maxval >> 1)) & maxval;

I think the mystery is solved.
I'm not sure in which universe line output is supposed to have 5Vpp swing, but such is my board anyway.