analyzer: fix trigger reliability (spurious triggers, re-runs, wide probes)

litescope/core.py

@@ -67,7 +67,11 @@ def __init__(self, data_width, depth=16):
         self.specials += MultiReg(done, self.done.status)
 
         # Memory and configuration.
-        mem = stream.AsyncFIFO([("mask", data_width), ("value", data_width)], depth)
+        # Use a buffered AsyncFIFO so Mask/Value come from registers: the unbuffered FIFO's
+        # output is the block RAM read port, whose clock-to-output delay would otherwise feed
+        # the data_width-wide comparator and the consume handshake within a single scope cycle
+        # and break timing as probes get wide.
+        mem = stream.AsyncFIFO([("mask", data_width), ("value", data_width)], depth, buffered=True)
         mem = ClockDomainsRenamer({"write": "sys", "read": "scope"})(mem)
         self.submodules += mem
         self.comb += [
@@ -77,20 +81,40 @@ def __init__(self, data_width, depth=16):
             self.mem_full.status.eq(~mem.sink.ready)
         ]
 
+        # Register Sink Data/Valid so the data_width-wide comparator sits between registers. The
+        # probe stream is free-running (the Mux always asserts valid), so sampling it
+        # unconditionally delays Data and Hit together by one cycle and preserves their
+        # alignment (the trigger position within the capture is unchanged).
+        sink_d = stream.Endpoint(core_layout(data_width))
+        self.sync.scope += [
+            sink_d.valid.eq(sink.valid),
+            sink_d.data.eq(sink.data),
+        ]
+        self.comb += sink.ready.eq(1)
+
         # Hit and memory read/flush.
