Correct downsampling and better layout

src/acquisition.cpp

@@ -319,6 +319,17 @@ void Acquisition::on_data(const udps_frame_t* frame) {
   BufferStore& store = model_.live();
   const u64 g = part.time_stride;
 
+  // The filter's groups are `g` long, so a re-plan has to start a fresh group rather than
+  // finish the one it was in with the wrong length.
+  if (acc_stride_ != g || acc_channels_ != channels.size()) {
+    acc_stride_ = g;
+    acc_channels_ = channels.size();
+    value_acc_.assign(channels.size(), 0.0);
+    value_acc_n_.assign(channels.size(), 0);
+    time_acc_ = 0.0;
+    time_acc_n_ = 0;
+  }
+
   // Calibration is read per frame; it only changes under the same lock.
   TriggerEngine& engine = engine_;
   const int trig_channel = tc.channel;
@@ -424,7 +435,16 @@ void Acquisition::on_data(const udps_frame_t* frame) {
     last_stamp_used = rel;
     last_stamp_ = rel;
 
-    if (tick % g == g - 1) store.time().push(static_cast<f32>(rel));
+    // The stored stamp is the *mean* of the group, not its last member: that is where the
+    // boxcar's group delay puts the averaged value, and stamping it at the end of the
+    // group would shift every trace half a stored sample late.
+    time_acc_ += rel;
+    ++time_acc_n_;
+    if (tick % g == g - 1) {
+      store.time().push(static_cast<f32>(time_acc_ / static_cast<double>(time_acc_n_)));
+      time_acc_ = 0.0;
+      time_acc_n_ = 0;
+    }
 
     // The latch belongs to this sample, but it cannot be published from inside the
     // channel loop: `publish_event()` clamps the slice to every value ring's
@@ -438,8 +458,6 @@ void Acquisition::on_data(const udps_frame_t* frame) {
     double latch_edge = 0.0;
 
     for (std::size_t i = 0; i < channels.size(); ++i) {
-      const u64 period = g * part.stride[i];
-      if (tick % period != period - 1) continue;
       const ChannelInfo& ch = channels[i];
       // A signal's columns are consecutive samples of the same channel: a scalar
       // (one element) carries one value per batched cycle, a block signal carries
@@ -449,8 +467,23 @@ void Acquisition::on_data(const udps_frame_t* frame) {
               ? udps_frame_value(frame, static_cast<std::uint32_t>(ch.signal), 0,
                                  std::min<std::uint32_t>(k, ch.elements - 1))
               : udps_frame_value(frame, static_cast<std::uint32_t>(ch.signal), k, 0);
-      const f32 value = static_cast<f32>(raw * ch.gain + ch.offset);
+      // Every sample is read and accumulated even when only one in `period` is stored:
+      // that is the whole point of the filter. Skipping the read -- which is what the
+      // old stride did -- is what made decimation alias.
+      value_acc_[i] += raw * ch.gain + ch.offset;
+      ++value_acc_n_[i];
+
+      const u64 period = g * part.stride[i];
+      if (tick % period != period - 1) continue;
+      const f32 value =
+          static_cast<f32>(value_acc_[i] / static_cast<double>(value_acc_n_[i]));
+      value_acc_[i] = 0.0;
+      value_acc_n_[i] = 0;
       store.values(i).push(value);
+      // The trigger sees the *stored* signal, not the wire signal. An edge found in a
+      // sample that is not in the buffer would be cut from data that does not contain it,
+      // and with the filter in front of it a spike above the stored Nyquist is exactly
+      // the thing that must not latch -- it is not in the record the user would get.
       if (static_cast<int>(i) == trig_channel) {
         if (engine.process(value, rel)) {
           latch_here = true;