diff --git a/frontend/src/__tests__/buzzer-metronome.test.ts b/frontend/src/__tests__/buzzer-metronome.test.ts index 699ff911..f44b3e52 100644 --- a/frontend/src/__tests__/buzzer-metronome.test.ts +++ b/frontend/src/__tests__/buzzer-metronome.test.ts @@ -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); + }); }); diff --git a/frontend/src/simulation/parts/ComplexParts.ts b/frontend/src/simulation/parts/ComplexParts.ts index 478a86ac..6f1b60af 100644 --- a/frontend/src/simulation/parts/ComplexParts.ts +++ b/frontend/src/simulation/parts/ComplexParts.ts @@ -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;