Conversation
- Track running min/max continuously and derive midpoint/amplitude at the end of the warm-up window (finishWarmup), used by later tasks to place a Schmitt-trigger threshold instead of a hard 0.0 threshold. - Report DetectorStatus::failedNoCrossings when the warmed-up amplitude is below silenceFloor, avoiding a divide-by-zero downstream. - tests: cover DC-offset sine, asymmetric ramp, and silence handling. - tests/CMakeLists.txt: run catch_discover_tests with DISCOVERY_MODE PRE_TEST so test enumeration happens after CodeSign; on Apple Silicon with the Xcode generator, enumerating at build time runs the unsigned binary and the kernel SIGKILLs it.
…mment
- Add a test that feeds the DC-offset sine through processBlock in 64
sample chunks (matching the real audio callback's 256-512 sample
buffers) and asserts midpoint()/amplitude() are identical to the
single-block result, closing a coverage gap a future refactor could
silently break through (member-scoped running state, finishWarmup()
guarded to fire once).
- Correct the processBlock comment: min/max keep updating continuously,
but levelMidpoint/levelAmplitude are latched once in finishWarmup() and
are not recomputed afterward. The previous wording ("follows slow level
changes... not just the warm-up window") was inaccurate about the
latched values, copied verbatim from the design spec, which has the
same error. Behaviour is intentionally unchanged: a threshold that
drifted mid-measurement would inject timing error into the periods
being measured, so the level is latched once at end of warm-up.
The 'trigger level adapts to very quiet and very hot signals' test asserted numPeriods() in [435, 441], but the fixture (48000 samples at 440 Hz, 480-sample warm-up) deterministically yields 434 for every amplitude, as verified independently against the committed PeriodDetector. The count was never in question; the bound was off by one. The test's actual purpose - proving the trigger level scales with measured amplitude - is better expressed by asserting the count is identical across quiet, mid, and hot signals, with a loose sanity range around it.
The spec asserted the detector follows slow level changes; it does not, and should not - a trigger threshold that drifts mid-measurement injects timing error into exactly the periods being measured. The level is latched at the end of warm-up by design. The plan's amplitude-adaptation test asserted a period count of >=435, written by estimate. The fixture yields 434 deterministically: warm-up consumes 4.4 cycles, the trigger needs up to another half cycle to arm, and the first crossing has no predecessor. The test now asserts amplitude-independence, which is what it was named for.
The previous formula used a negated slope and paired the previous sample with the wrong x coordinate, mirroring the fractional crossing position within the sample interval. It produced roughly twice the jitter of no interpolation at all, inflating error bars and causing spurious high jitter aborts above ~MIDI 100.
Adds numValidPeriods()/validPeriods() and the stability-window check that latches firstValidIndex once cfg.stabilityWindow consecutive periods fall within cfg.stabilityTolerance of their average. updateStability() sets DetectorStatus::stable once enough valid periods follow, and otherwise distinguishes the two terminal failure states when storage runs out: failedUnstable (never latched) versus failedBufferFull (latched, but requiredPeriods wasn't reached in time). It is called at the end of recordCrossing, after push_back, so it runs on the call that pushes the period reaching maxPeriods - recordCrossing's early-return guard means every later call is a no-op. Known failing test: "a constantly changing rate never stabilises and terminates" expects failedUnstable for a 200 Hz -> 2 kHz linear sweep, but the detector reports stable instead. At the start of the sweep the first five periods (209.3, 202.5, 196.3, 190.7, 185.5 samples) fall within the 10% stabilityTolerance window around their average, so firstValidIndex latches after period 5. Because updateStability() never re-checks steadiness once latched, the detector keeps counting through the ongoing drift and reaches requiredPeriods=20 at period 25, by which point the period has drifted to ~128.8 samples (roughly 39% off the latch point). This is a real interaction between the specified latch-once stability check and a slowly-varying sweep, not a typo in the bound. Left as-is per task instructions not to loosen/tighten the test to force a pass; needs a decision on whether stability should be re-validated continuously or the sweep fixture should change.
…stable updateStability() latched firstValidIndex on the first 5-period window that held within stabilityTolerance, then never rechecked it: a signal that satisfies one lucky window and then drifts arbitrarily far still counted every subsequent period toward requiredPeriods and was declared stable. A drifting oscillator - thermal drift, a mistracking VCO, a note still gliding - could hit exactly this path and produce a confident, wrong measurement, which is the failure class this branch exists to remove. (The shipping app has the same hole: Source/VCOTuner.cpp sets indexOfFirstValidPeriodLength once and never rechecks it either.) Before declaring stable, now re-checks every period in validPeriods() (the post-latch set only - the pre-latch entries are the settling transient we intend to exclude) against the mean of that same set, using the existing stabilityTolerance. Any period outside tolerance reports failedUnstable instead, which is the status that fits: a rate that never actually held steady. Fixes the previously-reported false positive on "a constantly changing rate never stabilises and terminates": the sweep's first five periods (209.3 down to 185.5 samples) happened to satisfy the single-window check, latching firstValidIndex=5; by the time 20 valid periods existed (periods[5..24], 180.7 down to 128.8) the set no longer held within 10% of its own average (average 150.6, boundary +-15.1; periods[5..8] and periods[21..24] all exceed it) and the new check now correctly reports failedUnstable instead of a spurious stable.
The plan's stability gate latched after one 5-period window and never rechecked, so a drifting oscillator could satisfy one lucky window and be reported as a confident measurement. The spec requires stable to mean genuinely steady.
Address review findings on Task 6:
- Add a computeMeasurement case using the analytic {8, 11, 9, 12} fit so
frequencyDeviation and pitchDeviation are asserted against literal,
hand-derived nonzero values, plus guards against the two named
mis-derivations (dropped /periodSamples, omitted 1/ln2). Every prior
computeMeasurement test used uniform periods, so periodStdError was
always zero and neither formula was actually exercised.
- Cover numPeriods == 2 for both fitPeriod and computeMeasurement: the
only degenerate input that reaches the sse/((n-2)*sxx) division rather
than returning early.
- Correct the fitPeriod docstring, which claimed a 3-period minimum; the
code has always accepted 2 periods (1 degree of freedom), and that is
correct as-is per review: a wide standard error communicates low
confidence rather than discarding the measurement.
- Reword the sxx <= 0.0 guard's comment to make clear it is unreachable
by design given the current input contract, not load-bearing.
Extract the state machine's timeout calculation into a pure, testable function, computeTimeoutCycles(). The old formula (roundToInt(1/f * numPeriods * 2 * 100)) rounds to zero at high pitches - e.g. MIDI 120 at 20 periods gives round(0.0048 * 100) = 0 - so the measurement gets roughly one 10ms timer tick to complete, well under a typical MIDI-plus-audio round trip. That aborts every note above roughly MIDI 108. The new function floors the result to 50 cycles (500ms at the default 10ms timer) and adds a fixed latency allowance. Adjusted the brief's "the latency allowance is included" test: at 440 Hz with numPeriods=20, the raw (unfloored) cycle counts for the no-latency and 0.3s-latency cases are 10 and 40 - both still under the 50-cycle floor - so both collapse to 50 and "with > without" cannot pass for any implementation that honours the documented floor. Raised numPeriods to 400 (already used elsewhere in this file) so both sides clear the floor and the comparison is meaningful.
reset() is called from the real-time audio callback and reserves the period buffer there, which is a heap allocation on the audio thread and can cause dropouts. prepare() reserves the same capacity up front from a non-realtime thread, so the reserve inside reset() asks for a capacity the buffer already has and does nothing. No change to reset()/processBlock()/status() semantics.
A note that cannot be measured is recorded in a FailureTracker and reported to the listeners through measurementFailed(), and the sweep moves on to the next pitch instead of aborting with a modal dialog. Only the three fatal errors still stop the run. This also wires vcotuner::PeriodDetector into the audio callback, which had to land in the same commit: removing lError and the LowLevelError enum breaks the state machine cases that this change rewrites. The reference measurement keeps aborting on failure. Every other note's pitch is expressed relative to the reference frequency, so a sweep without one would be meaningless. Also: - guards numInputChannels before reading channel 0, which previously read out of bounds on a device with no enabled input channels - makes the start/stop flags atomic, and publishes the detector status through an atomic, since both are shared with the audio thread - takes the frequency, pitch and their uncertainties from computeMeasurement()/fitPeriod() and the timeout from computeTimeoutCycles() - stops the sweep on the no-frequency-change error, which previously reported the error and then carried on regardless
failCurrentNote() armed stopMeasurement unconditionally, including on the path where the audio thread had already finished and cleared its own state. prepMeasurement then handed over a new run without checking the flag, so a callback arriving after the 100 ms settle window - a buffer period above 100 ms is enough - consumed the stale request and killed the run it was meant to start. The state machine read 'collecting', failed the note, armed the flag again, and every remaining note in the sweep failed the same way. Now the flag is only armed when a run really is in flight, and prepMeasurement waits for the audio thread to acknowledge it, matching what prepareSingleMeasurement and prepareContinuousFrequencyMeasurement already did. Also: - clears the output per channel, skipping the null pointers that JUCE gives for channels not enabled when the device was opened. Moving the clear ahead of the early returns had put it on that path. - initialises continuousFreqMeasurementResult to -1 and resets it in startContinuousMeasurement. Keeping the previous reading when a pass does not settle means it is no longer written on every pass, and ReportPrepScreen advances the report wizard on its value. - asserts at the reset() call site that cfg.maxPeriods still matches the capacity reserved in the constructor, which is what keeps the audio callback allocation-free.
Visualizer now implements VCOTuner::Listener::measurementFailed and tracks failed pitches separately from measurements. Auto-scaling skips failed notes so one bad reading no longer stretches the display range and squashes the real tuning curve. Failed notes are drawn in orangered instead of green/springgreen. A pitch is removed from failedPitches as soon as newMeasurementReady delivers a fresh reading for it, so a note that fails on one sweep cycle and then measures correctly on the next (e.g. after the user adjusts a trimmer) recovers its normal colour and rejoins auto-scaling without waiting for a full clearCache().
Two defects from code review, both present in the original task brief: 1. measurementFailed() only touched failedPitches, never measurements. Since the drawing loop iterates measurements and uses its index as the column position, a pitch that failed before ever succeeding had no column at all -- the graph just compacted around the gap, so the common case (first-attempt failure, or any failure right after clearCache()) was invisible. Fixed by factoring the existing add-or-replace logic out of newMeasurementReady() into a shared upsertMeasurement() helper, and having measurementFailed() upsert a zeroed placeholder (numMeasurements == 0) keyed on midiPitch. A later successful reading for the same pitch overwrites the placeholder via the same helper, so a pitch never holds more than one column. 2. orangered vs green is a near-luminance-matched red/green pair with no reliable brightness cue, and the average-value line carried no other cue at all. A failed note's point/band data is also meaningless (zero would misleadingly read as perfectly in tune), so rather than just recolouring it, failed columns now render as a translucent full-height column fill plus a bold orangered X, replacing the point/band entirely. Colour is now reinforcement, not the only signal. The auto-scaling exclusion (failedPitches.contains() in the min/max loop) already skips placeholder entries for the same reason it skips any failed pitch, so no change was needed there.
MainComponent now distinguishes live tuning from a report: - Live tuning (cycle == true) never raises a dialog for a failed note. A status label below the graph names the currently failing notes, cleared on every tunerStarted() so a note that recovers on the next cycle drops off immediately. Long failure lists are capped at 10 pitches with a "(+N more)" suffix. - A report (a single sweep with a real end) gets one consolidated dialog listing every failure when tunerFinished() fires. - Fatal errors are unchanged: they still abort and show a dialog via tunerStopped(), in both modes. Also settles three messaging gaps left open by earlier tasks: - Added a dedicated Errors::bufferFull message (VCOTuner.h/.cpp) and a matching describeError() case, instead of lumping a full measurement buffer in with 'unsteady rate'. describeError() lives in VCOTuner.cpp and is declared outside the class in VCOTuner.h, since Source/dsp/ must not depend on JUCE's String. - Reworded Errors::stableTimeout: it no longer asserts the crossings ARE arriving at a constant rate, since 'collecting' also covers a warm-up that never finished on a weak signal. - Left MeasurementError::highJitterTimeOut in place with a comment recording that it's unreachable now that the detector always reaches a terminal status itself (MeasurementErrorTests.cpp still asserts on it), but dropped the matching VCOTuner::Errors::highJitterTimeOut string, which nothing referenced.
… ends Task 12's summary dialog was gated on MainComponent::creatingReport, but nothing ever set that flag: the Create Report button launches ReportCreatorWindow with no reference back to MainComponent, and the report's own measurement is driven entirely by ReportDetailsEditorScreen (tuner->start()/startSingleMeasurement()), which registers itself as a VCOTuner::Listener independently. The summary was therefore unreachable in practice. Moved the fix to where the report actually finishes: ReportDetailsEditorScreen::tunerFinished() runs through two stages - the main sweep, then a single-note reference re-measurement to check for drift - so tunerFinished() fires more than once per report. The true end is the reMeasuringReference stage's two terminal branches (drift within margin, or the user chooses to keep a drifted result anyway), both marked by tunerHasFinished = true. Neither the 'repeat the measurement' branch nor the 'cancel' branch reaches this, since those are not the report finishing. Both branches now call the new showMeasurementFailureSummary() (VCOTuner.h/.cpp), reusing the exact describeError()-based wording from Task 12's MainComponent dialog so the two paths stay consistent. tuner->getFailures() is unaffected by the reference re-measurement in between (only a full sweep restart resets the failure tracker), so it still reflects the main sweep's failures at this point. Removed MainComponent::creatingReport, startCreatingReport() and the reportRange constant that only startCreatingReport() used: all three were part of the same dead, unwired mechanism (the real report uses ReportProperties, not MainComponent::reportRange), and leaving them in place risked a future double-dialog if someone ever did wire startCreatingReport() up. MainComponent::tunerFinished() no longer contains any dialog-raising code at all, which is a stronger guarantee than before that live tuning can never show one.
Errors::bufferFull and describeError's bufferFull case described failedUnstable (never reached a steady rate) instead of what DetectorStatus::failedBufferFull actually means. PeriodDetector.cpp's firstValidIndex check (see PeriodDetector.h's DetectorStatus comments) shows the two terminal states at maxPeriods are distinguished exactly because they're opposites: failedUnstable is 'never found a steady window', failedBufferFull is 'found one, just not for long enough to finish before the buffer filled'. The old wording told a user with a perfectly steady oscillator that their signal never settled, which could send them chasing a hardware problem that isn't there. Reworded both strings to say the signal did settle but didn't hold long enough before the buffer ran out - the 'try a lower resolution' remedy was already correct and is unchanged. Kept the two strings consistent with each other and distinct from highJitter's 'never settles' framing, which is what motivated splitting bufferFull out as its own case in the first place.
… note failCurrentNote() advanced currentIndex, but currentIndex has exactly one consumer: the "MIDI-to-CV interface isn't responding" check, which is gated on currentIndex == 0 so that it runs on the first measurement. Advancing it on failure meant that when the first note failed for any reason, the check never ran again for the whole sweep - a user whose interface is on the wrong MIDI channel then got a full-length sweep and a saved report showing a flat line at the reference frequency, with no error at all. currentIndex counts successful measurements again, restoring the invariant "currentIndex == 0 <=> no note has been measured successfully yet", so the check fires on the first successful measurement as it was meant to. Nothing else reads currentIndex; the Visualizer's column ordering comes from insertion order into its own array, keyed on midiPitch.
prepRefMeasurement was the only prep state that did not wait for a pending stop request to drain before starting a new run. switchState(stopped) always arms stopMeasurement, and MainComponent::comboBoxChanged does stop-then- restart within a single message-thread call stack, so changing the pitch range or resolution while running arms the flag roughly 100 ms before startDetectorRun() sets startMeasurement. On a device whose buffer period is longer than that (4096 frames at 44.1 kHz is 93 ms, 8192 is 186 ms) no audio callback intervenes, the next one consumes the stale flag and kills the reference run, and the user sees a spurious timeout error. The plain guard the other three states carry trades that rare spurious abort for a silent unbounded hang: only the audio callback clears the flag, so with no device running nothing ever does. All four states now share one bounded wait instead. awaitingStopRequest() counts the cycles spent waiting and, after 100 of them (one second at the 10 ms timer), reports the audio-device error and stops rather than waiting forever. One second is several times the longest realistic buffer period, so a legitimate slow start still proceeds. The wait uses its own counter rather than cycleCounter: cycleCounter gates the MIDI note-on (cycleCounter == 0) and the 100 ms oscillator settling window, so advancing it while waiting would skip the note-on entirely and cut the settling time short.
isFatal(), errorForStatus() and errorMessageForStatus() all deliberately omit a default label so that -Wswitch flags a future MeasurementError enumerator that nobody has classified. describeError() had one alongside its explicit cases, which silently swallowed exactly that. Every enumerator was already listed, so removing the label needs no new cases - just the trailing return the other three already use.
The three CI jobs that produce the shipped artifacts configure with a bare `cmake -B build`. With VCOTUNER_BUILD_TESTS defaulting to ON, each of them FetchContent'd and compiled Catch2 from github.com on every push, and a plain local configure needed network access. The design spec accepted that network dependency for the test job only, and the unitTests job already passes -DVCOTUNER_BUILD_TESTS=ON explicitly, so the default can be OFF.
The three failure statuses each have a bespoke fixture, but nothing pinned
the arithmetic between requiredPeriods (up to 400), stabilityWindow (5) and
maxPeriods (600) across the settings a user can actually select. A
table-driven case over MIDI {24, 60, 96} x resolution {20, 100, 400} against
a clean sine now asserts stable and numValidPeriods() >= requiredPeriods for
each. Raising the top resolution past 595 fails here rather than in a user's
sweep.
Re-validating steadiness across the whole valid set means a single two-sample click injected mid-capture into an otherwise perfect 440 Hz sine at requiredPeriods = 400 flips the detector from stable to failedUnstable. That was an unrecorded consequence of the change; this states it, so it is a specification rather than a surprise. The behaviour is deliberate: a glitched capture is failed and marked rather than absorbed into the fit's error bars, where it would yield a wrong frequency carrying a plausible-looking uncertainty. failedUnstable maps onto the non-fatal highJitter error, so the sweep carries on past the note - also asserted here.
Nothing asserted the invariant computeTimeoutCycles() exists for - that the timeout covers the time the measurement needs. The existing `cycles >= 50` check is tautological given the std::max floor, so a re-derivation that was non-zero but too short would still pass. That is precisely the failure mode of the original bug.
The comment on "the latency allowance is included" pointed at task-8-report.md for the root-cause analysis. That file is gitignored and will not exist in any checkout; the comment already explains the reasoning without it.
updateStability() carried two near-verbatim copies of the same mean / boundary / tolerance logic - one over the newest window, one over the whole valid set. They now call a single file-local isWithinTolerance() helper, so the two cannot drift about what "steady" means. No behaviour change; the existing tests cover both paths.
runningMin and runningMax were updated for every sample of every block, but finishWarmup() is their only reader and it runs exactly once - so after warm-up the two comparisons per sample on the audio thread fed nothing. They are now inside the warm-up branch, which covers exactly the same samples as before (including the one that ends warm-up). The comment above them implied the continuous tracking was deliberate. It was a leftover of an earlier design that was explicitly retracted: the trigger level is latched once, because a level that drifted mid-measurement would inject timing error into the very periods being measured.
prepRefMeasurement had no guard at all rather than an unbounded one, so "every prep state used to wait here forever" was true of three states, not four. Say what the bound prevents instead.
currentIndex == 0 used to mean "first note of the sweep" back when any
failed measurement aborted the run. Now the sweep continues past
failures and currentIndex only advances on success, so it can still be
0 when the sweep reaches referencePitch. Since referencePitch is always
MIDI 60 in every shipped configuration and is always measured, a run of
early failures below 60 makes the reference pitch the first successful
measurement, whose frequency trivially equals referenceFrequency. That
tripped the "MIDI output is not working" gate and aborted the sweep
with a misleading error. Add a currentPitch != referencePitch guard so
the comparison is skipped for the one note where it is meaningless, and
correct the comment above it to explain why.
Also correct a test comment in PeriodDetectorTests.cpp that overclaimed
the resolution combo offers only "20, 100 and 400" periods per note;
the shipped set is {20, 50, 100, 200, 400} and the test samples three
representative values from it. The test's behavior is unchanged.
core.autocrlf has never been set and there was no .gitattributes, which already let one contributor's editor silently flip two files from CRLF to LF, caught and reverted by hand. Surveyed all tracked files outside the deps/ submodule: 18 files, all direct children of Source/, are raw CRLF; everything else, including Source/ReportProperties.cpp (a sibling of the CRLF Source/ReportProperties.h), is LF. The 18 CRLF files are stored as unnormalized CRLF blobs, never having passed through Git's clean filter, so the usual `text eol=crlf` pattern is unsafe here: Git always canonicalizes a text path's stored blob to LF and only reinjects CRLF on checkout, which would rewrite all 18 blobs into a huge, blame-polluting diff despite the checked-out bytes looking unchanged. They are marked `-text` instead, which keeps them byte-for-byte as committed while still stopping any local autocrlf setting or future broad gitattributes pattern from touching them. Verified with `git add --renormalize . && git status --short`, which shows only .gitattributes itself, plus `file` spot-checks confirming Source/VCOTuner.cpp and Source/ReportProperties.h are still CRLF and Source/ReportProperties.cpp and Source/dsp/PeriodDetector.cpp are still LF. No binary files are currently tracked outside deps/; common binary extensions are pre-declared for whatever gets added later.
Removes the MACOSX_DEPLOYMENT_TARGET=11.0 workaround the build has needed on macOS: JUCE 6.1.5 called CGWindowListCreateImage, which Apple obsoleted in the macOS 15 SDK, and the prefix was the only way to reach the nested juceaide bootstrap. Two API migrations were required. audioDeviceIOCallback() no longer exists; it is replaced by audioDeviceIOCallbackWithContext(), which also takes its channel pointers as T* const* rather than T**. The override is now marked 'override' deliberately: the base-class implementation of the new callback is an empty body, so keeping the old signature still compiles and launches while never receiving a sample. A fork of this project shipped in exactly that state. 'override' turns that mistake into a compile error. Font::getStringWidth() was removed along with the rest of the old text metrics. The pitch-axis label spacing in Visualizer now uses GlyphArrangement::getStringWidth(), which is what JUCE prescribes.
JUCE 8.0.15 requires CMake 3.22, so the 3.19 pin had to go. While there: actions/checkout and upload-artifact moved from the deprecated v2 to v4, the cmake setup action to v2, and the Windows generator from Visual Studio 16 2019 to 17 2022, which is what windows-latest now provides. Adds libfontconfig1-dev, newly required by juce_graphics in JUCE 8. Also adds the dependency step to the unitTests job, which never had one. The tests link no JUCE, but configuring the project still pulls JUCE in and bootstraps juceaide, so that job needed the same packages and would have failed on Linux regardless of this upgrade.
Drops the macOS build prefix, corrects the test counts to 33, and adds a section for the two things the build cannot prove - that audio reaches the detector and that MIDI actually sends - plus a check that the graph's axis labels still lay out correctly after the text-metrics change.
Covers what changed for someone using the app rather than reading the code: narrower and correctly-defined error bars, failed notes marked instead of killing the sweep, far fewer false unstable-signal errors, high notes completing, and drifting oscillators being flagged. Includes the expected consequence of the stricter stability check - a capture containing a click is now failed rather than absorbed into a wider bar - so it reads as intended behaviour rather than a regression. Adds build-from-source instructions, since the CMake floor moved to 3.22 and the test suite is not built by default, and the Gatekeeper workaround for unsigned macOS downloads.
Closes the gap reported upstream as issue TheSlowGrowth#23. A reading whose centre falls outside the range drew nothing at all, and a blank column reads as 'never measured' rather than 'worse than the range shows'. This only bites in the report, which plots a fixed +/-15 cents, so a badly tracking oscillator went blank at exactly the notes worth looking at. The live graph auto-scales to fit and can never trigger it. An arrow at the edge the reading ran off says which way it went, with direction rather than colour carrying the meaning.
Records that the behaviour is report-only, since the live graph auto-scales and can never trigger it, and notes what was verified against a synthetic fixture versus what still needs a real report.
More than a patch: measurements are corrected rather than merely fixed up, a failed note no longer ends the sweep, and the report now marks readings that fall outside its range.
AudioDeviceSelectorComponent overrides its own height in resized() to fit however many devices and channels are attached, so its bottom edge is not where the caller put it. The MIDI channel label and combo were positioned off that bottom edge, so when the selector grew they were pushed off the bottom of the fixed-size dialog - making the channel unreachable exactly when a device was connected, which is the only time you need to set it. The fixed-height controls are now laid out from the bottom of the dialog upward and the selector gets what is left. The selector is also added first so it sits behind them rather than on top, since it was previously added last and would cover them if it overflowed. The old behaviour was known - there was a comment in resized() saying the selector overwrites its height and 'it doesnt seem to work'.
…hey behave Links now go to this fork's releases, issues and clone URL rather than upstream's, with an attribution line at the top. The forum link follows the muffwiggler -> modwiggler rebrand (the old URL 301s there anyway). The error-bar paragraph claimed 'far narrower bars'. Hardware testing showed that is not what you see: on a steady signal the uncertainty is below one pixel and no band is drawn at all, and the band reappears when the pitch actually moves - turning a trimmer mid-sweep makes it grow. Described that instead, since it is both what happens and the more useful way to read it.
Builds all three platforms, then a release job collects the artifacts and publishes them. It is gated on the tag ref, so ordinary pushes still just build and test. The macOS .app is zipped with ditto on the macOS runner rather than in the release job: a bundle contains symlinks and zipping it on Linux would mangle them. The release body states plainly that only the macOS build has been run against real hardware, and carries the Gatekeeper workaround, since the builds are unsigned and macOS will claim the app is damaged. Needs contents: write - GITHUB_TOKEN is read-only by default.
workflow_dispatch, so a build can be started without pushing a commit or a tag - useful for re-running a release build after enabling Actions on a fresh fork.
Both jobs were failing on the current runner images. Windows: the generator was pinned to 'Visual Studio 17 2022', but windows-latest is now windows-2025-vs2026 and CMake reported it could not find any instance of Visual Studio. The pin is removed so CMake picks whichever Visual Studio is installed, which also survives the next image bump. -A x64 is kept. Linux: packages were installed without running apt-get update first, so apt asked for .deb versions the mirrors had already superseded and got a 404 on libpciaccess-dev. This was pre-existing - the original workflow never updated either - and affected both Linux jobs.
Pinning CMake 3.28 on windows-latest fails: the image is now windows-2025-vs2026, and 3.28 predates Visual Studio 2026, so it finds no usable generator and dies with 'CMAKE_C_COMPILER not set'. The image's bundled CMake always knows the Visual Studio the image ships. The other three jobs keep the pin - it does no harm there and the project needs CMake 3.22 or later.
Brings in the 53-commit measurement overhaul (PeriodDetector, least-squares period estimation, adaptive hysteresis trigger, per-note failure handling, JUCE 8.0.15) and keeps this fork's CV output, calibration and modern UI. Conflict resolutions: - VCOTuner.cpp: the rework added early returns to the audio callback above where CV output used to sit, which would have dropped the CV to 0 V between notes and after a sweep. CV filling is hoisted above those returns and now stands in for the blanket output clear, using the upstream per-channel null check instead of AudioBuffer::clear() (which memset through unopened channels). - Visualizer.cpp: kept this fork's ModernLookAndFeel bars and ported the new failed-note and out-of-range markers onto them, retargeted from absolute y=0/imageHeight to this fork's chartTop-based geometry so they land inside the chart rather than over the top info panel. Font::getStringWidth is gone in JUCE 8, so label widths use GlyphArrangement::getStringWidth. - MainComponent.cpp: failureLabel takes a fixed row at the bottom; the tabbed component (not the raw Visualizer, which now lives inside it) yields the height for it. - CI: dropped the upstream CI.yaml and folded its unit-test job, release-on-tag job, universal macOS binary and explicit -A x64 into this fork's build.yml, keeping one workflow. Verified: configures and builds clean against JUCE 8.0.15; 33/33 tests pass (1652 assertions). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both bugs predate the merge with TimoRozendal/VCOTuner -- the v0.3.0 build
shows them too -- but the JUCE 8 upgrade is a good moment to close them.
Inactive tabs were invisible. drawTabAreaBehindFrontButton paints into
TabbedButtonBar::BehindFrontTabComp, a full-size child sized to the whole bar
and placed toBehind(frontTab), so it sits in front of every tab except the
selected one. Filling it opaquely with the background colour erased them. The
Chart tab was still there and still clickable, just never drawn. Draw the
separator line only.
The deviation readout showed "A±--.--c". A bare "±" in a narrow literal reaches
JUCE as two Latin-1 bytes; build the glyph with CharPointer_UTF8 instead.
tools/debug_harness.sh runs the gates in order and is fail-closed:
static no conflict markers; no raw non-ASCII in narrow literals; no
JUCE-7 Font::getStringWidth; the tab overlay is not an opaque fill;
CV output is filled before the audio callback's early returns; the
JUCE submodule matches what the tree pins
build configure (pinning Apple clang, since homebrew's arm-none-eabi-gcc
otherwise wins CMake's compiler search and fails the ABI test)
tests the full ctest suite
ui launches the app, clicks through both tabs, captures each by
CGWindowID -- forcing frontmost loses to the terminal
tools/harness_selftest.py breaks each static guard in turn and asserts that
gate fails, so a green run means checked rather than unchecked. The static
gates and the selftest are wired into the CI unit-test job.
Full run: 11 gates passed, 0 failed, 0 skipped; 33/33 tests, 1652 assertions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A capture containing one click was failed outright. The reasoning was sound as
far as it went: folding the bad crossing into the fit yields a wrong frequency
carrying a plausible-looking uncertainty, which is worse than no reading. On a
440 Hz sine with a two-sample click, absorbing it gives 441.472 Hz -- off by
5.78 cents, with a small error bar. Failing is indeed better than that.
But that choice is only forced if the fit has to absorb it. The reason it did
is that crossings were numbered by position, one cycle each. A click inserts a
crossing, so every crossing after it is numbered one cycle ahead of the truth
and the line acquires a step -- which is why a single glitch moved the slope so
far. Numbering by counting cycles instead removes the coupling: each gap is
rounded to the nearest whole number of median periods, so the numbering
survives an inserted or missing crossing, and anything still off the grid is
dropped from the fit rather than absorbed by it.
The median is the scale used for that rounding because it survives a minority
of bad crossings; the mean is what an outlier drags towards itself.
Same signal, same click, after the change: 440.000000 Hz, 0.0000 cents, one
crossing rejected out of 401 -- and the rejection is reported, not hidden.
fitPeriod numbers crossings by cycle count; returns rejectedCrossings
and usedCrossings. A missed crossing is spanned (a whole
number of cycles, nothing to reject); a spurious one is
dropped. The anchor only advances to crossings that landed on
the grid, so a rejected crossing's time never becomes the
reference for the ones after it.
PeriodDetector the whole-set steadiness check counted any deviation as
disqualifying, so it failed the note before the fit ever ran.
It now counts how many periods sit off the median and permits
maxOutlierFraction (2%), floored at minOutliersAllowed (2),
since one dropout costs two periods. Compared against the
median rather than the mean, for the same reason as above.
countOutliers() reuses a scratch buffer reserved alongside
periods, so the audio thread still never allocates.
measurement_t carries rejectedCrossings; the chart marks a repaired column
with a small warning dot, so a note that needed repairing is
not shown as indistinguishable from a clean one.
The test that specified the old behaviour is rewritten rather than deleted: it
now asserts the frequency that comes out is correct to a thousandth of a
semitone and that the repair is reported. Sustained jitter is pinned separately
-- a phase-continuous sine swinging 400/480 Hz still fails, so the allowance
cannot be mistaken for tolerance of a genuinely unsteady pitch.
39 tests, 1675 assertions; harness 11/11.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nal one
A period in samples becomes a frequency in Hz by dividing by the sample rate,
and the rate used was the one the device reports: a nominal 48000, not what the
converter's crystal actually runs at. Interfaces are out by tens to hundreds of
ppm, and 100 ppm is 0.173 cents. Unlike jitter that error is systematic, so a
longer measurement does not reduce it -- and now that the period fit reports
uncertainties well under a cent on a clean note, it is the dominant error left
in any absolute reading.
It never affected live tuning. The pitch offsets are ratios against a reference
pitch measured through the same clock, so a common scale factor cancels exactly.
What it does affect is every absolute number: the frequency readout, the
error-in-Hz display, and the frequencies written into a report.
ClockCalibrator fits host time against sample count, one point per audio block,
and takes the reciprocal of the slope as the true rate. That is the same
estimator the period fit uses and for the same reason: the slope through many
points is far better conditioned than any single difference. The sums are
accumulated online, so the audio thread stores nothing per block, never
allocates, and the baseline can grow for as long as the device runs.
Deliberate limits:
no host timestamps no estimate, ever. JUCE passes the device's own
timestamp; where the host does not supply one the
alternative would be reading a clock on the audio thread
and calling the scheduling noise on it a measurement.
implausible result refused. A correction past a few thousand ppm means a
broken timestamp source or a device that changed rate,
and applying it would be far worse than the error it
claims to fix.
short baseline refused. Jitter averages down over the span, so the
baseline is what bounds precision, not the block count.
a dropout restarts the run. Missing samples put a step between two
good segments; fitting across it reads the gap as a
sustained rate error.
The estimate is computed on the audio thread and published through two atomics,
so the message thread reads a finished answer rather than the running sums.
Every conversion from period to frequency now goes through effectiveSampleRate(),
and a new harness gate fails if one of them reverts to the nominal rate. The
timeout calculation deliberately stays on the nominal rate: it is about buffer
timing, where a hundred ppm means nothing.
The chart qualifies the frequency it shows with the correction behind it
("440.00 Hz clk +15ppm"), on the one number the correction actually moves.
47 tests, 1696 assertions; harness 12/12.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng noise Five things, found by reading the code and the compiler's own output rather than by guessing at what might be wrong. 1. The CV output never drove a tuning sweep. setActive(true) appeared only in CalibrationEngine, and the sweep called trySendMidiNoteOn/Off and nothing else. So the fork carried a CV output and a calibration engine for it, and still required an external MIDI-to-CV interface to tune anything -- an interface which is itself a DAC with its own scaling error sitting in the measurement chain. VCOTuner now has a pitch source. With cvOutput selected the sweep sets a voltage instead of sending MIDI, which takes that second converter out of the chain and closes the loop: the same app sets the voltage and measures what came back. The two functions are renamed playPitch/releasePitch, because a name that says MIDI while sending CV is what misleads the next reader. Releasing a pitch is a no-op under CV: a pitch CV has no note-off, the voltage is the note. Holding it means the next note settles from a neighbouring voltage rather than from 0 V, and leaves the oscillator sounding while a trimmer is adjusted. A run is refused up front when CV is selected with no output channel, rather than failing note by note, and the unresponsive-interface message now covers both sources. 2. Calibration could push the output past the interface range. outputVoltage() clamped, then applied gain * v + offset, and did not clamp again -- so a corrected voltage near full scale left the range and voltageToSample() handed the interface a magnitude past +/-1 to clip silently, losing accuracy at exactly the extremes the calibration was measured to fix. 3. None of that arithmetic was tested. The pure conversions are now vcotuner::CvScaling, free of JUCE so the test binary can link them, with CVOutputManager delegating rather than keeping a second copy. Twelve tests cover the defining properties: an octave is one volt, pitch survives a round trip through voltage, the two standards are different shapes, the sample mapping hits the rails exactly, and correction cannot escape the range -- which is the test that would have caught 2. 4. The trigger level was latched for the whole measurement. A drifting DC offset then moves the waveform under a threshold that stays put, so the crossing happens at a progressively different phase each cycle: the crossing times acquire a ramp and the fitted period comes out biased, not merely noisy. Measured on a 110 Hz sine drifting at 0.2/s, the latched level is out by 0.65 cents; following it between cycles brings that to 0.021. At 50 Hz the latched level fails the note outright -- the waveform walks clear of the threshold and stops crossing it. The level is updated only at crossings, never within a cycle, so a period is measured against the same threshold at both of its ends, and slowly, for the reason it was latched in the first place: a threshold chasing noise would put that noise into the crossing times. On a clean signal that costs 3e-5 cents, four orders of magnitude below the bias it removes. 5. 162 warning sites in our own code, now zero. Font -> FontOptions (JUCE 8), drawText's float coordinates onto the Rectangle<float> overload instead of truncating into the int one, unused parameters named out, missing override markers, container indices as size_t in CalibrationTable's Gaussian elimination, and three dead fields plus one dead helper removed. Low value one at a time -- but this is the noise the float-equality bug in Visualizer's in-tune line check was hiding, which is fixed here too. Two more harness gates, each proven to fail when broken: the sweep must dispatch on the pitch source, and the CV fill must stay ahead of the audio callback's early returns. 61 tests, 2229 assertions; harness 13/13; zero warnings in Source/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A release build, a passing test suite and a green harness had all been taken as
evidence the branch was ready. Building it a different way found three things
none of them could see.
The debug build did not compile. MainComponent::measurementFailed uses its
`reason` argument only inside a jassert, which compiles away in release -- so
clang reported the parameter unused, the warning sweep commented the name out,
and release carried on building. Restored, with ignoreUnused() to say why the
name is kept.
A LookAndFeel was constructed during static initialisation. MainComponent held
`static ModernLookAndFeel modernLookAndFeel` at file scope, which JUCE asserts
on ("you're using a static LookAndFeel object"): it races JUCE's own static
Colours. The assertion only exists in a debug build, but the undefined
initialisation order is there in release too. It is now a function-local static,
constructed on first use after main() has started and destroyed after shutdown()
has dropped the components that refer to it.
The harness was not hermetic. This machine's shell profile exports an ARM cross
toolchain globally -- CC/CXX set to arm-none-eabi, and C_INCLUDE_PATH /
CPLUS_INCLUDE_PATH / LIBRARY_PATH pointing into arm-none-eabi-newlib. That is
what makes a plain `cmake -B build` fail here with "unrecognized command-line
option '-arch'", and the include paths put newlib headers ahead of the system
ones for every native compile started from that shell. The harness now clears
them, so a run means the same thing wherever it is started.
Also: the UI gate failed on a clean run and passed on a warm one. The bundle
lives on an external disk and is ad-hoc signed immediately before launching, so
the first launch after a full rebuild spends longer in Gatekeeper than the 20
second limit allowed -- the window was still on its way. The wait is now patient
and reports how long it took, because a gate that fails on timing teaches people
to ignore it.
New gate, proven to fail when broken: nothing on the audio path may allocate.
VCOTunerRealtimeCheck replaces global operator new and counts allocations across
12 detector runs of 4000 blocks each, covering reset(), processBlock(),
ClockCalibrator::addBlock() and the estimate() published from the callback. A
malloc there can block on a lock held by another thread, and the dropout it
causes surfaces as a failed note rather than as the memory bug it is. Currently
zero.
Verified: release and debug both build clean; 62 tests, 2229 assertions, also
green under ASan and UBSan; debug run exercises both tabs and a CV-driven sweep
with no assertion and no leak report; harness 14/14 from a clean tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI's unit-test job built the target VCOTunerTests by name, so the realtime allocation check added alongside it was never built there. ctest found the test registered and the executable missing, reported "Not Run", and failed the suite at 61 of 62 -- while the same commit was green locally, because the harness builds everything rather than naming a target. Naming the new executable in the workflow as well would fix this run and leave the next one to fail the same way. The test directory now exposes one aggregate target that depends on everything ctest will try to run, and CI builds that, so adding a test target is a change in one file rather than two. Verified by reproducing CI's steps locally -- fresh configure, build only that target, run ctest: 62 of 62. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The README described the CV output and the measurement changes as though they were finished. They build on three platforms and pass their tests, but no part of what this fork adds has driven a real oscillator, and a README that does not say so is the wrong place to find that out. A status section up front separates what is proven from what is not: the MIDI-driven tuner underneath is the original application and has years of use; the CV output, the CV calibration, the sample-rate correction, the dropout repair and the trigger-level tracking are all new here and none of them have been near an oscillator. Windows and Linux compile in CI and have never been run at all. It also records where to look first if CV tracking is wrong -- the 100 ms settling allowance, inherited from the MIDI-to-CV path -- so that is not rediscovered from scratch. Two things the README had fallen behind on while I was here: CV output was not in the feature list at all, and "How It Works" still described MIDI as the only way a pitch reaches the oscillator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Merges the 53-commit measurement rework from TimoRozendal/VCOTuner into this fork's CV work, then builds on it.
Both forks branch from TheSlowGrowth, so this is a sibling merge rather than an upstream pull: Timo rewrote the measurement core, this fork built the CV output, calibration and UI on top of the old one.
The merge
Seven conflicts. The one that mattered was
VCOTuner.cpp: the rework added early returns to the audio callback above where CV output used to sit, so taking either side as-is would have dropped the CV to 0 V between notes and after every sweep. CV filling is hoisted above those returns and now stands in for the blanket output clear, using the upstream per-channel null check rather thanAudioBuffer::clear(), which was memsetting through unopened channels.Visualizerkeeps this fork's styling with the new failed-note and out-of-range markers ported onto itschartTopgeometry — the upstream versions drew at absolutey=0, which here is the top info panel.Font::getStringWidthis gone in JUCE 8, so label widths useGlyphArrangement. One CI workflow rather than two: the upstreamCI.yamlwas dropped and its unit-test job, release-on-tag job, universal macOS binary and explicit-A x64folded intobuild.yml.Two pre-existing bugs
Both present in v0.3.0, confirmed by screenshotting that build.
Inactive tabs were invisible.
drawTabAreaBehindFrontButtonpaints into a full-size child that JUCE placestoBehind(frontTab)— in front of every other tab. Filling it opaquely erased them. The Chart tab was always there and always clickable, just never drawn.±--.--cin the deviation readout — a bare±in a narrow literal reaching JUCE as two Latin-1 bytes.Measurement
A dropout is repaired instead of failing the note. Crossings were numbered by position, one cycle each, so a click's extra crossing shifted the cycle number of everything after it and the fitted line acquired a step. On a 440 Hz sine with a two-sample click, absorbing it gives 441.472 Hz — 5.78 cents off, with a small error bar; upstream's choice to fail instead was correct for that fit. Numbering by counting cycles removes the coupling: same signal, same click, now 440.000000 Hz with one crossing rejected of 401, and the rejection is reported rather than hidden. This reverses a spec upstream deliberately wrote down and tested; that test is rewritten, not deleted, and sustained jitter is pinned separately so the new allowance cannot be mistaken for tolerating an unsteady pitch.
The converter's real sample rate is measured rather than trusting the nominal 48000. Interfaces are out by tens to hundreds of ppm and 100 ppm is 0.173 cents — systematic, so a longer measurement does not reduce it. Measured on the development machine: −5.8 ppm, converging over ~17 s. It never affected live tuning (pitch offsets are ratios through the same clock) but it did affect every absolute reading. Refused when there are no host timestamps, when the correction is implausible, when the baseline is short, and restarted across a dropout.
The trigger level follows the signal between cycles. Latched for the whole measurement, a drifting DC offset makes the crossing happen at a progressively different phase, so crossing times acquire a ramp and the period comes out biased. At 110 Hz drifting 0.2/s: 0.65 cents latched, 0.021 cents tracked. At 50 Hz the latched level fails the note outright — the waveform walks clear of the threshold. Updated only at crossings, never within one, and slowly; on a clean signal that costs 3e-5 cents.
CV
The CV output now drives a tuning sweep.
setActive(true)previously appeared only inCalibrationEngine, so this fork carried a CV output and a calibration engine for it and still required an external MIDI-to-CV interface to tune anything — an interface that is itself a DAC with its own scaling error in the measurement chain. A pitch source selector picks MIDI or CV; under CV the sweep sets a voltage and no MIDI is sent.Calibration could push the output past the interface range — clamped before the correction but not after, so
voltageToSample()handed the interface magnitudes past ±1 to clip silently, losing accuracy at exactly the extremes calibration was measured to fix.The scaling arithmetic is now tested. Extracted to a JUCE-free
vcotuner::CvScalingso the test binary can link it, withCVOutputManagerdelegating rather than holding a second copy.Housekeeping
162 warning sites in
Source/down to zero —FontOptions,drawTextonto theRectangle<float>overload instead of truncating,overridemarkers,size_tindices, dead fields and helpers removed. That noise was hiding a float-equality bug in the chart's in-tune line check, fixed here.A static
ModernLookAndFeelat file scope was racing JUCE's staticColours— JUCE asserts on it in debug; the undefined ordering was there in release too.Verification
tools/debug_harness.sh— 14 gates, fail-closed, covering static checks, configure, build, tests, a UI smoke test that drives both tabs, and a realtime check that replaces globaloperator newand counts allocations on the audio path.tools/harness_selftest.pybreaks each static guard in turn and asserts that gate fails, so green means checked rather than unchecked.Not verified: everything here was built on macOS arm64 only — this PR is how Windows and Linux get their first look, including the
-A x64change and the unit-test job, neither of which has ever run. And the CV sweep has not driven a real oscillator; the settling allowance is the existing 100 ms, tuned for a MIDI-to-CV interface, which is the first number to try if tracking looks off on hardware.🤖 Generated with Claude Code