-        hit   = Signal()
-        flush = WaitTimer(2*depth)
-        flush = ClockDomainsRenamer("scope")(flush)
-        self.submodules += flush
+        # Flush pending terms on the falling edge of enable and stop as soon as the memory is
+        # empty: a fixed-duration flush window (previous implementation) also ran from reset and
+        # kept draining after the memory was already empty, silently consuming terms pushed
+        # shortly after reset or re-arming - the capture then started with an empty term memory,
+        # i.e. an immediate spurious trigger (easily hit with fast CSR transports).
+        hit      = Signal()
+        flushing = Signal()
+        self.sync.scope += [
+            If(~enable & enable_d,
+                flushing.eq(1)
+            ).Elif(~mem.source.valid,
+                flushing.eq(0)
+            )
+        ]
         self.comb += [
-            flush.wait.eq(~(~enable & enable_d)), # flush when disabling
-            hit.eq((sink.data & mem.source.mask) == (mem.source.value & mem.source.mask)),
-            mem.source.ready.eq((enable & hit) | ~flush.done),
+            hit.eq((sink_d.data & mem.source.mask) == (mem.source.value & mem.source.mask)),
+            mem.source.ready.eq((enable & sink_d.valid & hit) | flushing),
         ]
 
         # Output.
         self.comb += [
-            sink.connect(source),
+            sink_d.connect(source, omit={"ready"}),
             # Done when all triggers have been consumed.
             done.eq(~mem.source.valid),
             source.hit.eq(done)
@@ -307,6 +331,13 @@ def __init__(self, data_width, depth):
         mem_flush = ClockDomainsRenamer("scope")(mem_flush)
         self.submodules += mem_flush
 
+        # Read-path flush: samples from a previous capture/drain can linger in the CDC FIFO and
+        # read converter; drain them during FLUSH (no new data enters the read path in that
+        # state) so the upload starts with the new capture's first sample.
+        flush_read     = Signal()
+        flush_read_sys = Signal()
+        self.specials += MultiReg(flush_read, flush_read_sys)
+
         # FSM.
         fsm = FSM(reset_state="IDLE")
         fsm = ClockDomainsRenamer("scope")(fsm)
@@ -323,6 +354,7 @@ def __init__(self, data_width, depth):
             sink.ready.eq(1),
             mem_flush.wait.eq(1),
             mem.source.ready.eq(1),
+            flush_read.eq(1),
             If(mem_flush.done,
                 NextState("WAIT")
             )
@@ -357,7 +389,7 @@ def __init__(self, data_width, depth):
             self.comb += cdc.source.connect(read_source)
 
         self.comb += [
-            read_source.ready.eq(self.mem_data.rd_stb | ~self.enable.storage),
+            read_source.ready.eq(self.mem_data.rd_stb | ~self.enable.storage | flush_read_sys),
             self.mem_data.status.eq(read_source.data)
         ]
 

litescope/software/driver/analyzer.py

@@ -71,6 +71,10 @@ def __init__(self, regs, name, config_csv=None, debug=False):
         self.offset = 0
         self.length = None
 
+        # Configured trigger terms (mask, value); loaded into the gateware on each run() since
+        # the trigger memory consumes its terms on every capture.
+        self.trigger_terms = []
+
         # Disable trigger and storage
         self.trigger_enable.write(0)
         self.storage_enable.write(0)
@@ -111,24 +115,45 @@ def build(self):
             if self.name == key[:len(self.name)]:
                 key = key.replace(self.name + "_", "")
                 setattr(self, key, value)
-        for signals in self.layouts.values():
+        # Group-aware signal offsets/masks: triggers must resolve a signal's position in the
+        # currently selected group (a signal present in several groups can sit at different
+        # positions in each).
+        self.signal_offsets = {}
+        self.signal_masks   = {}
+        for group, signals in self.layouts.items():
             value = 1
             for name, length in signals:
-                setattr(self, name + "_o", value)
+                self.signal_offsets[(group, name)] = value
                 value = value*(2**length)
-        for signals in self.layouts.values():
             value = 0
             for name, length in signals:
-                setattr(self, name + "_m", (2**length-1) << value)
+                self.signal_masks[(group, name)] = (2**length-1) << value
                 value += length
+        # Keep the flat <name>_o/<name>_m attributes for compatibility; warn when a signal is
+        # ambiguous across groups (the flat attribute then reflects the last group only).
+        ambiguous = set()
+        for (group, name), offset in self.signal_offsets.items():
+            mask = self.signal_masks[(group, name)]
+            if hasattr(self, name + "_o") and \
+               ((getattr(self, name + "_o") != offset) or (getattr(self, name + "_m") != mask)):
+                ambiguous.add(name)
+            setattr(self, name + "_o", offset)
+            setattr(self, name + "_m", mask)
+        for name in sorted(ambiguous):
+            print(f"{self.name}: warning: signal '{name}' is present in several groups at "
+                   "different positions; triggers resolve it in the selected group.")
+
+    def _signal_offset(self, name):
+        return self.signal_offsets.get((self.group, name), getattr(self, name + "_o"))
+
+    def _signal_mask(self, name):
+        return self.signal_masks.get((self.group, name), getattr(self, name + "_m"))
 
     def configure_group(self, value):
         self.group = value
         self.mux_value.write(value)
 
     def add_trigger(self, value=0, mask=0, cond=None):
-        if self.trigger_mem_full.read():
-            raise ValueError("Trigger memory full, too much conditions")
         if cond is not None:
             for k, v in cond.items():
                 # Check for binary/hexa expressions
@@ -145,23 +170,41 @@ def add_trigger(self, value=0, mask=0, cond=None):
                         if c != "x":
                             v |= int(c, 16 if mx is not None else 2 )
                             m |= 0xf if mx is not None else 0b1
-                    value |= getattr(self, k + "_o")*v
-                    mask  |= getattr(self, k + "_m") & (getattr(self, k + "_o")*m)
+                    value |= self._signal_offset(k)*v
+                    mask  |= self._signal_mask(k) & (self._signal_offset(k)*m)
                 # Else convert to int
                 else:
-                    value |= getattr(self, k + "_o")*int(v, 0)
-                    mask  |= getattr(self, k + "_m")
-        self.trigger_mem_mask.write(mask)
-        self.trigger_mem_value.write(value)
-        self.trigger_mem_write.write(1)
+                    value |= self._signal_offset(k)*int(v, 0)
+                    mask  |= self._signal_mask(k)
+        self.trigger_terms.append((mask, value))
 
     def add_rising_edge_trigger(self, name):
-        self.add_trigger(getattr(self, name + "_o")*0, getattr(self, name + "_m"))
-        self.add_trigger(getattr(self, name + "_o")*1, getattr(self, name + "_m"))
+        self.add_trigger(self._signal_offset(name)*0, self._signal_mask(name))
+        self.add_trigger(self._signal_offset(name)*1, self._signal_mask(name))
 
     def add_falling_edge_trigger(self, name):
-        self.add_trigger(getattr(self, name + "_o")*1, getattr(self, name + "_m"))
-        self.add_trigger(getattr(self, name + "_o")*0, getattr(self, name + "_m"))
+        self.add_trigger(self._signal_offset(name)*1, self._signal_mask(name))
+        self.add_trigger(self._signal_offset(name)*0, self._signal_mask(name))
+
+    def _load_trigger_terms(self, timeout=1.0):
+        # Disarm the trigger; the gateware flushes any leftover terms (previous capture,
+        # aborted sequence) on the falling edge of enable.
+        self.trigger_enable.write(0)
+        # The flush window is 2*trigger_depth scope cycles; any CSR access takes far longer,
+        # but poll done (trigger memory empty) so the reload below cannot race the flush.
+        deadline = time.time() + timeout
+        for _ in range(2):
+            while not self.trigger_done.read():
+                if time.time() > deadline:
+                    raise TimeoutError("Trigger memory flush timeout")
+        # (Re-)load the configured terms so a capture can be re-run without reconfiguring:
+        # the trigger memory consumes its terms on every capture.
+        for mask, value in self.trigger_terms:
+            if self.trigger_mem_full.read():
+                raise ValueError("Trigger memory full, too much conditions")
+            self.trigger_mem_mask.write(mask)
+            self.trigger_mem_value.write(value)
+            self.trigger_mem_write.write(1)
 
     def configure_trigger(self, value=0, mask=0, cond=None):
         self.add_trigger(value, mask, cond)
@@ -193,8 +236,16 @@ def run(self, offset=0, length=None):
         self.length = length
         if self.debug:
             self._log(f"run (offset={offset}, length={length})")
+        # Disarm the trigger and reload its terms first: a previous capture leaves the trigger
+        # armed with an empty (fully consumed) term memory, whose hit output is a constant
+        # level that would otherwise fire the re-armed storage immediately. With the terms
+        # reloaded, hit stays low until a real match.
+        self._load_trigger_terms()
         self.storage_offset.write(offset)
         self.storage_length.write(length)
+        # Storage arms on the rising edge of enable: clear it first so run() also re-arms
+        # after a previous capture on the same driver instance.
+        self.storage_enable.write(0)
         self.storage_enable.write(1)
         self.trigger_enable.write(1)
 
@@ -203,6 +254,7 @@ def clear(self):
         self.offset = 0
         self.length = None
         self.rle_enabled = False
+        self.trigger_terms = []
         self.trigger_enable.write(0)
         self.storage_enable.write(0)
         if hasattr(self, "rle_enable"):
@@ -321,11 +373,12 @@ def get_instant_value(self, group, name):
         self.data = DumpData(self.data_width)
         self.debug = False
         self.configure_group(group)
+        self.trigger_terms = []
         self.configure_trigger()
         self.configure_subsampler(1)
         self.run(0, 1)
         self.wait_done()
         self.upload()
-        min_idx = log2_int(getattr(self, name + "_o"))
-        max_idx = min_idx + log2_int((getattr(self, name + "_m") >> min_idx) + 1)
+        min_idx = log2_int(self._signal_offset(name))
+        max_idx = min_idx + log2_int((self._signal_mask(name) >> min_idx) + 1)
         return self.data[min_idx:max_idx][0]