fix(sim): monophonic buzzer guard — replace note on pitch change (no stacking)

A melody / continuous tone (consecutive tone() with no noTone() between) is
back-to-back nonzero-OCR PWM writes with no note-off, so startTone() overwrote
activeOsc without stopping the previous node — oscillators stacked and were
never stopped (reported: created 6, started 6, never stopped 6).

Add a monophonic guard at the top of startTone(): release the live note
(gain ramp + stop) before starting the new one, so a pitch change REPLACES
rather than STACKS. Extract a shared releaseActive(off) helper (also used by
stopTone). Add two melody tests: one asserts starts === stops (no orphans),
monotonic onsets and per-note pitch; one asserts a melody ending without a
trailing noTone() leaves only the final note ringing (stops === starts - 1).

The metronome path is unaffected (each click is an onset→note-off pair, so the
guard never fires there); the three existing metronome tests stay green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
ciegovolador 2026-06-11 02:35:33 -03:00
parent b06f500ad6
commit 6a1f79e331
2 changed files with 136 additions and 12 deletions

View File

@ -196,4 +196,101 @@ describe('Buzzer — metronome quality', () => {
const lastGap = (ns[ns.length - 1].on - ns[ns.length - 2].on) * 1000;
expect(Math.abs(lastGap - 250)).toBeLessThan(10);
});
// Regression guard for the maintainer's review (PR #220, comment 4671821612):
// a melody / continuous tone is consecutive tone(pin, freqN) calls with NO
// noTone() between pitches — back-to-back nonzero OCR writes, each firing the
// PWM handler with duty>0 and no intervening note-off. The old code overwrote
// activeOsc on every pitch change without stopping the previous node, so
// oscillators were "started but never stopped" — they stacked and played
// forever. The monophonic guard must REPLACE the live note instead.
it('replaces rather than stacks oscillators on a melody / continuous tone', () => {
const { sim } = setupBuzzer();
const pwm = sim.pinManager.onPwmChange.mock.calls[0][1] as (
p: number,
dc: number,
t?: number,
) => void;
// A little tune: pitch changes with no note-off between them (legato).
const melody = [523, 587, 659, 698, 784, 659]; // C5 D5 E5 F5 G5 E5
const step = 200; // ms per note
melody.forEach((freq, i) => {
sim.cpu.data[OCR2A] = ocrFor(freq);
sim.cpu.data[TCCR2B] = 0x04; // CS22 -> prescaler 64
clock = (i * step) / 1000;
pwm(11, 0.5, i * step); // square-wave duty>0, pitch change, NO dc=0
});
// End the tune with a noTone — releases the final note.
clock = (melody.length * step) / 1000;
pwm(11, 0, melody.length * step);
const starts = sched.filter((e) => e.kind === 'start');
const stops = sched.filter((e) => e.kind === 'stop');
// Every oscillator that started is stopped — no orphans left ringing. This
// is the exact failure the maintainer saw ("started but never stopped: 6").
expect(starts.length).toBe(melody.length);
expect(stops.length).toBe(starts.length);
// One start per note, at the right pitch, in order. Compare against the
// CTC-reconstructed pitch (what the firmware's integer OCR actually yields),
// not the nominal note — same round-trip the buzzer's getFrequency does.
const heard = (freq: number) => Math.round(125000 / (ocrFor(freq) + 1));
starts.forEach((s, i) => {
expect(s.freq).toBe(heard(melody[i]));
if (i > 0) expect(s.when).toBeGreaterThan(starts[i - 1].when); // monotonic, no backward
});
// Pitch is read FRESH per note (not stuck on the first onset): the tune rises
// then falls, so the heard sequence is non-constant and tracks the melody.
const heardSeq = starts.map((s) => s.freq);
expect(new Set(heardSeq).size).toBeGreaterThan(1);
expect(heardSeq).toEqual(melody.map(heard));
// Bounded overlap (replacement, not stacking): each note is released close to
// the NEXT note's onset — not smeared to the end of the tune. Pair each start
// with its own stop (interleaved start/stop/start/stop… once the guard fires).
const notesSeq: { on: number; off: number }[] = [];
for (let i = 0; i + 1 < sched.length; i++) {
if (sched[i].kind === 'start' && sched[i + 1].kind === 'stop') {
notesSeq.push({ on: sched[i].when, off: sched[i + 1].when });
}
}
expect(notesSeq.length).toBe(melody.length);
for (let i = 0; i < notesSeq.length - 1; i++) {
// old note ends as the next begins (≤ a release tail past the next onset)
expect(notesSeq[i].off).toBeLessThanOrEqual(notesSeq[i + 1].on + 0.01);
expect(notesSeq[i].off).toBeGreaterThan(notesSeq[i].on); // positive duration
}
});
// A melody that ends WITHOUT a noTone() — the real "sketch loops tone() and
// never calls noTone()" pattern. Correct Arduino semantics: a tone() plays
// until noTone() or the NEXT tone(), so the final note must keep ringing. The
// guard must release exactly the SUPERSEDED notes (one stop each) and leave the
// last one sounding — not orphan the middle notes, not cut the last one short.
it('releases superseded notes but leaves the final note ringing (no trailing noTone)', () => {
const { sim } = setupBuzzer();
const pwm = sim.pinManager.onPwmChange.mock.calls[0][1] as (
p: number,
dc: number,
t?: number,
) => void;
const melody = [440, 494, 523]; // A4 B4 C5
const step = 200;
melody.forEach((freq, i) => {
sim.cpu.data[OCR2A] = ocrFor(freq);
sim.cpu.data[TCCR2B] = 0x04;
clock = (i * step) / 1000;
pwm(11, 0.5, i * step); // pitch change, NO dc=0 — and no noTone at the end
});
const starts = sched.filter((e) => e.kind === 'start');
const stops = sched.filter((e) => e.kind === 'stop');
// Every note starts; only the superseded ones stop → exactly one note (the
// last) is still live. Pre-guard this was starts=3, stops=0 (all orphaned).
expect(starts.length).toBe(melody.length);
expect(stops.length).toBe(starts.length - 1);
});
});

