I think we need to add more fluff for the full effects package as well.

• Maybe a bitcrasher?

I think we need to add more fluff for the full effects package as well.

* Maybe a bitcrasher?

I don't know exactly what you mean by "fluff", but the audiofilters.DistortionMode.LOFI is a bitcrusher. In fact, it may need to be renamed to better represent that.

Thank you for the review, @jepler ! I'll dig in as soon as I get the chance.

I've noticed a few areas within audiodelays.Echo and audiofilters.Filter which weren't using the practices you've outlined in your comments, @jepler. I've included those updates in a separate commit within this PR.

[copied from Discord]

I don't know that these other objects need a blocks property. But they need to call shared_bindings_synthio_lfo_tick once per 256 samples(*) to set the global data that causes synthio_block_slot_get to actually advance the LFOs that are used.

blocks is used as a way to tick blocks that aren't actually used by anything. or which aren't necessarily used by anything. So for instance if you wanted to have an LFO that is polled from CircuitPython code in order to set an LED color. Or, if you have an LFO that you want to keep advancing its phase regardless of whether it's associated with a playing note.

For the first case, having some object that is not a synthesizer/audio source at all might be the right solution.

For the second case, I don't know if that is applicable to the e.g., an audio echo effect.

(*) shared_bindings_synthio_lfo_tick could have a "number of samples" parameter added to the C function, so that you could calculate a different number of samples each time. Of course, since the arithmetic is all discrete you could get slightly different results from advancing LFOs by [email protected] units vs advancing them at 256-sample@8kHz or 512-sample@24kHz rates...

I don't know that these other objects need a blocks property. But they need to call shared_bindings_synthio_lfo_tick once per 256 samples(*) to set the global data that causes synthio_block_slot_get to actually advance the LFOs that are used.

blocks is used as a way to tick blocks that aren't actually used by anything. or which aren't necessarily used by anything. So for instance if you wanted to have an LFO that is polled from CircuitPython code in order to set an LED color. Or, if you have an LFO that you want to keep advancing its phase regardless of whether it's associated with a playing note.

Thank you for the explanation. I don't think I properly understood the purpose of the blocks properly and have misused it in the past. This all makes a lot more sense. I'll update this effect accordingly. If the solution is as simple as it seems, I may also update the other audio effects which use blocks within this PR (same as with the floating point constants).

(*) shared_bindings_synthio_lfo_tick could have a "number of samples" parameter added to the C function, so that you could calculate a different number of samples each time. Of course, since the arithmetic is all discrete you could get slightly different results from advancing LFOs by [email protected] units vs advancing them at 256-sample@8kHz or 512-sample@24kHz rates...

As for your note on the variable number of samples, I think that 256 samples is a generally ideal pace, but if the buffer size is not a factor of 256, it would cause uneven ticking of the blocks. Yet, using the entire buffer size may not be ideal either because the block inputs may not increment at a desirable pace. We may need a separate accumulator within audiofilters_distortion_obj_t to determine when to update the blocks.

@jepler I've taken some time to look into how the block ticking is actually handled, and I still don't think that the block system is ready to have this update to audioeffects classes.

audiocore.RawSample -> audiodelays.Echo -> audiobusio.I2SOut

In the example layout above, it would make sense to include shared_bindings_synthio_lfo_tick within audiodelays.Echo. Otherwise, synthio.BlockInput values would not be updated.

synthio.Synthesizer -> audiodelays.Echo -> audiobusio.I2SOut

synthio.Synthesizer -> audiomixer.MixerVoice
audiocore.RawSample -> audiodelays.Echo -> audiomixer.MixerVoice
audiomixer.Mixer -> audiobusio.I2SOut

In either of the two examples above, the global block ticking variables (ie: uint8_t synthio_global_tick) would be updated by two separate entities, causing uneven value changes especially when dealing with different buffer sizes. Working with two synthio.Synthesizer objects would likely do the same thing as well.

I think the solution is to store these global variables locally on each object and add them as arguments to synthio_block_slot_get. That should solve this problem. Sharing synthio.BlockInput objects among different audio objects would still be an issue.

That all said, I still don't plan to include this fix here, and it should instead be addressed in a separate PR connected to #9872.

In either of the two examples above, the global block ticking variables (ie: uint8_t synthio_global_tick) would be updated by two separate entities

This is why the rule exists that a block can only be associated with one audio source and associating it with more than one audio source is an undiagnosed error or undefined behavior, however you want to call it.

Here's why (I think that) a single global tick is sufficient, given that rule: synthio_global_tick is only ever compared for equality with last_tick. This means it's not a problem if 2 different audio sources both increment it; both of them will tick their associated blocks once, by one tick, the exact details being determined by the global rate scale (and, for filters, the global W scale).

As a footnote, this also ignores the possibility that synthio_global_tick is ticked exactly 256 times, as it is an 8 bit quantity for no good reason, it uses up 4 bytes of storage anyway:

typedef struct synthio_block_base {
    mp_obj_base_t base;
    uint8_t last_tick;
    mp_float_t value;
} synthio_block_base_t;

so with two synthesizers, a block used in synth A will see synthio_global_tick values like 0, 2, 4, ... and a block used in synth B will see ticks like 1, 3, 5, .... But it doesn't matter, since the tick value is always "different than the last time".

Why does the synthio_global_tick exist at all? So that synthio_block_slot_get knows whether it needs to call a particular block's tick or just return the old value. So for instance if you use the same LFO as the pitch bend of two notes, it still only gets advanced once despite two synthio_block_slot_get calls on it during the same tick. Otherwise, I'd have to explicitly track a lot more information, or not allow two uses of one LFO output.

@jepler I see what you mean and why the uneven synthio_global_tick values wouldn't make a difference.

I put in some work to separate those three global variables into a struct that is passed through to all necessary block functions: main...relic-se:circuitpython:local_block_ticking. I'm good to scrap this in favor of your approach.

I've also changed my mind on the 256 sample increments, and I think I'll make the audioeffects just tick the block inputs based on the size of the buffer. It'll make for a much simpler update.

@jepler I've updated shared_bindings_synthio_lfo_tick to support a variable number of samples and all audio effects to implement this call. I see there is a conflict with your PR, #9804 (https://github.com/adafruit/circuitpython/pull/9804/files#diff-8b26aff15abcdd9f17032d54412d3c7eda19e104eb216351be5836a441a97f83R244). Let me know how we'd like to handle that situation (I'm not ticking block biquads within this PR, btw).

I've tested the following script with and without the update and can confirm that it works now:

import audiobusio
import audiocore
import audiodelays
import audiomixer
import board
import synthio

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

wave_file = open("StreetChicken.wav", "rb")
wave = audiocore.WaveFile(wave_file)

# Use mixer to decrease volume of sample
mixer = audiomixer.Mixer(
    voice_count=1,
    channel_count=wave.channel_count,
    sample_rate=wave.sample_rate,
)
mixer.voice[0].level = 0.5

effect = audiodelays.Echo(
    delay_ms=synthio.LFO(rate=0.2, scale=100, offset=250),
    freq_shift=True,
    channel_count=wave.channel_count,
    sample_rate=wave.sample_rate,
)

audio.play(effect)
effect.play(mixer)
mixer.voice[0].play(wave, loop=True)

Same goes for this test with a synthio.Synthesizer sample. It works with and without the update (since the synth was also handling lfo ticking):

import audiobusio
import audiodelays
import board
import synthio
import time

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

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

effect = audiodelays.Echo(
    delay_ms=synthio.LFO(rate=0.2, scale=100, offset=250),
    freq_shift=True,
    channel_count=2,
    sample_rate=22050,
)

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

while True:
    synth.press(65)
    time.sleep(0.1)
    synth.release(65)
    time.sleep(0.9)

If you test the above examples with a large buffer_size on the effect, let's say 16384, you can clearly hear the stepping in the delay time of the output. Though it is very unlikely that a user would use a buffer_size of this amount, it does bring up a valid case for keeping lfo ticking to SYNTHIO_MAX_DUR intervals.

I've moved the block value calls around in order to force SYNTHIO_MAX_DUR intervals within the audio effects buffer loop. This fixes the large buffer issue. Here's another example to demonstrate this:

import audiobusio
import audiocore
import audiofilters
import board
import synthio

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

wave_file = open("StreetChicken.wav", "rb")
wave = audiocore.WaveFile(wave_file)

import audiofilters
effect = audiofilters.Distortion(
    mix=synthio.LFO(rate=0.5, offset=0.5, scale=0.5),
    pre_gain=-30,
    drive=0.75,
    channel_count=wave.channel_count,
    sample_rate=wave.sample_rate,
    buffer_size=32768, # large buffer size
)

effect.play(wave, loop=True) # if effect.play is called after audio.play, a delay of over 1s will occur while it fills the first buffer
audio.play(effect)

These changes will like cause merge conflicts with #9876 regarding audiodelays.Echo.