synthio: Fix note dropped when re-pressed after decay

[bdbarnett]

A note released and then re-pressed after its envelope has run down to 0
is silently dropped. synth.pressed reports it as pressed, and then the
next render loses it.

synthio_span_change_note()'s fast path for a note still on its channel
re-enters ATTACK without touching level. At level 0 the render loop's
"note is truly finished" check reaps the channel before the envelope is
stepped, so the re-press never sounds.

Test included; fails on main, passes with the fix.

[dhalbert]

FYI @relic-se @todbot; review welcome.

[relic-se]

The logic makes sense, but what if we instead changed synthio_envelope_state_step to not allow this to occur in the first place.

We can change the return type to flag if the envelope is done:

static bool synthio_envelope_state_step(synthio_envelope_state_t *state, synthio_envelope_definition_t *def, size_t n_steps) {
    state->substep += n_steps;
    while (state->substep >= SYNTHIO_MAX_DUR) {
        // max n_steps should be SYNTHIO_MAX_DUR so this loop executes at most
        // once
        state->substep -= SYNTHIO_MAX_DUR;
        switch (state->state) {
            case SYNTHIO_ENVELOPE_STATE_SUSTAIN:
                break;
            case SYNTHIO_ENVELOPE_STATE_ATTACK:
                state->level = MIN(state->level + def->attack_step, def->attack_level);
                if (state->level == def->attack_level) {
                    state->state = SYNTHIO_ENVELOPE_STATE_DECAY;
                }
                break;
            case SYNTHIO_ENVELOPE_STATE_DECAY:
                state->level = MAX(state->level + def->decay_step, def->sustain_level);
                if (state->level == def->sustain_level) {
                    state->state = SYNTHIO_ENVELOPE_STATE_SUSTAIN;
                }
                break;
            case SYNTHIO_ENVELOPE_STATE_RELEASE:
                state->level = MAX(state->level + def->release_step, 0);
                if (state->level == 0) {
                    return false; // Indicate that note envelope has completed
                }
        }
    }
    return true;
}

And then go into synthio_synth_synthesize to handle that case:

    // advance envelope states
    for (int chan = 0; chan < CIRCUITPY_SYNTHIO_MAX_CHANNELS; chan++) {
        mp_obj_t note_obj = synth->span.note_obj[chan];
        if (note_obj == SYNTHIO_SILENCE) {
            continue;
        }
        if (!synthio_envelope_state_step(&synth->envelope_state[chan], synthio_synth_get_note_envelope(synth, note_obj), dur)) {
            // Envelope has completed, go ahead and reset channel
            synth->span.note_obj[chan] = SYNTHIO_SILENCE;
        }
    }

[bdbarnett]

Agreed on the direction — level == 0 is the ambiguity that causes this, and
reporting completion from the step is the right place to resolve it.

One catch. It only fixes the re-press if the level == 0 reap in
synthio_synth_synthesize goes away (it runs before the envelope advance, so
otherwise it still claims the channel first). And that reap is doing real work
for envelopes with sustain_level = 0: they decay to silence and are never
released, so with completion reported only from RELEASE they would hold their
channel until an explicit note-off that percussion never sends.

Measured on a unix build, drum-shaped envelope (attack 1 ms, decay 20 ms,
sustain_level=0), pressing until refused:

channels filled: 14
still pressed after decay: 0
a further press after decay is accepted: True

With the patch as written those 14 stay pressed, and on a 14-channel build a
percussion kit runs out of voices.

So: return false whenever the level reaches 0 and cannot rise again without a
new press — the decay branch landing on a sustain_level of 0 as well as the
release branch.

case SYNTHIO_ENVELOPE_STATE_DECAY:
    state->level = MAX(state->level + def->decay_step, def->sustain_level);
    if (state->level == def->sustain_level) {
        if (state->level == 0) {
            return false;
        }
        state->state = SYNTHIO_ENVELOPE_STATE_SUSTAIN;
    }
    break;

Then the ambiguous check can go and both cases are handled at the
source rather than worked around.

Happy to redo the PR that way if you prefer it to the current fix.

[relic-se]

Just adding another level == 0 check to the decay case handles that potential issue with minimal additional complexity. Yep, I vote go ahead and either rework this one or start up another PR with this new direction.

[tannewt]

Let's merge and we can always change the internals later.