View File

@ -591,10 +591,47 @@ PartSimulationRegistry.register('buzzer', {
return when;
}
// Ramp the note currently sounding down to silence ending at audio time
// `off` and schedule its stop. Shared by stopTone (note-off) and the
// monophonic guard in startTone (a pitch change with no note-off). Keeps the
// envelope valid: never release before this note's own attack has finished,
// nor in the past.
//
// Bounded-overlap note (guard path): on a normal metronome/melody — onsets
// tens-to-hundreds of ms apart — the old note ends ~RELEASE before the next
// onset. On a degenerate sub-4 ms onset (a >250-note/s trill, or two tone()
// calls at the same simulated timestamp — neither of which a passive buzzer
// produces) the `onWhen + ATTACK` floor pushes `off` past the next onset, so
// two oscillators overlap for at most ~ATTACK+RELEASE (≈5 ms). That is
// inaudible and still leak-free (one stop per note). We deliberately keep the
// attack-finished envelope rather than clamp `off` down to the onset, which
// would start the down-ramp from a gain that never reached its peak.
function releaseActive(off: number) {
const ctx = audioCtx;
if (!ctx || !activeOsc || !activeGain) return;
if (onWhen !== null && off < onWhen + ATTACK + 0.002) off = onWhen + ATTACK + 0.002;
if (off < ctx.currentTime + 0.003) off = ctx.currentTime + 0.003;
try {
activeGain.gain.setValueAtTime(0.1, off);
activeGain.gain.linearRampToValueAtTime(0, off + RELEASE);
activeOsc.stop(off + RELEASE + 0.001);
} catch {
/* already scheduled */
}
activeOsc = null;
activeGain = null;
}
function startTone(freq: number, timeMs?: number) {
ensureCtx();
const ctx = audioCtx!;
const when = whenFor(timeMs); // the scheduler tracks ONSETS only (clean rhythm)
// Monophonic guard: a pitch change with no intervening note-off (a melody —
// consecutive tone() calls) must REPLACE the current note, not stack a new
// oscillator on top. Release the live note so it ends as the new one begins
// (seamless legato) instead of orphaning it to play forever. Reads the
// PREVIOUS note's onWhen, so it must run before onWhen is reassigned below.
if (activeOsc && activeGain) releaseActive(when);
onWhen = when;
onSimMs = timeMs ?? null;
const osc = ctx.createOscillator();
@ -626,21 +663,11 @@ PartSimulationRegistry.register('buzzer', {
// Note-off relative to its own onset, preserving the exact click length
// from the simulation (not via the onset scheduler, which would smear
// the short on→off and long off→on gaps together).
let off =
const off =
onWhen !== null && onSimMs !== null && timeMs !== undefined
? onWhen + Math.max(0.004, (timeMs - onSimMs) / 1000)
: ctx.currentTime + 0.02;
if (onWhen !== null && off < onWhen + ATTACK + 0.002) off = onWhen + ATTACK + 0.002;
if (off < ctx.currentTime + 0.003) off = ctx.currentTime + 0.003;
try {
activeGain.gain.setValueAtTime(0.1, off);
activeGain.gain.linearRampToValueAtTime(0, off + RELEASE);
activeOsc.stop(off + RELEASE + 0.001);
} catch {
/* already scheduled */
}
activeOsc = null;
activeGain = null;
releaseActive(off);
}
isSounding = false;
if (el.playing !== undefined) el.playing = false;