analyzer: fix trigger reliability (spurious triggers, re-runs, wide probes) - #53
Merged
Conversation
Two reliability fixes in the trigger: - Term flushing used a fixed 2*depth-cycle window that also ran from reset and kept draining after the memory was already empty. Terms pushed within that window - right after reset, or right after disarming to re-arm - were silently consumed, so the capture armed with an empty term memory and triggered immediately on anything. Slow interactive/UART hosts never noticed; fast CSR transports (PCIe, Ethernet) hit this easily, which shows up as captures that 'do not trigger correctly'. Flush now starts on the falling edge of enable and stops as soon as the memory is empty. - The comparator was a fully-combinational data_width-wide compare fed on one side by the unbuffered AsyncFIFO's block-RAM read port and on the other by the raw probe mux, feeding back into the consume handshake within a single scope cycle - a timing hazard that grows with probe width. The term memory is now a buffered FIFO and the probe data/valid are registered before the compare, so the wide comparator sits between registers. The registered input delays data and hit together by one scope cycle: captures shift by one cycle in absolute time, the trigger position within the capture is unchanged. The first analyzer test relied on the reset-window flush eating its term (it never enabled the trigger): it now arms the trigger properly on a reachable value and additionally checks the trigger sample's position; the group-mux test now observes the match exactly at index 0 with offset=0.
Samples from a previous capture or idle drain can linger in the CDC FIFO and read-width converter between the storage FIFO and the mem_data CSR; the next upload then starts with stale words and every following sample is shifted. Drain the read path during the storage FLUSH state (no new data enters it there), so an upload always starts with the new capture's first sample.
Three driver fixes so a capture can be re-run on the same driver instance: - The gateware's trigger memory consumes its terms on every capture, so re-running armed with an empty memory and triggered immediately on anything. add_trigger and the edge helpers now record the configured terms and run() disarms (letting the gateware flush leftovers), reloads all terms and rearms. As a consequence add_trigger no longer writes the hardware directly; the memory-full check happens at load time. - Storage arms on the rising edge of its enable, but run() only ever wrote 1: a second run() on the same instance never re-armed storage and the previous capture's state was re-read as a new one. run() now clears and rises enable, after reloading the trigger terms: an armed trigger with a consumed term memory outputs a constant hit level that would otherwise fire the re-armed storage immediately. - Signal offsets/masks for trigger conditions were flat attributes overwritten by each group, so a signal present in several groups always resolved to the last group's position regardless of the selected group. Conditions now resolve in the currently selected group; the flat attributes remain for compatibility and an ambiguity warning is printed when they differ across groups.
At data_width > 32 the trigger Mask/Value CSRs are compound (several bus words), but all existing tests poked CSRStorage.write() which sets the whole storage atomically - neither the word-by-word bus path that hosts actually perform nor a comparator wider than 32 bits was ever exercised. Drive a 128-bit-probe analyzer exclusively through a csr_bus.CSRBank, mirroring the host access pattern (MSB word first, ascending addresses, separate arm strobe): check the capture is a consecutive wide pattern, the trigger sample sits at its locked position within the capture, and a second re-armed capture behaves identically. Note: csr_bus.Interface.read samples dat_r one cycle too early for the bank's registered read path; the test uses a local read helper with the settle cycle.
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
Investigation of long-standing 'captures do not trigger correctly' reports, reproduced in simulation and on hardware. Four commits:
analyzer: fix trigger term flushing and register the comparator operands
2*depth-cycle window that also ran from reset and kept draining after the term memory was already empty. Terms pushed within that window — right after reset, or right after disarming to re-arm — were silently consumed, so the capture armed with an empty term memory and triggered immediately on anything. Slow interactive/UART hosts rarely notice; fast CSR transports (PCIe/Ethernet) hit it easily. Flush now starts on the falling edge of enable and stops as soon as the memory is empty. (Reproduced in sim: a term pushed ~20 cycles after reset was eaten and the capture triggered instantly.)data_width-wide compare fed by the unbuffered AsyncFIFO's block-RAM read port on one side and the raw probe mux on the other, feeding back into the consume handshake in a single scope cycle — a timing hazard that grows with probe width. The term memory is now a buffered FIFO and probe data/valid are registered, so the wide compare sits between registers. Captures shift by one scope cycle in absolute time; the trigger position within the capture is unchanged (locked by test).analyzer: drain stale read-path samples during storage flush
Samples from a previous capture lingered in the CDC FIFO/read converter and shifted the next upload's head.
driver: fix re-running captures, resolve offsets per group
run()on a used driver instance armed with an empty (consumed) term memory and never re-armed storage (rising-edge enable). Terms are now recorded and reloaded on everyrun(), storage enable is cycled, and ordering guarantees no constant-hit level fires the re-armed storage. Trigger conditions also now resolve signal offsets in the currently selected group instead of always the last group.test: exercise wide triggers through the CSR bus
The >32-bit trigger path had zero coverage: sims poked
CSRStorage.write()atomically, bypassing the multi-word bus path hosts actually use. New test drives a 128-bit-probe analyzer exclusively through acsr_bus.CSRBank.Validation