GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — synthio: Fix note dropped when re-pressed after decay by bdbarnett · Pull Request #11289 · adafruit/circuitpython

Conversation

@bdbarnett

@bdbarnett bdbarnett commented Sep 2, 2026

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bdbarnett
synthio: Fix note dropped when re-pressed after decay
e1b52a3
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.

Route that case through the same envelope init a fresh press uses.
@dhalbert

dhalbert commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

FYI @relic-se @todbot; review welcome.

relic-se reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@relic-se

relic-se commented Sep 2, 2026

Copy link
Copy Markdown

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;
        }
    }
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bdbarnett

bdbarnett commented Sep 4, 2026

Copy link
Copy Markdown
Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@relic-se

relic-se commented Sep 4, 2026

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tannewt
tannewt approved these changes Sep 4, 2026
View reviewed changes

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tannewt
tannewt merged commit a60677c into adafruit:main Sep 4, 2026
1073 of 1074 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bdbarnett
bdbarnett deleted the fix-note-repress-envelope-zero branch October 19, 2026 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tannewt tannewt tannewt approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@bdbarnett @dhalbert @relic-se @tannewt
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.