Fixed triggering mode change when acquiring

src/ui.cpp

@@ -99,12 +99,26 @@ struct App {
   /// Trigger-mode state for the status line: how many latches were on screen last
   /// frame, and whether an event is held (which is what "frozen" means).
   std::uint64_t seen_latches = 0;
+  /// Which snapshot was framed last. Separate from `seen_latches` because a Disarm during
+  /// a capture publishes a snapshot without an event having fired.
+  std::uint64_t seen_event_seq = 0;
   bool event_held = false;
-  /// Monotonic time of the latch `seen_latches` counts, so the status line can age it.
+  /// Whether a capture is open on screen: the edge has been accepted and the post window
+  /// is filling in view, on the coming event's own clock. It is what keeps
+  /// `clamp_to_data()` off the pinned axis and tells the status line apart from "armed".
+  bool capturing = false;
+  /// The edge of the capture `capturing` refers to, so a second edge accepted before the
+  /// first capture closed re-frames instead of being mistaken for the same one.
+  double capture_edge = 0.0;
+  /// Monotonic time of the last event, and the edge that started the clock. The clock
+  /// starts when the *edge* is accepted, not when the capture closes -- that is the
+  /// instant the user saw -- and the edge is kept so the latch that ends a live-updated
+  /// capture does not restart it.
   /// Wall time, not stream time: "how long since the last event" is a question about the
   /// user's wait, and a stream whose clock runs at any other speed would answer it wrong.
-  /// 0 until the first latch of the session.
+  /// 0 until the first event of the session.
   gint64 last_latch_us = 0;
+  double timed_edge = 0.0;
   double window_choices_limit = -1.0;  ///< the cap the dropdown was last built with
   /// The rate each channel actually *stored* over the last second, and the declared rate
   /// beside it. The app does no decimation, so the stored rate is the datasource's own
@@ -133,6 +147,10 @@ struct App {
   int dbg_reset_visible = -1;
   std::vector<std::uint64_t> last_stored;
   std::uint64_t dbg_latches_at_disarm = 0;
+  std::uint64_t dbg_seq_at_disarm = 0;
+  bool dbg_capturing_at_disarm = false;
+  int dbg_disarm_polls = 0;
+  int dbg_disarm_in_capture = 0;
   int dbg_swaps = 0;
   int dbg_axes_w_min = 1 << 20;
   int dbg_axes_w_max = -(1 << 20);
@@ -448,6 +466,7 @@ void on_arm_clicked(GtkButton* button, gpointer user) {
   }
   if (armed) {
     app->event_held = false;
+    app->capturing = false;
     app->model->trigger_time_valid().store(false, std::memory_order_relaxed);
   }
   // Arming is a statement that this signal should be watched, so its acquisition
@@ -457,8 +476,7 @@ void on_arm_clicked(GtkButton* button, gpointer user) {
   gtk_button_set_label(GTK_BUTTON(app->arm_button), armed ? "Disarm" : "Arm");
 }
 
-void on_trigger_field_changed(GtkWidget*, gpointer user) {
-  App* app = static_cast<App*>(user);
+void publish_trigger_fields(App* app) {
   if (app->updating_panels) return;
   // Read the widgets first (they may be touched again by a re-entrant handler if a
   // setter ran while the lock was held), then publish the values under the lock the
@@ -481,6 +499,18 @@ void on_trigger_field_changed(GtkWidget*, gpointer user) {
   }
 }
 
+void on_trigger_field_changed(GtkWidget*, gpointer user) {
+  publish_trigger_fields(static_cast<App*>(user));
+}
+
+/// The same publish, for the two dropdowns. It needs its own entry point because
+/// `notify::<prop>` passes a `GParamSpec*` ahead of the user data while
+/// `GtkSpinButton::value-changed` does not: a two-parameter callback on a `notify::`
+/// signal reads the pspec as its `App*` and crashes on the first field it touches.
+void on_trigger_drop_changed(GObject*, GParamSpec*, gpointer user) {
+  publish_trigger_fields(static_cast<App*>(user));
+}
+
 void on_trigger_signal_changed(GObject* obj, GParamSpec*, gpointer user) {
   App* app = static_cast<App*>(user);
   if (app->updating_panels) return;
@@ -897,6 +927,7 @@ void on_trigger_toggled(GtkToggleButton* button, gpointer user) {
     signal = tc.channel;
   }
   app->event_held = false;
+  app->capturing = false;
   if (!on) {
     app->model->trigger_time_valid().store(false, std::memory_order_relaxed);
     app->model->set_display_epoch(0.0);  // back to seconds since the stream started
@@ -1317,12 +1348,30 @@ gboolean tick(gpointer user) {
   }
   if (model.generation() != app->signal_generation) rebuild_signal_panels(*app);
 
+  // What the display shows, decided once for the whole frame: the producer can open or
+  // close a capture between any two reads of the model, and the bind, the epoch, the pin
+  // and the status line all have to describe the same picture.
+  const DisplayMode display = model.display_mode();
+  const double cap_edge = model.capture_edge();
+  const bool new_capture =
+      display == DisplayMode::Capturing && (!app->capturing || cap_edge != app->capture_edge);
+  app->capturing = display == DisplayMode::Capturing;
+  if (new_capture) app->capture_edge = cap_edge;
+
   {
     // Serialise against the acquisition thread's CONFIG handling and against the
     // UI's own structural changes: the bind takes shared handles into the live
     // store, and acquire() reads it.
     std::lock_guard<std::mutex> lock(model.mutex());
-    app->rows->bind(model);
+    // Mid-capture the live ring is drawn on the coming event's clock, so its edge sits at
+    // x = 0 exactly as a snapshot's does and the pinned [-pre, +post] axis means the same
+    // thing in both. Every other mode reads stream seconds. The epoch is a consumer-side
+    // offset on the live channels, so it has to be in place before the bind that hands
+    // them to the traces -- and it is written here, under the lock, because a rebuild can
+    // replace the store.
+    const double want_epoch = display == DisplayMode::Capturing ? -cap_edge : 0.0;
+    if (model.display_epoch() != want_epoch) model.set_display_epoch(want_epoch);
+    app->rows->bind(model, display);
     app->rows->acquire(model);
   }
 
@@ -1331,11 +1380,12 @@ gboolean tick(gpointer user) {
   // the group and pushes it to the others, so the "which row acted" search this replaced
   // (and the per-row pan/zoom propagation behind it) has no subject any more.
 
-  // While a captured event is on screen the window is pinned to [edge-pre,
-  // edge+post]; clamp_to_data would fight that pin whenever the bound series
-  // still reports the acquisition buffer's raw coordinates, dragging the view
-  // along the ring. Clamping is a live-view concern only.
-  if (!app->event_held) app->rows->clamp_to_data();
+  // While an event is on screen -- held, or still being captured -- the window is pinned
+  // to [-pre, +post]; clamp_to_data would fight that pin whenever the bound series still
+  // reports the acquisition buffer's raw coordinates, dragging the view along the ring.
+  // Clamping is a live-view concern only, and a capture is live data on a pinned axis,
+  // which is exactly the case it must not touch.
+  if (!app->event_held && !app->capturing) app->rows->clamp_to_data();
 
   // Curator: the trigger's arm state can change under us (single mode disarms
   // itself), so the button and the status line read the published atomics.
@@ -1345,48 +1395,88 @@ gboolean tick(gpointer user) {
   }
   const bool armed = model.trigger_armed().load(std::memory_order_relaxed);
   gtk_button_set_label(GTK_BUTTON(app->arm_button), armed ? "Disarm" : "Arm");
+  if (new_capture) {
+    // The edge has been accepted, which means the pre window was already full, so the
+    // whole event window can be framed now and the post side filled in view.
+    //
+    // The split is the *requested* one -- the same `post_span_s()` the producer will cut
+    // with -- because there is no snapshot to read it off yet, and the marker has to sit
+    // where that cut will put it rather than where a different rounding would.
+    double span = 0.0;
+    double post = 0.0;
+    {
+      std::lock_guard<std::mutex> lock(model.mutex());
+      span = std::max(1.0e-3, model.event_span());
+      post = model.trigger().post_span_s(span);
+    }
+    app->event_held = false;
+    app->rows->pin_event_window(span - post, post);
+    app->rows->set_trigger_marker(0.0, true);
+    select_window_choice(*app, span);
+    // The user saw the event at its edge, so that is when the counter starts.
+    app->timed_edge = cap_edge;
+    app->last_latch_us = g_get_monotonic_time();
+  }
+
   const std::uint64_t latches = model.trigger_latches().load(std::memory_order_relaxed);
   if (latches != app->seen_latches) {
     app->seen_latches = latches;
-    app->event_held = true;
-    app->last_latch_us = g_get_monotonic_time();
-    // The window is pinned to exactly what the snapshot captured, read off the
-    // snapshot itself. Not off `event_span`/`pretrigger_pct`: those say what was
-    // *asked for*, in stream seconds, while how much the capture actually covers
-    // depends on the cadence the trigger channel was running at. Pinning to the
-    // request collapsed the waveform into a sliver at x = 0 and left the rest of
-    // the axis -- the whole post side included -- blank.
-    //
-    // The snapshot is edge-relative (the edge at x = 0), so the axis needs no epoch
-    // shift: snapshot, pin and marker all speak the same coordinates. Rebind first
-    // so the slots hold the snapshot before the pin is applied.
-    {
-      std::lock_guard<std::mutex> lock(model.mutex());
-      app->rows->bind(model);
+    const double edge = model.trigger_time().load(std::memory_order_relaxed);
+    // A capture the UI watched fill already started this clock at its edge; only one that
+    // opened and closed inside a single frame -- never reported pending, so never
+    // displayed as a capture -- starts it here.
+    if (edge != app->timed_edge) {
+      app->timed_edge = edge;
+      app->last_latch_us = g_get_monotonic_time();
     }
-    double pre = 0.0;
-    double post = 0.0;
-    if (auto snap = model.event()) {
-      pre = snap->pre_s;
-      post = snap->post_s;
-    }
-    if (!(pre + post > 0.0)) {
-      // Nothing was captured (no samples in the ring yet): fall back to the
-      // requested window so the axis is at least readable.
-      const double span = std::max(1.0e-3, model.event_span());
-      pre = std::min(0.99, model.trigger().pretrigger_pct / 100.0) * span;
-      post = span - pre;
-    }
-    app->rows->pin_event_window(pre, post);
-    app->rows->set_trigger_marker(0.0, true);  // the edge sits at x = 0
-    select_window_choice(*app, pre + post);
   }
-  // The trigger instant is marked on every row while its event is the thing on
-  // screen, so the pre/post split around it can be read off the plot.
-  if (trigger_on && model.trigger_time_valid().load(std::memory_order_relaxed)) {
-    // x is seconds from the event: the display epoch is minus the edge time, so the
-    // edge sits at exactly x = 0 wherever the user has zoomed.
-  } else {
+
+  // Framing the held event is keyed on *which snapshot was published*, not on the latch
+  // count: a Disarm during a capture publishes what the capture got to without an event
+  // having fired, and that partial has to be framed and held exactly like one that ran
+  // to term. The latch count stays the answer to "how many events fired", which is what
+  // the probes assert on.
+  const std::uint64_t seq = model.event_seq();
+  if (seq != app->seen_event_seq) {
+    app->seen_event_seq = seq;
+    // A publication and the *next* edge can land in the same frame. The display follows
+    // the newer of the two, so re-framing on the snapshot is only right when the snapshot
+    // is what is bound; otherwise the capture above has already framed its own window.
+    if (display == DisplayMode::Held) {
+      app->event_held = true;
+      // The window is pinned to exactly what the snapshot captured, read off the
+      // snapshot itself. Not off `event_span`/`pretrigger_pct`: those say what was
+      // *asked for*, in stream seconds, while how much the capture actually covers
+      // depends on the cadence the trigger channel was running at. Pinning to the
+      // request collapsed the waveform into a sliver at x = 0 and left the rest of
+      // the axis -- the whole post side included -- blank.
+      //
+      // The snapshot is edge-relative (the edge at x = 0) and the epoch is back to 0, so
+      // the bind at the top of this frame already put the slots and the axis on the same
+      // coordinates the pin is about to state.
+      double pre = 0.0;
+      double post = 0.0;
+      if (auto snap = model.event()) {
+        pre = snap->pre_s;
+        post = snap->post_s;
+      }
+      if (!(pre + post > 0.0)) {
+        // Nothing was captured (no samples in the ring yet): fall back to the
+        // requested window so the axis is at least readable.
+        const double span = std::max(1.0e-3, model.event_span());
+        post = model.trigger().post_span_s(span);
+        pre = span - post;
+      }
+      app->rows->pin_event_window(pre, post);
+      app->rows->set_trigger_marker(0.0, true);  // the edge sits at x = 0
+      select_window_choice(*app, pre + post);
+    }
+  }
+  // The trigger instant is marked on every row while its event is the thing on screen --
+  // being captured or held. x is seconds from the event in both: the display epoch is
+  // minus the edge while the live ring is on a pinned axis, and the snapshot's own stamps
+  // are edge-relative, so the edge sits at exactly x = 0 wherever the user has zoomed.
+  if (!app->capturing && !(trigger_on && model.trigger_time_valid().load(std::memory_order_relaxed))) {
     app->rows->set_trigger_marker(0.0, false);
   }
 
@@ -1568,7 +1658,9 @@ gboolean tick(gpointer user) {
   StreamStats& st = model.stats();
   const char* mode = "live";
   if (trigger_on) {
-    if (app->event_held) {
+    if (app->capturing) {
+      mode = "trigger: CAPTURING (post-trigger window filling)";
+    } else if (app->event_held) {
       mode = "trigger: EVENT HELD (Arm to re-arm, or 'Go live' for streaming)";
     } else if (armed && have_trigger_value) {
       static char waiting[192];
@@ -1724,9 +1816,9 @@ GtkWidget* build_trigger_panel(App& app) {
     g_signal_connect(w, "value-changed", G_CALLBACK(on_trigger_field_changed), &app);
   }
   g_signal_connect(app.trigger_edge_drop, "notify::selected",
-                   G_CALLBACK(on_trigger_field_changed), &app);
+                   G_CALLBACK(on_trigger_drop_changed), &app);
   g_signal_connect(app.trigger_mode_drop, "notify::selected",
-                   G_CALLBACK(on_trigger_field_changed), &app);
+                   G_CALLBACK(on_trigger_drop_changed), &app);
   return box;
 }
 
@@ -2068,8 +2160,11 @@ gboolean trig_trace_probe(gpointer user) {
     for (int s = 0; s < RowSet::kSlotsPerRow; ++s) {
       const std::size_t si = static_cast<std::size_t>(s);
       if (r.channel[si] < 0) continue;
+      // A live binding in trigger mode is only legitimate as a capture in progress, so
+      // the two are named apart: `live` with `trigger=on` is the defect, `live(capturing)`
+      // is the feature.
       if (r.slots[si].bound_event() != nullptr) display = "event(frozen)";
-      else if (r.slots[si].bound() != nullptr) display = "live";
+      else if (r.slots[si].bound() != nullptr) display = a->capturing ? "live(capturing)" : "live";
       shown = &r.slots[si];
       found = true;
       break;
@@ -2160,10 +2255,11 @@ gboolean trig_trace_probe(gpointer user) {
                bound_hi, dx_nominal, dx_real,
                dx_real > 0.0 ? dx_nominal / dx_real : 0.0, got_lo, got_hi, want_lo, want_hi);
   std::fprintf(stderr,
-               "[trig-probe] trigger=%s pending=%d display=%s latches=%llu armed=%d "
+               "[trig-probe] trigger=%s pending=%d display=%s epoch=%.4f latches=%llu armed=%d "
                "live_pub=%llu snap=[%.4f..%.4f] n=%zu axis=[%.4f..%.4f] "
                "data=[%.4f..%.4f] drawn=[%.4f..%.4f]\n",
                m.trigger_enabled() ? "on" : "off", int(m.trigger_pending()), display,
+               m.display_epoch(),
                static_cast<unsigned long long>(m.trigger_latches().load()),
                int(m.trigger_armed().load()),
                static_cast<unsigned long long>(m.live().values(0).produced()), snap_lo, snap_hi,
@@ -2185,6 +2281,28 @@ gboolean arm_probe(gpointer user) {
   return G_SOURCE_REMOVE;
 }
 
+/// Headless self-check: changes the trigger panel's two dropdowns, which is what a user
+/// does to switch Normal/Single or the edge -- mid-capture, if the probe is timed into
+/// one. Reaching the dropdowns directly is the point: their `notify::selected` handler
+/// has a different signature from the spin buttons' `value-changed`, and connecting the
+/// wrong one crashed on the first field read with no way to tell from the state dump.
+/// A line here with no SIGSEGV after it is the assertion.
+gboolean fields_probe(gpointer user) {
+  App* a = static_cast<App*>(user);
+  if (a->window == nullptr) return G_SOURCE_REMOVE;
+  GtkDropDown* mode = GTK_DROP_DOWN(a->trigger_mode_drop);
+  GtkDropDown* edge = GTK_DROP_DOWN(a->trigger_edge_drop);
+  gtk_drop_down_set_selected(mode, gtk_drop_down_get_selected(mode) == 0 ? 1 : 0);
+  gtk_drop_down_set_selected(edge, (gtk_drop_down_get_selected(edge) + 1) % 3);
+  std::lock_guard<std::mutex> lock(a->model->mutex());
+  std::fprintf(stderr, "[fields-probe] mode=%u edge=%u published mode=%d edge=%d capturing=%d\n",
+               gtk_drop_down_get_selected(mode), gtk_drop_down_get_selected(edge),
+               int(a->model->trigger().mode), int(a->model->trigger().edge),
+               int(a->model->trigger_pending()));
+  std::fflush(stderr);
+  return G_SOURCE_REMOVE;
+}
+
 /// Headless self-check: acquire every signal the CONFIG described, the way the
 /// signal panel's switches would, and put each resulting channel on a plot, the way
 /// a drag from the panel onto a row would. Without the second half nothing is bound
@@ -2214,6 +2332,8 @@ gboolean enable_all_probe(gpointer user) {
   return G_SOURCE_REMOVE;
 }
 
+gboolean disarm_probe_step3(gpointer user);
+
 gboolean disarm_probe_step2(gpointer user) {
   App* a = static_cast<App*>(user);
   if (a->window == nullptr) return G_SOURCE_REMOVE;
@@ -2226,15 +2346,70 @@ gboolean disarm_probe_step2(gpointer user) {
   a->dbg_latches_at_disarm = a->model->trigger_latches().load();
   std::fprintf(stderr, "[disarm-probe] %llu events so far, armed now=%d\n",
                (unsigned long long)a->dbg_latches_at_disarm, int(a->model->trigger().armed));
+  a->dbg_capturing_at_disarm = a->model->trigger_pending();
+  a->dbg_seq_at_disarm = a->model->event_seq();
+  g_timeout_add(600, disarm_probe_step3, a);
   return G_SOURCE_REMOVE;
 }
 
+/// Headless self-check, the half that reads the *outcome* of the Disarm. A disarm that
+/// landed mid-capture (`was_capturing=1`) must leave an event on screen with the
+/// capture's own span: `display` reading `nothing` there is the "the event completely
+/// disappeared" defect, and `seq` must have advanced by one while `events` has not.
+gboolean disarm_probe_step3(gpointer user) {
+  App* a = static_cast<App*>(user);
+  if (a->window == nullptr) return G_SOURCE_REMOVE;
+  const DisplayMode d = a->model->display_mode();
+  auto snap = a->model->event();
+  RowSet::Row& r = a->rows->row(0);
+  const plot::Range axis = r.plot != nullptr ? r.plot->x_range(r.axis_x) : plot::Range{0.0, 0.0};
+  std::fprintf(stderr,
+               "[disarm-probe] was_capturing=%d display=%s seq=%llu->%llu events=%llu->%llu "
+               "snap=[%.4f..%.4f] n=%llu axis=[%.4f..%.4f]\n",
+               int(a->dbg_capturing_at_disarm),
+               d == DisplayMode::Held       ? "event(frozen)"
+               : d == DisplayMode::Live     ? "live"
+               : d == DisplayMode::Capturing ? "live(capturing)"
+                                            : "nothing",
+               (unsigned long long)a->dbg_seq_at_disarm,
+               (unsigned long long)a->model->event_seq(),
+               (unsigned long long)a->dbg_latches_at_disarm,
+               (unsigned long long)a->model->trigger_latches().load(),
+               snap ? -snap->pre_s : 0.0, snap ? snap->post_s : 0.0,
+               (unsigned long long)(snap ? snap->stamps.size() : 0), double(axis.min),
+               double(axis.max));
+  std::fflush(stderr);
+  return G_SOURCE_REMOVE;
+}
+
+/// Waits for a capture to actually be open before clicking Disarm. The interesting case
+/// is the Disarm that lands *mid-capture*, and a fixed delay catches it only by luck --
+/// the capture opens as soon as the ring holds a pre window and closes one post window
+/// later. Polling for `trigger_pending()` makes the probe deterministic.
+gboolean disarm_probe_wait(gpointer user) {
+  App* a = static_cast<App*>(user);
+  if (a->window == nullptr) return G_SOURCE_REMOVE;
+  ++a->dbg_disarm_polls;
+  if (a->model->trigger_pending()) ++a->dbg_disarm_in_capture;
+  // 300 ms into the capture: post-trigger data worth keeping, well short of the latch.
+  // The 10 s cap is for a stream that never triggers -- the probe still has to report.
+  if (a->dbg_disarm_in_capture >= 6 || a->dbg_disarm_polls > 200) {
+    if (a->dbg_disarm_in_capture == 0) {
+      std::fprintf(stderr, "[disarm-probe] no capture opened in 10 s; disarming anyway\n");
+      std::fflush(stderr);
+    }
+    disarm_probe_step2(a);
+    return G_SOURCE_REMOVE;
+  }
+  return G_SOURCE_CONTINUE;
+}
+
 gboolean disarm_probe_step1(gpointer user) {
   App* a = static_cast<App*>(user);
   if (a->window == nullptr) return G_SOURCE_REMOVE;
   if (a->trigger_toggle != nullptr)
     gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(a->trigger_toggle), TRUE);
-  g_timeout_add(2000, disarm_probe_step2, a);
+  g_timeout_add(50, disarm_probe_wait, a);
   return G_SOURCE_REMOVE;
 }
 
@@ -2350,6 +2525,9 @@ int run_ui(int argc, char** argv, ScopeModel& model, Acquisition& acq, RowSet& r
   if (const char* env = g_getenv("UDPSCOPE_ARM_TRIGGER_MS")) {
     g_timeout_add(static_cast<guint>(std::atoi(env)), arm_probe, &app);
   }
+  if (const char* env = g_getenv("UDPSCOPE_PROBE_FIELDS_MS")) {
+    g_timeout_add(static_cast<guint>(std::atoi(env)), fields_probe, &app);
+  }
   if (const char* env = g_getenv("UDPSCOPE_TRIG_TRACE_MS")) {
     g_timeout_add(static_cast<guint>(std::atoi(env)), trig_trace_probe, &app);
   }