Audible stepping when using `synthio.LFO` on `audiodelays.Echo.delay_ms`

[relic-se]

CircuitPython version and board name

Adafruit CircuitPython 9.2.4; Pimoroni Pico Plus2 with rp2350b

Code/REPL

import audiobusio
import audiodelays
import board, time
import synthio

SAMPLE_RATE = 48000
AMPLITUDE = 0.2
MIX = 1.0 # Would need to be 0.5 if #9994 is merged
TIME = 40
DEPTH = 8
RATE = 1.0
OCTAVE = -1

audio = audiobusio.I2SOut(
    bit_clock=board.GP0,
    word_select=board.GP1,
    data=board.GP2,
)

synth = synthio.Synthesizer(
    sample_rate=SAMPLE_RATE,
    channel_count=2,
)

effect = audiodelays.Echo(
    max_delay_ms=TIME,
    delay_ms=synthio.LFO(offset=TIME, scale=DEPTH, rate=RATE),
    decay=0.0,
    sample_rate=SAMPLE_RATE,
    channel_count=2,
    freq_shift=True,
)

effect.play(synth)
audio.play(effect)

# Chord
for i in range(3):
    synth.press(synthio.Note(
        frequency=synthio.midi_to_hz(65 + i * 4 + OCTAVE * 12),
        panning=synthio.LFO(rate=0.15 + i * 0.05),
        amplitude=AMPLITUDE,
    ))

while True:
    if effect.mix:
        effect.mix = 0.0
        print("Off")
    else:
        effect.mix = MIX
        print("On")
    time.sleep(2.0)

Behavior

There is audible stepping in the chorus effect.

Here's a recording of the above example: https://drive.google.com/file/d/13qotNTCFfkoQ_GuhENawpOw9rmh5oPoS/view?usp=drive_link

Description

No response

Additional information

The optimization at Echo.c#L313 prevents calling recalculate_delay at it's maximum rate of SYNTHIO_MAX_DUR and instead limits it to increments of sample_ms. For effects using larger delay lengths, this stepping isn't noticeable, but for effects which require a delay with a shorter delay length (ie: chorus, flanger), more granularity is required.

circuitpython/shared-module/audiodelays/Echo.c

Lines 313 to 315 in 4ba872c

	if (MICROPY_FLOAT_C_FUN(fabs)(self->current_delay_ms - f_delay_ms) >= self->sample_ms) {
	recalculate_delay(self, f_delay_ms);
	}

I'm thinking that a suitable solution is to check the difference by sample_ms / 256 when freq_shift=True (for the 8-bits of sub-resolution). That, or test the float equality using memcmp such as in the following function.

circuitpython/shared-module/synthio/BlockBiquad.c

Lines 65 to 74 in 4ba872c

	static int float_equal_or_update(
	mp_float_t *cached,
	mp_float_t new) {
	// uses memcmp to avoid error about equality float comparison
	if (memcmp(cached, &new, sizeof(mp_float_t))) {
	*cached = new;
	return false;
	}
	return true;
	}

[relic-se]

I've tested the proposed fix, and it doesn't appear to resolve the issue. I've also toyed around with a few other adjustments to no avail. Any ideas as to why this could be happening would be greatly appreciated.

[d1ffeq-diy]

Don't have the 2350 board at the moment, can you try to replace your chord iterator lines with a single tone note, like this:
synth.press(synthio.Note(frequency=440, amplitude=AMPLITUDE))

to narrow things down, as I've noticed in your sound example that stepping occurs approx. every 4.2s, exactly the period of your panning parameter LFO.

[relic-se]

@d1ffeq-diy Thank you for the review. While setting up an updated example to simplify the demonstration, I think I've come across the problem.

As per the current documentation on synthio.LFO (https://docs.circuitpython.org/en/latest/shared-bindings/synthio/index.html#synthio.LFO), it states "If waveform is None, a triangle waveform is used." When I specify the waveform directly (such as a sine wave in the example below), the program works as intended.

import audiobusio
import audiodelays
import audiofilters
import board, time
import ulab.numpy as np
import synthio

WAVEFORM = True # False is default, True is sine
MIX = 1.0 # Would need to be 0.5 if #9994 is merged

audio = audiobusio.I2SOut(
    bit_clock=board.GP0,
    word_select=board.GP1,
    data=board.GP2,
)

synth = synthio.Synthesizer(
    sample_rate=48000,
    channel_count=1,
)

effect_delay = audiodelays.Echo(
    max_delay_ms=50,
    delay_ms=synthio.LFO(
        offset=40,
        scale=10,
        rate=1.0,
        waveform=None if not WAVEFORM else np.array(np.sin(np.linspace(0, 2*np.pi, 512, endpoint=False)) * 32767, dtype=np.int16),
    ),
    decay=0.0,
    sample_rate=48000,
    channel_count=1,
    freq_shift=True,
)

effect_delay.play(synth)
audio.play(effect_delay)

synth.press(synthio.Note(
    frequency=synthio.midi_to_hz(53),
    amplitude=0.5,
))

while True:
    if effect_delay.mix:
        effect_delay.mix = 0.0
        print("Off")
    else:
        effect_delay.mix = MIX
        print("On")
    time.sleep(2.0)

Here's an audio recording of the above example, first with WAVEFORM=False then with WAVEFORM=True: https://drive.google.com/file/d/1SbZjiso4bfjI0rYGKnfCFdzwM7BZ7IuZ/view?usp=sharing

I'll need to double check the constructor code for synthio.LFO, but it's possible that it's actually defaulting to a square wave of sorts. However, I don't remember it exhibiting this functionality in the past.

[relic-se]

If I replace time.sleep(2.0) in the loop with the following:

tick = 40
while tick:
    print(effect_delay.delay_ms.value)
    time.sleep(0.05)
    tick -= 1

I can generate the graph below (in LibreOffice Calc):

It looks as if the default waveform is functioning as intended, but it is somehow not affecting the effect parameter correctly unless a waveform is specified by the user.

[d1ffeq-diy]

There's definitely a bug. Looking closely at harmonic content in spectrogram of the latest audio snippet, the modulation with WAVEFORM=False looks square-like:

and with WAVEFORM=True it looks exactly as it should:

also, the tiny slopes of the square-like transitions suggest that it once was a triangle wave. My theory is that default LFO values do not scale properly somewhere further down the line, causing waveform to be heavily clipped(amplified too much) between min and max values.

[d1ffeq-diy]

@relic-se any progress on tracking the LFO creation code?

[relic-se]

@d1ffeq-diy I haven't dig too deep into it as of yet. If you've got the know-how, feel free to poke around in the core. Otherwise, because the work-around is fairly simple (just providing your own waveform), I've got this as a low priority item atm.