feat: Network Analyzer instrument (non-invasive gain/NF sweep) - #65
Merged
Conversation
…n (12 with network_analyzer) Task 4 full-suite verification caught a third hardcoded registry-type-count assertion the plan's Task 3 brief did not enumerate (it only covered test_component_dispatch.cpp's two spots). test_component_authoring.cpp:25 independently asserts ComponentTypeRegistry::instance().all() against a literal 11-type list; updated to 12 with network_analyzer included.
Final whole-branch review (non-blocking Minor): the widget smoke test paired ImGui::CreateContext()/DestroyContext() as plain statements instead of the repo's established ImGuiFixture RAII pattern used elsewhere in this same file (AppFixture) and five other test files. An assertion failure mid-test would have skipped teardown via exception unwind, leaking the ImGui/ImPlot contexts. Also tidies app.h's m_serializer destruction-order comment, which still said 'both' after m_na_views became a third held reference.
…ing and private clone-chain measurement
…ct files
ProjectSerializer now saves the singleton NetworkAnalyzerEngine's four sweep
params plus Point A/B as {comp, port, is_output} pairs (same pin_map machinery
as probe_pins) and restores them on load, resolving pins through new_node_ids
so skipped components cannot shift the index mapping. Widget gains an
onParamChange callback (mirrors InspectorPanel) wired to markDirty in app.cpp,
so panel edits mark the project dirty. RfSimulatorApp exposes
testNetworkAnalyzerEngine() for round-trip tests.
test_network_analyzer.cpp fully rewritten for the v3 engine: test-local
INetworkAnalyzerHost/Scratch double (no app dependency), stimulus correctness
incl. absolute-power check via amplifier compression, gain/NF accuracy on real
generator->attenuator/amplifier chains, non-invasiveness against a real
second consumer, no-path/ambiguous/combiner all-NaN cases, mixer LO
translation via the copied parameter, RAII widget smoke test with and without
probe points. Registry tests reverted to 11 types (network_analyzer is no
longer a ComponentTypeRegistry row); project_file gains the NA block
round-trip.
…pin aliasing Task 3 review found: reset() (called by both newProject() and load()) never cleared m_na_engine's Point A/B, and load()'s restore only set them when present in the save file. Since pin ids are reallocated deterministically from the same base counters on every reset, a stale point id from a prior project could silently alias an unrelated pin in the next one, with no error or warning.
…sting find() Task 1 review flagged findByType as a byte-for-byte duplicate of the pre-existing find(). Single call site (NaScratch::createClone) switched to find() directly.
Final whole-branch review found: findUniquePath()'s adjacency discarded which output port a component used to reach the next node, and computeMeasurement() hardcoded outputs[0] everywhere a clone's Spectrum was read. Harmless for single-output components and Splitter (whose two outputs are identical -3dB copies), but silently wrong for any component with structurally different outputs (e.g. PFB Channelizer: OUT1 = active-channel narrowband, OUT2 = full-band) instead of the documented NaN fallback. findUniquePath() now returns a PathResult carrying the output port index used at each hop; computeMeasurement() wires clones and resolves Point B's own response using those real indices instead of index 0. Point B's port is resolved against the graph's raw output_pin_ids rather than IComponentEngine::outputPinId(port), since several multi-output engines (PFBChannelizerEngine) don't override that accessor. Investigated an end-to-end regression test via a real ADC->PFB chain; discovered a separate, deeper limitation (documented in the spec's Non-goals): any chain containing an ADC is currently unmeasurable via gainDb()/noiseFigureDb() regardless of port, because ADC unconditionally shifts every tone's frequency and computeMeasurement()'s tone matching compares by raw frequency value against the original stimulus grid. This makes the port-selection bug currently unobservable specifically through PFB (both ports read as all-NaN either way), so no regression test was added for that scenario; verified instead by code inspection and the full existing suite (263 assertions / 12 cases unchanged).
Final whole-branch review (Minor): path_count>=2 alone only bounds successful paths reaching Point B, not wasted exploration of dead-end branches that never reach it. Added a hard 100000-step cap; hitting it degrades to no-data (nullopt), consistent with every other ambiguous/unsupported topology this engine already handles.
striderZA
added a commit
that referenced
this pull request
Aug 10, 2026
Network Analyzer instrument (PR #65).
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
1 participant
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.
Summary
Adds a Network Analyzer instrument: an idealized 2-port network analyzer for measuring gain/noise-figure over frequency between any two points in the signal graph, non-invasively.
View > Network Analyzer), same pattern as Spectrum Analyzer — no canvas node, no wires.serialize()/deserialize()), feeds a synthetic tone-comb stimulus through only those clones, and reads gain/NF from the result. The real, live simulation is never touched..rfsimproject files.Non-goals (documented in
docs/superpowers/specs/2026-08-10-network-analyzer-design.md)Process
Spec → plan → 3 subagent implementation tasks (engine, app wiring, persistence+tests), each independently reviewed, full build+test after each. Final whole-branch review found and fixed two issues (see commits below), then a fix-confirmation review verified both.
Verification
ctest: 237/237 passing.test_network_analyzer: 263 assertions / 12 cases (stimulus correctness, gain/NF accuracy, non-invasiveness, no-path/ambiguous-path/Combiner-crossing → NaN, Mixer LO translation, widget smoke tests, persistence round-trip, stale-pin-on-reset regression).Disclosure on the two post-review fixes (commits
1faebf6,6e149fe):6e149fe(DFS step cap) is exercised indirectly by the existing suite continuing to pass and was manually traced for reachability; low risk (P3/minor, needs >100k DFS calls to matter at all).1faebf6(clone chain now reads the real output port used, instead of hardcoding port 0) is a real correctness fix with no dedicated regression test. The only multi-output components in the codebase are Splitter (both outputs are mathematically identical -3dB copies — can't distinguish the bug) and PFB Channelizer (OUT1/OUT2 are structurally different, which would distinguish it, but PFB requires a live ADC upstream, and the ADC non-goal above means any such chain reads all-NaN regardless of which port is used — making the bug currently unobservable black-box). Verified instead by two independent code-review passes (manual trace of index alignment + pin resolution logic) and the unchanged 263/12 suite. A white-box unit test exposingfindUniquePath()'s internalPathResultwas considered but not added, to avoid widening the public/test surface of the class for one fix; happy to add it if reviewers want stronger guarantees here.Commits
13 commits over
master. Seegit log master..feature/network-analyzerfor full history.