DEV-896 Fix Live Data plot stutter: O(1) monotonic-X trace bounds, batched chart locking

[jyong15]

Summary

Fixes the stuttering/janky Live Data plots (worst at UHD) at their root causes in the jchart2d-based plotting layer. Diagnosis was verified against the actual jchart2d bytecode the build resolves (3.3.2 — note the libs/ folder carries a stale 3.2.2 jar):

• Trace2DLtd.addPointInternal() evicts the oldest ring-buffer point once full, and when the evicted point holds an extreme it runs a full linear rescan of the buffer (minXSearch() etc.). For time-series data X is monotonically increasing, so the evicted point always holds min-X → an O(buffer) scan on essentially every sample in steady state.
• That work happens inside ATrace2D.addPoint() while holding synchronized(chart) — the same monitor Chart2D.paintComponent() needs — so the data thread starves the Swing EDT, and the per-sample lock churn multiplies it.

Changes (ShimmerDriverPC)

New Trace2DLtdMonotonicX (extends Trace2DLtd) — overrides addPointInternal and minXSearch/maxXSearch. When the ring buffer is provably sorted ascending by X (tracked via the run of consecutive non-decreasing-X insertions ≥ live buffer size), min/max X are answered in O(1) from the buffer's oldest/youngest elements. Anything that breaks the guarantee — backward X (rewind/replay), NaN-X discontinuity markers, in-place point mutation (STATE_CHANGED), buffer resize — falls back to the exact superclass scan until a full buffer of monotonic samples restores it. Y-range maintenance is deliberately left to the superclass (already amortized O(1) for random Y). Swapped in at the three live-plot construction sites in BasicPlotManagerPC; existing (Trace2DLtd) casts and the downsampler's runtime setMaxSize() calls are unaffected.

Batched chart locking (BasicPlotManagerPC) — filterDataAndPlot's multi-trace loop now acquires the chart monitor once per sample instead of once per trace-point (Java monitors are reentrant, so the library's internal per-point lock becomes free), and a new addPointsToTrace(trace, x[], y[], from, to) adds FFT bins in bounded 256-point chunks per lock acquisition. The trace list is snapshotted before entering the chart monitor to preserve the app's existing mListofTraces → chart lock ordering (see review notes below).

Cross-repo note

Shimmer-Advance-API's ShimmerDriverAdvancedPC includes :ShimmerDriverPC from source in its settings.gradle, so Consensys picks this up with no changes on that side (verified compiling). ConsensysApps is not involved.

Testing

• gradle compileJava: ShimmerDriverPC and ShimmerDriverAdvancedPC (Shimmer-Advance-API, building against these sources) both pass.
• Standalone correctness harness driving Trace2DLtdMonotonicX vs stock Trace2DLtd through a headless Chart2D: monotonic streaming past eviction, setMaxSize shrink+grow mid-stream, rewind/replay, repeated equal X — 11,596 min/max checks, 0 mismatches.
• Needs real-hardware verification with live streaming (especially at UHD/4K): plots should scroll smoothly, UI should stay responsive while streaming, axis auto-scaling should look identical, and clearing/resizing traces mid-stream should behave as before.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JjgNkS6wtPkQUdK3KdHVdN

---

Generated by Claude Code

[jyong15]

Adversarial review summary (automated reviewer pass over this changeset, verified against decompiled jchart2d 3.3.2 bytecode)

Verified sound:

• The run ≥ size "provably sorted" invariant is airtight: a ring buffer holds exactly the last size() insertions, so after any out-of-order X the fast path stays off until a full buffer of fresh monotonic samples has evicted the offending point — the exact superclass scan covers the interim. setMaxSize shrink keeps only the youngest (sorted) elements; grow leaves elements unchanged; both hold the invariant.
• minXSearch/maxXSearch callers were enumerated (eviction, firePointChanged REMOVED/CHANGED, removePoint, setMaxSize) and the fast-path answer is correct or safely disabled in each context; equal-X ties are fine; tracking fields are only mutated under the chart+trace locks (plus conservative volatile resets).
• Lock ordering: the library exclusively takes chart→trace (checked addPoint, removeAllPoints, setMaxSize, property setters, firePointChanged); the new outer chart lock preserves that order.
• The wrapped loop is byte-identical logic (git diff -w clean); the FFT batch reproduces the original point order and start-bin offset exactly.

Critical finding, fixed before this PR was opened: the first version of the batching held the chart monitor while calling mListofTraces.size()/.get() (chart→list order), while clearAllDataBuffer() and the trace-resize path hold the mListofTraces monitor while calling chart-locking trace mutators (list→chart). A user clearing/resizing plots mid-stream could deadlock the EDT and data thread permanently. Fixed by snapshotting the trace list before entering the chart monitor.

Minor hardening also applied: NaN-X discontinuity markers now reset the ascending run (previously absorbed by the first-point guard); in-place point mutation (STATE_CHANGED) resets the run so the class remains a safe drop-in even for usage patterns the app doesn't currently have; tracking fields made volatile so a torn 64-bit write can never spuriously enable the fast path.

Notes, no action needed:

• The lock trade is churn→duration by design: the chart monitor is now held across one multi-trace pass (bounded by trace count) or one 256-point FFT chunk.
• filterDataAndPlotList and filterDataAndPlotBasic are secondary paths that still add points per-sample; same batching treatment could be applied later if those paths matter.

---

Generated by Claude Code

[Copilot]

🟡 Changes recommended

The deferred replay path can dereference mChart when it may be null (explicitly supported by the new chartMonitor fallback), risking a NullPointerException.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR targets Live Data plot stutter in the PC plotting layer (jchart2d) by avoiding per-sample O(n) X-bound rescans for monotonic time-series data and by reducing lock contention between the data thread and the Swing EDT.

Changes:

• Introduces Trace2DLtdMonotonicX, a Trace2DLtd subclass that provides O(1) min/max X when the ring buffer is provably monotonic in X.
• Switches live plot trace construction in BasicPlotManagerPC to use Trace2DLtdMonotonicX.
• Batches chart-monitor locking in filterDataAndPlot(...) and adds a chunked addPointsToTrace(...) helper for FFT bin plotting; defers Swing/console work until after releasing the chart lock.

File summaries

File	Description
ShimmerDriverPC/src/main/java/com/shimmerresearch/guiUtilities/plot/Trace2DLtdMonotonicX.java	Adds a monotonic-X optimized Trace2DLtd variant to eliminate redundant min/max X rescans.
ShimmerDriverPC/src/main/java/com/shimmerresearch/guiUtilities/plot/BasicPlotManagerPC.java	Adopts the new trace type and batches chart locking + defers I/O/Swing work to reduce EDT starvation.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[jyong15]

Adversarial review — round 2 (after merging master and addressing round-1 findings)

Branch was merged with master (b8fc2d8, clean), round-1 findings were addressed in a1c5e09, and an independent adversarial pass against the decompiled jchart2d 3.3.2 bytecode (the version ShimmerDriverPC/build.gradle resolves) produced the findings below, all addressed in f436ff0.

Verified sound

• Trace2DLtdMonotonicX is correct. Trace2DLtd.addPointInternal adds to the ring buffer before running minXSearch/maxXSearch, so the fast path's getOldest()/getYoungest() see post-add state. RingBufferArrayFast.size() is the live element count, so run >= size() compares against the right number. The invariant "the buffer holds the most recent size() insertions" survives every mutation: add evicts only the oldest, setBufferSize drops the oldest on shrink and preserves order on grow, Trace2DLtd.removePointInternal is literally return null, and removeAllPoints disables the fast path. The NaN-X reset lands exactly on the insertion that evicts the NaN marker.
• The batched lock is the right monitor in the right order. ATrace2D.addPoint locks m_renderer then this; every trace in mListofTraces is attached to mChart at insertion, so the outer chart lock makes the inner per-point locks reentrant. The pre-batch toArray takes the mListofTraces monitor before the chart monitor, matching the existing mListofTraces → chart order. Every call inside the chart monitor was enumerated: none reaches mListofTraces, Swing, or the AWT tree lock.
• HR deferral is behaviour-preserving: one call per matching trace, same order, same args, so the Consensys HR counter window and setPnlHR output are unchanged.
• No downstream breakage: nobody overrides printSignalProps; both PlotManagerPC overrides are still called; ConsensysApps' super.addPointToTrace(...) signature is untouched. The stale jchart2d-3.2.2 jar in Consensys_lib exposes the same protected members, so the subclass stays binary-compatible if that jar wins at runtime.

Findings (addressed in f436ff0)

1. Medium — deferred work was dropped when the loop threw. The "Trace does not exist" throw sat inside the chart monitor while the replay loops came after it, so on that path earlier traces' HR updates and console dumps were lost. Now the batch is wrapped so replay runs on every exit path and the exception is rethrown afterwards.
2. Medium — only the X axis was fixed, and batching lengthened the Y worst case. addPointInternal runs the identical min/maxYSearch rescan on eviction, and for monotone or drifting Y channels (battery, temperature, GSR baseline, counters) the evicted oldest point holds the Y extreme on essentially every sample. Holding the lock across all traces made the EDT's worst-case wait N_traces × O(buffer). The chart monitor is now released every TRACE_BATCH_MAX = 8 traces, and the javadoc no longer claims Y is cheap.
3. Low — the fast path skipped expand{Min,Max}XErrorBarBounds(). Now delegates wholly to super when any IErrorBarPolicy is registered (none is in these repos today).
4. Low — the getRenderer()==null staleness guard was dead code that would have silently swallowed jchart2d's IllegalStateException for a never-attached trace. Removed; only a null-check remains.
5. Low — deferred console output was regrouped by kind. Now a single ordered list replays in the original interleaving.
6. Low — the shared deferral buffers relied on mListofPropertiestoPlot being a stable monitor, but it is a public non-final field reassigned in AbstractPlotManager constructors. Buffers are now method-local.
7. Low — comment/hot-path mismatches corrected. No HR-visibility flag exists in the base class, so HR recording stays unconditional.

Merge + compile

The merged master range (Verisense sensors, Configuration/ShimmerObject, DEV-975 BT module versions, two build.gradle version bumps) is entirely disjoint from the two changed plotting files; the jchart2d dependency is unchanged. ShimmerDriverPC gradle compileJava exits 0 on f436ff0. ShimmerDriverAdvancedPC also compiled clean against SJAA master in this round; an unrelated SensorArduino.java failure seen once earlier did not reproduce.

Still needs the real-hardware UHD streaming check in the PR description, ideally including a drifting-Y channel such as battery to exercise item 2.

---

Generated by Claude Code

[Copilot]

🟡 Changes recommended

There is a confirmed thread-safety issue in the new trace snapshotting logic and a confirmed avoidable allocation/GC cost in the hot path that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces non-trivial concurrency and performance-path changes in the live plotting hot path that require careful human validation (especially under real streaming/EDT load).

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The change materially alters concurrency/locking behavior in a performance-critical Swing plotting path and should receive final human review plus real-hardware validation before approval.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It changes core live-plot concurrency/locking behavior and trace-bound computations in a UI-critical path and needs final human review plus real-hardware streaming validation to ensure there are no responsiveness or correctness regressions.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite